bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

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

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

When `etags-xref-prefer-current-file' is t and only the current file
contains (multiple) definitions of the same function, the list returns
each tag twice.  To reproduce:

cd src/
../lib-src/etags term.c
./emacs -Q term.c
C-s tty_free_frame_resources RTN
M-. RTN

src/term.c
3786: tty_free_frame_resources
3798: tty_free_frame_resources

(setq etags-xref-prefer-current-file t)
M-.

src/term.c
3786: tty_free_frame_resources
3798: tty_free_frame_resources
3786: tty_free_frame_resources
3798: tty_free_frame_resources

The variable is new in 28.1, so I thought the bug might want fixing on
this branch.  In my testing, cl-delete appears not to delete ITEMs from
SEQ if such deletion would leave SEQ empty.  I can't find this behavior
documented anywhere, but I may be looking in the wrong places.

Patch attached, which just tests for equality of the two lists of
definitions, and only returns one of them in that case.

Thanks,

David.

In GNU Emacs 28.0.90 (build 3, i686-pc-linux-gnu, GTK+ Version 3.18.9,
cairo version 1.14.6)
 of 2021-12-22 built on newfont
Repository revision: ce7ec828f15446c1218614d083da489c76e9b37d
Repository branch: cl-del-again
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description: Slackware 14.2

Configured using:
 'configure
 PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/local/share/pkgconfig:/usr/lib/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND
THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LC_COLLATE: C
  value of $LANG: en_US.ISO8859-1
  locale-coding-system: iso-latin-1-unix

Major mode: ELisp/l

Minor modes in effect:
  bug-reference-prog-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-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

Load-path shadows:
/home/dfussner/.emacs.d/elpa/transient-20210426.2141/transient hides
/home/dfussner/src/emacs/emacs/lisp/transient

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date help-fns
radix-tree help-mode misearch multi-isearch vc-git diff-mode easy-mmode
vc-dispatcher bug-reference doom-opera-theme doom-themes
doom-themes-base edmacro kmacro server preview-latex auto-loads tex-site
finder-inf info 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 seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib 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 lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 8 275188 34213)
 (symbols 24 15726 0)
 (strings 16 79597 3815)
 (string-bytes 1 2052213)
 (vectors 8 24263)
 (vector-slots 4 365962 27260)
 (floats 8 208 168)
 (intervals 28 819 0)
 (buffers 564 14)
 (heap 1024 25437 912))

[-- Attachment #2: 0001-Fix-xref-backend-definitions-in-etags.el.patch --]
[-- Type: text/x-patch, Size: 1201 bytes --]

From ce7ec828f15446c1218614d083da489c76e9b37d Mon Sep 17 00:00:00 2001
From: David Fussner <dfussner@googlemail.com>
Date: Tue, 21 Dec 2021 17:38:52 +0000
Subject: [PATCH] Fix xref-backend-definitions in etags.el

* lisp/progmodes/etags.el (xref-backend-definitions): When
'etags-xref-prefer-current-file' is t, test whether 'definitions' and
'same-file-definitions' are equal before concatenating them.
---
 lisp/progmodes/etags.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index f53b09d9e8..0e70e7ee7a 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2092,8 +2092,9 @@ xref-backend-definitions
            (push definition same-file-definitions)
            t))
        definitions)
-      (setq definitions (nconc (nreverse same-file-definitions)
-                               definitions)))
+      (setq same-file-definitions (nreverse same-file-definitions))
+      (unless (equal definitions same-file-definitions)
+        (setq definitions (nconc same-file-definitions definitions))))
     definitions))
 
 (cl-defmethod xref-backend-apropos ((_backend (eql 'etags)) pattern)
-- 
2.17.6

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Eli Zaretskii]

> Date: Wed, 22 Dec 2021 15:01:54 +0000
> From:  David Fussner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> When `etags-xref-prefer-current-file' is t and only the current file
> contains (multiple) definitions of the same function, the list returns
> each tag twice.  To reproduce:
> 
> cd src/
> ../lib-src/etags term.c
> ./emacs -Q term.c
> C-s tty_free_frame_resources RTN
> M-. RTN
> 
> src/term.c
> 3786: tty_free_frame_resources
> 3798: tty_free_frame_resources
> 
> (setq etags-xref-prefer-current-file t)
> M-.
> 
> src/term.c
> 3786: tty_free_frame_resources
> 3798: tty_free_frame_resources
> 3786: tty_free_frame_resources
> 3798: tty_free_frame_resources
> 
> The variable is new in 28.1, so I thought the bug might want fixing on
> this branch.  In my testing, cl-delete appears not to delete ITEMs from
> SEQ if such deletion would leave SEQ empty.  I can't find this behavior
> documented anywhere, but I may be looking in the wrong places.
> 
> Patch attached, which just tests for equality of the two lists of
> definitions, and only returns one of them in that case.

