[clangd][CodeComplete] Improve FunctionCanBeCall

[zyounan]

Summary

From two aspects:

• For function templates, emit additional template argument

placeholders in the context where it can't be a call in order
to specify an instantiation explicitly.

• Consider expressions with base type specifier such as

'Derived().Base::foo^' a function call.

[zyounan]

Merge changes with function templates from D155370

[zyounan]

Trigger the build

[nridge]

Thanks for the patch, these are nice improvements

clang-tools-extra/clangd/CodeCompletionStrings.cpp

258

It looks like we are assuming two things:

1. Any LeftParen in an RK_Declaration starts a function argument list
2. Everything that comes after the function argument list can be discarded

I think these assumptions are fine to make, but let's be explicit about them in a comment

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

535

Suggestion: it would be slightly more interesting to make the function signature generic(U). Then, the function can be called as generic<T>(u) (with the template parameter U deduced), and the LastDeducibleArgument logic should make sure that only <T> is added to the code completion string, not <T, U>.

586

Since you're updating this test anyways, could you add signature() matchers for the non-template cases as well please?

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

178

Maybe add:

// Arguments dropped from snippet, kept in signature

so someone reading the test knows it's deliberate

clang/lib/Sema/SemaCodeComplete.cpp

363

The word "base" is a bit overloaded in this signature; in the parameter InBaseClass it refers to inheritance, but in BaseType it refers to the base expression of a MemberExpr.

Maybe we can name the new parameter BaseExprType to avoid confusion?

1363–1364

Should we propagate BaseType into this recursive call?

1475

Is there any situation where MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase) is false?

I am wondering if we can simplify this to:

if (!BaseType.isNull()) {
  R.FunctionCanBeCall = true;
}

3624

1. If this is not a call, we can avoid running the Sema::MarkDeducedTemplateParameters operation altogether.

2. A future improvement we could consider: if this is not a call, try to detect cases where the template parameters can be deduced from the surrounding context ("Deducing template arguments taking the address of a function template "). Maybe add a FIXME for this?

[sammccall]

Thanks, this does look better.

I agree with Nathan's comments and will leave final stamp to him.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

144

I find this comment very unclear, I think because it starts with implementation details and works its way up to general principles.

I think Buffer that snippet chunks are written to once we've decided to discard the snippet due to DropFunctionArguments or so would be enough.

However I'd rather drop this variable entirely if possible, the data flow is confusing. See below.

148

For the reason you give here, it's actually *not* preferable. So I'd suggest leaving out this comment.

258

Agree, but also I think the code could reflect this more directly:

• this should trigger only for CK_LeftParen, not CK_RightParen

Rather than redirecting output to a different string by reassigning the param, I think it would be a bit more direct to have

optional<unsigned> TruncateSnippetAt;
...
case CK_LeftBracket:
  if (DropFunctionArguments && ... && !TruncateSnippetAt)
    TruncateSnippetAt = Snippet->size();
...
if (TruncateSnippetAt)
  Snippet->resize(*TruncateSnippetAt);
}

(though still not totally clear)

clang-tools-extra/clangd/CodeCompletionStrings.h

45

Prefer positive sense for bool params (IncludeFunctionArguments) to avoid double-negative confusion

clang/lib/Sema/SemaCodeComplete.cpp

1465–1466

