comint-filename-completion and :exclusive completion bugs

comint-filename-completion and :exclusive completion bugs

[Vitalie Spinu]

Hi, 

In case of no completion comint-filename-completion doesn't transfer the
completion to next completion-at-point-function. 

Example:

(defun test-completion ()
  (message "here")
  nil)

(setq completion-at-point-functions '(comint-filename-completion test-completion))

Place the point after 'sfsdf  and M-x completion-at-point. You will get
"No-match" message. The intended behavior is to pass over to next
completion function `test-completion'.


The fix is to pass :exlusive 'no :

diff --git a/lisp/comint.el b/lisp/comint.el
index 4c2229f..4935630 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -3164,13 +3164,14 @@ the form (concat S2 S)."
         (let ((completion-ignore-case read-file-name-completion-ignore-case)
               (completion-ignored-extensions comint-completion-fignore))
           (complete-with-action action table string pred))))
-     (unless (zerop (length filesuffix))
-       (list :exit-function
-             (lambda (_s finished)
-               (when (memq finished '(sole finished))
-                 (if (looking-at (regexp-quote filesuffix))
-                     (goto-char (match-end 0))
-                   (insert filesuffix)))))))))
+     (append (list :exclusive 'no)
+	     (unless (zerop (length filesuffix))
+	       (list :exit-function
+		     (lambda (_s finished)
+		       (when (memq finished '(sole finished))
+			 (if (looking-at (regexp-quote filesuffix))
+			     (goto-char (match-end 0))
+			   (insert filesuffix))))))))))
 
 (defun comint-dynamic-complete-as-filename ()
   "Dynamically complete at point as a filename.


Now it works after 'sfsfd, but it reveals the following problem with
:exlusive argument:

Place your cursor after /bin and M-x completion-at-point, which
completes to /bin/, but calls test-completion with 3 arguments:

Debugger entered--Lisp error: (wrong-number-of-arguments (lambda nil (message "here") nil) 3)
  test-completion("/bin/" (closure ((table . #[771 "\306.\307\301G\301\310\211.&\a\311=\205$.\x02\301G\310O\302\304\203 .\304.!\202!.\x01P\262.\211\205/.\312.\300.\x06.$\211\205\342.\x02\242\313=\203l.\211\242\313=\203K.\211.\x1a\211A@)\206L.\307\313\301G.\301G\302GZ\\].\242\313=\205e.\x03\211.\x1aAA)BB\262.\202\342.\211;\203\230.\306.\307\302G\302\310\211.&\a\311=\205\342.\211\302G\310O\301\303\203\221.\303.!\202\222.\x01P\262.\202\342.\x02\311=\203\341.\314.\300.\x06\315$\211@\302GY\203\274.\303\203\270.\316\303.\"\202\334.\x01\202\334.\317\320\302.@\310O!P\321\310\316\322\323\324\325\326\303.	\"\327\"\330\331%.\x06\"\"\262.\262.\202\342.\211\207" [comint-completion-file-name-table "" "" comint-quote-filename comint-unquote-filename completion-ignore-case compare-strings 0 nil t complete-with-action boundaries completion-boundaries "" mapcar "\\`" regexp-quote delq make-byte-code 257 "\302\301.\"\205.\0\211\303\225\304O\300\203.\0\300.!\202.\0\211\262.\207" vconcat vector [string-match 0 nil] 4 "\n\n(fn C)" x] 18 "\n\n(fn STRING PRED ACTION)"]) (unquoted . #("/bin" 0 4 (fontified t))) (filename-end . 1984) (filename-beg . 1980) (filename . #("/bin" 0 4 (fontified t))) (filesuffix . " ") comint-dynamic-list-input-ring-window-conf t) (string pred action) (let ((completion-ignore-case read-file-name-completion-ignore-case) (completion-ignored-extensions comint-completion-fignore)) (complete-with-action action table string pred))) nil)
  completion--do-completion()
  minibuffer-complete()
  call-interactively(minibuffer-complete)
  #[1028 ".\x01\304.\x06.\x06\305\211\306%.\x18\305\262.\x02.\305\262.\307.\310\311#\210\307.\312\313#\210\n\2032.\314\315!\210p.\a.\a.\aF.\316\317\320\321\322\323.\x06!\324\"\325\326%DC\216\327\330!+\207" [minibuffer-completion-predicate minibuffer-completion-table completion-in-region-mode-predicate completion-in-region--data make-overlay nil t overlay-put field completion priority 100 completion-in-region-mode 1 funcall make-byte-code 0 "\301\300!\207" vconcat vector [delete-overlay] 2 "\n\n(fn)" call-interactively minibuffer-complete] 14 "\n\n(fn START END COLLECTION PREDICATE)"](1980 1984 (closure ((table . #[771 "\306.\307\301G\301\310\211.&\a\311=\205$.\x02\301G\310O\302\304\203 .\304.!\202!.\x01P\262.\211\205/.\312.\300.\x06.$\211\205\342.\x02\242\313=\203l.\211\242\313=\203K.\211.\x1a\211A@)\206L.\307\313\301G.\301G\302GZ\\].\242\313=\205e.\x03\211.\x1aAA)BB\262.\202\342.\211;\203\230.\306.\307\302G\302\310\211.&\a\311=\205\342.\211\302G\310O\301\303\203\221.\303.!\202\222.\x01P\262.\202\342.\x02\311=\203\341.\314.\300.\x06\315$\211@\302GY\203\274.\303\203\270.\316\303.\"\202\334.\x01\202\334.\317\320\302.@\310O!P\321\310\316\322\323\324\325\326\303.	\"\327\"\330\331%.\x06\"\"\262.\262.\202\342.\211\207" [comint-completion-file-name-table "" "" comint-quote-filename comint-unquote-filename completion-ignore-case compare-strings 0 nil t complete-with-action boundaries completion-boundaries "" mapcar "\\`" regexp-quote delq make-byte-code 257 "\302\301.\"\205.\0\211\303\225\304O\300\203.\0\300.!\202.\0\211\262.\207" vconcat vector [string-match 0 nil] 4 "\n\n(fn C)" x] 18 "\n\n(fn STRING PRED ACTION)"]) (unquoted . #("/bin" 0 4 (fontified t))) (filename-end . 1984) (filename-beg . 1980) (filename . #("/bin" 0 4 (fontified t))) (filesuffix . " ") comint-dynamic-list-input-ring-window-conf t) (string pred action) (let ((completion-ignore-case read-file-name-completion-ignore-case) (completion-ignored-extensions comint-completion-fignore)) (complete-with-action action table string pred))) nil)
  apply(#[1028 ".\x01\304.\x06.\x06\305\211\306%.\x18\305\262.\x02.\305\262.\307.\310\311#\210\307.\312\313#\210\n\2032.\314\315!\210p.\a.\a.\aF.\316\317\320\321\322\323.\x06!\324\"\325\326%DC\216\327\330!+\207" [minibuffer-completion-predicate minibuffer-completion-table completion-in-region-mode-predicate completion-in-region--data make-overlay nil t overlay-put field completion priority 100 completion-in-region-mode 1 funcall make-byte-code 0 "\301\300!\207" vconcat vector [delete-overlay] 2 "\n\n(fn)" call-interactively minibuffer-complete] 14 "\n\n(fn START END COLLECTION PREDICATE)"] (1980 1984 (closure ((table . #[771 "\306.\307\301G\301\310\211.&\a\311=\205$.\x02\301G\310O\302\304\203 .\304.!\202!.\x01P\262.\211\205/.\312.\300.\x06.$\211\205\342.\x02\242\313=\203l.\211\242\313=\203K.\211.\x1a\211A@)\206L.\307\313\301G.\301G\302GZ\\].\242\313=\205e.\x03\211.\x1aAA)BB\262.\202\342.\211;\203\230.\306.\307\302G\302\310\211.&\a\311=\205\342.\211\302G\310O\301\303\203\221.\303.!\202\222.\x01P\262.\202\342.\x02\311=\203\341.\314.\300.\x06\315$\211@\302GY\203\274.\303\203\270.\316\303.\"\202\334.\x01\202\334.\317\320\302.@\310O!P\321\310\316\322\323\324\325\326\303.	\"\327\"\330\331%.\x06\"\"\262.\262.\202\342.\211\207" [comint-completion-file-name-table "" "" comint-quote-filename comint-unquote-filename completion-ignore-case compare-strings 0 nil t complete-with-action boundaries completion-boundaries "" mapcar "\\`" regexp-quote delq make-byte-code 257 "\302\301.\"\205.\0\211\303\225\304O\300\203.\0\300.!\202.\0\211\262.\207" vconcat vector [string-match 0 nil] 4 "\n\n(fn C)" x] 18 "\n\n(fn STRING PRED ACTION)"]) (unquoted . #("/bin" 0 4 (fontified t))) (filename-end . 1984) (filename-beg . 1980) (filename . #("/bin" 0 4 (fontified t))) (filesuffix . " ") comint-dynamic-list-input-ring-window-conf t) (string pred action) (let ((completion-ignore-case read-file-name-completion-ignore-case) (completion-ignored-extensions comint-completion-fignore)) (complete-with-action action table string pred))) nil))
  #[771 ".:\2030.\x02@\301=\203.\0\300@\302.\x05A\"\303.#\207\304.@\305\306\307\310\311\312\300!\313\"\314\315%.\x06A.\x06#.#\207\304\316.\"\207" [(#0) t append nil apply apply-partially make-byte-code 642 "\300@.\x03.#\207" vconcat vector [] 7 "\n\n(fn FUNS GLOBAL &rest ARGS)" #[1028 ".\x01\304.\x06.\x06\305\211\306%.\x18\305\262.\x02.\305\262.\307.\310\311#\210\307.\312\313#\210\n\2032.\314\315!\210p.\a.\a.\aF.\316\317\320\321\322\323.\x06!\324\"\325\326%DC\216\327\330!+\207" [minibuffer-completion-predicate minibuffer-completion-table completion-in-region-mode-predicate completion-in-region--data make-overlay nil t overlay-put field completion priority 100 completion-in-region-mode 1 funcall make-byte-code 0 "\301\300!\207" vconcat vector [delete-overlay] 2 "\n\n(fn)" call-interactively minibuffer-complete] 14 "\n\n(fn START END COLLECTION PREDICATE)"]] 12 "\n\n(fn FUNS GLOBAL ARGS)"](nil nil (1980 1984 (closure ((table . #[771 "\306.\307\301G\301\310\211.&\a\311=\205$.\x02\301G\310O\302\304\203 .\304.!\202!.\x01P\262.\211\205/.\312.\300.\x06.$\211\205\342.\x02\242\313=\203l.\211\242\313=\203K.\211.\x1a\211A@)\206L.\307\313\301G.\301G\302GZ\\].\242\313=\205e.\x03\211.\x1aAA)BB\262.\202\342.\211;\203\230.\306.\307\302G\302\310\211.&\a\311=\205\342.\211\302G\310O\301\303\203\221.\303.!\202\222.\x01P\262.\202\342.\x02\311=\203\341.\314.\300.\x06\315$\211@\302GY\203\274.\303\203\270.\316\303.\"\202\334.\x01\202\334.\317\320\302.@\310O!P\321\310\316\322\323\324\325\326\303.	\"\327\"\330\331%.\x06\"\"\262.\262.\202\342.\211\207" [comint-completion-file-name-table "" "" comint-quote-filename comint-unquote-filename completion-ignore-case compare-strings 0 nil t complete-with-action boundaries completion-boundaries "" mapcar "\\`" regexp-quote delq make-byte-code 257 "\302\301.\"\205.\0\211\303\225\304O\300\203.\0\300.!\202.\0\211\262.\207" vconcat vector [string-match 0 nil] 4 "\n\n(fn C)" x] 18 "\n\n(fn STRING PRED ACTION)"]) (unquoted . #("/bin" 0 4 (fontified t))) (filename-end . 1984) (filename-beg . 1980) (filename . #("/bin" 0 4 (fontified t))) (filesuffix . " ") comint-dynamic-list-input-ring-window-conf t) (string pred action) (let ((completion-ignore-case read-file-name-completion-ignore-case) (completion-ignored-extensions comint-completion-fignore)) (complete-with-action action table string pred))) nil))
  completion-in-region(1980 1984 (closure ((table . #[771 "\306.\307\301G\301\310\211.&\a\311=\205$.\x02\301G\310O\302\304\203 .\304.!\202!.\x01P\262.\211\205/.\312.\300.\x06.$\211\205\342.\x02\242\313=\203l.\211\242\313=\203K.\211.\x1a\211A@)\206L.\307\313\301G.\301G\302GZ\\].\242\313=\205e.\x03\211.\x1aAA)BB\262.\202\342.\211;\203\230.\306.\307\302G\302\310\211.&\a\311=\205\342.\211\302G\310O\301\303\203\221.\303.!\202\222.\x01P\262.\202\342.\x02\311=\203\341.\314.\300.\x06\315$\211@\302GY\203\274.\303\203\270.\316\303.\"\202\334.\x01\202\334.\317\320\302.@\310O!P\321\310\316\322\323\324\325\326\303.	\"\327\"\330\331%.\x06\"\"\262.\262.\202\342.\211\207" [comint-completion-file-name-table "" "" comint-quote-filename comint-unquote-filename completion-ignore-case compare-strings 0 nil t complete-with-action boundaries completion-boundaries "" mapcar "\\`" regexp-quote delq make-byte-code 257 "\302\301.\"\205.\0\211\303\225\304O\300\203.\0\300.!\202.\0\211\262.\207" vconcat vector [string-match 0 nil] 4 "\n\n(fn C)" x] 18 "\n\n(fn STRING PRED ACTION)"]) (unquoted . #("/bin" 0 4 (fontified t))) (filename-end . 1984) (filename-beg . 1980) (filename . #("/bin" 0 4 (fontified t))) (filesuffix . " ") comint-dynamic-list-input-ring-window-conf t) (string pred action) (let ((completion-ignore-case read-file-name-completion-ignore-case) (completion-ignored-extensions comint-completion-fignore)) (complete-with-action action table string pred))) nil)
  completion-at-point()


It looks like the 3 spurious arguments are passed from
comint-filename-completion. Most surely this is not an intended behavior
of :exlusive keyword. Or, is it?

Vitalie.

Re: comint-filename-completion and :exclusive completion bugs

[Stefan Monnier]

> Place the point after 'sfsdf  and M-x completion-at-point.  You will get
> "No-match" message.  The intended behavior is to pass over to next
> completion function `test-completion'.

Actually, no this is not the intended behavior (unless you
specify :exclusive 'no): functions on completion-at-point-functions are
supposed to only return non-nil if the text at point is one for which
they intend  to take responsibility.

This is important, because it then allows completion-styles to try
harder ("aha! there's no matching prefix?  well, let's see if there's
some other way to find a match").  Otherwise, the completion UI gets
a dilemma: in case of completion style fails, should it try the next
completion style, or should it try to fallback on the next
completion data?

The ":exclusive 'no" solves this dilemma by only allowing prefix
completion.  I think that for filename completion, non-prefix completion
is important, so while I can agree that comint-filename-completion often
isn't really sure whether it should take responsibility (because it
doesn't actually know that there should be a filename at point),
preventing things like partial-completion on filenames would be sad.

Maybe we could provide a comint-maybe-filename-completion which would
set the ":exclusive 'no", for situations like yours where you prefer
falling back on some other completion data if the text isn't a prefix of
a valid filename.

Or maybe you can change your own completion function so that rather than
placing it after comint-filename-completion, you can place it before.

> Now it works after 'sfsfd, but it reveals the following problem with
> :exlusive argument:
> Place your cursor after /bin and M-x completion-at-point, which
> completes to /bin/, but calls test-completion with 3 arguments:

`test-completion' is a predefined function taking 3 args, which you just
redefined as something completely different, leading to all kinds of
funny results.


        Stefan

Re: comint-filename-completion and :exclusive completion bugs

[Vitalie Spinu]

>>>> Stefan Monnier <monnier@iro.umontreal.ca>
>>>> on Wed, 14 Mar 2012 09:53:57 -0400 wrote:

  > The ":exclusive 'no" solves this dilemma by only allowing prefix
  > completion.  I think that for filename completion, non-prefix completion
  > is important, so while I can agree that comint-filename-completion often
  > isn't really sure whether it should take responsibility (because it
  > doesn't actually know that there should be a filename at point),
  > preventing things like partial-completion on filenames would be sad.

I don't really understand how :exlusive 'no prevent partial completion
(in case you mean the obsolete package complete.el). So if there is no
partial expansion of foo_bar|, doesn't :exlusive pass the handling over?
You probably mean some advanced, usage pattern.

comint-filename-completion is active in all comint buffers, and it
prevents the completion UI to reach the global
tags-completion-at-point-function. And this is a bit of a limitation. 

In other modes than comint, file completion of 'sdfdsf, or anything else
not prefixed by "/", or even not quoted, is not a filename, and should
be passed over.

  > Maybe we could provide a comint-maybe-filename-completion which would
  > set the ":exclusive 'no", for situations like yours where you prefer
  > falling back on some other completion data if the text isn't a prefix of
  > a valid filename.

That would be great, for the simple reason that hacking
comint-filename-completion, in order to be used in other modes, is a bit
difficult. A real example:

(defun ess-filename-completion ()
  "Return completion only within string or comment."
  (when (ess-inside-string-or-comment-p (point))
    (comint-filename-completion)
    ))

Comint-filename-completion returns a list of length 2 or 3 depending on
filesuffix. So to tweak it, I have to check for the length, if 2,
append, if 3 insert :exclusive into the last list. Ugly as far as I am
concerned.

  > Or maybe you can change your own completion function so that rather than
  > placing it after comint-filename-completion, you can place it before.

Yes, this is what is now in ESS, but a knowledgeable user of emacs 24
would expect his tags completion to be reached.

  >> Now it works after 'sfsfd, but it reveals the following problem with
  >> :exlusive argument:
  >> Place your cursor after /bin and M-x completion-at-point, which
  >> completes to /bin/, but calls test-completion with 3 arguments:

  > `test-completion' is a predefined function taking 3 args, which you just
  > redefined as something completely different, leading to all kinds of
  > funny results.

Dammit, sorry for this noise, should have been named it
`sdfseref-completion`, this explains the wreckage.

Thanks, 
Vitalie.

Re: comint-filename-completion and :exclusive completion bugs

[Stefan Monnier]

>> The ":exclusive 'no" solves this dilemma by only allowing prefix
>> completion.  I think that for filename completion, non-prefix completion
>> is important, so while I can agree that comint-filename-completion often
>> isn't really sure whether it should take responsibility (because it
>> doesn't actually know that there should be a filename at point),
>> preventing things like partial-completion on filenames would be sad.
> I don't really understand how :exlusive 'no prevent partial completion
> (in case you mean the obsolete package complete.el).

This is a limitation due to the way it's currently implemented.

Hopefully the implementation will be improved, maybe by taking various
"completion data" and combining them along the lines of what
completion-table-in-turn does (so if you have three non-exclusive
completion data like files, tags, and whatnots, the completion would
first try prefix completion on files, then on tags, then on whatnots,
and then partial-completion on files, than on tags, then on whatnots,
...).  But this opens up various other problems:
- what if the beg..end of those completion data are not the same?
- what if the completion `category' of those completion tables is not
  the same, and hence the completion-styles and/or cycle-thresholds to
  use are not the same?

Another approach is to first try all completion styles for files, then
all completion styles for tags, then all completion styles
for whatnots.  But this will require less localized changes in
minibuffer.el.
  
> So if there is no partial expansion of foo_bar|, doesn't :exlusive
> pass the handling over?

No, I'm saying that if you typed "fo/ba" it will pass the handling over
before trying "foo/bar".

> comint-filename-completion is active in all comint buffers, and it
> prevents the completion UI to reach the global
> tags-completion-at-point-function.  And this is a bit of a limitation.

Indeed, it's a limitation.

> In other modes than comint, file completion of 'sdfdsf, or anything else
> not prefixed by "/", or even not quoted, is not a filename, and should
> be passed over.

At least for shell-mode, there are really very few cases where we can
*know* that a given shell argument is not a file name.  The only
exceptions I can think of are when we know it because we know what the
command is and what its arguments can be.
Otherwise, pretty much anything may be a valid file name, e.g. "'The",
"The\ ", and friends can very much be the prefix of a file or
directory name.

>> Maybe we could provide a comint-maybe-filename-completion which would
>> set the ":exclusive 'no", for situations like yours where you prefer
>> falling back on some other completion data if the text isn't a prefix of
>> a valid filename.
> That would be great, for the simple reason that hacking
> comint-filename-completion, in order to be used in other modes, is a bit
> difficult.  A real example:

> (defun ess-filename-completion ()
>   "Return completion only within string or comment."
>   (when (ess-inside-string-or-comment-p (point))
>     (comint-filename-completion)
>     ))

If you need to modify the return value of comint-filename-completion in
non-trivial ways, I recommend

 (pcase (comint-filename-completion)
   (`(,beg ,end ,table . ,props)
    <blabla>))
    
> Comint-filename-completion returns a list of length 2 or 3 depending on
> filesuffix.

Hmm... AFAICT, it's either 0, 3, or 5.

> So to tweak it, I have to check for the length, if 2, append, if
> 3 insert :exclusive into the last list.  Ugly as far as I am concerned.

If you only want to add :exclusive, (append <data> '(:exclusive no))
should work (except for the case where <data> is nil).


        Stefan

Re: comint-filename-completion and :exclusive completion bugs

[Vitalie Spinu]

>>>> Stefan Monnier <monnier@iro.umontreal.ca>
>>>> on Wed, 14 Mar 2012 11:45:59 -0400 wrote:

  > If you need to modify the return value of comint-filename-completion in
  > non-trivial ways, I recommend

  >  (pcase (comint-filename-completion)
  >    (`(,beg ,end ,table . ,props)
  >     <blabla>))

This stuff is amazing. But it took me a while to figure out the
docs. Would be nice to have an info with a small example for each type
of pattern in the docs. Thanks a bunch for the package!

  >> Comint-filename-completion returns a list of length 2 or 3 depending on
  >> filesuffix.

  > Hmm... AFAICT, it's either 0, 3, or 5.

  >> So to tweak it, I have to check for the length, if 2, append, if
  >> 3 insert :exclusive into the last list.  Ugly as far as I am concerned.

  > If you only want to add :exclusive, (append <data> '(:exclusive no))
  > should work (except for the case where <data> is nil).

Oh, yes you are right. I was confused with the help of
completion-at-point-functions. PROPS is a tail not a list by itself:)

Thanks, that definitely solves the problem, no need for another version
of completion function.

Vitalie.