diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -460,9 +460,9 @@ bool IsConcept = false; if (C.SemaResult) { getSignature(*SemaCCS, &S.Signature, &S.SnippetSuffix, C.SemaResult->Kind, - C.SemaResult->CursorKind, &Completion.RequiredQualifier); - if (!C.SemaResult->FunctionCanBeCall) - S.SnippetSuffix.clear(); + C.SemaResult->CursorKind, + /*IncludeFunctionArguments=*/C.SemaResult->FunctionCanBeCall, + /*RequiredQualifiers=*/&Completion.RequiredQualifier); S.ReturnType = getReturnType(*SemaCCS); if (C.SemaResult->Kind == CodeCompletionResult::RK_Declaration) if (const auto *D = C.SemaResult->getDeclaration()) diff --git a/clang-tools-extra/clangd/CodeCompletionStrings.h b/clang-tools-extra/clangd/CodeCompletionStrings.h --- a/clang-tools-extra/clangd/CodeCompletionStrings.h +++ b/clang-tools-extra/clangd/CodeCompletionStrings.h @@ -42,6 +42,9 @@ /// If set, RequiredQualifiers is the text that must be typed before the name. /// e.g "Base::" when calling a base class member function that's hidden. /// +/// If \p IncludeFunctionArguments is disabled, the \p Snippet will only +/// contain function name and template arguments, if any. +/// /// When \p ResultKind is RK_Pattern, the last placeholder will be $0, /// indicating the cursor should stay there. /// Note that for certain \p CursorKind like \p CXCursor_Constructor, $0 won't @@ -49,7 +52,7 @@ void getSignature(const CodeCompletionString &CCS, std::string *Signature, std::string *Snippet, CodeCompletionResult::ResultKind ResultKind, - CXCursorKind CursorKind, + CXCursorKind CursorKind, bool IncludeFunctionArguments = true, std::string *RequiredQualifiers = nullptr); /// Assembles formatted documentation for a completion result. This includes diff --git a/clang-tools-extra/clangd/CodeCompletionStrings.cpp b/clang-tools-extra/clangd/CodeCompletionStrings.cpp --- a/clang-tools-extra/clangd/CodeCompletionStrings.cpp +++ b/clang-tools-extra/clangd/CodeCompletionStrings.cpp @@ -12,6 +12,7 @@ #include "clang/AST/RawCommentList.h" #include "clang/Basic/SourceManager.h" #include "clang/Sema/CodeCompleteConsumer.h" +#include "llvm/Support/Compiler.h" #include "llvm/Support/JSON.h" #include #include @@ -118,7 +119,8 @@ void getSignature(const CodeCompletionString &CCS, std::string *Signature, std::string *Snippet, CodeCompletionResult::ResultKind ResultKind, - CXCursorKind CursorKind, std::string *RequiredQualifiers) { + CXCursorKind CursorKind, bool IncludeFunctionArguments, + std::string *RequiredQualifiers) { // Placeholder with this index will be $0 to mark final cursor position. // Usually we do not add $0, so the cursor is placed at end of completed text. unsigned CursorSnippetArg = std::numeric_limits::max(); @@ -138,6 +140,8 @@ unsigned SnippetArg = 0; bool HadObjCArguments = false; bool HadInformativeChunks = false; + + std::optional TruncateSnippetAt; for (const auto &Chunk : CCS) { // Informative qualifier chunks only clutter completion results, skip // them. @@ -243,6 +247,13 @@ "CompletionItems"); break; case CodeCompletionString::CK_LeftParen: + // We're assuming that a LeftParen in a declaration starts a function + // call, and arguments following the parenthesis could be discarded if + // IncludeFunctionArguments is false. + if (!IncludeFunctionArguments && + ResultKind == CodeCompletionResult::RK_Declaration) + TruncateSnippetAt.emplace(Snippet->size()); + LLVM_FALLTHROUGH; case CodeCompletionString::CK_RightParen: case CodeCompletionString::CK_LeftBracket: case CodeCompletionString::CK_RightBracket: @@ -264,6 +275,8 @@ break; } } + if (TruncateSnippetAt) + *Snippet = Snippet->substr(0, *TruncateSnippetAt); } std::string formatDocumentation(const CodeCompletionString &CCS, diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -530,54 +530,87 @@ Annotations Code(R"cpp( struct Foo { - static int staticMethod(); - int method() const; + static int staticMethod(int); + int method(int) const; + template + void generic(U); + template + static T staticGeneric(); Foo() { - this->$keepSnippet^ - $keepSnippet^ - Foo::$keepSnippet^ + this->$canBeCall^ + $canBeCall^ + Foo::$canBeCall^ } }; struct Derived : Foo { + using Foo::method; + using Foo::generic; Derived() { - Foo::$keepSnippet^ + Foo::$canBeCall^ } }; struct OtherClass { OtherClass() { Foo f; - f.$keepSnippet^ - &Foo::$noSnippet^ + Derived d; + f.$canBeCall^ + ; // Prevent parsing as 'f.f' + f.Foo::$canBeCall^ + &Foo::$canNotBeCall^ + ; + d.Foo::$canBeCall^ + ; + d.Derived::$canBeCall^ } }; int main() { Foo f; - f.$keepSnippet^ - &Foo::$noSnippet^ + Derived d; + f.$canBeCall^ + ; // Prevent parsing as 'f.f' + f.Foo::$canBeCall^ + &Foo::$canNotBeCall^ + ; + d.Foo::$canBeCall^ + ; + d.Derived::$canBeCall^ } )cpp"); auto TU = TestTU::withCode(Code.code()); - for (const auto &P : Code.points("noSnippet")) { + for (const auto &P : Code.points("canNotBeCall")) { auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts); EXPECT_THAT(Results.Completions, - Contains(AllOf(named("method"), snippetSuffix("")))); + Contains(AllOf(named("method"), signature("(int) const"), + snippetSuffix("")))); + EXPECT_THAT(Results.Completions, + Contains(AllOf(named("generic"), signature("(U)"), + snippetSuffix("<${1:typename T}>")))); } - for (const auto &P : Code.points("keepSnippet")) { + for (const auto &P : Code.points("canBeCall")) { auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts); EXPECT_THAT(Results.Completions, - Contains(AllOf(named("method"), snippetSuffix("()")))); + Contains(AllOf(named("method"), signature("(int) const"), + snippetSuffix("(${1:int})")))); + EXPECT_THAT(Results.Completions, + Contains(AllOf(named("generic"), signature("(U)"), + snippetSuffix("<${1:typename T}>(${2:U})")))); } // 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("()")))); + Contains(AllOf(named("staticMethod"), signature("(int)"), + snippetSuffix("(${1:int})")))); + EXPECT_THAT(Results.Completions, + Contains(AllOf( + named("staticGeneric"), signature("()"), + snippetSuffix("<${1:typename T}, ${2:int U}>()")))); } } diff --git a/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp @@ -25,11 +25,13 @@ protected: void computeSignature(const CodeCompletionString &CCS, CodeCompletionResult::ResultKind ResultKind = - CodeCompletionResult::ResultKind::RK_Declaration) { + CodeCompletionResult::ResultKind::RK_Declaration, + bool IncludeFunctionArguments = true) { Signature.clear(); Snippet.clear(); getSignature(CCS, &Signature, &Snippet, ResultKind, /*CursorKind=*/CXCursorKind::CXCursor_NotImplemented, + /*IncludeFunctionArguments=*/IncludeFunctionArguments, /*RequiredQualifiers=*/nullptr); } @@ -156,6 +158,28 @@ EXPECT_EQ(Snippet, " ${1:name} = $0;"); } +TEST_F(CompletionStringTest, DropFunctionArguments) { + Builder.AddTypedTextChunk("foo"); + Builder.AddChunk(CodeCompletionString::CK_LeftAngle); + Builder.AddPlaceholderChunk("typename T"); + Builder.AddChunk(CodeCompletionString::CK_Comma); + Builder.AddPlaceholderChunk("int U"); + Builder.AddChunk(CodeCompletionString::CK_RightAngle); + Builder.AddChunk(CodeCompletionString::CK_LeftParen); + Builder.AddPlaceholderChunk("arg1"); + Builder.AddChunk(CodeCompletionString::CK_Comma); + Builder.AddPlaceholderChunk("arg2"); + Builder.AddChunk(CodeCompletionString::CK_RightParen); + + computeSignature( + *Builder.TakeString(), + /*ResultKind=*/CodeCompletionResult::ResultKind::RK_Declaration, + /*IncludeFunctionArguments=*/false); + // Arguments dropped from snippet, kept in signature. + EXPECT_EQ(Signature, "(arg1, arg2)"); + EXPECT_EQ(Snippet, "<${1:typename T}, ${2:int U}>"); +} + TEST_F(CompletionStringTest, IgnoreInformativeQualifier) { Builder.AddTypedTextChunk("X"); Builder.AddInformativeChunk("info ok"); diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp --- a/clang/lib/Sema/SemaCodeComplete.cpp +++ b/clang/lib/Sema/SemaCodeComplete.cpp @@ -338,8 +338,11 @@ /// /// \param InBaseClass whether the result was found in a base /// class of the searched context. + /// + /// \param BaseExprType the type of expression that precedes the "." or "->" + /// in a member access expression. void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding, - bool InBaseClass); + bool InBaseClass, QualType BaseExprType); /// Add a new non-declaration result to this result set. void AddResult(Result R); @@ -1261,8 +1264,45 @@ : OverloadCompare::Dominated; } +static bool canCxxMethodBeCalled(DeclContext *CurContext, + const CXXMethodDecl *Method, + QualType BaseExprType) { + // 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 = CurContext; Ctx; Ctx = Ctx->getParent()) { + const auto *CtxMethod = llvm::dyn_cast(Ctx); + if (CtxMethod && !CtxMethod->getParent()->isLambda()) { + return CtxMethod->getParent(); + } + } + return nullptr; + }(); + + // 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. + bool FunctionCanBeCall = + CurrentClassScope && + (CurrentClassScope == Method->getParent() || + CurrentClassScope->isDerivedFrom(Method->getParent())); + + // Exception: foo->FooBase::bar() or foo->Foo::bar() *is* a call. + if (const CXXRecordDecl *MaybeDerived = + BaseExprType.isNull() ? nullptr + : BaseExprType->getAsCXXRecordDecl()) { + auto *MaybeBase = Method->getParent(); + FunctionCanBeCall = + MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase); + } + + return FunctionCanBeCall; +} + void ResultBuilder::AddResult(Result R, DeclContext *CurContext, - NamedDecl *Hiding, bool InBaseClass = false) { + NamedDecl *Hiding, bool InBaseClass = false, + QualType BaseExprType = QualType()) { if (R.Kind != Result::RK_Declaration) { // For non-declaration results, just add the result. Results.push_back(R); @@ -1278,7 +1318,8 @@ R.Availability == CXAvailability_Deprecated), std::move(R.FixIts)); Result.ShadowDecl = Using; - AddResult(Result, CurContext, Hiding); + AddResult(Result, CurContext, Hiding, /*InBaseClass=*/false, + /*BaseExprType=*/BaseExprType); return; } @@ -1380,30 +1421,16 @@ 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. + // Decide whether or not a non-static member function can be a call. if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) { - const auto *Method = dyn_cast(R.getDeclaration()); + const NamedDecl *ND = R.getDeclaration(); + if (const auto *FuncTmpl = dyn_cast(ND)) { + ND = FuncTmpl->getTemplatedDecl(); + } + const auto *Method = dyn_cast(ND); 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(Ctx); - if (CtxMethod && !CtxMethod->getParent()->isLambda()) { - return CtxMethod->getParent(); - } - } - return nullptr; - }(); - R.FunctionCanBeCall = - CurrentClassScope && - (CurrentClassScope == Method->getParent() || - CurrentClassScope->isDerivedFrom(Method->getParent())); + canCxxMethodBeCalled(SemaRef.CurContext, Method, BaseExprType); } } @@ -1679,7 +1706,7 @@ bool InBaseClass) override { ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr, false, IsAccessible(ND, Ctx), FixIts); - Results.AddResult(Result, InitialLookupCtx, Hiding, InBaseClass); + Results.AddResult(Result, InitialLookupCtx, Hiding, InBaseClass, BaseType); } void EnteredContext(DeclContext *Ctx) override { @@ -3550,10 +3577,19 @@ } } - if (LastDeducibleArgument) { + if (LastDeducibleArgument || !FunctionCanBeCall) { // Some of the function template arguments cannot be deduced from a // function call, so we introduce an explicit template argument list // containing all of the arguments up to the first deducible argument. + // + // Or, if this isn't a call, emit all the template arguments + // to disambiguate the (potential) overloads. + // + // FIXME: Detect cases where the function parameters can be deduced from + // the surrounding context, as per [temp.deduct.funcaddr]. + // e.g., + // template void foo(T); + // void (*f)(int) = foo; Result.AddChunk(CodeCompletionString::CK_LeftAngle); AddTemplateParameterChunks(Ctx, Policy, FunTmpl, Result, LastDeducibleArgument); diff --git a/clang/test/CodeCompletion/member-access.cpp b/clang/test/CodeCompletion/member-access.cpp --- a/clang/test/CodeCompletion/member-access.cpp +++ b/clang/test/CodeCompletion/member-access.cpp @@ -341,3 +341,14 @@ // RUN: %clang_cc1 -fsyntax-only -code-completion-with-fixits -code-completion-at=%s:339:10 %s -o - | FileCheck -check-prefix=CHECK-FIELD-DECLARED-VIA-USING %s // CHECK-FIELD-DECLARED-VIA-USING: [#int#]field (requires fix-it: {339:8-339:9} to "->") } + +namespace function_can_be_call { + struct S { + template + V foo(T, U); + }; + + &S::f + // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:351:7 %s -o - | FileCheck -check-prefix=CHECK_FUNCTION_CAN_BE_CALL %s + // CHECK_FUNCTION_CAN_BE_CALL: COMPLETION: foo : [#V#]foo<<#typename T#>, <#typename U#>{#, <#typename V#>#}>(<#T#>, <#U#>) +} diff --git a/clang/unittests/Sema/CodeCompleteTest.cpp b/clang/unittests/Sema/CodeCompleteTest.cpp --- a/clang/unittests/Sema/CodeCompleteTest.cpp +++ b/clang/unittests/Sema/CodeCompleteTest.cpp @@ -60,7 +60,10 @@ for (unsigned I = 0; I < NumResults; ++I) { auto R = Results[I]; if (R.Kind == CodeCompletionResult::RK_Declaration) { - if (const auto *FD = llvm::dyn_cast(R.getDeclaration())) { + auto *ND = R.getDeclaration(); + if (auto *Template = llvm::dyn_cast(ND)) + ND = Template->getTemplatedDecl(); + if (const auto *FD = llvm::dyn_cast(ND)) { CompletedFunctionDecl D; D.Name = FD->getNameAsString(); D.CanBeCall = R.FunctionCanBeCall; @@ -191,6 +194,10 @@ struct Foo { static int staticMethod(); int method() const; + template + void generic(U); + template + static T staticGeneric(); Foo() { this->$canBeCall^ $canBeCall^ @@ -199,6 +206,8 @@ }; struct Derived : Foo { + using Foo::method; + using Foo::generic; Derived() { Foo::$canBeCall^ } @@ -207,15 +216,29 @@ struct OtherClass { OtherClass() { Foo f; + Derived d; f.$canBeCall^ + ; // Prevent parsing as 'f.f' + f.Foo::$canBeCall^ &Foo::$cannotBeCall^ + ; + d.Foo::$canBeCall^ + ; + d.Derived::$canBeCall^ } }; int main() { Foo f; + Derived d; f.$canBeCall^ + ; // Prevent parsing as 'f.f' + f.Foo::$canBeCall^ &Foo::$cannotBeCall^ + ; + d.Foo::$canBeCall^ + ; + d.Derived::$canBeCall^ } )cpp"); @@ -223,12 +246,16 @@ auto Results = CollectCompletedFunctions(Code.code(), P); EXPECT_THAT(Results, Contains(AllOf(named("method"), isStatic(false), canBeCall(true)))); + EXPECT_THAT(Results, Contains(AllOf(named("generic"), 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)))); + EXPECT_THAT(Results, Contains(AllOf(named("generic"), isStatic(false), + canBeCall(false)))); } // static method can always be a call @@ -236,6 +263,8 @@ auto Results = CollectCompletedFunctions(Code.code(), P); EXPECT_THAT(Results, Contains(AllOf(named("staticMethod"), isStatic(true), canBeCall(true)))); + EXPECT_THAT(Results, Contains(AllOf(named("staticGeneric"), isStatic(true), + canBeCall(true)))); } }