bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

This is a followup to bug#65344, which see.

When Edebug matches a debug spec

  (&or (... def-form)
       (...))

and the form matched against is not something def-form matches,
an invalid-read-error is signaled, regardless of the context in which
def-form appears.  That means the second sub-clause of the &or never
gets a chance to match.

This is not limited to def-form and not limited to &or (think
&not, for example).

In GNU Emacs 30.0.50 (build 2, aarch64-apple-darwin22.6.0, NS
 appkit-2299.70 Version 13.5 (Build 22G74)) of 2023-08-24 built on
 Mini.fritz.box
Repository revision: 53c07bd04bf59f63e49af2c626714bf3fdd03ad6
Repository branch: scratch/pkg
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.5

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> This is a followup to bug#65344, which see.
>
> When Edebug matches a debug spec
>
>   (&or (... def-form)
>        (...))
>
> and the form matched against is not something def-form matches,
> an invalid-read-error is signaled, regardless of the context in which
> def-form appears.  That means the second sub-clause of the &or never
> gets a chance to match.
>
> This is not limited to def-form and not limited to &or (think
> &not, for example).
>
> In GNU Emacs 30.0.50 (build 2, aarch64-apple-darwin22.6.0, NS
>  appkit-2299.70 Version 13.5 (Build 22G74)) of 2023-08-24 built on
>  Mini.fritz.box
> Repository revision: 53c07bd04bf59f63e49af2c626714bf3fdd03ad6
> Repository branch: scratch/pkg
> Windowing system distributor 'Apple', version 10.3.2299
> System Description:  macOS 13.5

Hi Stefan,

I have a question that I hope you can help me with because you did a lot
of improvemnts there.  The question is not very important, but it's one
of those times when I can't make sense of something, and that starts
nagging me a bit :-).

My question is about edebug-no-match.  Do you perhaps have an idea what
the reason might be why it ever chooses to signal an error instead of
just throwing no-match?

I've looked through the history of edebug.el, to find out what the
use-case for signaling is, but that's difficult to follow, and I failed
to find the answer.  Probably I'm overlooking something obvious, as
usual :-).

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> My question is about edebug-no-match.  Do you perhaps have an idea what
> the reason might be why it ever chooses to signal an error instead of
> just throwing no-match?

`edebug-no-match` is the only place where we test `edebug-gate`, so if
we make it throw to `no-match` it would make `gate` into a no-op.

(`gate` doesn't occur explicitly in your example, but it's implicitly
present inside other things like `&define`, and hence `def-form`, IIRC).

In a sense `gate` is supposed to be a bit like Prolog's "cut", but
Edebug isn't quite like Prolog (e.g. it doesn't really do backtracking;
it works more like a PEG parser) and similarly its `gate` isn't the same
as "cut".

