[clangd][c++20] Drop first template argument in code completion in some contexts.

[massberg]

Summary

In case of a top level context the first template argument of a concept
should be dropped. Currently the indexer doesn't support different
signatures for different contexts (for an index entry always the default
Symbol context is used). Thus we add a hack which checks if we are in
a top level context and have a concept and in that case removes the
first argument of the signature and snippet suffix. If there is only a
single argument, the signature and snippet suffix are completely
removed. The check for the first argument is done by simply looking for
the first comma which should be sufficient in most cases.

[sammccall]

Cool! This makes sense, and is much cheaper than actually working out how to render the abbreviated arg list and store it in the index.

Nice that TopLevel gives us some of these contexts, but I suspect it's not all. (If not, it's fine to handle just this case for now and leave the others as tests showing the bad behavior with a FIXME)

clang-tools-extra/clangd/CodeComplete.cpp

321

RemoveFirst => removeFirst

321

(if this function takes StringRef instead you get nicer APIs like split, and less copying)

327

I don't love the arithmetic, the repetition of 2 as the length of the string, and the reliance on the space after the comma.

Maybe: (assuming Signature is a StringRef)

[First, Rest] = Signature.split(",");
if (Rest.empty()) // no comma => one argument => no list needed
  return "";
return ("<" + Rest.ltrim()).str();

479

more important to say why rather than what: which cases?

/// When a concept is used as a type-constraint (e.g. `Iterator auto x`),
/// and in some other contexts, its first type argument is not written.
/// Drop the parameter from the signature.

482

maybe leave a comment:

dropping the first placeholder from the suffix will leave a $2 with no $1.
However, editors appear to deal with this OK.

