Subject: Master: Wrong position for byte compiler warning message(2).

From: Alan Mackenzie <[email-masked]>
Date: Fri, 11 Oct 2024 16:24:16 +0000

Hello, Emacs.

On the master branch.

(i) Create the following file, bad-error-position3.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.  Note also that the
file is the same as bad-error-position2.el from bug#73725 apart from the
"," in ",(point-max)" on line 4.

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

Also, functions in byte-opt.el manipulate the forms, often replacing
symbols with position by other symbols lacking those positions.

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 same should also be
done with forms being optimised by byte-optimize-form in byte-opt.el.

The following patch appears to fix the bug:

diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
index d8dbfa62bf9..34802022a30 100644
--- a/lisp/emacs-lisp/byte-opt.el
+++ b/lisp/emacs-lisp/byte-opt.el
@@ -344,7 +344,7 @@ byte-optimize-form-code-walker
        ;; As an extra added bonus, this simplifies (progn <x>) --> <x>.
        (if (cdr exps)
            (macroexp-progn (byte-optimize-body exps for-effect))
-	 (byte-optimize-form (car exps) for-effect)))
+         (byte-optimize-form (car exps) for-effect)))
       (`(prog1 ,exp . ,exps)
        (let ((exp-opt (byte-optimize-form exp for-effect)))
          (if exps
@@ -510,8 +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.
         (and (consp form)
@@ -519,7 +520,9 @@ 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))))))))
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 29e7882c851..4272a6fb252 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2582,7 +2582,7 @@ 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))
+  (setq form (macroexpand-all form byte-compile-macro-environment t))
   ;; 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
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 4524eccc7ef..6e4a74cf0cb 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -237,22 +237,86 @@ 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)))
+
+(defmacro macroexp-preserve-posification (pos-form &rest body)
+  "Evaluate BODY..., posifying the result with POS-FORM's position, if any."
+  `(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)
-    (while (not (eq form (setq new-form (macroexpand-1 form env))))
-      (let ((fun (car-safe form)))
-        (setq form
-              (if (and fun (symbolp fun)
-                       (get fun 'byte-obsolete-info))
-                  (macroexp-warn-and-return
-                   (macroexp--obsolete-warning
-                    fun (get fun 'byte-obsolete-info)
-                    (if (symbolp (symbol-function fun)) "alias" "macro"))
-                   new-form (list 'obsolete fun) nil fun)
-                new-form))))
-    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)
+                        (get fun 'byte-obsolete-info))
+                   (macroexp-warn-and-return
+                    (macroexp--obsolete-warning
+                     fun (get fun 'byte-obsolete-info)
+                     (if (symbolp (symbol-function fun)) "alias" "macro"))
+                    new-form (list 'obsolete fun) nil fun)
+                 new-form))))
+     new-form)))
 
 (defun macroexp--unfold-lambda (form &optional name)
   (or name (setq name "anonymous lambda"))
@@ -516,14 +580,21 @@ macroexp--expand-all
             (_ form))))))
 
 ;;;###autoload
-(defun macroexpand-all (form &optional environment)
+(defun macroexpand-all (form &optional environment keep-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)
+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.  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'."
+  (let*
+      ((macroexpand-all-environment environment)
         (macroexp--dynvars macroexp--dynvars))
-    (macroexp--expand-all form)))
+    (if keep-pos
+        (macroexp-preserve-posification
+         form
+         (macroexp--expand-all form))
+      (macroexp--expand-all 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).