bug#54343: 28.0.91; find-function goes to a wrong place for erc

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Lin Jian]

[-- Attachment #1: Type: text/plain, Size: 4363 bytes --]

steps to reproduce this bug:

step 1: run: emacs -Q
step 2: eval (find-function 'erc)
step 3: the point goes to the defgroup of erc instead of the cl-defun of
it, which I think is wrong

wasamasa at #emacs:libera.chat helps to verify that this bug happens on
the master of emacs on March 11, 2022.

erc version is 5.4




In GNU Emacs 28.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.31, cairo version 1.16.0)
Repository revision: emacs-28.0.91
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: NixOS 22.05 (Quokka)

Configured using:
 'configure
 --prefix=/nix/store/ap3k7xlj8jh9x4gb98b0vbnn33kgqr96-emacs-gcc-28.0.91
 --disable-build-details --with-modules --with-x-toolkit=gtk3 --with-xft
 --with-cairo --with-native-compilation'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $EMACSLOADPATH: /nix/store/l07j503hxm0q6inagnc911y2vsq1i8vj-emacs-packages-deps/share/emacs/site-lisp:/nix/store/sm1w0fpbybib0fwq669ch4agjdfb1hs5-emacs-packages-deps/share/emacs/site-lisp:
  value of $EMACSNATIVELOADPATH: /nix/store/l07j503hxm0q6inagnc911y2vsq1i8vj-emacs-packages-deps/share/emacs/native-lisp::
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
/nix/store/sm1w0fpbybib0fwq669ch4agjdfb1hs5-emacs-packages-deps/share/emacs/site-lisp/site-start hides /nix/store/l07j503hxm0q6inagnc911y2vsq1i8vj-emacs-packages-deps/share/emacs/site-lisp/site-start

Features:
(shadow sort mail-extr emacsbug sendmail mm-archive message dired
dired-loaddefs rfc822 mml mml-sec epa derived gnus-util rmail
rmail-loaddefs text-property-search time-date mailabbrev gmm-utils
mailheader mm-decode mm-bodies mm-encode mail-utils gnutls mule-util
network-stream url-http mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr url-gw nsm rmc puny url-cache url-auth epg rfc6068
epg-config finder-inf package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars misearch
multi-isearch cl-loaddefs cl-lib seq byte-opt gv bytecomp byte-compile
cconv jka-compr find-func iso-transl tooltip eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 212408 11803)
 (symbols 48 8755 0)
 (strings 32 40745 2283)
 (string-bytes 1 1192908)
 (vectors 16 20578)
 (vector-slots 8 386857 19914)
 (floats 8 28 344)
 (intervals 56 17541 0)
 (buffers 992 16))

