bug#66912: With `require', the byte compiler reports the wrong file for errors.

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Emacs.

When byte compiling file1, and file1 requires file2, should there be an
error in file2, it is reported as being in file1, at the requires line.
This is entirely unhelpful; the reported position should be that of the
error in file2.  Things get even more unhelpful if there is a nesting of
required files, and the error occurs in a deeply nested file.

For an example, create the files ~/test-byte-compile-errors.el:

#########################################################################
;; -*- lexical-binding:t -*-
(require 'test-byte-compile-errors-2 "~/test-byte-compile-errors-2") 
#########################################################################

, and ~/test-byte-compile-errors-2.el:

#########################################################################
;; -*- lexical-binding:t -*-
(defvar foo nil)
(defun bar ()
  (setq foo)) ; <==================
(provide 'test-byte-compile-errors-2)
#########################################################################

..  From an Emacs session, do

    M-x byte-compile-file RET ~/test-byte-compile-errors.el RET

..  This will report the error as

Compiling file /home/acm/test-byte-compile-errors.el at Fri Nov  3 10:14:40 2023
test-byte-compile-errors.el:2:2: Error: Wrong number of arguments: setq, 1

..  This is not the error location.

#########################################################################

Preliminary analysis:

The pertinent error information is discarded by one of two
condition-cases in the macro displaying-byte-compile-warnings in
emacs-lisp/bytecomp.el.

If these condition-case's are disabled (for example by spiking the
enclosing `if' forms) and the necessary defuns recompiled, there instead
appears an error message in the display area.  On setting debug-on-error
to t and repeating the compilation, one gets a backtrace, which whilst
not ideal, is considerably more helpful than the original error message.

This appears to be a fundamental problem with condition-case.  When an
error occurs, the stack gets unwound before the error handlers have a
chance to analyse it.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The pertinent error information is discarded by one of two
> condition-cases in the macro displaying-byte-compile-warnings in
> emacs-lisp/bytecomp.el.

Kind of, yes.  But the info available there is hard to use: basically at
that point we could get the error message and the backtrace, which is
what we get when we set `byte-compile-debug`.  In the case where
`byte-compile-debug` is not set, we don't really want to display
a backtrace, so we'd have to "analyze" that backtrace to try and extract
a useful error message from it, which is hard&brittle.

> If these condition-case's are disabled (for example by spiking the
> enclosing `if' forms)

By "spiking" do you mean setting `byte-compile-debug`?

> This appears to be a fundamental problem with condition-case.  When an
> error occurs, the stack gets unwound before the error handlers have a
> chance to analyse it.

[ This is going a bit on a tangent, but I think it would be good to add
  some support for something like Common-Lisp's `handler-bind`,
  i.e. error handlers that are run before unwinding the stack (so they
  can capture the stack trace as well as the value of dynbound vars at
  the moment the error occurs, for example).

  It would make it possible for ERT to refrain from activating
  `debug-on-error`, for example (which it does in order to capture the
  backtrace at the time of the error).  ]

I think for this specific problem being discussed (which is indeed
a fairly common occurrence in my experience), the better solution is to
change `load` so it adds the "context" (i.e. filename and ideally also
the approximate file position info) to errors.
[ This may require something like `handler-bind`.  ]


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Fri, Nov 03, 2023 at 12:09:03 -0400, Stefan Monnier wrote:
> > The pertinent error information is discarded by one of two
> > condition-cases in the macro displaying-byte-compile-warnings in
> > emacs-lisp/bytecomp.el.

> Kind of, yes.  But the info available there is hard to use: basically at
> that point we could get the error message and the backtrace, which is
> what we get when we set `byte-compile-debug`.  In the case where
> `byte-compile-debug` is not set, we don't really want to display
> a backtrace, ....

Why not?  We're not in the compilation any more, we're loading a file.
Some error has prevented that file loading, so we want a backtrace just
as we would get with M-x load-file foo.elc RET.

> .... so we'd have to "analyze" that backtrace to try and extract a
> useful error message from it, which is hard&brittle.

Indeed, so.

> > If these condition-case's are disabled (for example by spiking the
> > enclosing `if' forms)

> By "spiking" do you mean setting `byte-compile-debug`?

I actually commented out byte-compile-debug, replacing it with t.  But
same idea, yes.

> > This appears to be a fundamental problem with condition-case.  When an
> > error occurs, the stack gets unwound before the error handlers have a
> > chance to analyse it.

> [ This is going a bit on a tangent, but I think it would be good to add
>   some support for something like Common-Lisp's `handler-bind`,
>   i.e. error handlers that are run before unwinding the stack (so they
>   can capture the stack trace as well as the value of dynbound vars at
>   the moment the error occurs, for example).

>   It would make it possible for ERT to refrain from activating
>   `debug-on-error`, for example (which it does in order to capture the
>   backtrace at the time of the error).  ]

That sounds kind of useful.

> I think for this specific problem being discussed (which is indeed
> a fairly common occurrence in my experience), the better solution is to
> change `load` so it adds the "context" (i.e. filename and ideally also
> the approximate file position info) to errors.
> [ This may require something like `handler-bind`.  ]

Another solution would be to dispense with
display-byte-compile-warnings, just letting compiler errors generate
backtraces.

The problem here is that there is no distinction in bytecomp.el between
"external" errors (such as from require) and errors detected by the
compiler in the source file being compiled.  The first decidedly want a
backtrace, the second probably not.  All these errors are handled as
though they were "internal" errors detected by the compiler.  This is
suboptimal.

Perhaps we should report the second type of error (detected by the
compiler) by calling a warning function, as we do for warnings, and
removing the damaging condition-case's as suggested two paragraphs back.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Why not?  We're not in the compilation any more, we're loading a file.
> Some error has prevented that file loading, so we want a backtrace just
> as we would get with M-x load-file foo.elc RET.

Hmm... that's a good point.

>> I think for this specific problem being discussed (which is indeed
>> a fairly common occurrence in my experience), the better solution is to
>> change `load` so it adds the "context" (i.e. filename and ideally also
>> the approximate file position info) to errors.
>> [ This may require something like `handler-bind`.  ]
> Another solution would be to dispense with
> display-byte-compile-warnings, just letting compiler errors generate
> backtraces.
>
> The problem here is that there is no distinction in bytecomp.el between
> "external" errors (such as from require) and errors detected by the
> compiler in the source file being compiled.

These are two fairly "clear" cases, admittedly.

But there are also cases in between where it's less clear, mostly with
errors during macro-expansion where the internal/external distinction is
not always that clear since some macros come from outside but others
come from the very file we're compiling, and where we can't easily tell
if an error is due to a bug in the macro definition or a bug in the use
of the macro.

> The first decidedly want a backtrace, the second probably not.
> All these errors are handled as though they were "internal" errors
> detected by the compiler.  This is suboptimal.

Also there are 2 questions:

- whether to give a backtrace (and/or enter the debugger).
- when we don't show a backtrace, what info do we put in the error message.

For the first, the current "solution" is to set `byte-compile-debug`.
It's not ideal, and we should improve it, but at least we do have
a solution for it.

For the second we currently don't show a good enough info and in my
previous response I focused on that part.

> Perhaps we should report the second type of error (detected by the
> compiler) by calling a warning function, as we do for warnings, and
> removing the damaging condition-case's as suggested two paragraphs back.

If the user is not asking to see backtraces, the current treatment seems
cleaner than without any `condition-case`.  So maybe those
`condition-case` should be turned into `condition-case-unless-debug`?


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Nov 12, 2023 at 12:28:13 -0500, Stefan Monnier wrote:
> > Why not?  We're not in the compilation any more, we're loading a file.
> > Some error has prevented that file loading, so we want a backtrace just
> > as we would get with M-x load-file foo.elc RET.

> Hmm... that's a good point.

> >> I think for this specific problem being discussed (which is indeed
> >> a fairly common occurrence in my experience), the better solution is to
> >> change `load` so it adds the "context" (i.e. filename and ideally also
> >> the approximate file position info) to errors.
> >> [ This may require something like `handler-bind`.  ]
> > Another solution would be to dispense with
> > display-byte-compile-warnings, just letting compiler errors generate
> > backtraces.

> > The problem here is that there is no distinction in bytecomp.el between
> > "external" errors (such as from require) and errors detected by the
> > compiler in the source file being compiled.

> These are two fairly "clear" cases, admittedly.

> But there are also cases in between where it's less clear, mostly with
> errors during macro-expansion where the internal/external distinction is
> not always that clear since some macros come from outside but others
> come from the very file we're compiling, and where we can't easily tell
> if an error is due to a bug in the macro definition or a bug in the use
> of the macro.

Question: will the user be able to identify the macro and its source
file if we just print the bare error message as enforced by
displaying-byte-compile-warnings?  It the answer is no or not really, we
should give her the backtrace to get started on.

> > The first decidedly want a backtrace, the second probably not.
> > All these errors are handled as though they were "internal" errors
> > detected by the compiler.  This is suboptimal.

> Also there are 2 questions:

> - whether to give a backtrace (and/or enter the debugger).
> - when we don't show a backtrace, what info do we put in the error message.

> For the first, the current "solution" is to set `byte-compile-debug`.
> It's not ideal, and we should improve it, but at least we do have
> a solution for it.

I suspect byte-compile-debug isn't widely known.  Its name is also a bit
discordant - it's not necessarily about debugging byte-compile, it's
just to get sensible error messages when something goes wrong,
especially when that something is not part of the byte compiler.

> For the second we currently don't show a good enough info and in my
> previous response I focused on that part.

Indeed, for the error message which provoked this bug report, the
current information is poor indeed.  Considering that require's can be
nested, we only tell the user the identity of the outermost one.

> > Perhaps we should report the second type of error (detected by the
> > compiler) by calling a warning function, as we do for warnings, and
> > removing the damaging condition-case's as suggested two paragraphs back.

> If the user is not asking to see backtraces, the current treatment seems
> cleaner than without any `condition-case`.

It's "neat and tidy", but at the cost of discarding all useful
information.  There are other common situations in Emacs where
the debugger is entered, or a backtrace output without debug-on-error
having to be set.  Perhaps this one should join them.

> So maybe those `condition-case` should be turned into
> `condition-case-unless-debug`?

I think this would be a very useful first step.  I think it likely a
user will set debug-on-error on encountering any unhelpful error
message.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> But there are also cases in between where it's less clear, mostly with
>> errors during macro-expansion where the internal/external distinction is
>> not always that clear since some macros come from outside but others
>> come from the very file we're compiling, and where we can't easily tell
>> if an error is due to a bug in the macro definition or a bug in the use
>> of the macro.
>
> Question: will the user be able to identify the macro and its source
> file if we just print the bare error message as enforced by
> displaying-byte-compile-warnings?

I think we print a "bare" error message (together with the location of
the macro call).  Often it's good enough (e.g. when the error is really
in the macro call itself).  Sometimes it's very perplexing :-(

> It the answer is no or not really, we should give her the backtrace to
> get started on.

Says the one who claimed earlier that backtraces are stressful :-)

Dumping the backtrace is a kind of cop-out.

Don't get me wrong, I love backtraces, but I don't think we should
blissfully throw backtraces at unsuspecting users (unlike Python, say).
IOW, we should first work harder to provide better error messages.

>> For the first, the current "solution" is to set `byte-compile-debug`.
>> It's not ideal, and we should improve it, but at least we do have
>> a solution for it.
>
> I suspect byte-compile-debug isn't widely known.  Its name is also a bit
> discordant - it's not necessarily about debugging byte-compile, it's
> just to get sensible error messages when something goes wrong,
> especially when that something is not part of the byte compiler.

Agreed.

>> For the second we currently don't show a good enough info and in my
>> previous response I focused on that part.
> Indeed, for the error message which provoked this bug report, the
> current information is poor indeed.  Considering that require's can be
> nested, we only tell the user the identity of the outermost one.

We don't even give that info.  We just give the line number of the
`require`.  It's almost as good as the outermost file name, but not quite.

> It's "neat and tidy", but at the cost of discarding all useful
> information.  There are other common situations in Emacs where
> the debugger is entered, or a backtrace output without debug-on-error
> having to be set.

Hmm... I can't think of such a situation.  When/where do we show
a backtrace without the user's explicit request?

>> So maybe those `condition-case` should be turned into
>> `condition-case-unless-debug`?
> I think this would be a very useful first step.  I think it likely a
> user will set debug-on-error on encountering any unhelpful error
> message.

AFAICT this would basically be equivalent to aliasing
`byte-compile-debug` to `debug-on-error`.

It may turn out to be annoying occasionally, but I think it's worth
a try (I've never found it useful to have `byte-compile-debug` set to
t without also setting `debug-on-error` to t).


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Drew Adams]

> Dumping the backtrace is a kind of cop-out.
> 
> Don't get me wrong, I love backtraces, but I don't think we should
> blissfully throw backtraces at unsuspecting users (unlike Python, say).
> IOW, we should first work harder to provide better error messages.

OT, so ... sorry.

It just occurred to me that instead of just having
a Boolean `debug-on-error', which turns use of the
debugger on/off for an error, there might be a
third possibility: show an error message and let
you then decide whether to open the backtrace --
maybe click (or `RET') the error msg, or in some
other way make the choice.

IOW, we could perhaps prepare a backtrace buffer
without actually entering its recursive edit etc.
If a user doesn't ask to see/use it then it just
sits there (buried) as an unused buffer.

Or display of the error msg could allow for (1)
activating/entering the backtrace buffer, (2)
leaving it buried, or (3) deleting it.

No idea about implementation or reasonableness.
Just something that occurred to me.  Yes, first
priority should be a good error msg.  Yes, users
should be able to optionally open the debugger.
But could we maybe  give them that possibility
after showing the error msg?

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

It's been almost a year since this bug got lost in the storm of other
things which needed doing.  I'd like to come back to it now.  To refresh
your memory, the problem was about not getting any information about foo
when an error occurred during (require 'foo).

And no apologies for the top posting, which just seems appropriate,
since new possibilities (handler-bind in particular, thanks!) have
appeared in the last year.

My latest idea is to put (the C equivalent of) handler-bind around the
readevalloop call near the end of Fload.  So with my test files from last
year, in place of the current error output 

    test-byte-compile-errors.el:2:11: Error: Wrong type argument: listp, baz

the following line would appear:

    test-byte-compile-errors.el:2:11: While loading "test-byte-compile-errors-2.el" Error: Wrong type argument: listp, baz

..  The "While loading "foo.el" " bit would be repeated as needed for
nested (require 'foo)s.  OK, the error line could be quite long, but the
information would at least be there.

This could surely be achieved with something like the following in Fload:

    (handler-bind ((lambda (err)
                    (signal (car err)
		            (combine-error-info "While loading " file
			                         (cdr err)))))
		  readevalloop (Qget_file_char, &input, hist_file_name,
		      0, Qnil, Qnil, Qnil, Qnil);)

(where, obviously, the details need to be worked out).  It would need
augmenting with handling for (eq debug-on-error t), and probably a few
other things, too.

What do you think of the idea?

-- 
Alan Mackenzie (Nuremberg, Germany).


On Sun, Nov 12, 2023 at 16:19:36 -0500, Stefan Monnier wrote:
> >> But there are also cases in between where it's less clear, mostly with
> >> errors during macro-expansion where the internal/external distinction is
> >> not always that clear since some macros come from outside but others
> >> come from the very file we're compiling, and where we can't easily tell
> >> if an error is due to a bug in the macro definition or a bug in the use
> >> of the macro.

> > Question: will the user be able to identify the macro and its source
> > file if we just print the bare error message as enforced by
> > displaying-byte-compile-warnings?

> I think we print a "bare" error message (together with the location of
> the macro call).  Often it's good enough (e.g. when the error is really
> in the macro call itself).  Sometimes it's very perplexing :-(

> > It the answer is no or not really, we should give her the backtrace to
> > get started on.

> Says the one who claimed earlier that backtraces are stressful :-)

> Dumping the backtrace is a kind of cop-out.

> Don't get me wrong, I love backtraces, but I don't think we should
> blissfully throw backtraces at unsuspecting users (unlike Python, say).
> IOW, we should first work harder to provide better error messages.

> >> For the first, the current "solution" is to set `byte-compile-debug`.
> >> It's not ideal, and we should improve it, but at least we do have
> >> a solution for it.

> > I suspect byte-compile-debug isn't widely known.  Its name is also a bit
> > discordant - it's not necessarily about debugging byte-compile, it's
> > just to get sensible error messages when something goes wrong,
> > especially when that something is not part of the byte compiler.

> Agreed.

> >> For the second we currently don't show a good enough info and in my
> >> previous response I focused on that part.
> > Indeed, for the error message which provoked this bug report, the
> > current information is poor indeed.  Considering that require's can be
> > nested, we only tell the user the identity of the outermost one.

> We don't even give that info.  We just give the line number of the
> `require`.  It's almost as good as the outermost file name, but not quite.

> > It's "neat and tidy", but at the cost of discarding all useful
> > information.  There are other common situations in Emacs where
> > the debugger is entered, or a backtrace output without debug-on-error
> > having to be set.

> Hmm... I can't think of such a situation.  When/where do we show
> a backtrace without the user's explicit request?

> >> So maybe those `condition-case` should be turned into
> >> `condition-case-unless-debug`?
> > I think this would be a very useful first step.  I think it likely a
> > user will set debug-on-error on encountering any unhelpful error
> > message.

> AFAICT this would basically be equivalent to aliasing
> `byte-compile-debug` to `debug-on-error`.

> It may turn out to be annoying occasionally, but I think it's worth
> a try (I've never found it useful to have `byte-compile-debug` set to
> t without also setting `debug-on-error` to t).


>         Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> year, in place of the current error output 
>
>     test-byte-compile-errors.el:2:11: Error: Wrong type argument: listp, baz
>
> the following line would appear:
>
>     test-byte-compile-errors.el:2:11: While loading "test-byte-compile-errors-2.el" Error: Wrong type argument: listp, baz

I can definitely live with this syntax, but maybe we should use
something more like what GCC uses (e.g. for errors in #included files)
which puts the "While loading" info on separate lines.

> This could surely be achieved with something like the following in Fload:
>
>     (handler-bind ((lambda (err)
>                     (signal (car err)
> 		            (combine-error-info "While loading " file
> 			                         (cdr err)))))
> 		  readevalloop (Qget_file_char, &input, hist_file_name,
> 		      0, Qnil, Qnil, Qnil, Qnil);)

That was my thinking as well (tho see below).

> (where, obviously, the details need to be worked out).  It would need
> augmenting with handling for (eq debug-on-error t), and probably a few
> other things, too.

`combine-error-info` is a bit problematic because we don't have clear
rules about the content of (cdr err), other than the fact that it should
be a list (tho we don't even enforce that very much).
Most likely we could append elements to that list, but we'd have to
worry about interactions with other libraries wanting to do similar
things.

So I was thinking that we should go instead with:

   (handler-bind ((error (lambda (err)
                           (push file (gethash err our-table-of-error-source)))))
     	  readevalloop (Qget_file_char, &input, hist_file_name,
     	      0, Qnil, Qnil, Qnil, Qnil);)

Where `our-table-of-error-source` would be a weak eq-hashtable.
Emacs Lisp guarantees that the `err` we get here will be the exact same
object that any subsequent `condition-case` will get when it finally
handles the error so that it can use `gethash` to fetch our
side information.

Note that we don't `signal` the error again, instead we let the error
handling code propagate it further, which is what `handler-bind` does
when the handler returns normally (which should also eliminate the
possible problems of interaction with `debug-on-error`).


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Wed, Oct 30, 2024 at 15:33:04 -0400, Stefan Monnier wrote:
> > year, in place of the current error output 

> >     test-byte-compile-errors.el:2:11: Error: Wrong type argument: listp, baz

> > the following line would appear:

> >     test-byte-compile-errors.el:2:11: While loading "test-byte-compile-errors-2.el" Error: Wrong type argument: listp, baz

> I can definitely live with this syntax, but maybe we should use
> something more like what GCC uses (e.g. for errors in #included files)
> which puts the "While loading" info on separate lines.

I thought about that, but seeing as how only one message at a time is
visible in the message area, we'd probably want to output one message
with embedded LFs, rather than several consecutive "While loading ..."s.

> > This could surely be achieved with something like the following in Fload:

> >     (handler-bind ((lambda (err)
> >                     (signal (car err)
> > 		            (combine-error-info "While loading " file
> > 			                         (cdr err)))))
> > 		  readevalloop (Qget_file_char, &input, hist_file_name,
> > 		      0, Qnil, Qnil, Qnil, Qnil);)

> That was my thinking as well (tho see below).

> > (where, obviously, the details need to be worked out).  It would need
> > augmenting with handling for (eq debug-on-error t), and probably a few
> > other things, too.

> `combine-error-info` is a bit problematic because we don't have clear
> rules about the content of (cdr err), other than the fact that it should
> be a list (tho we don't even enforce that very much).
> Most likely we could append elements to that list, but we'd have to
> worry about interactions with other libraries wanting to do similar
> things.

Do other libraries actually do such things?

> So I was thinking that we should go instead with:

>    (handler-bind ((error (lambda (err)
>                            (push file (gethash err our-table-of-error-source)))))
>      	  readevalloop (Qget_file_char, &input, hist_file_name,
>      	      0, Qnil, Qnil, Qnil, Qnil);)

> Where `our-table-of-error-source` would be a weak eq-hashtable.

Do we need a hash table when it's only going to have a few elements at
any time?  `require's rarely go more than 5 or 6 deep.  Why not just have
a simple dynamically bound list?  Or have I misunderstood what you mean?

> Emacs Lisp guarantees that the `err` we get here will be the exact same
> object that any subsequent `condition-case` will get when it finally
> handles the error so that it can use `gethash` to fetch our
> side information.

> Note that we don't `signal` the error again, instead we let the error
> handling code propagate it further, which is what `handler-bind` does
> when the handler returns normally (which should also eliminate the
> possible problems of interaction with `debug-on-error`).

The reason I suggested a signal call was so that the error information in
the successive ERRs would accumulate, rather than just being the fixed
ERR from the initial error.

And I think any call to the debugger on account of debug-on-error should
be in the innermost recursive `require', where the error is actually
signalled, so as to be of maximum use to the person debugging it.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I can definitely live with this syntax, but maybe we should use
>> something more like what GCC uses (e.g. for errors in #included files)
>> which puts the "While loading" info on separate lines.
> I thought about that, but seeing as how only one message at a time is
> visible in the message area, we'd probably want to output one message
> with embedded LFs, rather than several consecutive "While loading ..."s.

I don't have an opinion on that.  I only care about the case where that
info ends up in a file or buffer, along with other warnings and errors,
such as when I do `make` or `byte-compile-file`.  Ideally I'd like to be
able to click on each "While loading" to be brought to the place of the
corresponding `require`.  And ideally this would work with the existing
entries of `compilation-error-regexp-alist`.

>> `combine-error-info` is a bit problematic because we don't have clear
>> rules about the content of (cdr err), other than the fact that it should
>> be a list (tho we don't even enforce that very much).
>> Most likely we could append elements to that list, but we'd have to
>> worry about interactions with other libraries wanting to do similar
>> things.
>
> Do other libraries actually do such things?

Currently, this would be the first, but since I added `handler-bind`
I've already felt like doing such things on a few occasions, so it's
only a question of time.

>> So I was thinking that we should go instead with:
>
>>    (handler-bind ((error (lambda (err)
>>                            (push file (gethash err our-table-of-error-source)))))
>>      	  readevalloop (Qget_file_char, &input, hist_file_name,
>>      	      0, Qnil, Qnil, Qnil, Qnil);)
>
>> Where `our-table-of-error-source` would be a weak eq-hashtable.
>
> Do we need a hash table when it's only going to have a few elements at
> any time?  `require's rarely go more than 5 or 6 deep.  Why not just have
> a simple dynamically bound list?  Or have I misunderstood what you mean?

A hashtable is not the only solution, indeed.  But a weak hashtable
makes it possible to skip the need to use something that's "dynamically
bound", and hence to have to think about where we do the dynamic binding
and what to do if it's nested etc...
IOW, it seems simpler to me.

>> Emacs Lisp guarantees that the `err` we get here will be the exact same
>> object that any subsequent `condition-case` will get when it finally
>> handles the error so that it can use `gethash` to fetch our
>> side information.
>
>> Note that we don't `signal` the error again, instead we let the error
>> handling code propagate it further, which is what `handler-bind` does
>> when the handler returns normally (which should also eliminate the
>> possible problems of interaction with `debug-on-error`).
>
> The reason I suggested a signal call was so that the error information in
> the successive ERRs would accumulate, rather than just being the fixed
> ERR from the initial error.

In my suggestion I also accumulate them, but I put them in the side-info
hashtable instead of inside the error object.  I think it is important to
preserve the `eq`ness of the error object (since it embodies the fact
that we're still handling the same error), so if we don't use a side
table we would probably want to "combine" by mutating the error object.

> And I think any call to the debugger on account of debug-on-error should
> be in the innermost recursive `require', where the error is actually
> signalled, so as to be of maximum use to the person debugging it.

I think I agree tho "be in the innermost recursive `require'" seems
quite vague.  But in any case the handlers of `handler-bind` are run
before we unwind the stack (e.g. if your nesting looks like "require =>
require => error" the two handlers of your two `require`s will be run
before we get to the debugger but the debugger will still show the full
stack.  Tho with your use of "resignaling" within the handlers, the
stack will tend to be even "more full", with the two handlers nested
and still active), so no matter how we do it, I think we will indeed get
the behavior that I believe you describe.

More concretely, with your code, I think the debugger will be called
with a stack that looks like

     <calling the debugger>
     signal(...)
     ...
     <calling the handler of the outer handler-bind>
     signal(...)
     ...
     <calling the handler of the inner handler-bind>
     error("the actual error")
     ...
     handler-bind(...)    ; the inner one
     require(...)
     ...
     handler-bind(...)    ; the outer one
     require(...)

whereas with the code I suggest the stack should look like

     <calling the debugger>
     error("the actual error")
     ...
     handler-bind(...)    ; the inner one
     require(...)
     ...
     handler-bind(...)    ; the outer one
     require(...)

In any case, it should be easy to try out and change from one to the
other with very local changes (I'd expect that the code of the handlers
will be written in ELisp rather than C, right?).  So either way is fine.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Wed, Oct 30, 2024 at 18:31:35 -0400, Stefan Monnier wrote:
> >> I can definitely live with this syntax, but maybe we should use
> >> something more like what GCC uses (e.g. for errors in #included files)
> >> which puts the "While loading" info on separate lines.
> > I thought about that, but seeing as how only one message at a time is
> > visible in the message area, we'd probably want to output one message
> > with embedded LFs, rather than several consecutive "While loading ..."s.

> I don't have an opinion on that.  I only care about the case where that
> info ends up in a file or buffer, along with other warnings and errors,
> such as when I do `make` or `byte-compile-file`.

Indeed, it is a relatively small point, which can easily be changed once
we have working code.

> Ideally I'd like to be able to click on each "While loading" to be
> brought to the place of the corresponding `require`.  And ideally this
> would work with the existing entries of
> `compilation-error-regexp-alist`.

OK.  Again, I think this could easily be added once the basic code is in
place.

[ .... ]

> >> So I was thinking that we should go instead with:

> >>    (handler-bind ((error (lambda (err)
> >>                            (push file (gethash err our-table-of-error-source)))))
> >>      	  readevalloop (Qget_file_char, &input, hist_file_name,
> >>      	      0, Qnil, Qnil, Qnil, Qnil);)

> >> Where `our-table-of-error-source` would be a weak eq-hashtable.

> > Do we need a hash table when it's only going to have a few elements at
> > any time?  `require's rarely go more than 5 or 6 deep.  Why not just have
> > a simple dynamically bound list?  Or have I misunderstood what you mean?

> A hashtable is not the only solution, indeed.  But a weak hashtable
> makes it possible to skip the need to use something that's "dynamically
> bound", and hence to have to think about where we do the dynamic binding
> and what to do if it's nested etc...
> IOW, it seems simpler to me.

I don't think we need either.  In lread.c, there is already a static
variable Vloads_in_progress (which despite the name, is not visible in
Lisp).  This variable is a stack of the file names currently being
loaded.  We could surely just use this, and avoid code duplication.

[ .... ]

> >> Note that we don't `signal` the error again, instead we let the error
> >> handling code propagate it further, which is what `handler-bind` does
> >> when the handler returns normally (which should also eliminate the
> >> possible problems of interaction with `debug-on-error`).

> > The reason I suggested a signal call was so that the error information in
> > the successive ERRs would accumulate, rather than just being the fixed
> > ERR from the initial error.

I've been working out details in the last two or three days.  I actually
think the handler should exit with a (throw 'exit t).  That's because ...

> I think I agree tho "be in the innermost recursive `require'" seems
> quite vague.  But in any case the handlers of `handler-bind` are run
> before we unwind the stack (e.g. if your nesting looks like "require =>
> require => error" the two handlers of your two `require`s will be run
> before we get to the debugger but the debugger will still show the full
> stack.  Tho with your use of "resignaling" within the handlers, the
> stack will tend to be even "more full", with the two handlers nested
> and still active), so no matter how we do it, I think we will indeed get
> the behavior that I believe you describe.

We should respect any user setting of debug-on-error to anything non-nil.
If non-nil, we should enter the debugger within the handler-bind's
handler, so as to have access to Emacs's state when the error occurred.
This mechanism is used in eval-expression.  Also, this will prevent the
byte compiler having (eq byte-compile-debug nil) subverting the call of
the debugger.  After all, when loading a file, we're not actually in the
byte compiler, so byte-compile-debug shouldn't have an effect.

On leaving the debugger after an error, we definitely don't want to enter
the debugger again for the next file on the loading stack.  Hence we
should exit the handler with (throw 'exit t), or something like it.

[ .... ]

> In any case, it should be easy to try out and change from one to the
> other with very local changes (I'd expect that the code of the handlers
> will be written in ELisp rather than C, right?).  So either way is fine.

No, I think the handler code should be in C.  The function handler-bind-1
seems very clumsy for use from C code.  It requires a function with no
parameters, so this would likely involve creating a closure in the C
code.  This isn't good.

We should take inspiration from the functions internal_condition_case and
friends in eval.c, and create functions like internal_handler_bind.  This
would be almost identical to internal_condition_case, but would push a
HANDLER_BIND entry onto the handler stack rather than a CONDITION_CASE
one.  I think this would work well.

What do you say to all of this?  I'm about to start coding in earnest.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I don't think we need either.  In lread.c, there is already a static
> variable Vloads_in_progress (which despite the name, is not visible in
> Lisp).  This variable is a stack of the file names currently being
> loaded.  We could surely just use this, and avoid code duplication.

In that case we don't need `handler-bind` at all, and we just tack the
info when we build the error object in `Fsignal`.

But I don't think it would be correct in all cases: if file A loads file
B which compiles file C which loads file D which signals an error we
want the compiler error message to say "error in D loaded from C" and
not "error in D loaded from C loaded from B loaded from A".

With `handler-bind`s which add only the "current" file info, the error
will be labeled only with the relevant info, i.e. the intervening loads
between the `condition-case` (or `handler-bind`s) and the actual error.

> I've been working out details in the last two or three days.  I actually
> think the handler should exit with a (throw 'exit t).  That's because ...

What/where would be the matching `catch`?
I don't think the `handler-bind` we'd add to `load` should make
assumptions about how/where the errors will be handled.

>> I think I agree tho "be in the innermost recursive `require'" seems
>> quite vague.  But in any case the handlers of `handler-bind` are run
>> before we unwind the stack (e.g. if your nesting looks like "require =>
>> require => error" the two handlers of your two `require`s will be run
>> before we get to the debugger but the debugger will still show the full
>> stack.  Tho with your use of "resignaling" within the handlers, the
>> stack will tend to be even "more full", with the two handlers nested
>> and still active), so no matter how we do it, I think we will indeed get
>> the behavior that I believe you describe.
>
> We should respect any user setting of debug-on-error to anything non-nil.
> If non-nil, we should enter the debugger within the handler-bind's
> handler, so as to have access to Emacs's state when the error occurred.

I think we should do nothing special about `debug-on-error` and let it
be handled by the existing code: it should "just work".

> This mechanism is used in eval-expression.

That's because `eval-expression` doesn't want to obey `debug-on-error`.

> Also, this will prevent the byte compiler having (eq
> byte-compile-debug nil) subverting the call of the debugger.
> After all, when loading a file, we're not actually in the byte
> compiler, so byte-compile-debug shouldn't have an effect.

That would be nice.  Not sure how easy to do it, OTOH.

> On leaving the debugger after an error, we definitely don't want to
> enter the debugger again for the next file on the loading stack.
> Hence we should exit the handler with (throw 'exit t), or something
> like it.

If we're careful to preserve the `eq`ness of the error object, then the
debugger will know it has already been triggered for this error.

>> In any case, it should be easy to try out and change from one to the
>> other with very local changes (I'd expect that the code of the handlers
>> will be written in ELisp rather than C, right?).  So either way is fine.
> No, I think the handler code should be in C.  The function handler-bind-1
> seems very clumsy for use from C code.  It requires a function with no
> parameters, so this would likely involve creating a closure in the C
> code.  This isn't good.

I was talking about the code of the handlers.  I.e. the one we push
along with the HANDLER_BIND entry.   I'd build the handler by calling to
ELisp code (indeed, building closures from C is not good).


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

I have a first version of a patch for this.  Please see below.

On Sat, Nov 02, 2024 at 10:51:46 -0400, Stefan Monnier wrote:
> > I don't think we need either.  In lread.c, there is already a static
> > variable Vloads_in_progress (which despite the name, is not visible in
> > Lisp).  This variable is a stack of the file names currently being
> > loaded.  We could surely just use this, and avoid code duplication.

> In that case we don't need `handler-bind` at all, and we just tack the
> info when we build the error object in `Fsignal`.

I've actually made a copy of Vloads_in_progress to
Vloads_still_in_progress, a variable which isn't bound, hence whose
binding won't get lost in an error situation.  I'm not convinced we need
both of these, though.

> But I don't think it would be correct in all cases: if file A loads file
> B which compiles file C which loads file D which signals an error we
> want the compiler error message to say "error in D loaded from C" and
> not "error in D loaded from C loaded from B loaded from A".

Currently, my messages are "While loading foo.el... While loading
bar.el... While loading baz.el...\n<error message>.

[ .... ]

> > I've been working out details in the last two or three days.  I actually
> > think the handler should exit with a (throw 'exit t).  That's because ...

> What/where would be the matching `catch`?

I actually put in a (throw 'top-level t) on exit from the debugger.

> I don't think the `handler-bind` we'd add to `load` should make
> assumptions about how/where the errors will be handled.

The handler-bind handler I've added calls the debugger if debug-on-error
is non-nil.  Otherwise it just exits, letting something else handle the
error.

[ .... ]

> > We should respect any user setting of debug-on-error to anything
> > non-nil.  If non-nil, we should enter the debugger within the
> > handler-bind's handler, so as to have access to Emacs's state when
> > the error occurred.

> I think we should do nothing special about `debug-on-error` and let it
> be handled by the existing code: it should "just work".

Well, I've done something special with it, as I proposed.  

[ .... ]

> > Also, this will prevent the byte compiler having (eq
> > byte-compile-debug nil) subverting the call of the debugger.
> > After all, when loading a file, we're not actually in the byte
> > compiler, so byte-compile-debug shouldn't have an effect.

> That would be nice.  Not sure how easy to do it, OTOH.

It's done.  :-)  Please see my patch.

[ .... ]

> >> In any case, it should be easy to try out and change from one to the
> >> other with very local changes (I'd expect that the code of the handlers
> >> will be written in ELisp rather than C, right?).  So either way is fine.
> > No, I think the handler code should be in C.  The function handler-bind-1
> > seems very clumsy for use from C code.  It requires a function with no
> > parameters, so this would likely involve creating a closure in the C
> > code.  This isn't good.

> I was talking about the code of the handlers.  I.e. the one we push
> along with the HANDLER_BIND entry.   I'd build the handler by calling to
> ELisp code (indeed, building closures from C is not good).

I've extended the handler-bind mechanism to enable handlers to be
written in C.  They have the advantage that there is no minimum amount
of Lisp which needs to be loaded before the error handler can report a
problem with nested requires.

Anyhow, here's the patch (first version).  Please let me know what you
think.  Thanks!



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index f058fc48cc7..776083468c6 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1436,8 +1436,9 @@ byte-compile-report-error
 when printing the error message."
   (setq byte-compiler-error-flag t)
   (byte-compile-log-warning
-   (if (stringp error-info) error-info
-     (error-message-string error-info))
+   (prefix-load-file-names
+    (if (stringp error-info) error-info
+      (error-message-string error-info)))
    fill :error))
 \f
 ;;; sanity-checking arglists
diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index ec947c1215d..d49ebb20fd8 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -426,8 +426,10 @@ debugger--insert-header
     ;; User calls debug directly.
     (_
      (insert ": ")
-     (insert (backtrace-print-to-string (if (eq (car args) 'nil)
-                                            (cdr args) args)))
+     (insert
+      (prefix-load-file-names
+       (backtrace-print-to-string (if (eq (car args) 'nil)
+                                      (cdr args) args))))
      (insert ?\n))))
 
 \f
diff --git a/src/eval.c b/src/eval.c
index d0a2abf0089..8f95bd4d263 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1581,20 +1581,77 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 }
 
 /* Call the function BFUN with no arguments, catching errors within it
-   according to HANDLERS.  If there is an error, call HFUN with
-   one argument which is the data that describes the error:
+   according to HANDLERTYPE and HANDLERS.  If there is an error, call
+   HFUN with one argument which is the data that describes the error:
    (SIGNALNAME . DATA)
 
+   HANDLERTYPE must be either CONDITION_CASE or HANDLER_BIND.
+
    HANDLERS can be a list of conditions to catch.
    If HANDLERS is Qt, catch all errors.
    If HANDLERS is Qerror, catch all errors
    but allow the debugger to run if that is enabled.  */
 
+static Lisp_Object
+internal_cc_hb (enum handlertype handlertype,
+		Lisp_Object (*bfun) (void), Lisp_Object handlers,
+		Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, handlertype);
+
+  if (handlertype == HANDLER_BIND)
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
+  if (sys_setjmp (c->jmp))
+    {
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
+    }
+  else
+    {
+      Lisp_Object val = bfun ();
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
+    }
+}
+
 Lisp_Object
 internal_condition_case (Lisp_Object (*bfun) (void), Lisp_Object handlers,
 			 Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  return internal_cc_hb (CONDITION_CASE, bfun, handlers, hfun);
+}
+
+Lisp_Object
+internal_handler_bind (Lisp_Object (*bfun) (void), Lisp_Object handlers,
+		       Lisp_Object (*hfun) (Lisp_Object))
+{
+  if (NILP (Flistp (handlers)))
+    handlers = Fcons (handlers, Qnil);
+  return internal_cc_hb (HANDLER_BIND, bfun, handlers, hfun);
+}
+
+/* Like internal_cc_hb but call BFUN with ARG as its argument.  */
+
+static Lisp_Object
+internal_cc_hb_1 (enum handlertype handlertype,
+		  Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
+		  Lisp_Object handlers,
+		  Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, handlertype);
+  if (handlertype == HANDLER_BIND)
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1604,21 +1661,49 @@ internal_condition_case (Lisp_Object (*bfun) (void), Lisp_Object handlers,
     }
   else
     {
-      Lisp_Object val = bfun ();
+      Lisp_Object val = bfun (arg);
       eassert (handlerlist == c);
       handlerlist = c->next;
       return val;
     }
 }
 
-/* Like internal_condition_case but call BFUN with ARG as its argument.  */
-
 Lisp_Object
 internal_condition_case_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  return internal_cc_hb_1 (CONDITION_CASE, bfun, arg, handlers, hfun);
+}
+
+Lisp_Object
+internal_handler_bind_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  if (NILP (Flistp (handlers)))
+    handlers = Fcons (handlers, Qnil);
+  return internal_cc_hb_1 (HANDLER_BIND, bfun, arg, handlers, hfun);
+}
+
+/* Like internal_cc_hb_1 but call BFUN with ARG1 and ARG2 as
+   its arguments.  */
+
+static Lisp_Object
+internal_cc_hb_2 (enum handlertype handlertype,
+		  Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
+		  Lisp_Object arg1,
+		  Lisp_Object arg2,
+		  Lisp_Object handlers,
+		  Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, handlertype);
+  if (handlertype == HANDLER_BIND)
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1628,16 +1713,13 @@ internal_condition_case_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
     }
   else
     {
-      Lisp_Object val = bfun (arg);
+      Lisp_Object val = bfun (arg1, arg2);
       eassert (handlerlist == c);
       handlerlist = c->next;
       return val;
     }
 }
 
-/* Like internal_condition_case_1 but call BFUN with ARG1 and ARG2 as
-   its arguments.  */
-
 Lisp_Object
 internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
 			   Lisp_Object arg1,
@@ -1645,7 +1727,38 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  return internal_cc_hb_2 (CONDITION_CASE, bfun, arg1, arg2, handlers, hfun);
+}
+
+Lisp_Object
+internal_handler_bind_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
+			 Lisp_Object arg1,
+			 Lisp_Object arg2,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  if (NILP (Flistp (handlers)))
+    handlers = Fcons (handlers, Qnil);
+  return internal_cc_hb_2 (HANDLER_BIND, bfun, arg1, arg2, handlers, hfun);
+}
+
+/* Like internal_cc_hb_2 but the second argument is an arbitrary pointer.  */
+
+static Lisp_Object
+internal_cc_hb_1_voidstar (enum handlertype handlertype,
+			   Lisp_Object (*bfun) (Lisp_Object, void *),
+			   Lisp_Object arg1,
+			   void *arg2,
+			   Lisp_Object handlers,
+			   Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, handlertype);
+  if (handlertype == HANDLER_BIND)
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1662,6 +1775,30 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
     }
 }
 
