[clang-tools-extra] 0e00611 - [clangd] Add heuristic for dropping snippet when completing member function pointer
Tom Praschan via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 16 14:38:17 PST 2022
Author: Tom Praschan
Date: 2022-11-17T00:37:16+01:00
New Revision: 0e00611cbc2b2f27e247a58b512cb2cec0624290
URL: https://github.com/llvm/llvm-project/commit/0e00611cbc2b2f27e247a58b512cb2cec0624290
DIFF: https://github.com/llvm/llvm-project/commit/0e00611cbc2b2f27e247a58b512cb2cec0624290.diff
LOG: [clangd] Add heuristic for dropping snippet when completing member function pointer
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)
Reviewed By: nridge
Differential Revision: https://reviews.llvm.org/D137040
Added:
Modified:
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
clang/include/clang/Sema/CodeCompleteConsumer.h
clang/lib/Sema/SemaCodeComplete.cpp
clang/unittests/Sema/CodeCompleteTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index e52cb2643bab..7bdfdec64d1d 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/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);
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index 99d09ad43466..42d252149168 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/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;
diff --git a/clang/include/clang/Sema/CodeCompleteConsumer.h b/clang/include/clang/Sema/CodeCompleteConsumer.h
index 8620320a91f8..d8266c1b781d 100644
--- a/clang/include/clang/Sema/CodeCompleteConsumer.h
+++ b/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();
}
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index baf5676904c1..21726920dd93 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/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);
diff --git a/clang/unittests/Sema/CodeCompleteTest.cpp b/clang/unittests/Sema/CodeCompleteTest.cpp
index 33856284c4fa..1d453f6cb443 100644
--- a/clang/unittests/Sema/CodeCompleteTest.cpp
+++ b/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 {}
More information about the cfe-commits
mailing list