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

[Alan Mackenzie <acm@muc.de>]

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).