[clangd] Add heuristic for dropping snippet when completing member function pointer

[tom-anders]

Summary

This implements the 1st heuristic mentioned in https://github.com/clangd/clangd/issues/968#issuecomment-1002242704:

When completing a function that names a non-static member of a class, and we are not inside that class's scope, assume the reference will not be a call (and thus don't add the snippetSuffix)

[nridge]

Thanks for the patch!

The test looks good to me.

As for the fix, I wonder if it would make sense to implement it in the Sema layer rather than in clangd. For example, we could give clang::CodeCompletionResult a new flag called something like CanBeCall, which, for a completion result that refers to a function, is set to true (by the code that creates the CodeCompletionResult, in SemaCodeComplete.cpp) if the use of the function may be a call, and false if it cannot.

Then clangd can check this flag, and clear the snippet suffix (or even better, not build it in the first place) if the value is false.

I think this approach could have a couple of advantages:

1. By placing the logic into libSema, other consumers of the Sema completion layer, besides clangd, could benefit from it as well. (A couple of other consumers that I'm aware of are libclang and cling.)
2. Because the logic now resides inside Sema itself, if we make enhancements to the heuristics for computing CanBeCall that require additional state (besides CurContext), we can just use them in place rather than having to propagate them into places like clangd's CodeCompletionBuilder.

What do you think?

[tom-anders]

In D137040#3907497, @nridge wrote:

Thanks for the patch!

The test looks good to me.

As for the fix, I wonder if it would make sense to implement it in the Sema layer rather than in clangd. For example, we could give clang::CodeCompletionResult a new flag called something like CanBeCall, which, for a completion result that refers to a function, is set to true (by the code that creates the CodeCompletionResult, in SemaCodeComplete.cpp) if the use of the function may be a call, and false if it cannot.

Then clangd can check this flag, and clear the snippet suffix (or even better, not build it in the first place) if the value is false.

I think this approach could have a couple of advantages:

1. By placing the logic into libSema, other consumers of the Sema completion layer, besides clangd, could benefit from it as well. (A couple of other consumers that I'm aware of are libclang and cling.)
2. Because the logic now resides inside Sema itself, if we make enhancements to the heuristics for computing CanBeCall that require additional state (besides CurContext), we can just use them in place rather than having to propagate them into places like clangd's CodeCompletionBuilder.

What do you think?

That indeed sounds like a better solution. I haven't looked much into SemaCodeComplete.cpp up to now, but I'll take a look and try to implement the heuristic over there.

[tom-anders]

Move logic to SemaCodeComplete.cpp

[tom-anders]

The test in clangd/unittests/CodeCompleteTests.cpp still passes. We now probably also need a unit test for Sema that tests the new flag, right?

clang-tools-extra/clangd/CodeComplete.cpp

414

Here, the SnippetSuffix is still created and then immediately cleared again. But the logic inside getSignature is quite complex and it's used in multiple places, so I didn't really want to touch that function for now.

[nridge]

In D137040#3909906, @tom-anders wrote:

We now probably also need a unit test for Sema that tests the new flag, right?

Good point. My understanding is that most test coverage of libSema's code completion takes the form of lit tests which are suitable for checking the completion proposals themselves but not flags like this. However, we do have one unittest file which could be adapted for testing the flag. It may require some light refactoring to make this work (e.g. I don't think we can just stuff the CodeCompletionResults themselves into the returned CompletionContext because they point to Decls and such whose lifetime is tied to the AST which is destroyed by the time runCompletion() returns).

clang-tools-extra/clangd/CodeComplete.cpp

414

Fair enough, we can optimize this later if it turns out to be necessary

[tom-anders]

Add test to sema

[tom-anders]

Hmm I added the test for the flag to Sema, but now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

[tom-anders]

Fix test

[nridge]

Thanks!

In D137040#3918118, @tom-anders wrote:

now we kinda have the same test case duplicated in sema and clangd tests - I guess for clangd we now actually only have to test that the SnippetSuffix is cleared when FunctionCanBeCall is true, but I don't see an easy way to somehow inject fake Sema results into CodeComplete.cpp

I don't mind the duplication too much; I like that the clangd test is an "end to end" test directly expressing the user-visible behaviour that "in these situations we should get these completions".

clang/unittests/Sema/CodeCompleteTest.cpp

123 ↗

(On Diff #474358)

Since the constructor has only one other caller (runCompletion()), I'd rather remove the default arg and put the new VisitedContextFinder(...) in that caller.

As a bonus, we would no longer need to pass the CompletionContext to the CodeCompleteAction constructor, and don't need to use a "dummy context" in CollectCompletedFunctions().

231 ↗

(On Diff #474358)

Consider using this style of matcher (example use).

[tom-anders]

Clean up test

[nridge]

Thanks!

[tom-anders]

In D137040#3924115, @nridge wrote:

Thanks!

Thanks for the review!

clang-tools-extra/clangd/CodeComplete.cpp

struct CodeCompletionBuilder {
   void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
     assert(bool(C.SemaResult) == bool(SemaCCS));
     Bundled.emplace_back();
     BundledEntry &S = Bundled.back();
     if (C.SemaResult) {
       bool IsPattern = C.SemaResult->Kind == CodeCompletionResult::RK_Pattern;
       getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix,
                    &Completion.RequiredQualifier, IsPattern);
+      if (!C.SemaResult->FunctionCanBeCall)
+        S.SnippetSuffix.clear();
       S.ReturnType = getReturnType(*SemaCCS);
     } else if (C.IndexResult) {
       S.Signature = std::string(C.IndexResult->Signature);
       S.SnippetSuffix = std::string(C.IndexResult->CompletionSnippetSuffix);
       S.ReturnType = std::string(C.IndexResult->ReturnType);
     }
     if (!Completion.Documentation) {
       auto SetDoc = [&](llvm::StringRef Doc) {

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

auto Results = completions(
       )cpp",
       /*IndexSymbols=*/{}, Opts);
   EXPECT_THAT(
       Results.Completions,
       HasSubsequence(named("a"),
                      snippetSuffix("(${1:int i}, ${2:const float f})")));
 }
 
+TEST(CompletionTest, HeuristicsForMemberFunctionCompletion) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.EnableSnippets = true;
+
+  Annotations Code(R"cpp(
+      struct Foo {
+        static int staticMethod();
+        int method() const;
+        Foo() {
+          this->$keepSnippet^
+          $keepSnippet^
+          Foo::$keepSnippet^
+        }
+      };
+
+      struct Derived : Foo {
+        Derived() {
+          Foo::$keepSnippet^
+        }
+      };
+
+      struct OtherClass {
+        OtherClass() {
+          Foo f;
+          f.$keepSnippet^
+          &Foo::$noSnippet^
+        }
+      };
+
+      int main() {
+        Foo f;
+        f.$keepSnippet^
+        &Foo::$noSnippet^
+      }
+      )cpp");
+  auto TU = TestTU::withCode(Code.code());
+
+  for (const auto &P : Code.points("noSnippet")) {
+    auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+    EXPECT_THAT(Results.Completions,
+                Contains(AllOf(named("method"), snippetSuffix(""))));
+  }
+
+  for (const auto &P : Code.points("keepSnippet")) {
+    auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+    EXPECT_THAT(Results.Completions,
+                Contains(AllOf(named("method"), snippetSuffix("()"))));
+  }
+
+  // static method will always keep the snippet
+  for (const auto &P : Code.points()) {
+    auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
+    EXPECT_THAT(Results.Completions,
+                Contains(AllOf(named("staticMethod"), snippetSuffix("()"))));
+  }
+}
+
 TEST(CompletionTest, NoSnippetsInUsings) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
   auto Results = completions(
       R"cpp(
       namespace ns {
         int func(int a, int b);
       }

clang/include/clang/Sema/CodeCompleteConsumer.h

public:
   /// Whether all parameters (of a function, Objective-C
   /// method, etc.) should be considered "informative".
   bool AllParametersAreInformative : 1;
 
   /// Whether we're completing a declaration of the given entity,
   /// rather than a use of that entity.
   bool DeclaringEntity : 1;
 
+  /// When completing a function, whether it can be a call. This will usually be
+  /// true, but we have some heuristics, e.g. when a pointer to a non-static
+  /// member function is completed outside of that class' scope, it can never
+  /// be a call.
+  bool FunctionCanBeCall : 1;
+
   /// If the result should have a nested-name-specifier, this is it.
   /// When \c QualifierIsInformative, the nested-name-specifier is
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
   /// If this Decl was unshadowed by using declaration, this can store a
   /// pointer to the UsingShadowDecl which was used in the unshadowing process.
   /// This information can be used to uprank CodeCompletionResults / which have
CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
                        NestedNameSpecifier *Qualifier = nullptr,
                        bool QualifierIsInformative = false,
                        bool Accessible = true,
                        std::vector<FixItHint> FixIts = std::vector<FixItHint>())
       : Declaration(Declaration), Priority(Priority), Kind(RK_Declaration),
         FixIts(std::move(FixIts)), Hidden(false), InBaseClass(false),
         QualifierIsInformative(QualifierIsInformative),
         StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-        DeclaringEntity(false), Qualifier(Qualifier) {
+        DeclaringEntity(false), FunctionCanBeCall(true), Qualifier(Qualifier) {
     // FIXME: Add assert to check FixIts range requirements.
     computeCursorKindAndAvailability(Accessible);
   }
 
   /// Build a result that refers to a keyword or symbol.
   CodeCompletionResult(const char *Keyword, unsigned Priority = CCP_Keyword)
       : Keyword(Keyword), Priority(Priority), Kind(RK_Keyword),
         CursorKind(CXCursor_NotImplemented), Hidden(false), InBaseClass(false),
         QualifierIsInformative(false), StartsNestedNameSpecifier(false),
-        AllParametersAreInformative(false), DeclaringEntity(false) {}
+        AllParametersAreInformative(false), DeclaringEntity(false),
+        FunctionCanBeCall(true) {}
 
   /// Build a result that refers to a macro.
   CodeCompletionResult(const IdentifierInfo *Macro,
                        const MacroInfo *MI = nullptr,
                        unsigned Priority = CCP_Macro)
       : Macro(Macro), Priority(Priority), Kind(RK_Macro),
         CursorKind(CXCursor_MacroDefinition), Hidden(false), InBaseClass(false),
         QualifierIsInformative(false), StartsNestedNameSpecifier(false),
         AllParametersAreInformative(false), DeclaringEntity(false),
-        MacroDefInfo(MI) {}
+        FunctionCanBeCall(true), MacroDefInfo(MI) {}
 
   /// Build a result that refers to a pattern.
   CodeCompletionResult(
       CodeCompletionString *Pattern, unsigned Priority = CCP_CodePattern,
       CXCursorKind CursorKind = CXCursor_NotImplemented,
       CXAvailabilityKind Availability = CXAvailability_Available,
       const NamedDecl *D = nullptr)
       : Declaration(D), Pattern(Pattern), Priority(Priority), Kind(RK_Pattern),
         CursorKind(CursorKind), Availability(Availability), Hidden(false),
         InBaseClass(false), QualifierIsInformative(false),
         StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-        DeclaringEntity(false) {}
+        DeclaringEntity(false), FunctionCanBeCall(true) {}
 
   /// Build a result that refers to a pattern with an associated
   /// declaration.
   CodeCompletionResult(CodeCompletionString *Pattern, const NamedDecl *D,
                        unsigned Priority)
       : Declaration(D), Pattern(Pattern), Priority(Priority), Kind(RK_Pattern),
         Hidden(false), InBaseClass(false), QualifierIsInformative(false),
         StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
-        DeclaringEntity(false) {
+        DeclaringEntity(false), FunctionCanBeCall(true) {
     computeCursorKindAndAvailability();
   }
 
   /// Retrieve the declaration stored in this result. This might be nullptr if
   /// Kind is RK_Pattern.
   const NamedDecl *getDeclaration() const {
     assert(((Kind == RK_Declaration) || (Kind == RK_Pattern)) &&
            "Not a declaration or pattern result");

clang/lib/Sema/SemaCodeComplete.cpp

if (const auto *Method = dyn_cast<CXXMethodDecl>(R.Declaration))
             return;
           case OverloadCompare::BothViable:
             break;
           }
         }
         OverloadSet.Add(Method, Results.size());
       }
 
+  // When completing a non-static member function (and not via
+  // dot/arrow member access) and we're not inside that class' scope,
+  // it can't be a call.
+  if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
+    const auto *Method = dyn_cast<CXXMethodDecl>(R.getDeclaration());
+    if (Method && !Method->isStatic()) {
+      // Find the class scope that we're currently in.
+      // We could e.g. be inside a lambda, so walk up the DeclContext until we
+      // find a CXXMethodDecl.
+      const auto *CurrentClassScope = [&]() -> const CXXRecordDecl * {
+        for (DeclContext *Ctx = SemaRef.CurContext; Ctx;
+             Ctx = Ctx->getParent()) {
+          const auto *CtxMethod = llvm::dyn_cast<CXXMethodDecl>(Ctx);
+          if (CtxMethod && !CtxMethod->getParent()->isLambda()) {
+            return CtxMethod->getParent();
+          }
+        }
+        return nullptr;
+      }();
+
+      R.FunctionCanBeCall =
+          CurrentClassScope &&
+          (CurrentClassScope == Method->getParent() ||
+           CurrentClassScope->isDerivedFrom(Method->getParent()));
+    }
+  }
+
   // Insert this result into the set of results.
   Results.push_back(R);
 
   if (!AsNestedNameSpecifier)
     MaybeAddConstructorResults(R);
 }
 
 void ResultBuilder::AddResult(Result R) {