+Lisp_Object
+internal_condition_case_1_voidstar (Lisp_Object (*bfun) (Lisp_Object, void *),
+				    Lisp_Object arg1,
+				    void *arg2,
+				    Lisp_Object handlers,
+				    Lisp_Object (*hfun) (Lisp_Object))
+{
+  return internal_cc_hb_1_voidstar (CONDITION_CASE, bfun, arg1, arg2,
+				     handlers, hfun);
+}
+
+Lisp_Object
+internal_handler_bind_1_voidstar (Lisp_Object (*bfun) (Lisp_Object, void *),
+			 Lisp_Object arg1,
+			 void *arg2,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  if (NILP (Flistp (handlers)))
+    handlers = Fcons (handlers, Qnil);
+  return internal_cc_hb_1_voidstar (HANDLER_BIND, bfun, arg1, arg2,
+				    handlers, hfun);
+}
+
 /* Like internal_condition_case but call BFUN with NARGS as first,
    and ARGS as second argument.  */
 
@@ -1691,6 +1828,35 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
     }
 }
 
+Lisp_Object
+internal_handler_bind_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
+			 ptrdiff_t nargs,
+			 Lisp_Object *args,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object err))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
+  if (sys_setjmp (c->jmp))
+    {
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
+    }
+  else
+    {
+      Lisp_Object val = bfun (nargs, args);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
+    }
+}
+
 static Lisp_Object Qcatch_all_memory_full;
 
 /* Like a combination of internal_condition_case_1 and internal_catch.
@@ -1900,9 +2066,12 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool continuable)
 	        max_ensure_room (20);
 	        push_handler (make_fixnum (skip + h->bytecode_dest),
 	                      SKIP_CONDITIONS);
-	        call1 (h->val, error);
-	        unbind_to (count, Qnil);
-	        pop_handler ();
+		if (NILP (h->val))
+		  (h->bin_handler) (error);
+		else
+		  call1 (h->val, error);
+	        unbind_to (count, Qnil); /* Removes SKIP_CONDITIONS handler.  */
+	        pop_handler ();		 /* Discards HANDLER_BIND handler.  */
 	      }
 	    continue;
 	  }
