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

This implements the 1st heuristic mentioned in clangd/clangd#968 (comment):

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)

Reviewed By: nridge

Differential Revision: https://reviews.llvm.org/D137040

Author: tom-anders

clang-tools-extra/clangd/CodeComplete.cpp

@@ -411,6 +411,8 @@ struct CodeCompletionBuilder {
       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);

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

@@ -505,6 +505,63 @@ TEST(CompletionTest, Snippets) {
                      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;

clang/include/clang/Sema/CodeCompleteConsumer.h

@@ -850,6 +850,12 @@ class CodeCompletionResult {
   /// 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.
@@ -876,7 +882,7 @@ class CodeCompletionResult {
         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);
   }
@@ -886,7 +892,8 @@ class CodeCompletionResult {
       : 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,
@@ -896,7 +903,7 @@ class CodeCompletionResult {
         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(
@@ -908,7 +915,7 @@ class CodeCompletionResult {
         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.
@@ -917,7 +924,7 @@ class CodeCompletionResult {
       : 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();
   }
 

clang/lib/Sema/SemaCodeComplete.cpp

@@ -1379,6 +1379,33 @@ void ResultBuilder::AddResult(Result R, DeclContext *CurContext,
         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);
 

clang/unittests/Sema/CodeCompleteTest.cpp

@@ -23,6 +23,8 @@ namespace {
 
 using namespace clang;
 using namespace clang::tooling;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::Each;
 using ::testing::UnorderedElementsAre;
 
@@ -36,6 +38,51 @@ struct CompletionContext {
   std::string PtrDiffType;
 };
 
+struct CompletedFunctionDecl {
+  std::string Name;
+  bool IsStatic;
+  bool CanBeCall;
+};
+MATCHER_P(named, name, "") { return arg.Name == name; }
+MATCHER_P(isStatic, value, "") { return arg.IsStatic == value; }
+MATCHER_P(canBeCall, value, "") { return arg.CanBeCall == value; }
+
+class SaveCompletedFunctions : public CodeCompleteConsumer {
+public:
+  SaveCompletedFunctions(std::vector<CompletedFunctionDecl> &CompletedFuncDecls)
+      : CodeCompleteConsumer(/*CodeCompleteOpts=*/{}),
+        CompletedFuncDecls(CompletedFuncDecls),
+        CCTUInfo(std::make_shared<GlobalCodeCompletionAllocator>()) {}
+
+  void ProcessCodeCompleteResults(Sema &S, CodeCompletionContext Context,
+                                  CodeCompletionResult *Results,
+                                  unsigned NumResults) override {
+    for (unsigned I = 0; I < NumResults; ++I) {
+      auto R = Results[I];
+      if (R.Kind == CodeCompletionResult::RK_Declaration) {
+        if (const auto *FD = llvm::dyn_cast<FunctionDecl>(R.getDeclaration())) {
+          CompletedFunctionDecl D;
+          D.Name = FD->getNameAsString();
+          D.CanBeCall = R.FunctionCanBeCall;
+          D.IsStatic = FD->isStatic();
+          CompletedFuncDecls.emplace_back(std::move(D));
+        }
+      }
+    }
+  }
+
+private:
+  CodeCompletionAllocator &getAllocator() override {
+    return CCTUInfo.getAllocator();
+  }
+
+  CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
+
+  std::vector<CompletedFunctionDecl> &CompletedFuncDecls;
+
+  CodeCompletionTUInfo CCTUInfo;
+};
+
 class VisitedContextFinder : public CodeCompleteConsumer {
 public:
   VisitedContextFinder(CompletionContext &ResultCtx)
@@ -74,19 +121,19 @@ class VisitedContextFinder : public CodeCompleteConsumer {
 
 class CodeCompleteAction : public SyntaxOnlyAction {
 public:
-  CodeCompleteAction(ParsedSourceLocation P, CompletionContext &ResultCtx)
-      : CompletePosition(std::move(P)), ResultCtx(ResultCtx) {}
+  CodeCompleteAction(ParsedSourceLocation P, CodeCompleteConsumer *Consumer)
+      : CompletePosition(std::move(P)), Consumer(Consumer) {}
 
   bool BeginInvocation(CompilerInstance &CI) override {
     CI.getFrontendOpts().CodeCompletionAt = CompletePosition;
-    CI.setCodeCompletionConsumer(new VisitedContextFinder(ResultCtx));
+    CI.setCodeCompletionConsumer(Consumer);
     return true;
   }
 
 private:
   // 1-based code complete position <Line, Col>;
   ParsedSourceLocation CompletePosition;
-  CompletionContext &ResultCtx;
+  CodeCompleteConsumer *Consumer;
 };
 
 ParsedSourceLocation offsetToPosition(llvm::StringRef Code, size_t Offset) {
@@ -103,7 +150,7 @@ CompletionContext runCompletion(StringRef Code, size_t Offset) {
   CompletionContext ResultCtx;
   clang::tooling::runToolOnCodeWithArgs(
       std::make_unique<CodeCompleteAction>(offsetToPosition(Code, Offset),
-                                           ResultCtx),
+                                           new VisitedContextFinder(ResultCtx)),
       Code, {"-std=c++11"}, TestCCName);
   return ResultCtx;
 }
@@ -129,6 +176,69 @@ collectPreferredTypes(StringRef AnnotatedCode,
   return Types;
 }
 
+std::vector<CompletedFunctionDecl>
+CollectCompletedFunctions(StringRef Code, std::size_t Point) {
+  std::vector<CompletedFunctionDecl> Result;
+  clang::tooling::runToolOnCodeWithArgs(
+      std::make_unique<CodeCompleteAction>(offsetToPosition(Code, Point),
+                                           new SaveCompletedFunctions(Result)),
+      Code, {"-std=c++11"}, TestCCName);
+  return Result;
+}
+
+TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
+  llvm::Annotations Code(R"cpp(
+    struct Foo {
+      static int staticMethod();
+      int method() const;
+      Foo() {
+        this->$canBeCall^
+        $canBeCall^
+        Foo::$canBeCall^
+      }
+    };
+
+    struct Derived : Foo {
+      Derived() {
+        Foo::$canBeCall^
+      }
+    };
+
+    struct OtherClass {
+      OtherClass() {
+        Foo f;
+        f.$canBeCall^
+        &Foo::$cannotBeCall^
+      }
+    };
+
+    int main() {
+      Foo f;
+      f.$canBeCall^
+      &Foo::$cannotBeCall^
+    }
+    )cpp");
+
+  for (const auto &P : Code.points("canBeCall")) {
+    auto Results = CollectCompletedFunctions(Code.code(), P);
+    EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
+                                        canBeCall(true))));
+  }
+
+  for (const auto &P : Code.points("cannotBeCall")) {
+    auto Results = CollectCompletedFunctions(Code.code(), P);
+    EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false),
+                                        canBeCall(false))));
+  }
+
+  // static method can always be a call
+  for (const auto &P : Code.points()) {
+    auto Results = CollectCompletedFunctions(Code.code(), P);
+    EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true),
+                                        canBeCall(true))));
+  }
+}
+
 TEST(SemaCodeCompleteTest, VisitedNSForValidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
      namespace ns1 {}