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

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

[Alan Mackenzie]

Hello, Emacs.

On the master branch.

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

#########################################################################

Diagnosis
---------

When the byte compiler expands the invocation of `increase' on the last
line, `increase' is a symbol with position.  The form returned by the
macro expander, however, has no position on its first symbol, the `let'.

Thus when the warning is being processed, the pertinent entry on
byte-compile-form-stack has no symbols with position, so the code takes
a SWP from a deeper nested form, the `cond' form on line 14, and derives
the warning position from it.

#########################################################################

Proposed fix
------------

To fix this I propose amending the macro expander, such that if the
first symbol in a form being expanded is a SWP, the position is applied
to the expanded form, typically on the car of that form, but possibly
elsewhere if that expanded form isn't a list.

The following patch appears to fix the bug:


diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 29e7882c851..e0a59d19d4e 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2582,14 +2582,17 @@ byte-compile-flush-pending
               byte-compile-jump-tables nil))))
 
 (defun byte-compile-preprocess (form &optional _for-effect)
-  (setq form (macroexpand-all form byte-compile-macro-environment))
+  (let ((call-pos (and (consp form)
+                       (symbol-with-pos-p (car form))
+                       (symbol-with-pos-pos (car form)))))
+    (setq form (macroexpand-all form byte-compile-macro-environment call-pos))
   ;; FIXME: We should run byte-optimize-form here, but it currently does not
   ;; recurse through all the code, so we'd have to fix this first.
   ;; Maybe a good fix would be to merge byte-optimize-form into
   ;; macroexpand-all.
   ;; (if (memq byte-optimize '(t source))
   ;;     (setq form (byte-optimize-form form for-effect)))
-  (cconv-closure-convert form byte-compile-bound-variables))
+    (cconv-closure-convert form byte-compile-bound-variables)))
 
 ;; byte-hunk-handlers cannot call this!
 (defun byte-compile-toplevel-file-form (top-level-form)
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 4524eccc7ef..44d49bd7757 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -237,11 +237,63 @@ macroexpand-1
                 form))))))))
    (t form)))
 
+(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))))))
+
+(defun macroexp--posify-form (form call-pos)
+  "Try to apply the position CALL-POS to the form FORM.
+CALL-POS is a buffer position, a number.  FORM may be any lisp form,
+and is typically the output form returned by macro expansion.
+Apply CALL-POS to FORM as a symbol with position, such that
+`byte-compile--first-symbol-with-pos' can later return it.  Return
+the possibly modified FORM."
+  (let ((new-form (sub-macroexp--posify-form form call-pos 10)))
+    (or new-form form)))
+
 (defun macroexp-macroexpand (form env)
   "Like `macroexpand' but checking obsolescence."
   (let* ((macroexpand-all-environment env)
-         new-form)
+         (call-pos (and (consp form)
+                        (symbol-with-pos-p (car form))
+                        (symbol-with-pos-pos (car form))))
+         macroexpanded new-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)
@@ -252,6 +304,8 @@ macroexp-macroexpand
                     (if (symbolp (symbol-function fun)) "alias" "macro"))
                    new-form (list 'obsolete fun) nil fun)
                 new-form))))
+    (when (and macroexpanded call-pos)
+      (setq form (macroexp--posify-form form call-pos)))
     form))
 
 (defun macroexp--unfold-lambda (form &optional name)
@@ -516,14 +570,22 @@ macroexp--expand-all
             (_ form))))))
 
 ;;;###autoload
-(defun macroexpand-all (form &optional environment)
+(defun macroexpand-all (form &optional environment call-pos)
   "Return result of expanding macros at all levels in FORM.
 If no macros are expanded, FORM is returned unchanged.
 The second optional arg ENVIRONMENT specifies an environment of macro
-definitions to shadow the loaded ones for use in file byte-compilation."
-  (let ((macroexpand-all-environment environment)
-        (macroexp--dynvars macroexp--dynvars))
-    (macroexp--expand-all form)))
+definitions to shadow the loaded ones for use in file byte-compilation.
+CALL-POS, if non-nil, is a buffer position which will be incorporated
+into the result form as a symbol with position, later to be usable by
+`byte-compile--warning-source-offset'."
+  (let*
+      ((macroexpand-all-environment environment)
+        (macroexp--dynvars macroexp--dynvars)
+        (new-form
+         (macroexp--expand-all form)))
+    (if call-pos
+        (macroexp--posify-form new-form call-pos)
+      new-form)))
 
 ;; This function is like `macroexpand-all' but for use with top-level
 ;; forms.  It does not dynbind `macroexp--dynvars' because we want


-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Alan Mackenzie]

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

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

[Mattias Engdegård]

12 okt. 2024 kl. 01.45 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> That sounds potentially costly

I'm also concerned about costs but won't have time to look at it for at least a few days.

However since it touches code that I'm revamping (although that process has been demoted to power-saving mode at the moment) I'd like to give it a look-over when I can.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I haven't, no.  It might well work.  But I think the `push' form should
> go outside of the loop,

Really?  I thought it made more sense to put it inside the loop, so
a macro X1 that expands to macro X2 ... will get all of the
corresponding locations pushed, thus preserving more of the info about
where things are coming from.

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

The patch seemed like an easier way to describe the intent than trying
to phrase it in prose, but it's definitely not meant to be used as-is.
I agree that we need to make sure things get popped as needed (tho I got
the impression that it should indeed already occur (at least in the
call-path I looked at)).

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

Oh, indeed my patch won't work at all when the warning/error message is
emitted later on: we need to preserve the info in the macro-expanded
code itself rather than in the locations-info stack.


        Stefan

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

[Alan Mackenzie]

Hello again, Stefan.

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.

In this case, no.  The function from which the macro is called might have
a locally bound variable called `fooo'.  ;-)  But there are surely errors
in macros which aren't like that which might be picked up by compiling
the macro.  Then warnings containing the line in the macro could be
given.  That would be a non-trivial exercise.

> > +(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've fixed this bug together with bug#73746, and submitted the patch for
that other bug which fixes this bug, too.

I've just done $ time make -j25 bootstrap on a branch with the bug#73746
patch, and a (more or less) standard master.  The bug#73746 branch took
1min 58sec, the standard branch to 1min 51sec.  That's a difference of
~6% in the bootstrap.  It will of course be a bigger difference in the
compilation.

If that was felt to be too much, it would be possible instead to go round
macroexp--expand-all and byte-optimize-form-code-walker and all the
functions like byte-optimize-letX, putting in custom `position-symbol'
calls.  This would be a lot of work, though.

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

Like you said in another post, it might well not work for the byte-opt.el
functionality.  I think I was looking around for that sort of solution,
and decided it wouldn't work, for a reason I've forgotten, before coming
up with the idea in my patch

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).