bug#26837: Wrong file in "autoloading failed to define" error

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Package: emacs
Version: 26.0.50
Severity: minor

cat <<EOF > bar.el
(provide 'bar)
EOF

emacs -Q -L $PWD
(autoload 'bar "bar")
(bar)

 -> (error "Autoloading file /path/to/bar.el failed to define function bar")

which is correct and good.

Now:
C-]   ; exit debugger
(bar) ; repeat

 -> (error "Autoloading file /path/to/help-mode.elc failed to define function bar") 

Ie, the wrong file name is reported after the first time round.


Ref: http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00668.html

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Am I mistaken in the following patch to fix this, or has load not been
moving an already present file to the start of load-history since c 2005?

--- a/src/lread.c
+++ b/src/lread.c
@@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
       /* On the first cycle, we can easily test here
 	 whether we are reading the whole buffer.  */
       if (b && first_sexp)
-	whole_buffer = (PT == BEG && ZV == Z);
+	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
 
       instream = stream;
     read_next:

bug#26837: Wrong file in "autoloading failed to define" error

[Alex]

Glenn Morris <rgm@gnu.org> writes:

> Am I mistaken in the following patch to fix this, or has load not been
> moving an already present file to the start of load-history since c 2005?
>
> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>  
>        instream = stream;
>      read_next:

I can't claim to know what is going on within that function, but your
patch seems to only work for some files (e.g. your bar.el). If bar
starts with a comment, then bar isn't moved to the top after evaluating
(load "bar").

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Alex wrote:

>> -	whole_buffer = (PT == BEG && ZV == Z);
>> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
[...]
> I can't claim to know what is going on within that function, but your
> patch seems to only work for some files (e.g. your bar.el). If bar
> starts with a comment, then bar isn't moved to the top after evaluating
> (load "bar").

The intent of the code seems to be to only adjust an existing
load-history element for some file if the entire file is being loaded.
I guess in the leading comment case, something else has already moved
point looking for a significant leading comment (eg lexbind, script,
prop-line). Hmmm.

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Glenn Morris wrote:

>> I can't claim to know what is going on within that function, but your
>> patch seems to only work for some files (e.g. your bar.el). If bar
>> starts with a comment, then bar isn't moved to the top after evaluating
>> (load "bar").
>
> The intent of the code seems to be to only adjust an existing
> load-history element for some file if the entire file is being loaded.
> I guess in the leading comment case, something else has already moved
> point looking for a significant leading comment (eg lexbind, script,
> prop-line). Hmmm.

I think it was the lexical-binding thing. Following seems to work:

--- i/src/lread.c
+++ w/src/lread.c
@@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
       /* On the first cycle, we can easily test here
 	 whether we are reading the whole buffer.  */
       if (b && first_sexp)
-	whole_buffer = (PT == BEG && ZV == Z);
+	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
 
       instream = stream;
     read_next:
@@ -2008,6 +2008,7 @@ BUFFER is the buffer to evaluate (nil means use current buffer),
   record_unwind_protect (save_excursion_restore, save_excursion_save ());
   BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
   specbind (Qlexical_binding, lisp_file_lexically_bound_p (buf) ? Qt : Qnil);
+  BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
   readevalloop (buf, 0, filename,
 		!NILP (printflag), unibyte, Qnil, Qnil, Qnil);
   unbind_to (count, Qnil);

bug#26837: Wrong file in "autoloading failed to define" error

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 09 May 2017 14:39:24 -0400
> Cc: 26837@debbugs.gnu.org
> 
> I think it was the lexical-binding thing. Following seems to work:
> 
> --- i/src/lread.c
> +++ w/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));

You are saying that set_buffer_internal didn't do its job?  Of that
the call was bypassed?

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Eli Zaretskii wrote:

>> -	whole_buffer = (PT == BEG && ZV == Z);
>> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>
> You are saying that set_buffer_internal didn't do its job?  Of that
> the call was bypassed?

When START = nil (eg when called from eval-buffer), set_buffer_internal
is never called.

bug#26837: Wrong file in "autoloading failed to define" error

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: agrambot@gmail.com,  26837@debbugs.gnu.org
> Date: Tue, 09 May 2017 19:49:17 -0400
> 
> Eli Zaretskii wrote:
> 
> >> -	whole_buffer = (PT == BEG && ZV == Z);
> >> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
> >
> > You are saying that set_buffer_internal didn't do its job?  Of that
> > the call was bypassed?
> 
> When START = nil (eg when called from eval-buffer), set_buffer_internal
> is never called.

What's the use case when this goes together with b not being the
current buffer?

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Eli Zaretskii wrote:

> What's the use case when this goes together with b not being the
> current buffer?

(load "file.el")

-> load-source-file-function
-> load-with-code-conversion
-> (eval-buffer buffer ...)

bug#26837: Wrong file in "autoloading failed to define" error

[Alex]

