bug#47678: 27.1; `completion-boundaries` assertion failure for file

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Daniel Mendler]

1. Start emacs -Q, evaluate the following code:

~~~
(defun test-boundaries ()
   (interactive)
   (let* ((contents (minibuffer-contents-no-properties))
          (pt (max 0 (- (point) (minibuffer-prompt-end))))
          (before (substring contents 0 pt))
          (after (substring contents pt)))
     (completion-boundaries before
                            minibuffer-completion-table
                            minibuffer-completion-predicate
                            after)))
(global-set-key (kbd "C-.") #'test-boundaries)
~~~

2. Press "C-x C-f"
3. Enter the path "~//"
4. Move the point between the slashes, "~/|/"
5. Press "C-."

The function `test-boundaries' should determine the completion
boundaries correctly. I assume that the function `completion-boundaries'
should never fail with an error. This bug is hard to trigger with
default completion but occurs more often with continuously updating
interactive completion systems like "Vertico", when entering some path
and moving the point around.

I've got confirmation that the bug is still present in Emacs 28, but I 
have not tested this myself.

In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, 
cairo version 1.16.0)
  of 2021-02-09, modified by Debian built on 3df710f593d9
Repository revision: b0229d4bbaea7fcddffced393512c650212830db
Repository branch: deb/emacs/d/sid/master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Recent messages:
test-boundaries [2 times]
Quit
completion--some: Assertion failed: (string-prefix-p ustring ufull) [2 
times]
Quit
(basic)
nil [2 times]
completion--some: Assertion failed: (string-prefix-p ustring ufull) [2 
times]
Quit [2 times]
next-line: End of buffer
Mark set

Configured using:
  'configure --build x86_64-linux-gnu --prefix=/usr
  --sharedstatedir=/var/lib --libexecdir=/usr/lib
  --localstatedir=/var/lib --infodir=/usr/share/info
  --mandir=/usr/share/man --enable-libsystemd --with-pop=yes
 
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils --build
  x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
  --libexecdir=/usr/lib --localstatedir=/var/lib
  --infodir=/usr/share/info --mandir=/usr/share/man --enable-libsystemd
  --with-pop=yes
 
--enable-locallisppath=/etc/emacs:/usr/local/share/emacs/27.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/27.1/site-lisp:/usr/share/emacs/site-lisp
  --with-sound=alsa --without-gconf --with-mailutils --with-cairo
  --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
  'CFLAGS=-g -O2
  -fdebug-prefix-map=/emacs/emacs=. -fstack-protector-strong -Wformat
  -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
  -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD
JSON PDUMPER LCMS2 GMP

Important settings:
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   eldoc-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs text-property-search time-date subr-x seq
byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils edmacro kmacro vc-git
diff-mode easymenu easy-mmode cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process emacs)

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Stefan Monnier]

> 2. Press "C-x C-f"
> 3. Enter the path "~//"
> 4. Move the point between the slashes, "~/|/"
> 5. Press "C-."

Thanks!
[ It would have helped to say what happens for you there.  ]

> The function `test-boundaries' should determine the completion
> boundaries correctly. I assume that the function `completion-boundaries'
> should never fail with an error.

[ From this I can guess that you maybe got some kind of error?
  Including a backtrace or the text of the error or some
  description of the error would have been helpful.  ]

I installed the patch below, which seems to fix the immediate problem
I managed to reproduce, but it might lead to further problems down
the road.


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5f594679ca..c900b0d7ce 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -488,8 +488,17 @@ completion-table-with-quoting
              (qsuffix (cdr action))
              (ufull (if (zerop (length qsuffix)) ustring
                       (funcall unquote (concat string qsuffix))))
