bug#42149: Substring and flex completion ignore implicit trailing ‘any’

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi,

I have found out that substring and flex completion ignore the implicit
trailing ‘any’ introduced by ‘completion-pcm--optimize-pattern’.  This
is evident from the examples shown next.

My Emacs version is 28.0.50, built on 2020-07-01 from commit e98ddd6fc1.

Example 1
=========

  (completion-substring-all-completions "f" (list "f") nil 1)

and

  (completion-flex-all-completions "f" (list "f") nil 1)

both result in

  (#("f" 0 1 (face completions-common-part completion-score 0.0)) . 0)

whereas I would expect a completion score of 1.

Example 2
=========

  (completion-substring-all-completions "fo" (list "fo") nil 1)

results in

  (#("fo" 0 1 (face completions-common-part completion-score 0.5) 1 2
    (face (completions-first-difference completions-common-part))) . 0)

whereas I would again expect a completion score of 1.

Proposed Solution
=================

I propose that we make the implicit trailing ‘any’ explicit in
‘completion-substring--all-completions’.


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index d2c3f9045e..a598b1d1fd 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3585,10 +3585,12 @@ that is non-nil."
          (pattern (if (not (stringp (car basic-pattern)))
                       basic-pattern
                     (cons 'prefix basic-pattern)))
-         (pattern (completion-pcm--optimize-pattern
-                   (if transform-pattern-fn
-                       (funcall transform-pattern-fn pattern)
-                     pattern)))
+         (pattern (append
+                   (completion-pcm--optimize-pattern
+                    (if transform-pattern-fn
+                        (funcall transform-pattern-fn pattern)
+                      pattern))
+                   '(any)))             ; make implicit `any' explicit
          (all (completion-pcm--all-completions prefix pattern table pred)))
     (list all pattern prefix suffix (car bounds))))
 


This fixes the problem and seems to perform well from my testing.
However, I have no idea if I am overlooking something, so please let me
know.

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
Dario Gjorgjevski <[hidden email]> writes:

> Hi,
>
> I have found out that substring and flex completion ignore the implicit
> trailing ‘any’ introduced by ‘completion-pcm--optimize-pattern’.  This
> is evident from the examples shown next.
>
> My Emacs version is 28.0.50, built on 2020-07-01 from commit e98ddd6fc1.
>
> Example 1
> =========
>
>   (completion-substring-all-completions "f" (list "f") nil 1)
>
> and
>
>   (completion-flex-all-completions "f" (list "f") nil 1)
>
> both result in
>
>   (#("f" 0 1 (face completions-common-part completion-score 0.0)) . 0)
>
> whereas I would expect a completion score of 1.

Is it the fact that the completion-score is 0 that causes the earlier
problems you experienced?

> Example 2
> =========
>
>   (completion-substring-all-completions "fo" (list "fo") nil 1)
>
> results in
>
>   (#("fo" 0 1 (face completions-common-part completion-score 0.5) 1 2
>     (face (completions-first-difference completions-common-part))) . 0)
>
> whereas I would again expect a completion score of 1.
>
> Proposed Solution
> =================
>
> I propose that we make the implicit trailing ‘any’ explicit in
> ‘completion-substring--all-completions’.
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index d2c3f9045e..a598b1d1fd 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3585,10 +3585,12 @@ that is non-nil."
>           (pattern (if (not (stringp (car basic-pattern)))
>                        basic-pattern
>                      (cons 'prefix basic-pattern)))
> -         (pattern (completion-pcm--optimize-pattern
> -                   (if transform-pattern-fn
> -                       (funcall transform-pattern-fn pattern)
> -                     pattern)))
> +         (pattern (append
> +                   (completion-pcm--optimize-pattern
> +                    (if transform-pattern-fn
> +                        (funcall transform-pattern-fn pattern)
> +                      pattern))
> +                   '(any)))             ; make implicit `any' explicit
>           (all (completion-pcm--all-completions prefix pattern table pred)))
>      (list all pattern prefix suffix (car bounds))))
>  
>
>
> This fixes the problem and seems to perform well from my testing.
> However, I have no idea if I am overlooking something, so please let me
> know.

You analysis is good and it does reveal a quirk somewhere.  However, for
now, completion scores are relative things, i.e. they an absolute value
has no meaning by its own.

I can therefore understand that "t" disappears from your completion list
(goes to the very end) given that something else has non-zero score,
like "footrix".

But does the problem also manifest itself with two-character
completions?  I.e. is the 0.5 perfect match for "fo" (in the example you
gave) ever surpassed by another, presumably less good, match?

So I'd have to look better.  Either

- The bug is where you say it is, and the fix should go where you say it
  does.

- Some numeric adjust should be made to the algorithm
  completion-pcm--hilit-commonality.




Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
In reply to this post by Dario Gjorgjevski
Dario Gjorgjevski <[hidden email]> writes:

> > Hi,
> >
> > I have found out that substring and flex completion ignore the implicit
> > trailing ‘any’ introduced by ‘completion-pcm--optimize-pattern’.  This
> > is evident from the examples shown next.
> >
> > My Emacs version is 28.0.50, built on 2020-07-01 from commit e98ddd6fc1.
> >
> > Example 1
> > =========
> >
> >   (completion-substring-all-completions "f" (list "f") nil 1)
> >
> > and
> >
> >   (completion-flex-all-completions "f" (list "f") nil 1)
> >
> > both result in
> >
> >   (#("f" 0 1 (face completions-common-part completion-score 0.0)) . 0)
> >
> > whereas I would expect a completion score of 1.
> >
> > Example 2
> > =========
> >
> >   (completion-substring-all-completions "fo" (list "fo") nil 1)
> >
> > results in
> >
> >   (#("fo" 0 1 (face completions-common-part completion-score 0.5) 1 2
> >     (face (completions-first-difference completions-common-part))) . 0)
> >
> But does the problem also manifest itself with two-character
> completions?  I.e. is the 0.5 perfect match for "fo" (in the example you
> gave) ever surpassed by another, presumably less good, match?

Answering my own question, the answer seems to be "no".

   (completion-substring-all-completions "fo" (list "f" "fo" "foot") nil 1)
   (#("fo" 0 1
      (face completions-common-part completion-score 0.5)
      1 2
      (face
       (completions-first-difference completions-common-part)))
    #("foot" 0 1
      (face completions-common-part completion-score 0.25)
      1 2
      (face
       (completions-first-difference completions-common-part)))
    . 0)


But indeed there is the problem of the 1-long.  And the problem is that
_every_ completion gets 0.

   (completion-substring-all-completions "f" (list "f" "fo" "foot") nil 1)
   (#("f" 0 1
      (face completions-common-part completion-score 0.0))
    #("fo" 0 1
      (face completions-common-part completion-score 0.0)
      1 2
      (face completions-first-difference))
    #("foot" 0 1
      (face completions-common-part completion-score 0.0)
      1 2
      (face completions-first-difference))
    . 0)