diff --git a/src/keyboard.c b/src/keyboard.c
index 6d28dca9aeb..6758c328038 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1096,7 +1096,7 @@ DEFUN ("command-error-default-function", Fcommand_error_default_function,
 	  Fdiscard_input ();
 	  bitch_at_user ();
 	}
-
+      context = Fprefix_load_file_names (context);
       print_error_message (data, Qt, SSDATA (context), signal);
     }
   return Qnil;
diff --git a/src/lisp.h b/src/lisp.h
index 5ef97047f76..205a5aa747b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3870,7 +3870,9 @@ record_in_backtrace (Lisp_Object function, Lisp_Object *args, ptrdiff_t nargs)
                                    'val' holds the retval during longjmp.  */
   HANDLER_BIND,                 /* Entry for 'handler-bind'.
                                    'tag_or_ch' holds the list of conditions.
-                                   'val' holds the handler function.
+                                   `val', if non-nil, holds the Lisp handler
+                                   function.  If nil, the handler function
+                                   is a C function held in `bin_handler'.
                                    The rest of the handler is unused,
                                    except for 'bytecode_dest' that holds
                                    the number of preceding HANDLER_BIND
@@ -3920,6 +3922,7 @@ record_in_backtrace (Lisp_Object function, Lisp_Object *args, ptrdiff_t nargs)
   struct bc_frame *act_rec;
   int poll_suppress_count;
   int interrupt_input_blocked;
+  Lisp_Object (*bin_handler) (Lisp_Object); /* Used only for HANDLER_BIND. */
 
 #ifdef HAVE_X_WINDOWS
   int x_error_handler_depth;
@@ -4873,11 +4876,19 @@ xsignal (Lisp_Object error_symbol, Lisp_Object data)
 extern Lisp_Object internal_catch (Lisp_Object, Lisp_Object (*) (Lisp_Object), Lisp_Object);
 extern Lisp_Object internal_lisp_condition_case (Lisp_Object, Lisp_Object, Lisp_Object);
 extern Lisp_Object internal_condition_case (Lisp_Object (*) (void), Lisp_Object, Lisp_Object (*) (Lisp_Object));
