Package: emacs;
Reported by: Alan Mackenzie <acm <at> muc.de>
Date: Thu, 10 Oct 2024 10:24:02 UTC
Severity: normal
Tags: patch
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Alan Mackenzie <acm <at> muc.de> To: bug-gnu-emacs <at> gnu.org Subject: Master: Wrong position for byte compiler warning message. Date: Thu, 10 Oct 2024 10:22:49 +0000
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).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.