-             (_ (cl-assert (string-prefix-p ustring ufull)))
-             (usuffix (substring ufull (length ustring)))
+             ;; If (not (string-prefix-p ustring ufull)) we have a problem:
+             ;; the unquoting the qfull gives something "unrelated" to ustring.
+             ;; E.g. "~/" and "/" where "~//" gets unquoted to just "/" (see
+             ;; bug#47678).
+             ;; In that case we can't even tell if we're right before the
+             ;; "/" or right after it (aka if this "/" is from qstring or
+             ;; from qsuffix), which which usuffix to use is very unclear.
+             (usuffix (if (string-prefix-p ustring ufull)
+                          (substring ufull (length ustring))
+                        ;; FIXME: Maybe "" is preferable/safer?
+                        qsuffix))
              (boundaries (completion-boundaries ustring table pred usuffix))
              (qlboundary (car (funcall requote (car boundaries) string)))
              (qrboundary (if (zerop (cdr boundaries)) 0 ;Common case.

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Daniel Mendler]

On 4/13/21 1:28 AM, Stefan Monnier wrote:
>> 2. Press "C-x C-f"
>> 3. Enter the path "~//"
>> 4. Move the point between the slashes, "~/|/"
>> 5. Press "C-."
> 
> Thanks!
> [ It would have helped to say what happens for you there.  ]

Yes, I wrote "assertion failure". Sorry for not having been more precise 
(out of bounds error/assertion failure and pasted a stack trace, I will 
do that the next time). On Emacs 28 it seems to only give an out of 
bounds error directly. But from the patch below you identified the 
relevant line. I had seen ufull/ustring/usomething in the error message.

> I installed the patch below, which seems to fix the immediate problem
> I managed to reproduce, but it might lead to further problems down
> the road.

Thank you for looking into this. I hope this will not lead to further 
problems, it is mostly an edge case when moving around the cursor and 
the path at that point is not really valid? What kind of issues could 
happen? I can always enter some invalid/shadowed path ///, /~~~/, ~/~/~, 
which do not make problems (at least with this patch).

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Stefan Monnier]

> Thank you for looking into this. I hope this will not lead to further
> problems, it is mostly an edge case when moving around the cursor and the
> path at that point is not really valid? What kind of issues could happen?
> I can always enter some invalid/shadowed path ///, /~~~/, ~/~/~, which do
> not make problems (at least with this patch).

The problem has to do with quoting/unquoting: in `C-x C-f` we do not
quite enter "raw file name" because the name typed by the user is passed
through `substitute-in-file-name` which expands envvar references (and
de-doubles dollars for those files whose name looks like an envvar
reference) and does some truncation (like foo//bar => /bar).

So the `completion-file-name-table` (which completes raw file names) is
wrapped via `completion-table-with-quoting` and this function has to be
able to relate the unquoted text back to the quoted text, for example in
order to convert the boundaries returned by `completion-file-name-table`
(on the unquoted file name) into the corresponding boundaries that apply
to the quoted file name.

This is in general somewhere between hard and impossible.  So if/when we
get those boundaries wrong (for example) it may lead to misbehavior down
the line.
I'd rather not try and guess what those might be.
I'm just hoping that they're minor enough and rare enough.


        Stefan

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Daniel Mendler]

On 4/13/21 2:46 PM, Stefan Monnier wrote:
> The problem has to do with quoting/unquoting: in `C-x C-f` we do not
> quite enter "raw file name" because the name typed by the user is passed
> through `substitute-in-file-name` which expands envvar references (and
> de-doubles dollars for those files whose name looks like an envvar
> reference) and does some truncation (like foo//bar => /bar).

I am aware of that feature and use it but for now I avoided looking into 
how this is exactly implemented :)

> So the `completion-file-name-table` (which completes raw file names) is
> wrapped via `completion-table-with-quoting` and this function has to be
> able to relate the unquoted text back to the quoted text, for example in
> order to convert the boundaries returned by `completion-file-name-table`
> (on the unquoted file name) into the corresponding boundaries that apply
> to the quoted file name.
> 
> This is in general somewhere between hard and impossible.  So if/when we
> get those boundaries wrong (for example) it may lead to misbehavior down
> the line.
> I'd rather not try and guess what those might be.
> I'm just hoping that they're minor enough and rare enough.

Thank you for the detailed explanation.

Ok, but in the case where this issue occurs we already have a somehow 
broken path, especially when moving with the point over the shadowed 
parts. So as a user I would not be worried to much if this does not work.

I hope most assumptions down the line are checked by conditions. However 
given that in this particular case we already have an error we could 
only get more errors at worst down the line?

(Btw, in Vertico I throw out the shadowed paths as soon as I get the 
opportunity (vertico--tidy-shadowed-file) in the same way as Icomplete 
does it if enabled.)

Daniel

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Stefan Monnier]

> (Btw, in Vertico I throw out the shadowed paths as soon as I get the
> opportunity (vertico--tidy-shadowed-file) in the same way as Icomplete does
> it if enabled.)

The default completion UI tries pretty hard to avoid all operations that
are specific to a particular backend.  There are still some details
where we do things differently when we know we're completing filenames,
but I consider these to be failures of the API that need to be fixed.

IOW I consider the existence of `vertico--tidy-shadowed-file` as a bug
in itself ;-)


        Stefan

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Lars Ingebrigtsen]

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

> IOW I consider the existence of `vertico--tidy-shadowed-file` as a bug
> in itself ;-)

Skimming this thread, I'm not sure whether there's anything to be done
in Emacs here?

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

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Daniel Mendler]

