bug#16604: 24.3.50; False negatives in lisp-completion-at-point

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

Example:

(defmacro fizzle (&rest body)
  (declare (indent defun))
  `(let ((coooooooon 0))
     ,@body))

(defun fozzle ()
  (fizzle
    (foo co)))

1. Evaluate the macro.

2. Put point after "co" in fozzle, press C-M-i.

3. See it get automatically completed to "coooooooon", even though there
are many dynamic variables that start with "co".


In GNU Emacs 24.3.50.4 (x86_64-unknown-linux-gnu, GTK+ Version 3.8.6)
 of 2014-01-23 on axl
Repository revision: 116128 dmantipov@yandex.ru-20140123121808-f1qhuudj4oy3bxbi
Windowing system distributor `The X.Org Foundation', version 11.0.11405000
System Description:	Ubuntu 13.10

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> 3. See it get automatically completed to "coooooooon", even though there
> are many dynamic variables that start with "co".

Right, the completion first tries locally let-bound vars.  It only tries
to complete against global vars once this fails.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

On 31.01.2014 16:34, Stefan Monnier wrote:
>> 3. See it get automatically completed to "coooooooon", even though there
>> are many dynamic variables that start with "co".
>
> Right, the completion first tries locally let-bound vars.  It only tries
> to complete against global vars once this fails.

So this is the intended behavior? I don't like it.

I'd rather it offered both as completions, but put the local vars at the 
top (not sure if that's possible with completion-at-point).

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Thierry Volpiatto]

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

>> 3. See it get automatically completed to "coooooooon", even though there
>> are many dynamic variables that start with "co".
>
> Right, the completion first tries locally let-bound vars.  It only tries
> to complete against global vars once this fails.

Very bad IMO, what you do here is assuming the user wants to complete
against the let bounded var, which is maybe not the case.

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

Is it okay to install the following patch?

On a related note, company-capf returns nil to `duplicates', but with 
the patch below, for example, I do receive duplicates from it. And with 
another, functional completion table I wrote in a third-party package. 
Should I change `duplicates' to t?

=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-01-01 07:43:34 +0000
+++ lisp/emacs-lisp/lisp.el	2014-02-02 01:42:32 +0000
@@ -830,7 +830,7 @@
                  ;; use it to provide a more specific completion table 
in some
                  ;; cases.  E.g. filter out keywords that are not 
understood by
                  ;; the macro/function being called.
-                (list nil (completion-table-in-turn
+                (list nil (completion-table-merge
                             lisp--local-variables-completion-table
                             obarray)       ;Could be anything.
                        :annotation-function

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-01-07 23:36:29 +0000
+++ lisp/minibuffer.el	2014-02-02 01:46:41 +0000
@@ -393,6 +393,27 @@
                          (complete-with-action action table string pred))
                        tables)))

