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

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

Thanks for the reply!

On Fri, Oct 11, 2024 at 19:45:18 -0400, Stefan Monnier wrote:
> > (i) Create the following file, bad-error-position2.el:

> > #########################################################################
> > ;; -*- lexical-binding:t -*-
> > (eval-and-compile
> >   (defmacro increase ()
> >     `(let ((foo (point-max)))
> >        (cond
> > 	((consp foo)
> > 	 (message "consp %s" foo)
> > 	 foo)
> > 	((numberp foo)
> > 	 (1+ fooo))			; Note the misspelling.
> > 	(t (message "Something else: %s" foo))))))

> > (defun call-increase (bar)
> >   (cond
> >    ((not (or (consp bar)
> > 	     (numberp bar)))
> >     bar)
> >    (t (increase))))
> > #########################################################################

> > Note the misspelling of `foo' as `fooo' on line 10.

> > (ii) emacs -Q
> > (iii) M-x byte-compile-file /path/to/bad-error-position2.el.
> > (iv) This gives the warning message:

> >   bad-error-position2.el:14:4: Warning: reference to free variable `fooo'

> > (v) The position 14:4 is wrong.  That is the position of the `cond'
> > symbol in `call-increase'.
> > (vi) The correct message should be:

> >   bad-error-position2.el:18:8: Warning: reference to free variable `fooo'

> > ..  18:8 is the position of `increase' on the last line of the file.

> Nitpick: one could also argue that the "correct" message should point to
> line 8 where is the `fooo` typo.

> > +(defun sub-macroexp--posify-form (form call-pos depth)
> > +  "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."
> > +  (let (new-form
> > +        )
> > +    (cond
> > +     ((zerop depth) nil)
> > +     ((and (consp form)
> > +           (symbolp (car form))
> > +           (car form))
> > +      (setcar form (position-symbol (car form) call-pos))
> > +      form)
> > +     ((consp form)
> > +      (or (when (setq new-form (sub-macroexp--posify-form
> > +                                (car form) call-pos (1- depth)))
> > +            (setcar form new-form)
> > +            form)
> > +          (when (setq new-form (sub-macroexp--posify-form
> > +                                (cdr form) call-pos (1- depth)))
> > +            (setcdr form new-form)
> > +            form)))
> > +     ((symbolp form)
> > +      (if form                          ; Don't position nil!
> > +          (position-symbol form call-pos)))
> > +     ((and (or (vectorp form) (recordp form)))
> > +      (let ((len (length form))
> > +            (i 0)
> > +            )
> > +        (while (and (< i len)
> > +                    (not (setq new-form (sub-macroexp--posify-form
> > +                                         (aref form i) call-pos (1- depth)))))
> > +          (setq i (1+ i)))
> > +        (when (< i len)
> > +          (aset form i new-form)
> > +          form))))))

> That sounds potentially costly  Have you tried to measure the
> performance impact in some "typical" cases?

I haven't measured it, no, but I doubt it will be costly - the recursion in
sub-macroexp--posify-form will very rarely occur.  Virtually every form
passed to it will by a macro invocation or a symbol, I think.

> While reading your description I was naively thinking: we can
> probably fix it with a trivial patch like:

>     diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
>     index 19daa57b207..5cc471e32f6 100644
>     --- a/lisp/emacs-lisp/macroexp.el
>     +++ b/lisp/emacs-lisp/macroexp.el
>     @@ -246,6 +246,7 @@ macroexp-macroexpand
>        (let* ((macroexpand-all-environment env)
>               new-form)
>          (while (not (eq form (setq new-form (macroexpand-1 form env))))
>     +      (push form byte-compile-form-stack)
>            (let ((fun (car-safe form)))
>              (setq form
>                    (if (and fun (symbolp fun)

> Have you tried something like this?

I haven't, no.  It might well work.  But I think the `push' form should
go outside of the loop,  I'm also a little wary about pushing an item
onto stack when it doesn't get popped again after the form(s) it is
"guarding".

> If it doesn't work, do you happen to know why?

I'm not sure whether it would work for the (similar) bug, bug#73746,
where the symbols with position were getting lost in byte-opt.el.

But I'll give it a try.  I'm a bit busy in the next few days, so it might
be next week before I manage it.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).