This is confusing: the CCC_Symbol test is part of the specific heuristics being used (it's the "not via dot/arrow member access" part, right?) but you've moved the comment explaining what it does.

Also this function is just getting too long, and we're inlining more complicated control flow here.
Can we extract a function?

const auto *Method = ...;
if (Method & !Method->isStatic()) {
  R.FunctionCanBeCall = canMethodBeCalled(...);
}

1467

This description is hard for me to follow, and it's hard to tell if this is independent of the previous check, or an exception to it.

An example would be clearer. (I think instead rather than in addition):
Exception: foo->FooBase::bar() *is* a call.

1470

if (const CXXRecordDecl *BaseDecl = BaseType ? BaseType-> getAsCXXRecordDecl() : nullptr)?
avoiding the reassignment inside a boolean expression

[zyounan]

Address many comments.

[zyounan]

Thanks for Sam and Nathan's thorough review! I've updated, and please take another look.
(Apologize for churning on weekends, but I don't have chunk time during the week.)

clang-tools-extra/clangd/CodeCompletionStrings.cpp

258

It looks like we are assuming two things

Honestly, I don't quite prefer this assumption. However, we have lost some of the semantics e.g., the structure of a call expression within this function, and unfortunately it's not trivial to pass these out from clang. :-(

(though still not totally clear)

Yeah. I imagine a clearer way would be separating the calculation for Signature and Snippet, and we could bail out early for Snippet. But I'm afraid that making so many adjustments to getSignature for such a small feature is inappropriate.

Assuming the left parenthesis is the beginning of a call might be sufficient for the time being, and let's leave the refactoring to subsequent patches.

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

586

Of course!

clang/lib/Sema/SemaCodeComplete.cpp

363

Makes sense to me. Thanks for the correction.

1363–1364

Yes. And thanks for spotting this. I added another test case reflecting it.

1465–1466

it's the "not via dot/arrow member access" part

(Sorry for being unaware of the historical context). But I think CCC_Symbol should mean "we're completing a name such as a function or type name" per its comment. The heuristic for dot/arrow member access actually lies on the next line, i.e., if the completing decl is a CXXMethodDecl.

Can we extract a function?

Sure.

1475

I think this could prevent us from completing ill-formed expressions like:

struct B {
  void foo();
};

struct D : B {
  void bar();
};

struct S {
  void method();
};

void baz() {
  D().S::^
}

Currently, we have the candidate method, and completing it with parentheses is definitely wrong. (Although we shouldn't be providing method as well.)

3624

avoid running the Sema::MarkDeducedTemplateParameters operation altogether

I think doing so could cause too many adjustments to the flow, and I'm afraid that the Sema::MarkDeducedTemplateParameters would be required again when we decide to implement point 2.

I'm adding a FIXME anyway but leave the first intact. However, I'd be happy to rearrange the logic flow if you insist.

[zyounan]

Fix the bool expression.

[zyounan]

Sorry, forgot to update tests :(

[zyounan]

Gently ping~

[nridge]

(I'm away on travels, will get back to this within the next week)

[nridge]

clang/lib/Sema/SemaCodeComplete.cpp

1315

nit: we can avoid the computation in this block if FunctionCanBeCall was already initialized to true above

1465–1466

The check for CompletionContext.getKind() is in fact a part of the heuristic:

• for f.method, the kind is CCC_DotMemberAccess
• for f->method, the kind is CCC_ArrowMemberAccess
• for f.Foo::method and f->Foo::method, the kind is CCC_Symbol

(I realize that's a bit inconsistent. Maybe the f.Foo:: and f->Foo:: cases should be using DotMemberAccess and ArrowMemberAccess as well? Anyways, that's a pre-existing issue.)

So, the if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) check is what currently makes sure that in the f.method and f->method cases, we just keep FunctionCanBeCall = true without having to check any context or expression type.

I think it may be clearest to move this entire if block into the new function (whose name can be generalized to canBeCall or similar), and here just unconditionally set R.FunctionCanBeCall = canBeCall(CompletionContext, /* other things */).

3624

I realized there's actually an open question here: if FunctionCanBeCall == false, do we want to include all the template parameters, or just the non-deducible ones?

Let's consider an example:

class Foo {
  template <typename T, typename U>
  T convertTo(U from);
};

void bar() {
  Foo::^
}

Here, U can be deduced but T cannot.

The current behaviour is to complete convertTo<T>. That seems appropriate for a call, since U will be deduced from the call. But since this is not a call, wouldn't it be better to complete convertTo<T, U>?

[zyounan]

clang/lib/Sema/SemaCodeComplete.cpp

3624

But since this is not a call, wouldn't it be better to complete convertTo<T, U>?

(Ugh, this is exactly this patch's initial intention, but how...?)

Oh, I made a mistake here: I presumed LastDeducibleArgument would always be 0 if we're in a non-callable context, but this is wrong! This variable is calculated based on the function parameters, which ought to be equal to the number of template parameters deducible in function parameters.

( I used to duplicate this if block for the !FunctionCanBeCall case but finally, I merged these two erroneously. :-( )

[zyounan]

Address comments

[zyounan]

Thank you Nathan for your constructive opinions! I've updated this patch again, hopefully this becomes better.

clang/lib/Sema/SemaCodeComplete.cpp

1465–1466

I think it may be clearest to move this entire if block into the new function

Nice suggestion. I'll take it.

3624

A future improvement we could consider: if this is not a call, try to detect cases where the template parameters can be deduced from the surrounding context ("Deducing template arguments taking the address of a function template "). Maybe add a FIXME for this?

UPD: I've looked into the deduction implementation and found it not difficult to add such a heuristic: We have PreferredType that describes the type in a declaration, e.g., void (*)(int, int) and DeduceTemplateArgumentsByTypeMatch in Sema to tell if the template parameter type P could be deduced from the 'synthesized' type A (That is, the declaring type void (int, int), for example).

I suppose we could apply such a deduction for each candidate and eliminate its template parameters in the result if the deduction succeeds - But let me leave that for the next patch(es) so I don't overload you. :-)

[nridge]

Thank you, the updates look good! Please go ahead and merge after addressing the last minor nits.

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

594

It seems a bit inconsistent that the signature includes parameters with default arguments in the non-call case, and not in the call case. I guess the reason for this is that in the non-call case, the parameters with defaults become a CK_Optional chunk which clangd adds to the signature here, while in the call case the deduced parameters (which include the ones with defaults) are omitted from the completion string altogether.

It may be more consistent to put the deduced parameters into an optional chunk as well, and let the consumer of the CodeCompletionString decide whether to use them?

Anyways, that's an idea for the future, no change needed now.

clang/lib/Sema/SemaCodeComplete.cpp

1301

nit: let's shorten this comment to just "If we're not inside the scope of the method's class, it can't be a call"

(The parts about non-static and dot/arrow member access are checked and explained in canFunctionBeCalled())

3586–3593

nit: "We'd" --> "We should"

[zyounan]

Thank you guys again for the long and meticulous review! :)

For a quick note, here are points I've excerpted from Nathan's reviews that need improvements later:

1. Implement a heuristic that works for function template pointers whose template parameters could be deduced from the declaring type. https://reviews.llvm.org/D156605#inline-1546819

2. Currently, the deducible template parameters are discarded by default; it may be more consistent to put the deduced parameters into an optional chunk as well, and let the consumer of the CodeCompletionString decide whether to use them. https://reviews.llvm.org/D156605#inline-1548400

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

594

It seems a bit inconsistent that the signature includes parameters with default arguments in the non-call case, and not in the call case.

Indeed. If we just want consistent behavior (i.e., both get omitted) for default parameters, it is easy to fix at the moment. Note that

1. the loop for default template params will run iff the bit vector Deduced is not empty.

2. the vector would be resized in Sema::MarkDeducedTemplateParameters to fit the template params, which has been avoided in my patch to emit all template params for non-call cases. As a result, LastDeducibleArgument would always be 0 for non-calls, even with default template params.

We can change the Deduced to a default initialized N-size bit vector, where N is the size of template params. This way, the default params can be omitted in the loop as desired (by reducing LastDeducibleArgument to limit the range it would dump to the CCS chunk later), while non-default params get preserved.

It may be more consistent to put the deduced parameters into an optional chunk as well, and let the consumer of the CodeCompletionString decide whether to use them?

Sounds reasonable.

[zyounan]

Address nits; discard default template parameters

[zyounan]

Sorry, wrong patch

[zyounan]

Fix the CI

clang-tools-extra/clangd/CodeComplete.cpp

struct CodeCompletionBuilder {
   void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS,
            CodeCompletionContext::Kind ContextKind) {
     assert(bool(C.SemaResult) == bool(SemaCCS));
     Bundled.emplace_back();
     BundledEntry &S = Bundled.back();
     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())
           if (isa<ConceptDecl>(D))
             IsConcept = true;
     } else if (C.IndexResult) {
       S.Signature = std::string(C.IndexResult->Signature);
       S.SnippetSuffix = std::string(C.IndexResult->CompletionSnippetSuffix);

clang-tools-extra/clangd/CodeCompletionStrings.h

 
 /// Formats the signature for an item, as a display string and snippet.
 /// e.g. for const_reference std::vector<T>::at(size_type) const, this returns:
 ///   *Signature = "(size_type) const"
 ///   *Snippet = "(${1:size_type})"
 /// 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
 /// be emitted in order to avoid overlapping normal parameters.
 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
 /// documentation comments and other relevant information like annotations.
 ///
 /// \param DocComment is a documentation comment for the original declaration,
 ///        it should be obtained via getDocComment or getParameterDocComment.
 std::string formatDocumentation(const CodeCompletionString &CCS,

clang-tools-extra/clangd/CodeCompletionStrings.cpp

 //===--- CodeCompletionStrings.cpp -------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
 #include "clang-c/Index.h"
 #include "clang/AST/ASTContext.h"
 #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 <limits>
 #include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
   if (!llvm::json::isUTF8(Doc))
     Doc = llvm::json::fixUTF8(Doc);
   return Doc;
 }
 
 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<unsigned>::max();
 
   // If the snippet contains a group of statements, we replace the
   // last placeholder with $0 to leave the cursor there, e.g.
   //    namespace ${1:name} {
   //      ${0:decls}
   //    }
   // We try to identify such cases using the ResultKind and CursorKind.
   if (shouldPatchPlaceholder0(ResultKind, CursorKind)) {
     CursorSnippetArg =
         llvm::count_if(CCS, [](const CodeCompletionString::Chunk &C) {
           return C.Kind == CodeCompletionString::CK_Placeholder;
         });
   }
   unsigned SnippetArg = 0;
   bool HadObjCArguments = false;
   bool HadInformativeChunks = false;
+
+  std::optional<unsigned> TruncateSnippetAt;
   for (const auto &Chunk : CCS) {
     // Informative qualifier chunks only clutter completion results, skip
     // them.
     if (isInformativeQualifierChunk(Chunk))
       continue;
 
     switch (Chunk.Kind) {
     case CodeCompletionString::CK_TypedText:
       // The typed-text chunk is the actual name. We don't record this chunk.
       // C++:
       //   In general our string looks like <qualifiers><name><signature>.
       //   So once we see the name, any text we recorded so far should be
case CodeCompletionString::CK_ResultType:
       break;
     case CodeCompletionString::CK_CurrentParameter:
       // This should never be present while collecting completion items,
       // only while collecting overload candidates.
       llvm_unreachable("Unexpected CK_CurrentParameter while collecting "
                        "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:
     case CodeCompletionString::CK_LeftBrace:
     case CodeCompletionString::CK_RightBrace:
     case CodeCompletionString::CK_LeftAngle:
     case CodeCompletionString::CK_RightAngle:
     case CodeCompletionString::CK_Comma:
     case CodeCompletionString::CK_Colon:
     case CodeCompletionString::CK_SemiColon:
     case CodeCompletionString::CK_Equal:
     case CodeCompletionString::CK_HorizontalSpace:
       *Signature += Chunk.Text;
       *Snippet += Chunk.Text;
       break;
     case CodeCompletionString::CK_VerticalSpace:
       *Snippet += Chunk.Text;
       // Don't even add a space to the signature.
       break;
     }
   }
+  if (TruncateSnippetAt)
+    *Snippet = Snippet->substr(0, *TruncateSnippetAt);
 }
 
 std::string formatDocumentation(const CodeCompletionString &CCS,
                                 llvm::StringRef DocComment) {
   // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
   // information in the documentation field.
   std::string Result;
   const unsigned AnnotationCount = CCS.getAnnotationCount();

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

 }
 
 TEST(CompletionTest, HeuristicsForMemberFunctionCompletion) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
 
   Annotations Code(R"cpp(
       struct Foo {
-        static int staticMethod();
-        int method() const;
+        static int staticMethod(int);
+        int method(int) const;
+        template <typename T, typename U, typename V = int>
+        T generic(U, V);
+        template <typename T, int U>
+        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(""))));
+    // We don't have any arguments to deduce against if this isn't a call.
+    // Thus, we should emit these deducible template arguments explicitly.
+    EXPECT_THAT(
+        Results.Completions,
+        Contains(AllOf(named("generic"),
+                       signature("<typename T, typename U>(U, V)"),
+                       snippetSuffix("<${1:typename T}, ${2:typename U}>"))));
   }
 
-  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("<typename T>(U, V)"),
+                       snippetSuffix("<${1:typename T}>(${2:U}, ${3:V})"))));
   }
 
   // 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("<typename T, int U>()"),
