[PATCH] Extra information in autoload error

[PATCH] Extra information in autoload error

[Alex]

[-- Attachment #1: Type: text/plain, Size: 138 bytes --]

I felt that the error that `autoload-do-load` throws when it runs into a
failed autoload is lacking, so I made the following adjustment:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: using Vload_history --]
[-- Type: text/x-diff, Size: 530 bytes --]

diff --git a/src/eval.c b/src/eval.c
index e08a25a..3c619ba 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1987,8 +1987,9 @@ it defines a macro.  */)
       Lisp_Object fun = Findirect_function (funname, Qnil);
 
       if (!NILP (Fequal (fun, fundef)))
-	error ("Autoloading failed to define function %s",
-	       SDATA (SYMBOL_NAME (funname)));
+	error ("Autoloading failed to define function %s in file %s",
+	       SDATA (SYMBOL_NAME (funname)),
+	       SDATA (Fcar (Fcar (Vload_history))));
       else
 	return fun;
     }

[-- Attachment #3: Type: text/plain, Size: 172 bytes --]




This should give the file that was checked for the autoload. I tested it
out and it seems to work. In case Vload_history can't be used, I also
tried out the following:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: patch using funname --]
[-- Type: text/x-diff, Size: 529 bytes --]

diff --git a/src/eval.c b/src/eval.c
index e08a25a..8a573de 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1987,8 +1987,9 @@ it defines a macro.  */)
       Lisp_Object fun = Findirect_function (funname, Qnil);
 
       if (!NILP (Fequal (fun, fundef)))
-	error ("Autoloading failed to define function %s",
-	       SDATA (SYMBOL_NAME (funname)));
+	error ("Autoloading failed to define function %s with filename %s",
+	       SDATA (SYMBOL_NAME (funname)),
+	       SDATA (Fcar (Fcdr (fundef))));
       else
 	return fun;
     }

[-- Attachment #5: Type: text/plain, Size: 120 bytes --]




Would one of these be considered for inclusion into Emacs? I think both
error messages are more helpful than before.

Re: [PATCH] Extra information in autoload error

[Davis Herring]

> +	error ("Autoloading failed to define function %s in file %s",

I'd phrase it as "Autoloading file %s failed to define function %s". 
(It's a bit odd to say the function is in the file when we've just 
learned it isn't.)

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or 
too sparse, it is because mass-energy conversion has occurred during 
shipping.

Re: [PATCH] Extra information in autoload error

[Alex]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

Davis Herring <herring@lanl.gov> writes:

>> +	error ("Autoloading failed to define function %s in file %s",
>
> I'd phrase it as "Autoloading file %s failed to define function %s". (It's a bit
> odd to say the function is in the file when we've just learned it isn't.)
>
> Davis

Thanks, I agree that it's better. I suppose I wanted to append rather
than rewrite the message, but there's really no reason to hold back.

The updated patches follow:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vload_history --]
[-- Type: text/x-diff, Size: 479 bytes --]

diff --git a/src/eval.c b/src/eval.c
index e08a25a..4daf052 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1987,7 +1987,8 @@ it defines a macro.  */)
       Lisp_Object fun = Findirect_function (funname, Qnil);
 
       if (!NILP (Fequal (fun, fundef)))
-	error ("Autoloading failed to define function %s",
+	error ("Autoloading file %s failed to define function %s",
+	       SDATA (Fcar (Fcar (Vload_history))),
 	       SDATA (SYMBOL_NAME (funname)));
       else
 	return fun;

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: funname --]
[-- Type: text/x-diff, Size: 476 bytes --]

diff --git a/src/eval.c b/src/eval.c
index e08a25a..8c5c5e5 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1987,7 +1987,8 @@ it defines a macro.  */)
       Lisp_Object fun = Findirect_function (funname, Qnil);
 
       if (!NILP (Fequal (fun, fundef)))
