bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Daniel Koning]

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

Severity: minor
Tags: patch

The Eldoc message shows the documentation for the wrong function (or no
documentation at all) in this specific situation:

(a) You're typing an elisp expression into `read-from-minibuffer', and
(b) the function name contains punctuation, such as ! or ?, whose
character class is "punctuation" and not "symbol" in the standard syntax
table.

Function names as described in (b) are not only legal but quite common
in third-party code. There aren't many in the standard distribution, but
you'll notice a few here and there. Try this:

(require 'pcvs)
(call-interactively #'eval-expression)
-----
Eval: (cvs-mode!
-----

The mode line shows the documentation for `cvs-mode' (without the !),
which is a different function and has a different lambda list.

This is happening because the syntax table in the minibuffer never gets
changed to the elisp table, which acknowledges all the valid symbol
characters as part of the "symbol" class.

There are a couple different spots in the code to which you could
attribute this lapse. For one, the elisp-mode.el function
`elisp--current-symbol' isn't wrapped in a `with-syntax-table', unlike
other similar definitions in the same file. I think anyone invoking this
function could reasonably expect it to observe elisp syntax, so that's
what my tiny patch addresses. This fixes the Eldoc problem.

But here's another weird thing further down the call stack.
`read--expression' has a FIXME comment saying to turn on
`emacs-lisp-mode' in the minibuffer -- which would also set the
appropriate syntax table -- but it doesn't actually do it. I guess that
must not work for whatever reason (since it has to have taken longer to
write the comment than it would have taken to add the code). Should it
be changed now so that it does set the major mode? Is there a problem
with specialized major modes in the minibuffer? I hereby kick the can
over to whoever knows more.

Daniel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: elisp--current-symbol patch --]
[-- Type: text/x-patch, Size: 1335 bytes --]

From 1d4ed0e89b4ebc040609b7476128062685843c7f Mon Sep 17 00:00:00 2001
From: Daniel Koning <dk@danielkoning.com>
Date: Tue, 9 Jun 2020 19:24:11 -0500
Subject: [PATCH] lisp/progmodes/elisp-mode.el (elisp--current-symbol): Set
 syntax table.

Even if `elisp-current-symbol' is called from a buffer with a different
active syntax table (such as the minibuffer in `read-from-minibuffer'),
use `emacs-lisp-mode-syntax-table' to determine the symbol boundaries.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/elisp-mode.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index f39ecf9b7b..65c01b2ce1 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1615,10 +1615,11 @@ elisp--beginning-of-sexp
 
 ;; returns nil unless current word is an interned symbol.
 (defun elisp--current-symbol ()
-  (let ((c (char-after (point))))
-    (and c
-         (memq (char-syntax c) '(?w ?_))
-         (intern-soft (current-word)))))
+  (with-syntax-table emacs-lisp-mode-syntax-table
+    (let ((c (char-after (point))))
+      (and c
+           (memq (char-syntax c) '(?w ?_))
+           (intern-soft (current-word))))))
 
 (defun elisp-function-argstring (arglist)
   "Return ARGLIST as a string enclosed by ().
-- 
2.20.1

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Eli Zaretskii]

Stefan, any comments?

> From: Daniel Koning <dk@danielkoning.com>
> Date: Tue, 09 Jun 2020 21:56:14 -0500
> 
> The Eldoc message shows the documentation for the wrong function (or no
> documentation at all) in this specific situation:
> 
> (a) You're typing an elisp expression into `read-from-minibuffer', and
> (b) the function name contains punctuation, such as ! or ?, whose
> character class is "punctuation" and not "symbol" in the standard syntax
> table.
> 
> Function names as described in (b) are not only legal but quite common
> in third-party code. There aren't many in the standard distribution, but
> you'll notice a few here and there. Try this:
> 
> (require 'pcvs)
> (call-interactively #'eval-expression)
> -----
> Eval: (cvs-mode!
> -----
> 
> The mode line shows the documentation for `cvs-mode' (without the !),
> which is a different function and has a different lambda list.
> 
> This is happening because the syntax table in the minibuffer never gets
> changed to the elisp table, which acknowledges all the valid symbol
> characters as part of the "symbol" class.
> 
> There are a couple different spots in the code to which you could
> attribute this lapse. For one, the elisp-mode.el function
> `elisp--current-symbol' isn't wrapped in a `with-syntax-table', unlike
> other similar definitions in the same file. I think anyone invoking this
> function could reasonably expect it to observe elisp syntax, so that's
> what my tiny patch addresses. This fixes the Eldoc problem.
> 
> But here's another weird thing further down the call stack.
> `read--expression' has a FIXME comment saying to turn on
> `emacs-lisp-mode' in the minibuffer -- which would also set the
> appropriate syntax table -- but it doesn't actually do it. I guess that
> must not work for whatever reason (since it has to have taken longer to
> write the comment than it would have taken to add the code). Should it
> be changed now so that it does set the major mode? Is there a problem
> with specialized major modes in the minibuffer? I hereby kick the can
> over to whoever knows more.
> 
> Daniel
> 
> 
> [2:text/x-patch Hide Save:0001-lisp-progmodes-elisp-mode.el-elisp-current-symbol-Se.patch (1kB)]
> 
> >From 1d4ed0e89b4ebc040609b7476128062685843c7f Mon Sep 17 00:00:00 2001
> From: Daniel Koning <dk@danielkoning.com>
> Date: Tue, 9 Jun 2020 19:24:11 -0500
> Subject: [PATCH] lisp/progmodes/elisp-mode.el (elisp--current-symbol): Set
>  syntax table.
> 
> Even if `elisp-current-symbol' is called from a buffer with a different
> active syntax table (such as the minibuffer in `read-from-minibuffer'),
> use `emacs-lisp-mode-syntax-table' to determine the symbol boundaries.
> 
> Copyright-paperwork-exempt: yes
> ---
>  lisp/progmodes/elisp-mode.el | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> index f39ecf9b7b..65c01b2ce1 100644
> --- a/lisp/progmodes/elisp-mode.el
> +++ b/lisp/progmodes/elisp-mode.el
> @@ -1615,10 +1615,11 @@ elisp--beginning-of-sexp
>  
>  ;; returns nil unless current word is an interned symbol.
>  (defun elisp--current-symbol ()
> -  (let ((c (char-after (point))))
> -    (and c
> -         (memq (char-syntax c) '(?w ?_))
> -         (intern-soft (current-word)))))
> +  (with-syntax-table emacs-lisp-mode-syntax-table
> +    (let ((c (char-after (point))))
> +      (and c
> +           (memq (char-syntax c) '(?w ?_))
> +           (intern-soft (current-word))))))
>  
>  (defun elisp-function-argstring (arglist)
>    "Return ARGLIST as a string enclosed by ().
> -- 
> 2.20.1
> 

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Stefan Monnier]

>> (a) You're typing an elisp expression into `read-from-minibuffer', and
>> (b) the function name contains punctuation, such as ! or ?, whose
>> character class is "punctuation" and not "symbol" in the standard syntax
>> table.

When eldoc is activated in the minibuffer and uses the elisp backend
because we're reading an elisp expression, then the syntax-table should
also be set accordingly, indeed (not only for eldoc but also for
forward-sexp, ...).

Looks like a bug in the corresponding function (`read-expression` or nearby).

>> There are a couple different spots in the code to which you could
>> attribute this lapse. For one, the elisp-mode.el function
>> `elisp--current-symbol' isn't wrapped in a `with-syntax-table', unlike
>> other similar definitions in the same file. I think anyone invoking this
>> function could reasonably expect it to observe elisp syntax, so that's
>> what my tiny patch addresses. This fixes the Eldoc problem.

It's probably OK to do it as in your patch, yes.  Switching the
syntax-table can mess up `syntax-ppss`, so it's better if we can avoid
it, but in this specific case it seems unlikely to lead to a problem.

>> But here's another weird thing further down the call stack.
>> `read--expression' has a FIXME comment saying to turn on
>> `emacs-lisp-mode' in the minibuffer -- which would also set the
>> appropriate syntax table -- but it doesn't actually do it. I guess that
>> must not work for whatever reason (since it has to have taken longer to
>> write the comment than it would have taken to add the code). Should it
>> be changed now so that it does set the major mode?

I'm probably to blame for the comment.
I'm pretty sure just calling the major will break something.
I can't offhand tell you what, tho.  Writing the comment was faster than
trying it out and then seeing how to fix the corresponding problems.

>> Is there a problem with specialized major modes in the minibuffer?

There's the fact that it's not (never?) used, so it's a big unknown.
I think we *should* use major modes in the minibuffer (tho those major
modes will probably need to be custom tailored in most cases).
Any progress in that direction (e.g. just trying it to and reporting
the problems you encounter) will be welcome.


        Stefan

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Daniel Koning]

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

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

> When eldoc is activated in the minibuffer and uses the elisp backend
> because we're reading an elisp expression, then the syntax-table should
> also be set accordingly, indeed (not only for eldoc but also for
> forward-sexp, ...).
>
> Looks like a bug in the corresponding function (`read-expression` or nearby).

OK, I've attached another patch for that function.

>>> For one, the elisp-mode.el function `elisp--current-symbol' isn't
>>> wrapped in a `with-syntax-table', unlike other similar definitions
>>> in the same file. I think anyone invoking this function could
>>> reasonably expect it to observe elisp syntax, so that's what my tiny
>>> patch addresses.
>
> It's probably OK to do it as in your patch, yes.
> 
If you change `read--expression', I don't know whether you'll think it
makes sense to add the extra overhead to `elisp--current-symbol'. It
does have a silent dependency on the syntax table, which is arguably an
inherent bug. But it's an internal function which only (right now) gets
called in one place.

> I'm pretty sure just calling the major will break something.
> I can't offhand tell you what, tho.  Writing the comment was faster than
> trying it out and then seeing how to fix the corresponding problems.

I can confirm that it suffers from the fairly significant drawback of
not working at all. Since the major replaces the minibuffer-specific
keymap, nothing ends up bound to `exit-minibuffer', so you can't even
enter an expression (or do anything else, like history paging). You
could bind `overriding-local-map' at the top of the function, but with
that and the syntax table you're getting close to implementing a de
facto new mode inside the de jure old one. At that point it might as
well be a real custom major mode, as you suggested. And then there's the
possibility that some people's `emacs-lisp-mode-hook' might contain code
that assumes it's in a real buffer and misbehaves in the minibuffer.

On the other hand, you could do what eshell does for `eshell-command'
and turn on its major mode in the minibuffer while selectively binding
C-j, C-m, M-p, etc., inside the setup hook. I don't like this design at
all: if I were to make changes to my `minibuffer-local-map' bindings,
eshell would silently ignore them. Any custom mode for minibuffer input
should inherit an existing minibuffer keymap, in my book at least.

Daniel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: read--expression patch --]
[-- Type: text/x-patch, Size: 1283 bytes --]

From 7b3a19b3616b3a1c8dbd26673f2034bc6b19ebed Mon Sep 17 00:00:00 2001
From: Daniel Koning <dk@danielkoning.com>
Date: Mon, 22 Jun 2020 19:03:20 -0500
Subject: [PATCH] lisp/simple.el (read--expression): Set syntax table.

Use `emacs-lisp-mode-syntax-table' when reading a Lisp expression from
the minibuffer.

Copyright-paperwork-exempt: yes
---
 lisp/simple.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index ea16d1400c..8e71432f9c 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1608,8 +1608,11 @@ read--expression
   (let ((minibuffer-completing-symbol t))
     (minibuffer-with-setup-hook
         (lambda ()
-          ;; FIXME: call emacs-lisp-mode (see also
-          ;; `eldoc--eval-expression-setup')?
+          ;; FIXME: instead of just applying the syntax table, maybe
+          ;; use a special major mode tailored to reading Lisp
+          ;; expressions from the minibuffer? (`emacs-lisp-mode'
+          ;; doesn't preserve the necessary keybindings.)
+          (set-syntax-table emacs-lisp-mode-syntax-table)
           (add-hook 'completion-at-point-functions
                     #'elisp-completion-at-point nil t)
           (run-hooks 'eval-expression-minibuffer-setup-hook))
-- 
2.20.1

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Stefan Monnier]

>> Looks like a bug in the corresponding function (`read-expression` or nearby).
> OK, I've attached another patch for that function.

Looks good to me, thanks.

> If you change `read--expression', I don't know whether you'll think it
> makes sense to add the extra overhead to `elisp--current-symbol'.

I'm not worried about the overhead, but I don't like the potential
syntax-ppss interaction, so if it's not needed, I'd rather not go there.

> I can confirm that it suffers from the fairly significant drawback of
> not working at all.  Since the major replaces the minibuffer-specific
> keymap, nothing ends up bound to `exit-minibuffer', so you can't even
> enter an expression (or do anything else, like history paging).

Fun!  Thanks for trying ;-)

> You could bind `overriding-local-map' at the top of the function, but
> with that and the syntax table you're getting close to implementing
> a de facto new mode inside the de jure old one. At that point it might
> as well be a real custom major mode, as you suggested.

Right.  It's probably worth doing something here in the longer run, tho.
E.g. the `minibuffer-local-map` and friends should really be major mode
maps (and the inheritance between the maps should probably be reflected as
inheritance between corresponding modes).  Then you'd create an
`emacs-lisp-minibuffer-mode` by deriving from some "normal" `minibuffer-local-mode`.

> And then there's the possibility that some people's
> `emacs-lisp-mode-hook' might contain code that assumes it's in a real
> buffer and misbehaves in the minibuffer.

Indeed.

> On the other hand, you could do what eshell does for `eshell-command'
> and turn on its major mode in the minibuffer while selectively binding
> C-j, C-m, M-p, etc., inside the setup hook.  I don't like this design at
> all: if I were to make changes to my `minibuffer-local-map' bindings,
> eshell would silently ignore them.  Any custom mode for minibuffer input
> should inherit an existing minibuffer keymap, in my book at least.

Agreed.


        Stefan

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Daniel Koning]

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

> Then you'd create an `emacs-lisp-minibuffer-mode` by deriving from
> some "normal" `minibuffer-local-mode`.

Do you suppose it might be sensible to rename `minibuffer-inactive-mode'
to `minibuffer-local-mode' right away (or `minibuffer-mode',
`minibuffer-base-mode', or some such)? While preserving the old name as
an alias, of course.

Right now, unless overridden by eshell or another package like it, the
minibuffer is always in `minibuffer-inactive-mode', and it's kind of
confusing that an active minibuffer calls itself inactive when you ask
for its major mode.

Daniel

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Stefan Monnier]

>> Then you'd create an `emacs-lisp-minibuffer-mode` by deriving from
>> some "normal" `minibuffer-local-mode`.
> Do you suppose it might be sensible to rename
> `minibuffer-inactive-mode' to `minibuffer-local-mode' right away (or
> `minibuffer-mode', `minibuffer-base-mode', or some such)?

No, the `minibuffer-inactive-mode` is the mode used when *not* in the minibuffer.
Its main purpose is to install a special keymap (which can basically
only be really used when you have a separate minibuffer frame).

> Right now, unless overridden by eshell or another package like it, the
> minibuffer is always in `minibuffer-inactive-mode',

I don't think that's true in the sense of "you get the behavior of
minibuffer-inactive-mode" (most importantly you don't get its keymap).
It may be true in the sense of "that's what `major-mode` says", but
that's rather irrelevant.


        Stefan

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Daniel Koning]

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

>> Right now, unless overridden by eshell or another package like it, the
>> minibuffer is always in `minibuffer-inactive-mode',
>
> I don't think that's true in the sense of "you get the behavior of
> minibuffer-inactive-mode" (most importantly you don't get its keymap).
> It may be true in the sense of "that's what `major-mode` says", but
> that's rather irrelevant.

I see your point, but I doubt most users and elisp writers would agree
that it's irrelevant what the value of `major-mode' says in plain
English. We should be able to count on drawing some conclusions from it.

Generally, the use of one major for two very different keymaps and
program states (while circumventing the problems by changing things à la
carte) strikes me as a pattern worth deprecating. But I think we agree
on that.

Daniel

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Stefan Monnier]

> I see your point, but I doubt most users and elisp writers would agree
> that it's irrelevant what the value of `major-mode' says in plain
> English. We should be able to count on drawing some conclusions from it.

What I meant is that the value of `major-mode` you see is just an
accident because the minibuffer-setup code doesn't bother to really set
a new major mode (or call kill-all-local-variables), so you get a stale
value in the `major-mode` variable.


        Stefan

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  41781@debbugs.gnu.org
> Date: Mon, 22 Jun 2020 19:08:57 -0500
> 
> OK, I've attached another patch for that function.

Thanks, I installed this on the master branch.

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Koning <dk@danielkoning.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  41781@debbugs.gnu.org
>> Date: Mon, 22 Jun 2020 19:08:57 -0500
>> 
>> OK, I've attached another patch for that function.
>
> Thanks, I installed this on the master branch.

I've only skimmed this thread, but it seems like the patch that was
installed fixed the problem that was being discussed?  It's a bit
unclear...

So should this bug report be closed?

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

bug#41781: 27.0.91; [PATCH] Eldoc describes the wrong function when reading an expression from the minibuffer

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Daniel Koning <dk@danielkoning.com>,  monnier@iro.umontreal.ca,
>   41781@debbugs.gnu.org
> Date: Wed, 05 Aug 2020 17:59:28 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Daniel Koning <dk@danielkoning.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  41781@debbugs.gnu.org
> >> Date: Mon, 22 Jun 2020 19:08:57 -0500
> >> 
> >> OK, I've attached another patch for that function.
> >
> > Thanks, I installed this on the master branch.
> 
> I've only skimmed this thread, but it seems like the patch that was
> installed fixed the problem that was being discussed?  It's a bit
> unclear...
> 
> So should this bug report be closed?

I think it should be closed, indeed, so I have now done so.

Thanks.