+                    snippetSuffix("<${1:typename T}, ${2:int U}>()"))));
   }
 }
 
 TEST(CompletionTest, NoSnippetsInUsings) {
   clangd::CodeCompleteOptions Opts;
   Opts.EnableSnippets = true;
   auto Results = completions(
       R"cpp(

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

 public:
   CompletionStringTest()
       : Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator), Builder(*Allocator, CCTUInfo) {}
 
 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);
   }
 
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
   CodeCompletionBuilder Builder;
   std::string Signature;
   std::string Snippet;
TEST_F(CompletionStringTest, SnippetsInPatterns) {
   EXPECT_EQ(Snippet, " ${1:name} = ${2:target};");
 
   // When completing a pattern, the last placeholder holds the cursor position.
   computeSignature(MakeCCS(),
                    /*ResultKind=*/CodeCompletionResult::ResultKind::RK_Pattern);
   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, "<typename T, int U>(arg1, arg2)");
+  EXPECT_EQ(Snippet, "<${1:typename T}, ${2:int U}>");
+}
+
 TEST_F(CompletionStringTest, IgnoreInformativeQualifier) {
   Builder.AddTypedTextChunk("X");
   Builder.AddInformativeChunk("info ok");
   Builder.AddInformativeChunk("info no no::");
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "info ok");
   EXPECT_EQ(Snippet, "");
 }

