GNU bug report logs - #22294
25.0.50; edebug should support jumping into generic methods

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 22294 in the body.
You can then email your comments to 22294 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50; edebug should support jumping into generic methods
Date: Sat, 02 Jan 2016 20:53:14 +0200

This has come up in the discussion of bug#22292. I'd like to add my +1
to the request.

Message #8 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: bug#22294: 25.0.50;
 edebug should support jumping into generic methods
Date: Thu, 02 Mar 2017 18:52:50 -0800

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> This has come up in the discussion of bug#22292. I'd like to add my +1
> to the request.

I've just sent a patch for bug#24753, which fixes an "args out of range"
error that happened when asking Edebug to step through generic methods,
after more than one with the same name had been instrumented. With the
patch in place Edebug is stepping through methods for me. Can you try
the patch and see if it also fixes the problems you were having?

Message #11 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: bug#22294: 25.0.50; edebug should support jumping into generic
 methods
Date: Tue, 11 Apr 2017 13:57:39 +0300

Hi Gemini,

On 03.03.2017 04:52, Gemini Lasswell wrote:

> I've just sent a patch for bug#24753, which fixes an "args out of range"
> error that happened when asking Edebug to step through generic methods,
> after more than one with the same name had been instrumented. With the
> patch in place Edebug is stepping through methods for me. Can you try
> the patch and see if it also fixes the problems you were having?

Thanks for pinging me, but the patch does little to the problem I'm seeing.

Specifically, edebug-step-in doesn't work to step into a function call 
that's calling a generic method.

Instead, edebug simply quits and the execution of the current command 
continues.

Example:

1. Instrument xref-collect-references.

2. Type M-x xref-find-references.

3. When point reaches the semantic-symref-perform-search call, press 'i'.

Message #14 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 22294 <at> debbugs.gnu.org
Subject: Patch (was: bug#22294: 25.0.50;
 edebug should support jumping into generic methods)
Date: Wed, 26 Apr 2017 16:12:33 -0700

[Message part 1 (text/plain, inline)]

Hi Dmitry,

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Thanks for pinging me, but the patch does little to the problem I'm seeing.
>
> Specifically, edebug-step-in doesn't work to step into a function call
> that's calling a generic method.
>
Thanks for the more specific steps to reproduce your problem. I've had a
go at fixing it and have come up with the attached patch. It works by
finding and instrumenting all the methods belonging to the generic
function you are about to step into, before the call is made.

Some limitations:

- All the methods get debug instrumentation and temporary breakpoints,
not just the one that's about to be executed. But given the potential
complexity of method dispatch, it seems that fixing that would require
some deep intertwining of Edebug and cl-generic.

- If you use edebug-step-in twice on the same generic function it will
reinstrument the methods, as opposed to using edebug-step-in twice on a
regular function where Edebug can figure out that the function is
already instrumented. Fixing that would require some way to include
dynamic elements in the :name construct of an Edebug spec so that each
method could get a unique deterministic symbol as opposed to an
anonymous generated symbol. Or it could be fixed by the "future"
described in edebug-form-data's docstring.

This automates the steps to reproduce bug#24753, so apply that patch
before you try this one.

[0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch (text/plain, attachment)]

Message #17 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: Patch
Date: Sun, 7 May 2017 05:34:23 +0300

Hi Gemini,

Thank you very much for tackling this. I've tried the patch out, and it 
seems to work well.

We can install it if nobody else has any strong objections.

On 27.04.2017 2:12, Gemini Lasswell wrote:
> Some limitations:
> 
> - All the methods get debug instrumentation and temporary breakpoints,
> not just the one that's about to be executed. But given the potential
> complexity of method dispatch, it seems that fixing that would require
> some deep intertwining of Edebug and cl-generic.

This sounds totally fine to me, at this stage. I _think_ it shouldn't be 
too hard to change this, given that all the arguments are known by the 
time edebug-step-in, but it's not a major issue.

It might change the return value of edebug-instrument-function back 
again, though.

> - If you use edebug-step-in twice on the same generic function it will
> reinstrument the methods, as opposed to using edebug-step-in twice on a
> regular function where Edebug can figure out that the function is
> already instrumented.

This is a bit wasteful. But more importantly, it causes us to collect 
the list of anonymous symbols in a dynamic variable, instead of a more 
explicit data flow. Which is not great.

