Package: emacs;
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(2). 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).
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.