clang/lib/Sema/SemaCodeComplete.cpp

public:
   ///
   /// \param ND the declaration that we are inspecting.
   ///
   /// \param AsNestedNameSpecifier will be set true if this declaration is
   /// only interesting when it is a nested-name-specifier.
   bool isInterestingDecl(const NamedDecl *ND,
                          bool &AsNestedNameSpecifier) const;
 
+  /// Decide whether or not a use of function Decl can be a call.
+  ///
+  /// \param ND the function declaration.
+  ///
+  /// \param BaseExprType the object type in a member access expression,
+  /// if any.
+  bool canFunctionBeCalled(const NamedDecl *ND, QualType BaseExprType) const;
+
+  /// Decide whether or not a use of member function Decl can be a call.
+  ///
+  /// \param Method the function declaration.
+  ///
+  /// \param BaseExprType the object type in a member access expression,
+  /// if any.
+  bool canCxxMethodBeCalled(const CXXMethodDecl *Method,
+                            QualType BaseExprType) const;
+
   /// Check whether the result is hidden by the Hiding declaration.
   ///
   /// \returns true if the result is hidden and cannot be found, false if
   /// the hidden result could still be found. When false, \p R may be
   /// modified to describe how the result can be found (e.g., via extra
   /// qualification).
   bool CheckHiddenResult(Result &R, DeclContext *CurContext,
                          const NamedDecl *Hiding);
