GNU bug report logs - #45474
Icomplete exhibiting in recursive minibuffer when it shouldn’t

Previous Next

Package: emacs;

Reported by: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>

Date: Sun, 27 Dec 2020 17:45:02 UTC

Severity: normal

Done: Lars Ingebrigtsen <larsi <at> gnus.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45474 in the body.
You can then email your comments to 45474 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 27 Dec 2020 17:45:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sun, 27 Dec 2020 17:45:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 27 Dec 2020 18:44:40 +0100
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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 15 Apr 2021 17:41:02 GMT) Full text and rfc822 format available.

Message #8 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>
Cc: 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 15 Apr 2021 17:40:04 +0000
[Message part 1 (text/plain, inline)]
>
> 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.
[Disable-icomplete-mode-when-reading-an-Emacs-Lisp-ex.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 15 Apr 2021 21:13:01 GMT) Full text and rfc822 format available.

Message #11 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 16 Apr 2021 00:11:10 +0300
>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 15 Apr 2021 22:35:02 GMT) Full text and rfc822 format available.

Message #14 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 15 Apr 2021 22:34:11 +0000
[Message part 1 (text/plain, inline)]
>>> 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.
[Make-it-possible-to-disable-icomplete-mode-in-recurs.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 16 Apr 2021 00:04:01 GMT) Full text and rfc822 format available.

Message #17 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 16 Apr 2021 00:03:18 +0000
[Message part 1 (text/plain, inline)]
>
> 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.
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 16 Apr 2021 16:36:01 GMT) Full text and rfc822 format available.

Message #20 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 16 Apr 2021 19:34:19 +0300
> @@ -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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 16 Apr 2021 16:56:01 GMT) Full text and rfc822 format available.

Message #23 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 16 Apr 2021 16:55:56 +0000
>
> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 17 Apr 2021 20:54:02 GMT) Full text and rfc822 format available.

Message #26 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Sat, 17 Apr 2021 23:49:01 +0300
>> 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).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 17 Apr 2021 21:36:01 GMT) Full text and rfc822 format available.

Message #29 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sat, 17 Apr 2021 21:35:20 +0000
>
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 17 Apr 2021 21:59:02 GMT) Full text and rfc822 format available.

Message #32 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Sat, 17 Apr 2021 17:58:31 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 17 Apr 2021 22:17:01 GMT) Full text and rfc822 format available.

Message #35 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sat, 17 Apr 2021 22:16:16 +0000
[Message part 1 (text/plain, inline)]
>> 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.
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 17 Apr 2021 23:22:02 GMT) Full text and rfc822 format available.

Message #38 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Gregory Heytings
 <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>, Juri Linkov <juri <at> linkov.net>
Subject: RE: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sat, 17 Apr 2021 23:21:23 +0000
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 04:00:02 GMT) Full text and rfc822 format available.

Message #41 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#45474: Icomplete exhibiting in recursive
 minibuffer when it shouldn’t
Date: Sat, 17 Apr 2021 23:59:19 -0400
>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 04:05:02 GMT) Full text and rfc822 format available.

Message #44 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 04:04:36 +0000
> >> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 05:09:02 GMT) Full text and rfc822 format available.

Message #47 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#45474: Icomplete exhibiting in recursive
 minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 01:08:37 -0400
> 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






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 14:46:02 GMT) Full text and rfc822 format available.

Message #50 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Sun, 18 Apr 2021 10:44:57 -0400
>>> 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'.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 15:43:02 GMT) Full text and rfc822 format available.

Message #53 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 15:42:34 +0000
> > 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 18:36:02 GMT) Full text and rfc822 format available.

Message #56 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#45474: Icomplete exhibiting in recursive
 minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 14:35:25 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 20:12:01 GMT) Full text and rfc822 format available.

Message #59 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 20:11:22 +0000
> > 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 20:54:02 GMT) Full text and rfc822 format available.

Message #62 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#45474: Icomplete exhibiting in recursive
 minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 16:53:43 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 22:24:02 GMT) Full text and rfc822 format available.

Message #65 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 22:23:46 +0000
[Message part 1 (text/plain, inline)]
>> 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?
[Make-it-possible-to-disable-completion-in-recursive-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sun, 18 Apr 2021 23:47:01 GMT) Full text and rfc822 format available.