See bug#41988 for a case where we didn't want a failure in one
"definition form" to be allowed to continue matching in a second branch
of an `&or` (tho this was arguably because some of the code executed
along the way had side-effects that can't be undone).

:-(


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

(Adding Michael in CC because he might be interested.)

>> My question is about edebug-no-match.  Do you perhaps have an idea what
>> the reason might be why it ever chooses to signal an error instead of
>> just throwing no-match?
>
> `edebug-no-match` is the only place where we test `edebug-gate`, so if
> we make it throw to `no-match` it would make `gate` into a no-op.

Yes.

> (`gate` doesn't occur explicitly in your example, but it's implicitly
> present inside other things like `&define`, and hence `def-form`,
> IIRC).

Correct.

> In a sense `gate` is supposed to be a bit like Prolog's "cut", but
> Edebug isn't quite like Prolog (e.g. it doesn't really do backtracking;
> it works more like a PEG parser) and similarly its `gate` isn't the same
> as "cut".

Right.  

> See bug#41988 for a case where we didn't want a failure in one
> "definition form" to be allowed to continue matching in a second branch
> of an `&or` (tho this was arguably because some of the code executed
> along the way had side-effects that can't be undone).

Hurrah! :-)  Yes, that's it!  Thank you so much, that made my day!


As a fix, I'd like to propose to remove 'gate' as a debug spec entirely.

Reasons are:

- I doubt it's very useful to signal invalid-read-error while matching.
In fact, I found it very confusing, when I got it, wondering how the
Lisp reader comes into play here.  And the signal didn't give me any
helpful info either.

- I can't imagine anyone using 'gate' as it is.  I just don't see a
use-case for it.

I know that could be considered radical.  But, so what?

WDYT?

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> See bug#41988 for a case where we didn't want a failure in one
>> "definition form" to be allowed to continue matching in a second branch
>> of an `&or` (tho this was arguably because some of the code executed
>> along the way had side-effects that can't be undone).
>
> Hurrah! :-)  Yes, that's it!  Thank you so much, that made my day!
>
> As a fix, I'd like to propose to remove 'gate' as a debug spec entirely.

It's worth a try but:

- We should come up with a better fix for bug#41988/bug#41853 first.
- We need to check the impact on current users of that gate mechanism.

For the second, `grep -C9 debug **/*.el | grep '\<gate\>'`
finds only one direct user of `gate` inside Emacs or (Non)GNU ELPA,
i.e. `cl-macs.el`

    lisp/emacs-lisp/cl-macs.el:1224:;;   '(&or (loop-var . [&or nil loop-var]) [gate symbolp]))
    lisp/emacs-lisp/cl-macs.el:2858:                                          (gate gv-place &optional form)])
    lisp/emacs-lisp/cl-macs.el:2989:                  (gate ;; FIXME: Why?

but there might be users elsewhere.  And more importantly it's used
within

- strings
- `&define`
- quoted symbols?

The last one seems to be an old deprecated mechanism (a comment suggests
it should be replaced by a string) that just has never been actively
deprecated, so maybe we can disregard it.
But the first two are used in a lot of places, so it might be more problematic.


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> (`gate` doesn't occur explicitly in your example, but it's implicitly
>> present inside other things like `&define`, and hence `def-form`,
>> IIRC).
> Correct.

Hmm... actually, looking at `def-form` again it seems it doesn't use
`&define` nor `(edebug-)gate`.

Now I'm confused.


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hmm... actually, looking at `def-form` again it seems it doesn't use
> `&define` nor `(edebug-)gate`.

Dunno if this helps, I'm understanding it only half, but AFAIU,
`def-form` falls back to `edebug-form' which looks at &define and uses
`edebug-gate' indirectly when calling `edebug-list-form'.

Michael.

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Heerdegen [2023-08-28 00:58:31] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Hmm... actually, looking at `def-form` again it seems it doesn't use
>> `&define` nor `(edebug-)gate`.
> Dunno if this helps, I'm understanding it only half, but AFAIU,
> `def-form` falls back to `edebug-form' which looks at &define and uses
> `edebug-gate' indirectly when calling `edebug-list-form'.

Hmm... Could be, indeed.  In any case, this "gate" business is
quite messy.

I'd be tempted to start removing uses of it, bit by bit, to try and see
what breaks.  And if needed, maybe add a new replacement for it that
would be better defined (I'm imagining a kind of "scoped gate", which
could look like `[&gate-in ... [&gate-lock ...SPECS...] ...]` such
that if `...SPECS...` fails to match, we propagate this failure
immediately up to the `gate-in`).

This way

    [&gate-in [&or ["foo" &gate-lock ...]
                   ["foo" "bar"]]]

would never fallback to ["foo" "bar"] whereas

    [&or [&gate-in ["foo" &gate-lock ...]]
         ["foo" "bar"]]]

would fallback to ["foo" "bar"] if "..." fails to match.


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> See bug#41988 for a case where we didn't want a failure in one
>>> "definition form" to be allowed to continue matching in a second branch
>>> of an `&or` (tho this was arguably because some of the code executed
>>> along the way had side-effects that can't be undone).
>>
>> Hurrah! :-)  Yes, that's it!  Thank you so much, that made my day!
>>
>> As a fix, I'd like to propose to remove 'gate' as a debug spec entirely.
>
> It's worth a try but:
>
> - We should come up with a better fix for bug#41988/bug#41853 first.
> - We need to check the impact on current users of that gate mechanism.
>
> For the second, `grep -C9 debug **/*.el | grep '\<gate\>'`
> finds only one direct user of `gate` inside Emacs or (Non)GNU ELPA,
> i.e. `cl-macs.el`
>
>     lisp/emacs-lisp/cl-macs.el:1224:;;   '(&or (loop-var . [&or nil loop-var]) [gate symbolp]))
>     lisp/emacs-lisp/cl-macs.el:2858:                                          (gate gv-place &optional form)])
>     lisp/emacs-lisp/cl-macs.el:2989:                  (gate ;; FIXME: Why?
>
> but there might be users elsewhere.  And more importantly it's used
> within
>
> - strings
> - `&define`
> - quoted symbols?
>
> The last one seems to be an old deprecated mechanism (a comment suggests
> it should be replaced by a string) that just has never been actively
> deprecated, so maybe we can disregard it.
> But the first two are used in a lot of places, so it might be more problematic.

I'd change all these, but I can understand if that's too radical for
your taste :-).

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> Hmm... actually, looking at `def-form` again it seems it doesn't use
>> `&define` nor `(edebug-)gate`.
>
> Dunno if this helps, I'm understanding it only half, but AFAIU,
> `def-form` falls back to `edebug-form' which looks at &define and uses
> `edebug-gate' indirectly when calling `edebug-list-form'.

