Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Dmitry Gutov]

Thank you, this is a considerable improvement. See comments below.

On 08/11/2015 05:56 AM, Stephen Leake wrote:
> branch: master
> commit d7df36e745a5ba480559b6c8b5ebc93a18fe9bd1
> Author: Stephen Leake <stephen_leake@stephe-leake.org>
> Commit: Stephen Leake <stephen_leake@stephe-leake.org>
>
>      Always output all available definitions.

How come you saw fit to undo the tweaks that I've added over time?

In particular, now jumping to whitespace-mode will show two entries, 
even though they both lead to one location. That's a waste of time.

> -(defvar elisp--xref-format
> +(defconst elisp--xref-format
>     (let ((str "(%s %s)"))
>       (put-text-property 1 3 'face 'font-lock-keyword-face str)
>       (put-text-property 4 6 'face 'font-lock-function-name-face str)
>       str))

I'm not sure which part of the change did that, but now I don't see the 
colors in the output.

Even though the approach is questionable, the result should be worth 
keeping.

> +(defconst elisp--xref-format-cl-defmethod
> +  (let ((str "(%s %s %s)"))
> +    (put-text-property 1 3 'face 'font-lock-keyword-face str)
> +    (put-text-property 4 6 'face 'font-lock-function-name-face str)
> +    str))
> +
> +(defcustom find-feature-regexp

I think these should still go into find-func.el. Even if you can't 
require it at the top of elisp-mode.el, you can do that at the beginning 
of elisp--xref-find-definitions. If that's actually needed.

> +	   ((setq generic (cl--generic symbol))
> +	    (dolist (method (cl--generic-method-table generic))
> +	      (let* ((info (cl--generic-method-info method))
> +		     (met-name (cons symbol (cl--generic-method-specializers method)))
> +		     (descr (format elisp--xref-format-cl-defmethod 'cl-defmethod symbol (nth 1 info)))
> +		     (file (find-lisp-object-file-name met-name 'cl-defmethod)))

This should also account, if possible, for the default method 
definitions performed by cl-defgeneric (and hide them).

For instance, looking for project-search-path will display three 
entries, but the last two both lead to its generic definition. This is 
actually inaccurate as well, since both the first and the second item 
come from cl-defgeneric.

> +;; When tests are run from the Makefile, 'default-directory' is $HOME,
> +;; so we must provide this dir to expand-file-name in the expected
> +;; results. The Makefile sets EMACS_TEST_DIRECTORY.
> +(defconst emacs-test-dir (getenv "EMACS_TEST_DIRECTORY"))

You could also use (or load-file-name (buffer-file-name)). That has the 
benefit of being usable when tests are run inside the interactive 
session. Not via Makefile.

> +;; Source for both variable and defun is "(define-minor-mode
> +;; compilation-minor-mode". There is no way to tell that from the
> +;; symbol.

(and (fboundp sym)
      (memq sym minor-mode-list))

seemed to be a decent indicator.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stefan Monnier]

>> -(defvar elisp--xref-format
>> +(defconst elisp--xref-format
>> (let ((str "(%s %s)"))
>> (put-text-property 1 3 'face 'font-lock-keyword-face str)
>> (put-text-property 4 6 'face 'font-lock-function-name-face str)
>> str))

> I'm not sure which part of the change did that, but now I don't see the
> colors in the output.

I can shed some light here:
- because it's now a defconst, the value is purecopy'd (since
  elisp-mode is preloaded).
- purecopy currently doesn't know how to copy string's text properties
  so they're just thrown away.

I know because I bumped into that a few times in the last year or so.

So the quick fix is to use `defvar' (which doesn't purecopy its value),
and the better fix would be to improve purecopy to not throw away
those properties.


        Stefan

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Dmitry Gutov]

On 08/11/2015 07:25 PM, Stefan Monnier wrote:

> So the quick fix is to use `defvar' (which doesn't purecopy its value),

Oh, right. That's actually the reason I've used defvar originally.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08/11/2015 07:25 PM, Stefan Monnier wrote:
>
>> So the quick fix is to use `defvar' (which doesn't purecopy its value),
>
> Oh, right. That's actually the reason I've used defvar originally.

That's what comments are for; so other people can learn from your
experience, and not repeat the same mistakes.

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Dmitry Gutov]

On 08/12/2015 02:36 AM, Stephen Leake wrote:

>> Oh, right. That's actually the reason I've used defvar originally.
>
> That's what comments are for; so other people can learn from your
> experience, and not repeat the same mistakes.

Fair enough.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

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

>>> -(defvar elisp--xref-format
>>> +(defconst elisp--xref-format
>>> (let ((str "(%s %s)"))
>>> (put-text-property 1 3 'face 'font-lock-keyword-face str)
>>> (put-text-property 4 6 'face 'font-lock-function-name-face str)
>>> str))
>
>> I'm not sure which part of the change did that, but now I don't see the
>> colors in the output.
>
> I can shed some light here:
> - because it's now a defconst, the value is purecopy'd (since
>   elisp-mode is preloaded).
> - purecopy currently doesn't know how to copy string's text properties
>   so they're just thrown away.

> and the better fix would be to improve purecopy to not throw away
> those properties.

I filed bug#21237 for this.

In the meantime, we could at least document this behavior in the
defconst docstring.

Can we add a warning somewhere in the load/dump process for this?

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stefan Monnier]

> Can we add a warning somewhere in the load/dump process for this?

We already do, alloc.c:5357:

      if (XSTRING (obj)->intervals)
	message ("Dropping text-properties when making string pure");


-- Stefan

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 12 Aug 2015 10:08:00 -0400
> Cc: emacs-devel@gnu.org
> 
> > Can we add a warning somewhere in the load/dump process for this?
> 
> We already do, alloc.c:5357:
> 
>       if (XSTRING (obj)->intervals)
> 	message ("Dropping text-properties when making string pure");

Can this message include some data that will make it easier to find
the offending Lisp expression?

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stefan Monnier]

> Can this message include some data that will make it easier to find
> the offending Lisp expression?

Feel free to make it print the actual string, yes.


        Stefan

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: stephen_leake@stephe-leake.org,  emacs-devel@gnu.org
> Date: Wed, 12 Aug 2015 13:16:03 -0400
> 
> > Can this message include some data that will make it easier to find
> > the offending Lisp expression?
> 
> Feel free to make it print the actual string, yes.

Done.

Btw, there are 2 such defconst's in elisp-mode.el, but only the 2nd
one triggers the warning.  Why?

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stefan Monnier]

> Btw, there are 2 such defconst's in elisp-mode.el, but only the 2nd
> one triggers the warning.  Why?

Good question,


        Stefan

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: stephen_leake@stephe-leake.org,  emacs-devel@gnu.org
> Date: Thu, 13 Aug 2015 11:59:30 -0400
> 
> > Btw, there are 2 such defconst's in elisp-mode.el, but only the 2nd
> > one triggers the warning.  Why?
> 
> Good question,

You will find the (almost trivial) answer in commit 9d053b3.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: stephen_leake@stephe-leake.org,  emacs-devel@gnu.org
>> Date: Wed, 12 Aug 2015 13:16:03 -0400
>> 
>> > Can this message include some data that will make it easier to find
>> > the offending Lisp expression?
>> 
>> Feel free to make it print the actual string, yes.
>
> Done.
>
> Btw, there are 2 such defconst's in elisp-mode.el, but only the 2nd
> one triggers the warning.  Why?

That explains why I didn't see the warning when I tested for it; I only
had the first defconst present.

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

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

>> Can we add a warning somewhere in the load/dump process for this?
>
> We already do, alloc.c:5357:
>
>       if (XSTRING (obj)->intervals)
> 	message ("Dropping text-properties when making string pure");

Doesn't seem to work in this case. With 'defconst' in elisp-mode.c, I
get:

...
Loading tooltip...
Loading leim/leim-list.el (source)...
Finding pointers to doc strings...
Finding pointers to doc strings...done
Pure-hashed: 23816 strings, 3762 vectors, 38262 conses, 3665 bytecodes, 105 others
Dumping under the name emacs
2762775 pure bytes used
Adding name emacs-25.0.50.12.exe
Dumping from C:/Projects/emacs/master-build-mingw64/src/temacs.exe
          to C:/Projects/emacs/master-build-mingw64/src/emacs.exe
: paxctl -zex emacs.exe
ln -f emacs.exe bootstrap-emacs.exe
...

No occurrence of "dropping" anywhere in the build log.

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Wed, 12 Aug 2015 14:42:29 -0500
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> Can we add a warning somewhere in the load/dump process for this?
> >
> > We already do, alloc.c:5357:
> >
> >       if (XSTRING (obj)->intervals)
> > 	message ("Dropping text-properties when making string pure");
> 
> Doesn't seem to work in this case. With 'defconst' in elisp-mode.c, I
> get:
> 
> ...
> Loading tooltip...
> Loading leim/leim-list.el (source)...
> Finding pointers to doc strings...
> Finding pointers to doc strings...done
> Pure-hashed: 23816 strings, 3762 vectors, 38262 conses, 3665 bytecodes, 105 others
> Dumping under the name emacs
> 2762775 pure bytes used
> Adding name emacs-25.0.50.12.exe
> Dumping from C:/Projects/emacs/master-build-mingw64/src/temacs.exe
>           to C:/Projects/emacs/master-build-mingw64/src/emacs.exe
> : paxctl -zex emacs.exe
> ln -f emacs.exe bootstrap-emacs.exe
> ...
> 
> No occurrence of "dropping" anywhere in the build log.

??? I do see it here:

  Loading progmodes/prog-mode...
  Loading emacs-lisp/lisp-mode...
  Loading progmodes/elisp-mode...
  Dropping text-properties when making string pure
  Loading textmodes/text-mode...

Are you looking in the correct place of the correct build?

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Thank you, this is a considerable improvement. See comments below.
>
> On 08/11/2015 05:56 AM, Stephen Leake wrote:
>> branch: master
>> commit d7df36e745a5ba480559b6c8b5ebc93a18fe9bd1
>> Author: Stephen Leake <stephen_leake@stephe-leake.org>
>> Commit: Stephen Leake <stephen_leake@stephe-leake.org>
>>
>>      Always output all available definitions.
>
> How come you saw fit to undo the tweaks that I've added over time?

Because they got in the way of some of my uses of xref-find-definitions.

In general, rather than using similar heuristics in the low level code,
we should be adding hints from the source being searched, and/or the
user.

For example, one heuristic returned only the function when there are
both function and feature with the same name. But there are times when I
want to see both, or one or the other. So if I'm searching for the
identifier at point, in code like this:

        (dvc-log-edit ...) ;; point on '-l'; show defun
        (require 'dvc-log-edit) ;; point on '-l'; show feature

In both cases, we can easily tell from the source near point what the
user is searching for.

> In particular, now jumping to whitespace-mode will show two entries,
> even though they both lead to one location. That's a waste of time.

Yes. But I thought the heuristic the previous code used was: "if there
is both a variable and a function by the same name, _assume_ they are
located in the same place, so only return the function". That assumption
is broken for some of my code, and I assume in many others as well.

However, you point out later that you used (memq sym minor-mode-list) to
determine if this is from define-minor-mode.

I didn't realize why that was there when reading the code.

So I'll put that back, with a comment this time :).

>> -(defvar elisp--xref-format
>> +(defconst elisp--xref-format
>>     (let ((str "(%s %s)"))
>>       (put-text-property 1 3 'face 'font-lock-keyword-face str)
>>       (put-text-property 4 6 'face 'font-lock-function-name-face str)
>>       str))
>
> I'm not sure which part of the change did that, but now I don't see
> the colors in the output.
>
> Even though the approach is questionable, the result should be worth
> keeping.

I noticed that as well; I tracked it down. See bug#21237; if you use
defconst here, the text properties disappear when emacs is dumped.

So I've changed it back to defvar in my local code; it will be pushed
soon. 

>> +(defcustom find-feature-regexp
>
> I think these should still go into find-func.el. Even if you can't
> require it at the top of elisp-mode.el, you can do that at the
> beginning of elisp--xref-find-definitions. If that's actually needed.

The other one is find-alias-regexp.

Yes; then other code could use it as well. Should the default value of
find-function-regexp-alist contain them? I don't see any harm in that.

>> +	   ((setq generic (cl--generic symbol))
>> +	    (dolist (method (cl--generic-method-table generic))
>> +	      (let* ((info (cl--generic-method-info method))
>> +		     (met-name (cons symbol (cl--generic-method-specializers method)))
>> + (descr (format elisp--xref-format-cl-defmethod 'cl-defmethod
>> symbol (nth 1 info)))
>> +		     (file (find-lisp-object-file-name met-name 'cl-defmethod)))
>
> This should also account, if possible, for the default method
> definitions performed by cl-defgeneric (and hide them).

Yes, I just ran across that. I'll add a test case. I'm not sure how to
detect the default methods, but maybe there is a way.

> For instance, looking for project-search-path will display three
> entries, but the last two both lead to its generic definition. 

The three are:

  (cl-defgeneric project-search-path)
  (cl-defmethod project-search-path (project))
  (cl-defmethod project-search-path ((project (head vc))))

the first shows the cl-defgeneric location.

the second os the default method (no specializer arg), but it shows the
cl-defmethod for (head vc); I'm not clear why.

the third shows the cl-defmethod for (head vc), as it should.

>> +;; When tests are run from the Makefile, 'default-directory' is $HOME,
>> +;; so we must provide this dir to expand-file-name in the expected
>> +;; results. The Makefile sets EMACS_TEST_DIRECTORY.
>> +(defconst emacs-test-dir (getenv "EMACS_TEST_DIRECTORY"))
>
> You could also use (or load-file-name (buffer-file-name)). That has
> the benefit of being usable when tests are run inside the interactive
> session. Not via Makefile.

Ah, that's a good idea. I have a "(setenv "EMACS_TEST_DIRECTORY")" in my
notes file for the iteractive case.

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Dmitry Gutov]

On 08/12/2015 02:30 AM, Stephen Leake wrote:

>> How come you saw fit to undo the tweaks that I've added over time?
>
> Because they got in the way of some of my uses of xref-find-definitions.

At least a question would've been nice.

> In general, rather than using similar heuristics in the low level code,
> we should be adding hints from the source being searched, and/or the
> user.

In general, elisp-xref doesn't open the source files. When used with 
xref-find-apropos, that was rather slow.

> For example, one heuristic returned only the function when there are
> both function and feature with the same name. But there are times when I
> want to see both, or one or the other.

If the feature and the function have the same name, idiomatically, they 
will always be in the same file. Why wouldn't you want to jump to the 
function? If it's not right, M-< is not too far away (or M->, as per below).

I see you're also jumping to the (provide 'xxx) forms. What's the use in 
that?

> So if I'm searching for the
> identifier at point, in code like this:
>
>          (dvc-log-edit ...) ;; point on '-l'; show defun
>          (require 'dvc-log-edit) ;; point on '-l'; show feature
>
> In both cases, we can easily tell from the source near point what the
> user is searching for.

Right, it's a good default behavior. But then we'll have to decide how 
the user could indicate whether elisp-xref should *not* look at the 
context. Currently, C-u only forces the prompt to appear, but the 
default value will still use the text properties from the context (if 
xref-default-identifier-at-point includes them).

> Yes. But I thought the heuristic the previous code used was: "if there
> is both a variable and a function by the same name, _assume_ they are
> located in the same place, so only return the function". That assumption
> is broken for some of my code, and I assume in many others as well.
>
> However, you point out later that you used (memq sym minor-mode-list) to
> determine if this is from define-minor-mode.
>
> I didn't realize why that was there when reading the code.
>
> So I'll put that back, with a comment this time :).

There was a comment: "Don't show minor modes twice". I thought it 
explained the purpose of (not (and (fboundp sym) (memq sym 
minor-mode-list))) clearly enough.

And even if the code is not commented, one can try to preserve the part 
that aren't obviously in need of change.

> The other one is find-alias-regexp.
>
> Yes; then other code could use it as well. Should the default value of
> find-function-regexp-alist contain them? I don't see any harm in that.

I guess so.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08/12/2015 02:30 AM, Stephen Leake wrote:
>
>>> How come you saw fit to undo the tweaks that I've added over time?
>>
>> Because they got in the way of some of my uses of xref-find-definitions.
>
> At least a question would've been nice.
>
>> In general, rather than using similar heuristics in the low level code,
>> we should be adding hints from the source being searched, and/or the
>> user.
>
> In general, elisp-xref doesn't open the source files. When used with
> xref-find-apropos, that was rather slow.
>
>> For example, one heuristic returned only the function when there are
>> both function and feature with the same name. But there are times when I
>> want to see both, or one or the other.
>
> If the feature and the function have the same name, idiomatically,
> they will always be in the same file. 

I was going to say "not guarranteed", but that is a style we strongly
encourage, so I think it is reasonable to rely on it.

> Why wouldn't you want to jump to
> the function? 

> I see you're also jumping to the (provide 'xxx) forms. What's the use
> in that?

I think that's because I was trying to find the closest equivalent to
what Ada mode does. In Ada, every file has clear syntax indicating "the
start of the file code", and M-. on "with foo;" jumps to that point in
foo.ads.

(provide 'foo) is the closest thing in elisp syntax. We could jump
to the ;;; Code: comment.

For a better reason, sometimes it does matter what happens in the file
after (provide 'foo), so that's sometimes a useful place to go.

> If it's not right, M-< is not too far away (or M->, as per below).

and M-. to jump to the function is also not too far away.

But even better is the source code info discussed below; if that works,
this point is moot.

>> So if I'm searching for the
>> identifier at point, in code like this:
>>
>>          (dvc-log-edit ...) ;; point on '-l'; show defun
>>          (require 'dvc-log-edit) ;; point on '-l'; show feature
>>
>> In both cases, we can easily tell from the source near point what the
>> user is searching for.
>
> Right, it's a good default behavior. But then we'll have to decide how
> the user could indicate whether elisp-xref should *not* look at the
> context. Currently, C-u only forces the prompt to appear, but the
> default value will still use the text properties from the context (if
> xref-default-identifier-at-point includes them).

C-u should strip the text properties.

>> Yes. But I thought the heuristic the previous code used was: "if there
>> is both a variable and a function by the same name, _assume_ they are
>> located in the same place, so only return the function". That assumption
>> is broken for some of my code, and I assume in many others as well.
>>
>> However, you point out later that you used (memq sym minor-mode-list) to
>> determine if this is from define-minor-mode.
>>
>> I didn't realize why that was there when reading the code.
>>
>> So I'll put that back, with a comment this time :).
>
> There was a comment: "Don't show minor modes twice". I thought it
> explained the purpose of (not (and (fboundp sym) (memq sym
> minor-mode-list))) clearly enough.

Not for me, obviously.

I will ask first about code I don't understand in the future.

Did you find my comments clear enough?

-- 
-- Stephe

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Dmitry Gutov]

On 08/12/2015 10:40 AM, Stephen Leake wrote:

> (provide 'foo) is the closest thing in elisp syntax. We could jump
> to the ;;; Code: comment.

The first line looks like ;;; elisp-mode.el ---

One could also say it's the package declaration.

> For a better reason, sometimes it does matter what happens in the file
> after (provide 'foo), so that's sometimes a useful place to go.

I wonder how often that is. In most other cases, I think the user will 
be stumped to see Emacs navigate there.

>> If it's not right, M-< is not too far away (or M->, as per below).
>
> and M-. to jump to the function is also not too far away.

We'd have pressed it already by then, didn't we?

> C-u should strip the text properties.

I guess so.

> I will ask first about code I don't understand in the future.

Thank you.

> Did you find my comments clear enough?

I think so. Did you forget to do a push? The recent change only handles 
cl-struct constructor, but not defconst/defvar or the minor mode 
duplication.

Re: [Emacs-diffs] master d7df36e: Rewrite elisp--xref-find-definitions to handle many more cases; add tests.

[Stefan Monnier]

>> For a better reason, sometimes it does matter what happens in the file
>> after (provide 'foo), so that's sometimes a useful place to go.
> I wonder how often that is. In most other cases, I think the user will be
> stumped to see Emacs navigate there.

Yes, I'd be really perplexed.  While technically, it is the `provide' which
provides the feature, (require 'foo) most importantly requires *the
file*.


        Stefan