bug#73725: Master: Wrong position for byte compiler warning message.

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

Thanks for giving my patch such a thorough review.

On Sun, Oct 20, 2024 at 12:35:46 -0400, Stefan Monnier wrote:
> > I haven't heard anything back in over a week, so I assume my patch for
> > bug#73725 and bug#73746 is OK.  I've committed it.

> Sorry, got pushed too far down my todo while I was busy and forgot about it.

> Side note: thinking some more about the problem you're trying to fix,
> I can't help find it is somewhat funny: it's exactly the kind of
> "position-preservation" work we were hoping to avoid having to do by
> using SWPs.

Well, we always knew that macros would be problematic.

> Anyway, the general approach seems about right.
> See comments below:

> > @@ -510,7 +510,9 @@ byte-optimize-form
> >    (while
> >        (progn
> >          ;; First, optimize all sub-forms of this one.
> > -        (setq form (byte-optimize-form-code-walker form for-effect))
> > +        (setq form
> > +              (macroexp-preserve-posification
> > +               form (byte-optimize-form-code-walker form for-effect)))
> >  
> >          ;; If a form-specific optimizer is available, run it and start over
> >          ;; until a fixpoint has been reached.

> Is this needed?  I'd expect we need `macroexp-preserve-posification`
> only at those places where an optimization returns a form whose `car` is
> different from that of FORM.  So I expect this to happen in more
> specific places inside byte-opt.el than here.

Yes, it is needed.  byte-optimize-form-code-walker sometimes does return
a form with a different car.  For example, a progn form with a single
sub-form comes back without the progn.  Even if there are several
sub-forms, the code uses macroexp-progn to substitute a different progn
symbol without the position.  I don't know why it does this.

One of my ideas was to fix byte-optimize-form-code-walker by fixing each
individual bit where the SWP got lost.  In the end I gave that up as too
much work, and too difficult to test.

> IOW, this deserves a clear comment explaining why we need it here,
> probably with some kind of example.

OK, I'll put one in, along the lines of the above, but less wordy.

> > @@ -519,7 +521,8 @@ byte-optimize-form
> >               (let ((opt (byte-opt--fget (car form) 'byte-optimizer)))
> >                 (and opt
> >                      (let ((old form)
> > -                          (new (funcall opt form)))
> > +                          (new (macroexp-preserve-posification
> > +                                form (funcall opt form))))
> >  	              (byte-compile-log "  %s\t==>\t%s" old new)
> >                        (setq form new)
> >                        (not (eq new old))))))))

> E.g. this is the kind of place where it makes sense to me.

> > +(defun sub-macroexp--posify-form (form call-pos depth)

> Please don't eat up namespace gratuitously.
> IOW stick to the "macroexp-" prefix.

Sorry, I wasn't thinking straight when I named that.  I've already
corrected it in my sources, it will appear in the next patch or commit I
submit.

> > +  "Try to apply the transformation of `macroexp--posify-form' to FORM.
> > +FORM and CALL-POS are as in that function.  DEPTH is a small integer,
> > +decremented at each recursive call, to prevent infinite recursion.
> > +Return the changed form, or nil if no change happened."

> Somewhere in the doc or in a comment we should document the "design",
> which AFAICT is to try and find "one" place where we can put the
> `call-pos` info: the docstring suggests we'll apply it to all the
> symbols within `depth` (which would be both costly and undesirable),
> whereas we actually stop (more or less) as soon as we find a good spot.

Good point.  The doc string of that function is a little clumsy, as it
refers to the doc string of the next function macroexp--posify-form,
since it is the recursive part of that function.  Maybe I should just
write out that of the first function in full, even though that would
duplicate a lot of stuff.

But I'll write somewhere that we modify "a single symbol", or something
like that.

> > +  (let (new-form)
> > +    (cond
> > +     ((zerop depth) nil)
> > +     ((and (consp form)
> > +           (symbolp (car form))
> > +           (car form))
> > +      (setcar form (position-symbol (car form) call-pos))
> > +      form)

> AFAICT this can and occasionally will throw away valuable
> position information: if `(car form)` is an SWP we don't know for sure
> here that `call-pos` is always a better position info than the one
> already carried by `(car form)`.

Thanks.  Such occasions will be when the form returned by the macro has
as a car a SWP whose position is inside the macro.  Such a position
would indeed be a better position for the warning message that the
position of the macro call.

> We could consider combining position information (but this would make
> the whole system more complex: when printing warnings/errors we'd have
> to find a way to make use of the various recorded positions, ...).
> But a simpler choice is to check (not (symbol-with-pos-p (car form))).

I think the simpler choice is the one to go with, here.  ;-)

> > +     ((symbolp form)
> > +      (if form                          ; Don't position nil!
> > +          (position-symbol form call-pos)))

> Same here.

Yes.

> > +(defmacro macroexp-preserve-posification (pos-form &rest body)
> > +  "Evaluate BODY..., posifying the result with POS-FORM's position, if any."

> This lacks a `declare` with `debug` and `indent` specs.

Yes.  Sorry, I'll fix these.

> > +  `(let ((call-pos (cond
> > +                    ((consp ,pos-form)
> > +                     (and (symbol-with-pos-p (car ,pos-form))
> > +                          (symbol-with-pos-pos (car ,pos-form))))
> > +                    ((symbol-with-pos-p ,pos-form)
> > +                     (symbol-with-pos-pos ,pos-form))))
> > +         (new-value (progn ,@body)))
> > +     (if call-pos
> > +         (macroexp--posify-form new-value call-pos)
> > +       new-value)))
> > +
> >  (defun macroexp-macroexpand (form env)
> >    "Like `macroexpand' but checking obsolescence."
> >    (let* ((macroexpand-all-environment env)
> >           new-form)
> > +    (macroexp-preserve-posification
> > +     form
> >      (while (not (eq form (setq new-form (macroexpand-1 form env))))
> > +       (setq macroexpanded t)
> >        (let ((fun (car-safe form)))
> >          (setq form
> >                (if (and fun (symbolp fun)

> This `(setq macroexpanded t)` looks like some leftover code, at least
> I couldn't find this var declared or used elsewhere.

I remember removing it, but can't remember exactly why.  When I byte
compile the code, I don't get an undeclared variable warning for it, for
some reason.

> But yes, I suspect it would make a fair bit of sense to perform the
> "preserve-posification" only when `macroexpand-1` did return a new form.
> Did you try to do it (as suggested by this leftover code) and found it
> was not worth the trouble or that it didn't work?

I think that with the variable `macroexpand', the code was getting a bit
bulky.  But the check for macroexpand-1 returning a new form could
easily be performed in the macro macroexp-preserve-posification.  So
I'll try that, and see what sort of effect it has on the timings.

> If so, please add a comment recording it.

> > @@ -253,7 +316,7 @@
> >                      (if (symbolp (symbol-function fun)) "alias" "macro"))
> >                     new-form (list 'obsolete fun) nil fun)
> >                  new-form))))
> > -    form))
> > +     new-form)))

> Why?

I can't remember.  The change doesn't seem to make sense.

> > -(defun macroexpand-all (form &optional environment)
> > +(defun macroexpand-all (form &optional environment keep-pos)

> Any reason why we need this new argument?
> Can't we just always try to preserve the positions?

There are lots of calls to macroexpand-all (around 37), and I was
concerned about possible accidental side effects in these.  Also, always
trying to preserve the position might slow down compilation, but I
haven't measured that yet.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany)