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

From: Stefan Monnier <[email-masked]>
Date: Fri, 11 Oct 2024 19:45:18 -0400

> (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?

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?
If it doesn't work, do you happen to know why?

        Stefan