[-- Attachment #2: Type: text/html, Size: 5994 bytes --]

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[J.P.]

Hi Jian,

Lin Jian <jlin.dev@outlook.com> writes:

> steps to reproduce this bug:
>
> step 1: run: emacs -Q
> step 2: eval (find-function 'erc)
> step 3: the point goes to the defgroup of erc instead of the cl-defun of
> it, which I think is wrong
>
> wasamasa at #emacs:libera.chat helps to verify that this bug happens on
> the master of emacs on March 11, 2022.
>
> erc version is 5.4

Thanks for reporting this. I guess I felt obliged to say something since
ERC is in the subject line. But it looks like this bug affects more than
just ERC, so I'm going to cowardly defer to the experts here, if that's
all right.

But just as a dumb experiment, adding something like

  (let ((find-function-regexp
         (rx bol
             (* (syntax -))
             "("
             (? "cl-") ; <~~~~~~~~~~~~~~~~ THIS
             "def"
             (| (| "ine-skeleton"
                   "ine-generic-mode"
                   "ine-derived-mode"
                   "ine"
                   (? "-global")
                   "-minor-mode"
                   "ine-compilation-mode"
                   "un-cvs-mode"
                   "foo"
                   (: (| (not (any "icfgv")) (: "g" (not ?r)))
                      (| (+ word) (syntax symbol))
                      (? ?*)))
                (: "easy-mmode-define-" (+ (in (?a . ?z) ?-)))
                "easy-menu-define"
                "menu-bar-make-toggle"
                "menu-bar-make-toggle-command")
             (+ (| (syntax -) "\n" (: ";" (* nonl) "\n")))
             (: (? (| ?' "(quote ")) "%s" (| (syntax -) eol (in "()"))))))
    (find-function 'erc))

seems to work with emacs -Q. (That rx form is bogus, BTW; it doesn't
retain the capture groups and probably has other bugs.)

Anyway, naively splicing in the "cl-" prefix part doesn't seem to break
test/lisp/emacs-lisp/find-func-tests.el, but I didn't look closely
enough to see if that even factors in to the existing coverage. I also
have no clue whether such an addition might put a noticeable strain on
folks using `find-function-search-for-symbol' heavily in lisp code. If
so, another option might be to include it in the fallback regexp[1]
instead.

Hopefully someone in the know will chime in.

Thanks again!

[1] https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/find-func.el?id=0470a4a9#n442

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Michael Heerdegen]

"J.P." <jp@neverwas.me> writes:

> Hopefully someone in the know will chime in.

But you are not that Jens Petersen that find-func.el talks about when
saying "Please send improvements and fixes to the maintainer."
(`find-function-regexp')?

Michael.

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[F. Jason Park]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> But you are not that Jens Petersen that find-func.el talks about when
> saying "Please send improvements and fixes to the maintainer."

Alas, no.

(A mere impostor, I'm afraid.)

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[J.P.]

"J.P." <jp@neverwas.me> writes:

> seems to work with emacs -Q. (That rx form is bogus, BTW; it doesn't
> retain the capture groups and probably has other bugs.)

You're probably way ahead of me here, but I thought it prudent to
reemphasize that evaluating that ugly demo expression is useless. If
trying to convince yourself that splicing in a "cl-" prefix may be
promising, please just modify the original `find-function-regexp' value
instead:

  ""^\\s-*(\\(\\(?:cl-\\)?def\\(ine-skeleton...."
                   ^

FWIW, this one may be slightly less atrocious:

  (let ((find-function-regexp
         (rx bol
             (* (syntax -))
             "("
             (group
              (| (: (? "cl-")
                    "def"
                    (group
                     (| "ine-skeleton"
                        "ine-generic-mode"
                        "ine-derived-mode"
                        (: "ine" (? "-global") "-minor-mode")
                        "ine-compilation-mode"
                        "un-cvs-mode"
                        "foo"
                        (: (| (not (in "icfgv")) (: "g" (not ?r)))
                           (+ (group (| word (syntax symbol))))
                           (? ?*)))))
                 (: "easy-mmode-define-" (+ (in (?a . ?z) ?-)))
                 "easy-menu-define"
                 "menu-bar-make-toggle"
                 "menu-bar-make-toggle-command"))
             (+ (| (syntax -) "\n" (: ";" (* nonl) "\n")))
             (? (group (| ?' "(quote "))) "%s"
             (group (| (syntax -) eol (in "()"))))))
    (find-function 'erc))

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Lars Ingebrigtsen]

Lin Jian <jlin.dev@outlook.com> writes:

> steps to reproduce this bug:
>
> step 1: run: emacs -Q
> step 2: eval (find-function 'erc)
> step 3: the point goes to the defgroup of erc instead of the cl-defun of
> it, which I think is wrong

I've now fixed this in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[J.P.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lin Jian <jlin.dev@outlook.com> writes:
>
>> steps to reproduce this bug:
>>
>> step 1: run: emacs -Q
>> step 2: eval (find-function 'erc)
>> step 3: the point goes to the defgroup of erc instead of the cl-defun of
>> it, which I think is wrong
>
> I've now fixed this in Emacs 29.

Fantastic. What you did is much less invasive.

So, just to summarize the new behavior re generics, if I do

  (find-function 'xref-location-marker)

it jumps to the first method definition in xref.el, whereas it used to
end up on a defgeneric higher up in the same file, if present. But if
there's no method in the file (as with `xref-backend-definitions', for
example), the behavior remains unchanged. Probably not newsworthy
enough, but just thought I'd state it for the record.

Thanks.

P.S. OK to install #53617 duplicate checks in erc--switch-to-buffer?

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

> So, just to summarize the new behavior re generics, if I do
>
>   (find-function 'xref-location-marker)
>
> it jumps to the first method definition in xref.el, whereas it used to
> end up on a defgeneric higher up in the same file, if present. But if
> there's no method in the file (as with `xref-backend-definitions', for
> example), the behavior remains unchanged. Probably not newsworthy
> enough, but just thought I'd state it for the record.

Hm...  it would be nice if it hit the defgeneric instead.  But you can
have a defmethod without a defgeneric...

> P.S. OK to install #53617 duplicate checks in erc--switch-to-buffer?

Skimming that bug report, it's not clear to me what the issue was there?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[J.P.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  it would be nice if it hit the defgeneric instead.  But you can
> have a defmethod without a defgeneric...

Right. To my surprise, it seems there are over 300 such instances of the
"without" variant in the Emacs tree.

So, currently, we have

| defgeneric | defmethod | Old | New | Example                  |
|------------+-----------+-----+-----+--------------------------|
| yes        | no        | g   | g   | xref-backend-definitions |
| no         | yes       | m   | m   | delete-instance          |
| yes        | yes       | g   | m   | xref-location-marker     |

Whereas you're saying we should want

| yes        | yes       | g   | g   | xref-location-marker     |

I think simply adding "generic" to this union

  (: "cl-def" (| "un" "method" "generic"))

may do the trick. IOW

  -cl-\\(?:defun\\|defmethod\\)\\|\
  +cl-\\(?:defun\\|defgeneric\\|defmethod\\)\\|\

appears to work in cursory testing.

>> P.S. OK to install #53617 duplicate checks in erc--switch-to-buffer?
>
> Skimming that bug report, it's not clear to me what the issue was there?

Just a benign optimization involving some culling of redundant code in
`erc--switch-to-buffer'. This email contains the latest version, with a
slightly revised commit message:

  https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-01/msg02081.html

Thanks.

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

>   -cl-\\(?:defun\\|defmethod\\)\\|\
>   +cl-\\(?:defun\\|defgeneric\\|defmethod\\)\\|\
>
> appears to work in cursory testing.

Thanks.  So I've now added that in Emacs 29, too.  (But that regexp
there is the single worst thing I've seen in Emacs, so perhaps longer
term it would be nice if it was rewritten.  And perhaps the logic should
be "first look for these things, and if we don't find them, then look
for these other things".  That is, a priority thing.)

> Just a benign optimization involving some culling of redundant code in
> `erc--switch-to-buffer'. This email contains the latest version, with a
> slightly revised commit message:
>
>   https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-01/msg02081.html

I think that makes sense?  But I haven't tested the patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[J.P.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So I've now added that in Emacs 29, too.

Looks like the "def" part is missing from "defgeneric":

  diff --git a/lisp/emacs-lisp/find-func.el b/lisp/emacs-lisp/find-func.el
  index 777334a7a7..208d68d1ab 100644
  --- a/lisp/emacs-lisp/find-func.el
  +++ b/lisp/emacs-lisp/find-func.el
  @@ -61,7 +61,7 @@ find-function-regexp
   [...]
  -cl-\\(?:defun\\|defmethod\\)\\|\
  +cl-\\(?:defun\\|defmethod\\|generic\\)\\|\
                               ~~~~~~~

I'd used both in back-to-back examples (both def- and plain generic),
which was clearly a typo landmine (sorry).

> so perhaps longer term it would be nice if it was rewritten. And
> perhaps the logic should be "first look for these things, and if we
> don't find them, then look for these other things". That is, a
> priority thing.)

I was thinking much the same, albeit in a less evolved manner. The
alternate function form for the CDRs of `find-function-regexp-alist'
members may prove useful to do the prioritizing. Perhaps it'd even be
worthwhile to incorporate more refined strategies, when available, such
as the finders provided by cl-generic.el[1].

>> Just a benign optimization involving some culling of redundant code in
>> `erc--switch-to-buffer'. This email contains the latest version, with a
>> slightly revised commit message:
>>
>>   https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-01/msg02081.html
>
> I think that makes sense?  But I haven't tested the patch.

I'm testing it indirectly (in yet another bug) but will add a unit test
for good measure and ping you in the other thread. Thanks.


[1] https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/cl-generic.el?id=dd91aac5#n995

bug#54343: 28.0.91; find-function goes to a wrong place for erc

[Lars Ingebrigtsen]

"J.P." <jp@neverwas.me> writes:

> Looks like the "def" part is missing from "defgeneric":

Darn.  I didn't test, because it seemed to be "obvious"...

Fixed now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no