(assuming you've tested this in vscode)

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

3964

this doesn't look valid - what is U here?
shouldn't requires A be `requires A<...>?

(generally if we're mixing completion cases in one file it's important that they're valid so subsequent ones parse correctly - also to not confuse the reader)

3968–3972

I think it might be worth renaming "other" to "expr", it seems to more clearly describe all the situations here

3974–3975

can you also add tests for

void abbreviated(A auto x) {}

template<A auto X> int constrainedNTTP();

clang-tools-extra/clangd/unittests/TestIndex.h

38 ↗

(On Diff #537108)

this diverges from how the other helpers work: they're mostly creating minimal skeletons and encapsulate the mess of synthesizing USRs.

Instead of extending the function here, we can assign the appropriate fields on the returned symbol at the callsite (or add a local helper to do so)

(objcSym is also out-of-line with the pattern here, but we shouldn't follow it off the rails)

[massberg]

Update code by resolving comments.

[massberg]

clang-tools-extra/clangd/CodeComplete.cpp

327

Thanks! I agree that the code with StringRefs looks much better.

482

Yes I've tested it with vscode and it looks fine. Why is the numbering of the parameters required?

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

3974–3975

The
`
void abbreviated(A auto x) {}
`

case doesn't work yet. I will fix it in a follow-up patch.

[massberg]

Add test case that isn't correctly handled yet and add FIXME comment.

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

482

The numbering defines the logical sequence, in practice which placeholder will be selected first, and the order that pressing <tab> will visit the others in.

I guess this could make sense when they're not parameters but some other snippet placeholders, but honestly this seems more confusing than useful, I'd prefer they were un-numbered and always LTR.

clang-tools-extra/clangd/CodeComplete.cpp

struct ScoredBundleGreater {
   bool operator()(const ScoredBundle &L, const ScoredBundle &R) {
     if (L.second.Total != R.second.Total)
       return L.second.Total > R.second.Total;
     return L.first.front().Name <
            R.first.front().Name; // Earlier name is better.
   }
 };
 
+// Remove the first template argument from Signature.
+// If Signature only contains a single argument an empty string is returned.
+std::string removeFirstTemplateArg(llvm::StringRef Signature) {
+  auto Rest = Signature.split(",").second;
+  if (Rest.empty())
+    return "";
+  return ("<" + Rest.ltrim()).str();
+}
+
 // Assembles a code completion out of a bundle of >=1 completion candidates.
 // Many of the expensive strings are only computed at this point, once we know
 // the candidate bundle is going to be returned.
 //
 // Many fields are the same for all candidates in a bundle (e.g. name), and are
 // computed from the first candidate, in the constructor.
 // Others vary per candidate, so add() must be called for remaining candidates.
 struct CodeCompletionBuilder {
   CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
                         CodeCompletionString *SemaCCS,
                         llvm::ArrayRef<std::string> AccessibleScopes,
                         const IncludeInserter &Includes,
                         llvm::StringRef FileName,
                         CodeCompletionContext::Kind ContextKind,
                         const CodeCompleteOptions &Opts,
                         bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
       : ASTCtx(ASTCtx),
         EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
         IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
     Completion.Deprecated = true; // cleared by any non-deprecated overload.
-    add(C, SemaCCS);
+    add(C, SemaCCS, ContextKind);
     if (C.SemaResult) {
       assert(ASTCtx);
       Completion.Origin |= SymbolOrigin::AST;
       Completion.Name = std::string(llvm::StringRef(SemaCCS->getTypedText()));
       Completion.FilterText = SemaCCS->getAllTypedText();
       if (Completion.Scope.empty()) {
         if ((C.SemaResult->Kind == CodeCompletionResult::RK_Declaration) ||
             (C.SemaResult->Kind == CodeCompletionResult::RK_Pattern))
CodeCompletionBuilder(ASTContext *ASTCtx, const CompletionCandidate &C,
     // Prefer includes that do not need edits (i.e. already exist).
     std::stable_partition(Completion.Includes.begin(),
                           Completion.Includes.end(),
                           [](const CodeCompletion::IncludeCandidate &I) {
                             return !I.Insertion.has_value();
                           });
   }
 
-  void add(const CompletionCandidate &C, CodeCompletionString *SemaCCS) {
+  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();
       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);
       S.ReturnType = std::string(C.IndexResult->ReturnType);
+      if (C.IndexResult->SymInfo.Kind == index::SymbolKind::Concept)
+        IsConcept = true;
     }
+
+    /// When a concept is used as a type-constraint (e.g. `Iterator auto x`),
+    /// and in some other contexts, its first type argument is not written.
+    /// Drop the parameter from the signature.
+    if (IsConcept && ContextKind == CodeCompletionContext::CCC_TopLevel) {
+      S.Signature = removeFirstTemplateArg(S.Signature);
+      // Dropping the first placeholder from the suffix will leave a $2
+      // with no $1.
+      S.SnippetSuffix = removeFirstTemplateArg(S.SnippetSuffix);
+    }
+
     if (!Completion.Documentation) {
       auto SetDoc = [&](llvm::StringRef Doc) {
         if (!Doc.empty()) {
           Completion.Documentation.emplace();
           parseDocumentation(Doc, *Completion.Documentation);
         }
       };
       if (C.IndexResult) {
for (const auto &Item : Bundle) {
       CodeCompletionString *SemaCCS =
           Item.SemaResult ? Recorder->codeCompletionString(*Item.SemaResult)
                           : nullptr;
       if (!Builder)
         Builder.emplace(Recorder ? &Recorder->CCSema->getASTContext() : nullptr,
                         Item, SemaCCS, AccessibleScopes, *Inserter, FileName,
                         CCContextKind, Opts, IsUsingDeclaration, NextTokenKind);
       else
-        Builder->add(Item, SemaCCS);
+        Builder->add(Item, SemaCCS, CCContextKind);
     }
     return Builder->build();
   }
 };
 
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {

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

 TEST(CompletionTest, Concepts) {
   Annotations Code(R"cpp(
     template<class T>
     concept A = sizeof(T) <= 8;
 
     template<$tparam^A U>
     int foo();
 
+    template<typename T>
+    int bar(T t) requires $expr^A<int>;
+
     template<class T>
-    concept b = $other^A<T> && $other^sizeof(T) % 2 == 0 || $other^A<T> && sizeof(T) == 1;
+    concept b = $expr^A && $expr^sizeof(T) % 2 == 0 || $expr^A && sizeof(T) == 1;
+
+    $toplevel^A auto i = 19;
 
-    $other^A<T> auto i = 19;
+    template<$toplevel^A auto i> void constrainedNTTP();
+
+    // FIXME: The first parameter should be dropped in this case.
+    void abbreviated($expr^A auto x) {}
   )cpp");
   TestTU TU;
   TU.Code = Code.code().str();
   TU.ExtraArgs = {"-std=c++20"};
 
-  std::vector<Symbol> Syms = {conceptSym("same_as")};
+  auto Sym = conceptSym("same_as");
+  Sym.Signature = "<typename Tp, typename Up>";
+  Sym.CompletionSnippetSuffix = "<${1:typename Tp}, ${2:typename Up}>";
+  std::vector<Symbol> Syms = {Sym};
   for (auto P : Code.points("tparam")) {
-    ASSERT_THAT(completions(TU, P, Syms).Completions,
-                AllOf(Contains(named("A")), Contains(named("same_as")),
-                      Contains(named("class")), Contains(named("typename"))))
+    ASSERT_THAT(
+        completions(TU, P, Syms).Completions,
+        AllOf(Contains(AllOf(named("A"), signature(""), snippetSuffix(""))),
+              Contains(AllOf(named("same_as"), signature("<typename Up>"),
+                             snippetSuffix("<${2:typename Up}>"))),
         << "Completing template parameter at position " << P;
   }
 
-  for (auto P : Code.points("other")) {
-    EXPECT_THAT(completions(TU, P, Syms).Completions,
-                AllOf(Contains(named("A")), Contains(named("same_as"))))
+  for (auto P : Code.points("toplevel")) {
+    EXPECT_THAT(
+        completions(TU, P, Syms).Completions,
+        AllOf(Contains(AllOf(named("A"), signature(""), snippetSuffix(""))),
+              Contains(AllOf(named("same_as"), signature("<typename Up>"),
+                             snippetSuffix("<${2:typename Up}>")))))
+        << "Completing 'requires' expression at position " << P;
+  }
+
+  for (auto P : Code.points("expr")) {
+    EXPECT_THAT(
+        completions(TU, P, Syms).Completions,
+        AllOf(Contains(AllOf(named("A"), signature("<class T>"),
+                             snippetSuffix("<${1:class T}>"))),
+              Contains(AllOf(
+                  named("same_as"), signature("<typename Tp, typename Up>"),
+                  snippetSuffix("<${1:typename Tp}, ${2:typename Up}>")))))
         << "Completing 'requires' expression at position " << P;
   }
 }
 
 TEST(SignatureHelp, DocFormat) {
   Annotations Code(R"cpp(
     // Comment `with` markup.
     void foo(int);