bug#55491: All completion fragments get added to obarray

bug#55491: All completion fragments get added to obarray

[JD Smith]

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


In Emacs 27 or 28, each and every partial fragment of text introduced to the completion system gets put into obarray.  From emacs -Q:

(intern-soft "ohno") <C-M-x> -> nil
(ohno <M-TAB>                -> No match
(intern-soft "ohno") <C-M-x> -> ohno :(

This has the result that, e.g.:

(test-completion "ohno" obarray nil) <C-M-x> ; t!  Sigh

will always return t during completion, for any completed fragment.  For completion systems that complete against obarray (e.g. emacs-lisp), this is obviously undesirable.  Tracing intern doesn’t reveal any obvious places where completion fragments are being captured into the symbol array. 

In GNU Emacs 27.2 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G95))
 of 2021-03-27 built on builder10-14.porkrind.org
Windowing system distributor 'Apple', version 10.3.2113
System Description:  macOS 12.3.1

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Quit
Mark set
nil
No match [2 times]
ohno
next-line: End of buffer
next-line: End of buffer
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES
THREADS JSON PDUMPER GMP

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-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
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
apropos tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/ns-win ns-win ucs-normalize mule-util
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu 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 charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 46191 5995)
 (symbols 48 6035 1)
 (strings 32 15585 1651)
 (string-bytes 1 518510)
 (vectors 16 10357)
 (vector-slots 8 129155 12342)
 (floats 8 20 45)
 (intervals 56 196 0)
 (buffers 1000 12))


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

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

JD Smith <jdtsmith@gmail.com> writes:

>  (intern-soft "ohno") <C-M-x> -> nil
>  (ohno <M-TAB>                -> No match
>  (intern-soft "ohno") <C-M-x> -> ohno :(
>
> This has the result that, e.g.:
>
>  (test-completion "ohno" obarray nil) <C-M-x> ; t!  Sigh
>
> will always return t during completion, for any completed fragment.
> For completion systems that complete against obarray
> (e.g. emacs-lisp), this is obviously undesirable.

Completion in emacs-lisp-mode doesn't take unbound variables into
account, I think?  So putting stuff into the obarray shouldn't have much
(if any) noticeable effect.

Where do you see anything undesirable as a result of this?

(This behaviour is still present in Emacs 29.)

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

bug#55491: All completion fragments get added to obarray

[JD Smith]

Thanks for your thoughts.  The context is fairly specific — using cape-super-capf to compose two CAPF’s: elisp-completion-at-point and cape-dabbrev. This results in incorrect test-completion calls which interfere with candidate selection.   With your nudge, I discovered this is because cape is not handling the elisp--shorthand-aware-* predicates returned by the elisp completion system in all cases, so it can be fixed there. 

I guess whether this constitutes a bug depends on whether anyone would expect unbound completion fragments to pollute the obarray. 

> On May 17, 2022, at 4:30 PM, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> JD Smith <jdtsmith@gmail.com> writes:
> 
>> (intern-soft "ohno") <C-M-x> -> nil
>> (ohno <M-TAB>                -> No match
>> (intern-soft "ohno") <C-M-x> -> ohno :(
>> 
>> This has the result that, e.g.:
>> 
>> (test-completion "ohno" obarray nil) <C-M-x> ; t!  Sigh
>> 
>> will always return t during completion, for any completed fragment.
>> For completion systems that complete against obarray
>> (e.g. emacs-lisp), this is obviously undesirable.
> 
> Completion in emacs-lisp-mode doesn't take unbound variables into
> account, I think?  So putting stuff into the obarray shouldn't have much
> (if any) noticeable effect.
> 
> Where do you see anything undesirable as a result of this?
> 
> (This behaviour is still present in Emacs 29.)
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.no

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

JD Smith <jdtsmith@gmail.com> writes:

> Thanks for your thoughts.  The context is fairly specific — using
> cape-super-capf to compose two CAPF’s: elisp-completion-at-point and
> cape-dabbrev. This results in incorrect test-completion calls which
> interfere with candidate selection.  With your nudge, I discovered
> this is because cape is not handling the elisp--shorthand-aware-*
> predicates returned by the elisp completion system in all cases, so it
> can be fixed there.

Ah, I see.

> I guess whether this constitutes a bug depends on whether anyone would
> expect unbound completion fragments to pollute the obarray.

I think it's "always" been like this, so it may be unlikely to be
changed.  But I have to admit that I have no idea why completion is
interning stuff.  Does anybody know?  It does seem counter intuitive.

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

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think it's "always" been like this, so it may be unlikely to be
> changed.  But I have to admit that I have no idea why completion is
> interning stuff.  Does anybody know?  It does seem counter intuitive.

And as you pointed out -- `intern' itself doesn't seem to be actually
called by the completion machinery -- I tested under gdb just to make
sure.

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

bug#55491: All completion fragments get added to obarray

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Completion in emacs-lisp-mode doesn't take unbound variables into
  > account, I think?  So putting stuff into the obarray shouldn't have much
  > (if any) noticeable effect.

If it's a really large number of fragments, they could make `intern' slower.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

Poking at this some more, this behaviour comes from:

(defun elisp-completion-at-point ()
[...]
           (fun-sym (condition-case nil
                        (save-excursion
                          (up-list -1)
                          (forward-char 1)
                          (and (memq (char-syntax (char-after)) '(?w ?_))
                               (read (current-buffer))))
                      (error nil))))

That `read' there is interning `ohno' when hitting `C-M-i' after:

(ohno

And that's unnecessary -- we don't actually need the fun-sym; we're just
checking whether we're looking at `ignore-error'.  So I've now fixed
this in Emacs 29.

Note that completion in emacs-lisp-mode may still intern stuff, but not
as gratuitously as before. 

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

bug#55491: All completion fragments get added to obarray

[Dmitry Gutov]

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

On 20.05.2022 03:03, Lars Ingebrigtsen wrote:
> And that's unnecessary -- we don't actually need the fun-sym; we're just
> checking whether we're looking at `ignore-error'.  So I've now fixed
> this in Emacs 29.
> 
> Note that completion in emacs-lisp-mode may still intern stuff, but not
> as gratuitously as before.

Should we try to remove the last 'read' call on line 700? Like this.

[-- Attachment #2: elisp-c-a-p-no-read.diff --]
[-- Type: text/x-patch, Size: 827 bytes --]

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 77bf3f1ed1..0130c002da 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -697,7 +697,8 @@ elisp-completion-at-point
                                      (let ((c (char-after)))
                                        (if (eq c ?\() ?\(
                                          (if (memq (char-syntax c) '(?w ?_))
-                                             (read (current-buffer))))))
+                                             (and (looking-at "\\(?:\\sw\\|\\s_\\)+")
+                                                  (intern-soft (match-string 0)))))))
                             (error nil))))
                      (pcase parent
                        ;; FIXME: Rather than hardcode special cases here,

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Should we try to remove the last 'read' call on line 700? Like this.

Yes, makes sense to me.

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

bug#55491: All completion fragments get added to obarray

[Dmitry Gutov]

On 05.06.2022 17:15, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> Should we try to remove the last 'read' call on line 700? Like this.
> Yes, makes sense to me.

Looking at the possible problems, I guess the regexp doesn't account for 
possible escapings? Like a symbol 'let\ s\ go\ to\ the\ mall', which 
shouldn't be mistaken for 'let'. That would require a more complex regexp.

I wonder what other cases we're still missing.

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Looking at the possible problems, I guess the regexp doesn't account
> for possible escapings? Like a symbol 'let\ s\ go\ to\ the\ mall',
> which shouldn't be mistaken for 'let'. That would require a more
> complex regexp.

Yes, so that won't be the right thing.  It'd be nice if we had a version
of `read' that's more like "read the next symbol but return nil if it's
not interned already" for these cases.

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

bug#55491: All completion fragments get added to obarray

[Dmitry Gutov]

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

On 06.06.2022 15:44, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> Looking at the possible problems, I guess the regexp doesn't account
>> for possible escapings? Like a symbol 'let\ s\ go\ to\ the\ mall',
>> which shouldn't be mistaken for 'let'. That would require a more
>> complex regexp.
> Yes, so that won't be the right thing.  It'd be nice if we had a version
> of `read' that's more like "read the next symbol but return nil if it's
> not interned already" for these cases.

How about this, then?

Too bad 'forward-symbol' doesn't handle escapings, otherwise we could 
just do (intern-soft (thing-at-point 'symbol)).

[-- Attachment #2: elisp-c-a-p-no-read-v2.diff --]
[-- Type: text/x-patch, Size: 999 bytes --]

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 77bf3f1ed1..44f4c012ef 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -697,7 +697,11 @@ elisp-completion-at-point
                                      (let ((c (char-after)))
                                        (if (eq c ?\() ?\(
                                          (if (memq (char-syntax c) '(?w ?_))
-                                             (read (current-buffer))))))
+                                             (let ((pt (point)))
+                                               (ignore-errors
+                                                 (forward-sexp)
+                                                 (intern-soft
+                                                  (buffer-substring pt (point)))))))))
                             (error nil))))
                      (pcase parent
                        ;; FIXME: Rather than hardcode special cases here,

bug#55491: All completion fragments get added to obarray

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> How about this, then?

[...]

> -                                             (read (current-buffer))))))
> +                                             (let ((pt (point)))
> +                                               (ignore-errors
> +                                                 (forward-sexp)
> +                                                 (intern-soft
> +                                                  (buffer-substring pt (point)))))))))
>                              (error nil))))

Makes sense, I think.  (But the ignore-errors is probably not
necessary, since the entire form is already covered by one in form of
the condition-case...)

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

bug#55491: All completion fragments get added to obarray

[Dmitry Gutov]

On 07.06.2022 12:30, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> How about this, then?
> [...]
> 
>> -                                             (read (current-buffer))))))
>> +                                             (let ((pt (point)))
>> +                                               (ignore-errors
>> +                                                 (forward-sexp)
>> +                                                 (intern-soft
>> +                                                  (buffer-substring pt (point)))))))))
>>                               (error nil))))
> Makes sense, I think.  (But the ignore-errors is probably not
> necessary, since the entire form is already covered by one in form of
> the condition-case...)

Thanks.

I've pushed the updated change. Seems to work okay.