bug#74718: 29.4; Huge metadata with flex completion style

bug#74718: 29.4; Huge metadata with flex completion style

[Thierry Volpiatto]

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


The flex completion style uses a function
(completion--flex-adjust-metadata) to create its display-sort-function.
completion-all-completions calls completion--nth-completion which then
uses setcdr on metadata to setup sort-function with this function.  At
each call of completion-all-completions the metadata is growing quickly
and become huge (see attached file).  In addition to the
display-sort-function becoming huge, the objects are duplicated at each
turn because there is no measure to prevent duplicates.

Here the offending code in minibuffer.el (in
completion--nth-completion):

--8<---------------cut here---------------start------------->8---
   (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
    (when (and adjust-fn metadata)
      (setcdr metadata (cdr (funcall adjust-fn metadata))))
--8<---------------cut here---------------end--------------->8---

the bug is reproductible easily with helm which allows examining the
object while the session is running, here how to reproduce:

For example we will use `switch-to-buffer' (C-x b) with helm-mode enabled:

1) Once helm is installed enable helm-mode:

--8<---------------cut here---------------start------------->8---
    (helm-mode 1)
--8<---------------cut here---------------end--------------->8---


2) Then configure switch-to-buffer so that it uses flex style:

--8<---------------cut here---------------start------------->8---
    (add-to-list 'helm-completion-styles-alist '(switch-to-buffer . (emacs helm flex)))
--8<---------------cut here---------------end--------------->8---


3) Open some buffers, for example all helm files
4) C-x b to call switch-to-buffer and type a flex pattern like "hlmfi" to narrow down to "helm-fi..."
5) Insist typing "hlm" "hlmfi" "hlm" etc... (not too much because the metadata is growing incredibily quickly)
6) Now consult the log with C-h d (see the second section, the helm source)



In GNU Emacs 29.4 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.33,
 cairo version 1.16.0) of 2024-08-24 built on IPad-S340
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Linux Mint 21.3

Configured using:
 'configure CFLAGS=-O8 --bindir=/usr/local/sbin/emacs-29.4 --with-cairo
 --with-modules --without-tree-sitter --without-native-compilation'

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

Important settings:
  value of $LANG: fr_FR.UTF-8
  locale-coding-system: utf-8-unix

Major mode: 

Minor modes in effect:
  emms-mode-line-mode: t
  emms-playing-time-display-mode: t
  emms-playing-time-mode: t
  server-mode: t
  psession-mode: t
  psession-savehist-mode: t
  register-preview-mode: t
  global-git-gutter-mode: t
  display-time-mode: t
  winner-mode: t
  tv-save-place-mode: t
  helm-epa-mode: t
  helm-descbinds-mode: t
  helm-top-poll-mode: t
  helm-adaptive-mode: t
  helm-mode: t
  helm-minibuffer-history-mode: t
  helm-ff-icon-mode: t
  shell-dirtrack-mode: t
  helm-popup-tip-mode: t
  dired-async-mode: t
  minibuffer-depth-indicate-mode: t
  gcmh-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow epa-mail face-remap emacsbug image-file image-converter
helm-dabbrev smiley gnus-cite qp mm-archive mail-extr textsec
uni-scripts idna-mapping ucs-normalize uni-confusable textsec-check
addressbook-bookmark tv-mu4e-config advice gnus-and-mu4e mu4e-patch
mu4e-contrib eshell esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg
esh-module esh-groups esh-util mu4e mu4e-org mu4e-notification
notifications mu4e-main smtpmail mu4e-view mu4e-mime-parts mu4e-headers
mu4e-thread mu4e-actions mu4e-compose mu4e-draft gnus-msg mu4e-search
mu4e-lists mu4e-bookmarks mu4e-mark mu4e-message flow-fill hl-line
mu4e-contacts mu4e-update mu4e-folders mu4e-context mu4e-query-items
mu4e-server mu4e-modeline mu4e-vars mu4e-helpers mu4e-config mu4e-window
ido mu4e-obsolete char-fold ffap helm-ring cl-print helm-command
helm-elisp helm-eval edebug debug backtrace helm-x-files helm-for-files
helm-bookmark helm-info bookmark emms-config emms-idapi-browser
emms-idapi emms-idapi-musicbrainz emms-mpris emms-librefm-stream
emms-librefm-scrobbler emms-playlist-limit emms-i18n emms-history
emms-score emms-stream-info emms-metaplaylist-mode emms-bookmarks
emms-cue emms-mode-line-icon emms-browser sort emms-volume
emms-volume-sndioctl emms-volume-mixerctl emms-volume-pulse
emms-volume-amixer emms-playlist-sort emms-last-played emms-player-xine
emms-player-mpd tq emms-lyrics emms-url emms-streams emms-show-all
emms-tag-editor emms-tag-tracktag emms-mark emms-mode-line emms-cache
emms-info-native emms-info-native-spc emms-info-native-mp3
emms-info-native-ogg emms-info-native-opus emms-info-native-flac
emms-info-native-vorbis bindat emms-info-exiftool emms-info-tinytag
emms-info-metaflac emms-info-opusinfo emms-info-ogginfo
emms-info-mp3info emms-playlist-mode emms-player-vlc emms-player-mpv
emms-playing-time emms-info emms-later-do emms-player-mplayer
emms-player-simple emms-source-playlist emms-source-file locate
emms-setup emms emms-compat emms-auto helm-external helm-net isl
cl-indent tramp-archive tramp-gvfs tramp-cache time-stamp zeroconf
helm-ls-git vc-git diff-mode vc vc-dispatcher emacs-news-mode
flymake-shellcheck flymake-proc flymake project warnings sh-script smie
treesit executable org-indent org-element org-persist org-id org-refile
avl-tree generator oc-basic ol-eww eww url-queue thingatpt mm-url
ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu mml2015
mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku
url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus dbus xml
gnus-cloud nnimap nnmail mail-source utf7 nnoo gnus-spec gnus-int
gnus-range message sendmail yank-media puny rfc822 mml mml-sec mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums
mailabbrev gmm-utils mailheader gnus-win gnus nnheader gnus-util
mail-utils range mm-util mail-prsvr ol-docview doc-view jka-compr
ol-bibtex bibtex ol-bbdb ol-w3m ol-doi org-link-doi org-config
ob-gnuplot org-crypt org-protocol org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete org-list
org-footnote org-faces org-entities noutline outline ob-emacs-lisp
ob-core ob-eval org-cycle org-table ol org-fold org-fold-core org-keys
oc org-loaddefs find-func org-version org-compat org-macs bug-reference
cus-start naquadah-theme solar cal-dst holidays holiday-loaddefs appt
diary-lib diary-loaddefs cal-menu calendar cal-loaddefs server bm
cl-extra imenu tv-utils psession frameset register-preview pcase
git-gutter mule-util dired-extension time winner describe-variable
help-fns radix-tree help-mode tv-save-place.el init-helm epa derived epg
rfc6068 epg-config helm-epa helm-descbinds cus-edit pp icons wid-edit
helm-sys helm-adaptive helm-mode helm-misc helm-files image-dired
image-dired-tags image-dired-external image-dired-util xdg image-mode
exif filenotify tramp tramp-loaddefs trampver tramp-integration files-x
tramp-compat rx shell pcomplete parse-time iso8601 time-date
helm-buffers all-the-icons all-the-icons-faces data-material
data-weathericons data-octicons data-fileicons data-faicons
data-alltheicons helm-occur helm-tags helm-locate helm-grep wgrep-helm
wgrep grep compile text-property-search comint ansi-osc ring helm-regexp
format-spec ansi-color helm-utils helm-help helm-types
helm-extensions-autoloads helm-autoloads helm helm-global-bindings
helm-easymenu edmacro kmacro helm-core helm-source helm-multi-match
helm-lib dired-async async dired-aux dired dired-loaddefs isl-autoloads
mb-depth avoid cus-load gcmh easy-mmode all-the-icons-autoloads
bash-completion-autoloads info ledger-mode-autoloads
markdown-mode-autoloads w3m-load w3m-autoloads package browse-url url
url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv 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 nadvice seq simple cl-generic indonesian philippine
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 abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 1364610 516926)
 (symbols 48 37867 5)
 (strings 32 278923 31015)
 (string-bytes 1 7703229)
 (vectors 16 107254)
 (vector-slots 8 2306363 262067)
 (floats 8 2805 2066)
 (intervals 56 118280 8725)
 (buffers 976 139))
<#secure method=pgpmime mode=sign>

-- 
Thierry

[-- Attachment #2: helm.log --]
[-- Type: application/octet-stream, Size: 20373 bytes --]

bug#74718: 29.4; Huge metadata with flex completion style

[Dmitry Gutov]

Hi Thierry,

On 07/12/2024 10:06, Thierry Volpiatto wrote:
> 
> The flex completion style uses a function
> (completion--flex-adjust-metadata) to create its display-sort-function.
> completion-all-completions calls completion--nth-completion which then
> uses setcdr on metadata to setup sort-function with this function.  At
> each call of completion-all-completions the metadata is growing quickly
> and become huge (see attached file).  In addition to the
> display-sort-function becoming huge, the objects are duplicated at each
> turn because there is no measure to prevent duplicates.

Thanks for the report.

> Here the offending code in minibuffer.el (in
> completion--nth-completion):
> 
> --8<---------------cut here---------------start------------->8---
>     (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
>      (when (and adjust-fn metadata)
>        (setcdr metadata (cdr (funcall adjust-fn metadata))))
> --8<---------------cut here---------------end--------------->8---
> 
> the bug is reproductible easily with helm which allows examining the
> object while the session is running, here how to reproduce:
> 
> For example we will use `switch-to-buffer' (C-x b) with helm-mode enabled:
> 
> 1) Once helm is installed enable helm-mode:
> ...