public:
   /// \param R the result to add (if it is unique).
   ///
   /// \param CurContext the context in which this result will be named.
   ///
   /// \param Hiding the declaration that hides the result.
   ///
   /// \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);
 
   /// Enter into a new scope.
   void EnterNewScope();
 
   /// Exit from the current scope.
static OverloadCompare compareOverloads(const CXXMethodDecl &Candidate,
   bool CandidateSuperset = CandidateQual.compatiblyIncludes(IncumbentQual);
   bool IncumbentSuperset = IncumbentQual.compatiblyIncludes(CandidateQual);
   if (CandidateSuperset == IncumbentSuperset)
     return OverloadCompare::BothViable;
   return IncumbentSuperset ? OverloadCompare::Dominates
                            : OverloadCompare::Dominated;
 }
 
+bool ResultBuilder::canCxxMethodBeCalled(const CXXMethodDecl *Method,
+                                         QualType BaseExprType) const {
+  // 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.
+  DeclContext *CurContext = SemaRef.CurContext;
+  const auto *CurrentClassScope = [&]() -> const CXXRecordDecl * {
+    for (DeclContext *Ctx = CurContext; Ctx; Ctx = Ctx->getParent()) {
+      const auto *CtxMethod = llvm::dyn_cast<CXXMethodDecl>(Ctx);
+      if (CtxMethod && !CtxMethod->getParent()->isLambda()) {
+        return CtxMethod->getParent();
+      }
+    }
+    return nullptr;
+  }();
+
+  // If we're not inside the scope of the method's class, it can't be a call.
+  bool FunctionCanBeCall =
+      CurrentClassScope &&
+      (CurrentClassScope == Method->getParent() ||
+       CurrentClassScope->isDerivedFrom(Method->getParent()));
+
+  // We skip the following calculation for exceptions if it's already true.
+  if (FunctionCanBeCall)
+    return true;
+
+  // 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;
+}
+
+bool ResultBuilder::canFunctionBeCalled(const NamedDecl *ND,
+                                        QualType BaseExprType) const {
+  // We apply heuristics only to CCC_Symbol:
+  // * CCC_{Arrow,Dot}MemberAccess reflect member access expressions:
+  //   f.method() and f->method(). These are always calls.
+  // * A qualified name to a member function may *not* be a call. We have to
+  //   subdivide the cases: For example, f.Base::method(), which is regarded as
+  //   CCC_Symbol, should be a call.
+  // * Non-member functions and static member functions are always considered
+  //   calls.
+  if (CompletionContext.getKind() == clang::CodeCompletionContext::CCC_Symbol) {
+    if (const auto *FuncTmpl = dyn_cast<FunctionTemplateDecl>(ND)) {
+      ND = FuncTmpl->getTemplatedDecl();
+    }
+    const auto *Method = dyn_cast<CXXMethodDecl>(ND);
+    if (Method && !Method->isStatic()) {
+      return canCxxMethodBeCalled(Method, BaseExprType);
+    }
+  }
+  return true;
+}
+
 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);
     return;
   }
 
   // Look through using declarations.
   if (const auto *Using = dyn_cast<UsingShadowDecl>(R.Declaration)) {
     CodeCompletionResult Result(Using->getTargetDecl(),
                                 getBasePriority(Using->getTargetDecl()),
                                 R.Qualifier, false,
                                 (R.Availability == CXAvailability_Available ||
                                  R.Availability == CXAvailability_Deprecated),
                                 std::move(R.FixIts));
     Result.ShadowDecl = Using;
-    AddResult(Result, CurContext, Hiding);
+    AddResult(Result, CurContext, Hiding, /*InBaseClass=*/false,
+              /*BaseExprType=*/BaseExprType);
     return;
   }
 
   bool AsNestedNameSpecifier = false;
   if (!isInterestingDecl(R.Declaration, AsNestedNameSpecifier))
     return;
 
   // C++ constructors are never found by name lookup.