I think that's correct.

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Michael Heerdegen [2023-08-28 00:58:31] wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Hmm... actually, looking at `def-form` again it seems it doesn't use
>>> `&define` nor `(edebug-)gate`.
>> Dunno if this helps, I'm understanding it only half, but AFAIU,
>> `def-form` falls back to `edebug-form' which looks at &define and uses
>> `edebug-gate' indirectly when calling `edebug-list-form'.
>
> Hmm... Could be, indeed.  In any case, this "gate" business is
> quite messy.

Yup :-)

>
> I'd be tempted to start removing uses of it, bit by bit, to try and see
> what breaks.  And if needed, maybe add a new replacement for it that
> would be better defined (I'm imagining a kind of "scoped gate", which
> could look like `[&gate-in ... [&gate-lock ...SPECS...] ...]` such
> that if `...SPECS...` fails to match, we propagate this failure
> immediately up to the `gate-in`).
>
> This way
>
>     [&gate-in [&or ["foo" &gate-lock ...]
>                    ["foo" "bar"]]]
>
> would never fallback to ["foo" "bar"] whereas
>
>     [&or [&gate-in ["foo" &gate-lock ...]]
>          ["foo" "bar"]]]
>
> would fallback to ["foo" "bar"] if "..." fails to match.

That's a possibility.  I can't say much more because I fail to
understand the motivation why gate is used in the first place.  What did
the developers using it want to achieve?

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I'd change all these, but I can understand if that's too radical for
> your taste :-).

In any case, we first have to find another solution to bug#41988.
I.e. a way to "undo" the effects of `edebug-make-form-wrapper`
or to change `edebug-make-form-wrapper` so that it matches SPECS
*first* and only when/if that's done do the rest (so there's no need to
"undo").


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'd change all these, but I can understand if that's too radical for
>> your taste :-).
>
> In any case, we first have to find another solution to bug#41988.
> I.e. a way to "undo" the effects of `edebug-make-form-wrapper`
> or to change `edebug-make-form-wrapper` so that it matches SPECS
> *first* and only when/if that's done do the rest (so there's no need to
> "undo").

Right.

To summarize: The "culprit" is this commit:

commit 53dfd85a7f971875e716a55f010ee508bce89eed
Author: Philipp Stephani <phst@google.com>
Date:   Thu Mar 18 12:40:08 2021 +0100

    Edebug: Disable backtracking when hitting a &define keyword.
    
    Edebug doesn't deal well with backtracking out of definitions, see
    Bug#41988.  Rather than trying to support this rare situation (e.g. by
    implementing a multipass parser), prevent it by adding an implicit
    gate.
    
    * lisp/emacs-lisp/edebug.el (edebug--match-&-spec-op): Disable
    backtracking when hitting a &define keyword.
    
    * test/lisp/emacs-lisp/edebug-tests.el
    (edebug-tests-duplicate-&define): New unit test.
    (edebug-tests--duplicate-&define): New helper macro.
    
    * doc/lispref/edebug.texi (Backtracking): Mention &define in the list
    of constructs that disable backtracking.
    
    * etc/NEWS: Document new behavior.

This commit causes &define to signal if it doesn't match:

--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -1942,14 +1942,16 @@ edebug--match-&-spec-op
   ;; Normally, &define is interpreted specially other places.
   ;; This should only be called inside of a spec list to match the remainder
   ;; of the current list.  e.g. ("lambda" &define args def-body)