Can you also reproduce it without Helm?

I've tried to do that with the default UI for a bit, and also with 
company-mode enabled (which also reuses this code). No luck triggering 
this so far.

To test this, I added a printing statement and re-evaluated the defun:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 405ee21cdb2..6ae3c7b5ee3 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1310,6 +1310,7 @@ completion--nth-completion
           (adjust-fn (get (cdr result-and-style) 
'completion--adjust-metadata)))
      (when (and adjust-fn metadata)
        (setcdr metadata (cdr (funcall adjust-fn metadata))))
+    (message "%S" metadata)
      (if requote
          (funcall requote (car result-and-style) n)
        (car result-and-style))))

bug#74718: 29.4; Huge metadata with flex completion style

[Thierry Volpiatto]

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


Hi Dmitry,

Dmitry Gutov <dmitry@gutov.dev> writes:

> Can you also reproduce it without Helm?

Here a small recipe you can play with:

From emacs -Q:

1) Open some buffers
2) eval this in scratch:

(setq completion-styles '(flex))
(let* ((collection (mapcar #'buffer-name (buffer-list)))
       (metadata (completion-metadata "" collection nil))
       (completion-function (lambda (str _pred _action)
                              (let* ((comps (completion-all-completions str collection nil (length str) metadata))
				     (sort-fn (completion-metadata-get metadata 'display-sort-function))
				     (last (last comps)))
				(when (cdr last)
				  (setcdr last nil))
				(message "%S" metadata)
				(if (and sort-fn (> (length str) 0)) (funcall sort-fn comps) comps)))))
  (completing-read "test: " completion-function))

Try to complete with some flex patterns e.g. "hlm" for "helm" and
consult *messages* buffer.

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

bug#74718: 29.4; Huge metadata with flex completion style

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

Thierry Volpiatto <thievol@posteo.net> writes:
> From emacs -Q:
>
> 1) Open some buffers
> 2) eval this in scratch:
>
> (setq completion-styles '(flex))
> (let* ((collection (mapcar #'buffer-name (buffer-list)))
>        (metadata (completion-metadata "" collection nil))
>        (completion-function (lambda (str _pred _action)
>                               (let* ((comps (completion-all-completions str collection nil (length str) metadata))
> 				     (sort-fn (completion-metadata-get metadata 'display-sort-function))
> 				     (last (last comps)))
> 				(when (cdr last)
> 				  (setcdr last nil))
> 				(message "%S" metadata)
> 				(if (and sort-fn (> (length str) 0)) (funcall sort-fn comps) comps)))))
>   (completing-read "test: " completion-function))

Hello Thierry,

`completion-all-completions' should not be called inside completion
tables. Instead they should use `all-completions' to perform filtering
instead. `completion-all-completions' is the "frontend" API, which uses
completion styles, which then call the completion table backend.

The prototypical programmable completion table has the following form,
where `complete-with-action' provides the default implementation for the
ACTION argument:

(let ((candidates '("list" "of" "candidates")))
  (lambda (str pred action)
    (complete-with-action action candidates str pred)))

Depending on your use case, you may want to implement some ACTIONs
yourself, e.g., `metadata':

(let ((candidates '("list" "of" "candidates")))
  (lambda (str pred action)
    (if (eq action 'metadata)
        `(metadata (category . my-candidate-category))
      (complete-with-action action candidates str pred))))

Furthermore candidates can be computed dynamically, see the completion
tables `completion-table-dynamic' or `completion-table-with-cache'.

Daniel

bug#74718: 29.4; Huge metadata with flex completion style

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

> Here the offending code in minibuffer.el (in
> completion--nth-completion):
>
> --8<---------------cut here---------------start------------->8---
>    (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
>     (when (and adjust-fn metadata)
>       (setcdr metadata (cdr (funcall adjust-fn metadata))))
> --8<---------------cut here---------------end--------------->8---

Thanks Thierry.
The above code is indeed a hack we should try get rid of.
I suspect it remained unnoticed until now because most UIs get a "fresh"
new metadata before calling `completion-try/all-completions`, but we
shouldn't rely on such a property.

I think I vaguely remember when this code was added that we discussed
whether it's OK to do that (for all I know, I may even have suggested
this hack), so replacing it with a more robust solution may be
not straightforward.

In the mean time, maybe a patch like the one below can temporarily paper
over the underlying problem?


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 405ee21cdb2..d258085c778 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1307,9 +1307,15 @@ completion--nth-completion
                                string table pred point)))
                    (and probe (cons probe style))))))
            (completion--styles md)))
