Subject: Re: bug#78737: sit-for behavior changes when byte-compiled

From: Daniel Colascione <[email-masked]>
Date: Wed, 11 Jun 2025 05:40:29 -0700

Pip Cet <[email-masked]> writes:

> "Daniel Colascione" <[email-masked]> writes:
>
>> On June 10, 2025 10:40:39 AM PDT, Pip Cet <[email-masked]> wrote:
>>>"Stefan Monnier" <[email-masked]> writes:
>>>
>>>>> BTW: the problem isn't just with transient. It also manifests with
>>>>> read-extended-command! It's a nasty race that, AFAICT, has been with us
>>>>> since the 90s. I think defining read_char to translate quits to quit_char
>>>>> solves the problem.
>>>>
>>>> I like your way of thinking.  I'm not completely sure it will solve
>>>> world hunger, and it may come with regressions, but it's worth a try.
>>>
>>>I must be missing something: read_char translates quits to quit_char if
>>>called with inhibit-quit bound to t, and never returns with quit-flag
>>>set to t in that case.
>>
>> You shouldn't have to call it with inhibit-quit for it do that. It should just happen all the time.
>
> But we don't usually want read-event to eat quits and report them by
> returning quit_char.  The old behavior gave me a choice.

You still have a choice.  You can signal quit if you get a quit event.

> (while t (read-event))
>
> on your branch appears to hang the Emacs session unquittably (even
> SIGUSR2 doesn't seem to work).  It should permit quits, because that
> code says nothing about wanting quits to be reported.

By calling read-event, you're asking Emacs read an event.  C-g is an
event.  Lisp isn't saying
read-event-unless-it's-C-g-in-which-case-exit-non-locally.  Lisp says
read-event.  Emacs is doing exactly what Lisp is asking it to do.
If Lisp wants to quit in response to a char_char event, it can just
(signal 'quit nil).

Besides, (let ((inhibit-quit t)) (while t (read-event))) will be
similarly unquittable on master today.

(while t ((read-key-sequence "))) is unquittable too.

On current master, (read-event) then C-g signals quit but
(read-key-sequence "") C-g returns a quit_char key.  Why?  Why should we
expect (while t (read-event)) to be quittable but not (while t
(read-key-sequence "")), which can't be quit today on master?  Why would
the _high level_ event reading function report quit as a character but
the low-level one exit nonlocally?

Consider, say, (let ((unread-command-events '(\?C-g))) (read-event)).
That'll return \?C-g, not quit.  Therefore, anyone who calls
(read-event) has to account for both ways of C-g being reported.
What's the benefit of this complexity?

So what if the behavior of read-event changes?  Does having read-event
report a quit as an event actually break anything considering that
_sometimes_ it already can report a C-g as an event and not a
quit signal.

We need to be consistent about how we treat C-g when reading events.
It makes no sense to quit while reading an event because in order to
quit, you must first read an event!

The current ambiguity is confusing and leads to real-world actual bugs.
A lisp-visible behavior change isn't dispositive by itself: any
resolution of the current ambiguity will result in new behavior, so we
might as well make it good new behavior.

If C-g means quit, we should resolve the inconsistency above by having
read-event quit if it finds a C-g events in a queue.  We also need to
change cmd_error to detect quits, push a synthetic quit_char to
unread_command_events, return Qt, and have the command loop look up the
event on the next loop.  That'll fix the transient bug.

If C-g means quit_char, which is the better behavior, we need to
document that functions that read events report quits as quit_char.

Thanks for pointing out the SIGUSR2 thing.  That's just a bug we'll fix
on the branch.  Likewise, we should try to get the emergency quit
mechanism working somehow too.  None of this means that (read-event)
then physically hitting C-g means (read-event) should quit rather
than return.

> Reporting them as quit_char doesn't always make sense, since there are
> other ways to generate a quit, such as SIGUSR2,

We can make SIGUSR2 generate a subtype of quit and have read_char
translate only the basic event.  Likewise for emergency quits.

> and quit_char can change.

Can it in practice?  Lots of stuff hardcodes C-g and I'm not confident
that a non-default quit_char would produce usable behavior.  I'd rather
just remove set-quit-char than worry about quit_char changing.

>>  I don't think it should do that, it doesn't
>>>match the quit-flag documentation, but it is what happens right now.  Do
>>>we really need a new function which is precisely equivalent to
>>>
>>>(let ((inhibit-quit t)) (read-event ...)) ?
>>>
>>>As for throw-on-input, I don't know what Daniel is proposing to do to
>>>handle it.  Is every caller of read-event supposed to check
>>>throw-on-input and simulate it?  How is that better than looking at
>>>quit-flag, or simply keeping inhibit-quit bound for the critical
>>>section?
>>
>> See the fix in the fix i mentioned a message ago. Now the read event
>> functions detect that they're in a context in which quitting is
>> inevitable and try to save the event before control even gets to
>> Lisp. Should be transparent change.
>
> It's pushing the event without the (cons t event) wrapper now?  Won't
> that change behavior of sit-for (and adding the wrapper would change
> behavior of other users)?
>
> Maybe we should just fix the original problem by making read_char call
> maybe_quit instead of removing an event from the queue, if Vquit_flag is
> set.

I don't think it's a good idea to have parallel event processing paths,
one for events delivered as Lisp objects and one for events delivered
as signals.

> No reason to change anything about the API, read_char already calls Lisp
> so quitting should not be a problem (famous last words).
>
> Of course, the ability to peek at/wait for events would still be a good
> thing, as would the ability to nest while-no-input invocations.
>
>>>As for peeking at events, the easiest way seems to me to be to let-bind
>>>unread-command-events to nil around a call to read-event.  That'll make
>>>it ignore them, read the next event, which you can then append to
>>>unread-command-events or not depending on whether you want the command
>>>loop to handle it.
>>
>> Isn't unread command events kind of lossy and not something we want to use all the time?
>
> How is it "kind of lossy"?  Anyway, you're using it on your branch, so
> if it's unsafe it'll be unsafe for you, too.
>
> If your concern is that unread-command-events might be updated
> asynchronously as quit-flag is, I don't think we do that.
>
>>>I agree it would make sense to separate inhibit-quit meaning "inhibit
>>>acting on the quit flag" and inhibit-quit meaning "inhibit setting the
>>>quit flag", but that seems a minor change.
>>
>> We have a lot of code that makes subtle assumptions about the meaning
>> of the quit flag. I wouldn't change it lightly. The more local changes
>> on my branch seem sufficient.
>
> Er, you just did change it, in a way that breaks existing behavior.  But
> I agree, no changes such as that one are necessary.

The above is talking about changing the low-level meaning of
Vinhibit_quit in C.  Lots of code assumes that binding Vinhibit_quit
will result in Vquit_flag being set and I'm a lot more worried about
breaking things as a result of changing _that_ behavior than I am about
making read_char consistent about how it reports C-g.

> Anyway, here are the minor changes to keyboard.c to avoid the original
> problem (the third change is somewhat independent and avoids quitting in
> kbd_buffer_get_event):

This change papers over the problem of ambiguous C-g representation
without trying to fix it or address the even worse problem of the quits
going to the event loop and not being looked up as commands.