I still don't know what the proper fix this, just adding some
information I think is relevant.

Thanks,
João



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João,

> I still don't know what the proper fix this, just adding some
> information I think is relevant.

Indeed the problem is that they all get a completion score of 0, and I
would expect the exact match to get a score of 1.

You’re right that we can also modify the algorithm of
‘completion-pcm--hilit-commonality’.


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index d2c3f9045e..e1f1ffed1c 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3251,6 +3251,9 @@ one-letter-long matches).")
                 (update-score
                  (lambda (a b)
                    "Update score variables given match range (A B)."
+   (add-face-text-property a b
+   'completions-common-part
+   nil str)
                    (setq
                     score-numerator   (+ score-numerator (- b a)))
                    (unless (or (= a last-b)
@@ -3264,19 +3267,10 @@ one-letter-long matches).")
                                                     flex-score-match-tightness)))))
                    (setq
                     last-b              b))))
-           (funcall update-score start start)
            (while md
-             (funcall update-score start (car md))
-             (add-face-text-property
-              start (pop md)
-              'completions-common-part
-              nil str)
+             (funcall update-score start (pop md))
              (setq start (pop md)))
-           (funcall update-score len len)
-           (add-face-text-property
-            start end
-            'completions-common-part
-            nil str)
+           (funcall update-score start end)
            (if (> (length str) pos)
                (add-face-text-property
                 pos (1+ pos)

This modification also solves the issue (and simplifies the code a
little bit), but I’m not sure of unwanted side effects.

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João, hi everyone,

Is there any progress on this?

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
On Tue, Sep 8, 2020 at 10:05 AM Dario Gjorgjevski <[hidden email]> wrote:
Hi João, hi everyone,

Is there any progress on this?

I don't think so. Could you (re)state the use case where this matters?
That helps prioritize it.

João
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
> I don't think so. Could you (re)state the use case where this matters?
> That helps prioritize it.

Hi João,

My use case is very simple: I have a directory which, among others,
contains files named 1, 2, etc.  It’s really annoying that C-x C-f 2 RET
does not find 2 but instead finds some other file containing 2 in its
name.

Also, I can’t start an R process by doing M-x R RET.  (You need to have
R and ESS installed for this, of course.)

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora


On Tue, Sep 8, 2020 at 10:44 AM Dario Gjorgjevski <[hidden email]> wrote:
> I don't think so. Could you (re)state the use case where this matters?
> That helps prioritize it.

Hi João,

My use case is very simple: I have a directory which, among others,
contains files named 1, 2, etc.  It’s really annoying that C-x C-f 2 RET
does not find 2 but instead finds some other file containing 2 in its
name.

Is this is vanilla emacs, or are you using some icomplete-mode or fido-mode?
I.e. can you post the entire Emacs -Q recipe?
 
Also, I can’t start an R process by doing M-x R RET.  (You need to have
R and ESS installed for this, of course.)

Same question. If you're using, say, fido-mode, you can probably fix this by typing
M-j instead of RET in these one-letter completion cases. Or even C-u M-j, if that
doesn't work.

João
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
> Is this is vanilla emacs, or are you using some icomplete-mode or
> fido-mode?
> I.e. can you post the entire Emacs -Q recipe?

It is really simple to reproduce.

0. Run ‘emacs -Q’ in a directory with two files: “1” and “foo1”.
1. Enable fido-mode.
2. C-x C-f 1 RET.

Emacs will open “foo1” even though I would expect it to open “1” as that
is an exact match.  But, I really think this is a pointless discussion.

The issue *is not caused by fido-mode* but by the mechanism of substring
(and therefore, flex) completion.  You can also trigger it without
fido-mode by invoking minibuffer-force-complete-and-exit.

    (completion-flex-all-completions "1" '("1" "11" "1122") nil 1)
    (completion-substring-all-completions "1" '("1" "11" "1122") nil 1)

Both return completely the nonsensical result of

    (#("1"
       0 1 (face completions-common-part completion-score 0.0))
     #("11"
       0 1 (face completions-common-part completion-score 0.0)
       1 2 (face completions-first-difference))
     #("1122"
       0 1 (face completions-common-part completion-score 0.0)
       1 2 (face completions-first-difference))
     . 0)

(Why are all the completion-score values 0?)  Applying the attached
patch changes the result to

    (#("1"
       0 1 (face completions-common-part completion-score 1.0))
     #("11"
       0 1 (face completions-common-part completion-score 0.5)
       1 2 (face completions-first-difference))
     #("1122"
       0 1 (face completions-common-part completion-score 0.25)
       1 2 (face completions-first-difference))
     . 0)