Message #68 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: RE: [External] : bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sun, 18 Apr 2021 23:46:01 +0000
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 01:27:01 GMT) Full text and rfc822 format available.

Message #71 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Sun, 18 Apr 2021 21:26:13 -0400
>>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 12:15:02 GMT) Full text and rfc822 format available.

Message #74 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Mon, 19 Apr 2021 12:14:18 +0000
>> 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).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 15:59:01 GMT) Full text and rfc822 format available.

Message #77 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Mon, 19 Apr 2021 11:57:59 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 18:23:02 GMT) Full text and rfc822 format available.

Message #80 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Mon, 19 Apr 2021 21:16:18 +0300
[Message part 1 (text/plain, inline)]
> 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'?


[completing-minibuffer.patch (text/x-diff, inline)]
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)))
 
 ;; Miscellaneous
 

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 20:21:01 GMT) Full text and rfc822 format available.

Message #83 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Mon, 19 Apr 2021 20:20:04 +0000
>
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 19 Apr 2021 21:43:01 GMT) Full text and rfc822 format available.

Message #86 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> linkov.net>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Mon, 19 Apr 2021 17:42:02 -0400
> 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))





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Tue, 20 Apr 2021 19:01:02 GMT) Full text and rfc822 format available.

Message #89 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Tue, 20 Apr 2021 19:00:29 +0000
[Message part 1 (text/plain, inline)]
>> 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.
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Tue, 20 Apr 2021 19:15:02 GMT) Full text and rfc822 format available.

Message #92 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Tue, 20 Apr 2021 22:01:31 +0300
>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 13:55:02 GMT) Full text and rfc822 format available.

Message #95 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> linkov.net>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Thu, 22 Apr 2021 09:54:48 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 13:57:01 GMT) Full text and rfc822 format available.

Message #98 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Thu, 22 Apr 2021 09:56:39 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 14:09:01 GMT) Full text and rfc822 format available.

Message #101 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 14:08:03 +0000
>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 14:14:01 GMT) Full text and rfc822 format available.

Message #104 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> linkov.net>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Thu, 22 Apr 2021 10:13:22 -0400
>> 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))





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 14:19:01 GMT) Full text and rfc822 format available.

Message #107 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 14:18:06 +0000
>
> 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.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 15:04:02 GMT) Full text and rfc822 format available.

Message #110 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Gregory Heytings <gregory <at> heytings.org>, Juri Linkov <juri <at> linkov.net>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 "45474 <at> debbugs.gnu.org" <45474 <at> debbugs.gnu.org>
Subject: Re: [External] : bug#45474: Icomplete exhibiting in recursive
 minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 11:03:26 -0400
> 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))





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 15:19:02 GMT) Full text and rfc822 format available.

Message #113 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 15:18:49 +0000
[Message part 1 (text/plain, inline)]
>> 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.
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 18:37:01 GMT) Full text and rfc822 format available.

Message #116 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Thu, 22 Apr 2021 14:36:49 -0400
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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 19:05:02 GMT) Full text and rfc822 format available.

Message #119 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 19:04:04 +0000
>
> 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"...




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 20:00:02 GMT) Full text and rfc822 format available.

Message #122 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 19:59:03 +0000
>>>    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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 20:58:02 GMT) Full text and rfc822 format available.

Message #125 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Thu, 22 Apr 2021 20:57:25 +0000
[Message part 1 (text/plain, inline)]
>>>>    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.
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 22:11:02 GMT) Full text and rfc822 format available.

Message #128 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 00:57:56 +0300
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Thu, 22 Apr 2021 23:25:02 GMT) Full text and rfc822 format available.

Message #131 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Thu, 22 Apr 2021 19:24:30 -0400
>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 06:08:01 GMT) Full text and rfc822 format available.

Message #134 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: gregory <at> heytings.org, juri <at> linkov.net, dario.gjorgjevski <at> gmail.com,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 09:06:44 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Thu, 22 Apr 2021 19:24:30 -0400
> Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
>  Juri Linkov <juri <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 07:00:02 GMT) Full text and rfc822 format available.

