bug#66979: Wrong number of arguments with completion-at-point

bug#66979: Wrong number of arguments with completion-at-point

[Juri Linkov]

Some recent change broke completion-at-point:

0. emacs -Q
1. (setq debug-on-error t)
2. Type in the *scratch*:

  (defun a M-C-i

raises the error (wrong-number-of-arguments (2 . 2) 1)

bug#66979: Wrong number of arguments with completion-at-point

[Juri Linkov]

> Some recent change broke completion-at-point:
>
> 0. emacs -Q
> 1. (setq debug-on-error t)
> 2. Type in the *scratch*:
>
>   (defun a M-C-i
>
> raises the error (wrong-number-of-arguments (2 . 2) 1)

Stefan, this looks like another case for the recently fixed
bug#58148, bug#58396, bug#58557.

bug#66979: Wrong number of arguments with completion-at-point

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

>> Some recent change broke completion-at-point:
>>
>> 0. emacs -Q
>> 1. (setq debug-on-error t)
>> 2. Type in the *scratch*:
>>
>>   (defun a M-C-i
>>
>> raises the error (wrong-number-of-arguments (2 . 2) 1)
>
> Stefan, this looks like another case for the recently fixed
> bug#58148, bug#58396, bug#58557.

So you're suggesting we should remove the `debug` from the
`condition-case` in `elisp--local-variables`?

This was added by Alan in:

    commit f931cebce76d911dfc61274e0a8c1de3627b9179
    Author: Alan Mackenzie <acm@muc.de>
    Date:   Wed Sep 20 15:51:17 2023 +0000

    Insert symbol `debug' into two condition-case handlers
    
    This fixes bug#65622.  Also correct a mismatch between a
    function to which advice is added, and that from which it is
    removed.
    
    * lisp/emacs-lisp/macroexp.el (internal-macroexpand-for-load):
    Add a `debug' to the condition-case handler for `error', so
    that a useful backtrace will be produced on a macro expansion
    error.
    
    * lisp/progmodes/elisp-mode.el (elisp--local-variables): Add
    `debug' to a condition-case handler, as above.  In the
    advice-remove call, give the same function, macroexpand-1, as
    in the corresponding advice-add call.

Alan do you remember why you also added the `debug` to the
condition-case in `elisp--local-variables`?
The rest of the commit looks right to me.

Macro expansion errors in there are perfectly normal since
`elisp--local-variables` routinely passes incomplete code to
macroexpand.  IOW most errors signal'd in there probably don't need to
be debugged at all.


        Stefan

bug#66979: Wrong number of arguments with completion-at-point

[Alan Mackenzie]

Hello, Stefan

On Tue, Nov 07, 2023 at 13:31:40 -0500, Stefan Monnier wrote:
> >> Some recent change broke completion-at-point:

> >> 0. emacs -Q
> >> 1. (setq debug-on-error t)
> >> 2. Type in the *scratch*:

> >>   (defun a M-C-i

> >> raises the error (wrong-number-of-arguments (2 . 2) 1)

> > Stefan, this looks like another case for the recently fixed
> > bug#58148, bug#58396, bug#58557.

> So you're suggesting we should remove the `debug` from the
> `condition-case` in `elisp--local-variables`?

> This was added by Alan in:

>     commit f931cebce76d911dfc61274e0a8c1de3627b9179
>     Author: Alan Mackenzie <acm@muc.de>
>     Date:   Wed Sep 20 15:51:17 2023 +0000

>     Insert symbol `debug' into two condition-case handlers
    
>     This fixes bug#65622.  Also correct a mismatch between a
>     function to which advice is added, and that from which it is
>     removed.
    
>     * lisp/emacs-lisp/macroexp.el (internal-macroexpand-for-load):
>     Add a `debug' to the condition-case handler for `error', so
>     that a useful backtrace will be produced on a macro expansion
>     error.
    
>     * lisp/progmodes/elisp-mode.el (elisp--local-variables): Add
>     `debug' to a condition-case handler, as above.  In the
>     advice-remove call, give the same function, macroexpand-1, as
>     in the corresponding advice-add call.

> Alan do you remember why you also added the `debug` to the
> condition-case in `elisp--local-variables`?
> The rest of the commit looks right to me.

I was trying to debug an error in eager macro expansion (i.e. macro
expansion in forms called directly by read), and that was the
condition-case that was suppressing the backtrace.

> Macro expansion errors in there are perfectly normal since
> `elisp--local-variables` routinely passes incomplete code to
> macroexpand.  IOW most errors signal'd in there probably don't need to
> be debugged at all.

But when somebody has set debug-on-error to t, they _want_ those errors
signalled, surely?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66979: Wrong number of arguments with completion-at-point

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

>>     commit f931cebce76d911dfc61274e0a8c1de3627b9179
>>     Author: Alan Mackenzie <acm@muc.de>
>>     Date:   Wed Sep 20 15:51:17 2023 +0000
>
>>     Insert symbol `debug' into two condition-case handlers
>     
>>     This fixes bug#65622.  Also correct a mismatch between a
>>     function to which advice is added, and that from which it is
>>     removed.
>     
>>     * lisp/emacs-lisp/macroexp.el (internal-macroexpand-for-load):
>>     Add a `debug' to the condition-case handler for `error', so
>>     that a useful backtrace will be produced on a macro expansion
>>     error.
>     
>>     * lisp/progmodes/elisp-mode.el (elisp--local-variables): Add
>>     `debug' to a condition-case handler, as above.  In the
>>     advice-remove call, give the same function, macroexpand-1, as
>>     in the corresponding advice-add call.
>
>> Alan do you remember why you also added the `debug` to the
>> condition-case in `elisp--local-variables`?
>> The rest of the commit looks right to me.
>
> I was trying to debug an error in eager macro expansion (i.e. macro
> expansion in forms called directly by read), and that was the
> condition-case that was suppressing the backtrace.

Really?  I'd expect this case to go through the `condition-case` that's
in `internal-macroexpand-for-load` but not the condition-case that's in
`elisp--local-variables`.

Any chance you can still reproduce the bug and get a backtrace showing
how `elisp--local-variables` gets involved?

>> Macro expansion errors in there are perfectly normal since
>> `elisp--local-variables` routinely passes incomplete code to
>> macroexpand.  IOW most errors signal'd in there probably don't need to
>> be debugged at all.
> But when somebody has set debug-on-error to t, they _want_ those errors
> signalled, surely?

No, I have it set and don't want to be told that the internal completion
machinery extracted broken code from the current buffer in its
best-effort attempt to compute the set of surrounding lexical variables.

In 99% of the cases, it is neither a bug in the code I'm editing nor in
the macros.  The design of `elisp--local-variables` is such that it
often builds syntactically invalid code to pass to the macro expander.


        Stefan

bug#66979: Wrong number of arguments with completion-at-point

[Alan Mackenzie]

Hello, Stefan.

On Tue, Nov 07, 2023 at 15:13:53 -0500, Stefan Monnier wrote:
> >>     commit f931cebce76d911dfc61274e0a8c1de3627b9179
> >>     Author: Alan Mackenzie <acm@muc.de>
> >>     Date:   Wed Sep 20 15:51:17 2023 +0000

> >>     Insert symbol `debug' into two condition-case handlers

> >>     This fixes bug#65622.  Also correct a mismatch between a
> >>     function to which advice is added, and that from which it is
> >>     removed.

> >>     * lisp/emacs-lisp/macroexp.el (internal-macroexpand-for-load):
> >>     Add a `debug' to the condition-case handler for `error', so
> >>     that a useful backtrace will be produced on a macro expansion
> >>     error.

> >>     * lisp/progmodes/elisp-mode.el (elisp--local-variables): Add
> >>     `debug' to a condition-case handler, as above.  In the
> >>     advice-remove call, give the same function, macroexpand-1, as
> >>     in the corresponding advice-add call.

> >> Alan do you remember why you also added the `debug` to the
> >> condition-case in `elisp--local-variables`?
> >> The rest of the commit looks right to me.

> > I was trying to debug an error in eager macro expansion (i.e. macro
> > expansion in forms called directly by read), and that was the
> > condition-case that was suppressing the backtrace.

> Really?  I'd expect this case to go through the `condition-case` that's
> in `internal-macroexpand-for-load` but not the condition-case that's in
> `elisp--local-variables`.

> Any chance you can still reproduce the bug and get a backtrace showing
> how `elisp--local-variables` gets involved?

It's difficult, no I can't get the backtrace, it is being suppressed by
some condition-case somewhere.  But I do get the error message "Ignoring
macroexpansion error: (void-function edebug-after)".  That "Ignoring
macroexpansion error" comes from elisp--local-variables.

To get this, from a reasonably up to date master:
(i) git checkout 1d46bca1^.
(ii) make -j<whatever> bootstrap.
(iii) Follow the recipe in bug #65622, including (setq debug-on-error t).
(iv) Repeat the C-u C-M-x in the recipe several times, until it no longer
outputs a backtrace.

The message one now sees in the echo area is the "Ignoring macroexpansion
error:" one.

What has happened is that the advice macroexpand-advice in
elisp--local-variables has been applied to macroexpand-1, and due to the
typo there, never gets removed (now fixed with bug #65622).  This piece
of advice is what suppresses the backtrace.

> >> Macro expansion errors in there are perfectly normal since
> >> `elisp--local-variables` routinely passes incomplete code to
> >> macroexpand.  IOW most errors signal'd in there probably don't need to
> >> be debugged at all.
> > But when somebody has set debug-on-error to t, they _want_ those errors
> > signalled, surely?

> No, I have it set and don't want to be told that the internal completion
> machinery extracted broken code from the current buffer in its
> best-effort attempt to compute the set of surrounding lexical variables.

> In 99% of the cases, it is neither a bug in the code I'm editing nor in
> the macros.  The design of `elisp--local-variables` is such that it
> often builds syntactically invalid code to pass to the macro expander.

Which is anything but obvious from the (lack of) comments around that
condition case, and in the function in general.

But I think I added the debug to that condition-case handler before
spotting and correcting the typo in macroexpand[-1].  So it may well be
that that debug could be removed without great damage.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#66979: Wrong number of arguments with completion-at-point

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

> What has happened is that the advice macroexpand-advice in
> elisp--local-variables has been applied to macroexpand-1, and due to the
> typo there, never gets removed (now fixed with bug #65622).  This piece
> of advice is what suppresses the backtrace.

Ah, right, so we're actually not inside `elisp--local-variables` any
more, it's just that the advice was left by error.

> But I think I added the debug to that condition-case handler before
> spotting and correcting the typo in macroexpand[-1].  So it may well be
> that that debug could be removed without great damage.

Exactly.  Thanks,


        Stefan

bug#66979: Wrong number of arguments with completion-at-point

[Juri Linkov]

close 66979 30.0.50
thanks

>> But I think I added the debug to that condition-case handler before
>> spotting and correcting the typo in macroexpand[-1].  So it may well be
>> that that debug could be removed without great damage.
>
> Exactly.  Thanks,

So now debug is removed from `elisp--local-variables`.

bug#66979: Wrong number of arguments with completion-at-point

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

> So now debug is removed from `elisp--local-variables`.

Thanks, Juri,


        Stefan