On 5/6/21 12:01 PM, Lars Ingebrigtsen wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>> IOW I consider the existence of `vertico--tidy-shadowed-file` as a bug
>> in itself ;-)
> 
> Skimming this thread, I'm not sure whether there's anything to be done
> in Emacs here?

@Stefan The existence of this function is a bug for people who want to
keep their shadowed paths. This is a matter of preference. For me
shadowed paths are a cheap means of changing the directory during
completion and I usually don't want to go back to the old path in the
process. Icomplete has the same function, but guarded behind a
customization option. Why do you prefer to keep the shadowed paths?

@Lars From what I see, Stefan already fixed the bug?

Daniel

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> @Lars From what I see, Stefan already fixed the bug?

OK, closing this bug report, then.

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

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Daniel Mendler [2021-05-06 13:07:40] wrote:
> On 5/6/21 12:01 PM, Lars Ingebrigtsen wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> IOW I consider the existence of `vertico--tidy-shadowed-file` as a bug
>>> in itself ;-)
>> Skimming this thread, I'm not sure whether there's anything to be done
>> in Emacs here?

There is, but it's not a simple "bug fix".  What I meant above is that the
existence of `vertico--tidy-shadowed-file` reflects a shortcoming of the
completion API, which does not offer any way for the completion-table to
explain this kind of "shadowing", so the UI is stuck using ad-hoc tricks
by trying to detect use of some specific completion tables (e.g. by
testing `minibuffer-completing-file-name`) and then using
"inside-knowledge" of the semantics behind it.

Not only the way it's done is inevitably "hackish", but it's also very
hard to make it correct.  E.g. the shadowing is not a property of
file names but of `substitute-in-file-name`, so `file-name-shadow-mode`
and `vertico--tidy-shadowed-file` can misfire if you're reading a file
name which will not be passed to `substitute-in-file-name`.

> @Stefan The existence of this function is a bug for people who want to
> keep their shadowed paths. This is a matter of preference. For me
> shadowed paths are a cheap means of changing the directory during
> completion and I usually don't want to go back to the old path in the
> process. Icomplete has the same function, but guarded behind a
> customization option. Why do you prefer to keep the shadowed paths?

I don't.  I think it makes a lot of sense as a UI feature.

To do it right, we'd need completion tables to provide some kind of
"tidy up" or "simplify" or "canonicalize" method, I think.


        Stefan

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Daniel Mendler]

On 5/9/21 1:41 AM, Stefan Monnier wrote:
> Not only the way it's done is inevitably "hackish", but it's also very
> hard to make it correct.  E.g. the shadowing is not a property of
> file names but of `substitute-in-file-name`, so `file-name-shadow-mode`
> and `vertico--tidy-shadowed-file` can misfire if you're reading a file
> name which will not be passed to `substitute-in-file-name`.
> ...
> To do it right, we'd need completion tables to provide some kind of
> "tidy up" or "simplify" or "canonicalize" method, I think.

Thank you for the clarification. I agree. A while ago when fiddling with
file based commands I had wondered why completing-read or the UI can
even return shadowed paths leading to complications down the road. A
canonicalize method would be a good way to achieve this generically. Are
there more examples where one would want to do such a canonicalization?

In the case of file name canonicalization, one has to consider - the
file name is allowed to contain environment variables. This is a feature
which I would like to retain at the UI level. It would be good if the
returned string is canonicalized, but I do not want to dynamically
expand the environment variable while the user enters input. In
contrast, with shadowed paths I am perfectly fine if the UI removes that
shadowed path automatically, like Icomplete and Vertico do it.

What do you mean by "providing a method"? Add a method to the completion
table function itself or to add a completion metadata
`canonicalize-function`? Modifying the completion table specification
itself is more problematic since it is not extensible in its current form.

Daniel

bug#47678: 27.1; `completion-boundaries` assertion failure for file

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Thank you for the clarification. I agree. A while ago when fiddling with
> file based commands I had wondered why completing-read or the UI can
> even return shadowed paths leading to complications down the road. A
> canonicalize method would be a good way to achieve this generically. Are
> there more examples where one would want to do such a canonicalization?

Good question.  I can imagine it being used in a few other "file-like"
completion tables (e.g. MH mailboxes), but it doesn't seem nearly as
useful.  There might also be some possible uses in a context of quoting
where it could return the string but quoted in a "canonical way"
(e.g. for file names, it could maybe double the lone `$` signs that
happen not to match any env-var, or add {...} around the env-vars), ...

But no, none of those are very compelling candidates.
For this reason, the current hack lives on.

> What do you mean by "providing a method"?

Nothing specific.


        Stefan