+extern Lisp_Object internal_handler_bind (Lisp_Object (*) (void), Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_condition_case_1 (Lisp_Object (*) (Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
+extern Lisp_Object internal_handler_bind_1 (Lisp_Object (*) (Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_condition_case_2 (Lisp_Object (*) (Lisp_Object, Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
+extern Lisp_Object internal_handler_bind_2 (Lisp_Object (*) (Lisp_Object, Lisp_Object), Lisp_Object, Lisp_Object, Lisp_Object, Lisp_Object (*) (Lisp_Object));
+extern Lisp_Object internal_condition_case_1_voidstar (Lisp_Object (*) (Lisp_Object, void*), Lisp_Object, void *, Lisp_Object, Lisp_Object (*) (Lisp_Object));
+extern Lisp_Object internal_handler_bind_1_voidstar (Lisp_Object (*) (Lisp_Object, void *), Lisp_Object, void *, Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_condition_case_n
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
      Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
+extern Lisp_Object internal_handler_bind_n
+    (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
+     Lisp_Object, Lisp_Object (*) (Lisp_Object));
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
 extern struct handler *push_handler (Lisp_Object, enum handlertype)
   ATTRIBUTE_RETURNS_NONNULL;
diff --git a/src/lread.c b/src/lread.c
index ea0398196e3..2fb407d9948 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -234,8 +234,10 @@ #define USE_ANDROID_ASSETS
 
 /* A list of file names for files being loaded in Fload.  Used to
    check for recursive loads.  */
-
 static Lisp_Object Vloads_in_progress;
+/* The same as the above, except it survives the unbinding done in the
+   event of an error, and can thus be used in error handling.  */
+Lisp_Object Vloads_still_in_progress;
 
 static int read_emacs_mule_char (int, int (*) (int, Lisp_Object),
                                  Lisp_Object);
@@ -1271,6 +1273,51 @@ close_file_unwind_android_fd (void *ptr)
 
 #endif
 
+/* Call readevalloop inside a `handler-bind' form.  */
+static Lisp_Object
+load_readevalloop (Lisp_Object hist_file_name, struct infile *input)
+{
+  readevalloop (Qget_file_char, input, hist_file_name,
+		0, Qnil, Qnil, Qnil, Qnil);
+  return Qnil;
+}
+
+DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
+       Sprefix_load_file_names, 1, 1, 0,
+       doc: /* Prefix the string BASE_STRING with a message about each
+file currently being loaded.  Return the resulting string and reset this
+information to null.  */)
+  (Lisp_Object base_string)
+{
+  Lisp_Object result = build_string ("");
+  Lisp_Object while_loading = build_string ("While loading ");
+  Lisp_Object ellipsis = build_string ("... ");
+
+  while (!NILP (Vloads_still_in_progress))
+    {
+      result = concat2 (concat3 (while_loading,
+				 Fcar (Vloads_still_in_progress),
+				 ellipsis),
+			result);
+      Vloads_still_in_progress = Fcdr (Vloads_still_in_progress);
+    }
+  result = concat3 (result, build_string ("\n"), base_string);
+  return result;
+}
+
+/* Handle errors signalled by the above function.  */
+static Lisp_Object
+rel_error_handler (Lisp_Object err)
+{
+  if (!NILP (Vdebug_on_error))
+    {
+      call_debugger (err);
+      Fthrow (Qtop_level, Qt);
+    }
+  else
+    return err;
+}
+
 DEFUN ("load", Fload, Sload, 1, 5, 0,
        doc: /* Execute a file of Lisp code named FILE.
 First try FILE with `.elc' appended, then try with `.el', then try
@@ -1516,6 +1563,7 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
 	signal_error ("Recursive load", Fcons (found, Vloads_in_progress));
     record_unwind_protect (record_load_unwind, Vloads_in_progress);
     Vloads_in_progress = Fcons (found, Vloads_in_progress);
+    Vloads_still_in_progress = Vloads_in_progress;
   }
 
   /* All loads are by default dynamic, unless the file itself specifies
@@ -1606,16 +1654,25 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
       if (!NILP (Vload_source_file_function))
 	{
 	  Lisp_Object val;
+	  Lisp_Object args[5];
 
 	  if (lread_fd_p)
 	    {
 	      lread_close (fd);
 	      clear_unwind_protect (fd_index);
 	    }
-	  val = call4 (Vload_source_file_function, found, hist_file_name,
-		       NILP (noerror) ? Qnil : Qt,
-		       (NILP (nomessage) || force_load_messages) ? Qnil : Qt);
-	  return unbind_to (count, val);
+	  args[0] = Vload_source_file_function;
+	  args[1] = found;
+	  args[2] = hist_file_name;
+	  args[3] = NILP (noerror) ? Qnil : Qt;
+	  args[4] = (NILP (nomessage) || force_load_messages) ? Qnil : Qt;
+	  val = internal_handler_bind_n (Ffuncall,
+					 5, args,
+					 Qerror,
+					 rel_error_handler);
+	  unbind_to (count, Qnil);
+	  Vloads_still_in_progress = Vloads_in_progress;
+	  return val;
 	}
     }
 
@@ -1724,8 +1781,11 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
         Fset (Qlexical_binding, Qt);
 
       if (! version || version >= 22)
-        readevalloop (Qget_file_char, &input, hist_file_name,
-                      0, Qnil, Qnil, Qnil, Qnil);
+	internal_handler_bind_1_voidstar
+	  ((Lisp_Object (*) (Lisp_Object, void *)) load_readevalloop,
+	   hist_file_name,
+	   &input,
+	   Qerror, rel_error_handler);
       else
         {
           /* We can't handle a file which was compiled with
@@ -1741,6 +1801,8 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
   if (!NILP (Ffboundp (Qdo_after_load_evaluation)))
     call1 (Qdo_after_load_evaluation, hist_file_name) ;
 
+  Vloads_still_in_progress = Vloads_in_progress;
+
   for (int i = 0; i < ARRAYELTS (saved_strings); i++)
     {
       xfree (saved_strings[i].string);
@@ -5772,6 +5834,7 @@ init_lread (void)
   Vload_true_file_name = Qnil;
   Vstandard_input = Qt;
   Vloads_in_progress = Qnil;
+  Vloads_still_in_progress = Qnil;
 }
 
 /* Print a warning that directory intended for use USE and with name
@@ -5819,6 +5882,7 @@ syms_of_lread (void)
   defsubr (&Sintern_soft);
   defsubr (&Sunintern);
   defsubr (&Sget_load_suffixes);
+  defsubr (&Sprefix_load_file_names);
   defsubr (&Sload);
   defsubr (&Seval_buffer);
   defsubr (&Seval_region);
@@ -6138,6 +6202,8 @@ syms_of_lread (void)
 
   Vloads_in_progress = Qnil;
   staticpro (&Vloads_in_progress);
+  Vloads_still_in_progress = Qnil;
+  staticpro (&Vloads_still_in_progress);
 
   DEFSYM (Qhash_table, "hash-table");
   DEFSYM (Qdata, "data");


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Alan,

I couldn't properly review your code because I had trouble
understanding it.  So instead, it's mostly questions.

> +static Lisp_Object
> +internal_cc_hb (enum handlertype handlertype,
> +		Lisp_Object (*bfun) (void), Lisp_Object handlers,
> +		Lisp_Object (*hfun) (Lisp_Object))
> +{
> +  struct handler *c = push_handler (handlers, handlertype);
> +
> +  if (handlertype == HANDLER_BIND)
> +    {
> +      c->val = Qnil;
> +      c->bin_handler = hfun;
> +      c->bytecode_dest = 0;
> +    }
> +  if (sys_setjmp (c->jmp))
> +    {
> +      Lisp_Object val = handlerlist->val;
> +      clobbered_eassert (handlerlist == c);
> +      handlerlist = handlerlist->next;
> +      return hfun (val);
> +    }
> +  else
> +    {
> +      Lisp_Object val = bfun ();
> +      eassert (handlerlist == c);
> +      handlerlist = c->next;
> +      return val;
> +    }
> +}

I don't understand the above code: `handler-bind` is not supposed to
call setjmp/longjmp: when the handler of a `handler-bind` gets called,
the stack is not unwound.
So what is the `sys_setjmp` for when `handlertype == HANDLER_BIND`?

> @@ -1900,9 +2066,12 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool continuable)
>  	        max_ensure_room (20);
>  	        push_handler (make_fixnum (skip + h->bytecode_dest),
>  	                      SKIP_CONDITIONS);
> -	        call1 (h->val, error);
> -	        unbind_to (count, Qnil);
> -	        pop_handler ();
> +		if (NILP (h->val))
> +		  (h->bin_handler) (error);

[ We don't need parens around `h->bin_handler`.  ]

> +		else
> +		  call1 (h->val, error);
> +	        unbind_to (count, Qnil); /* Removes SKIP_CONDITIONS handler.  */
> +	        pop_handler ();		 /* Discards HANDLER_BIND handler.  */

These comments don't match my understanding of the code: IIRC the
`pop_handler` pops the `SKIP_CONDITIONS` handler.  Also there's no
reason to presume the HANDLER_BIND handler is at the top, so if we
wanted to remove it, we'd have to work harder.

> +/* Handle errors signalled by the above function.  */
> +static Lisp_Object
> +rel_error_handler (Lisp_Object err)
> +{
> +  if (!NILP (Vdebug_on_error))
> +    {
> +      call_debugger (err);
> +      Fthrow (Qtop_level, Qt);
> +    }
> +  else
> +    return err;
> +}

I don't understand that.
AFAICT, this is the only HANDLER_BIND handler you install from
C code, right?  So this is the thing that motivates all the changes like
`internal_cc_hb`, etc... so clearly it must do something worthwhile, but
I don't see what it is.
How is this related to `Fprefix_load_file_names`?


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Eli Zaretskii]

> Cc: acm@muc.de, 66912@debbugs.gnu.org
> Date: Sun, 3 Nov 2024 22:34:58 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> +static Lisp_Object
> +internal_cc_hb (enum handlertype handlertype,

Please don't use such cryptic names for functions.  There's no need to
keep function names short if that runs afoul of mnemonics.

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

Thanks for such a rapid response!

On Sun, Nov 03, 2024 at 21:46:42 -0500, Stefan Monnier wrote:
> Hi Alan,

> I couldn't properly review your code because I had trouble
> understanding it.  So instead, it's mostly questions.

> > +static Lisp_Object
> > +internal_cc_hb (enum handlertype handlertype,
> > +		Lisp_Object (*bfun) (void), Lisp_Object handlers,
> > +		Lisp_Object (*hfun) (Lisp_Object))
> > +{
> > +  struct handler *c = push_handler (handlers, handlertype);
> > +
> > +  if (handlertype == HANDLER_BIND)
> > +    {
> > +      c->val = Qnil;
> > +      c->bin_handler = hfun;
> > +      c->bytecode_dest = 0;
> > +    }
> > +  if (sys_setjmp (c->jmp))
> > +    {
> > +      Lisp_Object val = handlerlist->val;
> > +      clobbered_eassert (handlerlist == c);
> > +      handlerlist = handlerlist->next;
> > +      return hfun (val);
> > +    }
> > +  else
> > +    {
> > +      Lisp_Object val = bfun ();
> > +      eassert (handlerlist == c);
> > +      handlerlist = c->next;
> > +      return val;
> > +    }
> > +}

> I don't understand the above code: `handler-bind` is not supposed to
> call setjmp/longjmp: when the handler of a `handler-bind` gets called,
> the stack is not unwound.

I started off by just making the HANDLER_BIND case use the same code as
CONDITION_CASE, and got segfaults.  I've tried to find the places where
a longjmp gets called for condition-case, but my brain's tired after a
weekend of heavy coding.

Given HANDLER_BIND doesn't need the setjmp/longjmp mechanism, it would
seem there's no sense in combining the HANDLER_BIND and CONDITION_CASE
cases in internal_cc_hb and friends.  I should just restore the
condition-case functions to what they were, and add separate new ones
for handler-bind.

> So what is the `sys_setjmp` for when `handlertype == HANDLER_BIND`?

After reading your previous paragraph, probably nothing.

[ .... ]

> [ We don't need parens around `h->bin_handler`.  ]

They've gone.

> > +		else
> > +		  call1 (h->val, error);
> > +	        unbind_to (count, Qnil); /* Removes SKIP_CONDITIONS handler.  */
> > +	        pop_handler ();		 /* Discards HANDLER_BIND handler.  */

> These comments don't match my understanding of the code: IIRC the
> `pop_handler` pops the `SKIP_CONDITIONS` handler.

I think I see now you're right.  push_handler doesn't push an entry onto
the binding stack.  I'll amend these comments as soon as I understand
the code.  I think these lines definitely need comments.

> Also there's no reason to presume the HANDLER_BIND handler is at the
> top, so if we wanted to remove it, we'd have to work harder.

This code is difficult to understand.  What is the purpose of the
binding block around the call of the handler function?  I think a
comment here would help.

> > +/* Handle errors signalled by the above function.  */
> > +static Lisp_Object
> > +rel_error_handler (Lisp_Object err)
> > +{
> > +  if (!NILP (Vdebug_on_error))
> > +    {
> > +      call_debugger (err);
> > +      Fthrow (Qtop_level, Qt);
> > +    }
> > +  else
> > +    return err;
> > +}

> I don't understand that.
> AFAICT, this is the only HANDLER_BIND handler you install from
> C code, right?  So this is the thing that motivates all the changes like
> `internal_cc_hb`, etc... so clearly it must do something worthwhile, but
> I don't see what it is.

rel_error_handler is currently the only such handler, yes.  All this
code (once it's debugged) will fix the inconsistency that
condition-case, unwind-protect, catch and throw, ... can all be used
from C code, but handler-bind can't be.

rel_error_handler, in particular, allows the debugger to be called
regardless of any active condition_cases which would otherwise block it.
In the byte compiler, there is such a blocking condition-case set up by
bytecomp--displaying-warnings when byte-compile-debug is nil.  This
mechanism is currently only used in Fload.

> How is this related to `Fprefix_load_file_names`?

Not closely.  I think I should have propsed two separate patches, rather
than the big one I did.  Fprefix_load_file_names is what puts the "While
loading foo.el... " in front of an error message.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Eli.

On Mon, Nov 04, 2024 at 14:20:06 +0200, Eli Zaretskii wrote:
> > Cc: acm@muc.de, 66912@debbugs.gnu.org
> > Date: Sun, 3 Nov 2024 22:34:58 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > +static Lisp_Object
> > +internal_cc_hb (enum handlertype handlertype,

> Please don't use such cryptic names for functions.  There's no need to
> keep function names short if that runs afoul of mnemonics.

I thought that one was clear enough in the context in eval.c.  After
Stefan M's comments, the function isn't suitable for what it tries to
do, so will be disappearing.  But OK, I'll watch out for similar things
in the future.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Eli Zaretskii]

> Date: Mon, 4 Nov 2024 13:13:13 +0000
> Cc: monnier@iro.umontreal.ca, 66912@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > > +static Lisp_Object
> > > +internal_cc_hb (enum handlertype handlertype,
> 
> > Please don't use such cryptic names for functions.  There's no need to
> > keep function names short if that runs afoul of mnemonics.
> 
> I thought that one was clear enough in the context in eval.c.  After
> Stefan M's comments, the function isn't suitable for what it tries to
> do, so will be disappearing.  But OK, I'll watch out for similar things
> in the future.

Thanks.

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I don't understand the above code: `handler-bind` is not supposed to
>> call setjmp/longjmp: when the handler of a `handler-bind` gets called,
>> the stack is not unwound.
>
> I started off by just making the HANDLER_BIND case use the same code as
> CONDITION_CASE, and got segfaults.  I've tried to find the places where
> a longjmp gets called for condition-case, but my brain's tired after a
> weekend of heavy coding.

The longjmp is in `unwind_to_catch`, called from `signal_or_quit`, but
we only get there for CONDITION_CASE, not for HANDLER_BIND.

> Given HANDLER_BIND doesn't need the setjmp/longjmp mechanism, it would
> seem there's no sense in combining the HANDLER_BIND and CONDITION_CASE
> cases in internal_cc_hb and friends.  I should just restore the
> condition-case functions to what they were, and add separate new ones
> for handler-bind.

OK.

> I think I see now you're right.  push_handler doesn't push an entry onto
> the binding stack.  I'll amend these comments as soon as I understand
> the code.  I think these lines definitely need comments.

Maybe pointing the reader to the SKIP_CONDITIONS comment in `lisp.h`?

>> Also there's no reason to presume the HANDLER_BIND handler is at the
>> top, so if we wanted to remove it, we'd have to work harder.
> This code is difficult to understand.  What is the purpose of the
> binding block around the call of the handler function?  I think a
> comment here would help.

The `unbind_to` is there because `max_ensure_room` may
`specbind` something.  In 99% of the cases it does nothing.

>> How is this related to `Fprefix_load_file_names`?
> Not closely.  I think I should have propsed two separate patches, rather
> than the big one I did.  Fprefix_load_file_names is what puts the "While
> loading foo.el... " in front of an error message.

Splitting it into two would be good yes.
AFAICT, the `Fprefix_load_file_names` is the part that aims to address
bug#66912.  IMO the other belongs in another bug-report/feature-request?


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello again, Stefan.

[ I've just got your latest post, answering my last one.  Thanks!  But
I'm sending you this post anyway, which I'd almost finished. ]

Progress!

On Mon, Nov 04, 2024 at 12:52:10 +0000, Alan Mackenzie wrote:
> On Sun, Nov 03, 2024 at 21:46:42 -0500, Stefan Monnier wrote:
> > Hi Alan,

[ .... ]

> > I don't understand the above code: `handler-bind` is not supposed to
> > call setjmp/longjmp: when the handler of a `handler-bind` gets
> > called, the stack is not unwound.

[ .... ]

> Given HANDLER_BIND doesn't need the setjmp/longjmp mechanism, it would
> seem there's no sense in combining the HANDLER_BIND and CONDITION_CASE
> cases in internal_cc_hb and friends.  I should just restore the
> condition-case functions to what they were, and add separate new ones
> for handler-bind.

I've now done this.  Possibly the new function
internal_condition_case_1_voidstar is superfluous and should be deleted.
Also, not all of the internal_handler_bind* functions are currently
used.

[ .... ]

> > > +		else
> > > +		  call1 (h->val, error);
> > > +	        unbind_to (count, Qnil); /* Removes SKIP_CONDITIONS handler.  */
> > > +	        pop_handler ();		 /* Discards HANDLER_BIND handler.  */

> > These comments don't match my understanding of the code: IIRC the
> > `pop_handler` pops the `SKIP_CONDITIONS` handler.

> I think I see now you're right.  push_handler doesn't push an entry onto
> the binding stack.  I'll amend these comments as soon as I understand
> the code.  I think these lines definitely need comments.

> > Also there's no reason to presume the HANDLER_BIND handler is at the
> > top, so if we wanted to remove it, we'd have to work harder.

> This code is difficult to understand.  What is the purpose of the
> binding block around the call of the handler function?  I think a
> comment here would help.

I've worked this out.  The binding block is to restore the size of the
stack via the unwind_protect set up in the call to max_ensure_room.
I've added a comment to say that.

[ .... ]

I've also amended the doc string of Fhandler_bind_1, saying that the
return value from a handler gets ignored, and clarifying that a normal
return from a handler means the error hasn't been handled.  If you agree
with this, I will make a corresponding amendment to the macro
handler-bind.

Here is the revised part of the patch, just for eval.c:



diff --git a/src/eval.c b/src/eval.c
index d0a2abf0089..79cf9115379 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -256,7 +256,7 @@ restore_stack_limits (Lisp_Object data)
   max_lisp_eval_depth = old_depth;
 }
 
-/* Try and ensure that we have at least B dpeth available.  */
+/* Try and ensure that we have at least B depth available.  */
 
 static void
 max_ensure_room (intmax_t b)
@@ -1450,9 +1450,9 @@ DEFUN ("handler-bind-1", Fhandler_bind_1, Shandler_bind_1, 1, MANY, 0,
 error matches one of CONDITIONS, then the associated HANDLER is
 called with the error as argument.
 HANDLER should either transfer the control via a non-local exit,
-or return normally.
-If it returns normally, the search for an error handler continues
-from where it left off.
+or return normally, should it fail to handle the error.
+If it returns normally, the returned value is ignored, and the
+search for an error handler continues from where it left off.
 
 usage: (handler-bind BODYFUN [CONDITIONS HANDLER]...)  */)
   (ptrdiff_t nargs, Lisp_Object *args)
@@ -1583,7 +1583,7 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 /* Call the function BFUN with no arguments, catching errors within it
    according to HANDLERS.  If there is an error, call HFUN with
    one argument which is the data that describes the error:
-   (SIGNALNAME . DATA)
+   (SIGNALNAME . DATA).
 
    HANDLERS can be a list of conditions to catch.
    If HANDLERS is Qt, catch all errors.
@@ -1662,6 +1662,33 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
     }
 }
 
+/* Like internal_condition_case_2 but the second argument is an arbitrary
+   pointer.  */
+
+Lisp_Object
+internal_condition_case_1_voidstar (Lisp_Object (*bfun) (Lisp_Object, void *),
+				    Lisp_Object arg1,
+				    void *arg2,
+				    Lisp_Object handlers,
+				    Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  if (sys_setjmp (c->jmp))
+    {
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
+    }
+  else
+    {
+      Lisp_Object val = bfun (arg1, arg2);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
+    }
+}
+
 /* Like internal_condition_case but call BFUN with NARGS as first,
    and ARGS as second argument.  */
 
@@ -1691,6 +1718,137 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
     }
 }
 
+/* Call the function BFUN with no arguments, catching errors within it
+   in a `handler-bind' construct according to HANDLERS.  If there is an
+   error, call HFUN with one argument which is the data that describes
+   the error: (SIGNALNAME . DATA).
+
+   HANDLERS is either a list of conditions to catch, or one of these
+   symbols:
+   If HANDLERS is Qt, catch all errors.
+   If HANDLERS is Qerror, catch all errors, but allow the debugger to
+   run if that is enabled.  */
+
+Lisp_Object
+internal_handler_bind (Lisp_Object (*bfun) (void), Lisp_Object handlers,
+		       Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+  /* if (NILP (Flistp (handlers))) */
+  /*   handlers = Fcons (handlers, Qnil); */
+  c->val = Qnil;
+  c->bin_handler = hfun;
+  c->bytecode_dest = 0;
+
+  {
+    Lisp_Object val = bfun ();
+    eassert (handlerlist == c);
+    handlerlist = c->next;
+    return val;
+  }
+}
+
+/* Like internal_handler_bind, but call BFUN with ARG as its argument.  */
+
+Lisp_Object
+internal_handler_bind_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+  /* if (NILP (Flistp (handlers))) */
+  /*   handlers = Fcons (handlers, Qnil); */
+  c->val = Qnil;
+  c->bin_handler = hfun;
+  c->bytecode_dest = 0;
+  {
+    Lisp_Object val = bfun (arg);
+    eassert (handlerlist == c);
+    handlerlist = c->next;
+    return val;
+  }
+}
+
+/* Like internal_handler_bind_1, but call BFUN with ARG1 and ARG2 as
+   its arguments.  */
+
+Lisp_Object
+internal_handler_bind_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
+			 Lisp_Object arg1,
+			 Lisp_Object arg2,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+  /* if (NILP (Flistp (handlers))) */
+  /*   handlers = Fcons (handlers, Qnil); */
+  c->val = Qnil;
+  c->bin_handler = hfun;
+  c->bytecode_dest = 0;
+  {
+    Lisp_Object val = bfun (arg1, arg2);
+    eassert (handlerlist == c);
+    handlerlist = c->next;
+    return val;
+  }
+}
+
+/* Like internal_handler_bind_2, but the second argument is an arbitrary
+   pointer.  */
+
+Lisp_Object
+internal_handler_bind_1_voidstar (Lisp_Object (*bfun) (Lisp_Object, void *),
+			 Lisp_Object arg1,
+			 void *arg2,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+  /* if (NILP (Flistp (handlers))) */
+  /*   handlers = Fcons (handlers, Qnil); */
+  c->val = Qnil;
+  c->bin_handler = hfun;
+  c->bytecode_dest = 0;
+  {
+    Lisp_Object val = bfun (arg1, arg2);
+    eassert (handlerlist == c);
+    handlerlist = c->next;
+    return val;
+  }
+}
+
+/* Like internal_handler_bind, but call BFUN with NARGS as first,
+   and ARGS as second argument.  */
+
+Lisp_Object
+internal_handler_bind_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
+			 ptrdiff_t nargs,
+			 Lisp_Object *args,
+			 Lisp_Object handlers,
+			 Lisp_Object (*hfun) (Lisp_Object err))
+{
+  struct handler *c = push_handler (handlers, HANDLER_BIND);
+    {
+      c->val = Qnil;
+      c->bin_handler = hfun;
+      c->bytecode_dest = 0;
+    }
+  if (sys_setjmp (c->jmp))
+    {
+      Lisp_Object val = handlerlist->val;
+      clobbered_eassert (handlerlist == c);
+      handlerlist = handlerlist->next;
+      return hfun (val);
+    }
+  else
+    {
+      Lisp_Object val = bfun (nargs, args);
+      eassert (handlerlist == c);
+      handlerlist = c->next;
+      return val;
+    }
+}
+
 static Lisp_Object Qcatch_all_memory_full;
 
 /* Like a combination of internal_condition_case_1 and internal_catch.
@@ -1900,9 +2058,12 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool continuable)
 	        max_ensure_room (20);
 	        push_handler (make_fixnum (skip + h->bytecode_dest),
 	                      SKIP_CONDITIONS);
-	        call1 (h->val, error);
-	        unbind_to (count, Qnil);
-	        pop_handler ();
+		if (NILP (h->val))
+		  h->bin_handler (error);
+		else
+		  call1 (h->val, error);
+	        unbind_to (count, Qnil); /* Restore after `max_ensure_room'.  */
+	        pop_handler ();          /* Remove SKIP_CONDITIONS handler.  */
 	      }
 	    continue;
 	  }


> >         Stefan

> -- 
> Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Mon, Nov 04, 2024 at 11:12:19 -0500, Stefan Monnier wrote:
> >> I don't understand the above code: `handler-bind` is not supposed to
> >> call setjmp/longjmp: when the handler of a `handler-bind` gets called,
> >> the stack is not unwound.

> > I started off by just making the HANDLER_BIND case use the same code as
> > CONDITION_CASE, and got segfaults.  I've tried to find the places where
> > a longjmp gets called for condition-case, but my brain's tired after a
> > weekend of heavy coding.

> The longjmp is in `unwind_to_catch`, called from `signal_or_quit`, but
> we only get there for CONDITION_CASE, not for HANDLER_BIND.

OK, thanks.

> > Given HANDLER_BIND doesn't need the setjmp/longjmp mechanism, it would
> > seem there's no sense in combining the HANDLER_BIND and CONDITION_CASE
> > cases in internal_cc_hb and friends.  I should just restore the
> > condition-case functions to what they were, and add separate new ones
> > for handler-bind.

> OK.

DONE.

> > I think I see now you're right.  push_handler doesn't push an entry onto
> > the binding stack.  I'll amend these comments as soon as I understand
> > the code.  I think these lines definitely need comments.

> Maybe pointing the reader to the SKIP_CONDITIONS comment in `lisp.h`?

I've put a brief comment on the line saying /* Restore after
`max_ensure_room'.  */

> >> Also there's no reason to presume the HANDLER_BIND handler is at the
> >> top, so if we wanted to remove it, we'd have to work harder.
> > This code is difficult to understand.  What is the purpose of the
> > binding block around the call of the handler function?  I think a
> > comment here would help.

> The `unbind_to` is there because `max_ensure_room` may
> `specbind` something.  In 99% of the cases it does nothing.

OK, thanks, I've got that.

> >> How is this related to `Fprefix_load_file_names`?
> > Not closely.  I think I should have propsed two separate patches, rather
> > than the big one I did.  Fprefix_load_file_names is what puts the "While
> > loading foo.el... " in front of an error message.

> Splitting it into two would be good yes.

OK, I think I agree, even if I would rather not have to advocate for the
second patch again.  ;-)

> AFAICT, the `Fprefix_load_file_names` is the part that aims to address
> bug#66912.  IMO the other belongs in another bug-report/feature-request?

OK, I've separated out the bit that applies "While loading foo.el... "
from all the stuff about handler-bind.  The patch below should, on its
own, fix bug#66912.

What's not there yet is being able to get into the debugger when a byte
compilation is requiring a file, and debug-on-error has been set.  That
will be the subject of the next bug report.

Here's the amended patch.  It's a lot shorter (and easier) than the
previous version.  I would like to commit it as the fix to bug#66912.



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index f058fc48cc7..776083468c6 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1436,8 +1436,9 @@ byte-compile-report-error
 when printing the error message."
   (setq byte-compiler-error-flag t)
   (byte-compile-log-warning
-   (if (stringp error-info) error-info
-     (error-message-string error-info))
+   (prefix-load-file-names
+    (if (stringp error-info) error-info
+      (error-message-string error-info)))
    fill :error))
 \f
 ;;; sanity-checking arglists
diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index ec947c1215d..4068daf6614 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -418,7 +418,9 @@ debugger--insert-header
     ;; Debugger entered for an error.
     ('error
      (insert "--Lisp error: ")
-     (insert (backtrace-print-to-string (nth 1 args)))
+     (insert
+      (prefix-load-file-names
+       (backtrace-print-to-string (nth 1 args))))
      (insert ?\n))
     ;; debug-on-call, when the next thing is an eval.
     ('t
@@ -426,8 +428,10 @@ debugger--insert-header
     ;; User calls debug directly.
     (_
      (insert ": ")
-     (insert (backtrace-print-to-string (if (eq (car args) 'nil)
-                                            (cdr args) args)))
+     (insert
+      (prefix-load-file-names
+       (backtrace-print-to-string (if (eq (car args) 'nil)
+                                      (cdr args) args))))
      (insert ?\n))))
 
 \f
diff --git a/src/keyboard.c b/src/keyboard.c
index 6d28dca9aeb..6758c328038 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1096,7 +1096,7 @@ DEFUN ("command-error-default-function", Fcommand_error_default_function,
 	  Fdiscard_input ();
 	  bitch_at_user ();
 	}
-
+      context = Fprefix_load_file_names (context);
       print_error_message (data, Qt, SSDATA (context), signal);
     }
   return Qnil;
diff --git a/src/lread.c b/src/lread.c
index ea0398196e3..f81efb3df70 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -236,6 +236,9 @@ #define USE_ANDROID_ASSETS
    check for recursive loads.  */
 
 static Lisp_Object Vloads_in_progress;
+/* The same as the above, except it survives the unbinding done in the
+   event of an error, and can thus be used in error handling.  */
+Lisp_Object Vloads_still_in_progress;
 
 static int read_emacs_mule_char (int, int (*) (int, Lisp_Object),
                                  Lisp_Object);
@@ -1271,6 +1274,29 @@ close_file_unwind_android_fd (void *ptr)
 
 #endif
 
+DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
+       Sprefix_load_file_names, 1, 1, 0,
+       doc: /* Prefix the string BASE_STRING with a message about each
+file currently being loaded.  Return the resulting string and reset this
+information to null.  */)
+  (Lisp_Object base_string)
+{
+  Lisp_Object result = build_string ("");
+  Lisp_Object while_loading = build_string ("While loading ");
+  Lisp_Object ellipsis = build_string ("... ");
+
+  while (!NILP (Vloads_still_in_progress))
+    {
+      result = concat2 (concat3 (while_loading,
+				 Fcar (Vloads_still_in_progress),
+				 ellipsis),
+			result);
+      Vloads_still_in_progress = Fcdr (Vloads_still_in_progress);
+    }
+  result = concat3 (result, build_string ("\n"), base_string);
+  return result;
+}
+
 DEFUN ("load", Fload, Sload, 1, 5, 0,
        doc: /* Execute a file of Lisp code named FILE.
 First try FILE with `.elc' appended, then try with `.el', then try
@@ -1516,6 +1542,7 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
 	signal_error ("Recursive load", Fcons (found, Vloads_in_progress));
     record_unwind_protect (record_load_unwind, Vloads_in_progress);
     Vloads_in_progress = Fcons (found, Vloads_in_progress);
+    Vloads_still_in_progress = Vloads_in_progress;
   }
 
   /* All loads are by default dynamic, unless the file itself specifies
@@ -1615,7 +1642,9 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
 	  val = call4 (Vload_source_file_function, found, hist_file_name,
 		       NILP (noerror) ? Qnil : Qt,
 		       (NILP (nomessage) || force_load_messages) ? Qnil : Qt);
-	  return unbind_to (count, val);
+	  unbind_to (count, Qnil);
+	  Vloads_still_in_progress = Vloads_in_progress;
+	  return val;
 	}
     }
 
@@ -1741,6 +1770,8 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
   if (!NILP (Ffboundp (Qdo_after_load_evaluation)))
     call1 (Qdo_after_load_evaluation, hist_file_name) ;
 
+  Vloads_still_in_progress = Vloads_in_progress;
+
   for (int i = 0; i < ARRAYELTS (saved_strings); i++)
     {
       xfree (saved_strings[i].string);
@@ -5772,6 +5803,7 @@ init_lread (void)
   Vload_true_file_name = Qnil;
   Vstandard_input = Qt;
   Vloads_in_progress = Qnil;
+  Vloads_still_in_progress = Qnil;
 }
 
 /* Print a warning that directory intended for use USE and with name
@@ -5819,6 +5851,7 @@ syms_of_lread (void)
   defsubr (&Sintern_soft);
   defsubr (&Sunintern);
   defsubr (&Sget_load_suffixes);
+  defsubr (&Sprefix_load_file_names);
   defsubr (&Sload);
   defsubr (&Seval_buffer);
   defsubr (&Seval_region);
@@ -6138,6 +6171,8 @@ syms_of_lread (void)
 
   Vloads_in_progress = Qnil;
   staticpro (&Vloads_in_progress);
+  Vloads_still_in_progress = Qnil;
+  staticpro (&Vloads_still_in_progress);
 
   DEFSYM (Qhash_table, "hash-table");
   DEFSYM (Qdata, "data");


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here's the amended patch.  It's a lot shorter (and easier) than the
> previous version.  I would like to commit it as the fix to bug#66912.

Thanks.  I'm not in love with this approach, among other things because
of the problem I described earlier:

    But I don't think it would be correct in all cases: if file A loads
    file B which compiles file C which loads file D which signals an
    error we want the compiler error message to say "error in D loaded
    from C" and not "error in D loaded from C loaded from B loaded from
    A".

but it's not the end of the world, so I don't object to installing it on
`master`.

>  static Lisp_Object Vloads_in_progress;
> +/* The same as the above, except it survives the unbinding done in the
> +   event of an error, and can thus be used in error handling.  */
> +Lisp_Object Vloads_still_in_progress;

Please clarify how "it survives the unbinding".

[ BTW, a nice improvement would be to keep track of where we are in the
  outer load when the start the inner load, so that the "While loading"
  message can point to the place where the loading was triggered.  ]


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Here's the amended patch.  It's a lot shorter (and easier) than the
>> previous version.  I would like to commit it as the fix to bug#66912.
>
> Thanks.  I'm not in love with this approach, among other things because
> of the problem I described earlier:
>
>     But I don't think it would be correct in all cases: if file A loads
>     file B which compiles file C which loads file D which signals an
>     error we want the compiler error message to say "error in D loaded
>     from C" and not "error in D loaded from C loaded from B loaded from
>     A".
>
> but it's not the end of the world, so I don't object to installing it on
> `master`.

Also, I think it could report incorrect information in cases such as:

    (condition-case nil
        (load "FILE-A")
      (error ... (forward-sexp) ...))

where an error in the `forward-sexp` might be reported as occurring
"while loading FILE-A" because we didn't revert the
`Vloads_still_in_progress` when unwinding the stack.

I suspect such situations won't occur very often, tho.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Eli Zaretskii]

> Cc: acm@muc.de, 66912@debbugs.gnu.org
> Date: Mon, 4 Nov 2024 21:08:35 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> Here's the amended patch.  It's a lot shorter (and easier) than the
> previous version.  I would like to commit it as the fix to bug#66912.

On master, I presume?  Not on emacs-30, please.

> +DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
> +       Sprefix_load_file_names, 1, 1, 0,
> +       doc: /* Prefix the string BASE_STRING with a message about each
> +file currently being loaded.  Return the resulting string and reset this
> +information to null.  */)

What "this information"?

Btw, why isn't this function marked as internal by double-dash?  Or
did you want to document it?

Thanks.

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hallo, Eli.

On Tue, Nov 05, 2024 at 14:18:11 +0200, Eli Zaretskii wrote:
> > Cc: acm@muc.de, 66912@debbugs.gnu.org
> > Date: Mon, 4 Nov 2024 21:08:35 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > Here's the amended patch.  It's a lot shorter (and easier) than the
> > previous version.  I would like to commit it as the fix to bug#66912.

> On master, I presume?  Not on emacs-30, please.

Yes, on master.  It's not a documentation fix, and it's not a trivial
fix which is bound to be right.

> > +DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
> > +       Sprefix_load_file_names, 1, 1, 0,
> > +       doc: /* Prefix the string BASE_STRING with a message about each
> > +file currently being loaded.  Return the resulting string and reset this
> > +information to null.  */)

