bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.

[Tim Ruffing <crypto@timruffing.de>]

On Sat, 2024-01-13 at 12:40 -0500, Stefan Monnier wrote:

> I tend to agree that it's closer to the ideal behavior (even though
> I still don't know what that is :-).

I think that's the crucial question for now. I'm happy to follow up
with an updated patch, but we should first see in which direction we
want to head.


> I think the problem is that we haven't clearly decided what
> `execute-kbd-macro` is supposed to do. The problem is that the way
> it's
> expected to work implies a form of "nesting" such that at the end of
> processing those events we return from the function, but sometimes
> that's not what happens. E.g. a kmacro can end in the middle of
> a recursive edit.
> 
> Your patch brings is closer to a non-nesting semantics, a bit as if
> `execute-kbd-macro` pushed all those events into the incoming input
> queue and returned immediately.
> 
> Maybe that's what `execute-kbd-macro` should do, really: push all the
> events into `unread-command-events` and return.

Just to make sure we're on same page: You mean it should (maybe) push
the events into `unread-command-events` assuming LIFO semantics? 

That is, in simple cases when there's no lisp code that reads events
explicitly (`read-char` etc) and you have a kmacro A that executes
another kmacro B, the it's still the case that we get the normal
nesting semantics.

In other words, consider the following kmacros:

A: read char x, read char y (which does nothing but trigger B), read
char z
B: read char b

Then executing A will be as if the user typed "x b z" explicitly.

I think your proposal makes sense. It's simple, it does "what you'd
expect" in simple cases, and it seems compatible to the current
behavior, up to the -1 bug. (And don't think there's a natural
expectation for cases where we hit -1.) 

> But then, how would we
> do the boolean tests for `executing-kbd-macro` which we perform at
> various places? [ I guess we could label/wrap the events such that
> they
> get executed in a special way (which let-binds `executing-kbd-macro`
> to
> t? ]

Yeah, this sounds reasonable (at least at first glance :)). 

> Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
> kmacros to contain "complete sequences"), maybe a half-way
> behavior between the current one and your new one would be for
> `read_char` to return -1 (or rather a more explicit `end-of-kbd-
> macro`)
> event *once* and then on the next call to go on and read from
> the keyboard.

Hm, indeed. But to be honest, I'm not convinced that lisp code should
be able to distinguish this at all when trying to get an event. (If you
*really* want to detect this, you could still check `executing-kbd-
macro` after reading an event and see if it has changed from non-nil to
nil.) Moreover, it would make the C code more complicated, because
we'll probably have to add another state variable that captures if
we've returned -1 (or a symbol) already or not. 

Another compromise, which I find nicer, is to introduce a new variant
of `read-char` (or add a flag) that suppresses the -1 entirely. We'd
probably still need the state variable in the C code, but at least this
won't change anything for existing callers. We could even keep the -
1...

Well okay, assuming we agree that the non-nesting semantics is in
general what we want to have, what should we (or I) do now?

   1. Work on an improved version of my patch, which brings it closer
      to the ideal semantics, but doesn't touch too much of the code?
   2. Write a new patch that implements the "full" non-nesting
      semantics suggest above?
   3. Rewrite my patch to do something in the middle? (Returning -1
      once, or have a boolean flag that fixes the -1.)
   4. Do nothing because all of it is too dangerous.

If anything, I think option 3 with a flag beats 4, because 3 won't be
too dangerous then.

But for the rest, I'm really unsure (and anyway, I don't think I'm the
one to make the call). 2 sounds nice, but do you think there will be
even more dragons with 2 than with 1? It will be an even larger change
for sure...

----

More inline replies:


> IIUC, with your patch, we have the following scenario:
> - Say we're inside a kmacro with N events left to execute.
> - Dbus reads those N events and stashes them them onto `unread-
> command-events`.
> - Dbus finally can read the actual dbus event and does its thing.
> - Good!
> - But now `at_end_of_macro_p` will return true, even though we've yet
> to
> execute the last N events. We'll presumably still execute them
> (since they're in `unread-command-events`) but that won't be
> considered as coming from a kmacro any more.

This matches my understanding. My thinking is that this is not a big
deal in this specific case. The dbus code currently relies on the idea
of reading events and putting them back to `unread-command-events`.
While the patch affects the behavior, it doesn't change anything about
the fact that this is an ugly hack anyway. So I'm not really convinced
that we should care about this change of behavior too much, as long as
we don't break anything. 


> 
> > And I'm also aware that it doesn't spark confidence when someone
> > with
> > (almost) zero code contributions submits a patch of this kind.
> 
> [ Actually, such carefully written code and (even more so) the
>   convention-abiding commit messages coming from someone who's not
>   a regular contributor spiked my ears.  It made me think
>   that this is either a carefully crafted trojan, or it's a really
>   welcome new kid on the block :-)  ]
> 

Hehe, thanks for the kind words. I'll submit the trojan in the next
patch then. ;) On a more serious note, I took the time to dig deeply
here, but I don't think I'll have the bandwidth regularly (as you see
from my response time...) 

By the way, the CONTRIBUTE file is pretty long, but it's also pretty
good and clear. 

> Yes, I think we should document in a comment somewhere how the end of
> kmacro is handled from a "global" perspective: what `read_char`
> does when it's the case, where/when it's supposed to be detected and
> how
> this "signal" is propagated from there to the corresponding call to
> `execute-kbd-macro`, how that relates to setting `executing-kbd-
> macro`
> back to nil, ...
> 

Agreed.

> "-1 is not propagated to lisp" doesn't say what happens in its stead.
> What this does really is transparently continue reading input from
> the
> keyboard when reaching the end of a kmacro.
> 

Agreed, I can change it. But let's first decide what we would like to
do in general.

Best,
Tim