[CodeComplete] Filter override completions by function name

[ilya-biryukov]

Summary

We put only part of the signature starting with a function name into "typed text"
chunks now, previously the whole signature was "typed text".

This leads to meaningful fuzzy match scores, giving better signals to
compare with other completion items.

Ideally, we would not display the result type to the user, but that requires adding
a new kind of completion chunk.

[ilya-biryukov]

Will need to update the test before landing, but wanted to share the implementation right away

[ilya-biryukov]

• Use the old name for the helper function

[ilya-biryukov]

• Update the tests

[ilya-biryukov]

• Simplify negative tests a bit

[ilya-biryukov]

Sorry for so many changes, this is ready for review now!

[kadircet]

clang/lib/Sema/SemaCodeComplete.cpp

3154 ↗

(On Diff #200927)

change name to printOverrideString

3166 ↗

(On Diff #200927)

I believe it is sensible to make resulttype a typed text as well, because people might start typing the function signature instead of name if they are not familiar with the feature

3182 ↗

(On Diff #200927)

I believe we should make override a typed text as well.

clang/test/CodeCompletion/overrides.cpp

23 ↗

(On Diff #200927)

either restore this to original version(if we decide to make return type a typed_text as well) or get rid of the void keyword completely and only keep v? because at first sight it looks like we are triggering on return type :D

26 ↗

(On Diff #200927)

why move this ?

[ilya-biryukov]

clang/lib/Sema/SemaCodeComplete.cpp

3166 ↗

(On Diff #200927)

Yeah, I can see why that looks appealing (return type is the prefix of completion item, so it feels most natural to type it), but that's exactly what renders the fuzzy match scores unreliable. Other items are matched by name, so to give the match scores a meaningful interpretation across different completion items we should match by name whenever we can.

Reasons why it felt like matching names instead of return types is better:

1. some return types are incredibly common, e.g. bool and void. If they are part of the typed text, the users can either (1) type void or bool and get too many results (tried that in PPCallbacks descendants) or (2) type the function name and get the override completions will be penalized because the name of the function is too far away in the text we try to match.
2. it feels like remembering names of functions you want to override is more common than remembering return types of functions you to override.

3182 ↗

(On Diff #200927)

Agree that would be nice and I actually tried it. But it didn't work: we can only have one typed text chunk, e.g. clangd will break if there are more than one and maybe other tools have this assumption too.

Would be nice to allow this (by fixing the code to not have this assumption, I guess), but I'd avoid doing this in this patch. Should I add a FIXME?

clang/test/CodeCompletion/overrides.cpp

26 ↗

(On Diff #200927)

Order matters for -NOT matches and I believe it was wrong before (given checks marked with CHECK-CC1)

E.g.

// CHECK: a
// CHECK-NOT: b

will match without error against

b
a

and give an error on

b
a

[ilya-biryukov]

Summarizing the offline discussion, the final results we want in the long run is a completion item of the form:

• Displayed to the user: override foo(int a, int b)
• Inserted into the editor: return_type foo(int a, int b) override
• Filtered by override foo (that allows to filter with override or foo, giving reasonably good ranking)

It's almost possible to achieve this with the current abstractions, but we don't have completion string chunks that are printed but not shown to the user (we need those for return_type and override at the of the completion label).

[ilya-biryukov]

• Make first letter of the helper function lowercase
• New model: everything before name is a text chunk, everything after it is typed chunk
• Test the filter text produced by clangd

[ilya-biryukov]

I think that's a good step forward, although not yet ideal. The typed chunk now contains everything starting function name and ending with override, so one gets both nice prefix match scores when typing a function and possibility to say override to get all completion items that do overrides.

clang/test/CodeCompletion/overrides.cpp

29 ↗

(On Diff #201000)

I've removed this to avoid dealing with the error return code (vfo is unresolved, so clang produces error there).
Happy to bring it back if you feel it's important

[ilya-biryukov]

After landing this, will try to add new presentation options for completion items here.

[kadircet]

LGTM

Have one question though, does it improve behavior in vscode? Since label seems to be the same, it will most definitely improve clangd's ranking but vscode ignores it anyway.

clang/lib/Sema/SemaCodeComplete.cpp

3165 ↗

(On Diff #201000)

why not just put SeenTypedChunk |= Chunk.Kind == CodeCompletionString::CK_TypedText

3172 ↗

(On Diff #201000)

do we expect anything but return type in the BeforeName or any case where we shouldn't put a space between BeforeName and NameAndSignature ?

why not let the user add a space while concatenating these two?

3191 ↗

(On Diff #201000)

let's move this into printOverrideString

clang/test/CodeCompletion/overrides.cpp

14 ↗

(On Diff #201000)

nit: I suppose it should be vfu?(same thing for the comments below starting with Runs completion...

29 ↗

(On Diff #201000)

it was here to prevent a regression maybe trigger it on the 13th line instead?

[ilya-biryukov]

In D62298#1515399, @kadircet wrote:

LGTM

Have one question though, does it improve behavior in vscode? Since label seems to be the same, it will most definitely improve clangd's ranking but vscode ignores it anyway.

It does, VSCode relies on filterText for ranking and since it now starts with the function name the ranking is much better.

Will fix the rest of the comments and land this.

[ilya-biryukov]

clang/lib/Sema/SemaCodeComplete.cpp

3172 ↗

(On Diff #201000)

Maybe annotations? But not sure where they go (also not sure we should print them here).
I could move adding the space to the point where we concatenate BeforeName and NameAndSignature if that would make it clearer.

[kadircet]

clang/lib/Sema/SemaCodeComplete.cpp

3172 ↗

(On Diff #201000)

I usually find it annoying when some helper function returns strings with trailing whitespaces, which is not useful in the general case. (Faced those a lot during HoverInfo patch.) I believe it would make the function more usable to others, so let's make it part of concat logic.

[ilya-biryukov]

• Address comments

clang/test/CodeCompletion/overrides.cpp

14 ↗

(On Diff #201000)

No, the idea was to test the vfunc item prefix-matching on vfo but still matches on vf

[ilya-biryukov]

• Add whitespace outside printOverrideString

[ilya-biryukov]

clang/lib/Sema/SemaCodeComplete.cpp

3172 ↗

(On Diff #201000)

Agree, it's cleaner that way.

[ilya-biryukov]

• Do not add an extra 'override' on optional chunks

[ilya-biryukov]

• Remove redundant test

[ilya-biryukov]

• Update a comment in the test
• Reformat the code

clangd/unittests/CodeCompleteTests.cpp

 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void onDiagnosticsReady(PathRef File,
                           std::vector<Diag> Diagnostics) override {}
 };
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.Name == Name; }
+MATCHER_P(NameStartsWith, Prefix, "") {
+  return llvm::StringRef(arg.Name).startswith(Prefix);
+}
 MATCHER_P(Scope, S, "") { return arg.Scope == S; }
 MATCHER_P(Qualifier, Q, "") { return arg.RequiredQualifier == Q; }
 MATCHER_P(Labeled, Label, "") {
   return arg.RequiredQualifier + arg.Name + arg.Signature == Label;
 }
 MATCHER_P(SigHelpLabeled, Label, "") { return arg.label == Label; }
 MATCHER_P(Kind, K, "") { return arg.Kind == K; }
 MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
TEST(CompletionTest, SuggestOverrides) {
   };
   class C : public B {
    public:
     void vfunc(bool param) override;
     ^
   };
   )cpp");
   const auto Results = completions(Text);
-  EXPECT_THAT(Results.Completions,
-              AllOf(Contains(Labeled("void vfunc(bool param, int p) override")),
-                    Contains(Labeled("void ttt(bool param) const override")),
-                    Not(Contains(Labeled("void vfunc(bool param) override")))));
+  EXPECT_THAT(
+      Results.Completions,
+      AllOf(Contains(AllOf(Labeled("void vfunc(bool param, int p) override"),
+                           NameStartsWith("vfunc"))),
+            Contains(AllOf(Labeled("void ttt(bool param) const override"),
+                           NameStartsWith("ttt"))),
 }
 
 TEST(CompletionTest, OverridesNonIdentName) {
   // Check the completions call does not crash.
   completions(R"cpp(
     struct Base {
       virtual ~Base() = 0;
       virtual operator int() = 0;