bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Dario Gjorgjevski]

How to reproduce from emacs -Q:

  (setq enable-recursive-minibuffers t)
  (setq icomplete-show-matches-on-no-input t)
  M-x icomplete-mode RET
  C-x C-f
    [Icomplete now exhibits some filenames in the minibuffer]
  M-:
    [Icomplete shouldn’t exhibit anything here but it still
     exhibits the filenames from the previous step and even
     allows you to complete them with C-j]

You can replace C-x C-f with other similar commands, e.g., C-x b or M-x.
I’m not sure if this bug comes from how Icomplete configures itself or
the implementation of recursive minibuffers.  It appears that
icomplete-simple-completing-p returns t when the M-: is happening in a
recursive minibuffer, even though it should return nil so that Icomplete
doesn’t configure itself.

If the M-: is not in a recursive minibuffer, everything is OK since
icomplete-simple-completing-p returns nil.

Best regards,
Dario
  
-- 
$ keyserver=hkps://hkps.pool.sks-keyservers.net
$ keyid=744A4F0B4F1C9371
$ gpg --keyserver $keyserver --search-keys $keyid

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>
> How to reproduce from emacs -Q:
>
>  (setq enable-recursive-minibuffers t)
>  (setq icomplete-show-matches-on-no-input t)
>  M-x icomplete-mode RET
>  C-x C-f
>    [Icomplete now exhibits some filenames in the minibuffer]
>  M-:
>    [Icomplete shouldn’t exhibit anything here but it still
>     exhibits the filenames from the previous step and even
>     allows you to complete them with C-j]
>
> You can replace C-x C-f with other similar commands, e.g., C-x b or M-x. 
> I’m not sure if this bug comes from how Icomplete configures itself or 
> the implementation of recursive minibuffers.  It appears that 
> icomplete-simple-completing-p returns t when the M-: is happening in a 
> recursive minibuffer, even though it should return nil so that Icomplete 
> doesn’t configure itself.
>
> If the M-: is not in a recursive minibuffer, everything is OK since 
> icomplete-simple-completing-p returns nil.
>

Indeed; patch attached.  Ideally this should be done inside 
icomplete-minibuffer-setup, but if we did this (by checking 
minibuffer-completing-symbol), it would prevent icomplete-mode from being 
active in the opposite situation: M-: followed by C-x C-f.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Disable-icomplete-mode-when-reading-an-Emacs-Lisp-ex.patch, Size: 1237 bytes --]

From e4cb07276724486bf01875c2463debf81784d59f Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 15 Apr 2021 17:33:37 +0000
Subject: [PATCH] Disable icomplete-mode when reading an Emacs Lisp expression

* lisp/simple.el (read--expression): Disable icomplete-mode when
entering a recursive minibuffer to read an Emacs Lisp expression
(bug#45474).
---
 lisp/simple.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..8f9d7a197c 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1754,7 +1754,9 @@ read--expression
           (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))
+          (run-hooks 'eval-expression-minibuffer-setup-hook)
+          ;; if we enter a recursive minibuffer, disable icomplete (bug#45474)
+          (setq-local icomplete-mode nil))
       (read-from-minibuffer prompt initial-contents
                             read-expression-map t
                             'read-expression-history))))
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

>> If the M-: is not in a recursive minibuffer, everything is OK since
>> icomplete-simple-completing-p returns nil.
>
> Indeed; patch attached.  Ideally this should be done inside
> icomplete-minibuffer-setup, but if we did this (by checking
> minibuffer-completing-symbol), it would prevent icomplete-mode from being
> active in the opposite situation: M-: followed by C-x C-f.
>
> @@ -1754,7 +1754,9 @@ read--expression
>            (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))
> +          (run-hooks 'eval-expression-minibuffer-setup-hook)
> +          ;; if we enter a recursive minibuffer, disable icomplete (bug#45474)
> +          (setq-local icomplete-mode nil))

Is this really specific only to read--expression?
I can reproduce the same issue with any command
that doesn't use completion, e.g. C-x C-f C-x f.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>>> If the M-: is not in a recursive minibuffer, everything is OK since 
>>> icomplete-simple-completing-p returns nil.
>>
>> Indeed; patch attached.  Ideally this should be done inside 
>> icomplete-minibuffer-setup, but if we did this (by checking 
>> minibuffer-completing-symbol), it would prevent icomplete-mode from 
>> being active in the opposite situation: M-: followed by C-x C-f.
>>
>> @@ -1754,7 +1754,9 @@ read--expression
>>            (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))
>> +          (run-hooks 'eval-expression-minibuffer-setup-hook)
>> +          ;; if we enter a recursive minibuffer, disable icomplete (bug#45474)
>> +          (setq-local icomplete-mode nil))
>
> Is this really specific only to read--expression? I can reproduce the 
> same issue with any command that doesn't use completion, e.g. C-x C-f 
> C-x f.
>

Indeed, but the bug report was only about read-expression.  It's far more 
annoying to see completion candidates when you want to type a complete 
expression than when you just have to enter a short argument.

Anyway, given that you asked for it, here's a more general solution.  I 
did not check all places where read-from-minibuffer is used, but adapting 
them is straightforward.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-icomplete-mode-in-recurs.patch, Size: 4658 bytes --]

From 7fc38f2a66f5545c276836d62a7e3a8669c3eb10 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 15 Apr 2021 22:27:08 +0000
Subject: [PATCH] Make it possible to disable icomplete-mode in recursive
 minibuffers

* lisp/icomplete.el (minibuffer-disable-icomplete-mode): New internal
variable.
(icomplete-minibuffer-setup): Disable icomplete-mode when
minibuffer-disable-icomplete-mode is non-nil

* lisp/subr.el (read-number, read-char-from-minibuffer, y-or-n-p): Use it.

* lisp/simple.el (read--expression): Use it.
---
 lisp/icomplete.el |  5 +++++
 lisp/simple.el    |  3 +++
 lisp/subr.el      | 17 ++++++++++++-----
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d5b6f76d7b..551b8bdc4a 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -442,10 +442,15 @@ icomplete-simple-completing-p
                (eq icomplete-with-completion-tables t)
                (member table icomplete-with-completion-tables))))))
 