> What "this information"?

It's the variable Vloads_still_in_progress which I want to avoid
exposing to the Lisp world.  I think I need to find a better way to
express that.

But I've become a little uneasy about the way I handle that variable,
and think I need to bind it in the command loop.  Please see my (not yet
written) reply to Stefan M's last post.

> Btw, why isn't this function marked as internal by double-dash?  Or
> did you want to document it?

It's used by other libraries, notably the debugger and the byte
compiler.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> +DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
>> +       Sprefix_load_file_names, 1, 1, 0,
>> +       doc: /* Prefix the string BASE_STRING with a message about each
>> +file currently being loaded.  Return the resulting string and reset this
>> +information to null.  */)
>
> What "this information"?

Oh, also, I missed it but: "file currently being loaded" is not right
(by the time we use this function, we're usually not loading the file
any more, because of the error: we already unwound the stack and exited
the `load`s).

> Btw, why isn't this function marked as internal by double-dash?
> Or did you want to document it?

Given its odd calling convention (it strongly depends on timing and on
the current implementation strategy), I'd recommend a double dash
because if we ever care to fix its shortcomings we'd have to change it
in an incompatible way.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Mon, Nov 04, 2024 at 22:27:00 -0500, Stefan Monnier wrote:
> > Here's the amended patch.  It's a lot shorter (and easier) than the
> > previous version.  I would like to commit it as the fix to bug#66912.

> Thanks.  I'm not in love with this approach, among other things because
> of the problem I described earlier:

>     But I don't think it would be correct in all cases: if file A loads
>     file B which compiles file C which loads file D which signals an
>     error we want the compiler error message to say "error in D loaded
>     from C" and not "error in D loaded from C loaded from B loaded from
>     A".

> but it's not the end of the world, so I don't object to installing it on
> `master`.

OK, thanks!

> >  static Lisp_Object Vloads_in_progress;
> > +/* The same as the above, except it survives the unbinding done in the
> > +   event of an error, and can thus be used in error handling.  */
> > +Lisp_Object Vloads_still_in_progress;

> Please clarify how "it survives the unbinding".

It is a global variable which never gets bound, hence when an error is
signalled and the binding stack unwound, it keeps its value.  But you
want me to write this into the comment, not just explain it here.  ;-)

I've become a little uncertain about this mechanism.  Should an error
occur during loading, and the code somehow avoid calling
prefix-load-file-names, perhaps by some obscure setting of `debugger',
Vloads_still_in_progress would stay non-nil, and pollute the next error
message to be output.  I think the default global value of
Vloads_still_in_progress should be Qnil, and I should bind it to itself
each iteration of the command loop, probably in command_loop_1.  That
would protect it from wierd uses of recursive_edit, and avoid the need
to reset Vloads_still_in_progress in prefix-load-file-names.

What do you (or Eli) think?

> [ BTW, a nice improvement would be to keep track of where we are in the
>   outer load when the start the inner load, so that the "While loading"
>   message can point to the place where the loading was triggered.  ]

I think I would agree, but this would be difficult for .el[cn] files.
How about giving the line number just for .el files, something like:

While loading foo.el line 42... While loading bar.elc... While loading baz.el... \n
Wrong type argument: listp, baz

I'm sure this wouldn't be too difficult.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > Here's the amended patch.  It's a lot shorter (and easier) than the
>> > previous version.  I would like to commit it as the fix to bug#66912.
>
>> Thanks.  I'm not in love with this approach, among other things because
>> of the problem I described earlier:
>
>>     But I don't think it would be correct in all cases: if file A loads
>>     file B which compiles file C which loads file D which signals an
>>     error we want the compiler error message to say "error in D loaded
>>     from C" and not "error in D loaded from C loaded from B loaded from
>>     A".
>
>> but it's not the end of the world, so I don't object to installing it on
>> `master`.
>
> OK, thanks!
>
>> >  static Lisp_Object Vloads_in_progress;
>> > +/* The same as the above, except it survives the unbinding done in the
>> > +   event of an error, and can thus be used in error handling.  */
>> > +Lisp_Object Vloads_still_in_progress;
>
>> Please clarify how "it survives the unbinding".
>
> It is a global variable which never gets bound, hence when an error is
> signalled and the binding stack unwound, it keeps its value.  But you
> want me to write this into the comment, not just explain it here.  ;-)
>
> I've become a little uncertain about this mechanism.  Should an error
> occur during loading, and the code somehow avoid calling
> prefix-load-file-names, perhaps by some obscure setting of `debugger',
> Vloads_still_in_progress would stay non-nil, and pollute the next error
> message to be output.  I think the default global value of
> Vloads_still_in_progress should be Qnil, and I should bind it to itself
> each iteration of the command loop, probably in command_loop_1.  That
> would protect it from wierd uses of recursive_edit, and avoid the need
> to reset Vloads_still_in_progress in prefix-load-file-names.
>
> What do you (or Eli) think?

