Setting `minibuffer-completion-table` buffer-locally

Setting `minibuffer-completion-table` buffer-locally

[Stefan Monnier]

Following a long discussion in bug#45474, I'm warming up to the idea to
install the patch below which changes `completing-read(-default)` such
that it sets the `minibuffer-completion-*` variables buffer-locally in
the mini-buffer instead of let-binding them globally.

I've had such a change in the back of my mind for a while, and the
discussion around bug#45474 convinced me that setting the vars
buffer-locally is the only right solution.  It's also necessary in order
for the multiple active minibuffers that can now be displayed
simultaneously to work correctly.

I was a bit scared of the potential for breaking existing packages,
since this is an incompatible change, but my analysis of all the code
I could find in MELPA and GNU ELPA suggests that there should be very
little breakage, if any.

It worked fine in my tests with the default UI, Icomplete, Vertico,
and Ivy.

If people could test it (and report here) against their favorite funky
completion package with their personal configuration (Helm, Ivy,
Selectrum, Icicles, Younameit, ...) I'd be most welcome.

There's a "competing" patch from Gregory which does something similar
but in a slightly different way (less clean but potentially a bit more
backward compatible), and I think in order to decide which is the best
approach we need to have a more concrete information about the risk of
breakage and what we might need to do to minimize it.


        Stefan


diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 7cf2fcf68f2..2d284f5b85d 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -1188,9 +1188,9 @@ Completion Commands
 @defvar minibuffer-completion-table
 The value of this variable is the completion table (@pxref{Basic
 Completion}) used for completion in the minibuffer.  This is the
-global variable that contains what @code{completing-read} passes to
+buffer-local variable that contains what @code{completing-read} passes to
 @code{try-completion}.  It is used by minibuffer completion commands
-such as @code{minibuffer-complete-word}.
+such as @code{minibuffer-complete}.
 @end defvar
 
 @defvar minibuffer-completion-predicate
@@ -1201,7 +1201,7 @@ Completion Commands
 
 @defvar minibuffer-completion-confirm
 This variable determines whether Emacs asks for confirmation before
-exiting the minibuffer; @code{completing-read} binds this variable,
+exiting the minibuffer; @code{completing-read} sets this variable,
 and the function @code{minibuffer-complete-and-exit} checks the value
 before exiting.  If the value is @code{nil}, confirmation is not
 required.  If the value is @code{confirm}, the user may exit with an
diff --git a/etc/NEWS b/etc/NEWS
index 7d600eb374d..709c74b38df 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2427,6 +2427,11 @@ This is to keep the same behavior as Eshell.
 \f
 * Incompatible Lisp Changes in Emacs 28.1
 
++++
+** 'completing-read-default' sets completion variables buffer-locally.
+'minibuffer-completion-table' and related variables are now set buffer-locally
+in the minibuffer instead of being set via a global let-binding.
+
 +++
 ** The use of positional arguments in 'define-minor-mode' is obsolete.
 These were actually rendered obsolete in Emacs-21 but were never
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 51e0519d489..2eafc12d882 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))

Re: Setting `minibuffer-completion-table` buffer-locally

[Daniel Mendler]

On 4/24/21 4:56 AM, Stefan Monnier wrote:
> Following a long discussion in bug#45474, I'm warming up to the idea to
> install the patch below which changes `completing-read(-default)` such
> that it sets the `minibuffer-completion-*` variables buffer-locally in
> the mini-buffer instead of let-binding them globally.

I am in favor of such a change. In the completion systems or packages 
centered around completion I am involved with (Selectrum, Vertico, 
Marginalia, Consult, Embark, ...) we always followed the style of 
encapsulating minibuffer-local state inside the minibuffer. Let-binding 
the built-in table always seemed  odd. I don't expect breakage from 
these packages. If some issues occur despite my expectations we will fix 
them promptly. It is no problem to fix issues in a backward compatible 
way by always accessing the minibuffer-completion-table via the 
minibuffer, if for some reason the variable is accessed from the context 
of another buffer.

Daniel

Re: Setting `minibuffer-completion-table` buffer-locally

[Basil L. Contovounesios]

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

> If people could test it (and report here) against their favorite funky
> completion package with their personal configuration (Helm, Ivy,
> Selectrum, Icicles, Younameit, ...) I'd be most welcome.

Ivy doesn't use or modify completing-read-default, so it should be
unaffected.  Some light testing of the patch with my config seems to
confirm this.

Thanks,

-- 
Basil

Re: Setting `minibuffer-completion-table` buffer-locally

[Gregory Heytings]

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


>
> There's a "competing" patch from Gregory which does something similar 
> but in a slightly different way (less clean but potentially a bit more 
> backward compatible), and I think in order to decide which is the best 
> approach we need to have a more concrete information about the risk of 
> breakage and what we might need to do to minimize it.
>

In the interest of fairness in the discussion and comparison, and given 
the length of the discussion in bug#45474, here is the "competing" patch.

The five major differences between the two approaches are (m-c-* stands 
for the minibuffer-completion-* variables, r-f-m for read-from-minibuffer, 
and m-w-s-h for minibuffer-with-setup-hook):

(1) SM: m-c-* variables are passed explicitly as arguments to r-f-m with a 
m-w-s-h, they are stricly buffer-local

GH: m-c-* variables are passed implicitly as arguments to r-f-m, they 
automatically become buffer-local upon entering the minibuffer

(2) SM: it is and it will remain necessary to use a m-w-s-h with 
setq-locals to have buffer-local m-c-* variables in r-f-m

GH: for Emacs 28 it is necessary to let-bind minibuffer-local-completion 
to t around the call to r-f-m (to preserve backward compatibility), for 
Emacs 29 and above minibuffer-local-completion t will be assumed (or, with 
the strong version: the only possible behavior)

(3) SM: when m-c-* variables are strictly buffer-local (i.e., when they 
are passed in a m-w-s-h to r-f-m), they cannot interfere with code outside 
of the minibuffer

GH: because m-c-* variables become buffer-local only upon entering the 
minibuffer, they can possibly interfere with code that is executed around 
the call to r-f-m, between the moment they have been let-bound and the 
moment they become buffer-local

(4) SM: the semantics of r-f-m is not changed

GH: the semantics of r-f-m is changed, in Emacs 28 the m-c-* variables 
become buffer-local when minibuffer-local-completion is t, in Emacs 29 
they become buffer-local unless minibuffer-local-completion is nil (or, 
with the strong version: they become buffer-local)

(5) SM: the patch fixes the behavior of completing-read-default

GH: the patch fixes the behavior of completing-read-default and makes, in 
the medium term, that new behavior the default one for all uses of r-f-m

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

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..584afd5d04 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,23 @@ 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.
+
+[STRONG VERSION:] Setting this variable in Emacs 29 will have no effect;
+the value t will be assumed.
+
+[WEAK VERSION:] The default will be t for Emacs 29.
+
+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.  */);