GNU bug report logs - #73725
Master: Wrong position for byte compiler warning message.

Previous Next

Full log

View this message in rfc822 format

From: Alan Mackenzie <acm <at> muc.de>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: acm <at> muc.de, Mattias Engdegård <mattias.engdegard <at> gmail.com>, 73725 <at> debbugs.gnu.org
Subject: bug#73725: Master: Wrong position for byte compiler warning message.
Date: Sun, 13 Oct 2024 15:31:55 +0000

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.