which I hope you would agree makes a lot more sense.

> M-j instead of RET in these one-letter completion cases. Or even C-u
> M-j, if that doesn't work.

Sure, but my muscle memory opposes that.

Best regards,
Dario



--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

completion-substring--all-completions.diff (548 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
Dario Gjorgjevski <[hidden email]> writes:

>> Is this is vanilla emacs, or are you using some icomplete-mode or
>> fido-mode?
>> I.e. can you post the entire Emacs -Q recipe?
>
> It is really simple to reproduce.
>
> 0. Run ‘emacs -Q’ in a directory with two files: “1” and “foo1”.
> 1. Enable fido-mode.
> 2. C-x C-f 1 RET.
>
> Emacs will open “foo1” even though I would expect it to open “1” as that
> is an exact match.  But, I really think this is a pointless
> discussion.

It's not.  Before, you hadn't mentioned fido-mode.  As I told you, I
need to assess the priority of this bug, in terms of whom it affects and
to what intensity.

> The issue *is not caused by fido-mode* but by the mechanism of substring
> (and therefore, flex) completion.

Obviously, I'm not disputing that in any way.

>> M-j instead of RET in these one-letter completion cases. Or even C-u
>> M-j, if that doesn't work.
>
> Sure, but my muscle memory opposes that.

I'm sorry, but that's the only workaround I can think of.  Bear in mind
that, when using fido-mode, you need to be aware of that shortcut anyway
in some situations where the text you want to input is not among the
completions.  C-c C-w to write a new file would likely be the most
prominent example.

To summarize, I believe this isn't a very common case, and it is easily
circumvented with the workaround I gave you.  I of course acknowledge
this bug report as a bug.  Maybe you can try making a patch for Emacs
that fixes it and test it.

João



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
On Tue, Sep 8, 2020 at 12:30 PM Dario Gjorgjevski <[hidden email]> wrote:
> To summarize, I believe this isn't a very common case, and it is easily
> circumvented with the workaround I gave you.  I of course acknowledge
> this bug report as a bug.  Maybe you can try making a patch for Emacs
> that fixes it and test it.

Thanks, I’ll look into it some more and submit a patch.

To be honest, it seems that the issue is also exhibited by
completion-pcm-all-completions, so the best place to fix it would be
completion-pcm--hilit-commonality.
 
Sounds good. I'm adding Stefan Monnier to this conversation
as he is expert in these matters.

João
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João, hi Stefan,

Please find attached a patch that fixes this issue and optimizes the
scoring in PCM completion a bit.  As far as I can tell, this works fine
and can be merged, but please have a look and do some testing yourself.



Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

0001-Fix-and-optimize-scoring-in-PCM-completion.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João, hi Stefan,

Unfortunately, I found some bugs in the patch I shared above.  While it
works well for substring and flex completion (with the minor caveat of
ignoring the point’s position which I will take care of), it breaks
under vanilla PCM completion.  I will try to fix it.

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
> BTW, this code sorely needs tests, so when you find a bug (either in
> the original code or in changes you introduced temporarily), if you
> could add corresponding tests, that would be a great help.

Hi Stefan, hi João,

Thanks a lot.  I managed to iron out the bugs in a way that I’m happy
with.  It seems to work well with everything I tested so far, and I went
ahead and added some tests.

Please have a look.



Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

0001-Fix-and-optimize-scoring-in-PCM-completion.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi Stefan, hi João,

I rebased my patch on top of the fix introduced for bug#41705 and
confirmed that it does not cause a regression.  Have you been able to
look into it?  Please let me know if you think there’s something missing
or if I should add additional tests.

I am attaching the patch below.



Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

0001-Fix-and-optimize-scoring-in-PCM-completion.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
Hi Dario,

Sorry for the delay in handling this.

I haven't had time to look into it in detail, which I must do since
it is reasonably complex.  However it looks good on the surface
and from what I remember your original bug report holds water.

I have two questions and a nitpick:

1. Question: if I compile and/or evaluate the changes to the
test/lisp/minibuffer-tests.el file but _don't_ compile the changes
to the lisp/minibuffer.el file, will they expose this bug and this
bug only?  In other words, will I get exactly the same failures
that you describe originally in this issue and will that fact be
apparent in the failure message(s)?

2. Question: are the changes to completion-pcm--optimize-pattern
an optimization or does the fix above depend on them?  If the
former, could you make it a separate commit?

3. Nitpick: the commit message is broadly according to the
format, but I find it hard to parse its intentions. Though
conventions vary, I usually like to format the commit message
like in this example which separates the what, the why and
the how.

"Fix the thing imperatively because racecar (bug#12345)

Before, when the user did foo the system stupidly behaved
bar. Now it's much better, it does baz.

I fixed this by doing quux and quzz

* file (function): use more intense quuxing."

João


Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João,

> Sorry for the delay in handling this.

No problem; it’s not a very critical bug after all, just an annoying
one.

> 1. Question: if I compile and/or evaluate the changes to the
> test/lisp/minibuffer-tests.el file but _don't_ compile the changes
> to the lisp/minibuffer.el file, will they expose this bug and this
> bug only?  In other words, will I get exactly the same failures
> that you describe originally in this issue and will that fact be
> apparent in the failure message(s)?

Yes.  As for how apparent they’ll be, well, I guess that’s up to ERT.
You will get something along the lines of

    (ert-test-failed
     ((should
       (eql
        (get-text-property 0 'completion-score
                           (car ...))
        1.0))
      :form
      (eql 0.0 1.0)
      :value nil))

Which says that the ‘completion-score’ was 0 but should have been 1, as
indicated in the bug report.  Of course, some of the tests are more
general tests for the sake of catching regressions.

> 2. Question: are the changes to completion-pcm--optimize-pattern
> an optimization or does the fix above depend on them?  If the
> former, could you make it a separate commit?

Unfortunately, the fix loosely depends on them.  Without them, having
multiple consecutive “*” would mess up the PCM scoring.

> 3. Nitpick: the commit message is broadly according to the
> format, but I find it hard to parse its intentions. Though
> conventions vary, I usually like to format the commit message
> like in this example which separates the what, the why and
> the how.

Thanks for both the remark and the useful example.  I will fix it and
come back with a new patch.

Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Hi João, hi Stefan,

Please find attached a patch with an edited commit message.  It should
be better.



Best regards,
Dario

--
[hidden email] :: +49 1525 8666837
%   gpg --keyserver 'hkps://hkps.pool.sks-keyservers.net' \
\`>     --recv-keys '744A4F0B4F1C9371'

0001-Fix-PCM-scoring-ignoring-last-part-of-query-string.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

Dario Gjorgjevski
Just a friendly bump.

Best regards,
Dario

--
$ keyserver=hkps://hkps.pool.sks-keyservers.net
$ keyid=744A4F0B4F1C9371
$ gpg --keyserver $keyserver --search-keys $keyid



Reply | Threaded
Open this post in threaded view
|

bug#42149: Substring and flex completion ignore implicit trailing ‘any’

João Távora
Indeed, sometimes a bump is needed.  I'll have a good look
as soon as possible.

João

On Fri, Nov 20, 2020 at 8:39 PM Dario Gjorgjevski <[hidden email]> wrote:
Just a friendly bump.

Best regards,
Dario

--
$ keyserver=hkps://hkps.pool.sks-keyservers.net
$ keyid=744A4F0B4F1C9371
$ gpg --keyserver $keyserver --search-keys $keyid


--
João Távora
12