unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#67483: Wrong warning position given by the byte compiler for a malformed function
@ 2023-11-27 12:40 Alan Mackenzie
  2023-11-27 13:14 ` Eli Zaretskii
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Alan Mackenzie @ 2023-11-27 12:40 UTC (permalink / raw)
  To: 67483

Hello, Emacs

In any recent or semi-recent Emacs create a file bad-error-position.el
with these contents:

    (defun foo ()
      (let ((bar 'bar))
        (if ("foo")  ; Erroneous "function".
            (baz))))))

..  Use M-x compile-defun to compile it.  This gives an error message:

    Buffer bad-error-position.el:2:4: Warning: `foo' is a malformed function

..  This position 2:4 is wrong; it is the position of the `let' symbol.
The correct position would be 3:6, the position of the `if' symbol.

#########################################################################

The cause of the error is in byte-optimize-form in
lisp/emacs-lisp/byte-opt.el.  There, although the code recurses, it
fails to push the current form onto byte-compile-form-stack.  Thus when
byte-compile-warn-x is called, there is nothing usable on that stack
inside the let form.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 12:40 bug#67483: Wrong warning position given by the byte compiler for a malformed function Alan Mackenzie
@ 2023-11-27 13:14 ` Eli Zaretskii
  2023-11-27 13:20   ` Alan Mackenzie
  2023-11-30 10:37 ` Mattias Engdegård
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2023-11-27 13:14 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: 67483

> Date: Mon, 27 Nov 2023 12:40:49 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> In any recent or semi-recent Emacs create a file bad-error-position.el
> with these contents:

Which branch? master or emacs-29?





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 13:14 ` Eli Zaretskii
@ 2023-11-27 13:20   ` Alan Mackenzie
  2023-11-27 13:50     ` Eli Zaretskii
  0 siblings, 1 reply; 17+ messages in thread
From: Alan Mackenzie @ 2023-11-27 13:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: acm, 67483

Hello, Eli.

On Mon, Nov 27, 2023 at 15:14:29 +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Nov 2023 12:40:49 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > In any recent or semi-recent Emacs create a file bad-error-position.el
> > with these contents:

> Which branch? master or emacs-29?

Both.  Probably emacs-28, too.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 13:20   ` Alan Mackenzie
@ 2023-11-27 13:50     ` Eli Zaretskii
  2023-11-27 14:01       ` Alan Mackenzie
  0 siblings, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2023-11-27 13:50 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: acm, 67483

> Date: Mon, 27 Nov 2023 13:20:54 +0000
> Cc: 67483@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> On Mon, Nov 27, 2023 at 15:14:29 +0200, Eli Zaretskii wrote:
> > > Date: Mon, 27 Nov 2023 12:40:49 +0000
> > > From: Alan Mackenzie <acm@muc.de>
> 
> > > In any recent or semi-recent Emacs create a file bad-error-position.el
> > > with these contents:
> 
> > Which branch? master or emacs-29?
> 
> Both.  Probably emacs-28, too.

So it isn't a "semi-recent" problem, is it?





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 13:50     ` Eli Zaretskii
@ 2023-11-27 14:01       ` Alan Mackenzie
  2023-11-27 15:09         ` Eli Zaretskii
  0 siblings, 1 reply; 17+ messages in thread
From: Alan Mackenzie @ 2023-11-27 14:01 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: acm, 67483

Hello, Eli.

On Mon, Nov 27, 2023 at 15:50:25 +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Nov 2023 13:20:54 +0000
> > Cc: 67483@debbugs.gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > On Mon, Nov 27, 2023 at 15:14:29 +0200, Eli Zaretskii wrote:
> > > > Date: Mon, 27 Nov 2023 12:40:49 +0000
> > > > From: Alan Mackenzie <acm@muc.de>

> > > > In any recent or semi-recent Emacs create a file bad-error-position.el
> > > > with these contents:

> > > Which branch? master or emacs-29?

> > Both.  Probably emacs-28, too.

> So it isn't a "semi-recent" problem, is it?

It isn't ten years old, either.  I have a fix ready to commit to the
master branch.