-         (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
-    (when (and adjust-fn metadata)
-      (setcdr metadata (cdr (funcall adjust-fn metadata))))
+         (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata))
+         (adjusted (completion-metadata-get
+                    metadata 'completion--adjusted-metadata)))
+    (when (and adjust-fn metadata
+               ;; Avoid re-applying the same adjustment (bug#74718).
+               (not (memq (cdr result-and-style) adjusted)))
+      (setcdr metadata `((completion--adjusted-metadata
+                          ,(cdr result-and-style) . ,adjusted)
+                         . ,(cdr (funcall adjust-fn metadata)))))
     (if requote
         (funcall requote (car result-and-style) n)
       (car result-and-style))))

bug#74718: 29.4; Huge metadata with flex completion style

[Thierry Volpiatto]

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


Hello Stefan,

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

>> Here the offending code in minibuffer.el (in
>> completion--nth-completion):
>>
>> --8<---------------cut here---------------start------------->8---
>>    (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
>>     (when (and adjust-fn metadata)
>>       (setcdr metadata (cdr (funcall adjust-fn metadata))))
>> --8<---------------cut here---------------end--------------->8---
>
> Thanks Thierry.
> The above code is indeed a hack we should try get rid of.
> I suspect it remained unnoticed until now because most UIs get a "fresh"
> new metadata before calling `completion-try/all-completions`, but we
> shouldn't rely on such a property.
>
> I think I vaguely remember when this code was added that we discussed
> whether it's OK to do that (for all I know, I may even have suggested
> this hack), so replacing it with a more robust solution may be
> not straightforward.
>
> In the mean time, maybe a patch like the one below can temporarily paper
> over the underlying problem?

Yes your patch fixed it, it avoids the duplicates as well.

Thanks.

>
>         Stefan
>
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 405ee21cdb2..d258085c778 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -1307,9 +1307,15 @@ completion--nth-completion
>                                 string table pred point)))
>                     (and probe (cons probe style))))))
>             (completion--styles md)))
> -         (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata)))
> -    (when (and adjust-fn metadata)
> -      (setcdr metadata (cdr (funcall adjust-fn metadata))))
> +         (adjust-fn (get (cdr result-and-style) 'completion--adjust-metadata))
> +         (adjusted (completion-metadata-get
> +                    metadata 'completion--adjusted-metadata)))
> +    (when (and adjust-fn metadata
> +               ;; Avoid re-applying the same adjustment (bug#74718).
> +               (not (memq (cdr result-and-style) adjusted)))
> +      (setcdr metadata `((completion--adjusted-metadata
> +                          ,(cdr result-and-style) . ,adjusted)
> +                         . ,(cdr (funcall adjust-fn metadata)))))
>      (if requote
>          (funcall requote (car result-and-style) n)
>        (car result-and-style))))

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

bug#74718: 29.4; Huge metadata with flex completion style

[Thierry Volpiatto]

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


Hello Daniel,

Daniel Mendler <mail@daniel-mendler.de> writes:

> `completion-all-completions' should not be called inside completion
> tables.

Yes it "should not" but it "can be" as well.  I have still some code
doing this, though not used by default.

Thanks.

-- 
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

bug#74718: 29.4; Huge metadata with flex completion style

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

> Yes your patch fixed it, it avoids the duplicates as well.

Thanks, pushed,


        Stefan