bug#67483: Wrong warning position given by the byte compiler for a malformed function

[Alan Mackenzie <acm@muc.de>]

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