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

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

[Alan Mackenzie]

Hello, Mattias and Stefan.

On Sat, Oct 12, 2024 at 14:01:20 +0200, Mattias Engdegård wrote:
> 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.

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.

If I don't hear anything more soon, I will close these two bugs as fixed.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Mattias Engdegård]

20 okt. 2024 kl. 16.13 skrev Alan Mackenzie <acm@muc.de>:

> 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 about the delay, but you can't make that assumption. Now reverted.
I promise I'll look at your report soon, but let's not make your original patch a fait accompli.

(Are 73725 and 73746 about different problems? They looked confusingly similar.)

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

[Alan Mackenzie]

Hello, Mattias.

On Sun, Oct 20, 2024 at 17:06:01 +0200, Mattias Engdegård wrote:
> 20 okt. 2024 kl. 16.13 skrev Alan Mackenzie <acm@muc.de>:

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

[ .... ]

> (Are 73725 and 73746 about different problems? They looked confusingly
> similar.)

As explained in the text for 73746, there is a "," different.  This has a
huge effect on the optimisation which is carried out on the macro.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#73746: 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 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.

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.

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

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

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

> +  (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)`.

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

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

Same here.

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

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

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

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


        Stefan

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

[Alan Mackenzie]

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)

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]

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

This should usually be harmless since the subform should come with its own
position information.

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

This is more annoying, indeed.  Ideally, we'd move the
`macroexp-preserve-posification` to the place(s) where this remove+readd
`progn` happens.

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

Hmm... I see.  We can leave for a later optimization of
`byte-optimize-form-code-walker`.

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

Thanks.

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

That part did not bother me.  These are internal functions so it's OK if
the docstring is a bit less "self contained".

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

Thanks.

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

[ Interesting.  I'll try to remember to track down this sucker later.  ]

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

I think we should assume that those potential side effects would more
likely be beneficial than harmful.

Another way to look at it is to look at the doc you provided:

    KEEP-POS, if non-nil, specifies that any symbol-with-position for
    FORM should be preserved, later to be usable by
    `byte-compile--warning-source-offset'.

Even when `keep-pos` is nil, `macroexpand-all` will preserve most of the
SWPs, so the doc is misleading.

> Also, always trying to preserve the position might slow down
> compilation, but I haven't measured that yet.

I think the calls where you currently ask for `keep-pos` account for the
vast majority of the time spent macro-expanding, so I'd be surprised if
doing it in all cases would make it measurably worse.


        Stefan