Message #137 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 23 Apr 2021 06:59:09 +0000
>>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 13:13:01 GMT) Full text and rfc822 format available.

Message #140 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gregory <at> heytings.org, juri <at> linkov.net, dario.gjorgjevski <at> gmail.com,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
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.


        Stefan





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 13:20:02 GMT) Full text and rfc822 format available.

Message #143 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: gregory <at> heytings.org, juri <at> linkov.net, dario.gjorgjevski <at> gmail.com,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 16:19:42 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: gregory <at> heytings.org,  dario.gjorgjevski <at> gmail.com,
>   45474 <at> debbugs.gnu.org,  juri <at> 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".




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 13:22:01 GMT) Full text and rfc822 format available.

Message #146 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 09:21:05 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 13:46:02 GMT) Full text and rfc822 format available.

Message #149 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 23 Apr 2021 13:45:47 +0000
>> 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)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 15:20:01 GMT) Full text and rfc822 format available.

Message #152 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gregory <at> heytings.org, juri <at> linkov.net, dario.gjorgjevski <at> gmail.com,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 11:18:48 -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".

;-)

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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 15:36:01 GMT) Full text and rfc822 format available.

Message #155 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 11:35:40 -0400
>>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 15:54:02 GMT) Full text and rfc822 format available.

Message #158 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> linkov.net>
Cc: Gregory Heytings <gregory <at> heytings.org>,
 Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 11:53:13 -0400
>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 15:59:01 GMT) Full text and rfc822 format available.

Message #161 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 23 Apr 2021 15:58:51 +0000
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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 16:39:02 GMT) Full text and rfc822 format available.

Message #164 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 19:36:16 +0300
>>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 16:56:02 GMT) Full text and rfc822 format available.

Message #167 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 12:55:02 -0400
> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 17:38:02 GMT) Full text and rfc822 format available.

Message #170 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: gregory <at> heytings.org, juri <at> linkov.net, dario.gjorgjevski <at> gmail.com,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 20:37:23 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: gregory <at> heytings.org,  dario.gjorgjevski <at> gmail.com,
>   45474 <at> debbugs.gnu.org,  juri <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 18:14:01 GMT) Full text and rfc822 format available.

Message #173 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 23 Apr 2021 18:13:21 +0000
>> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 20:25:02 GMT) Full text and rfc822 format available.

Message #176 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 16:24:34 -0400
>>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 21:37:01 GMT) Full text and rfc822 format available.

Message #179 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Fri, 23 Apr 2021 21:36:31 +0000
>> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Fri, 23 Apr 2021 21:55:01 GMT) Full text and rfc822 format available.

Message #182 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Fri, 23 Apr 2021 17:54:30 -0400
>>> 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





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 24 Apr 2021 08:45:02 GMT) Full text and rfc822 format available.

Message #185 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Sat, 24 Apr 2021 08:44:38 +0000
[Message part 1 (text/plain, inline)]
>> 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().
[Make-it-possible-to-disable-a-completion-backend-in-.patch (text/x-diff, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Sat, 01 May 2021 19:35:02 GMT) Full text and rfc822 format available.

Message #188 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Sat, 01 May 2021 15:34:44 -0400
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().





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Mon, 03 May 2021 08:41:01 GMT) Full text and rfc822 format available.

Message #191 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>, 45474 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it shouldn’t
Date: Mon, 03 May 2021 08:40:38 +0000
>
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#45474; Package emacs. (Tue, 07 Jun 2022 12:05:01 GMT) Full text and rfc822 format available.

Message #194 received at 45474 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com>,
 Juri Linkov <juri <at> linkov.net>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 45474 <at> debbugs.gnu.org
Subject: Re: bug#45474: Icomplete exhibiting in recursive minibuffer when it
 shouldn’t
Date: Tue, 07 Jun 2022 14:04:35 +0200
Gregory Heytings <gregory <at> 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




bug closed, send any further explanations to 45474 <at> debbugs.gnu.org and Dario Gjorgjevski <dario.gjorgjevski <at> gmail.com> Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Tue, 07 Jun 2022 12:05:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 06 Jul 2022 11:24:10 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 267 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.