Seeing as how the bug isn't recently introduced, and isn't a critical
bug, I'm assuming the patch is not destined for the release branch.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 14:01       ` Alan Mackenzie
@ 2023-11-27 15:09         ` Eli Zaretskii
  2023-11-27 15:47           ` Alan Mackenzie
  0 siblings, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2023-11-27 15:09 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: acm, 67483

> Date: Mon, 27 Nov 2023 14:01:40 +0000
> Cc: 67483@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > > > Which branch? master or emacs-29?
> 
> > > Both.  Probably emacs-28, too.
> 
> > So it isn't a "semi-recent" problem, is it?
> 
> It isn't ten years old, either.  I have a fix ready to commit to the
> master branch.
> 
> Seeing as how the bug isn't recently introduced, and isn't a critical
> bug, I'm assuming the patch is not destined for the release branch.

Definitely not.





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 15:09         ` Eli Zaretskii
@ 2023-11-27 15:47           ` Alan Mackenzie
  0 siblings, 0 replies; 17+ messages in thread
From: Alan Mackenzie @ 2023-11-27 15:47 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 67483-done, acm

Hello, Eli.

On Mon, Nov 27, 2023 at 17:09:10 +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Nov 2023 14:01:40 +0000
> > Cc: 67483@debbugs.gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > > > > Which branch? master or emacs-29?

> > > > Both.  Probably emacs-28, too.

> > > So it isn't a "semi-recent" problem, is it?

> > It isn't ten years old, either.  I have a fix ready to commit to the
> > master branch.

> > Seeing as how the bug isn't recently introduced, and isn't a critical
> > bug, I'm assuming the patch is not destined for the release branch.

> Definitely not.

No.  I've committed the fix to master, and I'm closing the bug.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 12:40 bug#67483: Wrong warning position given by the byte compiler for a malformed function Alan Mackenzie
  2023-11-27 13:14 ` Eli Zaretskii
@ 2023-11-30 10:37 ` Mattias Engdegård
  2023-12-04 18:19   ` Alan Mackenzie
  2023-12-04 16:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
       [not found] ` <handler.67483.B.170108887925620.ack@debbugs.gnu.org>
  3 siblings, 1 reply; 17+ messages in thread
From: Mattias Engdegård @ 2023-11-30 10:37 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Stefan Monnier, 67483

>     Buffer bad-error-position.el:2:4: Warning: `foo' is a malformed function
> 
> ..  This position 2:4 is wrong; it is the position of the `let' symbol.
> The correct position would be 3:6, the position of the `if' symbol.

(Actually the correct position would be the position of the string literal but of course our location tracking system is too simplistic for that.)

Thank you for bringing this to our attention. Now I only saw this from the reference in your commit message; would you CC me next time? (Stefan, too, unless he objects.)

I'm modifying your work a bit because we're trying to remove warnings from the optimiser, not entrenching them there. The warning is now in cconv but perhaps it should be moved to macroexp-all, it's not very important. I hope being able to reshape the front-end a bit later on.

Also, we usually prefer let-binding dynamic variables to push-pop pairs.






^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-27 12:40 bug#67483: Wrong warning position given by the byte compiler for a malformed function Alan Mackenzie
  2023-11-27 13:14 ` Eli Zaretskii
  2023-11-30 10:37 ` Mattias Engdegård
@ 2023-12-04 16:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
       [not found] ` <handler.67483.B.170108887925620.ack@debbugs.gnu.org>
  3 siblings, 0 replies; 17+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-12-04 16:44 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: 67483

>     (defun foo ()
>       (let ((bar 'bar))
>         (if ("foo")  ; Erroneous "function".
>             (baz))))))
>
> ..  Use M-x compile-defun to compile it.  This gives an error message:
>
>     Buffer bad-error-position.el:2:4: Warning: `foo' is a malformed function
>
> ..  This position 2:4 is wrong; it is the position of the `let' symbol.
> The correct position would be 3:6, the position of the `if' symbol.

Also the message should use `"foo"` rather than `foo` (IOW,
the `%s` should be a `%S`).  As a general rule of thumb, `%s` should be
used *only* when displaying the content of a string, and `%S` should be
used for most/all other cases (e.g. when displaying what we expect is
a symbol, like here).


        Stefan






^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-11-30 10:37 ` Mattias Engdegård
@ 2023-12-04 18:19   ` Alan Mackenzie
  2023-12-19 18:23     ` Mattias Engdegård
  2023-12-21 12:22     ` Mattias Engdegård
  0 siblings, 2 replies; 17+ messages in thread
From: Alan Mackenzie @ 2023-12-04 18:19 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: acm, Stefan Monnier, 67483

Hello, Mattias.

On Thu, Nov 30, 2023 at 11:37:31 +0100, Mattias Engdegård wrote:
> >     Buffer bad-error-position.el:2:4: Warning: `foo' is a malformed function