Glenn Morris <rgm@gnu.org> writes:

> Glenn Morris wrote:
>
> I think it was the lexical-binding thing. Following seems to work:
>
> --- i/src/lread.c
> +++ w/src/lread.c
> @@ -1885,7 +1885,7 @@ PREDICATE can also be an integer to pass to the faccessat(2) function,
>        /* On the first cycle, we can easily test here
>  	 whether we are reading the whole buffer.  */
>        if (b && first_sexp)
> -	whole_buffer = (PT == BEG && ZV == Z);
> +	whole_buffer = (BUF_PT (b) == BUF_BEG (b) && BUF_ZV (b) == BUF_Z (b));
>  
>        instream = stream;
>      read_next:
> @@ -2008,6 +2008,7 @@ BUFFER is the buffer to evaluate (nil means use current buffer),
>    record_unwind_protect (save_excursion_restore, save_excursion_save ());
>    BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
>    specbind (Qlexical_binding, lisp_file_lexically_bound_p (buf) ? Qt : Qnil);
> +  BUF_TEMP_SET_PT (XBUFFER (buf), BUF_BEGV (XBUFFER (buf)));
>    readevalloop (buf, 0, filename,
>  		!NILP (printflag), unibyte, Qnil, Qnil, Qnil);
>    unbind_to (count, Qnil);

That seems to work for me as well. Should it be documented that
reevaluations of a file update the position of the file in load-history?
I assumed this was already the case when making my autoload patch
because of the "history" in load-history, but documentation would help
to make that clear.

PS: I realized that I was a bit careless in using SDATA without checking
that the argument is necessarily a string. I don't think it matters much
if your above patch is applied, but it's probably not a good idea to
leave it in, just in case there's a bug somewhere else in the future.

What do you think about the following diff? This removes the dependency
on load-history altogether:

--- a/src/eval.c
+++ b/src/eval.c
@@ -2021,7 +2021,10 @@ it defines a macro.  */)
 
       if (!NILP (Fequal (fun, fundef)))
        error ("Autoloading file %s failed to define function %s",
-              SDATA (Fcar (Fcar (Vload_history))),
+              SDATA (Flocate_file_internal (Fcar (Fcdr (fundef)),
+                                            Vload_path,
+                                            Vload_suffixes,
+                                            Qnil)),
               SDATA (SYMBOL_NAME (funname)));
       else
        return fun;

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Alex wrote:

> That seems to work for me as well. Should it be documented that
> reevaluations of a file update the position of the file in load-history?

I think it's mentioned in the doc of load:

  Loading a file records its definitions, and its `provide' and
  `require' calls, in an element of `load-history' whose
  car is the file name loaded.  See `load-history'.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
but that did seem to be the only mention I found in a quick search.

> PS: I realized that I was a bit careless in using SDATA without checking
> that the argument is necessarily a string. I don't think it matters much
> if your above patch is applied, but it's probably not a good idea to
> leave it in, just in case there's a bug somewhere else in the future.

In 58326f0 I check if the car is nil (though I don't see how it could be).
Could add the same thing to eval.c.

> What do you think about the following diff? This removes the dependency
> on load-history altogether:
>
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -2021,7 +2021,10 @@ it defines a macro.  */)
>  
>        if (!NILP (Fequal (fun, fundef)))
>         error ("Autoloading file %s failed to define function %s",
> -              SDATA (Fcar (Fcar (Vload_history))),
> +              SDATA (Flocate_file_internal (Fcar (Fcdr (fundef)),
> +                                            Vload_path,
> +                                            Vload_suffixes,
> +                                            Qnil)),
>                SDATA (SYMBOL_NAME (funname)));
>        else
>         return fun;

I don't have an informed opinion right now.

bug#26837: Wrong file in "autoloading failed to define" error

[Glenn Morris]

Glenn Morris wrote:

> I think it's mentioned in the doc of load:
>
>   Loading a file records its definitions, and its `provide' and
>   `require' calls, in an element of `load-history' whose
>   car is the file name loaded.  See `load-history'.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Duh, re-reading that I see it doesn't say what I thought it said.
So I guess it isn't documented, though it was clearly the intent of the
code. I don't know if it should be mentioned explicitly.

bug#26837: Wrong file in "autoloading failed to define" error

[Alex]

Glenn Morris <rgm@gnu.org> writes:

> In 58326f0 I check if the car is nil (though I don't see how it could be).
> Could add the same thing to eval.c.

According to the docstring of load-history:

 As an exception, one of the alist elements may have FILE-NAME nil,
 for symbols and features not associated with any file.

With your proposed patch I don't think it's really an issue anymore, but
previously it could have triggered a segfault under that rare condition.

> I don't know if it should be mentioned explicitly.

If the intention is that someone can expect the position of files in
load-history to be updated so that it provides a history of some sort
(i.e. use (caar load-history) to get the last file evaluated), then I
would think that there should be a line mentioning it somewhere.