Dmitry, any comments?  The patch LGTM, FWIW.

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Dmitry Gutov]

Hi!

On 22.12.2021 18:01, David Fussner via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> The variable is new in 28.1, so I thought the bug might want fixing on
> this branch.  In my testing, cl-delete appears not to delete ITEMs from
> SEQ if such deletion would leave SEQ empty.  I can't find this behavior
> documented anywhere, but I may be looking in the wrong places.

The thing about cl-delete-if is it (like other sequence functions) has 
the changed sequence in its return value (which the piece of code under 
discussion leaves unused -- a brain fart on my part).

As a destructive function, it also modifies the original structure, but 
when the variable is still pointing at the first link of the linked 
list, the behavior is basically unpredictable. Even though it might look 
right most of the time under usual conditions.

See a different patch in the other email.

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Dmitry Gutov]

Hi Eli,

On 23.12.2021 12:12, Eli Zaretskii wrote:
> Dmitry, any comments?  The patch LGTM, FWIW.

How about the patch below instead?

And I'd like to apply it to emacs-28.

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index f53b09d9e8..a63c3f3397 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2084,14 +2084,15 @@ xref-backend-definitions
          (definitions (etags--xref-find-definitions symbol))
          same-file-definitions)
      (when (and etags-xref-prefer-current-file file)
-      (cl-delete-if
-       (lambda (definition)
-         (when (equal file
-                      (xref-location-group
-                       (xref-item-location definition)))
-           (push definition same-file-definitions)
-           t))
-       definitions)
+      (setq definitions
+            (cl-delete-if
+             (lambda (definition)
+               (when (equal file
+                            (xref-location-group
+                             (xref-item-location definition)))
+                 (push definition same-file-definitions)
+                 t))
+             definitions))
        (setq definitions (nconc (nreverse same-file-definitions)
                                 definitions)))
      definitions))

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Eli Zaretskii]

> Cc: 52734@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 Dec 2021 02:27:45 +0200
> 
> On 23.12.2021 12:12, Eli Zaretskii wrote:
> > Dmitry, any comments?  The patch LGTM, FWIW.
> 
> How about the patch below instead?
> 
> And I'd like to apply it to emacs-28.

Fine with me, on both counts.

Thanks.

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

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

Thanks, Dmitry, both for the explanation and for the (much superior)
patch. I've tried it on the emacs-28 branch and it works well, so
please close the bug whenever it's convenient.

All best,

David.

On Fri, 24 Dec 2021 at 07:08, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: 52734@debbugs.gnu.org
> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Fri, 24 Dec 2021 02:27:45 +0200
> >
> > On 23.12.2021 12:12, Eli Zaretskii wrote:
> > > Dmitry, any comments?  The patch LGTM, FWIW.
> >
> > How about the patch below instead?
> >
> > And I'd like to apply it to emacs-28.
>
> Fine with me, on both counts.
>
> Thanks.

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Eli Zaretskii]

> From: David Fussner <dfussner@googlemail.com>
> Date: Fri, 24 Dec 2021 13:09:14 +0000
> Cc: Dmitry Gutov <dgutov@yandex.ru>, 52734@debbugs.gnu.org
> 
> Thanks, Dmitry, both for the explanation and for the (much superior)
> patch. I've tried it on the emacs-28 branch and it works well, so
> please close the bug whenever it's convenient.

Thanks for testing the patch, I'm therefore closing this bug.

bug#52734: 28.0.90; [PATCH] xref-backend-definitions in etags.el duplicates list

[Dmitry Gutov]

On 24.12.2021 16:20, Eli Zaretskii wrote:
>> From: David Fussner<dfussner@googlemail.com>
>> Date: Fri, 24 Dec 2021 13:09:14 +0000
>> Cc: Dmitry Gutov<dgutov@yandex.ru>,52734@debbugs.gnu.org
>>
>> Thanks, Dmitry, both for the explanation and for the (much superior)
>> patch. I've tried it on the emacs-28 branch and it works well, so
>> please close the bug whenever it's convenient.
> Thanks for testing the patch, I'm therefore closing this bug.

I guess I should install the patch then!

(Revision c09ad0cabd)