> > ..  This position 2:4 is wrong; it is the position of the `let' symbol.
> > The correct position would be 3:6, the position of the `if' symbol.

> (Actually the correct position would be the position of the string
> literal but of course our location tracking system is too simplistic
> for that.)

Indeed.

> Thank you for bringing this to our attention. Now I only saw this from
> the reference in your commit message; would you CC me next time?
> (Stefan, too, unless he objects.)

OK.

> I'm modifying your work a bit because we're trying to remove warnings
> from the optimiser, not entrenching them there. The warning is now in
> cconv but perhaps it should be moved to macroexp-all, it's not very
> important.

Many people consider accurate diagnostics to be very important.

> I hope being able to reshape the front-end a bit later on.

Well, I can hardly say thank you, here.  You've undone the bug fix, and
the bug is there again.  Did you actually test it before commiting your
change?  What should I do, now?  Reopen the bug and ask you actually to
fix it?  Or put the fix back in again myself?

You may not like warning handling code in the optimiser, but unless you
can come up with something better, its presence there is essential to
getting good diagnostics.

> Also, we usually prefer let-binding dynamic variables to push-pop
> pairs.

We do indeed, but here binding the variable simply doesn't work.  Parts
of the compiler, when they encounter errors, signal an error which gets
caught by a condition-case somewhere.  This would unbind
byte-compile-form-stack before the warning handlers could get a chance
to use it.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Acknowledgement (Wrong warning position given by the byte compiler for a malformed function)
       [not found] ` <handler.67483.B.170108887925620.ack@debbugs.gnu.org>
@ 2023-12-08 20:19   ` Alan Mackenzie
  0 siblings, 0 replies; 17+ messages in thread
From: Alan Mackenzie @ 2023-12-08 20:19 UTC (permalink / raw)
  To: 67483, control; +Cc: acm

reopen 67483
quit

The fix to this bug was undone by the following commit:

commit 8525be6d5eca0c75008ec1dc799cae537156feea
Author: Mattias Engdegård <mattiase@acm.org>
Date:   Wed Nov 29 17:51:46 2023 +0100

    Move malformed-function warning from byte-opt to cconv (bug#67483)

    We shouldn't be warning inside the optimiser in the first place.

    * lisp/emacs-lisp/byte-opt.el (byte-optimize-form):
    Remove byte-compile-form-stack manipulation.
    (byte-optimize-form-code-walker): Move malformed function warning
    from here...
    * lisp/emacs-lisp/cconv.el: ...to here.

On Mon, Nov 27, 2023 at 12:42:02 +0000, GNU bug Tracking System wrote:
> Thank you for filing a new bug report with debbugs.gnu.org.

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-04 18:19   ` Alan Mackenzie
@ 2023-12-19 18:23     ` Mattias Engdegård
  2023-12-21 12:22     ` Mattias Engdegård
  1 sibling, 0 replies; 17+ messages in thread
From: Mattias Engdegård @ 2023-12-19 18:23 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Stefan Monnier, 67483

Sorry about the delay. I hope being able to take a new look at this bug shortly.






^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-04 18:19   ` Alan Mackenzie
  2023-12-19 18:23     ` Mattias Engdegård
@ 2023-12-21 12:22     ` Mattias Engdegård
  2023-12-22 11:24       ` Alan Mackenzie
  1 sibling, 1 reply; 17+ messages in thread
From: Mattias Engdegård @ 2023-12-21 12:22 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Stefan Monnier, 67483

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

4 dec. 2023 kl. 19.19 skrev Alan Mackenzie <acm@muc.de>:

> You've undone the bug fix, and the bug is there again.

Oops. An honest mistake, very sorry about that.
On the other hand, these blunders often turn out to be beneficial in the end because they force us to take a better look at the problem.

I ended up maintaining `byte-compile-form-stack` in `cconv-convert` which isn't free but a lot better than doing it in the optimiser. We emit a fair amount of warnings in cconv so this should be useful for other reasons as well.

The warning was also reverted from delayed to immediate, which makes sense in this case (since it's essentially a syntax error) and in fact it wouldn't work otherwise because delayed warnings implicitly use the byte-compile-form-stack we have when traversing the post-optimisation tree in codegen.

I'm inclined to do something about this last problem for good measure (see attached patch). Most of the time the byte-compile-form-stack doesn't matter much because the warning argument contains a symbol with position, but when it does, the stack state in codegen when the warning is emitted is likely to be less useful than when the warning was registered in the front-end. Haven't made up my mind about this yet.

> We do indeed, but here binding the variable simply doesn't work.  Parts
> of the compiler, when they encounter errors, signal an error which gets
> caught by a condition-case somewhere.

Yes, so I noticed. This is rubbish of course; we should do something about it. We have some options.
Meanwhile I made a macro to encapsulate the ugly push-pop logic in one place.


[-- Attachment #2: 0001-Capture-byte-compile-form-stack-in-delayed-warnings-.patch --]
[-- Type: application/octet-stream, Size: 1744 bytes --]

From 0a4222090ee3b6690b3097a06dacd23546c7350b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 20 Dec 2023 11:11:56 +0100
Subject: [PATCH] Capture byte-compile-form-stack in delayed warnings
 (bug#67483)

* lisp/emacs-lisp/macroexp.el (macroexp--warn-wrap):
When recording a delayed warning, capture the current value of
`byte-compile-form-stack` because it is more likely to contain a
relevant source location than whatever we are traversing in codegen.
---
 lisp/emacs-lisp/macroexp.el | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 2a646be9725..8cf6ad7256d 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -168,12 +168,14 @@ macroexp-file-name
 (defvar macroexp--warned (make-hash-table :test #'equal :weakness 'key))
 
 (defun macroexp--warn-wrap (arg msg form category)
-  (let ((when-compiled
-	 (lambda ()
-           (when (if (consp category)
-                     (apply #'byte-compile-warning-enabled-p category)
-                   (byte-compile-warning-enabled-p category))
-             (byte-compile-warn-x arg "%s" msg)))))
+  (let* ((stack byte-compile-form-stack)
+         (when-compiled
+	  (lambda ()
+            (when (if (consp category)
+                      (apply #'byte-compile-warning-enabled-p category)
+                    (byte-compile-warning-enabled-p category))
+              (let ((byte-compile-form-stack stack))
+                (byte-compile-warn-x arg "%s" msg))))))
     `(progn
        (macroexp--funcall-if-compiled ',when-compiled)
        ,form)))