> Fixing that would require some way to include
> dynamic elements in the :name construct of an Edebug spec so that each
> method could get a unique deterministic symbol as opposed to an
> anonymous generated symbol.

Ideally, we'd do this, I think. If :name spec is allowed to be a 
function, it could construct the unique symbol like (intern (format 
"%s-%s" name arguments)). Then edebug-instrument-function could also 
call this logic itself, instead of relying on the symbol being recorded 
in edebug--step-in-symbols.

(On the other hand, the proposed approach probably fixes stepping into 
any edebug-able form, not just generic methods).

> Or it could be fixed by the "future"
> described in edebug-form-data's docstring.

We'd still need to construct the unique symbol this way, at least 
somewhere, I think.

A couple notes on the patch itself:

> -  ;; Func should be a function symbol.
> -  ;; Return the function symbol, or nil if not instrumented.
> -  (let ((func-marker (get func 'edebug)))
> +  "Instrument the function or generic method FUNC.
> +Return the list of function symbols which were instrumented.
> +This may be simply (FUNC) for a normal function, or a list of
> +generated symbols for methods.  If a function or method to
> +instrument cannot be found, signal an error."
> +  (let ((func-marker (get func 'edebug))

The signature change looked worrying, but all the callers seem fine 
(there are not many of them).

> +     ((get func 'cl--generic)
> +      (let ((method-defs (method-files func)))
> +        (unless method-defs
> +          (error "Could not find any me
> 
> thod definitions for %s" func))
> +        (while method-defs
> +          (let* ((file (caar method-defs))
> +                 (spec (cdar method-defs))

It would be better to use `dolist' here, or even `pcase-dolist', see an 
example in pcase-dolist.

>   
> +
> +(require 'cl-generic)

This adds a second empty line in a row.

Message #20 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: bug#22294: Patch
Date: Sun, 07 May 2017 13:28:18 -0700

[Message part 1 (text/plain, inline)]

Dmitry Gutov <dgutov <at> yandex.ru> writes:

>> Some limitations:
>>
>> - All the methods get debug instrumentation and temporary breakpoints,
>> not just the one that's about to be executed. But given the potential
>> complexity of method dispatch, it seems that fixing that would require
>> some deep intertwining of Edebug and cl-generic.
>
> This sounds totally fine to me, at this stage. I _think_ it shouldn't
> be too hard to change this, given that all the arguments are known by
> the time edebug-step-in, but it's not a major issue.

Actually the arguments are not known because they have not yet been
evaluated when edebug-step-in is invoked. So it would have to set a
breakpoint after argument evaluation, run the code under debugging
until it gets to that breakpoint, look at the evaluated arguments,
figure out what method to instrument, instrument the method and set a
breakpoint in it, and then run again.

> This is a bit wasteful. But more importantly, it causes us to collect
> the list of anonymous symbols in a dynamic variable, instead of a more
> explicit data flow. Which is not great.

Agreed. Edebug already has far too many dynamic variables obscuring
its logic, making it appear the safest change is to simply add another
one rather than change the logic to allow more explicit data flow. I
don't think there is an easy answer. I have started writing tests for
Edebug, as a step towards making it possible to improve it without
worrying about breaking things that are working. Probably it will help
me understand the code in there better too.

> A couple notes on the patch itself:
> It would be better to use `dolist' here, or even `pcase-dolist', see
> an example in pcase-dolist.

Here's a revised patch. Thanks for letting me know about pcase-dolist
as it looks very useful. It's not documented and I didn't know it
existed.

[0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch (text/plain, attachment)]

Message #23 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: bug#22294: Patch
Date: Mon, 8 May 2017 05:19:28 +0300

[Message part 1 (text/plain, inline)]

On 07.05.2017 23:28, Gemini Lasswell wrote:

> Actually the arguments are not known because they have not yet been
> evaluated when edebug-step-in is invoked. So it would have to set a
> breakpoint after argument evaluation, run the code under debugging
> until it gets to that breakpoint, look at the evaluated arguments,
> figure out what method to instrument, instrument the method and set a
> breakpoint in it, and then run again.

Very good point, it sounds like a pain.

Fixing the edebug name for cl-defmethod seems like it should be a 
smaller effort.

I've tried to do that via a new edebug spec for method arguments, see 
the attached variation of your patch (only partially tested).

Unfortunately, this code fails when instrumenting a generic method (e.g. 
using C-u C-M-x) with something like:

Unknown specializer foo <at> setf\ \(v\ \(_y\ \(eql\ 4\)\)\ z\)

Any thoughts? edebug-match-method-args is definitely at fault there, but 
I'm not sure how to improve it.

> I have started writing tests for
> Edebug, as a step towards making it possible to improve it without
> worrying about breaking things that are working. Probably it will help
> me understand the code in there better too.

Sounds great.

> Here's a revised patch. Thanks for letting me know about pcase-dolist
> as it looks very useful. It's not documented and I didn't know it
> existed.

I usually find those via normal introspection.

Try typing 'C-h f pcase- TAB' and looking at the "public" names (without 
the double-dash in their name).

[0002-edebug-and-defmethod.diff (text/x-patch, attachment)]

Message #26 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 22294 <at> debbugs.gnu.org
Subject: Generating Edebug names for generic methods (was: bug#22294: Patch)
Date: Tue, 09 May 2017 22:07:57 -0700

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Unfortunately, this code fails when instrumenting a generic method
> (e.g. using C-u C-M-x) with something like:
>
> Unknown specializer foo <at> setf\ \(v\ \(_y\ \(eql\ 4\)\)\ z\)
>
> Any thoughts? edebug-match-method-args is definitely at fault there,
> but I'm not sure how to improve it.
>

Changing the return value of edebug-match-method-args from nil to
(list args) makes the error go away and the code sample from #24753
work. I haven't tested it beyond that.

Message #29 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294 <at> debbugs.gnu.org
Subject: Re: bug#22294: Generating Edebug names for generic methods
Date: Wed, 10 May 2017 17:18:42 +0300

On 10.05.2017 8:07, Gemini Lasswell wrote:

> Changing the return value of edebug-match-method-args from nil to
> (list args) makes the error go away and the code sample from #24753
> work. I haven't tested it beyond that.

Thanks! I only tried `args' (without (list)). My only excuse is that it 
was late at night.

So, what do you think about this modification?

It works well in my testing, and it avoids introducing the dynamic 
variable. I'll commit it in your name (since you did most of the work 
anyway) if you like it.

Message #32 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Gemini Lasswell <gazally <at> runbox.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 22294 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#22294: Generating Edebug names for generic methods
Date: Sat, 13 May 2017 13:58:08 -0700

[Message part 1 (text/plain, inline)]

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> On 10.05.2017 8:07, Gemini Lasswell wrote:
>
> It works well in my testing, and it avoids introducing the dynamic
> variable. I'll commit it in your name (since you did most of the work
> anyway) if you like it.

It works in my testing too. Here's a revised version of the patch with
your changes incorporated and a couple of other changes too. Since a
method's argument list is supposed to be a list but not nil, I changed
the test for a valid one from listp to consp.

I also changed the names of method-args and edebug-match-method-args to
cl-generic-method-args and edebug-match-cl-generic-method-args to better
associate them with the code that uses them, and to avoid the idea that
this might be new Edebug specification list functionality that should
really be documented. That documentation already is complex enough that
it's hard to understand and doesn't need any new complex things added to
it. But if you don't like the name change feel free to change it back.

[0001-Make-edebug-step-in-work-on-generic-methods-Bug-2229.patch (text/plain, attachment)]

Message #35 received at 22294 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#22294: Generating Edebug names for generic methods
Date: Sun, 14 May 2017 04:17:47 +0300

On 13.05.2017 23:58, Gemini Lasswell wrote:

> It works in my testing too. Here's a revised version of the patch with
> your changes incorporated and a couple of other changes too.

Thanks! I'll push this version shortly.

> Since a
> method's argument list is supposed to be a list but not nil, I changed
> the test for a valid one from listp to consp.

Like mentioned, nil arguments list also works (in Emacs but not in 
CLOS). If people prefer this possibility is not used, though, that's 
fine by me.

> I also changed the names of method-args and edebug-match-method-args to
> cl-generic-method-args and edebug-match-cl-generic-method-args to better
> associate them with the code that uses them, and to avoid the idea that
> this might be new Edebug specification list functionality that should
> really be documented.

Sounds good.

Message #40 received at 22294-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Gemini Lasswell <gazally <at> runbox.com>
Cc: 22294-done <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#22294: Generating Edebug names for generic methods
Date: Sun, 14 May 2017 23:33:56 +0300

On 14.05.2017 4:17, Dmitry Gutov wrote:
> I'll push this version shortly.

Done, and closing!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.