bug#54079: 29.0.50; Method dispatching eratically fails

[Alan Mackenzie]

Hello, Stefan.

On Tue, Mar 08, 2022 at 14:53:07 -0500, Stefan Monnier wrote:
> > diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
> > index 9be44a8d5a..fb9f70bd67 100644
> > --- a/lisp/emacs-lisp/bytecomp.el
> > +++ b/lisp/emacs-lisp/bytecomp.el
> > @@ -499,9 +499,10 @@ byte-compile-initial-macro-environment
> >                                         (byte-compile-new-defuns
> >                                          byte-compile-new-defuns))
> >                                     (setf result
> > -                                         (byte-compile-eval
> > +                                         (byte-run-strip-symbol-positions
> > +                                          (byte-compile-eval
> >                                             (byte-compile-top-level
> > -                                            (byte-compile-preprocess 
> > form)))))))
> > +                                            (byte-compile-preprocess 
> > form))))))))

> I'd expect the reverse: strip first and then eval the result.
> Why should we not strip the form passed to `byte-compile-eval`?

It's an edge case either way, but the form being evaluated might be a
`byte-compile', in which case it's (much) better to leave the positions
in place during this operation.

> Does `byte-compile-top-level` already return a stripped form of code?

Compiled code is always stripped, at least since the weekend!

> And why bother stripping the result of `byte-compile-eval`?

Because it might be the result of evaluating a defun (or defvar or
defconst).  This was the situation which gave rise to the bug.


> > @@ -512,9 +513,10 @@ byte-compile-initial-macro-environment
> >                                ;; or byte-compile-file-form.
> >                                (let* ((print-symbols-bare t) ; Possibly 
> > redundant binding.
> >                                       (expanded
> > -                                      (macroexpand--all-toplevel
> > -                                       form
> > -                                       macroexpand-all-environment)))
> > +                                      (byte-run-strip-symbol-positions
> > +                                       (macroexpand--all-toplevel
> > +                                        form
> > +                                        macroexpand-all-environment))))
> >                                  (eval expanded lexical-binding)
> >                                  expanded)))))
> >      (with-suppressed-warnings

> Fundamentally, `eval` should always strip before doing its job.

Except when what it's evaluating is a defun, defmacrro, defsubst, etc.
Then it would be better to evaluate SWPs (which would work, since we're
inside a compilation, where enable-symbols-with-pos has been bound).
But here EXPANDED has been stripped before being evaluated, so I'm not
sure what you're saying here.

> Yes, I know, it might be a bit expensive, but we should probably
> define a local function in `bytecomp.el` which does strip+eval and use
> that instead of `eval` (both here and in `byte-compile-eval`).  WDYT?

I don't think stripping is really all that expensive.  There are one or
two .el files in Emacs (ucs-normalize.el springs to mind) which have
very large lists with vectors in them, yet they don't seem noticeably to
slow down the Emacs build.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).