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

[Stefan Monnier <monnier@iro.umontreal.ca>]

>>> 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