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

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

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