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

[Alan Mackenzie <acm@muc.de>]

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