-   (edebug-make-form-wrapper
-    cursor
-    (edebug-before-offset cursor)
-    ;; Find the last offset in the list.
-    (let ((offsets (edebug-cursor-offsets cursor)))
-      (while (consp offsets) (setq offsets (cdr offsets)))
-      offsets)
-    specs))
+  (prog1 (edebug-make-form-wrapper
+          cursor
+          (edebug-before-offset cursor)
+          ;; Find the last offset in the list.
+          (let ((offsets (edebug-cursor-offsets cursor)))
+            (while (consp offsets) (setq offsets (cdr offsets)))
+            offsets)
+          specs)
+    ;; Stop backtracking here (Bug#41988).
+    (setq edebug-gate t)))

The reason for making it signal is that e-m-f-wrapper doesn't take into
account that matching might fail, and instead modifies global state.
For example, it changes symbol properties.  (Maybe also other global
state, I haven't checked.)

Do you think it would be possible to let e-m-f-wrapper just put
something on a new "list of actions to be performed oncee the whole
debug spec has been matched"?  I'm thinking of closures as "actions",
here.

That would be the first thing coming to my mind.  I'd find that easier
to follow than an undo.

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> I'd change all these, but I can understand if that's too radical for
>>> your taste :-).
>>
>> In any case, we first have to find another solution to bug#41988.
>> I.e. a way to "undo" the effects of `edebug-make-form-wrapper`
>> or to change `edebug-make-form-wrapper` so that it matches SPECS
>> *first* and only when/if that's done do the rest (so there's no need to
>> "undo").
>
> Right.
>
> To summarize: The "culprit" is this commit:

Have you confirmed experimentally that this is the culprit for your
use case?  Do you have a recipe?

> +  (prog1 (edebug-make-form-wrapper
> +          cursor
> +          (edebug-before-offset cursor)
> +          ;; Find the last offset in the list.
> +          (let ((offsets (edebug-cursor-offsets cursor)))
> +            (while (consp offsets) (setq offsets (cdr offsets)))
> +            offsets)
> +          specs)
> +    ;; Stop backtracking here (Bug#41988).
> +    (setq edebug-gate t)))
[...]
> Do you think it would be possible to let e-m-f-wrapper just put
> something on a new "list of actions to be performed oncee the whole
> debug spec has been matched"?  I'm thinking of closures as "actions",
> here.

Maybe, but at the same time, when I read `edebug-make-form-wrapper`
I get the impression that it matches `specs` quite early on, before it
does much damage to the global state, so I think I'd need to step
through the code to better understand what was the problem that the
patch intended to fix.


        Stefan

bug#65516: 30.0.50; Edebug behavior of signaling errors in &or

[Gerd Möllmann]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> To summarize: The "culprit" is this commit:
>
> Have you confirmed experimentally that this is the culprit for your
> use case?  

No, I didn't experiment.  I found the circumstancial evidence sufficient
for me.  Of course, there might be additional stuff and so on, as usual.

> Do you have a recipe?

Checkout cc0f546825e whidh should have an cl-flet where the first clause
of the &or has a def-form in it.  Then run cl-macs-tests and/or
edebug-tests.  IIRC, Stefan Kangas noticed that, which led to the
current bug.

>> +  (prog1 (edebug-make-form-wrapper
>> +          cursor
>> +          (edebug-before-offset cursor)
>> +          ;; Find the last offset in the list.
>> +          (let ((offsets (edebug-cursor-offsets cursor)))
>> +            (while (consp offsets) (setq offsets (cdr offsets)))
>> +            offsets)
>> +          specs)
>> +    ;; Stop backtracking here (Bug#41988).
>> +    (setq edebug-gate t)))
> [...]
>> Do you think it would be possible to let e-m-f-wrapper just put
>> something on a new "list of actions to be performed oncee the whole
>> debug spec has been matched"?  I'm thinking of closures as "actions",
>> here.
>
> Maybe, but at the same time, when I read `edebug-make-form-wrapper`
> I get the impression that it matches `specs` quite early on, before it
> does much damage to the global state, so I think I'd need to step
> through the code to better understand what was the problem that the
> patch intended to fix.

Ok, thanks.

I'm thinking of how to proceed from here.

IMO, there's at this point enough info now that someone (tm) could fix
this, but the cost/benefit ration is sp high that I at least can't bring
me to to do it.  I can only offer to put it on my todo list, which is,
TBH, rather a to-procrastinate list.

Any other takers?

If not, I'd say let's close this or what else seems appropriate to the
maintainers.