if (const auto *Method = dyn_cast<CXXMethodDecl>(R.Declaration))
             // This overload can't be called, drop it.
             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()));
-    }
-  }
+  R.FunctionCanBeCall = canFunctionBeCalled(R.getDeclaration(), BaseExprType);
 
   // Insert this result into the set of results.
   Results.push_back(R);
 
   if (!AsNestedNameSpecifier)
     MaybeAddConstructorResults(R);
 }
 
 void ResultBuilder::AddResult(Result R) {
   assert(R.Kind != Result::RK_Declaration &&
          "Declaration results need more context");
   Results.push_back(R);
 }
 
 /// Enter into a new scope.
 void ResultBuilder::EnterNewScope() { ShadowMaps.emplace_back(); }
 
CodeCompletionDeclConsumer(
     }
     this->BaseType = BaseType;
   }
 
   void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
                  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 {
     Results.addVisitedContext(Ctx);
   }
 
 private:
   bool IsAccessible(NamedDecl *ND, DeclContext *Ctx) {
if (const FunctionTemplateDecl *FunTmpl =
           dyn_cast<FunctionTemplateDecl>(ND)) {
     AddQualifierToCompletionString(Result, Qualifier, QualifierIsInformative,
                                    Ctx, Policy);
     FunctionDecl *Function = FunTmpl->getTemplatedDecl();
     AddTypedNameChunk(Ctx, Policy, Function, Result);
 
     // Figure out which template parameters are deduced (or have default
     // arguments).
-    llvm::SmallBitVector Deduced;
+    // Note that we're creating a non-empty bit vector so that we can go
+    // through the loop below to omit default template parameters for non-call
+    // cases.
+    llvm::SmallBitVector Deduced(FunTmpl->getTemplateParameters()->size());
+    // Avoid running it if this is not a call: We should emit *all* template
+    // parameters.
+    if (FunctionCanBeCall)
       Sema::MarkDeducedTemplateParameters(Ctx, FunTmpl, Deduced);
     unsigned LastDeducibleArgument;
     for (LastDeducibleArgument = Deduced.size(); LastDeducibleArgument > 0;
          --LastDeducibleArgument) {
       if (!Deduced[LastDeducibleArgument - 1]) {
         // C++0x: Figure out if the template argument has a default. If so,
         // the user doesn't need to type this argument.
         // FIXME: We need to abstract template parameters better!
         bool HasDefaultArg = false;
for (LastDeducibleArgument = Deduced.size(); LastDeducibleArgument > 0;
               cast<TemplateTemplateParmDecl>(Param)->hasDefaultArgument();
         }
 
         if (!HasDefaultArg)
           break;
       }
     }
 
-    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 <class T> void foo(T);
+      // void (*f)(int) = foo;
       Result.AddChunk(CodeCompletionString::CK_LeftAngle);
       AddTemplateParameterChunks(Ctx, Policy, FunTmpl, Result,
                                  LastDeducibleArgument);
       Result.AddChunk(CodeCompletionString::CK_RightAngle);
     }
 
     // Add the function parameters
     Result.AddChunk(CodeCompletionString::CK_LeftParen);

clang/test/CodeCompletion/member-access.cpp

namespace members_using_fixits {
   // CHECK-METHOD-DECLARED-VIA-USING: [#void#]method() (requires fix-it: {333:8-333:9} to "->")
 
   void testField(Baz* ptr) {
     ptr.f
   }
   // 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 <typename T, typename U, typename V = int>
+    T foo(U, V);
+  };
+
+  &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 : [#T#]foo<<#typename T#>, <#typename U#>>(<#U#>, <#V#>)
+}

clang/unittests/Sema/CodeCompleteTest.cpp

SaveCompletedFunctions(std::vector<CompletedFunctionDecl> &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())) {
+        auto *ND = R.getDeclaration();
+        if (auto *Template = llvm::dyn_cast<FunctionTemplateDecl>(ND))
+          ND = Template->getTemplatedDecl();
+        if (const auto *FD = llvm::dyn_cast<FunctionDecl>(ND)) {
           CompletedFunctionDecl D;
           D.Name = FD->getNameAsString();
           D.CanBeCall = R.FunctionCanBeCall;
           D.IsStatic = FD->isStatic();
           CompletedFuncDecls.emplace_back(std::move(D));
         }
       }
     }
CollectCompletedFunctions(StringRef Code, std::size_t Point) {
   return Result;
 }
 
 TEST(SemaCodeCompleteTest, FunctionCanBeCall) {
   llvm::Annotations Code(R"cpp(
     struct Foo {
       static int staticMethod();
       int method() const;
+      template <typename T, typename U, typename V = int>
+      T generic(U, V);
+      template <typename T, int U = 3>
+      static T staticGeneric();
       Foo() {
         this->$canBeCall^
         $canBeCall^
         Foo::$canBeCall^
       }
     };
 
     struct Derived : Foo {
+      using Foo::method;
+      using Foo::generic;
       Derived() {
         Foo::$canBeCall^
       }
     };
 
     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");
 
   for (const auto &P : Code.points("canBeCall")) {
     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
   for (const auto &P : Code.points()) {
     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))));
   }
 }
 
 TEST(SemaCodeCompleteTest, VisitedNSForValidQualifiedId) {
   auto VisitedNS = runCodeCompleteOnCode(R"cpp(
      namespace ns1 {}
      namespace ns2 {}
      namespace ns3 {}