[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-28 Thread Younan Zhang via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG23ef8bf9c0f3: [clangd][CodeComplete] Improve 
FunctionCanBeCall (authored by zyounan).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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 
+  T generic(U, V);
+  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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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 
+T foo(U, V);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -310,6 +310,23 @@
   bool isInterestingDecl(const NamedDecl *ND,
  bool ) 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.

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-28 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 557443.
zyounan added a comment.

Fix the CI


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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 
+  T generic(U, V);
+  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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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 
+T foo(U, V);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -310,6 +310,23 @@
   bool isInterestingDecl(const NamedDecl *ND,
  bool ) 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

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-28 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 557442.
zyounan added a comment.

Sorry, wrong patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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 
+  T generic(U, V);
+  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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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 
+T foo(U, V);
+  };
+
+  ::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#>{#, <#typename V#>#}>(<#U#>, <#V#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -310,6 +310,23 @@
   bool isInterestingDecl(const NamedDecl *ND,
  bool ) 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

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-28 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 557441.
zyounan added a comment.

Address nits; discard default template parameters


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp

Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -2239,6 +2239,16 @@
   llvm_unreachable("Invalid Type Class!");
 }
 
+Sema::TemplateDeductionResult Sema::DeduceTemplateArgumentsByTypeMatch(
+Sema , TemplateParameterList *TemplateParams, QualType P, QualType A,
+sema::TemplateDeductionInfo ,
+SmallVectorImpl , unsigned TDF,
+bool PartialOrdering, bool DeducedFromArrayBound) {
+  return ::DeduceTemplateArgumentsByTypeMatch(S, TemplateParams, P, A, Info,
+  Deduced, TDF, PartialOrdering,
+  DeducedFromArrayBound);
+}
+
 static Sema::TemplateDeductionResult
 DeduceTemplateArguments(Sema , TemplateParameterList *TemplateParams,
 const TemplateArgument , TemplateArgument A,
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12460,6 +12460,7 @@
 //   overloaded functions considered.
 FunctionDecl *Specialization = nullptr;
 TemplateDeductionInfo Info(FailedCandidates.getLocation());
+// TargetFunctionType->dump(llvm::errs(), Context);
 if (Sema::TemplateDeductionResult Result
   = S.DeduceTemplateArguments(FunctionTemplate,
   ,
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -41,6 +41,7 @@
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/SemaInternal.h"
+// #include "clang/Sema/Template.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallBitVector.h"
@@ -1463,6 +1464,38 @@
 OverloadSet.Add(Method, Results.size());
   }
 
+#if 0
+  const FunctionTemplateDecl *FTD = nullptr;
+  FTD = dyn_cast(R.Declaration);
+  llvm::errs() << "SCC 0: " << (void *)FTD << " " << PreferredType.isNull()
+   << "\n";
+  if (FTD && PreferredType) {
+auto Pointee = PreferredType->getTypePtr()->getPointeeType();
+llvm::errs() << "SCC 1\n";
+if (!Pointee.isNull()) {
+  llvm::errs() << "SCC 2\n";
+  if (const auto *FT = Pointee->getAs()) {
+llvm::errs() << "SCC 3\n";
+sema::TemplateDeductionInfo Info(SourceLocation{});
+llvm::SmallVector Deduced;
+Deduced.resize(FTD->getTemplateParameters()->size());
+FT->dump();
+if (auto Result = Sema::DeduceTemplateArgumentsByTypeMatch(
+SemaRef, FTD->getTemplateParameters(),
+FTD->getTemplatedDecl()->getType(),
+FT->getCanonicalTypeInternal(), Info, Deduced, 48)) {
+  llvm::errs() << FTD->getNameAsString()
+   << " could not be deduced from enclosing type: "
+   << Result << "\n";
+} else
+  llvm::errs() << FTD->getNameAsString()
+   << " could be deduced from enclosing type"
+   << "\n";
+  }
+}
+  }
+#endif
+
   R.FunctionCanBeCall = canFunctionBeCalled(R.getDeclaration(), BaseExprType);
 
   // Insert this result into the set of results.
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -9138,7 +9138,7 @@
 /// a dependent parameter type did not match the corresponding element
 /// of the corresponding argument (when deducing from an initializer list).
 TDK_DeducedMismatchNested,
-/// A non-depnedent component of the parameter did not match the
+/// A non-dependent component of the parameter did not match the
 /// corresponding component of the argument.
 TDK_NonDeducedMismatch,
 /// When performing template argument deduction for a function
@@ -9232,6 +9232,12 @@
   sema::TemplateDeductionInfo ,
   bool IsAddressOfFunction = false);
 
+  static Sema::TemplateDeductionResult DeduceTemplateArgumentsByTypeMatch(
+  Sema , TemplateParameterList *TemplateParams, QualType P, QualType A,
+  sema::TemplateDeductionInfo ,
+  SmallVectorImpl , unsigned TDF,
+  bool PartialOrdering = false, bool DeducedFromArrayBound = false);
+

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-28 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan marked 3 inline comments as done.
zyounan added a comment.

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




Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:594
+Contains(AllOf(named("generic"),
+   signature("(U, V)"),
+   snippetSuffix("<${1:typename T}, ${2:typename U}>";

nridge wrote:
> 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](https://searchfox.org/llvm/rev/4c241a9335c3046e418e1b4afc162bc576c072fd/clang-tools-extra/clangd/CodeCompletionStrings.cpp#213-214),
>  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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-25 Thread Nathan Ridge via Phabricator via cfe-commits 
nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:594
+Contains(AllOf(named("generic"),
+   signature("(U, V)"),
+   snippetSuffix("<${1:typename T}, ${2:typename U}>";

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](https://searchfox.org/llvm/rev/4c241a9335c3046e418e1b4afc162bc576c072fd/clang-tools-extra/clangd/CodeCompletionStrings.cpp#213-214),
 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.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1301
+
+  // When completing a non-static member function (and not via
+  // dot/arrow member access) and we're not inside that class' scope,

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()`)



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3589
 llvm::SmallBitVector Deduced;
-Sema::MarkDeducedTemplateParameters(Ctx, FunTmpl, Deduced);
+// Avoid running it if this is not a call: We'd emit *all* template
+// parameters.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-17 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan added a comment.

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




Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // 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) 
{

nridge wrote:
> zyounan wrote:
> > sammccall wrote:
> > > 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(...);
> > > }
> > > ```
> > > 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. 
> 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 */)`.
> I think it may be clearest to move this entire if block into the new function

Nice suggestion. I'll take it.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
   // 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.

zyounan wrote:
> nridge wrote:
> > zyounan wrote:
> > > nridge wrote:
> > > > 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 
> > > > "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe add a FIXME 
> > > > for this?
> > > > 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.
> > 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 
> >   T convertTo(U from);
> > };
> > 
> > void bar() {
> >   Foo::^
> > }
> > ```
> > 
> > Here, `U` can be deduced but `T` cannot.
> > 
> > The current behaviour is to complete `convertTo`. 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`?
> > But since this is not a call, wouldn't it be better to complete 
> > convertTo?
> 
> (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

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-17 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 556909.
zyounan marked 2 inline comments as done.
zyounan added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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 
+  T generic(U, V);
+  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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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 
+T foo(U, V);
+  };
+
+  ::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#>{#, <#typename V#>#}>(<#U#>, <#V#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -310,6 +310,23 @@
   bool isInterestingDecl(const NamedDecl *ND,
  bool ) 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

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-10 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan added inline comments.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
   // 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.

nridge wrote:
> zyounan wrote:
> > nridge wrote:
> > > 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 "](https://eel.is/c++draft/temp.deduct.funcaddr)). 
> > > Maybe add a FIXME for this?
> > > 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.
> 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 
>   T convertTo(U from);
> };
> 
> void bar() {
>   Foo::^
> }
> ```
> 
> Here, `U` can be deduced but `T` cannot.
> 
> The current behaviour is to complete `convertTo`. 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`?
> But since this is not a call, wouldn't it be better to complete convertTo 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. :-( )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-09-05 Thread Nathan Ridge via Phabricator via cfe-commits 
nridge requested changes to this revision.
nridge added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1297
+FunctionCanBeCall =
+MaybeDerived == MaybeBase || MaybeDerived->isDerivedFrom(MaybeBase);
+  }

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



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // 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) 
{

zyounan wrote:
> sammccall wrote:
> > 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(...);
> > }
> > ```
> > 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. 
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 */)`.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
   // 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.

zyounan wrote:
> nridge wrote:
> > 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 "](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe 
> > add a FIXME for this?
> > 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.
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 
  T convertTo(U from);
};

void bar() {
  Foo::^
}
```

Here, `U` can be deduced but `T` cannot.

The current behaviour is to complete `convertTo`. 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`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-26 Thread Nathan Ridge via Phabricator via cfe-commits 
nridge added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-26 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan added a comment.

Gently ping~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-19 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 551736.
zyounan added a comment.

Sorry, forgot to update tests :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ 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);
+

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-19 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 551730.
zyounan added a comment.

Fix the bool expression.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ 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);
+

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-19 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan added a comment.

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.)




Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
 case CodeCompletionString::CK_RightParen:
+  if (DropFunctionArguments &&
+  ResultKind == CodeCompletionResult::RK_Declaration)

sammccall wrote:
> nridge wrote:
> > 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
> 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 TruncateSnippetAt;
> ...
> case CK_LeftBracket:
>   if (DropFunctionArguments && ... && !TruncateSnippetAt)
> TruncateSnippetAt = Snippet->size();
> ...
> if (TruncateSnippetAt)
>   Snippet->resize(*TruncateSnippetAt);
> }
> ```
> 
> (though still not totally clear)
> 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.




Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
 auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
 EXPECT_THAT(Results.Completions,
 Contains(AllOf(named("method"), snippetSuffix("";

nridge wrote:
> Since you're updating this test anyways, could you add `signature()` matchers 
> for the non-template cases as well please?
Of course!



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+ bool InBaseClass, QualType BaseType);
 

nridge wrote:
> 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?
Makes sense to me. Thanks for the correction.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
 Result.ShadowDecl = Using;
 AddResult(Result, CurContext, Hiding);
 return;

nridge wrote:
> Should we propagate `BaseType` into this recursive call?
Yes. And thanks for spotting this. I added another test case reflecting it.



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // 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) 
{

sammccall wrote:
> 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(...);
> }
> ```
> 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. 



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+R.FunctionCanBeCall =
+MaybeDerived == MaybeBase || 
MaybeDerived->isDerivedFrom(MaybeBase);
+  }

nridge wrote:
> Is there any situation where `MaybeDerived == MaybeBase || 
>

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-19 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 551723.
zyounan marked 14 inline comments as done.
zyounan added a comment.

Address many comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
+;
+d.Derived::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$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  : 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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ 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,
-

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-16 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

Thanks, this does look better.

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




Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:144
+
+  // This variable would be discarded directly at the end of this function. We
+  // store part of the chunks of snippets here if DropFunctionArguments is

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.



Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:148
+  // Signature for the function would be preserved.
+  // It is preferable if we don't produce the arguments at the clang site. But
+  // that would also lose the signatures, which could sometimes help users to

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



Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
 case CodeCompletionString::CK_RightParen:
+  if (DropFunctionArguments &&
+  ResultKind == CodeCompletionResult::RK_Declaration)

nridge wrote:
> 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
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 TruncateSnippetAt;
...
case CK_LeftBracket:
  if (DropFunctionArguments && ... && !TruncateSnippetAt)
TruncateSnippetAt = Snippet->size();
...
if (TruncateSnippetAt)
  Snippet->resize(*TruncateSnippetAt);
}
```

(though still not totally clear)



Comment at: clang-tools-extra/clangd/CodeCompletionStrings.h:45
 ///
+/// \p DropFunctionArguments indicates that the function call arguments should
+/// be omitted from the \p Snippet. If enabled, the \p Snippet will only 
contain

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



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1387
 
-  // 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) 
{

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(...);
}
```



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1417
+
+  // If the member access "." or "->" is followed by a qualified Id and the
+  // object type is derived from or the same as that of the Id, then

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`.





Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1420
+  // the candidate functions should be perceived as calls.
+  if (const CXXRecordDecl *MaybeDerived = nullptr;
+  !BaseType.isNull() &&

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-13 Thread Nathan Ridge via Phabricator via cfe-commits 
nridge added a comment.

Thanks for the patch, these are nice improvements




Comment at: clang-tools-extra/clangd/CodeCompletionStrings.cpp:258
 case CodeCompletionString::CK_RightParen:
+  if (DropFunctionArguments &&
+  ResultKind == CodeCompletionResult::RK_Declaration)

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



Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:536
+template 
+void generic(T);
+template 

Suggestion: it would be slightly more interesting to make the function 
signature `generic(U)`. Then, the function can be called as `generic(u)` 
(with the template parameter `U` deduced), and the 
[LastDeducibleArgument](https://searchfox.org/llvm/rev/bd7c6e3c48e9281ceeaae3a93cc493b35a3c9f29/clang/lib/Sema/SemaCodeComplete.cpp#3553)
 logic should make sure that only `` is added to the code completion string, 
not ``.



Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:580
 auto Results = completions(TU, P, /*IndexSymbols*/ {}, Opts);
 EXPECT_THAT(Results.Completions,
 Contains(AllOf(named("method"), snippetSuffix("";

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



Comment at: 
clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp:178
+  /*DropFunctionArguments=*/true);
+  EXPECT_EQ(Signature, "(arg1, arg2)");
+  EXPECT_EQ(Snippet, "<${1:typename T}, ${2:int U}>");

Maybe add:

```
// Arguments dropped from snippet, kept in signature
```

so someone reading the test knows it's deliberate



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:345
   void AddResult(Result R, DeclContext *CurContext, NamedDecl *Hiding,
- bool InBaseClass);
+ bool InBaseClass, QualType BaseType);
 

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?



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1285
 Result.ShadowDecl = Using;
 AddResult(Result, CurContext, Hiding);
 return;

Should we propagate `BaseType` into this recursive call?



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:1425
+R.FunctionCanBeCall =
+MaybeDerived == MaybeBase || 
MaybeDerived->isDerivedFrom(MaybeBase);
+  }

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;
}
```



Comment at: clang/lib/Sema/SemaCodeComplete.cpp:3577
   // 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.

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 
"](https://eel.is/c++draft/temp.deduct.funcaddr)). Maybe add a FIXME for this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156605: [clangd][CodeComplete] Improve FunctionCanBeCall

2023-08-06 Thread Younan Zhang via Phabricator via cfe-commits 
zyounan updated this revision to Diff 547569.
zyounan added a comment.

Trigger the build


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156605/new/

https://reviews.llvm.org/D156605

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.cpp
  clang-tools-extra/clangd/CodeCompletionStrings.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/CodeCompletionStringsTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/member-access.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ 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(T);
+  template 
+  static T staticGeneric();
   Foo() {
 this->$canBeCall^
 $canBeCall^
@@ -207,15 +214,25 @@
 struct OtherClass {
   OtherClass() {
 Foo f;
+Derived d;
 f.$canBeCall^
+; // Prevent parsing as 'f.f'
+f.Foo::$canBeCall^
 ::$cannotBeCall^
+;
+d.Foo::$canBeCall^
   }
 };
 
 int main() {
   Foo f;
+  Derived d;
   f.$canBeCall^
+  ; // Prevent parsing as 'f.f'
+  f.Foo::$canBeCall^
   ::$cannotBeCall^
+  ;
+  d.Foo::$canBeCall^
 }
 )cpp");
 
@@ -223,12 +240,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  : 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 +257,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;
   }
 }
 
Index: clang/test/CodeCompletion/member-access.cpp
===
--- clang/test/CodeCompletion/member-access.cpp
+++ 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);
+  };
+
+  ::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#>)
+}
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ 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 BaseType 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 BaseType);
 
   /// Add a new non-declaration result to this result set.
   void AddResult(Result R);
@@ -1262,7 +1265,8 @@
 }
 
 void ResultBuilder::AddResult(Result R, DeclContext *CurContext,
-