+(defun completion-table-merge (&rest tables)
+  "Create a completion table that collects completions from all TABLES."
+  ;; FIXME: same caveat as in `completion-table-in-turn', only harder
+  ;; to fix.
+  (lambda (string pred action)
+    (cond
+     ((null action)
+      (try-completion string
+                      (mapcar (lambda (table)
+                                (try-completion string table pred))
+                              tables)
+                      pred))
+     ((eq action t)
+      (apply #'append (mapcar (lambda (table)
+                                (all-completions string table pred))
+                              tables)))
+     ((not (or (eq (car-safe action) 'boundaries)
+               (eq action 'metadata)))
+      (completion--some (lambda (table) (test-completion string table 
pred))
+                        tables)))))
+
  (defun completion-table-with-quoting (table unquote requote)
    ;; A difficult part of completion-with-quoting is to map positions 
in the
    ;; quoted string to equivalent positions in the unquoted string and

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> Is it okay to install the following patch?

I think it should wait.  I also think it should be configurable (I
definitely prefer the current behavior; in my use cases your behavior
makes the lisp--local-variables-completion-table pretty much useless
because there's almost always some other global variable that starts
with a similar prefix).

> On a related note, company-capf returns nil to `duplicates', but with the
> patch below, for example, I do receive duplicates from it.  And with another,
> functional completion table I wrote in a third-party package.  Should
> I change `duplicates' to t?

Hmm... I think so, yes.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

On 02.02.2014 04:39, Stefan Monnier wrote:
> I think it should wait.

Pity. It's a regression compared to company-elisp, as far as I'm concerned.

> I also think it should be configurable (I
> definitely prefer the current behavior; in my use cases your behavior
> makes the lisp--local-variables-completion-table pretty much useless
> because there's almost always some other global variable that starts
> with a similar prefix).

True, lisp--local-variables-completion-table is less useful this way, 
but it still plays a part when the binding form hasn't been evaluated 
yet, and so the local variable symbols aren't yet in obarray.

If you were using Company, by the way, you could take advantage of the 
ordering of candidates based on their occurrences in the visible part of 
the buffer, which will be supported in the next version. Naturally, 
local vars will be at the top, as long as they're visible in the window.

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> Pity. It's a regression compared to company-elisp, as far as I'm concerned.

You can still use company-elisp in the mean time.

>> I also think it should be configurable (I definitely prefer the
>> current behavior; in my use cases your behavior makes the
>> lisp--local-variables-completion-table pretty much useless because
>> there's almost always some other global variable that starts with
>> a similar prefix).
> True, lisp--local-variables-completion-table is less useful this way, but it
> still plays a part when the binding form hasn't been evaluated yet, and so
> the local variable symbols aren't yet in obarray.

I guess it's true that if you choose among a list of completions (or
if you use completion cycling), the extra elements from obarray aren't
nearly as problematic as if you rely on just "plain completion".


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> I guess it's true that if you choose among a list of completions (or
> if you use completion cycling), the extra elements from obarray aren't
> nearly as problematic as if you rely on just "plain completion".

So maybe "merge" is indeed a better default than "in-turn".

The behavior I'm looking for in completion-at-point should probably be
obtained more along the lines of what is done in file-name completion
where completion-ignored-extensions is used before calling
try-completion.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> So maybe "merge" is indeed a better default than "in-turn".

I guess it means it might be worth trying to fix it for 24.4.
But your completion-table-merge needs to be improved: the list passed to
the outer `try-completion' needs to only hold strings, whereas the inner
`try-completion's can return nil or t.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

>> So maybe "merge" is indeed a better default than "in-turn".
> I guess it means it might be worth trying to fix it for 24.4.
> But your completion-table-merge needs to be improved: the list passed to
> the outer `try-completion' needs to only hold strings, whereas the inner
> `try-completion's can return nil or t.

Also, the outer try-completion could end up returning t if one of the
inner ones returns the string we're trying to complete.
We might be better off always going through "(apply #'append (mapcar
#'all-completions ...))", e.g. via completion-table-dynamic.

Also, we should try and check that the sub-tables all have "trivial"
boundaries, and no quoting.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

On 02.02.2014 20:24, Stefan Monnier wrote:
> The behavior I'm looking for in completion-at-point should probably be
> obtained more along the lines of what is done in file-name completion
> where completion-ignored-extensions is used before calling
> try-completion.

I'm not sure I understand. Would there be a variable defining a regexp 
or glob for variable and function names that will be ignored unless they 
are the only candidates?

 > Also, we should try and check that the sub-tables all have "trivial"
boundaries, and no quoting.

Do we do that at "runtime" (after the lambda has been returned)? And 
just blow up calls with action `metadata' or `boundaries . ...' with 
error whenever that's not true?

I figured just documenting problematic cases might be enough (like 
`completion-table-in-turn' does, I suppose it has a similar problem with 
quoting).

 > Also, the outer try-completion could end up returning t if one of the
inner ones returns the string we're trying to complete.
We might be better off always going through "(apply #'append (mapcar
#'all-completions ...))", e.g. via completion-table-dynamic.

Sounds not very efficient. See the updated patch, does this look right 
to you?

I have a hard time testing it, though. lisp-completion-at-point seems to 
suggest any symbols that I've ever typed anyway, so there's no way to 
check that lisp--local-variables-completion-table is even used.


=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-01-01 07:43:34 +0000
+++ lisp/emacs-lisp/lisp.el	2014-02-02 01:42:32 +0000
@@ -830,7 +830,7 @@
                  ;; use it to provide a more specific completion table 
in some
                  ;; cases.  E.g. filter out keywords that are not 
understood by
                  ;; the macro/function being called.
-                (list nil (completion-table-in-turn
+                (list nil (completion-table-merge
                             lisp--local-variables-completion-table
                             obarray)       ;Could be anything.
                        :annotation-function

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-01-07 23:36:29 +0000
+++ lisp/minibuffer.el	2014-02-04 05:25:30 +0000
@@ -393,6 +393,36 @@
                          (complete-with-action action table string pred))
                        tables)))

+(defun completion-table-merge (&rest tables)
+  "Create a completion table that collects completions from all TABLES."
+  ;; FIXME: same caveat as in `completion-table-in-turn', only harder
+  ;; to fix.
+  (lambda (string pred action)
+    (cond
+     ((null action)
+      (let ((retvals (mapcar (lambda (table)
+                               (try-completion string table pred))
+                             tables))
+            (prelim (try-completion string retvals pred)))
+        (cond
+         ((and (stringp prelim) (not (memq t retvals))) prelim)
+         ((null prelim) (and (memq t retvals) t))
+         ;; Here `prelim' is either t, and that means there's at least
+         ;; one string in `retvals', and all of them are equal to
+         ;; STRING.
+         ;; Or `prelim' is a string, but there's a `t' in `retvals',
+         ;; which means those matches won't all match `prelim'.
+         (t string))))
+     ((eq action t)
+
+      (apply #'append (mapcar (lambda (table)
+                                (all-completions string table pred))
+                              tables)))
+     (t
+      (completion--some (lambda (table)
+                          (complete-with-action action table string pred))
+                        tables)))))
+
  (defun completion-table-with-quoting (table unquote requote)
    ;; A difficult part of completion-with-quoting is to map positions 
in the
    ;; quoted string to equivalent positions in the unquoted string and

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> I'm not sure I understand. Would there be a variable defining a regexp or
> glob for variable and function names that will be ignored unless they are
> the only candidates?

I was thinking of a new property which would define a function which
reduces the set of candidates considered for
completion-try-completions.  File completion could use it to implement
completion-ignored-extensions and we could use it here to prefer
a local varname.

>> Also, we should try and check that the sub-tables all have "trivial"
>> boundaries, and no quoting.

> Do we do that at "runtime" (after the lambda has been returned)? And just
> blow up calls with action `metadata' or `boundaries . ...' with error
> whenever that's not true?

We don't want to do those checks all the time, so it should probably be
done only once when we combine the two tables (if possible) or not at all.

> I figured just documenting problematic cases might be enough (like
> completion-table-in-turn' does, I suppose it has a similar problem with
> quoting).

Right.

> Sounds not very efficient.

Probably lost in the noise.

> See the updated patch, does this look right to you?

Could be, see below.

> I have a hard time testing it, though. lisp-completion-at-point seems to
> suggest any symbols that I've ever typed anyway, so there's no way to check
> that lisp--local-variables-completion-table is even used.

Sounds like a bug in lisp-completion-at-point.

> +      (let ((retvals (mapcar (lambda (table)
> +                               (try-completion string table pred))
> +                             tables))
> +            (prelim (try-completion string retvals pred)))

try-completion's behavior when passed a list mixing strings and symbols
is not really defined.  So this second call to `try-completion' relies
on largely undocumented behavior.  Two solutions: either you add
a comment about what behavior you assume, or you change the code to
avoid this problem.

E.g. change retvals with (delq nil ...) and replace t with `string'.


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

On 04.02.2014 19:54, Stefan Monnier wrote:
> I was thinking of a new property which would define a function which
> reduces the set of candidates considered for
> completion-try-completions.  File completion could use it to implement
> completion-ignored-extensions and we could use it here to prefer
> a local varname.

An approach that would work for completion-at-point, but wouldn't touch 
compan-capf? Sounds promising, though out of scope for this bug.

> We don't want to do those checks all the time, so it should probably be
> done only once when we combine the two tables (if possible)

`completion-metadata' requires both input string and a predicate (I 
guess, for maximum flexibility of functional completion tables). Pass it 
an empty string and a nil?

And completion boundaries requires suffix as an input. I guess we could 
see if the table supports boundaries at all by passing an empty string, 
and if so, raise an error (ignoring the possibility that the table 
supports only a certain set of suffixes). In that case, we'd be breaking 
support for merging tables that define boundaries, but are known by the 
caller to return identical ones. Which could be useful, I guess.

> or not at all.

Sounds good to me.

> Sounds like a bug in lisp-completion-at-point.

See http://debbugs.gnu.org/16646.

>> +      (let ((retvals (mapcar (lambda (table)
>> +                               (try-completion string table pred))
>> +                             tables))
>> +            (prelim (try-completion string retvals pred)))
>
> try-completion's behavior when passed a list mixing strings and symbols
> is not really defined.  So this second call to `try-completion' relies
> on largely undocumented behavior.  Two solutions: either you add
> a comment about what behavior you assume, or you change the code to
> avoid this problem.
>
> E.g. change retvals with (delq nil ...) and replace t with `string'.

Sure, thanks for catching this.


=== modified file 'lisp/emacs-lisp/lisp.el'
--- lisp/emacs-lisp/lisp.el	2014-01-01 07:43:34 +0000
+++ lisp/emacs-lisp/lisp.el	2014-02-02 01:42:32 +0000
@@ -830,7 +830,7 @@
                  ;; use it to provide a more specific completion table 
in some
                  ;; cases.  E.g. filter out keywords that are not 
understood by
                  ;; the macro/function being called.
-                (list nil (completion-table-in-turn
+                (list nil (completion-table-merge
                             lisp--local-variables-completion-table
                             obarray)       ;Could be anything.
                        :annotation-function

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-01-07 23:36:29 +0000
+++ lisp/minibuffer.el	2014-02-05 04:38:11 +0000
@@ -388,11 +388,38 @@
    "Create a completion table that tries each table in TABLES in turn."
    ;; FIXME: the boundaries may come from TABLE1 even when the 
completion list
    ;; is returned by TABLE2 (because TABLE1 returned an empty list).
+  ;; Same potential problem if any of the tables use quoting.
    (lambda (string pred action)
      (completion--some (lambda (table)
                          (complete-with-action action table string pred))
                        tables)))

+(defun completion-table-merge (&rest tables)
+  "Create a completion table that collects completions from all TABLES."
+  ;; FIXME: same caveats as in `completion-table-in-turn', only harder
+  ;; to fix.
+  (lambda (string pred action)
+    (cond
+     ((null action)
+      (let ((retvals (mapcar (lambda (table)
+                               (try-completion string table pred))
+                             tables)))
+        (if (member string retvals)
+            string
+          (try-completion string
+                          (mapcar (lambda (value)
+                                    (if (eq value t) string value))
+                                  (delq nil retvals))
+                          pred))))
+     ((eq action t)
+      (apply #'append (mapcar (lambda (table)
+                                (all-completions string table pred))
+                              tables)))
+     (t
+      (completion--some (lambda (table)
+                          (complete-with-action action table string pred))
+                        tables)))))
+
  (defun completion-table-with-quoting (table unquote requote)
    ;; A difficult part of completion-with-quoting is to map positions 
in the
    ;; quoted string to equivalent positions in the unquoted string and

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Stefan Monnier]

> `completion-metadata' requires both input string and a predicate (I guess,
> for maximum flexibility of functional completion tables).  Pass it an empty
> string and a nil?

Right.

> And completion boundaries requires suffix as an input.

The suffix is not a problem.  But the prefix is, because with an empty
string, the boundary will always be zero.  And we can't disallow tables
that return an explicit 0 boundary.

>> or not at all.
> Sounds good to me.

Yup, sounds pretty good.

>> Sounds like a bug in lisp-completion-at-point.
> See http://debbugs.gnu.org/16646.

Thanks.

> Sure, thanks for catching this.

Please install, thank you,


        Stefan

bug#16604: 24.3.50; False negatives in lisp-completion-at-point

[Dmitry Gutov]

Version: 24.4

On 05.02.2014 15:53, Stefan Monnier wrote:
> The suffix is not a problem.  But the prefix is, because with an empty
> string, the boundary will always be zero.  And we can't disallow tables
> that return an explicit 0 boundary.

True.

> Please install, thank you,

Revision 116279.