+(defvar minibuffer-disable-icomplete-mode nil)
+
 ;;;_ > icomplete-minibuffer-setup ()
 (defun icomplete-minibuffer-setup ()
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
+  (when minibuffer-disable-icomplete-mode
+    (setq-local icomplete-mode nil)
+    (setq minibuffer-disable-icomplete-mode nil))
   (when (and icomplete-mode (icomplete-simple-completing-p))
     (setq-local icomplete--initial-input (icomplete--field-string))
     (setq-local completion-show-inline-help nil)
diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..9626b3605f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1739,6 +1739,8 @@ eval-expression-print-format
 (defvar eval-expression-minibuffer-setup-hook nil
   "Hook run by `eval-expression' when entering the minibuffer.")
 
+(defvar minibuffer-disable-icomplete-mode)
+
 (defun read--expression (prompt &optional initial-contents)
   "Read an Emacs Lisp expression from the minibuffer.
 
@@ -1755,6 +1757,7 @@ read--expression
           (add-hook 'completion-at-point-functions
                     #'elisp-completion-at-point nil t)
           (run-hooks 'eval-expression-minibuffer-setup-hook))
+      (setq minibuffer-disable-icomplete-mode t)
       (read-from-minibuffer prompt initial-contents
                             read-expression-map t
                             'read-expression-history))))
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..bc25fe234f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2794,6 +2794,8 @@ read-passwd
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
+(defvar minibuffer-disable-icomplete-mode)
+
 (defun read-number (prompt &optional default hist)
   "Read a numeric value in the minibuffer, prompting with PROMPT.
 DEFAULT specifies a default value to return if the user just types RET.
@@ -2812,6 +2814,7 @@ read-number
 					prompt t t))))
     (while
 	(progn
+	  (setq minibuffer-disable-icomplete-mode t)
 	  (let ((str (read-from-minibuffer
 		      prompt nil nil nil (or hist 'read-number-history)
 		      (when default
@@ -3052,8 +3055,10 @@ read-char-from-minibuffer
          ;; Protect this-command when called from pre-command-hook (bug#45029)
          (this-command this-command)
          (result
-          (read-from-minibuffer prompt nil map nil
-                                (or history 'empty-history)))
+          (progn
+            (setq minibuffer-disable-icomplete-mode t)
+            (read-from-minibuffer prompt nil map nil
+                                  (or history 'empty-history))))
          (char
           (if (> (length result) 0)
               ;; We have a string (with one character), so return the first one.
@@ -3247,9 +3252,11 @@ y-or-n-p
                        map))
              ;; Protect this-command when called from pre-command-hook (bug#45029)
              (this-command this-command)
-             (str (read-from-minibuffer
-                   prompt nil keymap nil
-                   (or y-or-n-p-history-variable 'empty-history))))
+             (str (progn
+                    (setq minibuffer-disable-icomplete-mode t)
+                    (read-from-minibuffer
+                     prompt nil keymap nil
+                     (or y-or-n-p-history-variable 'empty-history)))))
         (setq answer (if (member str '("y" "Y")) 'act 'skip)))))
     (let ((ret (eq answer 'act)))
       (unless noninteractive
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>
> Anyway, given that you asked for it, here's a more general solution.  I 
> did not check all places where read-from-minibuffer is used, but 
> adapting them is straightforward.
>

And this becomes more elegant, and can be used by other completion 
backends, with two macros.  See attached patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-a-completion-backend-in-.patch, Size: 4780 bytes --]

From 99050c96e7ff522aa1b180920fac74e98a2d6c79 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 15 Apr 2021 23:50:24 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

* lisp/subr.el (read-from-minibuffer-simple): New macro.
(disable-for-read-from-minibuffer-simple): New macro.
(read-from-minibuffer-simple): New internal variable.
(read-number, read-char-from-minibuffer, y-or-n-p): Use the new macro

* lisp/simple.el (read--expression): Use the new macro.

* lisp/icomplete.el (icomplete-minibuffer-setup): Use the new macro.
---
 lisp/icomplete.el |  1 +
 lisp/simple.el    |  6 +++---
 lisp/subr.el      | 28 ++++++++++++++++++++++++----
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d5b6f76d7b..76313eb91d 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -446,6 +446,7 @@ icomplete-simple-completing-p
 (defun icomplete-minibuffer-setup ()
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
+  (disable-for-read-from-minibuffer-simple icomplete-mode)
   (when (and icomplete-mode (icomplete-simple-completing-p))
     (setq-local icomplete--initial-input (icomplete--field-string))
     (setq-local completion-show-inline-help nil)
diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..7a0953a939 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1755,9 +1755,9 @@ read--expression
           (add-hook 'completion-at-point-functions
                     #'elisp-completion-at-point nil t)
           (run-hooks 'eval-expression-minibuffer-setup-hook))
-      (read-from-minibuffer prompt initial-contents
-                            read-expression-map t
-                            'read-expression-history))))
+      (read-from-minibuffer-simple prompt initial-contents
+                                   read-expression-map t
+                                   'read-expression-history))))
 
 (defun read--expression-try-read ()
   "Try to read an Emacs Lisp expression in the minibuffer.
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..d2230f6034 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2791,6 +2791,26 @@ read-passwd
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
+(defvar read-from-minibuffer-simple nil
+  "Whether `read-from-minibuffer' should display completion candidates.")
+
+(defmacro read-from-minibuffer-simple (&rest body)
+  "Read a string from the minibuffer without completion.
+Set `read-from-minibuffer-simple' to t, and call `read-from-minibuffer',
+which see."
+  `(progn
+     (setq read-from-minibuffer-simple t)
+     (read-from-minibuffer ,@body)))
+
+(defmacro disable-for-read-from-minibuffer-simple (completion-mode)
+  "Set COMPLETION-MODE to nil for `read-from-minibuffer-simple'.
+This is meant to be used by completion backends to setup the minibuffer
+to conditionally activate completion in each recursive invocation of
+the minibuffer, without affecting other recursive invocations."
+  `(when read-from-minibuffer-simple
+     (setq-local ,completion-mode nil)
+     (setq read-from-minibuffer-simple nil)))
+
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
@@ -2812,7 +2832,7 @@ read-number
 					prompt t t))))
     (while
 	(progn
-	  (let ((str (read-from-minibuffer
+	  (let ((str (read-from-minibuffer-simple
 		      prompt nil nil nil (or hist 'read-number-history)
 		      (when default
 			(if (consp default)
@@ -3052,8 +3072,8 @@ read-char-from-minibuffer
          ;; Protect this-command when called from pre-command-hook (bug#45029)
          (this-command this-command)
          (result
-          (read-from-minibuffer prompt nil map nil
-                                (or history 'empty-history)))
+          (read-from-minibuffer-simple prompt nil map nil
+                                       (or history 'empty-history)))
          (char
           (if (> (length result) 0)
               ;; We have a string (with one character), so return the first one.
@@ -3247,7 +3267,7 @@ y-or-n-p
                        map))
              ;; Protect this-command when called from pre-command-hook (bug#45029)
              (this-command this-command)
-             (str (read-from-minibuffer
+             (str (read-from-minibuffer-simple
                    prompt nil keymap nil
                    (or y-or-n-p-history-variable 'empty-history))))
         (setq answer (if (member str '("y" "Y")) 'act 'skip)))))
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

> @@ -2812,7 +2832,7 @@ read-number
> -	  (let ((str (read-from-minibuffer
> +	  (let ((str (read-from-minibuffer-simple

Thanks for handling 'read-number', I use commands from the 'M-g'
prefix map that read numbers in the completion minibuffer.

> @@ -3052,8 +3072,8 @@ read-char-from-minibuffer
> -          (read-from-minibuffer prompt nil map nil
> -                                (or history 'empty-history)))
> +          (read-from-minibuffer-simple prompt nil map nil
> +                                       (or history 'empty-history)))
>…
> @@ -3247,7 +3267,7 @@ y-or-n-p
> -             (str (read-from-minibuffer
> +             (str (read-from-minibuffer-simple

I wonder do you really intend to replace all hundreds of
read-from-minibuffer calls with read-from-minibuffer-simple?
For example, I use 'query-replace' a lot in the completion minibuffer,
but it's not fixed.

To be able to narrow the fix to icomplete.el only, it's
possible to check the value returned from (minibuffer-depth)
before icomplete kicks in, and disable icomplete completions
when the value is greater than it was when entered the first
minibuffer initially, thus handling recursive minibuffers
that don't provide completions.  Or maybe simply disable
previous icomplete when recursive minibuffer doesn't use
completions.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> I wonder do you really intend to replace all hundreds of 
> read-from-minibuffer calls with read-from-minibuffer-simple? For 
> example, I use 'query-replace' a lot in the completion minibuffer, but 
> it's not fixed.
>

I don't know.  Either we can fix them one by one as bug reports are filed, 
or we can fix them in one shot, possibly with a shorter name (say 
"input-from-minibuffer" or "get-from-minibuffer").  I don't see why it 
would be wrong to do that, or what it could break; the macro is as simple 
as possible.

>
> To be able to narrow the fix to icomplete.el only, it's possible to 
> check the value returned from (minibuffer-depth) before icomplete kicks 
> in, and disable icomplete completions when the value is greater than it 
> was when entered the first minibuffer initially, thus handling recursive 
> minibuffers that don't provide completions.
>

I'm not sure I understand what you mean by this, but AFAICS 
minibuffer-depth cannot be used to determine whether icomplete (or other 
completion backends) should be activated or not.

If you do M-: C-x C-f, you want completions at minibuffer-depth = 1 and 
not at minibuffer-depth = 0; if you do C-x C-f M-:, you want completions 
at minibuffer-depth 0 and not at minibuffer-depth 1; if you do C-x C-f C-x 
8 RET you want completions at minibuffer-depth 0 and at minibuffer-depth 
1; if you do M-: C-x f you do not want completions at minibuffer-depth 0 
nor at minibuffer-depth 1.

>
> Or maybe simply disable previous icomplete when recursive minibuffer 
> doesn't use completions.
>

And how would you detect whether a recursive minibuffer expects 
completions or not?

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

>> Or maybe simply disable previous icomplete when recursive minibuffer
>> doesn't use completions.
>
> And how would you detect whether a recursive minibuffer expects completions
> or not?

Oh, I see it's a more fundamental problem:

1. completing-read-default let-binds minibuffer-completion-table to a collection
   before calling read-from-minibuffer;
2. any nested call to read-from-minibuffer reuses the same value of
   minibuffer-completion-table, even if it doesn't use completion;
3. icomplete checks for minibuffer-completion-table to decide
   whether to activate icomplete completions in the minibuffer.

This is how non-completion minibuffers get non-nil minibuffer-completion-table.

'read-string' solves this problem by let-binding minibuffer-completion-table
to nil before calling read-from-minibuffer.  'read-number' could do the same.
But what about all other calls of 'read-from-minibuffer'?  They all can't be
replaced with a new function that will let-bind minibuffer-completion-table
to nil.

I have no idea how to fix this.  Maybe Stefan could help (Cc:ed).

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> Oh, I see it's a more fundamental problem:
>

Yes ;-)

>
> 1. completing-read-default let-binds minibuffer-completion-table to a 
> collection before calling read-from-minibuffer;
>
> 2. any nested call to read-from-minibuffer reuses the same value of 
> minibuffer-completion-table, even if it doesn't use completion;
>

Not necessarily, if another completion table has been set during the new 
minibuffer invocation.  That's what happens with C-x C-f followed by C-x 8 
RET.

>
> 3. icomplete checks for minibuffer-completion-table to decide whether to 
> activate icomplete completions in the minibuffer.
>
> This is how non-completion minibuffers get non-nil 
> minibuffer-completion-table.
>
> 'read-string' solves this problem by let-binding 
> minibuffer-completion-table to nil before calling read-from-minibuffer. 
> 'read-number' could do the same. But what about all other calls of 
> 'read-from-minibuffer'?  They all can't be replaced with a new function 
> that will let-bind minibuffer-completion-table to nil.
>

You forgot one element of the problem: completing-read also uses 
read-from-minibuffer.  So you cannot simply use read-from-minibuffer => no 
completions, completing-read => completions.

>
> I have no idea how to fix this.  Maybe Stefan could help (Cc:ed).
>

What's wrong with my approach, which disables the completion backend on 
demand?  A variant of it would be to add an eight argument to 
read-from-minibuffer.  AFAICS it's only the caller that can know whether 
the completion backend should be used, IOW, the only thing that the 
completion backend can do is to obey the caller.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> What's wrong with my approach, which disables the completion backend on
> demand?

[ I'm not sure which is "your approach", sorry.  ]

> A variant of it would be to add an eight argument to
> read-from-minibuffer.  AFAICS it's only the caller that can know whether the
> completion backend should be used, IOW, the only thing that the completion
> backend can do is to obey the caller.

We should think hard before adding yet another argument.  I agree that
the current design is problematic.  Basically, I think that
`minibuffer-completion-table` should be set buffer-locally in the
minibuffer instead of being "set" by dynamic scoping.

Fixing the problem "right" might call for a significant redesign of
`read-from-minibuffer`s API and `completing-read`s implementation.

A quick&dirty workaround for now would be for `completing-read` to set
some var alongside `minibuffer-completion-table` which indicates *which*
minibuffer this is for (e.g. some minibuffer-depth information).
This way `read-from-minibuffer` could distinguish
a `minibuffer-completion-table` passed for the current invocation from
one passed for the outer invocation and could hence temporarily rebind
`minibuffer-completion-table` to nil.

Another approach would be for `completing-read` not to let-bind those
vars but instead to use `minibuffer-setup-hook` to set the vars
buffer-locally.

Of course, this ties-in with the discussion about `minibuffer-mode`
where I mentioned that I think the minibuffers should use dedicated
major modes, and that which major mode to use should be indicated via an
argument passed to `read-from-minibuffer`.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>> What's wrong with my approach, which disables the completion backend on 
>> demand?
>
> [ I'm not sure which is "your approach", sorry.  ]
>

See the attached patch.  I's a draft, as I said 
read-from-minibuffer-simple could probably renamed to something more 
elegant, and (at least some of) the other calls to read-from-minibuffer 
could be replaced by the macro.

>> A variant of it would be to add an eight argument to 
>> read-from-minibuffer.  AFAICS it's only the caller that can know 
>> whether the completion backend should be used, IOW, the only thing that 
>> the completion backend can do is to obey the caller.
>
> We should think hard before adding yet another argument.
>

Yes ;-)  Which is why I did not do that.

>
> I agree that the current design is problematic.  Basically, I think that 
> `minibuffer-completion-table` should be set buffer-locally in the 
> minibuffer instead of being "set" by dynamic scoping.
>
> Fixing the problem "right" might call for a significant redesign of 
> `read-from-minibuffer`s API and `completing-read`s implementation.
>
> A quick&dirty workaround for now would be for `completing-read` to set 
> some var alongside `minibuffer-completion-table` which indicates *which* 
> minibuffer this is for (e.g. some minibuffer-depth information). This 
> way `read-from-minibuffer` could distinguish a 
> `minibuffer-completion-table` passed for the current invocation from one 
> passed for the outer invocation and could hence temporarily rebind 
> `minibuffer-completion-table` to nil.
>
> Another approach would be for `completing-read` not to let-bind those 
> vars but instead to use `minibuffer-setup-hook` to set the vars 
> buffer-locally.
>

These are yet other possible approaches indeed, but it seems to me at 
first sight that they are more complex than the solution I suggest.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-a-completion-backend-in-.patch, Size: 4780 bytes --]

From 99050c96e7ff522aa1b180920fac74e98a2d6c79 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 15 Apr 2021 23:50:24 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

* lisp/subr.el (read-from-minibuffer-simple): New macro.
(disable-for-read-from-minibuffer-simple): New macro.
(read-from-minibuffer-simple): New internal variable.
(read-number, read-char-from-minibuffer, y-or-n-p): Use the new macro

* lisp/simple.el (read--expression): Use the new macro.

* lisp/icomplete.el (icomplete-minibuffer-setup): Use the new macro.
---
 lisp/icomplete.el |  1 +
 lisp/simple.el    |  6 +++---
 lisp/subr.el      | 28 ++++++++++++++++++++++++----
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d5b6f76d7b..76313eb91d 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -446,6 +446,7 @@ icomplete-simple-completing-p
 (defun icomplete-minibuffer-setup ()
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
+  (disable-for-read-from-minibuffer-simple icomplete-mode)
   (when (and icomplete-mode (icomplete-simple-completing-p))
     (setq-local icomplete--initial-input (icomplete--field-string))
     (setq-local completion-show-inline-help nil)
diff --git a/lisp/simple.el b/lisp/simple.el
index 999755a642..7a0953a939 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1755,9 +1755,9 @@ read--expression
           (add-hook 'completion-at-point-functions
                     #'elisp-completion-at-point nil t)
           (run-hooks 'eval-expression-minibuffer-setup-hook))
-      (read-from-minibuffer prompt initial-contents
-                            read-expression-map t
-                            'read-expression-history))))
+      (read-from-minibuffer-simple prompt initial-contents
+                                   read-expression-map t
+                                   'read-expression-history))))
 
 (defun read--expression-try-read ()
   "Try to read an Emacs Lisp expression in the minibuffer.
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..d2230f6034 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2791,6 +2791,26 @@ read-passwd
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
+(defvar read-from-minibuffer-simple nil
+  "Whether `read-from-minibuffer' should display completion candidates.")
+
+(defmacro read-from-minibuffer-simple (&rest body)
+  "Read a string from the minibuffer without completion.
+Set `read-from-minibuffer-simple' to t, and call `read-from-minibuffer',
+which see."
+  `(progn
+     (setq read-from-minibuffer-simple t)
+     (read-from-minibuffer ,@body)))
+
+(defmacro disable-for-read-from-minibuffer-simple (completion-mode)
+  "Set COMPLETION-MODE to nil for `read-from-minibuffer-simple'.
+This is meant to be used by completion backends to setup the minibuffer
+to conditionally activate completion in each recursive invocation of
+the minibuffer, without affecting other recursive invocations."
+  `(when read-from-minibuffer-simple
+     (setq-local ,completion-mode nil)
+     (setq read-from-minibuffer-simple nil)))
+
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
@@ -2812,7 +2832,7 @@ read-number
 					prompt t t))))
     (while
 	(progn
-	  (let ((str (read-from-minibuffer
+	  (let ((str (read-from-minibuffer-simple
 		      prompt nil nil nil (or hist 'read-number-history)
 		      (when default
 			(if (consp default)
@@ -3052,8 +3072,8 @@ read-char-from-minibuffer
          ;; Protect this-command when called from pre-command-hook (bug#45029)
          (this-command this-command)
          (result
-          (read-from-minibuffer prompt nil map nil
-                                (or history 'empty-history)))
+          (read-from-minibuffer-simple prompt nil map nil
+                                       (or history 'empty-history)))
          (char
           (if (> (length result) 0)
               ;; We have a string (with one character), so return the first one.
@@ -3247,7 +3267,7 @@ y-or-n-p
                        map))
              ;; Protect this-command when called from pre-command-hook (bug#45029)
              (this-command this-command)
-             (str (read-from-minibuffer
+             (str (read-from-minibuffer-simple
                    prompt nil keymap nil
                    (or y-or-n-p-history-variable 'empty-history))))
         (setq answer (if (member str '("y" "Y")) 'act 'skip)))))
-- 
2.30.2

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Drew Adams]

> We should think hard before adding yet another argument.  I agree that
> the current design is problematic.  Basically, I think that
> `minibuffer-completion-table` should be set buffer-locally in the
> minibuffer instead of being "set" by dynamic scoping.

Apologies for not following this thread, and so
possibly misunderstanding what you mean there.

But my code depends on `minibuffer-completion-table'
being "set" by dynamic scoping.  I update/change the
table dynamically during the same minibuffer reading.

I do this in several ways, for different purposes
(multiple, very different, purposes).

I also change `minibuffer-completion-predicate',
similarly.

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> We should think hard before adding yet another argument.  I agree that
>> the current design is problematic.  Basically, I think that
>> `minibuffer-completion-table` should be set buffer-locally in the
>> minibuffer instead of being "set" by dynamic scoping.
>
> Apologies for not following this thread, and so
> possibly misunderstanding what you mean there.
>
> But my code depends on `minibuffer-completion-table'
> being "set" by dynamic scoping.

I'm pretty sure there can be such corner cases, but your description
doesn't make it clear to me why you think setting buffer-locally
would interact poorly with your use.


        Stefan

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Drew Adams]

> >> We should think hard before adding yet another argument.  I agree that
> >> the current design is problematic.  Basically, I think that
> >> `minibuffer-completion-table` should be set buffer-locally in the
> >> minibuffer instead of being "set" by dynamic scoping.
> >
> > Apologies for not following this thread, and so
> > possibly misunderstanding what you mean there.
> >
> > But my code depends on `minibuffer-completion-table'
> > being "set" by dynamic scoping.
> 
> I'm pretty sure there can be such corner cases, but your description
> doesn't make it clear to me why you think setting buffer-locally
> would interact poorly with your use.

I don't know what more to say.  Not being able to
have `minibuffer-completion-table' dynamically
scoped and settable/redefinable as such a variable
would effectively destroy Icicles features that
rely on exactly that ability.  For Icicles, there's
nothing "corner" about this.

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> I don't know what more to say.  Not being able to
> have `minibuffer-completion-table' dynamically
> scoped and settable/redefinable as such a variable
> would effectively destroy Icicles features that
> rely on exactly that ability.

Let me rephrase my question:
What makes you think that having them be set buffer-locally rather than via
a global let-binding would prevent Icicles setting/redefining them?


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>>> What's wrong with my approach, which disables the completion backend
>>> on demand?
>> [ I'm not sure which is "your approach", sorry.  ]
> See the attached patch.  I's a draft, as I said read-from-minibuffer-simple
> could probably renamed to something more elegant, and (at least some of) the
> other calls to read-from-minibuffer could be replaced by the macro.

Ah, I see.  But that's based on "blacklisting" those places that don't
want to use minibuffer-completion-table, so it requires changes in many
places (including outside of Emacs's codebase).

> These are yet other possible approaches indeed, but it seems to me at first
> sight that they are more complex than the solution I suggest.

The patch below shows one way to do what I suggest.

Just like your approach, I think this is only a temporary fix until we
can solve the problem for real by making `minibuffer-completion-table`
buffer-local (which is also indispensable if we want to make completion
work with Alan's new support for having several active minibuffers
visible at the same time).


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c900b0d7ce6..e148c73889d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3843,6 +3843,7 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
+         (minibuffer--completion-keymap keymap)
          (result (read-from-minibuffer prompt initial-input keymap
                                        nil hist def inherit-input-method)))
     (when (and (equal result "") def)
diff --git a/src/minibuf.c b/src/minibuf.c
index a3c1b99bf32..872928ad578 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -563,6 +563,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   specbind (Qminibuffer_default, defalt);
   specbind (Qinhibit_read_only, Qnil);
 
+  if (!EQ (Vminibuffer__completion_keymap, map))
+    specbind (Qminibuffer_completion_table, Qnil);
+
   /* If Vminibuffer_completing_file_name is `lambda' on entry, it was t
      in previous recursive minibuffer, but was not set explicitly
      to t for this invocation, so set it to nil in this minibuffer.
@@ -1348,12 +1351,6 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
   Lisp_Object val;
   ptrdiff_t count = SPECPDL_INDEX ();
 
-  /* Just in case we're in a recursive minibuffer, make it clear that the
-     previous minibuffer's completion table does not apply to the new
-     minibuffer.
-     FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
-  specbind (Qminibuffer_completion_table, Qnil);
-
   val = Fread_from_minibuffer (prompt, initial_input, Qnil,
 			       Qnil, history, default_value,
 			       inherit_input_method);
@@ -2404,6 +2401,12 @@ syms_of_minibuf (void)
 variable is non-nil. */);
   enable_recursive_minibuffers = 0;
 
+  DEFVAR_LISP ("minibuffer--completion-keymap", Vminibuffer__completion_keymap,
+               doc: /* Keymap used together with `minibuffer-completion-table'.
+`read-from-minibuffer' compares it with its `keymap' to determine if
+the completion table was intended for it or not.  */);
+  Vminibuffer__completion_keymap = Qnil;
+
   DEFVAR_LISP ("minibuffer-completion-table", Vminibuffer_completion_table,
 	       doc: /* Alist or obarray used for completion in the minibuffer.
 This becomes the ALIST argument to `try-completion' and `all-completions'.

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Drew Adams]

> > I don't know what more to say.  Not being able to
> > have `minibuffer-completion-table' dynamically
> > scoped and settable/redefinable as such a variable
> > would effectively destroy Icicles features that
> > rely on exactly that ability.
> 
> Let me rephrase my question:
> What makes you think that having them be set
> buffer-locally rather than via a global let-binding
> would prevent Icicles setting/redefining them?

Let-binding of global, dynamic variables (defvar)?
Or something else?

I don't know what making such vars buffer-local might
do to the (Icicles) behavior.  It's not even clear to
me what buffer-local means for the minibuffer these
days (what with experiments/proposals about changing
its mode etc.).

It's possible that the effect wouldn't be nefarious.
But it's also possible it would be.  During minibuffer
use, other buffers can be current at various points,
for example.  But I'm not sure there's a problem there.

My reason for posting was that you were, or I thought
you were, contrasting dynamic and lexical binding, of
`minibuffer-completion-table' (and maybe similar vars).

It's dynamic binding that my code depends on for such
vars.  Dunno how important (for my code) global vs
buffer-local is in this context.  That's something
different.

Am I mistaken that you seem to have switched from a
consideration of dynamic vs lexical to global dynamic
vs buffer-local dynamic?

I'm sorry; I haven't followed this discussion.  I
just wanted to give a heads-up that it would be
problematic for me if such vars were made lexical or,
say, were simply passed as args.  I bind and set them
as dynamically-scoped vars.

If necessary, I can dig out examples of what I do.
I hope it doesn't come to that, though.

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> Am I mistaken that you seem to have switched from a
> consideration of dynamic vs lexical to global dynamic
> vs buffer-local dynamic?

Yes you are.


        Stefan

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Drew Adams]

> > Am I mistaken that you seem to have switched from a
> > consideration of dynamic vs lexical to global dynamic
> > vs buffer-local dynamic?
> 
> Yes you are.

So I guess you're not proposing that that variable no
longer be dynamically bound.  (If that guess is wrong,
please let me know.  Your communication is a bit "sec".)

This is what made me think that was your proposal, at
the outset:

  SM: "instead of being "set" by dynamic scoping"

You did mention buffer-local, but you also mentioned
"instead of" dynamic scoping.  It's still not too clear
to me what you have in mind.

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> So I guess you're not proposing that that variable no
> longer be dynamically bound.  (If that guess is wrong,
> please let me know.  Your communication is a bit "sec".)

Yes, it's a bit "sec" because your objection reduces to just "you're
proposing a change and I have no idea what you're talking about but I'll
object anyway because by definition a change may break something".

You're right, but it doesn't bring anything to the discussion.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>> See the attached patch.  I's a draft, as I said 
>> read-from-minibuffer-simple could probably renamed to something more 
>> elegant, and (at least some of) the other calls to read-from-minibuffer 
>> could be replaced by the macro.
>
> Ah, I see.  But that's based on "blacklisting" those places that don't 
> want to use minibuffer-completion-table, so it requires changes in many 
> places (including outside of Emacs's codebase).
>

It would be possible to use whitelisting instead by renaming the current 
read-from-minibuffer to internal-read-from-minibuffer, which would be 
wrapped in a macro read-from-minibuffer.  The change would be transparent, 
except for those places (e.g. completing-read-default) where what we 
actually want is to use internal-read-from-minibuffer.  But this change 
would be slightly more invasive than what follows.

>> These are yet other possible approaches indeed, but it seems to me at 
>> first sight that they are more complex than the solution I suggest.
>
> The patch below shows one way to do what I suggest.
>

Thanks.  Somehow I feel that using the keymap to decide whether the 
completion table should be used isn't safe enough, it's possible (at least 
in theory) that a minibuffer with a certain keymap uses completion tables 
and another one using the same keymap does not.  ISTM that it's safer (and 
more explicit) to use the current minibuffer depth for that purpose; see 
attached patch.

>
> Just like your approach, I think this is only a temporary fix until we 
> can solve the problem for real by making `minibuffer-completion-table` 
> buffer-local
>

I'm not sure I fully understand why this is necessary, but is 
"Fmake_variable_buffer_local (Qminibuffer_completion_table);" just after 
"if ... specbind (Qminibuffer_completion_table, Qnil);" not enough for 
this?

[-- Attachment #2: Type: text/x-diff, Size: 2918 bytes --]

From 4654c5251cab6703e4dcd94ecaa900bb970bb589 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 18 Apr 2021 22:15:08 +0000
Subject: [PATCH] Make it possible to disable completion in recursive
 minibuffers

---
 lisp/minibuffer.el |  1 +
 src/minibuf.c      | 15 +++++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c900b0d7ce..991ed9dc93 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3825,6 +3825,7 @@ completing-read-default
                 (1+ (cdr initial-input)))))
 
   (let* ((minibuffer-completion-table collection)
+         (minibuffer--depth-for-completion-table (minibuffer-depth))
          (minibuffer-completion-predicate predicate)
          ;; FIXME: Remove/rename this var, see the next one.
          (minibuffer-completion-confirm (unless (eq require-match t)
diff --git a/src/minibuf.c b/src/minibuf.c
index c9831fd50f..77992e1fea 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -559,6 +559,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   specbind (Qminibuffer_default, defalt);
   specbind (Qinhibit_read_only, Qnil);
 
+  if (!EQ (Vminibuffer__depth_for_completion_table, make_fixnum (minibuf_level)))
+    specbind (Qminibuffer_completion_table, Qnil);
+
   /* If Vminibuffer_completing_file_name is `lambda' on entry, it was t
      in previous recursive minibuffer, but was not set explicitly
      to t for this invocation, so set it to nil in this minibuffer.
@@ -1338,12 +1341,6 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
   Lisp_Object val;
   ptrdiff_t count = SPECPDL_INDEX ();
 
-  /* Just in case we're in a recursive minibuffer, make it clear that the
-     previous minibuffer's completion table does not apply to the new
-     minibuffer.
-     FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
-  specbind (Qminibuffer_completion_table, Qnil);
-
   val = Fread_from_minibuffer (prompt, initial_input, Qnil,
 			       Qnil, history, default_value,
 			       inherit_input_method);
@@ -2394,6 +2391,12 @@ syms_of_minibuf (void)
 variable is non-nil. */);
   enable_recursive_minibuffers = 0;
 
+  DEFVAR_LISP ("minibuffer--depth-for-completion-table", Vminibuffer__depth_for_completion_table,
+               doc: /* Minibuffer depth used together with `minibuffer-completion-table'.
+`read-from-minibuffer' compares it with the current minibuffer depth to
+determine if the completion table was intended for it or not.  */);
+  Vminibuffer__depth_for_completion_table = Qnil;
+
   DEFVAR_LISP ("minibuffer-completion-table", Vminibuffer_completion_table,
 	       doc: /* Alist or obarray used for completion in the minibuffer.
 This becomes the ALIST argument to `try-completion' and `all-completions'.
-- 
2.30.2

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Drew Adams]

> your objection reduces to just "you're proposing a
> change and I have no idea what you're talking about
> but I'll object anyway because by definition a change
> may break something".  You're right, but it doesn't
> bring anything to the discussion.

That's insulting, and not a helpful or fair
characterization.  I didn't object simply because
a change, any change, may break something.  I think
you know that.

I said clearly, at the outset:

   Apologies for not following this thread, and so
   possibly misunderstanding what you mean there.

That's a polite way of asking for clarification of
what you wrote there.

I said what I guessed you meant, and I said that if
that's what you meant then it's problematic for me.

It was clear that I understood you to be proposing
non-dynamic scoping for `minibuffer-completion-table',
based on your saying:

   instead of being "set" by dynamic scoping

And it was clear that I wasn't sure ("possibly
misunderstanding") what you meant by that phrase.

If you didn't mean that, and it was clear to you
that I was misunderstanding you, you could have
simply said so, and clarified what you meant.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>>> See the attached patch.  I's a draft, as I said
>>> read-from-minibuffer-simple could probably renamed to something more
>>> elegant, and (at least some of) the other calls to read-from-minibuffer
>>> could be replaced by the macro.
>> Ah, I see.  But that's based on "blacklisting" those places that don't
>> want to use minibuffer-completion-table, so it requires changes in many
>> places (including outside of Emacs's codebase).
> It would be possible to use whitelisting instead by renaming the current
> read-from-minibuffer to internal-read-from-minibuffer, which would be
> wrapped in a macro read-from-minibuffer.

Indeed.  I think we need more data in order to figure out which of the
two options breaks less/more code out there.

I was working under the assumption that the only calls to
`read-from-minibuffer` which need the minibuffer to have
a `minibuffer-completion-table` are those coming from
`completing-read-default` (in which case the whitelisting is the better
approach since it requires a change only in `completing-read-default`),
but the fact that there's a `completing-read-function` is a strong hint
that this assumption is probably wrong.

> The change would be transparent, except for those places
> (e.g. completing-read-default) where what we actually want is to use
> internal-read-from-minibuffer.  But this change would be slightly more
> invasive than what follows.

Actually, probably not very much, and it would be a lot cleaner.

>>> These are yet other possible approaches indeed, but it seems to me at
>>> first sight that they are more complex than the solution I suggest.
>> The patch below shows one way to do what I suggest.
> Thanks.  Somehow I feel that using the keymap to decide whether the
> completion table should be used isn't safe enough, it's possible (at least
> in theory) that a minibuffer with a certain keymap uses completion tables
> and another one using the same keymap does not.

I agree that it's possible in theory, but I think in practice it's
extremely unlikely ;-)

> ISTM that it's safer (and more explicit) to use the current minibuffer
> depth for that purpose; see attached patch.

Actually, I think this is not safe: I suspect that minibuffer uses that
take place from the debugger (or the Edebugger) right between the
let-binding of `minibuffer-completion-map` and the call to
`read-from-minibuffer` would all have just the same depth as the one
that will apply to the call to `read-from-minibuffer` so they would
"misfire".

If we add "recursive edit depth" to the mix, we may get something that
is somewhat reliable, tho.

Still, sounds terribly hackish/hideous.  Not much better than my "equal
keymap" heuristic.  Your above `internal-read-from-minibuffer` would be
miles better, I think.

>> Just like your approach, I think this is only a temporary fix until we can
>> solve the problem for real by making `minibuffer-completion-table`
>> buffer-local
> I'm not sure I fully understand why this is necessary, but is
> "Fmake_variable_buffer_local (Qminibuffer_completion_table);" just after "if
> ... specbind (Qminibuffer_completion_table, Qnil);" not enough for this?

No, I meant that instead of using let-binding to set the var, we'd use
`setq-local`.  This requires the code to run from within the minibuffer,
contrary to the current situation where the let-binding takes place
outside of the minibuffer.

IOW, the idea is that we'd pass to `read-from-minibuffer` some kind of
setup function.

But this is a much deeper change, and I expect it would break some
completion UIs which currently use `minibuffer-completion-table` (and
related variables) from other buffers than the minibuffer (e.g. from
*Completions*).
I wrote "would", but I do think such a change should happen at
some point.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>> It would be possible to use whitelisting instead by renaming the 
>> current read-from-minibuffer to internal-read-from-minibuffer, which 
>> would be wrapped in a macro read-from-minibuffer.
>
> Indeed.  I think we need more data in order to figure out which of the 
> two options breaks less/more code out there.
>

Here is some (hopefully useful) data:

There are 213 calls to read-from-minibuffer in Emacs core.  AFAICS, the 
only call that uses minibuffer-completion-table is the one inside 
completing-read-default.  There's another one inside ido-read-internal, 
but it doesn't seem to use minibuffer-completion-table, AFAIU it replaces 
completing-read with its own processing in ido-exhibit.

There are 76 calls to read-from-minibuffer on ELPA.  AFAICS, the only call 
that uses minibuffer-completion-table is the one iside ivy-read.

There are 903 calls to read-from-minibuffer on MELPA.  AFAICS, only a 
handful of them (yatex, gams-mode, python-django, magit) use 
minibuffer-completion-table.  The case of Magit is interesting: it solves 
the current problem by defining two functions, 
magit-completing-read-multiple that let-binds minibuffer-completion-table 
to the appropriate value, and magit-read-string that let-binds 
minibuffer-completion-table to nil.

>
> I was working under the assumption that the only calls to 
> `read-from-minibuffer` which need the minibuffer to have a 
> `minibuffer-completion-table` are those coming from 
> `completing-read-default` (in which case the whitelisting is the better 
> approach since it requires a change only in `completing-read-default`), 
> but the fact that there's a `completing-read-function` is a strong hint 
> that this assumption is probably wrong.
>

I believe your assumption is mostly correct.  There are apparently only 
very few occurrences of read-from-minibuffer that use 
minibuffer-completion-table, and my understanding is that those who set 
completing-read-function to another value do not use 
minibuffer-completion-table anymore.

>> The change would be transparent, except for those places (e.g. 
>> completing-read-default) where what we actually want is to use 
>> internal-read-from-minibuffer.  But this change would be slightly more 
>> invasive than what follows.
>
> Actually, probably not very much, and it would be a lot cleaner.
>

I agree.

>>> Just like your approach, I think this is only a temporary fix until we 
>>> can solve the problem for real by making `minibuffer-completion-table` 
>>> buffer-local
>>
>> I'm not sure I fully understand why this is necessary, but is 
>> "Fmake_variable_buffer_local (Qminibuffer_completion_table);" just 
>> after "if ... specbind (Qminibuffer_completion_table, Qnil);" not 
>> enough for this?
>
> No, I meant that instead of using let-binding to set the var, we'd use 
> `setq-local`. This requires the code to run from within the minibuffer, 
> contrary to the current situation where the let-binding takes place 
> outside of the minibuffer.
>

Yes, I understood this, but the effect of let-binding the var and make it 
buffer-local after the minibuffer has been created but before the 
minibuffer-setup-hook is executed is the same.  Or am I missing something? 
If not, this would solve the problem without breaking anything (as the 
global value of minibuffer-completion-table would not change).

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> There are 76 calls to read-from-minibuffer on ELPA.  AFAICS, the only call
> that uses minibuffer-completion-table is the one iside ivy-read.
[...]
> There are 903 calls to read-from-minibuffer on MELPA.  AFAICS, only
> a handful of them (yatex, gams-mode, python-django, magit) use
> minibuffer-completion-table.  The case of Magit is interesting: it solves
> the current problem by defining two functions,
> magit-completing-read-multiple that let-binds minibuffer-completion-table to
> the appropriate value, and magit-read-string that let-binds
> minibuffer-completion-table to nil.

[ Thanks for the footwork.  ]

So the whitelist approach we're considering would break and few cases
like `ivy-read`, and `magit-completing-read-multiple` and would fix the
corner case misbehavior in hundred of other calls.

The blacklist approach would instead not break anything but would
require adding extra code to a few hundred places to fix those corner
case misbehaviors.

I'm starting to feel that neither option is very attractive.
I think we should:
- Do something along the lines of the whitelist approach, as the
  "long term" solution.
- Add to it some "short term" heuristic (e.g. using something like the
  "equal keymap" or the minibuffer depth) that keeps it working for
  ivy-read and friends, maybe emitting a warning along the way (and with
  a clean way to silence this warning when it's a false positive) so we
  can remove the heuristic in Emacs-30.

WDYT?

>> No, I meant that instead of using let-binding to set the var, we'd use
>> `setq-local`. This requires the code to run from within the minibuffer,
>> contrary to the current situation where the let-binding takes place
>> outside of the minibuffer.
> Yes, I understood this, but the effect of let-binding the var and make it
> buffer-local after the minibuffer has been created but before the
> minibuffer-setup-hook is executed is the same.  Or am I missing something?

[ Note: I consider this part of the discussion as not directly relevant to
  bug#45474.  ]

Oh, so `read-from-minibuffer` receives it as an "implicit argument" vet
dynamically scoped let-binding and then sets it buffer locally?

That sounds like a potentially good halfway point, so code which
(incorrectly) uses `minibuffer-completion-table` from non-mini buffers
will still usually get the right table (e.g. when there's only one
minibuffer active).

> If not, this would solve the problem without breaking anything (as the
> global value of minibuffer-completion-table would not change).

Exactly.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

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

> A quick&dirty workaround for now would be for `completing-read` to set
> some var alongside `minibuffer-completion-table` which indicates *which*
> minibuffer this is for (e.g. some minibuffer-depth information).
> This way `read-from-minibuffer` could distinguish
> a `minibuffer-completion-table` passed for the current invocation from
> one passed for the outer invocation and could hence temporarily rebind
> `minibuffer-completion-table` to nil.

This patch is how I thought your suggestion could be implemented,
but then later you posted a different patch.  Anyway FWIW here is
a safer workable workaround that implements your first suggestion
and sets a new explicit buffer-local variable in completing minibuffers,
so modes that need to distinguish such minibuffers could check it.
Maybe this minibuffer-with-setup-hook could be moved even to
'completing-read'?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completing-minibuffer.patch --]
[-- Type: text/x-diff, Size: 1820 bytes --]

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 91bbb60013..543e451c5d 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -400,7 +400,7 @@ icomplete-mode
     (add-hook 'minibuffer-setup-hook #'icomplete-minibuffer-setup)))
 
 (defun icomplete--completion-table ()
-  (if (window-minibuffer-p) minibuffer-completion-table
+  (if (window-minibuffer-p) (and completing-minibuffer minibuffer-completion-table)
     (or (nth 2 completion-in-region--data)
 	(message "In %S (w=%S): %S"
 		 (current-buffer) (selected-window) (window-minibuffer-p)))))
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index a0247d1fba..3ceb67733d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3826,6 +3826,8 @@ completing-read-function
   "The function called by `completing-read' to do its work.
 It should accept the same arguments as `completing-read'.")
 
+(defvar completing-minibuffer nil)
+
 (defun completing-read-default (prompt collection &optional predicate
                                        require-match initial-input
                                        hist def inherit-input-method)
@@ -3839,6 +3841,9 @@ completing-read-default
                 ;; `read-from-minibuffer' uses 1-based index.
                 (1+ (cdr initial-input)))))
 
+  (minibuffer-with-setup-hook
+      (lambda ()
+        (setq-local completing-minibuffer t))
     (let* ((minibuffer-completion-table collection)
            (minibuffer-completion-predicate predicate)
            ;; FIXME: Remove/rename this var, see the next one.
@@ -3862,7 +3867,7 @@ completing-read-default
                                          nil hist def inherit-input-method)))
       (when (and (equal result "") def)
         (setq result (if (consp def) (car def) def)))
-    result))
+      result)))
 \f
 ;; Miscellaneous
 

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> I think we should:
>
> - Do something along the lines of the whitelist approach, as the "long 
> term" solution.
>
> - Add to it some "short term" heuristic (e.g. using something like the 
> "equal keymap" or the minibuffer depth) that keeps it working for 
> ivy-read and friends, maybe emitting a warning along the way (and with a 
> clean way to silence this warning when it's a false positive) so we can 
> remove the heuristic in Emacs-30.
>
> WDYT?
>

I've been thinking about this, and now have another potential solution in 
mind, which I'm not sure it is feasible.  The assumption of this idea is 
that the present problem is an instance of a more general problem, i.e. 
that similar problems will arise for other functions in the future.  The 
general idea is to make the following possible, when a function foo needs 
to be changed to depend on some variable bar or to accept an additional 
argument baz:

Emacs N:

- rename the function foo to internal-foo

- add a macro foo which (a) either calls internal-foo after setting bar or 
baz to a default value that make internal-foo behave as foo in Emacs N - 
1, or (b) calls internal-foo directly (i.e. assuming that bar or baz have 
been set appropriately)

The macro would (and here I'm not sure it is feasible) expand to (a) for 
external packages that have not yet been upgraded, and to (b) for those 
who have been upgraded and for Emacs core.  The question is: is it 
possible to have a predicate function to distinguish those two cases?

Emacs N + 1:

- remove the macro foo

- rename the function internal-foo back to foo

To illustrate the above with the present problem, read-from-minibuffer 
would depend on the value of completing-read-from-minibuffer, which would 
default to nil for Emacs core (and packages that have been upgraded) and 
would be let-bound to t in completing-read-default, and would default to t 
for external packages (that have not yet been upgraded).

>> Yes, I understood this, but the effect of let-binding the var and make 
>> it buffer-local after the minibuffer has been created but before the 
>> minibuffer-setup-hook is executed is the same.  Or am I missing 
>> something?
>
> [ Note: I consider this part of the discussion as not directly relevant 
> to bug#45474.  ]
>

Indeed ;-)  I guess the above idea is also a bit far from bug#45474...

>
> Oh, so `read-from-minibuffer` receives it as an "implicit argument" vet 
> dynamically scoped let-binding and then sets it buffer locally?
>

Exactly.  It receives an argument through its environment instead of an 
explicit one.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> but then later you posted a different patch.  Anyway FWIW here is
> a safer workable workaround that implements your first suggestion
> and sets a new explicit buffer-local variable in completing minibuffers,
> so modes that need to distinguish such minibuffers could check it.

I combined your idea with that of Gregory for the patch below.
It's far from perfect, but I think it strikes a good balance of
simplicity, preserving compatibility, and moving in the right direction.

WDYT?

> Maybe this minibuffer-with-setup-hook could be moved even to
> 'completing-read'?

Because of the fundamentally broken&messy way
`minibuffer-with-setup-hook` works, it's best to keep it as close as
possible to the call to `read-from-minibuffer`, IMO.


        Stefan


diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 91bbb600136..df9c5241234 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -400,7 +400,9 @@ icomplete-mode
     (add-hook 'minibuffer-setup-hook #'icomplete-minibuffer-setup)))
 
 (defun icomplete--completion-table ()
-  (if (window-minibuffer-p) minibuffer-completion-table
+  (if (window-minibuffer-p)
+      (if (local-variable-p 'minibuffer-completion-table)
+          minibuffer-completion-table)
     (or (nth 2 completion-in-region--data)
 	(message "In %S (w=%S): %S"
 		 (current-buffer) (selected-window) (window-minibuffer-p)))))
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c19f9096290..04fbd092c27 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3857,8 +3857,11 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
-         (result (read-from-minibuffer prompt initial-input keymap
-                                       nil hist def inherit-input-method)))
+         (result
+          (minibuffer-with-setup-hook
+              (lambda () (make-local-variable 'minibuffer-completion-table))
+            (read-from-minibuffer prompt initial-input keymap
+                                  nil hist def inherit-input-method))))
     (when (and (equal result "") def)
       (setq result (if (consp def) (car def) def)))
     result))

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>> but then later you posted a different patch.  Anyway FWIW here is a 
>> safer workable workaround that implements your first suggestion and 
>> sets a new explicit buffer-local variable in completing minibuffers, so 
>> modes that need to distinguish such minibuffers could check it.
>
> I combined your idea with that of Gregory for the patch below. It's far 
> from perfect, but I think it strikes a good balance of simplicity, 
> preserving compatibility, and moving in the right direction.
>
> WDYT?
>

I really like the simplicity of that solution, but (you know me, there's 
always a but...) I do not see what the next step in that direction could 
be.  My fear is that package authors will be encouraged to use a similar 
duct tape in their code, and that making the behavior of 
read-from-minibuffer depend on a environment variable (or an additional 
parameter) will never happen.

I attach a patch which is, I believe, safe in the sense that it cannot 
break any package, and which creates a transition period after which 
minibuffer-completion-table will automatically become buffer-local upon 
entering the minibuffer.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-a-completion-backend-in-.patch, Size: 6906 bytes --]

From 93a257cf50f1210f8b31487f892ea6bf8ba450db Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 20 Apr 2021 18:46:33 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

---
 lisp/minibuffer.el |  1 +
 lisp/subr.el       | 13 +++++++++++++
 src/fns.c          |  6 +++---
 src/minibuf.c      | 37 +++++++++++++++++++++++++++++++------
 4 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c900b0d7ce..20103b5191 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3825,6 +3825,7 @@ completing-read-default
                 (1+ (cdr initial-input)))))
 
   (let* ((minibuffer-completion-table collection)
+         (minibuffer-local-completion-table t)
          (minibuffer-completion-predicate predicate)
          ;; FIXME: Remove/rename this var, see the next one.
          (minibuffer-completion-confirm (unless (eq require-match t)
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..28b42fa398 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2791,6 +2791,19 @@ read-passwd
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
+(defmacro read-from-minibuffer (&rest body)
+  "Read a string from the minibuffer with `internal-read-from-minibuffer'.
+See `internal-read-from-minibuffer' for a description of the arguments.
+This macro exists only in Emacs 28, for the transition period during which
+the default value of `minibuffer-local-completion-table' is nil, and will be
+removed in Emacs 29.  Likewise, `internal-read-from-minibuffer' will be
+removed in Emacs 29, please do not use it directly."
+  `(if minibuffer-local-completion-table
+       (let ((minibuffer-local-completion-table minibuffer-completion-table))
+         (let ((minibuffer-completion-table nil))
+           (internal-read-from-minibuffer ,@body)))
+     (internal-read-from-minibuffer ,@body)))
+
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
diff --git a/src/fns.c b/src/fns.c
index 1758148ff2..db6679a847 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2985,9 +2985,9 @@ DEFUN ("yes-or-no-p", Fyes_or_no_p, Syes_or_no_p, 1, 1, 0,
 
   while (1)
     {
-      ans = Fdowncase (Fread_from_minibuffer (prompt, Qnil, Qnil, Qnil,
-					      Qyes_or_no_p_history, Qnil,
-					      Qnil));
+      ans = Fdowncase (Finternal_read_from_minibuffer (prompt, Qnil, Qnil, Qnil,
+						       Qyes_or_no_p_history, Qnil,
+						       Qnil));
       if (SCHARS (ans) == 3 && !strcmp (SSDATA (ans), "yes"))
 	return unbind_to (count, Qt);
       if (SCHARS (ans) == 2 && !strcmp (SSDATA (ans), "no"))
diff --git a/src/minibuf.c b/src/minibuf.c
index c9831fd50f..6d4c2848f6 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -862,6 +862,12 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
     call1 (Qactivate_input_method, input_method);
 
+  if (! EQ (Vminibuffer_local_completion_table, Qnil)) {
+    Fmake_local_variable (Qminibuffer_completion_table);
+    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
+    specbind (Qminibuffer_local_completion_table, Qnil);
+  }
+
   run_hook (Qminibuffer_setup_hook);
 
   /* Don't allow the user to undo past this point.  */
@@ -1223,9 +1229,16 @@ barf_if_interaction_inhibited (void)
     xsignal0 (Qinhibited_interaction);
 }
 
-DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
-       Sread_from_minibuffer, 1, 7, 0,
+DEFUN ("internal-read-from-minibuffer", Finternal_read_from_minibuffer,
+       Sinternal_read_from_minibuffer, 1, 7, 0,
        doc: /* Read a string from the minibuffer, prompting with string PROMPT.
+
+Note: Do not use this function directly, use `read-from-minibuffer' instead,
+whith the arguments described below.  The `read-from-minibuffer' macro exists
+only in Emacs 28 for the transition period during which the default value of
+`minibuffer-local-completion-table' is nil, it will be removed in Emacs 29,
+and `internal-read-from-minibuffer' will become `read-from-minibuffer' again.
+
 The optional second arg INITIAL-CONTENTS is an obsolete alternative to
   DEFAULT-VALUE.  It normally should be nil in new code, except when
   HIST is a cons.  It is discussed in more detail below.
@@ -1344,9 +1357,9 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
      FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
   specbind (Qminibuffer_completion_table, Qnil);
 
-  val = Fread_from_minibuffer (prompt, initial_input, Qnil,
-			       Qnil, history, default_value,
-			       inherit_input_method);
+  val = Finternal_read_from_minibuffer (prompt, initial_input, Qnil,
+				        Qnil, history, default_value,
+				        inherit_input_method);
   if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
     val = CONSP (default_value) ? XCAR (default_value) : default_value;
   return unbind_to (count, val);
@@ -2297,6 +2310,7 @@ syms_of_minibuf (void)
   Fset (Qminibuffer_default, Qnil);
 
   DEFSYM (Qminibuffer_completion_table, "minibuffer-completion-table");
+  DEFSYM (Qminibuffer_local_completion_table, "minibuffer-local-completion-table");
 
   staticpro (&last_minibuf_string);
 
@@ -2409,6 +2423,17 @@ syms_of_minibuf (void)
   lambda -- return t if STRING is a valid completion as it stands.  */);
   Vminibuffer_completion_table = Qnil;
 
+  DEFVAR_LISP ("minibuffer-local-completion-table", Vminibuffer_local_completion_table,
+	       doc: /* Whether `minibuffer-completion-table' should become buffer-local.
+The default is nil for Emacs 28.  Setting this variable in Emacs 29 will have no effect;
+the value t will be assumed.
+When t, `minibuffer-completion-table' becomes buffer-local upon entering the minibuffer,
+and is nil in recursive invocations of the minibuffer, unless it has been let-bound to
+a value.
+When nil, `minibuffer-completion-table' is shared between the recursive invocations of
+the minibuffer, unless it has been let-bound to another value.  */);
+  Vminibuffer_local_completion_table = Qnil;
+
   DEFVAR_LISP ("minibuffer-completion-predicate", Vminibuffer_completion_predicate,
 	       doc: /* Within call to `completing-read', this holds the PREDICATE argument.  */);
   Vminibuffer_completion_predicate = Qnil;
@@ -2496,7 +2521,7 @@ syms_of_minibuf (void)
 
   defsubr (&Sactive_minibuffer_window);
   defsubr (&Sset_minibuffer_window);
-  defsubr (&Sread_from_minibuffer);
+  defsubr (&Sinternal_read_from_minibuffer);
   defsubr (&Sread_string);
   defsubr (&Sread_command);
   defsubr (&Sread_variable);
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

>> but then later you posted a different patch.  Anyway FWIW here is
>> a safer workable workaround that implements your first suggestion
>> and sets a new explicit buffer-local variable in completing minibuffers,
>> so modes that need to distinguish such minibuffers could check it.
>
> I combined your idea with that of Gregory for the patch below.
> It's far from perfect, but I think it strikes a good balance of
> simplicity, preserving compatibility, and moving in the right direction.
>
> WDYT?

I tested your patch and discovered that it fails when two nested
minibuffers both use completion, e.g. C-x C-f C-h v raises the error:

   Error in post-command-hook (icomplete-post-command-hook): (wrong-type-argument symbolp

The value of minibuffer-completion-table is 'read-file-name-internal'
in the C-x C-f minibuffer that is correct.  But the same value is also
in the nested C-h v minibuffer instead of the correct help--symbol-completion-table.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> I tested your patch and discovered that it fails when two nested
> minibuffers both use completion, e.g. C-x C-f C-h v raises the error:
>
>    Error in post-command-hook (icomplete-post-command-hook): (wrong-type-argument symbolp

Indeed, it's nasty:

- In the first step, we
  - let-bind (globally) m-c-table to read-file-name-internal
  - go to minibuf-1
  - make it buffer-local in minibuf-1
  - everything's dandy
- In the second step, things get ugly
  - we're in minibuf-1
  - we let-bind m-c-table to help--symbol-completion-table which
    only works buffer-locally in minibuf-1 (thus temporarily overriding
    the value set just before).
  - we go to minibuf-2 which still has no buffer-local setting so it
    sees the global value that's still read-file-name-internal (because
    of the very first part of the first step)
  - we make it buffer-local but to this wrong value

So in the end we get a wrong m-c-table in both buffers (the values are reversed!).


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> diff --git a/src/minibuf.c b/src/minibuf.c
> index c9831fd50f..6d4c2848f6 100644
> --- a/src/minibuf.c
> +++ b/src/minibuf.c
> @@ -862,6 +862,12 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>    if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
>      call1 (Qactivate_input_method, input_method);
>  
> +  if (! EQ (Vminibuffer_local_completion_table, Qnil)) {
> +    Fmake_local_variable (Qminibuffer_completion_table);
> +    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
> +    specbind (Qminibuffer_local_completion_table, Qnil);
> +  }
> +
>    run_hook (Qminibuffer_setup_hook);

I'm afraid this will suffer a similar problem to the one Juri
spotted in my code.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>> diff --git a/src/minibuf.c b/src/minibuf.c
>> index c9831fd50f..6d4c2848f6 100644
>> --- a/src/minibuf.c
>> +++ b/src/minibuf.c
>> @@ -862,6 +862,12 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
>>      call1 (Qactivate_input_method, input_method);
>>
>> +  if (! EQ (Vminibuffer_local_completion_table, Qnil)) {
>> +    Fmake_local_variable (Qminibuffer_completion_table);
>> +    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
>> +    specbind (Qminibuffer_local_completion_table, Qnil);
>> +  }
>> +
>>    run_hook (Qminibuffer_setup_hook);
>
> I'm afraid this will suffer a similar problem to the one Juri spotted in 
> my code.
>

I'm glad to tell you that it does not ;-)  I tested this thoroughly, of 
course it's possible that something is still wrong, but I tried many 
combinations and it seems to Just Work^TM.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> I tested your patch and discovered that it fails when two nested
>> minibuffers both use completion, e.g. C-x C-f C-h v raises the error:
>>
>>    Error in post-command-hook (icomplete-post-command-hook): (wrong-type-argument symbolp
>
> Indeed, it's nasty:
>
> - In the first step, we
>   - let-bind (globally) m-c-table to read-file-name-internal
>   - go to minibuf-1
>   - make it buffer-local in minibuf-1
>   - everything's dandy
> - In the second step, things get ugly
>   - we're in minibuf-1
>   - we let-bind m-c-table to help--symbol-completion-table which
>     only works buffer-locally in minibuf-1 (thus temporarily overriding
>     the value set just before).
>   - we go to minibuf-2 which still has no buffer-local setting so it
>     sees the global value that's still read-file-name-internal (because
>     of the very first part of the first step)
>   - we make it buffer-local but to this wrong value
>
> So in the end we get a wrong m-c-table in both buffers (the values are reversed!).

I'm more and more thinking that we should really bite the bullet and go
for a "really buffer-local" setup, such as in the patch below.
I'd be surprised if it doesn't introduce backward incompatibilities, but
at least it's "The Right Thing" to do.


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7da3c39e6b9..ce136b90449 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3884,13 +3884,7 @@ completing-read-default
                 ;; `read-from-minibuffer' uses 1-based index.
                 (1+ (cdr initial-input)))))
 
-  (let* ((minibuffer-completion-table collection)
-         (minibuffer-completion-predicate predicate)
-         ;; FIXME: Remove/rename this var, see the next one.
-         (minibuffer-completion-confirm (unless (eq require-match t)
-                                          require-match))
-         (minibuffer--require-match require-match)
-         (base-keymap (if require-match
+  (let* ((base-keymap (if require-match
                          minibuffer-local-must-match-map
                         minibuffer-local-completion-map))
          (keymap (if (memq minibuffer-completing-file-name '(nil lambda))
@@ -3903,8 +3897,17 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
-         (result (read-from-minibuffer prompt initial-input keymap
-                                       nil hist def inherit-input-method)))
+         (result
+          (minibuffer-with-setup-hook
+              (lambda ()
+                (setq-local minibuffer-completion-table collection)
+                (setq-local minibuffer-completion-predicate predicate)
+                ;; FIXME: Remove/rename this var, see the next one.
+                (setq-local minibuffer-completion-confirm
+                            (unless (eq require-match t) require-match))
+                (setq-local minibuffer--require-match require-match))
+            (read-from-minibuffer prompt initial-input keymap
+                                  nil hist def inherit-input-method))))
     (when (and (equal result "") def)
       (setq result (if (consp def) (car def) def)))
     result))

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> I'm more and more thinking that we should really bite the bullet and go 
> for a "really buffer-local" setup, such as in the patch below.
>
> I'd be surprised if it doesn't introduce backward incompatibilities, but 
> at least it's "The Right Thing" to do.
>

Well, that's what my patch does, too, except that it delays the backward 
incompatilibites during one Emacs major release...  (I did not include 
minibuffer-completion-predicate and minibuffer-completion-confirm because 
they haven't been discussed yet, but they are easy to add.)

bug#45474: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> And it was clear that I wasn't sure ("possibly
> misunderstanding") what you meant by that phrase.

Try the patch below,


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7da3c39e6b9..ce136b90449 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3884,13 +3884,7 @@ completing-read-default
                 ;; `read-from-minibuffer' uses 1-based index.
                 (1+ (cdr initial-input)))))
 
-  (let* ((minibuffer-completion-table collection)
-         (minibuffer-completion-predicate predicate)
-         ;; FIXME: Remove/rename this var, see the next one.
-         (minibuffer-completion-confirm (unless (eq require-match t)
-                                          require-match))
-         (minibuffer--require-match require-match)
-         (base-keymap (if require-match
+  (let* ((base-keymap (if require-match
                          minibuffer-local-must-match-map
                         minibuffer-local-completion-map))
          (keymap (if (memq minibuffer-completing-file-name '(nil lambda))
@@ -3903,8 +3897,17 @@ completing-read-default
                     ;; in minibuffer-local-filename-completion-map can
                     ;; override bindings in base-keymap.
                     base-keymap)))
-         (result (read-from-minibuffer prompt initial-input keymap
-                                       nil hist def inherit-input-method)))
+         (result
+          (minibuffer-with-setup-hook
+              (lambda ()
+                (setq-local minibuffer-completion-table collection)
+                (setq-local minibuffer-completion-predicate predicate)
+                ;; FIXME: Remove/rename this var, see the next one.
+                (setq-local minibuffer-completion-confirm
+                            (unless (eq require-match t) require-match))
+                (setq-local minibuffer--require-match require-match))
+            (read-from-minibuffer prompt initial-input keymap
+                                  nil hist def inherit-input-method))))
     (when (and (equal result "") def)
       (setq result (if (consp def) (car def) def)))
     result))

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>> I'm more and more thinking that we should really bite the bullet and go 
>> for a "really buffer-local" setup, such as in the patch below.
>> 
>> I'd be surprised if it doesn't introduce backward incompatibilities, 
>> but at least it's "The Right Thing" to do.
>
> Well, that's what my patch does, too, except that it delays the backward 
> incompatilibites during one Emacs major release...  (I did not include 
> minibuffer-completion-predicate and minibuffer-completion-confirm 
> because they haven't been discussed yet, but they are easy to add.)
>

And (to show that they are indeed easy to add) here is the updated patch, 
which also takes care of minibuffer-completion-predicate and 
minibuffer-completion-confirm.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-a-completion-backend-in-.patch, Size: 8358 bytes --]

From 57ac0d5c0408bb835eb78f7497a425d8390a7460 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 22 Apr 2021 15:12:56 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

---
 lisp/minibuffer.el |  1 +
 lisp/subr.el       | 17 +++++++++++++
 src/fns.c          |  6 ++---
 src/minibuf.c      | 59 +++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7da3c39e6b..1796b97990 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3885,6 +3885,7 @@ completing-read-default
                 (1+ (cdr initial-input)))))
 
   (let* ((minibuffer-completion-table collection)
+         (minibuffer-local-completion t)
          (minibuffer-completion-predicate predicate)
          ;; FIXME: Remove/rename this var, see the next one.
          (minibuffer-completion-confirm (unless (eq require-match t)
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..2414f60940 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2791,6 +2791,23 @@ read-passwd
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
+(defmacro read-from-minibuffer (&rest body)
+  "Read a string from the minibuffer with `internal-read-from-minibuffer'.
+See `internal-read-from-minibuffer' for a description of the arguments.
+This macro exists only in Emacs 28, for the transition period during which
+the default value of `minibuffer-local-completion' is nil, and will be
+removed in Emacs 29.  Likewise, `internal-read-from-minibuffer' will be
+removed in Emacs 29, please do not use it directly."
+  `(if minibuffer-local-completion
+       (let ((minibuffer-local-completion-table minibuffer-completion-table)
+             (minibuffer-local-completion-predicate minibuffer-completion-predicate)
+             (minibuffer-local-completion-confirm minibuffer-completion-confirm))
+         (let ((minibuffer-completion-table nil)
+               (minibuffer-completion-predicate nil)
+               (minibuffer-completion-confirm nil))
+           (internal-read-from-minibuffer ,@body)))
+     (internal-read-from-minibuffer ,@body)))
+
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
diff --git a/src/fns.c b/src/fns.c
index 1758148ff2..db6679a847 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2985,9 +2985,9 @@ DEFUN ("yes-or-no-p", Fyes_or_no_p, Syes_or_no_p, 1, 1, 0,
 
   while (1)
     {
-      ans = Fdowncase (Fread_from_minibuffer (prompt, Qnil, Qnil, Qnil,
-					      Qyes_or_no_p_history, Qnil,
-					      Qnil));
+      ans = Fdowncase (Finternal_read_from_minibuffer (prompt, Qnil, Qnil, Qnil,
+						       Qyes_or_no_p_history, Qnil,
+						       Qnil));
       if (SCHARS (ans) == 3 && !strcmp (SSDATA (ans), "yes"))
 	return unbind_to (count, Qt);
       if (SCHARS (ans) == 2 && !strcmp (SSDATA (ans), "no"))
diff --git a/src/minibuf.c b/src/minibuf.c
index 1a637c86ad..fd780bd5c1 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -865,6 +865,16 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
     call1 (Qactivate_input_method, input_method);
 
+  if (! EQ (Vminibuffer_local_completion, Qnil)) {
+    Fmake_local_variable (Qminibuffer_completion_table);
+    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
+    Fmake_local_variable (Qminibuffer_completion_predicate);
+    Fset (Qminibuffer_completion_predicate, Vminibuffer_local_completion_predicate);
+    Fmake_local_variable (Qminibuffer_completion_confirm);
+    Fset (Qminibuffer_completion_confirm, Vminibuffer_local_completion_confirm);
+    specbind (Qminibuffer_local_completion, Qnil);
+  }
+
   run_hook (Qminibuffer_setup_hook);
 
   /* Don't allow the user to undo past this point.  */
@@ -1231,9 +1241,16 @@ barf_if_interaction_inhibited (void)
     xsignal0 (Qinhibited_interaction);
 }
 
-DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
-       Sread_from_minibuffer, 1, 7, 0,
+DEFUN ("internal-read-from-minibuffer", Finternal_read_from_minibuffer,
+       Sinternal_read_from_minibuffer, 1, 7, 0,
        doc: /* Read a string from the minibuffer, prompting with string PROMPT.
+
+Note: Do not use this function directly, use `read-from-minibuffer' instead,
+whith the arguments described below.  The `read-from-minibuffer' macro exists
+only in Emacs 28 for the transition period during which the default value of
+`minibuffer-local-completion' is nil, it will be removed in Emacs 29, and
+`internal-read-from-minibuffer' will become `read-from-minibuffer' again.
+
 The optional second arg INITIAL-CONTENTS is an obsolete alternative to
   DEFAULT-VALUE.  It normally should be nil in new code, except when
   HIST is a cons.  It is discussed in more detail below.
@@ -1352,9 +1369,9 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
      FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
   specbind (Qminibuffer_completion_table, Qnil);
 
-  val = Fread_from_minibuffer (prompt, initial_input, Qnil,
-			       Qnil, history, default_value,
-			       inherit_input_method);
+  val = Finternal_read_from_minibuffer (prompt, initial_input, Qnil,
+				        Qnil, history, default_value,
+				        inherit_input_method);
   if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
     val = CONSP (default_value) ? XCAR (default_value) : default_value;
   return unbind_to (count, val);
@@ -2282,6 +2299,9 @@ syms_of_minibuf (void)
   Fset (Qminibuffer_default, Qnil);
 
   DEFSYM (Qminibuffer_completion_table, "minibuffer-completion-table");
+  DEFSYM (Qminibuffer_completion_predicate, "minibuffer-completion-predicate");
+  DEFSYM (Qminibuffer_completion_confirm, "minibuffer-completion-confirm");
+  DEFSYM (Qminibuffer_local_completion, "minibuffer-local-completion");
 
   staticpro (&last_minibuf_string);
 
@@ -2415,6 +2435,33 @@ syms_of_minibuf (void)
  completion commands listed in `minibuffer-confirm-exit-commands'.  */);
   Vminibuffer_completion_confirm = Qnil;
 
+  DEFVAR_LISP ("minibuffer-local-completion", Vminibuffer_local_completion,
+	       doc: /* Whether minibuffer completion elements should become buffer-local.
+The default is nil for Emacs 28.  Setting this variable in Emacs 29 will have no effect;
+the value t will be assumed.
+When t, `minibuffer-completion-table', `minibuffer-completion-predicate' and
+`minibuffer-completion-confirm' become buffer-local upon entering the minibuffer,
+and are nil in recursive invocations of the minibuffer, unless they have been let-bound
+to a value.
+When nil, their values is shared between the recursive invocations of the minibuffer,
+unless they have been let-bound to another value.  */);
+  Vminibuffer_local_completion = Qnil;
+
+  DEFVAR_LISP ("minibuffer-local-completion-table", Vminibuffer_local_completion_table,
+	       doc: /* The local completion table used in the minibuffer.
+See `minibuffer-local-completion'.  */);
+  Vminibuffer_local_completion_table = Qnil;
+
+  DEFVAR_LISP ("minibuffer-local-completion-predicate", Vminibuffer_local_completion_predicate,
+	       doc: /* The local completion predicate used in the minibuffer.
+See `minibuffer-local-completion'.  */);
+  Vminibuffer_local_completion_predicate = Qnil;
+
+  DEFVAR_LISP ("minibuffer-local-completion-confirm", Vminibuffer_local_completion_confirm,
+	       doc: /* The local completion confirm used in the minibuffer.
+See `minibuffer-local-completion'.  */);
+  Vminibuffer_local_completion_confirm = Qnil;
+
   DEFVAR_LISP ("minibuffer-completing-file-name",
 	       Vminibuffer_completing_file_name,
 	       doc: /* Non-nil means completing file names.  */);
@@ -2487,7 +2534,7 @@ syms_of_minibuf (void)
 
   defsubr (&Sactive_minibuffer_window);
   defsubr (&Sset_minibuffer_window);
-  defsubr (&Sread_from_minibuffer);
+  defsubr (&Sinternal_read_from_minibuffer);
   defsubr (&Sread_string);
   defsubr (&Sread_command);
   defsubr (&Sread_variable);
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

I have the impression that I suffer from NIH syndrome with respect to
your patch, so I'll refrain from commenting on it by and large, except
for this part:

> diff --git a/src/minibuf.c b/src/minibuf.c
> index 1a637c86ad..fd780bd5c1 100644
> --- a/src/minibuf.c
> +++ b/src/minibuf.c
> @@ -865,6 +865,16 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>    if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
>      call1 (Qactivate_input_method, input_method);
>  
> +  if (! EQ (Vminibuffer_local_completion, Qnil)) {
> +    Fmake_local_variable (Qminibuffer_completion_table);
> +    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
> +    Fmake_local_variable (Qminibuffer_completion_predicate);
> +    Fset (Qminibuffer_completion_predicate, Vminibuffer_local_completion_predicate);
> +    Fmake_local_variable (Qminibuffer_completion_confirm);
> +    Fset (Qminibuffer_completion_confirm, Vminibuffer_local_completion_confirm);
> +    specbind (Qminibuffer_local_completion, Qnil);
> +  }

I really don't like adding knowledge of those variables to this
function, which so far is completely oblivious to whether the
minibuffer is used with completion or not.

>    run_hook (Qminibuffer_setup_hook);

As in my patch, you could use this hook to do the completion-specific part
of the setup.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> I have the impression that I suffer from NIH syndrome with respect to 
> your patch, so I'll refrain from commenting on it by and large
>

I'm not sure I understand what you mean here.

It uses a nice trick (intermediate variables) to provide a 
backward-compatible behavior for read-from-minibuffer that would last for 
one major Emacs release (to avoid breaking external packages), while 
providing the needed feature (buffer-local completion tables) for code 
those that want to use it.  Doesn't that fit the bill?

>> diff --git a/src/minibuf.c b/src/minibuf.c
>> index 1a637c86ad..fd780bd5c1 100644
>> --- a/src/minibuf.c
>> +++ b/src/minibuf.c
>> @@ -865,6 +865,16 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
>>      call1 (Qactivate_input_method, input_method);
>>
>> +  if (! EQ (Vminibuffer_local_completion, Qnil)) {
>> +    Fmake_local_variable (Qminibuffer_completion_table);
>> +    Fset (Qminibuffer_completion_table, Vminibuffer_local_completion_table);
>> +    Fmake_local_variable (Qminibuffer_completion_predicate);
>> +    Fset (Qminibuffer_completion_predicate, Vminibuffer_local_completion_predicate);
>> +    Fmake_local_variable (Qminibuffer_completion_confirm);
>> +    Fset (Qminibuffer_completion_confirm, Vminibuffer_local_completion_confirm);
>> +    specbind (Qminibuffer_local_completion, Qnil);
>> +  }
>
> I really don't like adding knowledge of those variables to this 
> function, which so far is completely oblivious to whether the minibuffer 
> is used with completion or not.
>

Hmmm...  That's not true, see the occurrences of 
minibuffer_completing_file_name.  And that would be temporary anyway, in 
Emacs 29 this code could be removed from read_minibuf() I think.

>>    run_hook (Qminibuffer_setup_hook);
>
> As in my patch, you could use this hook to do the completion-specific 
> part of the setup.
>

Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally 
broken and messy"...

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>>>    run_hook (Qminibuffer_setup_hook);
>> 
>> As in my patch, you could use this hook to do the completion-specific 
>> part of the setup.
>
> Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally 
> broken and messy"...
>

I forgot to add that if the idea is to change the semantics of 
read-from-minibuffer in the long-term, this must happen inside 
read-from-minibuffer, not with a minibuffer-with-setup-hook around 
read-from-minibuffer.  What would be possible here (I think) is to move 
this piece of code in a minibuffer-with-setup-hook inside the 
read-from-minibuffer macro.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>>>>    run_hook (Qminibuffer_setup_hook);
>>> 
>>> As in my patch, you could use this hook to do the completion-specific 
>>> part of the setup.
>> 
>> Indeed, but you said that 'minibuffer-with-setup-hook' is 
>> "fundamentally broken and messy"...
>
> I forgot to add that if the idea is to change the semantics of 
> read-from-minibuffer in the long term, this must happen inside 
> read-from-minibuffer, not with a minibuffer-with-setup-hook around 
> read-from-minibuffer.  What would be possible here (I think) is to move 
> this piece of code in a minibuffer-with-setup-hook inside the 
> read-from-minibuffer macro.
>

And here is the patch which does this, in case you prefer it.  AFAICS it 
is functionally equivalent to the other one.

[-- Attachment #2: Type: text/x-diff, Size: 6301 bytes --]

From edd46c1a2df6dea2154eb893da51eca1abd2da83 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 22 Apr 2021 20:51:52 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

---
 lisp/minibuffer.el |  3 ++-
 lisp/subr.el       | 34 ++++++++++++++++++++++++++++++++++
 src/fns.c          |  6 +++---
 src/minibuf.c      | 20 ++++++++++++++------
 4 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7da3c39e6b..379dadef9d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3884,7 +3884,8 @@ completing-read-default
                 ;; `read-from-minibuffer' uses 1-based index.
                 (1+ (cdr initial-input)))))
 
-  (let* ((minibuffer-completion-table collection)
+  (let* ((minibuffer-local-completion t)
+         (minibuffer-completion-table collection)
          (minibuffer-completion-predicate predicate)
          ;; FIXME: Remove/rename this var, see the next one.
          (minibuffer-completion-confirm (unless (eq require-match t)
diff --git a/lisp/subr.el b/lisp/subr.el
index c2be26a15f..972422f343 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2791,6 +2791,40 @@ read-passwd
               ;; And of course, don't keep the sensitive data around.
               (erase-buffer))))))))
 
+(defvar minibuffer-local-completion nil
+  "Whether minibuffer completion elements should become buffer-local.
+The default is nil for Emacs 28.  Setting this variable in Emacs 29 will
+have no effect; the value t will be assumed.
+When t, `minibuffer-completion-table', `minibuffer-completion-predicate'
+and `minibuffer-completion-confirm' become buffer-local upon entering the
+minibuffer, and are nil in recursive invocations of the minibuffer, unless
+they have been let-bound to a value.
+When nil, their values is shared between the recursive invocations of the
+minibuffer, unless they have been let-bound to another value.")
+
+(defmacro read-from-minibuffer (&rest body)
+  "Read a string from the minibuffer with `internal-read-from-minibuffer'.
+See `internal-read-from-minibuffer' for a description of the arguments.
+This macro exists only in Emacs 28, for the transition period during which
+the default value of `minibuffer-local-completion' is nil, and will be
+removed in Emacs 29.  Likewise, `internal-read-from-minibuffer' will be
+removed in Emacs 29, please do not use it directly."
+  `(if minibuffer-local-completion
+       (let ((minibuffer-local-c-t minibuffer-completion-table)
+             (minibuffer-local-c-p minibuffer-completion-predicate)
+             (minibuffer-local-c-c minibuffer-completion-confirm))
+         (let ((minibuffer-completion-table nil)
+               (minibuffer-completion-predicate nil)
+               (minibuffer-completion-confirm nil))
+           (minibuffer-with-setup-hook
+               (lambda ()
+                 (setq-local minibuffer-completion-table minibuffer-local-c-t
+                             minibuffer-completion-predicate minibuffer-local-c-p
+                             minibuffer-completion-confirm minibuffer-local-c-c)
+                 (setq minibuffer-local-completion nil))
+             (internal-read-from-minibuffer ,@body))))
+     (internal-read-from-minibuffer ,@body)))
+
 (defvar read-number-history nil
   "The default history for the `read-number' function.")
 
diff --git a/src/fns.c b/src/fns.c
index 1758148ff2..db6679a847 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2985,9 +2985,9 @@ DEFUN ("yes-or-no-p", Fyes_or_no_p, Syes_or_no_p, 1, 1, 0,
 
   while (1)
     {
-      ans = Fdowncase (Fread_from_minibuffer (prompt, Qnil, Qnil, Qnil,
-					      Qyes_or_no_p_history, Qnil,
-					      Qnil));
+      ans = Fdowncase (Finternal_read_from_minibuffer (prompt, Qnil, Qnil, Qnil,
+						       Qyes_or_no_p_history, Qnil,
+						       Qnil));
       if (SCHARS (ans) == 3 && !strcmp (SSDATA (ans), "yes"))
 	return unbind_to (count, Qt);
       if (SCHARS (ans) == 2 && !strcmp (SSDATA (ans), "no"))
diff --git a/src/minibuf.c b/src/minibuf.c
index 1a637c86ad..90f329ddb2 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1231,9 +1231,17 @@ barf_if_interaction_inhibited (void)
     xsignal0 (Qinhibited_interaction);
 }
 
-DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
-       Sread_from_minibuffer, 1, 7, 0,
+DEFUN ("internal-read-from-minibuffer", Finternal_read_from_minibuffer,
+       Sinternal_read_from_minibuffer, 1, 7, 0,
        doc: /* Read a string from the minibuffer, prompting with string PROMPT.
+
+Warning: Do not use this function directly, use `read-from-minibuffer'
+instead, with the arguments described below.  The `read-from-minibuffer'
+macro exists only in Emacs 28 for the transition period during which the
+default value of `minibuffer-local-completion' is nil, it will be removed
+in Emacs 29, and `internal--read-from-minibuffer' will become
+`read-from-minibuffer' again.
+
 The optional second arg INITIAL-CONTENTS is an obsolete alternative to
   DEFAULT-VALUE.  It normally should be nil in new code, except when
   HIST is a cons.  It is discussed in more detail below.
@@ -1352,9 +1360,9 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
      FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
   specbind (Qminibuffer_completion_table, Qnil);
 
-  val = Fread_from_minibuffer (prompt, initial_input, Qnil,
-			       Qnil, history, default_value,
-			       inherit_input_method);
+  val = Finternal_read_from_minibuffer (prompt, initial_input, Qnil,
+				        Qnil, history, default_value,
+				        inherit_input_method);
   if (STRINGP (val) && SCHARS (val) == 0 && ! NILP (default_value))
     val = CONSP (default_value) ? XCAR (default_value) : default_value;
   return unbind_to (count, val);
@@ -2487,7 +2495,7 @@ syms_of_minibuf (void)
 
   defsubr (&Sactive_minibuffer_window);
   defsubr (&Sset_minibuffer_window);
-  defsubr (&Sread_from_minibuffer);
+  defsubr (&Sinternal_read_from_minibuffer);
   defsubr (&Sread_string);
   defsubr (&Sread_command);
   defsubr (&Sread_variable);
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

> I'm more and more thinking that we should really bite the bullet and go
> for a "really buffer-local" setup, such as in the patch below.
> I'd be surprised if it doesn't introduce backward incompatibilities, but
> at least it's "The Right Thing" to do.

I've tested your new patch with tests that Gregory posted
in an earlier message that cover all cases of nested
completion and non-completion minibuffers:

  M-:       C-x C-f
  C-x C-f   M-:
  C-x C-f   C-x 8 RET
  M-:       C-x f

with

  (icomplete-mode 1)
  (setq icomplete-show-matches-on-no-input t
        enable-recursive-minibuffers t)

and everything works fine without problems.  Of course, this doesn't mean
that some corner cases still exist.  But I don't know a better way
to find out than to install it and wait for bug reports.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> I have the impression that I suffer from NIH syndrome with respect to your
>> patch, so I'll refrain from commenting on it by and large
> I'm not sure I understand what you mean here.

I mean that I dislike your proposal but can't quite say why, so
I suspect it's just a case of "Not Invented Here" syndrome.

> Hmmm...  That's not true, see the occurrences of
> minibuffer_completing_file_name.

Indeed, it's a hack trying to solve the exact same kind of problems
we're trying to solve.

>>>    run_hook (Qminibuffer_setup_hook);
>> As in my patch, you could use this hook to do the completion-specific part
>> of the setup.
> Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally
> broken and messy"...

All schemes using dynamic scoping to pass the information are similarly
broken&messy, so yes, I don't like using it, but at least it's not
specific to completion.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 22 Apr 2021 19:24:30 -0400
> Cc: Dario Gjorgjevski <dario.gjorgjevski@gmail.com>, 45474@debbugs.gnu.org,
>  Juri Linkov <juri@linkov.net>
> 
> > Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally
> > broken and messy"...
> 
> All schemes using dynamic scoping to pass the information are similarly
> broken&messy

I don't agree.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>>> I have the impression that I suffer from NIH syndrome with respect to 
>>> your patch, so I'll refrain from commenting on it by and large
>>
>> I'm not sure I understand what you mean here.
>
> I mean that I dislike your proposal but can't quite say why, so I 
> suspect it's just a case of "Not Invented Here" syndrome.
>

It seems to me that my proposal is better, and here's why.  The right 
thing to do in this case is not to install a local fix in 
completing-read-default, because completing-read-default is not where the 
root cause of the current problem lies.  The right thing to do is to 
change the semantics of read-from-minibuffer (while preserving backward 
compatibility for a limited amount of time), in such a way it receives 
some of its arguments through its environment.  The latter will in the 
medium term improve the situation for all users of read-from-minibuffer.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> > Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally
>> > broken and messy"...
>> All schemes using dynamic scoping to pass the information are similarly
>> broken&messy
> I don't agree.

I don't know what that means, here.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: gregory@heytings.org,  dario.gjorgjevski@gmail.com,
>   45474@debbugs.gnu.org,  juri@linkov.net
> Date: Fri, 23 Apr 2021 09:12:08 -0400
> 
> >> > Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally
> >> > broken and messy"...
> >> All schemes using dynamic scoping to pass the information are similarly
> >> broken&messy
> > I don't agree.
> 
> I don't know what that means, here.

It means I disagree that "all schemes using dynamic scoping to pass
the information are broken and messy".

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> It seems to me that my proposal is better, and here's why.  The right thing
> to do in this case is not to install a local fix in completing-read-default,
> because completing-read-default is not where the root cause of the current
> problem lies.

Hmm... that's odd: the problem has to do with values of
`minibuffer-completion-*` appearing where they shouldn't, and those
values are set by `completing-read-default`, so I think it's clearly
the culprit.

AFAIK The patch I'm proposing is (in the long term) doing The Right
Thing (tho not quite in The Right Way since it still uses
`minibuffer-with-setup-hook`, but that's an internal detail that can be
fixed in the future by changing `read-from-minibuffer` to offer some
other way to run code in the new minibuffer).

> The right thing to do is to change the semantics of
> read-from-minibuffer (while preserving backward compatibility for
> a limited amount of time), in such a way it receives some of its
> arguments through its environment.

The core problem is the fact that dynamic scoping leaks: the parameters
passed to `read-from-minibuffer` via dynamic scoping and up being passed
to all other uses of `read-from-minibuffer` which happen to take place
within the same dynamic extent.

I can't see how "The right thing to do" can be compatible with "receives
some of its arguments through its environment".
[ Note that I'm not saying that doing it is necessarily wrong for a
  short term fix, tho.  ]


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>> It seems to me that my proposal is better, and here's why.  The right 
>> thing to do in this case is not to install a local fix in 
>> completing-read-default, because completing-read-default is not where 
>> the root cause of the current problem lies.
>
> Hmm... that's odd: the problem has to do with values of 
> `minibuffer-completion-*` appearing where they shouldn't, and those 
> values are set by `completing-read-default`, so I think it's clearly the 
> culprit.
>

By 'completing-read-default' _and_ by other completion backends that set 
minibuffer-completion-* elements and call read-from-minibuffer.  Or am I 
misunderstanding something?

If not, it means that your patch will fix the problem for 
completing-read-default, but not for other completion backends, who will 
have to resort on a similar trick to get the same effect.  IMO it would be 
better to avoid that, and to give "read-from-minibuffer" the new semantics 
that it takes some arguments from the environment and that these arguments 
are not anymore visible by all other (recursive) uses of 
read-from-minibuffer.

>> The right thing to do is to change the semantics of 
>> read-from-minibuffer (while preserving backward compatibility for a 
>> limited amount of time), in such a way it receives some of its 
>> arguments through its environment.
>
> The core problem is the fact that dynamic scoping leaks: the parameters 
> passed to `read-from-minibuffer` via dynamic scoping and up being passed 
> to all other uses of `read-from-minibuffer` which happen to take place 
> within the same dynamic extent.
>

Not with the patch I'm proposing.  What it does is the following (in 
abbreviated form):

(let ((minibuffer-local-* minibuffer-completion-*))
    (let ((minibuffer-completion-* nil))
       (internal-read-from-minibuffer ...)))

Line 1 saves the parameters in temporary variables, and line 2 resets the 
values of the parameters to nil, which means that they will not be visible 
anymore to all other uses of read-from-minibuffer within the same dynamic 
extent.  Isn't that a nice trick?

So you get all you want at once:

- receiving the arguments from the environment (thereby avoiding to add 
new explicit parameters)

- buffer-local values of the arguments on demand (thereby getting better 
semantics for callers that want it, in particular completing-read-default)

- be backward compatible the current semantics of read-from-minibufer 
(thereby avoiding to break compatibility for callers that did not adapt to 
the the new semantics)

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> >> > Indeed, but you said that 'minibuffer-with-setup-hook' is "fundamentally
>> >> > broken and messy"...
>> >> All schemes using dynamic scoping to pass the information are similarly
>> >> broken&messy
>> > I don't agree.
>> I don't know what that means, here.
> It means I disagree that "all schemes using dynamic scoping to pass
> the information are broken and messy".

;-)

That's the part I understood, indeed.  There are two aspects which make
it rather unclear to me:
- First, from where I stand, what I stated is not really a matter of
  opinion but a mere result of the underlying way the problem works.
  There's a admittedly some amount of "degree" that can depend on some
  of the details, which is why I said "similarly".
- Second I don't know what it implies in terms of your opinion w.r.t the
  various potential problems with:
  - the current way of passing the information (just plain let-binding).
  - the way proposed by Gregory (let-binding of minibuffer-complete-*
    followed by let-binding to other-named vars followed by setq-local
    of minibuffer-complete-*, where the dance is performed in
    `read-from-minibuffer`).
  - the way I suggested (minibuffer-with-setup-hook + setq-local, done
    in `completing-read-default`).
  It seems to suggest that you may not find them all "similarly
  broken&messy", but even that is not sure.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>>> It seems to me that my proposal is better, and here's why.  The right
>>> thing to do in this case is not to install a local fix in
>>> completing-read-default, because completing-read-default is not where the
>>> root cause of the current problem lies.
>>
>> Hmm... that's odd: the problem has to do with values of
>> `minibuffer-completion-*` appearing where they shouldn't, and those values
>> are set by `completing-read-default`, so I think it's clearly the culprit.
>
> By 'completing-read-default' _and_ by other completion backends that set
> minibuffer-completion-* elements and call read-from-minibuffer.  Or am
> I misunderstanding something?

AFAICT there are very few other pieces of code which let-bind (or set)
minibuffer-completion-*.  And my suggested patch should hopefully not
affect them too significantly.  Also I don't think we can fix this
problem without introducing corner-case incompatibilities and/or extra
code/changes in other frontends or backends.

> If not, it means that your patch will fix the problem for
> completing-read-default, but not for other completion backends, who will
> have to resort on a similar trick to get the same effect.

I think they'd need to make similar changes to fix the problem under
discussion in this longish thread, but they can keep using their old way
of working and the consequence will just be that they will keep
suffering from the old problem.

>> The core problem is the fact that dynamic scoping leaks: the parameters
>> passed to `read-from-minibuffer` via dynamic scoping and up being passed
>> to all other uses of `read-from-minibuffer` which happen to take place
>> within the same dynamic extent.
> Not with the patch I'm proposing.  What it does is the following (in
> abbreviated form):
>
> (let ((minibuffer-local-* minibuffer-completion-*))
>    (let ((minibuffer-completion-* nil))
>       (internal-read-from-minibuffer ...)))

Not quite.  The actual code is more like:

    (let ((minibuffer-local-* minibuffer-completion-*))
       [SOMETHING1]
       (let ((minibuffer-completion-* nil))
          (internal-read-from-minibuffer ...))
       [SOMETHING2])

and those two [SOMETHINGn] still leak.

[ Another problem is that this approach breaks when you have
  simultaneous active minibuffers, and the inner minibuffer is triggered
  from the outer one (e.g. `C-x C-f` following by `C-h o`) since the
  let-bindings above will (when run for the `C-h o`) temporarily
  override the completion information of the outer minibuffer.
  This is not too serious in the sense that it's no worse than what we
  have now, tho.  ]

> Line 1 saves the parameters in temporary variables, and line 2 resets the
> values of the parameters to nil, which means that they will not be visible
> anymore to all other uses of read-from-minibuffer within the same dynamic
> extent.  Isn't that a nice trick?
>
> So you get all you want at once:
>
> - receiving the arguments from the environment (thereby avoiding to add new
>  explicit parameters)
>
> - buffer-local values of the arguments on demand (thereby getting better
>    semantics for callers that want it, in particular
>   completing-read-default)
>
> - be backward compatible the current semantics of read-from-minibufer
>    (thereby avoiding to break compatibility for callers that did not adapt
>    to the the new semantics)

You share the main downside of my proposal:
`minibuffer-local-*` are now only non-nil buffer-locally in
the minibuffers.

I mean it's good because it's what we want in the long term, but it's
bad because it's not backward compatible and there's probably some code
out there which will need to be adjusted to work with this.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>> I'm more and more thinking that we should really bite the bullet and go
>> for a "really buffer-local" setup, such as in the patch below.
>> I'd be surprised if it doesn't introduce backward incompatibilities, but
>> at least it's "The Right Thing" to do.
> I've tested your new patch with tests that Gregory posted
> in an earlier message that cover all cases of nested
> completion and non-completion minibuffers:

AFAICT my proposed patch should work fine with the builtin UI and with
icomplete, yes.  I expect ido-ubiquitous should also work just fine.

The only significant source of incompatibility I can foresee is for code
which uses `minibuffer-completion-*` from code running outside of the
minibuffer (such as in a buffer like *Completion*).

There's another source of incompatibility for functions that currently
operate like `completion-read-default` (i.e. let-bind
`minibuffer-completion-*` and then use that setting in another buffer,
such as a new minibuffer): these may fail to work when used from within
a completing minibuffer.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

Thanks for your detailed comments.

>> By 'completing-read-default' _and_ by other completion backends that 
>> set minibuffer-completion-* elements and call read-from-minibuffer. 
>> Or am I misunderstanding something?
>
> AFAICT there are very few other pieces of code which let-bind (or set) 
> minibuffer-completion-*.  And my suggested patch should hopefully not 
> affect them too significantly.  Also I don't think we can fix this 
> problem without introducing corner-case incompatibilities and/or extra 
> code/changes in other frontends or backends.
>

Well, the fact that there were a few other pieces of code which do that 
was (at least as I understood it) part of the initial problem.  And the 
purpose of the discussion (at least as I understood it) was to solve the 
current problem without breaking these few other pieces of code.

>> If not, it means that your patch will fix the problem for 
>> completing-read-default, but not for other completion backends, who 
>> will have to resort on a similar trick to get the same effect.
>
> I think they'd need to make similar changes to fix the problem under 
> discussion in this longish thread, but they can keep using their old way 
> of working and the consequence will just be that they will keep 
> suffering from the old problem.
>

With my patch all they have to do is to add (minibuffer-local-completion 
t) before calling read-from-minibuffer.

>> Not with the patch I'm proposing.  What it does is the following (in 
>> abbreviated form):
>>
>> (let ((minibuffer-local-* minibuffer-completion-*))
>>    (let ((minibuffer-completion-* nil))
>>       (internal-read-from-minibuffer ...)))
>
> Not quite.  The actual code is more like:
>
>    (let ((minibuffer-local-* minibuffer-completion-*))
>       [SOMETHING1]
>       (let ((minibuffer-completion-* nil))
>          (internal-read-from-minibuffer ...))
>       [SOMETHING2])
>
> and those two [SOMETHINGn] still leak.
>

I admit that you lost me here.  What are these [SOMETHINGn]'s, and why are 
they happening?  Wasn't the point to not leak the minibuffer-completion-* 
variables?

>
> [ Another problem is that this approach breaks when you have 
> simultaneous active minibuffers, and the inner minibuffer is triggered 
> from the outer one (e.g. `C-x C-f` following by `C-h o`) since the 
> let-bindings above will (when run for the `C-h o`) temporarily override 
> the completion information of the outer minibuffer. This is not too 
> serious in the sense that it's no worse than what we have now, tho.  ]
>

I won't comment on this, because I'm deeply saddened to see this 
happening.  Emacs has had a nice Lisp-like stack of minibuffers forever, 
what the purpose of this new thing is is beyond me.

>
> You share the main downside of my proposal: `minibuffer-local-*` are now 
> only non-nil buffer-locally in the minibuffers.
>
> I mean it's good because it's what we want in the long term, but it's 
> bad because it's not backward compatible and there's probably some code 
> out there which will need to be adjusted to work with this.
>

I have to admit again that you lost me here.  What do you mean?  With my 
patch, inside the minibuffer, the minibuffer-local-* variables aren't used 
anymore, the usual minibuffer-completion-* variables are used, and they 
are buffer-local, so the sole and only thing that a piece of code wishing 
to use the new semantics needs to do is to let-bind 
(minibuffer-local-completion t) around the read-from-minibuffer call.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Juri Linkov]

>>> If not, it means that your patch will fix the problem for
>>> completing-read-default, but not for other completion backends, who will
>>> have to resort on a similar trick to get the same effect.
>>
>> I think they'd need to make similar changes to fix the problem under
>> discussion in this longish thread, but they can keep using their old way
>> of working and the consequence will just be that they will keep suffering
>> from the old problem.
>
> With my patch all they have to do is to add (minibuffer-local-completion t)
> before calling read-from-minibuffer.

Since other completion backends need to add (minibuffer-local-completion t)
anyway, the safest and most backward-compatible solution is to set
this new variable buffer-local in completing-read-default with

          (minibuffer-with-setup-hook
              (lambda ()
                (setq-local minibuffer-local-completion t))
            (read-from-minibuffer prompt initial-input keymap
                                  nil hist def inherit-input-method))))

Then modes like icomplete interested in knowing whether the minibuffer
was created for completions, can check for the buffer-local value
of minibuffer-local-completion.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

> Well, the fact that there were a few other pieces of code which do that was
> (at least as I understood it) part of the initial problem.  And the purpose
> of the discussion (at least as I understood it) was to solve the current
> problem without breaking these few other pieces of code.

Indeed, and I think my patch should by and large leave them unaffected
(it neither magically fixes them nor breaks them).

>>> If not, it means that your patch will fix the problem for
>>> completing-read-default, but not for other completion backends, who will
>>> have to resort on a similar trick to get the same effect.
>>
>> I think they'd need to make similar changes to fix the problem under
>> discussion in this longish thread, but they can keep using their old way
>> of working and the consequence will just be that they will keep suffering
>> from the old problem.
> With my patch all they have to do is to add (minibuffer-local-completion t)
> before calling read-from-minibuffer.

I think whichever approach we choose, the fix is pretty simple.
IMO the only real gain we can try and aim for is to minimize the need for
change at all.

>>> Not with the patch I'm proposing.  What it does is the following (in
>>> abbreviated form):
>>>
>>> (let ((minibuffer-local-* minibuffer-completion-*))
>>>    (let ((minibuffer-completion-* nil))
>>>       (internal-read-from-minibuffer ...)))
>>
>> Not quite.  The actual code is more like:
>>
>>    (let ((minibuffer-local-* minibuffer-completion-*))
>>       [SOMETHING1]
>>       (let ((minibuffer-completion-* nil))
>>          (internal-read-from-minibuffer ...))
>>       [SOMETHING2])
>>
>> and those two [SOMETHINGn] still leak.
>
> I admit that you lost me here.  What are these [SOMETHINGn]'s, and why are
> they happening?

Because inevitably code can run in there, e.g. because the debugger gets
triggered in there or because the caller of `read-from-minibuffer` was not
careful to place the let-bindings of `minibuffer-completion-*` as close
as possible to `read-from-minibuffer`.

[ Side note: you can't define `read-from-minibuffer` as a macro because
  it is not compatible with the byte-code generated from code which
  called `read-from-minibuffer` as a function.  So your patch would
  need to be adjusted to keep `read-from-minibuffer` as a function.
  That opens up yet more opportuninities for those [SOMETHINGn], such
  as advice placed on `read-from-minibuffer`, ...  ]

BTW, these corner case problems are the same kind of corner case
problems which plague `minibuffer-with-setup-hook` as well, of course.

>> You share the main downside of my proposal: `minibuffer-local-*` are now
>> only non-nil buffer-locally in the minibuffers.
>>
>> I mean it's good because it's what we want in the long term, but it's bad
>> because it's not backward compatible and there's probably some code out
>> there which will need to be adjusted to work with this.
>
> I have to admit again that you lost me here.

No wonder: I wrote `minibuffer-local-*` when I meant `minibuffer-completion-*`.
Sorry 'bout that.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: gregory@heytings.org,  dario.gjorgjevski@gmail.com,
>   45474@debbugs.gnu.org,  juri@linkov.net
> Date: Fri, 23 Apr 2021 11:18:48 -0400
> 
> > It means I disagree that "all schemes using dynamic scoping to pass
> > the information are broken and messy".
> 
> ;-)
> 
> That's the part I understood, indeed.  There are two aspects which make
> it rather unclear to me:
> - First, from where I stand, what I stated is not really a matter of
>   opinion but a mere result of the underlying way the problem works.
>   There's a admittedly some amount of "degree" that can depend on some
>   of the details, which is why I said "similarly".
> - Second I don't know what it implies in terms of your opinion w.r.t the
>   various potential problems with:
>   - the current way of passing the information (just plain let-binding).
>   - the way proposed by Gregory (let-binding of minibuffer-complete-*
>     followed by let-binding to other-named vars followed by setq-local
>     of minibuffer-complete-*, where the dance is performed in
>     `read-from-minibuffer`).
>   - the way I suggested (minibuffer-with-setup-hook + setq-local, done
>     in `completing-read-default`).
>   It seems to suggest that you may not find them all "similarly
>   broken&messy", but even that is not sure.

Your original opinion sounded like a general objection to passing
information via let-binding of dynamic variables.  I wanted to voice
my disagreement with such a general POV.  I wasn't saying anything
about this particular use of dynamic binding.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>> Well, the fact that there were a few other pieces of code which do that 
>> was (at least as I understood it) part of the initial problem.  And the 
>> purpose of the discussion (at least as I understood it) was to solve 
>> the current problem without breaking these few other pieces of code.
>
> Indeed, and I think my patch should by and large leave them unaffected 
> (it neither magically fixes them nor breaks them).
>

Indeed, but... it doesn't help/incite them to move forward in the "right" 
direction, to finally have what has been on wishlist for quite a long 
time: to have buffer-local minibuffer-completion-* elements...

>> I admit that you lost me here.  What are these [SOMETHINGn]'s, and why 
>> are they happening?
>
> Because inevitably code can run in there, e.g. because the debugger gets 
> triggered in there or because the caller of `read-from-minibuffer` was 
> not careful to place the let-bindings of `minibuffer-completion-*` as 
> close as possible to `read-from-minibuffer`.
>

I see.  But when the let-bindings are in a macro the caller doesn't have 
to care with them, and they are indeed happening as close as possible to 
internal-read-from-minibuffer.  Unless they deliberately use 
internal-read-from-minibuffer and do some weird things, of course.

>
> [ Side note: you can't define `read-from-minibuffer` as a macro because 
> it is not compatible with the byte-code generated from code which called 
> `read-from-minibuffer` as a function.  So your patch would need to be 
> adjusted to keep `read-from-minibuffer` as a function. That opens up yet 
> more opportuninities for those [SOMETHINGn], such as advice placed on 
> `read-from-minibuffer`, ...  ]
>

I didn't know that ELC files had to be backward-compatible between major 
releases.  That being said, my preference would be to have the whole 
dancing happening at the C level (inside read_from_minibuffer and/or 
read_minibuf), which would solve that problem/these problems.

>>> You share the main downside of my proposal: `minibuffer-local-*` are 
>>> now only non-nil buffer-locally in the minibuffers.
>>>
>>> I mean it's good because it's what we want in the long term, but it's 
>>> bad because it's not backward compatible and there's probably some 
>>> code out there which will need to be adjusted to work with this.
>>
>> I have to admit again that you lost me here.
>
> No wonder: I wrote `minibuffer-local-*` when I meant 
> `minibuffer-completion-*`. Sorry 'bout that.
>

No worries ;-)  Now I see what you mean, and I do not see where you see a 
potential problem there: whether the minibuffer-completion-* elements 
become buffer-local depends on the minibuffer-local-completion variable. 
When it is nil (the default), they do not become buffer-local, and the 
behavior of read-from-minibuffer is the same as earlier.  This gives 
external package plenty of time to adapt their code to the future 
minibuffer-local-completion = t situation.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>>> Well, the fact that there were a few other pieces of code which do that
>>> was (at least as I understood it) part of the initial problem.  And the
>>> purpose of the discussion (at least as I understood it) was to solve the
>>> current problem without breaking these few other pieces of code.
>> Indeed, and I think my patch should by and large leave them unaffected (it
>> neither magically fixes them nor breaks them).
> Indeed, but... it doesn't help/incite them to move forward in the "right"
> direction, to finally have what has been on wishlist for quite a long time:
> to have buffer-local minibuffer-completion-* elements...

It helps by showing how to do it.  I haven't seen any proposal here
which helps further than that (except maybe for some proposals which
might break such code, thus inciting them to use a better approach ;-).

>>> I admit that you lost me here.  What are these [SOMETHINGn]'s, and why
>>> are they happening?
>> Because inevitably code can run in there, e.g. because the debugger gets
>> triggered in there or because the caller of `read-from-minibuffer` was not
>> careful to place the let-bindings of `minibuffer-completion-*` as close as
>> possible to `read-from-minibuffer`.
> I see.  But when the let-bindings are in a macro the caller doesn't have to
> care with them, and they are indeed happening as close as possible to
> internal-read-from-minibuffer.

AFAICT you're talking about the let-bindings of `minibuffer-local-*`
whereas the problematic let-bindings are those of
`minibuffer-completion-*` and those are outside of
`read-from-minibuffer`.

> I didn't know that ELC files had to be backward-compatible between major
> releases.

We don't guarantee such compatibility 100%, but we do our best, and it's
pretty easy to avoid the problem here, so turning `read-from-minibuffer`
into a macro would not qualify as "do our best", I think.

> That being said, my preference would be to have the whole dancing
> happening at the C level (inside read_from_minibuffer and/or read_minibuf),
> which would solve that problem/these problems.

The [SOMETHINGn] are still outside of `read-from-minibuffer` so the
implementation of `read-from-minibuffer` doesn't affect the
problem, really.

> No worries ;-)  Now I see what you mean, and I do not see where you see
> a potential problem there: whether the minibuffer-completion-* elements
> become buffer-local depends on the minibuffer-local-completion
> variable. When it is nil (the default), they do not become buffer-local, and
> the behavior of read-from-minibuffer is the same as earlier.  This gives
> external package plenty of time to adapt their code to the future
> minibuffer-local-completion = t situation.

No, I'm talking about external packages for example those working like
`icomplete-mode`: they don't change the code which sets
`minibuffer-completion-table`, they just look for the completion table
in `minibuffer-completion-table`.  Currently such packages can access
`minibuffer-completion-table` from any buffer.
With our patches this is not the case any more: they can only access it
from the minibuffer.

So far I haven't found such code, tho, so maybe the risk of breakage is
actually much less severe than I imagined.  In any case, this is risk is
the same for both patches.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>> Indeed, but... it doesn't help/incite them to move forward in the 
>> "right" direction, to finally have what has been on wishlist for quite 
>> a long time: to have buffer-local minibuffer-completion-* elements...
>
> It helps by showing how to do it.  I haven't seen any proposal here 
> which helps further than that (except maybe for some proposals which 
> might break such code, thus inciting them to use a better approach ;-).
>

I believe that creating an optional behavior that is easy to use, and 
stating that that behavior will become mandatory in the next major Emacs 
release, is a stronger incentive.

>> But when the let-bindings are in a macro the caller doesn't have to 
>> care with them, and they are indeed happening as close as possible to 
>> internal-read-from-minibuffer.
>
> AFAICT you're talking about the let-bindings of `minibuffer-local-*` 
> whereas the problematic let-bindings are those of 
> `minibuffer-completion-*` and those are outside of 
> `read-from-minibuffer`.
>

Aren't these problems orthogonal to the problem at hand?  It seems to me 
that this is not different from the traditional way of passing arguments 
to a function; of course something unexpected can happen when they are 
evaluated, before the function is entered, but that's something outside 
the responsibility of the function.

My aim here was to (help to) fix the nine years old "FIXME: 
`minibuffer-completion-table' should be buffer-local instead."  What would 
you do to fix it, in an ideal world in which backward compatibility is not 
an issue?

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

>>> Indeed, but... it doesn't help/incite them to move forward in the "right"
>>> direction, to finally have what has been on wishlist for quite a long
>>> time: to have buffer-local minibuffer-completion-* elements...
>> It helps by showing how to do it.  I haven't seen any proposal here which
>> helps further than that (except maybe for some proposals which might break
>> such code, thus inciting them to use a better approach ;-).
> I believe that creating an optional behavior that is easy to use, and
> stating that that behavior will become mandatory in the next major Emacs
> release, is a stronger incentive.

We could add some "break old code" option, indeed.
I think we can do that with pretty much all the proposed patches.
We could also add a "warn when detecting old code" option; again
I suspect that this can be done with most of the proposed patches so far.
So, it's rather orthogonal to the choice of which approach is preferable.

>>> But when the let-bindings are in a macro the caller doesn't have to care
>>> with them, and they are indeed happening as close as possible to
>>> internal-read-from-minibuffer.
>> AFAICT you're talking about the let-bindings of `minibuffer-local-*`
>> whereas the problematic let-bindings are those of
>> `minibuffer-completion-*` and those are outside of `read-from-minibuffer`.
> Aren't these problems orthogonal to the problem at hand?  It seems to me
> that this is not different from the traditional way of passing arguments to
> a function; of course something unexpected can happen when they are
> evaluated, before the function is entered, but that's something outside the
> responsibility of the function.

No, the problem is not "can someone change `minibuffer-completion-table`
before we get to its intended consumer" but "will the let-binding of
`minibuffer-completion-table` also affect code which was not the
intended consumer".
This problem does not exist with traditional/explicit argument passing.

> My aim here was to (help to) fix the nine years old "FIXME:
> `minibuffer-completion-table' should be buffer-local instead."  What would
> you do to fix it, in an ideal world in which backward compatibility is not
> an issue?

The patch I sent does just that.


        Stefan

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

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


>> Aren't these problems orthogonal to the problem at hand?  It seems to 
>> me that this is not different from the traditional way of passing 
>> arguments to a function; of course something unexpected can happen when 
>> they are evaluated, before the function is entered, but that's 
>> something outside the responsibility of the function.
>
> No, the problem is not "can someone change `minibuffer-completion-table` 
> before we get to its intended consumer" but "will the let-binding of 
> `minibuffer-completion-table` also affect code which was not the 
> intended consumer". This problem does not exist with 
> traditional/explicit argument passing.
>

Again, it seems to me that this problem is not directly related to the 
problem at hand.  If the caller of read-from-minibuffer is not careful 
enough and binds minibuffer-completion-* too far from the call, in such a 
way that other code is unexpectedly affected (or could unexpectedly be 
affected) by this binding, it's the responsibility of that caller to fix 
that problem.

Anyway, I attach a last version of my patch, in which all the dancing 
happens at the C level.  It is probably also possible to finally get rid 
of the 15 lines with the minibuffer-completing-file-name = Qlambda hack in 
read_minibuf().

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Make-it-possible-to-disable-a-completion-backend-in-.patch, Size: 5859 bytes --]

From 33dcdd6ea992e88614baa77b42c3e53bf9f6a08a Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 24 Apr 2021 08:43:45 +0000
Subject: [PATCH] Make it possible to disable a completion backend in recursive
 minibuffers

---
 lisp/minibuffer.el |  3 ++-
 src/minibuf.c      | 49 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7da3c39e6b..379dadef9d 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3884,7 +3884,8 @@ completing-read-default
                 ;; `read-from-minibuffer' uses 1-based index.
                 (1+ (cdr initial-input)))))
 
-  (let* ((minibuffer-completion-table collection)
+  (let* ((minibuffer-local-completion t)
+         (minibuffer-completion-table collection)
          (minibuffer-completion-predicate predicate)
          ;; FIXME: Remove/rename this var, see the next one.
          (minibuffer-completion-confirm (unless (eq require-match t)
diff --git a/src/minibuf.c b/src/minibuf.c
index c4482d7f1e..d0b804cdff 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -68,6 +68,9 @@ Copyright (C) 1985-1986, 1993-2021 Free Software Foundation, Inc.
 /* Width of current mini-buffer prompt.  Only set after display_line
    of the line that contains the prompt.  */
 
+static Lisp_Object minibuffer_completion_table, minibuffer_completion_predicate,
+  minibuffer_completion_confirm;
+
 static ptrdiff_t minibuf_prompt_width;
 
 static Lisp_Object nth_minibuffer (EMACS_INT depth);
@@ -862,6 +865,16 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
   if (STRINGP (input_method) && !NILP (Ffboundp (Qactivate_input_method)))
     call1 (Qactivate_input_method, input_method);
 
+  if (! EQ (Vminibuffer_local_completion, Qnil)) {
+    Fmake_local_variable (Qminibuffer_completion_table);
+    Fset (Qminibuffer_completion_table, minibuffer_completion_table);
+    Fmake_local_variable (Qminibuffer_completion_predicate);
+    Fset (Qminibuffer_completion_predicate, minibuffer_completion_predicate);
+    Fmake_local_variable (Qminibuffer_completion_confirm);
+    Fset (Qminibuffer_completion_confirm, minibuffer_completion_confirm);
+    Vminibuffer_local_completion = Qnil;
+  }
+
   run_hook (Qminibuffer_setup_hook);
 
   /* Don't allow the user to undo past this point.  */
@@ -1291,6 +1304,7 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
   (Lisp_Object prompt, Lisp_Object initial_contents, Lisp_Object keymap, Lisp_Object read, Lisp_Object hist, Lisp_Object default_value, Lisp_Object inherit_input_method)
 {
   Lisp_Object histvar, histpos, val;
+  ptrdiff_t count;
 
   barf_if_interaction_inhibited ();
 
@@ -1315,11 +1329,25 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
   if (NILP (histpos))
     XSETFASTINT (histpos, 0);
 
+  count = SPECPDL_INDEX ();
+
+  if (! EQ (Vminibuffer_local_completion, Qnil)) {
+    minibuffer_completion_table = Vminibuffer_completion_table;
+    minibuffer_completion_predicate = Vminibuffer_completion_predicate;
+    minibuffer_completion_confirm = Vminibuffer_completion_confirm;
+    specbind (Qminibuffer_completion_table, Qnil);
+    specbind (Qminibuffer_completion_predicate, Qnil);
+    specbind (Qminibuffer_completion_confirm, Qnil);
+  }
+
   val = read_minibuf (keymap, initial_contents, prompt,
 		      !NILP (read),
 		      histvar, histpos, default_value,
 		      minibuffer_allow_text_properties,
 		      !NILP (inherit_input_method));
+
+  unbind_to (count, Qnil);
+
   return val;
 }
 
@@ -1345,11 +1373,9 @@ DEFUN ("read-string", Fread_string, Sread_string, 1, 5, 0,
   Lisp_Object val;
   ptrdiff_t count = SPECPDL_INDEX ();
 
-  /* Just in case we're in a recursive minibuffer, make it clear that the
-     previous minibuffer's completion table does not apply to the new
-     minibuffer.
-     FIXME: `minibuffer-completion-table' should be buffer-local instead.  */
   specbind (Qminibuffer_completion_table, Qnil);
+  specbind (Qminibuffer_completion_predicate, Qnil);
+  specbind (Qminibuffer_completion_confirm, Qnil);
 
   val = Fread_from_minibuffer (prompt, initial_input, Qnil,
 			       Qnil, history, default_value,
@@ -2281,6 +2307,9 @@ syms_of_minibuf (void)
   Fset (Qminibuffer_default, Qnil);
 
   DEFSYM (Qminibuffer_completion_table, "minibuffer-completion-table");
+  DEFSYM (Qminibuffer_completion_predicate, "minibuffer-completion-predicate");
+  DEFSYM (Qminibuffer_completion_confirm, "minibuffer-completion-confirm");
+  DEFSYM (Qminibuffer_local_completion, "minibuffer-local-completion");
 
   staticpro (&last_minibuf_string);
 
@@ -2414,6 +2443,18 @@ syms_of_minibuf (void)
  completion commands listed in `minibuffer-confirm-exit-commands'.  */);
   Vminibuffer_completion_confirm = Qnil;
 
+  DEFVAR_LISP ("minibuffer-local-completion", Vminibuffer_local_completion,
+	       doc: /* Whether minibuffer completion elements should become buffer-local.
+The default is nil for Emacs 28.  Setting this variable in Emacs 29 will have no effect;
+the value t will be assumed.
+When t, `minibuffer-completion-table', `minibuffer-completion-predicate' and
+`minibuffer-completion-confirm' become buffer-local upon entering the minibuffer,
+and are nil in recursive invocations of the minibuffer, unless they have been let-bound
+to a value.
+When nil, their values is shared between the recursive invocations of the minibuffer,
+unless they have been let-bound to another value.  */);
+  Vminibuffer_local_completion = Qnil;
+
   DEFVAR_LISP ("minibuffer-completing-file-name",
 	       Vminibuffer_completing_file_name,
 	       doc: /* Non-nil means completing file names.  */);
-- 
2.30.2

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Stefan Monnier]

OK, seeing how noone reported problems with my patch, I pushed it to
`master`.  In case someone later reports a problem, we may "revert" to
your approach if its slightly better compatibility proves useful.


        Stefan


Gregory Heytings [2021-04-24 08:44:38] wrote:

>>> Aren't these problems orthogonal to the problem at hand?  It seems to me
>>> that this is not different from the traditional way of passing arguments
>>> to a function; of course something unexpected can happen when they are
>>> evaluated, before the function is entered, but that's something outside
>>> the responsibility of the function.
>>
>> No, the problem is not "can someone change `minibuffer-completion-table`
>> before we get to its intended consumer" but "will the let-binding of
>> `minibuffer-completion-table` also affect code which was not the intended
>> consumer". This problem does not exist with traditional/explicit
>> argument passing.
>>
>
> Again, it seems to me that this problem is not directly related to the
>  problem at hand.  If the caller of read-from-minibuffer is not careful
>  enough and binds minibuffer-completion-* too far from the call, in such
>  a way that other code is unexpectedly affected (or could unexpectedly be
>  affected) by this binding, it's the responsibility of that caller to fix
>  that problem.
>
> Anyway, I attach a last version of my patch, in which all the dancing
>  happens at the C level.  It is probably also possible to finally get rid of
>  the 15 lines with the minibuffer-completing-file-name = Qlambda hack in
>  read_minibuf().

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Gregory Heytings]

>
> OK, seeing how noone reported problems with my patch, I pushed it to 
> `master`.  In case someone later reports a problem, we may "revert" to 
> your approach if its slightly better compatibility proves useful.
>

Okay, I guess the best thing to do now is to just let it go.  The main 
point of my approach was not a better compatibility, but to fix that 
problem in a more general way that would have cleaned up the situation in 
a longer term.

bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t

[Lars Ingebrigtsen]

Gregory Heytings <gregory@heytings.org> writes:

>> OK, seeing how noone reported problems with my patch, I pushed it to
>> `master`.  In case someone later reports a problem, we may "revert"
>> to your approach if its slightly better compatibility proves useful.
>>
>
> Okay, I guess the best thing to do now is to just let it go.  The main
> point of my approach was not a better compatibility, but to fix that
> problem in a more general way that would have cleaned up the situation
> in a longer term.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

This was a very long thread, and I did not read it all, but if I
understand this correctly, Stefan's patch fixed the issue, so I'm
closing this bug report.  If that's a mistake, please respond to the
debbugs address and we'll reopen.

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