BTW, stepping back a little I wonder if we shouldn't do it a bit
differently:

In `signal_or_quit`, stash the current value of `Vload_in_progress` into
the global variable `Vload_in_progress_of_last_error`.

So the doc doesn't need to explain how `Vload_in_progress_of_last_error`
"survives unbinding", it can just say it's the info that was current
when the last error was signaled.

It may still hold "the wrong info" if by bad luck some other error
occurred between the error in which you're interested and the moment you
can read `Vload_in_progress_of_last_error`, a bit like when
another regexp-match occurs before you use "your match"'s `match-data`,
but at least semantically, I think it is simpler to explain and
understand than your current code.

We could arguably try and generalize this to hold more data than just
the loads in progress, e.g. merging it with the current
`Vsignaling_function`, so call it `dynamic-context-of-last-error`
or something.

> I think I would agree, but this would be difficult for .el[cn] files.
> How about giving the line number just for .el files, something like:

That's what I meant, yes.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

Here are some of my thoughts on your new idea.  They probably seem a bit
incoherent, for which I apologise.  I'm trying to think up all the
problems we might have with it first, before trying to implement it.
That will hopefully reduce frustration and hassle.

On Tue, Nov 05, 2024 at 14:00:43 -0500, Stefan Monnier wrote:

> >> >  static Lisp_Object Vloads_in_progress;
> >> > +/* The same as the above, except it survives the unbinding done in the
> >> > +   event of an error, and can thus be used in error handling.  */
> >> > +Lisp_Object Vloads_still_in_progress;

> >> Please clarify how "it survives the unbinding".

> > It is a global variable which never gets bound, hence when an error is
> > signalled and the binding stack unwound, it keeps its value.  But you
> > want me to write this into the comment, not just explain it here.  ;-)

> > I've become a little uncertain about this mechanism.  Should an error
> > occur during loading, and the code somehow avoid calling
> > prefix-load-file-names, perhaps by some obscure setting of `debugger',
> > Vloads_still_in_progress would stay non-nil, and pollute the next error
> > message to be output.  I think the default global value of
> > Vloads_still_in_progress should be Qnil, and I should bind it to itself
> > each iteration of the command loop, probably in command_loop_1.  That
> > would protect it from wierd uses of recursive_edit, and avoid the need
> > to reset Vloads_still_in_progress in prefix-load-file-names.

> > What do you (or Eli) think?

> BTW, stepping back a little I wonder if we shouldn't do it a bit
> differently:

> In `signal_or_quit`, stash the current value of `Vloads_in_progress` into
> the global variable `Vloads_in_progress_of_last_error`.

OK.  This new variable would then have a guaranteed accurate copy of
Vl_i_p.  Should signal_or_quit end up re-invoking itself (as happens, for
example, when there's a throw to an un-caught symbol, when signal_or_quit
throws Qt to Qtop_level), the value of Vl_i_p won't have changed, so
setting it twice won't matter.  At least, I can't at the moment see how
it would matter.

> So the doc doesn't need to explain how
> `Vloads_in_progress_of_last_error` "survives unbinding", it can just
> say it's the info that was current when the last error was signaled.

That would be a good thing, yes.

There needs to be some mechanism for resetting this variable to Qnil,
should the error handler fail to access it.  Possibly by binding it
somewhere where it's binding will be undone before the next command.  Or
maybe not, if an error handler is always preceded by setting
Vl_i_p_of_last_error for that same error.

Also, we need to be careful about condition-cases in debuggers, where an
error, even if handled, might overwrite Vloads_in_progress_of_last_error.
debug.el and edebug.el both contain condition-cases.  We might not like a
necessity to access Vl_i_p_of_last_error before evaluating any
condition-cases.  That would appear to be inviting trouble.

As a wild idea, we could bind some flag to Qt in Fload, and only set
Vloads_in_progress_of_last_error if the flag is still set.  We would
clear the flag at the same time as setting that variable, thus protecting
it against the next condition-case or whatever.  Possibly we could use
Vl_i_p_of_last_error itself, by setting it to t near the start of Fload.

However, signal_or_quit is supposed to be a general low-level function,
and it doesn't seem ideal to bloat it out with special purpose tests.

Another possible problem with this wild idea is that debuggers, or parts
of them, might need to be autoloaded.  This shouldn't be a problem with
edebug.el, where the file would be loaded to be able to instrument a
form, but I don't think anything's loading debug.el before it gets used.

> It may still hold "the wrong info" if by bad luck some other error
> occurred between the error in which you're interested and the moment you
> can read `Vload_in_progress_of_last_error`, a bit like when
> another regexp-match occurs before you use "your match"'s `match-data`,
> but at least semantically, I think it is simpler to explain and
> understand than your current code.

I think that could only happen if there were a bug in a debugger.

I think your idea might be better than my current patch, but I doubt it's
going to be much simpler.

> We could arguably try and generalize this to hold more data than just
> the loads in progress, e.g. merging it with the current
> `Vsignaling_function`, so call it `dynamic-context-of-last-error`
> or something.

Let's get it working first.  ;-)

> > I think I would agree, but this would be difficult for .el[cn] files.
> > How about giving the line number just for .el files, something like:

> That's what I meant, yes.

OK, I'll think about that, but after we've got the basics (see above)
sorted out for this bug.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> There needs to be some mechanism for resetting this variable to Qnil,
> should the error handler fail to access it.

I was thinking we wouldn't bother, just like the we don't bother
emptying the match-data after we used it.

> Also, we need to be careful about condition-cases in debuggers, where an
> error, even if handled, might overwrite Vloads_in_progress_of_last_error.

Yes, we may want to locally let-bind the var to preserve it, similarly
to the occasional use of `save-match-data`.

> However, signal_or_quit is supposed to be a general low-level function,
> and it doesn't seem ideal to bloat it out with special purpose tests.

Indeed, which is also why I was pointing out that the thing could be
generalized to other context information, so we don't get fixated on `load`.

> Another possible problem with this wild idea is that debuggers, or parts
> of them, might need to be autoloaded.  This shouldn't be a problem with
> edebug.el, where the file would be loaded to be able to instrument a
> form, but I don't think anything's loading debug.el before it gets used.

If the let-binding that preserves the new var is performed around the
call to the debugger (rather than being performed inside the debugger
itself), we should be OK.

> I think your idea might be better than my current patch, but I doubt
> it's going to be much simpler.

The thing I like is that it should be a better replacement for
`Vsignaling_function` (which is currently a very low-tech feature which
works only half the time).

>> > I think I would agree, but this would be difficult for .el[cn] files.
>> > How about giving the line number just for .el files, something like:
>> That's what I meant, yes.
> OK, I'll think about that, but after we've got the basics (see above)
> sorted out for this bug.

Yes, there's no hurry.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

I've implemented your suggestion, and it mostly works.  Let me know if
you'd like to see the patch (for which I'd have to tidy up the source
code).  But ....

On Tue, Nov 05, 2024 at 18:20:03 -0500, Stefan Monnier wrote:
> > There needs to be some mechanism for resetting this variable to Qnil,
> > should the error handler fail to access it.

> I was thinking we wouldn't bother, just like the we don't bother
> emptying the match-data after we used it.

I'm getting problems when debug-on-error is non-nil, and a *Backtrace*
is on the screen.  At this stage, Vloads_in_progress is still non-nil.
So when another error occurs, even a trivial one like "next-line: End of
buffer", its error message gets prefixed by the "While loading ..."
strings.

This is a Bad Thing.  Do you have any suggestions for a fix?  Things
like artificially setting or binding Vloads_in_progress to nil don't
seem like proper fixes.

> > Also, we need to be careful about condition-cases in debuggers, where an
> > error, even if handled, might overwrite Vloads_in_progress_of_last_error.

> Yes, we may want to locally let-bind the var to preserve it, similarly
> to the occasional use of `save-match-data`.

I don't think this is a problem any more.  I only set
Vloads_in_progress_at_error (as I've renamed it as) when there's an
actual error, not the handling of a condition-case thing, or the like.

> > However, signal_or_quit is supposed to be a general low-level function,
> > and it doesn't seem ideal to bloat it out with special purpose tests.

> Indeed, which is also why I was pointing out that the thing could be
> generalized to other context information, so we don't get fixated on `load`.

That wasn't quite the sense I meant it in.  ;-)

> > Another possible problem with this wild idea is that debuggers, or parts
> > of them, might need to be autoloaded.  This shouldn't be a problem with
> > edebug.el, where the file would be loaded to be able to instrument a
> > form, but I don't think anything's loading debug.el before it gets used.

> If the let-binding that preserves the new var is performed around the
> call to the debugger (rather than being performed inside the debugger
> itself), we should be OK.

> > I think your idea might be better than my current patch, but I doubt
> > it's going to be much simpler.

I've tried it, and the above problem seems definitely to make it less
simple than the approach I originally took (which currently works).

Maybe we should go back to my original working approach.

> The thing I like is that it should be a better replacement for
> `Vsignaling_function` (which is currently a very low-tech feature which
> works only half the time).

OK, I haven't looked at that.

> >> > I think I would agree, but this would be difficult for .el[cn] files.
> >> > How about giving the line number just for .el files, something like:
> >> That's what I meant, yes.
> > OK, I'll think about that, but after we've got the basics (see above)
> > sorted out for this bug.

> Yes, there's no hurry.

Indeed not.  The bug has just passed its 1-year anniversary.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I was thinking we wouldn't bother, just like the we don't bother
>> emptying the match-data after we used it.
> I'm getting problems when debug-on-error is non-nil, and a *Backtrace*
> is on the screen.  At this stage, Vloads_in_progress is still non-nil.

Right, because we're still in the process of loading those files.

> So when another error occurs, even a trivial one like "next-line: End of
> buffer", its error message gets prefixed by the "While loading ..."
> strings.

Of course.

This is an instance of the problem I described as follows:

    But I don't think it would be correct in all cases: if file A loads
    file B which compiles file C which loads file D which signals an
    error we want the compiler error message to say "error in D loaded
    from C" and not "error in D loaded from C loaded from B loaded from A".

> This is a Bad Thing.  Do you have any suggestions for a fix?

Other than changing to another approach (e.g. one that incrementally
adds the context info via `handler-bind`s so you only get the context
that found "between" the error and the place where we actually handle
the error), I think the easy solution is to consider not the whole
`Vloads_in_progress_of_last_error` but only the part which is not still
present in `Vloads_in_progress`.

Something like

    (while (not (equal Vloads_in_progress_of_last_error Vloads_in_progress))
      (setq msg (concat msg " while loading "
                        (pop Vloads_in_progress_of_last_error))))

> I've tried it, and the above problem seems definitely to make it less
> simple than the approach I originally took (which currently works).

AFAIK, your previous approach suffered from the exact same problem, tho
maybe in fewer cases.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Wed, Nov 06, 2024 at 13:51:05 -0500, Stefan Monnier wrote:
> >> I was thinking we wouldn't bother, just like the we don't bother
> >> emptying the match-data after we used it.
> > I'm getting problems when debug-on-error is non-nil, and a *Backtrace*
> > is on the screen.  At this stage, Vloads_in_progress is still non-nil.

> Right, because we're still in the process of loading those files.

> > So when another error occurs, even a trivial one like "next-line: End of
> > buffer", its error message gets prefixed by the "While loading ..."
> > strings.

> Of course.

Of course?  You foresaw this and didn't tell me about it.  :-)

> This is an instance of the problem I described as follows:

>     But I don't think it would be correct in all cases: if file A loads
>     file B which compiles file C which loads file D which signals an
>     error we want the compiler error message to say "error in D loaded
>     from C" and not "error in D loaded from C loaded from B loaded from A".

No, I don't think so.  Loading a file which compiles another file is
somewhat unusual, to say the least.  I don't recall seeing it anywhere
in Emacs.  What the message would say in such a circumstance is "While
loading A... While loading B... While loading D... \n(Wrong type
argument listp baz)".  This is accurate, though maybe not the whole
story in the case where there's a compilation in the middle of all that
lot.  This case is probably rare enough not to matter.

> > This is a Bad Thing.  Do you have any suggestions for a fix?

> Other than changing to another approach (e.g. one that incrementally
> adds the context info via `handler-bind`s so you only get the context
> that found "between" the error and the place where we actually handle
> the error), ....

I don't really understand that at the moment.  I'll need to think about
it.

> .... I think the easy solution is to consider not the whole
> `Vloads_in_progress_of_last_error` but only the part which is not
> still present in `Vloads_in_progress`.

In either case, the simplicity of the approach, which was meant to
recommend it, has disappeared, being replaced by appreciable complexity.

> Something like

>     (while (not (equal Vloads_in_progress_of_last_error Vloads_in_progress))
>       (setq msg (concat msg " while loading "
>                         (pop Vloads_in_progress_of_last_error))))

This doesn't make sense in the current implementation (sorry for not
sending you a patch), because Vloads_in_progress_at_error is copied from
Vloads_in_progress in signal_or_quit.

> > I've tried it, and the above problem seems definitely to make it less
> > simple than the approach I originally took (which currently works).

> AFAIK, your previous approach suffered from the exact same problem, tho
> maybe in fewer cases.

No, it doesn't.  There, Vloads_still_in_progress is kept in synch with
Vloads_in_progress when Fload operations start and end.  When the
debugger or error handler outputs a message using Vl_still_i_p, it
resets that variable to nil, preventing it getting output again.

That strategem isn't available in the newer approach, where
Vl_i_p_at_error gets "refreshed" at each error occurring in the
debugger's recursive edit.

At the moment, I think my currently working solution is the best way to
go.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>     (while (not (equal Vloads_in_progress_of_last_error Vloads_in_progress))
>>       (setq msg (concat msg " while loading "
>>                         (pop Vloads_in_progress_of_last_error))))
>
> This doesn't make sense in the current implementation (sorry for not
> sending you a patch), because Vloads_in_progress_at_error is copied from
> Vloads_in_progress in signal_or_quit.

I don't understand why you say it doesn't make sense:

An error in signaled from with a => B, so in `signal_or_quit` you copy
`Vloads_in_progress` which contains `("B" "A")` to
`Vloads_in_progress_at_error` and then you throw the error at the
condition-case which was installed (say) from within the file A so
when you reach the handler, `Vloads_in_progress` will have been reset to
`("A")` so the above loop will correctly say that the error occurred
from within B.

>> > I've tried it, and the above problem seems definitely to make it less
>> > simple than the approach I originally took (which currently works).
>
>> AFAIK, your previous approach suffered from the exact same problem, tho
>> maybe in fewer cases.
>
> No, it doesn't.  There, Vloads_still_in_progress is kept in synch with
> Vloads_in_progress when Fload operations start and end.  When the
> debugger or error handler outputs a message using Vl_still_i_p, it
> resets that variable to nil, preventing it getting output again.

You may be right that the "fewer" case are so few so that there really
aren't any.  I'm not convinced there cannot be a code path that happens
not to pass via those settings to nil, but maybe you're right.
Still, my A => B => compile => C => D examples still stands, which is
still at heart the same problem IMO, and could be worked around with my
`while` loop above.

> At the moment, I think my currently working solution is the best way
> to go.

[ As you can guess, I disagree, mostly because I think the problems of
  my suggested approach are "familiar", since the behavior can be
  compared to things like the match-data, so we know how to live with
  its shortcomings, whereas I can't think of something else we already
  have which follows an approach like yours, so we're in new territory
  and it's harder to see what the problems could be and what workarounds
  to use.  ]

Then you'd be better get cracking at the documentation of "survives
unbinding" 🙂


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Wed, Nov 06, 2024 at 18:14:18 -0500, Stefan Monnier wrote:
> >>     (while (not (equal Vloads_in_progress_of_last_error Vloads_in_progress))
> >>       (setq msg (concat msg " while loading "
> >>                         (pop Vloads_in_progress_of_last_error))))

> > This doesn't make sense in the current implementation (sorry for not
> > sending you a patch), because Vloads_in_progress_at_error is copied from
> > Vloads_in_progress in signal_or_quit.

> I don't understand why you say it doesn't make sense:

> An error in signaled from with a => B, so in `signal_or_quit` you copy
> `Vloads_in_progress` which contains `("B" "A")` to
> `Vloads_in_progress_at_error` and then you throw the error at the
> condition-case which was installed (say) from within the file A so
> when you reach the handler, `Vloads_in_progress` will have been reset to
> `("A")` so the above loop will correctly say that the error occurred
> from within B.

OK, I see what you mean.  But if the error gets handled in a handler-bind
handler, or goes through to the debugger without being intercepted by a
condition-case, the binding stack will not have been unwound.  In that
case the difference between the two lists would be empty.

> >> > I've tried it, and the above problem seems definitely to make it less
> >> > simple than the approach I originally took (which currently works).

> >> AFAIK, your previous approach suffered from the exact same problem, tho
> >> maybe in fewer cases.

> > No, it doesn't.  There, Vloads_still_in_progress is kept in synch with
> > Vloads_in_progress when Fload operations start and end.  When the
> > debugger or error handler outputs a message using Vl_still_i_p, it
> > resets that variable to nil, preventing it getting output again.

> You may be right that the "fewer" case are so few so that there really
> aren't any.  I'm not convinced there cannot be a code path that happens
> not to pass via those settings to nil, but maybe you're right.
> Still, my A => B => compile => C => D examples still stands, which is
> still at heart the same problem IMO, and could be worked around with my
> `while` loop above.

One way to fix this would be to make Vloads_still_in_progress visible to
Lisp, and to bind it to nil in the byte compiler.  But that might get a
bit complicated.

> > At the moment, I think my currently working solution is the best way
> > to go.

> [ As you can guess, I disagree, mostly because I think the problems of
>   my suggested approach are "familiar", since the behavior can be
>   compared to things like the match-data, so we know how to live with
>   its shortcomings, whereas I can't think of something else we already
>   have which follows an approach like yours, ....

Something very similar, if not the same, was the original handling of
byte-compile-form-stack.  There a form was pushed onto the stack at the
start of a byte-compile- function, and popped off again at its end,
without using binding.  In the case of an error, where the binding stack
gets unwound, byte-compile-form-stack retained its value at the time of
the error.  It worked without trouble (as far as I'm aware).

>   .... so we're in new territory and it's harder to see what the
>   problems could be and what workarounds to use.  ]

> Then you'd be better get cracking at the documentation of "survives
> unbinding" 🙂

I will do that.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello again, Stefan.

On Wed, Nov 06, 2024 at 18:14:18 -0500, Stefan Monnier wrote:

[ .... ]

> > At the moment, I think my currently working solution is the best way
> > to go.

> [ As you can guess, I disagree, mostly because I think the problems of
>   my suggested approach are "familiar", since the behavior can be
>   compared to things like the match-data, so we know how to live with
>   its shortcomings, whereas I can't think of something else we already
>   have which follows an approach like yours, so we're in new territory
>   and it's harder to see what the problems could be and what workarounds
>   to use.  ]

> Then you'd be better get cracking at the documentation of "survives
> unbinding" 🙂

DONE.

Here is, perhaps, the final version of my patch, containing the revised
doc comment for Vloads_still_in_progress, and for
Fprefix_loads_in_progress.

Maybe, just maybe, I can commit this as the fix to bug#66912.



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index f058fc48cc7..776083468c6 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1436,8 +1436,9 @@ byte-compile-report-error
 when printing the error message."
   (setq byte-compiler-error-flag t)
   (byte-compile-log-warning
-   (if (stringp error-info) error-info
-     (error-message-string error-info))
+   (prefix-load-file-names
+    (if (stringp error-info) error-info
+      (error-message-string error-info)))
    fill :error))
 \f
 ;;; sanity-checking arglists
diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index ec947c1215d..4068daf6614 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -418,7 +418,9 @@ debugger--insert-header
     ;; Debugger entered for an error.
     ('error
      (insert "--Lisp error: ")
-     (insert (backtrace-print-to-string (nth 1 args)))
+     (insert
+      (prefix-load-file-names
+       (backtrace-print-to-string (nth 1 args))))
      (insert ?\n))
     ;; debug-on-call, when the next thing is an eval.
     ('t
@@ -426,8 +428,10 @@ debugger--insert-header
     ;; User calls debug directly.
     (_
      (insert ": ")
-     (insert (backtrace-print-to-string (if (eq (car args) 'nil)
-                                            (cdr args) args)))
+     (insert
+      (prefix-load-file-names
+       (backtrace-print-to-string (if (eq (car args) 'nil)
+                                      (cdr args) args))))
      (insert ?\n))))
 
 \f
diff --git a/src/keyboard.c b/src/keyboard.c
index 6d28dca9aeb..5d615233f15 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1096,7 +1096,7 @@ DEFUN ("command-error-default-function", Fcommand_error_default_function,
 	  Fdiscard_input ();
 	  bitch_at_user ();
 	}
-
+      context = Fprefix_load_file_names (context);
       print_error_message (data, Qt, SSDATA (context), signal);
     }
   return Qnil;
@@ -1109,6 +1109,11 @@ DEFUN ("command-error-default-function", Fcommand_error_default_function,
    This level has the catches for exiting/returning to editor command loop.
    It returns nil to exit recursive edit, t to abort it.  */
 
+static void restore_Vloads_still_in_progress (Lisp_Object arg)
+{
+  Vloads_still_in_progress = arg;
+}
+
 Lisp_Object
 command_loop (void)
 {
@@ -1137,6 +1142,10 @@ command_loop (void)
   else
     while (1)
       {
+	specpdl_ref ccount = SPECPDL_INDEX ();
+
+	record_unwind_protect (restore_Vloads_still_in_progress,
+			       Vloads_still_in_progress);
 	internal_catch (Qtop_level, top_level_1, Qnil);
 	internal_catch (Qtop_level, command_loop_2, Qerror);
 	executing_kbd_macro = Qnil;
@@ -1144,6 +1153,7 @@ command_loop (void)
 	/* End of file in -batch run causes exit here.  */
 	if (noninteractive)
 	  Fkill_emacs (Qt, Qnil);
+	unbind_to (ccount, Qnil);
       }
 }
 
diff --git a/src/lisp.h b/src/lisp.h
index 5ef97047f76..e7eecba96f6 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4794,6 +4794,7 @@ #define FLOAT_TO_STRING_BUFSIZE 350
   ATTRIBUTE_FORMAT_PRINTF (5, 0);
 
 /* Defined in lread.c.  */
+extern Lisp_Object Vloads_still_in_progress;
 extern Lisp_Object intern_1 (const char *, ptrdiff_t);
 extern Lisp_Object intern_c_string_1 (const char *, ptrdiff_t);
 extern Lisp_Object intern_driver (Lisp_Object, Lisp_Object, Lisp_Object);
diff --git a/src/lread.c b/src/lread.c
index ea0398196e3..234f3b1cd25 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -234,9 +234,17 @@ #define USE_ANDROID_ASSETS
 
 /* A list of file names for files being loaded in Fload.  Used to
    check for recursive loads.  */
-
 static Lisp_Object Vloads_in_progress;
 
+/* The same as the above, but is bound only in the command loop, not in
+   Fload.  It is set to the value of Floads_in_progress at the start and
+   end of Fload.  In the event of an error being signaled and the
+   binding stack being unwound, this variable keeps its value.  Its sole
+   use is to supplement error messages, giving the names of the stack of
+   files currently being loaded.  See Fprefix_load_file_names in this
+   file.  */
+Lisp_Object Vloads_still_in_progress;
+
 static int read_emacs_mule_char (int, int (*) (int, Lisp_Object),
                                  Lisp_Object);
 
@@ -1271,6 +1279,34 @@ close_file_unwind_android_fd (void *ptr)
 
 #endif
 
+DEFUN ("prefix-load-file-names", Fprefix_load_file_names,
+       Sprefix_load_file_names, 1, 1, 0,
+       doc: /* Prefix the string BASE_STRING with a message about each
+file currently being loaded.  Return the resulting string and reset the
+internal variable holding this information to nil.  */)
+  (Lisp_Object base_string)
+{
+  if (NILP (Vloads_still_in_progress))
+    return base_string;
+  else
+    {
+      Lisp_Object result = build_string ("");
+      Lisp_Object while_loading = build_string ("While loading ");
+      Lisp_Object ellipsis = build_string ("... ");
+
+      while (!NILP (Vloads_still_in_progress))
+	{
+	  result = concat2 (concat3 (while_loading,
+				     Fcar (Vloads_still_in_progress),
+				     ellipsis),
+			    result);
+	  Vloads_still_in_progress = Fcdr (Vloads_still_in_progress);
+	}
+      result = concat3 (result, build_string ("\n"), base_string);
+      return result;
+    }
+}
+
 DEFUN ("load", Fload, Sload, 1, 5, 0,
        doc: /* Execute a file of Lisp code named FILE.
 First try FILE with `.elc' appended, then try with `.el', then try