-	error ("Autoloading failed to define function %s",
+	error ("Autoloading filename %s failed to define function %s",
+	       SDATA (Fcar (Fcdr (fundef))),
 	       SDATA (SYMBOL_NAME (funname)));
       else
 	return fun;

Re: [PATCH] Extra information in autoload error

[Alex]

Alex <agrambot@gmail.com> writes:

> Davis Herring <herring@lanl.gov> writes:
>
>>> +	error ("Autoloading failed to define function %s in file %s",
>>
>> I'd phrase it as "Autoloading file %s failed to define function %s". (It's a bit
>> odd to say the function is in the file when we've just learned it isn't.)
>>
>> Davis
>
> Thanks, I agree that it's better. I suppose I wanted to append rather
> than rewrite the message, but there's really no reason to hold back.
>
> The updated patches follow:
>
> diff --git a/src/eval.c b/src/eval.c
> index e08a25a..4daf052 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -1987,7 +1987,8 @@ it defines a macro.  */)
>        Lisp_Object fun = Findirect_function (funname, Qnil);
>  
>        if (!NILP (Fequal (fun, fundef)))
> -	error ("Autoloading failed to define function %s",
> +	error ("Autoloading file %s failed to define function %s",
> +	       SDATA (Fcar (Fcar (Vload_history))),
>  	       SDATA (SYMBOL_NAME (funname)));
>        else
>  	return fun;
>
> diff --git a/src/eval.c b/src/eval.c
> index e08a25a..8c5c5e5 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -1987,7 +1987,8 @@ it defines a macro.  */)
>        Lisp_Object fun = Findirect_function (funname, Qnil);
>  
>        if (!NILP (Fequal (fun, fundef)))
> -	error ("Autoloading failed to define function %s",
> +	error ("Autoloading filename %s failed to define function %s",
> +	       SDATA (Fcar (Fcdr (fundef))),
>  	       SDATA (SYMBOL_NAME (funname)));
>        else
>  	return fun;

Ping. Does anyone have an opinion on which of the two diffs is better?

Re: [PATCH] Extra information in autoload error

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Date: Sat, 22 Oct 2016 10:23:06 -0600
> Cc: emacs-devel@gnu.org
> 
> Alex <agrambot@gmail.com> writes:
> 
> > Davis Herring <herring@lanl.gov> writes:
> >
> >>> +	error ("Autoloading failed to define function %s in file %s",
> >>
> >> I'd phrase it as "Autoloading file %s failed to define function %s". (It's a bit
> >> odd to say the function is in the file when we've just learned it isn't.)
> >>
> >> Davis
> >
> > Thanks, I agree that it's better. I suppose I wanted to append rather
> > than rewrite the message, but there's really no reason to hold back.
> >
> > The updated patches follow:
> >
> > diff --git a/src/eval.c b/src/eval.c
> > index e08a25a..4daf052 100644
> > --- a/src/eval.c
> > +++ b/src/eval.c
> > @@ -1987,7 +1987,8 @@ it defines a macro.  */)
> >        Lisp_Object fun = Findirect_function (funname, Qnil);
> >  
> >        if (!NILP (Fequal (fun, fundef)))
> > -	error ("Autoloading failed to define function %s",
> > +	error ("Autoloading file %s failed to define function %s",
> > +	       SDATA (Fcar (Fcar (Vload_history))),
> >  	       SDATA (SYMBOL_NAME (funname)));
> >        else
> >  	return fun;
> >
> > diff --git a/src/eval.c b/src/eval.c
> > index e08a25a..8c5c5e5 100644
> > --- a/src/eval.c
> > +++ b/src/eval.c
> > @@ -1987,7 +1987,8 @@ it defines a macro.  */)
> >        Lisp_Object fun = Findirect_function (funname, Qnil);
> >  
> >        if (!NILP (Fequal (fun, fundef)))
> > -	error ("Autoloading failed to define function %s",
> > +	error ("Autoloading filename %s failed to define function %s",
> > +	       SDATA (Fcar (Fcdr (fundef))),
> >  	       SDATA (SYMBOL_NAME (funname)));
> >        else
> >  	return fun;
> 
> Ping. Does anyone have an opinion on which of the two diffs is better?

The second one, IMO.

Re: [PATCH] Extra information in autoload error

[Stefan Monnier]

>> if (!NILP (Fequal (fun, fundef)))
>> -	error ("Autoloading failed to define function %s",
>> +	error ("Autoloading file %s failed to define function %s",
>> +	       SDATA (Fcar (Fcar (Vload_history))),

If the autoloaded file had a `require' or a `load', I suspect that
Fcar (Fcar (Vload_history)) might return the wrong file name.

> Ping. Does anyone have an opinion on which of the two diffs is better?

I'd go with the second,


        Stefan

Re: [PATCH] Extra information in autoload error

[Alex]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> if (!NILP (Fequal (fun, fundef)))
>>> -	error ("Autoloading failed to define function %s",
>>> +	error ("Autoloading file %s failed to define function %s",
>>> +	       SDATA (Fcar (Fcar (Vload_history))),
>
> If the autoloaded file had a `require' or a `load', I suspect that
> Fcar (Fcar (Vload_history)) might return the wrong file name.
>

I tried testing this with nlinum-mode. In emacs -Q evaluate:

(progn
  (autoload 'nlinum-mode2 "~/.emacs.d/elpa/nlinum-1.6/nlinum")
  (nlinum-mode2))

If nlinum-mode is installed then this will error with:

(error "Autoloading failed to define function nlinum-mode2 in file
/home/alex/.emacs.d/elpa/nlinum-1.6/nlinum.elc")

I also replaced the (require 'linum) with (load "linum") and got the
same result.

So it seems to work. It seems that if there's a `require' or `load',
then those files get added to `load-history' before the initial file.

>> Ping. Does anyone have an opinion on which of the two diffs is better?
>
> I'd go with the second,
>
>
>         Stefan

Is it just because it's a "safer" option, or do you (and Eli) figure the
second error message would just be better?

Re: [PATCH] Extra information in autoload error

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 22 Oct 2016 14:23:34 -0600
> 
> (error "Autoloading failed to define function nlinum-mode2 in file
> /home/alex/.emacs.d/elpa/nlinum-1.6/nlinum.elc")
> 
> I also replaced the (require 'linum) with (load "linum") and got the
> same result.
> 
> So it seems to work. It seems that if there's a `require' or `load',
> then those files get added to `load-history' before the initial file.
> 
> >> Ping. Does anyone have an opinion on which of the two diffs is better?
> >
> > I'd go with the second,
> >
> >
> >         Stefan
> 
> Is it just because it's a "safer" option, or do you (and Eli) figure the
> second error message would just be better?

The second variant is IMO more accurate: it doesn't claim that the
missing autoload is in the named file.  In the case above it would say

  Autoloading file /home/alex/.emacs.d/elpa/nlinum-1.6/nlinum.elc
    failed to define function nlinum-mode2

which is more honest, since we don't know whether the
problematic/missing autoload is in the named file or in some file(s)
loaded/required by it, or some macro defined elsewhere altogether.

Re: [PATCH] Extra information in autoload error

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: emacs-devel@gnu.org, Eli Zaretskii <eliz@gnu.org>
>> Date: Sat, 22 Oct 2016 14:23:34 -0600
>> 
>> (error "Autoloading failed to define function nlinum-mode2 in file
>> /home/alex/.emacs.d/elpa/nlinum-1.6/nlinum.elc")
>> 
>> I also replaced the (require 'linum) with (load "linum") and got the
>> same result.
>> 
>> So it seems to work. It seems that if there's a `require' or `load',
>> then those files get added to `load-history' before the initial file.
>> 
>> >> Ping. Does anyone have an opinion on which of the two diffs is better?
>> >
>> > I'd go with the second,
>> >
>> >
>> >         Stefan
>> 
>> Is it just because it's a "safer" option, or do you (and Eli) figure the
>> second error message would just be better?
>
> The second variant is IMO more accurate: it doesn't claim that the
> missing autoload is in the named file.  In the case above it would say
>
>   Autoloading file /home/alex/.emacs.d/elpa/nlinum-1.6/nlinum.elc
>     failed to define function nlinum-mode2
>
> which is more honest, since we don't know whether the
> problematic/missing autoload is in the named file or in some file(s)
> loaded/required by it, or some macro defined elsewhere altogether.

Sorry, I accidentally posted the older error message instead of the new
one. I was wondering if you preferred the version that used

 SDATA (Fcar (Fcar (Vload_history)))

or

 SDATA (Fcar (Fcdr (fundef)))

to get the extra information. The first one gets the file path, while
the second one just gets what was in the autoload object.

Re: [PATCH] Extra information in autoload error

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sun, 23 Oct 2016 15:24:01 -0600
> 
> I was wondering if you preferred the version that used
> 
>  SDATA (Fcar (Fcar (Vload_history)))
> 
> or
> 
>  SDATA (Fcar (Fcdr (fundef)))
> 
> to get the extra information. The first one gets the file path, while
> the second one just gets what was in the autoload object.

Don't we want both?  Can you show the textual description of the
"autoload object" that is returned by the latter form?  (Sorry, too
busy with more urgent issues to try that myself.)

Re: [PATCH] Extra information in autoload error

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Sun, 23 Oct 2016 15:24:01 -0600
>> 
>> I was wondering if you preferred the version that used
>> 
>>  SDATA (Fcar (Fcar (Vload_history)))
>> 
>> or
>> 
>>  SDATA (Fcar (Fcdr (fundef)))
>> 
>> to get the extra information. The first one gets the file path, while
>> the second one just gets what was in the autoload object.
>
> Don't we want both?

That would display more information, but it would also make the error
message more verbose.

> Can you show the textual description of the
> "autoload object" that is returned by the latter form?

Here, fundef is the autoload object. That is, it's a list of the form:

  (autoload filename docstring interactive type)

So SDATA (Fcar (Fcdr (fundef))) just gets the filename of the autoload
object. The filename of the autoload object is just the 2nd argument of
the `autoload' function.

So the Vload_history version returns the whole path, while the fundef
version just uses whatever was passed into autoload. Usually that is
just the filename sans extension, e.g.:

(symbol-function 'linum-mode) => (autoload "linum" 1174895 t nil)

> (Sorry, too busy with more urgent issues to try that myself.)

No worries.

Re: [PATCH] Extra information in autoload error

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Mon, 24 Oct 2016 13:21:08 -0600
> 
> Here, fundef is the autoload object. That is, it's a list of the form:
> 
>   (autoload filename docstring interactive type)
> 
> So SDATA (Fcar (Fcdr (fundef))) just gets the filename of the autoload
> object. The filename of the autoload object is just the 2nd argument of
> the `autoload' function.
> 
> So the Vload_history version returns the whole path, while the fundef
> version just uses whatever was passed into autoload. Usually that is
> just the filename sans extension, e.g.:
> 
> (symbol-function 'linum-mode) => (autoload "linum" 1174895 t nil)

I think the file name alone will be enough.  Having a full absolute
file name would be nicer, though.  I see no need for anything else:
just the file name and the feature that was supposed to be autoloaded.

Thanks.

Re: [PATCH] Extra information in autoload error

[Alex]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Mon, 24 Oct 2016 13:21:08 -0600
>> 
>> Here, fundef is the autoload object. That is, it's a list of the form:
>> 
>>   (autoload filename docstring interactive type)
>> 
>> So SDATA (Fcar (Fcdr (fundef))) just gets the filename of the autoload
>> object. The filename of the autoload object is just the 2nd argument of
>> the `autoload' function.
>> 
>> So the Vload_history version returns the whole path, while the fundef
>> version just uses whatever was passed into autoload. Usually that is
>> just the filename sans extension, e.g.:
>> 
>> (symbol-function 'linum-mode) => (autoload "linum" 1174895 t nil)
>
> I think the file name alone will be enough.  Having a full absolute
> file name would be nicer, though.  I see no need for anything else:
> just the file name and the feature that was supposed to be autoloaded.
>
> Thanks.

Alright, I've attached a patch that includes both the absolute file name
and feature. If it's agreeable, then would you apply it for me?

Just to be clear, the autoload (autoload 'linum2 "linum") with this
patch will produce something like:

(error "Autoloading file /home/alex/emacs/lisp/linum.elc failed to
define function linum2")


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 856 bytes --]

From e8e8544b1805d474921974faa92d040699074878 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Mon, 24 Oct 2016 23:20:54 -0600
Subject: [PATCH] Add extra information in autoload error

* src/eval.c (Fautoload_do_load): Include the absolute file name in the
  error.
---
 src/eval.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/eval.c b/src/eval.c
index a9bad24..caeb791 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1988,7 +1988,8 @@ it defines a macro.  */)
       Lisp_Object fun = Findirect_function (funname, Qnil);
 
       if (!NILP (Fequal (fun, fundef)))
-	error ("Autoloading failed to define function %s",
+	error ("Autoloading file %s failed to define function %s",
+	       SDATA (Fcar (Fcar (Vload_history))),
 	       SDATA (SYMBOL_NAME (funname)));
       else
 	return fun;
-- 
2.9.3

Re: [PATCH] Extra information in autoload error

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Mon, 24 Oct 2016 23:47:57 -0600
> 
> Alright, I've attached a patch that includes both the absolute file name
> and feature. If it's agreeable, then would you apply it for me?
> 
> Just to be clear, the autoload (autoload 'linum2 "linum") with this
> patch will produce something like:
> 
> (error "Autoloading file /home/alex/emacs/lisp/linum.elc failed to
> define function linum2")

Thanks, pushed to master.