-- 
2.32.0 (Apple Git-132)


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-21 12:22     ` Mattias Engdegård
@ 2023-12-22 11:24       ` Alan Mackenzie
  2023-12-22 13:12         ` Mattias Engdegård
  0 siblings, 1 reply; 17+ messages in thread
From: Alan Mackenzie @ 2023-12-22 11:24 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: acm, Stefan Monnier, 67483

Hello, Mattias.

On Thu, Dec 21, 2023 at 13:22:17 +0100, Mattias Engdegård wrote:
> 4 dec. 2023 kl. 19.19 skrev Alan Mackenzie <acm@muc.de>:

> > You've undone the bug fix, and the bug is there again.

> Oops. An honest mistake, very sorry about that.

Ah, these things happen!  No great problem.

> On the other hand, these blunders often turn out to be beneficial in
> the end because they force us to take a better look at the problem.

> I ended up maintaining `byte-compile-form-stack` in `cconv-convert`
> which isn't free but a lot better than doing it in the optimiser. We
> emit a fair amount of warnings in cconv so this should be useful for
> other reasons as well.

I don't understand this.  What's the process of converting to a closure
got to do with maintaining the stack of forms for error processing?
cconv shouldn't even be involved at all for dynamic binding - it would
seem to me that the only reason for calling cconv in this case is now to
get the error handling.  This doesn't seem good.  What am I missing?

> The warning was also reverted from delayed to immediate, which makes
> sense in this case (since it's essentially a syntax error) and in fact
> it wouldn't work otherwise because delayed warnings implicitly use the
> byte-compile-form-stack we have when traversing the post-optimisation
> tree in codegen.

This seems indeed a good thing.  I've never understood the delayed
warning mechanism.

> I'm inclined to do something about this last problem for good measure
> (see attached patch).

Whoops!  There was no patch.

> Most of the time the byte-compile-form-stack doesn't matter much
> because the warning argument contains a symbol with position, but when
> it does, the stack state in codegen when the warning is emitted is
> likely to be less useful than when the warning was registered in the
> front-end. Haven't made up my mind about this yet.

> > We do indeed, but here binding the variable simply doesn't work.  Parts
> > of the compiler, when they encounter errors, signal an error which gets
> > caught by a condition-case somewhere.

> Yes, so I noticed. This is rubbish of course; we should do something
> about it. We have some options.  Meanwhile I made a macro to
> encapsulate the ugly push-pop logic in one place.

You've put the new macro into macroexp.el.  This file is purely about
macro handling.  The new macro has nothing to do with this, it is part
of the compiler.  Surely it should be
byte-compile-with-extended-form-stack.  And is the "--" in the name
appropriate, given that the macro is used by several files?  I'm not
sure about that rule.

Also, byte-compile-form-stack gets bound in cconv-closure-convert.  Why?
It seems unneeded, and in the event of an error being caught by a
condition-case will undo all the good work that came from the tedious
discipline of using push and pop rather than binding.  

But cconv-closure-convert doesn't get called recursively.  So it would
seem the wrong place to be maintaining byte-compile-form-stack.  What's
needed is a place where that stack grows steadily as the source code is
recursed into, to ensure there will be a correct position on it in the
event of an warning/error.

I don't think this bug is properly fixed, yet.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-22 11:24       ` Alan Mackenzie
@ 2023-12-22 13:12         ` Mattias Engdegård
  2023-12-22 20:09           ` Alan Mackenzie
  0 siblings, 1 reply; 17+ messages in thread
From: Mattias Engdegård @ 2023-12-22 13:12 UTC (permalink / raw)
  To: Alan Mackenzie; +Cc: Stefan Monnier, 67483

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

22 dec. 2023 kl. 12.24 skrev Alan Mackenzie <acm@muc.de>:

> What's the process of converting to a closure
> got to do with maintaining the stack of forms for error processing?

Not much, but cconv has evolved to one major part of the compiler front-end (the other being macroexp-all) and isn't restricted to just closure conversion. In fact, it's now used for dynbound code as well.

In particular it's a natural place for various front-end checks and transforms, so don't let the place and name distract you. There are plans to refactor it later on for other reasons.

> Whoops!  There was no patch.

Attached it now, sorry.

> You've put the new macro into macroexp.el.  This file is purely about
> macro handling.

Actually macroexp.el does more than that, and in any case the file isn't very important; the macro ended up there to be next to byte-compile-form-stack. Nor is the name; it can be changed at any time.

However, it probably needs to be in that file for bootstrap reasons.

> And is the "--" in the name
> appropriate, given that the macro is used by several files?  I'm not
> sure about that rule.

The double-dash just means that users shouldn't get any funny ideas. (The converse isn't true: a name without double-dash isn't automatically fair game.)

> Also, byte-compile-form-stack gets bound in cconv-closure-convert.  Why?

It's just a backstop. Not strictly needed. It's probably fine to remove it if you are worried, but then again there shouldn't be any (non-bug) error signalling here. I'll have a look.


