bug#68568: transient.el interns potentially enormous symbols as commands

bug#68568: transient.el interns potentially enormous symbols as commands

[João Távora]

Hello,

I'd never used a package that made use of "transient.el" before.
I have now, via gptel.el  Takes a bit of getting used to, but I like it
(and can see why people like it in Magit).

Now, immediately after using it, I start noticing that the completion
that Fido mode offers on M-x starts to get weirdly contaminated by these
symbols with enormous symbols names.  This makes my Emacs almost
unusable.  I'd think other completion packages would be similarly
affected unless they special case transient symols some how.

Peeping into the transient source code, we can see that it uses a lot
of 'eval' to define some commands just in time as a user navigates the
menus and submenus.

All these commands seem point to the same actual command, and do not
seem to be meant to be called with M-x at all.  They are generated at
lazily at runtime and only for the submenus being visited.

So they're just temporary artefacts of implementation.  Who know if this
is where the library gets its name.  It's curious, but not going to
argue much on this approach.

Anyway, transient uses 'intern' when I think it could just use
'make-symbol' to avoid polluting the obarray and this whole problem.  It
seems to keep functioning and solves my problem.  Here's the trivial
patch.  Would you look at it, Jonas?

diff --git a/lisp/transient.el b/lisp/transient.el
index f9060f5ba85..249c25262ea 100644
--- a/lisp/transient.el
+++ b/lisp/transient.el
@@ -1127,7 +1127,7 @@ transient--parse-suffix
        ((and (commandp car)
              (not (stringp car)))
         (let ((cmd pop)
-              (sym (intern
+              (sym (make-symbol
                     (format "transient:%s:%s"
                             prefix
                             (let ((desc (plist-get args :description)))
@@ -1156,7 +1156,7 @@ transient--parse-suffix
              (when-let ((shortarg (transient--derive-shortarg arg)))
                (setq args (plist-put args :shortarg shortarg)))
              (setq args (plist-put args :argument arg))))
-          (setq sym (intern (format "transient:%s:%s" prefix arg)))
+          (setq sym (make-symbol (format "transient:%s:%s" prefix arg)))
           (setq args (plist-put
                       args :command
                       `(prog1 ',sym



If some kind of persistent storage for these symbols IS needed I
recommend a separate obarray.  Also, why does the full and potentially
very long description in plain text have to be a part of the symbol
name?  This doesn't matter with the 'make-symbol' approach, but I still
find it curious.

João

bug#68568: transient.el interns potentially enormous symbols as commands

[Psionic K]

There's an option mentioned in the Transient change log:

https://github.com/magit/transient/blob/main/CHANGELOG#v042----2023-08-25

Enormous is not quite what I would call these symbols since they are
on the same order of magnitude as regular symbol lengths.

If you get the option set and are satisfied, you should consider where
you would have found this in the user manual and make the appropriate
edits.  The texi file is generated from an org file.

bug#68568: transient.el interns potentially enormous symbols as commands

[João Távora]

On Sun, Jan 21, 2024 at 4:27 AM Psionic K <psionik@positron.solutions> wrote:
>
> There's an option mentioned in the Transient change log:
>
> https://github.com/magit/transient/blob/main/CHANGELOG#v042----2023-08-25

Thanks, but user options to get out of common bugs isn't what's wanted.
Setting this "recommended option" has other effects I don't want to
incur in (which is why, for good reason, it isn't the default)

Either there is a good reason for those transient symbols to be interned
in the obarray or there isn't.

Right now, it seems there isn't.  transient.el works just fine with
make-symbol instead of intern.  Intern is for persisting a symbol
long-term and referencing back to it by name, which is what M-x
execute-extended-command and the Lisp reader do, to name two examples.

transient.el doesn't need that, it seems.  Or do you have reasons to
believe it does?

> Enormous is not quite what I would call these symbols since they are
> on the same order of magnitude as regular symbol lengths.

GPTel, which I am planning on using, uses long descriptions of infix
commands (pretty reasonably). I have a number of symbols nearing 200
characters, all transient's.  The mean length is 17.  That's above one
full decimal order of magnitude above.  So yes, "enormous" in my book.

João

bug#68568: transient.el interns potentially enormous symbols as commands

[Psionic K]

> long descriptions of infix commands

That's a user choice btw.  Anyone can make long symbols through a
variety of means that are out of control of any package.  They can
definitely take alternative routes to avoid creating long symbols in
transient.

200 and how many instances?

> transient.el works just fine with make-symbol instead of intern.

It sounds like you have a patch ready for the upstream.  Bon appetit:
https://github.com/magit/transient

bug#68568: transient.el interns potentially enormous symbols as commands

[João Távora]

On Sun, Jan 21, 2024 at 7:29 AM Psionic K <psionik@positron.solutions> wrote:
>
> > long descriptions of infix commands
>
> That's a user choice btw.  Anyone can make long symbols through a

Not sure what you mean by "user".  I'm a "user" of GPTel, but
I didn't make those long description.  GPTel uses those descriptions
and, I think, reasonably.

> variety of means that are out of control of any package.

Sure, but that package shouldn't gratuitously have that choice
(questionable as it may be -- I think it's not) wreak havoc everywhere
else in Emacs.  This is what happens with transient's interning
of such symbols

> definitely take alternative routes to avoid creating long symbols in
> transient.

I'd rather not bother transient-using packages with code complications.
GPTel's interface is fine as it is.

> 200 and how many instances?

About 8 or 9, all transient's.  Doesn't matter because the greediness of the
flex matching algorithm, which was designed for short symbols,
eventually picks up on a pattern which is highly likely to appear in
the extremely long tail of the enormous symbol

So my usual searches like M-x vc SPC always where I expect to see
"vc" commands turn up some silly transient command, useless and
unwanted in that context.

> > transient.el works just fine with make-symbol instead of intern.
>
> It sounds like you have a patch ready for the upstream.  Bon appetit:
> https://github.com/magit/transient

I take it you still can't think of a good reason why those symbols
should be internet.  OK.  Created a PR. I hope this gets fixed
quickly over here at the downstream.  This is not something easy
to monkey-patch around.

João

bug#68568: transient.el interns potentially enormous symbols as commands

[Psionic K]

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

> Not sure what you mean by "user"

GPTel is on the user side of transient.  Karthink or another contributor
wrote GPTel to use transient that way.  Perhaps they need to create new
suffixes dynamically, but even that can be done with `define-suffix' etc.
If someone calls `define-suffix' with a really long name, there's nothing
transient can or should do about it.

Actually, I recommend taking this to GPTel.  You can ping me on their
Github @psionic-k and I'll help Karthink or others consider an alternative
approach that has sensible symbol names.

> is highly likely to appear in the extremely long tail of the enormous
symbol

True, but the symbols in question contain "transient:" and ":--" strings
that should be easy to filter out.  Lots of completion packages can filter
those.

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

bug#68568: transient.el interns potentially enormous symbols as commands

[João Távora]

Sun, Jan 21, 2024 at 8:26 AM Psionic K <psionik@positron.solutions> wrote:

> GPTel is on the user side of transient.  Karthink or another contributor wrote GPTel to use transient that way.  Perhaps they need to create new suffixes dynamically, but even that can be done with `define-suffix' etc.  If someone calls `define-suffix' with a really long name, there's nothing transient can or should do about it.

It should probably not intern that stuff in the obarray.
UNLESS you know of a reason to.  Do you?

> True, but the symbols in question contain "transient:" and ":--" strings that should be easy to filter out.  Lots of completion packages can filter those.

Why should a completion package know about "transient"
or this :-- and be slowed down by this crud?

bug#68568: transient.el interns potentially enormous symbols as commands

[Eli Zaretskii]

Ping!

Jonas, any comments or suggestions?

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Jan 2024 11:50:19 +0000
> 
> Hello,
> 
> I'd never used a package that made use of "transient.el" before.
> I have now, via gptel.el  Takes a bit of getting used to, but I like it
> (and can see why people like it in Magit).
> 
> Now, immediately after using it, I start noticing that the completion
> that Fido mode offers on M-x starts to get weirdly contaminated by these
> symbols with enormous symbols names.  This makes my Emacs almost
> unusable.  I'd think other completion packages would be similarly
> affected unless they special case transient symols some how.
> 
> Peeping into the transient source code, we can see that it uses a lot
> of 'eval' to define some commands just in time as a user navigates the
> menus and submenus.
> 
> All these commands seem point to the same actual command, and do not
> seem to be meant to be called with M-x at all.  They are generated at
> lazily at runtime and only for the submenus being visited.
> 
> So they're just temporary artefacts of implementation.  Who know if this
> is where the library gets its name.  It's curious, but not going to
> argue much on this approach.
> 
> Anyway, transient uses 'intern' when I think it could just use
> 'make-symbol' to avoid polluting the obarray and this whole problem.  It
> seems to keep functioning and solves my problem.  Here's the trivial
> patch.  Would you look at it, Jonas?
> 
> diff --git a/lisp/transient.el b/lisp/transient.el
> index f9060f5ba85..249c25262ea 100644
> --- a/lisp/transient.el
> +++ b/lisp/transient.el
> @@ -1127,7 +1127,7 @@ transient--parse-suffix
>         ((and (commandp car)
>               (not (stringp car)))
>          (let ((cmd pop)
> -              (sym (intern
> +              (sym (make-symbol
>                      (format "transient:%s:%s"
>                              prefix
>                              (let ((desc (plist-get args :description)))
> @@ -1156,7 +1156,7 @@ transient--parse-suffix
>               (when-let ((shortarg (transient--derive-shortarg arg)))
>                 (setq args (plist-put args :shortarg shortarg)))
>               (setq args (plist-put args :argument arg))))
> -          (setq sym (intern (format "transient:%s:%s" prefix arg)))
> +          (setq sym (make-symbol (format "transient:%s:%s" prefix arg)))
>            (setq args (plist-put
>                        args :command
>                        `(prog1 ',sym
> 
> 
> 
> If some kind of persistent storage for these symbols IS needed I
> recommend a separate obarray.  Also, why does the full and potentially
> very long description in plain text have to be a part of the symbol
> name?  This doesn't matter with the 'make-symbol' approach, but I still
> find it curious.
> 
> João
> 
> 
> 
> 

bug#68568: transient.el interns potentially enormous symbols as commands

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

Eli Zaretskii <eliz@gnu.org> writes:

> Ping!
>
> Jonas, any comments or suggestions?

We discussed at https://github.com/magit/transient/pull/273.

Summary: It would probably be possible to avoid interning these symbols.
I did some experimentation to come to that provisional assessment.  But
Transient has been using interned from the beginning, and as a result,
many parts of Transient assume that interned symbols are being used.
Using symbols made this aspect of Transient much simpler.  This allowed
me to focus on gradually improve other aspects, which are more
complicated by necessity.  If I had not used symbols, this would have
slowed down development.  Switching to not using symbols for some
commands, would mean many things would have to be adjusted to handle
both cases.  This would undoubtedly lead to bugs and slow down
development.  I would rather work on the parts of Transient that IMO
need improvement.

IMO using commands that are fbound to interned symbols has one and
only one drawback: they are offered as completion candidates by
execute-extended-command.

I have addressed this by setting read-extended-command-predicate to a
function that hides "transient" commands that should be hidden and
nothing else. The option is only set if, and only if, it is not already
set to something other than nil.  If the user customizes the option and
pick one of the predicates offered by Emacs, then that predicate also
hides Transient's "anonymous" commands. [There was a bug, I just fixed
that.]

IMO the result of this approach is that the only drawback of interning
the symbols is gone; without me having to trade in that one drawback for
many avoidable complications, as I would have to if I followed the
request to not "needlessly" intern the "anonymous" symbols.

An additional benefit of using read-extended-command-predicate is that
it allows all of Transient's commands that should be hidden, to be
hidden.  Even if I did stop using interned symbols for the "anonymous"
commands, which are inline when a transient menu is defined, there would
still be other commands that are defined non-anonymously, but never the
less should be hidden.  (E.g., commands that toggle arguments and are
defined using transient-define-infix, because they are useful in more
than one menu, and are then referenced by name in the definitions of
these menus.)

Therefore, the addition to read-extended-command-predicate is necessary,
whether I avoid interning the "anonymous" commands or not.  And as a
consequence of that, there would be zero benefit to avoiding to intern
the "anonymous" commands; but many drawbacks.

I have therefore decided to stick to using interned symbols.

Even if you don't agree with me by now, there is one big drawback that
moving away from interned symbols would have, which I believe will win
you over.  Each command is associated with an object and the symbol is
used to link the two.  As a consequence all "anonymous" commands that
set an argument can be mere aliases to the same command, which only has
to live in memory once.  If the symbol can no longer be used to link the
command and object, then the anonymous commands would have to be defined
as oclosures.  These closures would be nearly identical, but each one of
them would permanently use up memory.

-- Jonas

bug#68568: transient.el interns potentially enormous symbols as commands

[Eli Zaretskii]

tags 68568 wontfix
close 68568
thanks

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 68568@debbugs.gnu.org
> Date: Sat, 27 Jan 2024 19:18:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ping!
> >
> > Jonas, any comments or suggestions?
> 
> We discussed at https://github.com/magit/transient/pull/273.
> [...]

Thanks, I'm therefore closing this bug as wontfix.