@@ -1516,6 +1552,7 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
 	signal_error ("Recursive load", Fcons (found, Vloads_in_progress));
     record_unwind_protect (record_load_unwind, Vloads_in_progress);
     Vloads_in_progress = Fcons (found, Vloads_in_progress);
+    Vloads_still_in_progress = Vloads_in_progress;
   }
 
   /* All loads are by default dynamic, unless the file itself specifies
@@ -1615,7 +1652,9 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
 	  val = call4 (Vload_source_file_function, found, hist_file_name,
 		       NILP (noerror) ? Qnil : Qt,
 		       (NILP (nomessage) || force_load_messages) ? Qnil : Qt);
-	  return unbind_to (count, val);
+	  unbind_to (count, Qnil);
+	  Vloads_still_in_progress = Vloads_in_progress;
+	  return val;
 	}
     }
 
@@ -1741,6 +1780,8 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
   if (!NILP (Ffboundp (Qdo_after_load_evaluation)))
     call1 (Qdo_after_load_evaluation, hist_file_name) ;
 
+  Vloads_still_in_progress = Vloads_in_progress;
+
   for (int i = 0; i < ARRAYELTS (saved_strings); i++)
     {
       xfree (saved_strings[i].string);
@@ -5772,6 +5813,7 @@ init_lread (void)
   Vload_true_file_name = Qnil;
   Vstandard_input = Qt;
   Vloads_in_progress = Qnil;
+  Vloads_still_in_progress = Qnil;
 }
 
 /* Print a warning that directory intended for use USE and with name
@@ -5819,6 +5861,7 @@ syms_of_lread (void)
   defsubr (&Sintern_soft);
   defsubr (&Sunintern);
   defsubr (&Sget_load_suffixes);
+  defsubr (&Sprefix_load_file_names);
   defsubr (&Sload);
   defsubr (&Seval_buffer);
   defsubr (&Seval_region);
@@ -6138,6 +6181,8 @@ syms_of_lread (void)
 
   Vloads_in_progress = Qnil;
   staticpro (&Vloads_in_progress);
+  Vloads_still_in_progress = Qnil;
+  staticpro (&Vloads_still_in_progress);
 
   DEFSYM (Qhash_table, "hash-table");
   DEFSYM (Qdata, "data");


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> OK, I see what you mean.  But if the error gets handled in a handler-bind
> handler, or goes through to the debugger without being intercepted by a
> condition-case, the binding stack will not have been unwound.  In that
> case the difference between the two lists would be empty.

Indeed, these are cases where the error hasn't reached its corresponding
handler yet.  Do you think it's a problem?

>> >> > I've tried it, and the above problem seems definitely to make it less
>> >> > simple than the approach I originally took (which currently works).
>
>> >> AFAIK, your previous approach suffered from the exact same problem, tho
>> >> maybe in fewer cases.
>
>> > No, it doesn't.  There, Vloads_still_in_progress is kept in synch with
>> > Vloads_in_progress when Fload operations start and end.  When the
>> > debugger or error handler outputs a message using Vl_still_i_p, it
>> > resets that variable to nil, preventing it getting output again.
>
>> You may be right that the "fewer" case are so few so that there really
>> aren't any.  I'm not convinced there cannot be a code path that happens
>> not to pass via those settings to nil, but maybe you're right.
>> Still, my A => B => compile => C => D examples still stands, which is
>> still at heart the same problem IMO, and could be worked around with my
>> `while` loop above.
>
> One way to fix this would be to make Vloads_still_in_progress visible to
> Lisp, and to bind it to nil in the byte compiler.  But that might get a
> bit complicated.

And if an error in D gets handled in A, you'd have lost part of the "A
=> B => compile => C => D" context information because the rebinding to
nil would cause `Vloads_still_in_progress_at_error` to contains only
the "C => D" part.

> Something very similar, if not the same, was the original handling of
> byte-compile-form-stack.

Something only you worked with, AFAICT.  So it doesn't have the same
"known issues" advantage for the rest of us.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Thu, Nov 07, 2024 at 11:09:19 -0500, Stefan Monnier wrote:
> > OK, I see what you mean.  But if the error gets handled in a handler-bind
> > handler, or goes through to the debugger without being intercepted by a
> > condition-case, the binding stack will not have been unwound.  In that
> > case the difference between the two lists would be empty.

> Indeed, these are cases where the error hasn't reached its corresponding
> handler yet.  Do you think it's a problem?

Yes.  When the debugger handles the error, the binding stack hasn't been
unwound at all, so Vloads_in_progress and Vloads_in_progress_at_error are
EQ.  So the difference between them would be empty.

> >> >> > I've tried it, and the above problem seems definitely to make it less
> >> >> > simple than the approach I originally took (which currently works).

> >> >> AFAIK, your previous approach suffered from the exact same problem, tho
> >> >> maybe in fewer cases.

> >> > No, it doesn't.  There, Vloads_still_in_progress is kept in synch with
> >> > Vloads_in_progress when Fload operations start and end.  When the
> >> > debugger or error handler outputs a message using Vl_still_i_p, it
> >> > resets that variable to nil, preventing it getting output again.

> >> You may be right that the "fewer" case are so few so that there really
> >> aren't any.  I'm not convinced there cannot be a code path that happens
> >> not to pass via those settings to nil, but maybe you're right.
> >> Still, my A => B => compile => C => D examples still stands, which is
> >> still at heart the same problem IMO, and could be worked around with my
> >> `while` loop above.

> > One way to fix this would be to make Vloads_still_in_progress visible to
> > Lisp, and to bind it to nil in the byte compiler.  But that might get a
> > bit complicated.

> And if an error in D gets handled in A, you'd have lost part of the "A
> => B => compile => C => D" context information because the rebinding to
> nil would cause `Vloads_still_in_progress_at_error` to contains only
> the "C => D" part.

Yes.  As I say, it could get complicated.  I don't think such a binding
in the byte compiler is a good idea, but it's a possibility.

> > Something very similar, if not the same, was the original handling of
> > byte-compile-form-stack.

> Something only you worked with, AFAICT.  So it doesn't have the same
> "known issues" advantage for the rest of us.

Oh, come on, Stefan!  It is a very simple mechanism, easily understood,
and it worked without known error for several years, before being
replaced by something more complicated.  How do you think this mechanism
could go wrong if used again for Vloads_still_in_progress?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Yes.  When the debugger handles the error, the binding stack hasn't been
> unwound at all, so Vloads_in_progress and Vloads_in_progress_at_error are
> EQ.  So the difference between them would be empty.

I understand that, but I don't think it explains why you think it's
a problem.  E.g. when you're in the debugger, you can see the stack
trace which tells you we're loading A, so you don't need to be told
"while loading A" in the error message.

>> > Something very similar, if not the same, was the original handling of
>> > byte-compile-form-stack.
>> Something only you worked with, AFAICT.  So it doesn't have the same
>> "known issues" advantage for the rest of us.
> Oh, come on, Stefan!

I'm just describing the way I see it: I personally don't have a good
intuition of how/when it could misbehave nor how to work around such
cases, whereas I very much do for the approach I propose and AFAICT it's
not just because I proposed it but it's because it follows
a known pattern, so I expect the same will hold for other coders.

Of course it wouldn't be the first time I'd be wrong.  Also, I didn't
say I objected to your approach, I just have a different preference: you
don't have to convince me.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Fri, Nov 08, 2024 at 08:42:29 -0500, Stefan Monnier wrote:
> > Yes.  When the debugger handles the error, the binding stack hasn't been
> > unwound at all, so Vloads_in_progress and Vloads_in_progress_at_error are
> > EQ.  So the difference between them would be empty.

> I understand that, but I don't think it explains why you think it's
> a problem.  E.g. when you're in the debugger, you can see the stack
> trace which tells you we're loading A, so you don't need to be told
> "while loading A" in the error message.

OK, I see what you mean.  I took it for granted that the message should
be the same, regardless of whether it is reported by a debugger or by
the error handler.  You're suggesting that's not really necessary.

There may be some way the message might be output by a handler-bind
handler before the stack gets unwound, but which doesn't end up with a
debugger backtrace.  I haven't really thought that through yet.

Even when the information is in a debugger backtrace, I think there
would be merit in outputting "While loading foo.el... " as part of the
error message.  In the backtrace, the succession of entries for Fload is
going to be dispersed and not all that easy to trace, but is likely to
be one of the first things the person debugging will want to see.

> >> > Something very similar, if not the same, was the original handling of
> >> > byte-compile-form-stack.
> >> Something only you worked with, AFAICT.  So it doesn't have the same
> >> "known issues" advantage for the rest of us.
> > Oh, come on, Stefan!

> I'm just describing the way I see it: I personally don't have a good
> intuition of how/when it could misbehave nor how to work around such
> cases, whereas I very much do for the approach I propose and AFAICT it's
> not just because I proposed it but it's because it follows
> a known pattern, so I expect the same will hold for other coders.

Experience with byte-compile-form-stack suggests it won't misbehave.
It's simplicity should make it easy to think through.

> Of course it wouldn't be the first time I'd be wrong.  Also, I didn't
> say I objected to your approach, I just have a different preference: you
> don't have to convince me.

In the approach where we copy Vloads_in_progress inside signal_or_quit,
we still don't have a reliable way of preventing the "While loading
foo.el... " from getting into other messages where it doesn't belong,
like "end of screen".  You've suggested taking the difference between
Vloads_in_progress at two stages of the error handling.  I'll admit I
haven't tried that out yet, but it feels to me complicated and fragile.

It feels we've lost momentum for this fix.  I still favour my current
version of the patch, which works, and I think is simpler than the
alternative approach.

It would be nice to get this finalised and committed.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I understand that, but I don't think it explains why you think it's
>> a problem.  E.g. when you're in the debugger, you can see the stack
>> trace which tells you we're loading A, so you don't need to be told
>> "while loading A" in the error message.
> OK, I see what you mean.  I took it for granted that the message should
> be the same, regardless of whether it is reported by a debugger or by
> the error handler.

AFAIK the debugger does not emit the "error message" at all, it shows
the error object instead, so it's already different.

And the full info would readily be available from `Vloads_in_progress_at_error`.

>> I'm just describing the way I see it: I personally don't have a good
>> intuition of how/when it could misbehave nor how to work around such
>> cases, whereas I very much do for the approach I propose and AFAICT it's
>> not just because I proposed it but it's because it follows
>> a known pattern, so I expect the same will hold for other coders.
> Experience with byte-compile-form-stack suggests it won't misbehave.
> Its simplicity should make it easy to think through.

AFAICT

    (equal (error-message-string ERROR-OBJECT)
           (error-message-string ERROR-OBJECT))

will not always return t, which I'd consider as a misbehavior.

I don't mean that we need to fix it.  Just that that there *will* be
misbehaviors, because we use a low-tech approach which stashes the info
in a global variable.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Fri, Nov 08, 2024 at 15:26:51 -0500, Stefan Monnier wrote:
> >> I understand that, but I don't think it explains why you think it's
> >> a problem.  E.g. when you're in the debugger, you can see the stack
> >> trace which tells you we're loading A, so you don't need to be told
> >> "while loading A" in the error message.
> > OK, I see what you mean.  I took it for granted that the message should
> > be the same, regardless of whether it is reported by a debugger or by
> > the error handler.

> AFAIK the debugger does not emit the "error message" at all, it shows
> the error object instead, so it's already different.

That doesn't seem to address the point I made above at all.

> And the full info would readily be available from `Vloads_in_progress_at_error`.

Vloads_in_progress_at_error is not available to the user, being an
internal variable.

The problem with the Vloads_in_progress_at_error approach is that its
information gets prefixed to EVERY error message which occurs while the
debugger's recursive edit is in progress.  This is not a Good Thing.
The methods suggested so far to fix this don't as yet seem satisfactory,
and we don't seem to be progressing on this front.

> >> I'm just describing the way I see it: I personally don't have a good
> >> intuition of how/when it could misbehave nor how to work around such
> >> cases, whereas I very much do for the approach I propose and AFAICT it's
> >> not just because I proposed it but it's because it follows
> >> a known pattern, so I expect the same will hold for other coders.
> > Experience with byte-compile-form-stack suggests it won't misbehave.
> > Its simplicity should make it easy to think through.

> AFAICT

>     (equal (error-message-string ERROR-OBJECT)
>            (error-message-string ERROR-OBJECT))

> will not always return t, which I'd consider as a misbehavior.

No.  The proposed mechanism of Vloads_still_in_progress has no influence
on error-message-string.

> I don't mean that we need to fix it.  Just that that there *will* be
> misbehaviors, because we use a low-tech approach which stashes the info
> in a global variable.

It may be low tech, but it's effective.  Its simplicity makes
misbehaviour unlikely.  We didn't see any (that I know of) with
byte-compile-form-stack.

Can we get this bug closed soon, please?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > OK, I see what you mean.  I took it for granted that the message should
>> > be the same, regardless of whether it is reported by a debugger or by
>> > the error handler.
>> AFAIK the debugger does not emit the "error message" at all, it shows
>> the error object instead, so it's already different.
> That doesn't seem to address the point I made above at all.

Then I don't know what "the message" you're referring to.
(or are you referring to some other point you made elsewhere?)

>> And the full info would readily be available from `Vloads_in_progress_at_error`.
> Vloads_in_progress_at_error is not available to the user, being an
> internal variable.

[ I'd make it user-visible, as well Vload_in_progress, but that's not
  actually relevant.  ]

Even if it's not directly exposed to the user the info *is* available,
so we can offer ways to make use of it.

> The problem with the Vloads_in_progress_at_error approach is that its
> information gets prefixed to EVERY error message which occurs while the
> debugger's recursive edit is in progress.  This is not a Good Thing.

If `load-in-progress-at-error` hold (C B A), I'd make the error message
emit either nothing, or just C, or B and C, or A and B and C depending
on where in the call stack we're calling from (using the `while` loop
that compares to `load-in-progress`).

From within the debugger, presumably `load-in-progress` would still be
(C B A), so error messages would usually display no "while loading" at
all, unless the errors themselves occur within a load of some file D,
of course.

Note that this is not just the behavior we happen to get with the hacks
I came upon along the way.  It's the behavior I'm aiming for (e.g. it's
also the behavior that we'd get with the `handler-bind` approach
I advocated earlier).

>> AFAICT
>
>>     (equal (error-message-string ERROR-OBJECT)
>>            (error-message-string ERROR-OBJECT))
>
>> will not always return t, which I'd consider as a misbehavior.
>
> No.  The proposed mechanism of Vloads_still_in_progress has no influence
> on error-message-string.

Sorry, I meant

    (equal (prefix-load-file-names
            (error-message-string ERROR-OBJECT))
           (prefix-load-file-names
            (error-message-string ERROR-OBJECT)))

[ I assumed you had added a call to `prefix-load-file-names` in
  `error-message-string`.  Not sure why you didn't, but I haven't
  thought very much about which option is best in this regard.  ]

> Can we get this bug closed soon, please?

As I said, you don't need to convince me.


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Sat, Nov 09, 2024 at 11:45:36 -0500, Stefan Monnier wrote:
> >> > OK, I see what you mean.  I took it for granted that the message should
> >> > be the same, regardless of whether it is reported by a debugger or by
> >> > the error handler.
> >> AFAIK the debugger does not emit the "error message" at all, it shows
> >> the error object instead, so it's already different.
> > That doesn't seem to address the point I made above at all.

> Then I don't know what "the message" you're referring to.
> (or are you referring to some other point you made elsewhere?)

I simply meant what the user sees on the screen.  If the user sees
"While loading foo.el... \nWrong type argument: listp, baz" whilst doing
something, then enables the debugger and repeats the action, she should
see the same message at the top of the backtrace.  Surely?  Truncating
that message can only lead to confusion.

> >> And the full info would readily be available from `Vloads_in_progress_at_error`.
> > Vloads_in_progress_at_error is not available to the user, being an
> > internal variable.

> [ I'd make it user-visible, as well Vload_in_progress, but that's not
>   actually relevant.  ]

> Even if it's not directly exposed to the user the info *is* available,
> so we can offer ways to make use of it.

I am proposing a single way.

> > The problem with the Vloads_in_progress_at_error approach is that its
> > information gets prefixed to EVERY error message which occurs while the
> > debugger's recursive edit is in progress.  This is not a Good Thing.

> If `load-in-progress-at-error` hold (C B A), I'd make the error message
> emit either nothing, or just C, or B and C, or A and B and C depending
> on where in the call stack we're calling from (using the `while` loop
> that compares to `load-in-progress`).

> >From within the debugger, presumably `load-in-progress` would still be
> (C B A), so error messages would usually display no "while loading" at
> all, unless the errors themselves occur within a load of some file D,
> of course.

> Note that this is not just the behavior we happen to get with the hacks
> I came upon along the way.  It's the behavior I'm aiming for (e.g. it's
> also the behavior that we'd get with the `handler-bind` approach
> I advocated earlier).

I'll go back and see if I can find that.

Again, why do you think this is what we should aim for, rather than
having the same message in the error handler and at the top of a
backtrace?

[ .... ]

> Sorry, I meant

>     (equal (prefix-load-file-names
>             (error-message-string ERROR-OBJECT))
>            (prefix-load-file-names
>             (error-message-string ERROR-OBJECT)))

OK, that would indeed return nil if the first call to p-l-f-names was
the first such call since the error occurred.

> [ I assumed you had added a call to `prefix-load-file-names` in
>   `error-message-string`.  Not sure why you didn't, but I haven't
>   thought very much about which option is best in this regard.  ]

I wasn't actually aware of the function error-message-string.  Seeing as
how it is called from many places (over 100), there doesn't seem to be
any advantage in putting a call to prefix-load-file-names inside it.

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Then I don't know what "the message" you're referring to.
>> (or are you referring to some other point you made elsewhere?)
> I simply meant what the user sees on the screen.  If the user sees
> "While loading foo.el... \nWrong type argument: listp, baz" whilst doing
> something, then enables the debugger and repeats the action, she should
> see the same message at the top of the backtrace.  Surely?  Truncating
> that message can only lead to confusion.

I still don't understand: currently, if you see a message "Wrong
type argument: listp, baz", then enable the debugger and reproduce the
error, you won't see "Wrong type argument: listp, baz" but

    Debugger entered--Lisp error: (wrong-type-argument listp baz)

instead.  IOW, instead of the error message, you see the error object.

> Again, why do you think this is what we should aim for, rather than
> having the same message in the error handler and at the top of a
> backtrace?

Because of my "A => B => compile => C => D" example: the message I want
to have depends on the chain from the point where I catch the error
(i.e. the `condition-case`) to the point where the error is signaled.
And I don't really care about the debugger case.

        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Nov 10, 2024 at 11:45:38 -0500, Stefan Monnier wrote:
> >> Then I don't know what "the message" you're referring to.
> >> (or are you referring to some other point you made elsewhere?)
> > I simply meant what the user sees on the screen.  If the user sees
> > "While loading foo.el... \nWrong type argument: listp, baz" whilst doing
> > something, then enables the debugger and repeats the action, she should
> > see the same message at the top of the backtrace.  Surely?  Truncating
> > that message can only lead to confusion.

> I still don't understand: currently, if you see a message "Wrong
> type argument: listp, baz", then enable the debugger and reproduce the
> error, you won't see "Wrong type argument: listp, baz" but

>     Debugger entered--Lisp error: (wrong-type-argument listp baz)

> instead.  IOW, instead of the error message, you see the error object.

I've never noticed these to be different.  I suspect most users wouldn't
either.  I wasn't talking about the boilerplate and formatting being
different, I was talking about the essence of the message, the
substance.  It is the same in the two examples you have given.

Part of that essence is to be the "While loading foo.el... " bit.  I
think the user should see that, both when the error first occurs, and
when it occurs again after enabling debug-on-error and repeating the
action that caused it.

> > Again, why do you think this is what we should aim for, rather than
> > having the same message in the error handler and at the top of a
> > backtrace?

> Because of my "A => B => compile => C => D" example: the message I want
> to have depends on the chain from the point where I catch the error
> (i.e. the `condition-case`) to the point where the error is signaled.
> And I don't really care about the debugger case.

That is no explanation as to WHY you think it's the right thing.  The
point in that chain where we catch the error is an internal
implementation detail, of no interest to the user.  WHY are you so
unconcerned about the debugger case?  I think it would be consistent to
display "While loading..." the same in both cases.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Because of my "A => B => compile => C => D" example: the message I want
>> to have depends on the chain from the point where I catch the error
>> (i.e. the `condition-case`) to the point where the error is signaled.
>> And I don't really care about the debugger case.
> That is no explanation as to WHY you think it's the right thing.
> The point in that chain where we catch the error is an internal
> implementation detail, of no interest to the user.

Not at all.  In the above example, the error message goes to the
compilation buffer, and it would be weird for that buffer to say "while
loading A while loading B" whereas the compiled code loads neither.

> WHY are you so unconcerned about the debugger case?

In the debugger case, I usually have direct access to the problematic
code and usually don't care that much about the further parts of the
context explaining why that code is executed.
Occasionally I do, but "loading" is not special in this respect: I will
just as often be interested to know which command was used, in which
buffer, or which hook caused that code to be run.

> I think it would be consistent to display "While loading..." the same
> in both cases.

To be honest, I don't see much value in displaying "while loading" in
the echo area in general.  I'm not opposed to it, but the only case
where I know it could be helpful is during compilation (which I'm
mostly interested to see it in the compilation buffer).


        Stefan

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Nov 10, 2024 at 16:37:26 -0500, Stefan Monnier wrote:
> >> Because of my "A => B => compile => C => D" example: the message I want
> >> to have depends on the chain from the point where I catch the error
> >> (i.e. the `condition-case`) to the point where the error is signaled.
> >> And I don't really care about the debugger case.
> > That is no explanation as to WHY you think it's the right thing.
> > The point in that chain where we catch the error is an internal
> > implementation detail, of no interest to the user.

> Not at all.  In the above example, the error message goes to the
> compilation buffer, and it would be weird for that buffer to say "while
> loading A while loading B" whereas the compiled code loads neither.

OK, so you were using "the point in the chain where we catch the error",
which is of no interest to the user, as a proxy for how and where an
error message gets displayed.

I thought we were agreed that the "A => B => compile => C => D" was
vanishingly rare and probably doesn't occur at all in the Emacs sources.
In that case, why are you giving it priority over the other concerns
(such as consistency of messages displayed to the user) that I have
raised?

> > WHY are you so unconcerned about the debugger case?

> In the debugger case, I usually have direct access to the problematic
> code and usually don't care that much about the further parts of the
> context explaining why that code is executed.

But other users might like to see this information.  I certainly would.
The stack of files being loaded is likely to be of interest.  Even you
imply you would sometimes like to see it.

Would you please clarify whether you are opposed to outputting the
information (about the stack of files being loaded) in the debugger
case, or are merely indifferent about it.  If the former, I still don't
understand why; I would still appreciate justification.

> Occasionally I do, but "loading" is not special in this respect: I will
> just as often be interested to know which command was used, in which
> buffer, or which hook caused that code to be run.

It is true that Emacs error messages are typically poor in the extreme,
in that they fail to identify the pertinent command, buffer, hook, or
variable.  I have been trying to improve this for several years, with
mixed success.  The thing about loading is that it is frequently
recursive, typically by the function `require'.  Being told which file
the failing Lisp came from is as good as saying which buffer, and likely
better in batch mode.

> > I think it would be consistent to display "While loading..." the same
> > in both cases.

> To be honest, I don't see much value in displaying "while loading" in
> the echo area in general.

To be frank, displaying nothing but

    Wrong type argument: integer-or-marker-p, nil

to the user is insulting.  It is nearly content-free, giving the user no
clue as to where to start looking for the error.  It implies that the
users' time is less important than that of Emacs developers, who have
failed to implement proper error handling.

Informing the user which file the error occurred in, though far from
ideal, is significantly better than not doing so.

> I'm not opposed to it, but the only case where I know it could be
> helpful is during compilation (which I'm mostly interested to see it
> in the compilation buffer).

In this case, the error message is indeed displayed in *Compile-Log*.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66912: With `require', the byte compiler reports the wrong file for errors.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> OK, so you were using "the point in the chain where we catch the
> error", which is of no interest to the user, as a proxy for how and
> where an error message gets displayed.

I conflate the two, yes.

> I thought we were agreed that the "A => B => compile => C => D" was
> vanishingly rare and probably doesn't occur at all in the Emacs sources.

I like to think of such corner cases because it gives me more clarity
about the general case.  If I focus only on the scenario at hand it's
all too easy to design something that doesn't work well in the
general case.

> In that case, why are you giving it priority over the other concerns
> (such as consistency of messages displayed to the user) that I have
> raised?

Because I don't see the benefit of using "consistent" messages when
the circumstances are very different.

> Would you please clarify whether you are opposed to outputting the
> information (about the stack of files being loaded) in the debugger
> case, or are merely indifferent about it.

I'm indifferent about it.
But I do care about the debugger showing me the error *object* rather
than merely the error *message*.

> It is true that Emacs error messages are typically poor in the extreme,
> in that they fail to identify the pertinent command, buffer, hook, or
> variable.  I have been trying to improve this for several years, with
> mixed success.  The thing about loading is that it is frequently
> recursive, typically by the function `require'.  Being told which file
> the failing Lisp came from is as good as saying which buffer, and likely
> better in batch mode.

That's why I'm thinking of generalizing the `load-in-progress-at-error`
into a `dynamic-context-at-error` where we could stash arbitrarily more
information.  Ideally, error objects would come with a full backtrace,
from which we could extract the stack of pending loads, the name of the
current command, or process filter, or hooks, ...
[ In SML/NJ, exception objects keep a reference to the "stack" for that
  purpose, but of course, that's easy for them because they heap
  allocate their "stack", so it's a simple matter of keeping
  a reference, whereas we'd have to copy our "stack", which could
  slow down `signal` significantly.  ]

>> > I think it would be consistent to display "While loading..." the same
>> > in both cases.
>
>> To be honest, I don't see much value in displaying "while loading" in
>> the echo area in general.
>
> To be frank, displaying nothing but
>
>     Wrong type argument: integer-or-marker-p, nil
>
> to the user is insulting.

Agreed.  At the same time, it corresponds to Python dumping a backtrace
onto the unsuspecting user, or C crashing down with a core dump, so
I think we're in good company.

IOW when the end user sees such an error message, it means we have a bug
in the ELisp code somewhere.  The bug is either in that we should not
trigger this error, or that we should catch it and turn it into an error
that's meaningful in the context of the command where it's executed.

Note that in presence of such a bug, adding "while loading A" won't
really help.
The only two behaviors that would help would be either to drop
into the debugger (so as to help diagnose/find/fix the bug), or to emit
a message explaining to the user that they found a bug (and maybe try
and help them submit a bug report).

Dropping into the debugger is what we get with `debug-on-error`, but
I don't think we can enable it by default, because it's a bit too
delicate to use for some users.  Maybe we should improve that.

> Informing the user which file the error occurred in, though far from
> ideal, is significantly better than not doing so.

IME, the above kind of error usually happens outside of loading files,
so bug#66912 will almost never make a difference for that kind of error.

>> I'm not opposed to it, but the only case where I know it could be
>> helpful is during compilation (which I'm mostly interested to see it
>> in the compilation buffer).
> In this case, the error message is indeed displayed in *Compile-Log*.

🙂


        Stefan