[-- Attachment #2: 0001-Capture-byte-compile-form-stack-in-delayed-warnings-.patch --]
[-- Type: application/octet-stream, Size: 1744 bytes --]

From 0a4222090ee3b6690b3097a06dacd23546c7350b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 20 Dec 2023 11:11:56 +0100
Subject: [PATCH] Capture byte-compile-form-stack in delayed warnings
 (bug#67483)

* lisp/emacs-lisp/macroexp.el (macroexp--warn-wrap):
When recording a delayed warning, capture the current value of
`byte-compile-form-stack` because it is more likely to contain a
relevant source location than whatever we are traversing in codegen.
---
 lisp/emacs-lisp/macroexp.el | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 2a646be9725..8cf6ad7256d 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -168,12 +168,14 @@ macroexp-file-name
 (defvar macroexp--warned (make-hash-table :test #'equal :weakness 'key))
 
 (defun macroexp--warn-wrap (arg msg form category)
-  (let ((when-compiled
-	 (lambda ()
-           (when (if (consp category)
-                     (apply #'byte-compile-warning-enabled-p category)
-                   (byte-compile-warning-enabled-p category))
-             (byte-compile-warn-x arg "%s" msg)))))
+  (let* ((stack byte-compile-form-stack)
+         (when-compiled
+	  (lambda ()
+            (when (if (consp category)
+                      (apply #'byte-compile-warning-enabled-p category)
+                    (byte-compile-warning-enabled-p category))
+              (let ((byte-compile-form-stack stack))
+                (byte-compile-warn-x arg "%s" msg))))))
     `(progn
        (macroexp--funcall-if-compiled ',when-compiled)
        ,form)))
-- 
2.32.0 (Apple Git-132)


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-22 13:12         ` Mattias Engdegård
@ 2023-12-22 20:09           ` Alan Mackenzie
  2024-01-13 14:05             ` Alan Mackenzie
  0 siblings, 1 reply; 17+ messages in thread
From: Alan Mackenzie @ 2023-12-22 20:09 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: acm, Stefan Monnier, 67483

Hello, Mattias.

On Fri, Dec 22, 2023 at 14:12:46 +0100, Mattias Engdegård wrote:
> 22 dec. 2023 kl. 12.24 skrev Alan Mackenzie <acm@muc.de>:

> > What's the process of converting to a closure got to do with
> > maintaining the stack of forms for error processing?

> Not much, but cconv has evolved to one major part of the compiler
> front-end (the other being macroexp-all) and isn't restricted to just
> closure conversion.

No, it hasn't "evolved", somebody has changed it.  Why?  A file, just
like a function, should have a particular purpose, and degrading a
specific purpose file to being a general place to put random things is a
BAD THING.  What's going on, here?

> In fact, it's now used for dynbound code as well.

Does this file, in fact, still have a purpose?

> In particular it's a natural place for various front-end checks and
> transforms, so don't let the place and name distract you. There are
> plans to refactor it later on for other reasons.

> > Whoops!  There was no patch.

> Attached it now, sorry.

> > You've put the new macro into macroexp.el.  This file is purely about
> > macro handling.

> Actually macroexp.el does more than that, and in any case the file
> isn't very important; the macro ended up there to be next to
> byte-compile-form-stack. Nor is the name; it can be changed at any
> time.

> However, it probably needs to be in that file for bootstrap reasons.

byte-compile-form-stack is in there for bootstrap reasons.  But it's
logically part of bytecomp.el, hence the name.

> > And is the "--" in the name appropriate, given that the macro is used
> > by several files?  I'm not sure about that rule.

> The double-dash just means that users shouldn't get any funny ideas.
> (The converse isn't true: a name without double-dash isn't
> automatically fair game.)

> > Also, byte-compile-form-stack gets bound in cconv-closure-convert.
> > Why?

> It's just a backstop. Not strictly needed. It's probably fine to remove
> it if you are worried, but then again there shouldn't be any (non-bug)
> error signalling here. I'll have a look.

How can you be so casual about this?  It's critically important to
byte-compile-form-stack's correct working that it does NOT get bound.
Surely you understand this?

Also, you ignored and snipped the most important point in my last post,
namely this:

> > But cconv-closure-convert doesn't get called recursively.  So it
> > would seem the wrong place to be maintaining byte-compile-form-stack.
> > What's needed is a place where that stack grows steadily as the
> > source code is recursed into, to ensure there will be a correct
> > position on it in the event of an warning/error.

Please attend to this point now.  Does byte-compile-form-stack get pushed
onto at each recursive descent into the source code?  This is important.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

* bug#67483: Wrong warning position given by the byte compiler for a malformed function
  2023-12-22 20:09           ` Alan Mackenzie
@ 2024-01-13 14:05             ` Alan Mackenzie
  0 siblings, 0 replies; 17+ messages in thread
From: Alan Mackenzie @ 2024-01-13 14:05 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: Stefan Monnier, 67483

Hello, Mattias.

Ping?

On Fri, Dec 22, 2023 at 20:09:19 +0000, Alan Mackenzie wrote:
> Hello, Mattias.

> On Fri, Dec 22, 2023 at 14:12:46 +0100, Mattias Engdegård wrote:
> > 22 dec. 2023 kl. 12.24 skrev Alan Mackenzie <acm@muc.de>:

> > > What's the process of converting to a closure got to do with
> > > maintaining the stack of forms for error processing?

> > Not much, but cconv has evolved to one major part of the compiler
> > front-end (the other being macroexp-all) and isn't restricted to just
> > closure conversion.

> No, it hasn't "evolved", somebody has changed it.  Why?  A file, just
> like a function, should have a particular purpose, and degrading a
> specific purpose file to being a general place to put random things is a
> BAD THING.  What's going on, here?

> > In fact, it's now used for dynbound code as well.

> Does this file, in fact, still have a purpose?

> > In particular it's a natural place for various front-end checks and
> > transforms, so don't let the place and name distract you. There are
> > plans to refactor it later on for other reasons.

> > > Whoops!  There was no patch.

> > Attached it now, sorry.

> > > You've put the new macro into macroexp.el.  This file is purely about
> > > macro handling.

> > Actually macroexp.el does more than that, and in any case the file
> > isn't very important; the macro ended up there to be next to
> > byte-compile-form-stack. Nor is the name; it can be changed at any
> > time.

> > However, it probably needs to be in that file for bootstrap reasons.

> byte-compile-form-stack is in there for bootstrap reasons.  But it's
> logically part of bytecomp.el, hence the name.

> > > And is the "--" in the name appropriate, given that the macro is used
> > > by several files?  I'm not sure about that rule.

> > The double-dash just means that users shouldn't get any funny ideas.
> > (The converse isn't true: a name without double-dash isn't
> > automatically fair game.)

> > > Also, byte-compile-form-stack gets bound in cconv-closure-convert.
> > > Why?

> > It's just a backstop. Not strictly needed. It's probably fine to remove
> > it if you are worried, but then again there shouldn't be any (non-bug)
> > error signalling here. I'll have a look.

> How can you be so casual about this?  It's critically important to
> byte-compile-form-stack's correct working that it does NOT get bound.
> Surely you understand this?

> Also, you ignored and snipped the most important point in my last post,
> namely this:

> > > But cconv-closure-convert doesn't get called recursively.  So it
> > > would seem the wrong place to be maintaining byte-compile-form-stack.
> > > What's needed is a place where that stack grows steadily as the
> > > source code is recursed into, to ensure there will be a correct
> > > position on it in the event of an warning/error.

> Please attend to this point now.  Does byte-compile-form-stack get pushed
> onto at each recursive descent into the source code?  This is important.

-- 
Alan Mackenzie (Nuremberg, Germany).





^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2024-01-13 14:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-27 12:40 bug#67483: Wrong warning position given by the byte compiler for a malformed function Alan Mackenzie
2023-11-27 13:14 ` Eli Zaretskii
2023-11-27 13:20   ` Alan Mackenzie
2023-11-27 13:50     ` Eli Zaretskii
2023-11-27 14:01       ` Alan Mackenzie
2023-11-27 15:09         ` Eli Zaretskii
2023-11-27 15:47           ` Alan Mackenzie
2023-11-30 10:37 ` Mattias Engdegård
2023-12-04 18:19   ` Alan Mackenzie
2023-12-19 18:23     ` Mattias Engdegård
2023-12-21 12:22     ` Mattias Engdegård
2023-12-22 11:24       ` Alan Mackenzie
2023-12-22 13:12         ` Mattias Engdegård
2023-12-22 20:09           ` Alan Mackenzie
2024-01-13 14:05             ` Alan Mackenzie
2023-12-04 16:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
     [not found] ` <handler.67483.B.170108887925620.ack@debbugs.gnu.org>
2023-12-08 20:19   ` bug#67483: Acknowledgement (Wrong warning position given by the byte compiler for a malformed function) Alan Mackenzie

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).