[clangd] Add code completion of param name on /* inside function calls.

[adamcz]

Summary

For example, if you have:

void foo(int bar);
foo(/*^

it should auto-complete to "bar=".

Because Sema callbacks for code completion in comments happen before we
have an AST we need to cheat in clangd by detecting completion on /*
before, moving cursor back by two characters, then running a simplified
verion of SignatureHelp to extract argument name(s) from possible
overloads.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

1148

change the ending comment (well, I'd actually drop it completely)

1925

i think it's subtle that we require a fullpatch here. maybe put a comment saying that "we want to see signatures coming from newly introduced includes, hence a full patch".

1972

what if the user triggers at foo(/* b^? I think we'd still want to provide bar.

I think we should detect whether we're in a comment first, we can run the Lexer for that but it'd probably be an overkill. For now I think we can just check for the string after last */, if it has a /* we're in a comment, otherwise we check for the current line only. if it has a // we're again in a comment.

Then we can switch to the codeCompleteComment mode, inside there one branch can collect completion results for possible parameter names (while filtering using the currently typed text) and we can expand with more logic later on (e.g. suggest identifiers from index?).

WDYT?

1979

i think we should propagate Opts here and then override the pieces accordingly.

[adamcz]

addressed review comments

clang-tools-extra/clangd/CodeComplete.cpp

1148

Oops, too much copy/paste.

We should have a clang-tidy check for that ;-)

1972

Good point about /* b^, thanks! I did not consider manually triggered code completion at all.

Detecting last */ the way you suggest seems fragile. There's macros that can expand to /* or */, there's strings, lines that are already comments (i.e. "// blah /* blah), "#if 0" sections, etc.
Running Lexer is an option, but I don't think it's necessary. Currently there's very few options when we can offer code completion:
/*^
/* ^
/*foo^
/* foo^
This can be quickly detected by scanning back through whitespace, then valid identifier characters, then whitespace again.

Since it's unclear if and when we'll be adding more code completion in comments I want to keep this simple rather than build for the future when we may indeed need a lexer.

WDYT?

(also added some tests to cover this).

1979

Hmm...why?

It seems that everything that could be relevant would be overridden here. SignatureHelp, which is very similar to this, doesn't use code complete options either. I think it may be confusing to accept an object but then override literally every single thing that matters anyway.

It's not unlikely that in the future we'll need options, but for now it seems useless. Do you have a specific option you want propagated?

[kadircet]

thanks, mostly LG, a couple of nits :)

clang-tools-extra/clangd/CodeComplete.cpp

1120

nit: early exit

auto *Func = ...;
if(!Func)
  continue;
...

1124

nit: !PVD same for Ident

1145

nit: Why not directly have an llvm::DenseSet here? I am not sure if the order we're preserving currently is a good one anyway (depends a lot on the order of includes, i think?). If we really want to preserve some ordering, it would probably make sense to have the lexicographical one instead.

1963

let's name it more explicitly that this only checks if user is likely to be completing for function argument comment.

maybeFunctionArgumentComment ?

documentation just needs to reflect that fact now rather than explaining the being only type of comment completion supported and such.

1964

this might read easier as:

while (!Contents.empty() && isAciiIdentifierContinue(Contents.back())
  Contents = Contents.drop_back();
Contents.rtrim(); // by default this drops the chars you have in `Whitespace` already.
Offset = Contents.size();
return Contents.endswith("/*");

this has the different behaviour for /* qwer ^ but I don't think we want to complete after a word anyway (that's not what we do in the rest of the completion, space terminates the previous identifier).

As for propagating prefix, I think it's enough to check that the file content endswith whatever name we're going to suggest in the signature help collector.

nit: As for Offset, I might actually prefer returning an Optional<unsigned> while renaming the function to getFunctionArgumentCommentStart, but i am fine with this too, up to you.

1972

As discussed offline, I was mostly worried about extending this in the future, but we can think about that later. So this is good as-is.

1979

ah nvm. I confused clang::CodeCompleteOptions with clangds

[adamcz]

next round of review comments

clang-tools-extra/clangd/CodeComplete.cpp

1145

I decided to preserve the order to match what SignatureHelp is doing, but I now realize it re-orders them anyway, so good point, fixed.

Note that I used std::set, since that works with std::string and I don't think we can safely return llvm::StringRef here.

1964

First, returning Optional instead of bool + unsigned is better. Done.

Second, you're right that there's no need to return Prefix, it can be just as easily computed later. However, we still need it. I'm not sure what you're suggesting with checking if content ends with name. If it's literally Content.endswith(Name) that means you must have already typed the entire argument name.
I moved the CommentPrefix computation out of this function to make the signature nicer. It's just one line anyway. We can't easily move this to the collector, since it gets the original content, meaning we'd have to pass two offsets (original completion point and the new one) so we can extract something like "fun(/*foo^);" - we need to remove everything before /* and everything after ^. Does that explanation make sense?

Third, your version is better now. Note there is no difference in behavior on "/* qwer ^". There's even a test for that. I think I ended up with that complicated version because of the Prefix extraction, and, ironically, the "/* qwer ^" case - I just confused myself too much ;-)

Thanks

[kadircet]

thanks, LGTM!

clang-tools-extra/clangd/CodeComplete.cpp

1145

right. you can also try llvm::StringMap which owns its keys.

1964

Thanks!

re endswith, yeah that is definitely wrong. I actually had checking a prefix of the suggestion is a suffix of the content, but even that doesn't do the trick (Comes down to what you explained after second point). We somehow need the last (possibly empty) identifier in the contents before cursor instead.

re behaviour, I missed the special if block sitting in the middle checking for trimmed.endswith("/*").

1972

Let's drop this one as you're already checking this in codeCompleteComment (+ it's going to be true almost always, so it's not like this is saving time in the general case)

[adamcz]

final review comment (dropped Preamble check)

clang-tools-extra/clangd/CodeComplete.cpp

1979

Done. Amusingly, this changed the output of the CompletionRange test. Technically it fixed the issue that test was demonstrating, but only for that one very specific case, so I changed the test to continue demonstating the problem (difference between Sema and non-Sema behavior).

clang-tools-extra/clangd/ClangdLSPServer.cpp

llvm::json::Object ServerCaps{
       {"completionProvider",
        llvm::json::Object{
            {"allCommitCharacters",
             {" ", "\t", "(", ")", "[", "]", "{",  "}", "<",
              ">", ":",  ";", ",", "+", "-", "/",  "*", "%",
              "^", "&",  "#", "?", ".", "=", "\"", "'", "|"}},
            {"resolveProvider", false},
            // We do extra checks, e.g. that > is part of ->.
-           {"triggerCharacters", {".", "<", ">", ":", "\"", "/"}},
+           {"triggerCharacters", {".", "<", ">", ":", "\"", "/", "*"}},
        }},
       {"semanticTokensProvider",
        llvm::json::Object{
            {"full", llvm::json::Object{{"delta", true}}},
            {"range", false},
            {"legend",
             llvm::json::Object{{"tokenTypes", semanticTokenTypes()},
                                {"tokenModifiers", semanticTokenModifiers()}}},

clang-tools-extra/clangd/CodeComplete.cpp

private:
   }
 
   SignatureHelp &SigHelp;
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
   const SymbolIndex *Index;
 }; // SignatureHelpCollector
 
+// Used only for completion of C-style comments in function call (i.e.
+// /*foo=*/7). Similar to SignatureHelpCollector, but needs to do less work.
+class ParamNameCollector final : public CodeCompleteConsumer {
+public:
+  ParamNameCollector(const clang::CodeCompleteOptions &CodeCompleteOpts,
+                     std::set<std::string> &ParamNames)
+      : CodeCompleteConsumer(CodeCompleteOpts),
+        Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
+        CCTUInfo(Allocator), ParamNames(ParamNames) {}
+
+  void ProcessOverloadCandidates(Sema &S, unsigned CurrentArg,
+                                 OverloadCandidate *Candidates,
+                                 unsigned NumCandidates,
+                                 SourceLocation OpenParLoc) override {
+    assert(CurrentArg <= (unsigned)std::numeric_limits<int>::max() &&
+           "too many arguments");
+
+    for (unsigned I = 0; I < NumCandidates; ++I) {
+      OverloadCandidate Candidate = Candidates[I];
+      auto *Func = Candidate.getFunction();
+      if (!Func || Func->getNumParams() <= CurrentArg)
+        continue;
+      auto *PVD = Func->getParamDecl(CurrentArg);
+      if (!PVD)
+        continue;
+      auto *Ident = PVD->getIdentifier();
+      if (!Ident)
+        continue;
+      auto Name = Ident->getName();
+      if (!Name.empty())
+        ParamNames.insert(Name.str());
+    }
+  }
+
+private:
+  GlobalCodeCompletionAllocator &getAllocator() override { return *Allocator; }
+
+  CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; }
+
+  std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
+  CodeCompletionTUInfo CCTUInfo;
+  std::set<std::string> &ParamNames;
+};
+
 struct SemaCompleteInput {
   PathRef FileName;
   size_t Offset;
   const PreambleData &Preamble;
   const llvm::Optional<PreamblePatch> Patch;
   const ParseInputs &ParseInput;
 };
 
 void loadMainFilePreambleMacros(const Preprocessor &PP,
                                 const PreambleData &Preamble) {
   // The ExternalPreprocessorSource has our macros, if we know where to look.
   // We can read all the macros using PreambleMacros->ReadDefinedMacros(),
while (Rest.consume_back("::") && !Rest.endswith(":")) // reject ::::
     while (!Rest.empty() && isAsciiIdentifierContinue(Rest.back()))
       Rest = Rest.drop_back();
   Result.Qualifier =
       Content.slice(Rest.size(), Result.Name.begin() - Content.begin());
 
   return Result;
 }
 
+// Code complete the argument name on "/*" inside function call.
+// Offset should be pointing to the start of the comment, i.e.:
+// foo(^/*, rather than foo(/*^) where the cursor probably is.
+CodeCompleteResult codeCompleteComment(PathRef FileName, unsigned Offset,
+                                       llvm::StringRef Prefix,
+                                       const PreambleData *Preamble,
+                                       const ParseInputs &ParseInput) {
+  if (Preamble == nullptr) // Can't run without Sema.
+    return CodeCompleteResult();
+
+  clang::CodeCompleteOptions Options;
+  Options.IncludeGlobals = false;
+  Options.IncludeMacros = false;
+  Options.IncludeCodePatterns = false;
+  Options.IncludeBriefComments = false;
+  std::set<std::string> ParamNames;
+  // We want to see signatures coming from newly introduced includes, hence a
+  // full patch.
+  semaCodeComplete(
+      std::make_unique<ParamNameCollector>(Options, ParamNames), Options,
+      {FileName, Offset, *Preamble,
+       PreamblePatch::createFullPatch(FileName, ParseInput, *Preamble),
+       ParseInput});
+  if (ParamNames.empty())
+    return CodeCompleteResult();
+
+  CodeCompleteResult Result;
+  Result.Context = CodeCompletionContext::CCC_NaturalLanguage;
+  for (llvm::StringRef Name : ParamNames) {
+    if (!Name.startswith(Prefix))
+      continue;
+    CodeCompletion Item;
+    Item.Name = Name.str() + "=";
+    Item.Kind = CompletionItemKind::Text;
+    Result.Completions.push_back(Item);
+  }
+
+  return Result;
+}
+
+// If Offset is inside what looks like argument comment (e.g.
+// "/*^" or "/* foo^"), returns new offset pointing to the start of the /*
+// (place where semaCodeComplete should run).
+llvm::Optional<unsigned>
+maybeFunctionArgumentCommentStart(llvm::StringRef Content) {
+  while (!Content.empty() && isAsciiIdentifierContinue(Content.back()))
+    Content = Content.drop_back();
+  Content = Content.rtrim();
+  if (Content.endswith("/*"))
+    return Content.size() - 2;
+  return None;
+}
+
 CodeCompleteResult codeComplete(PathRef FileName, Position Pos,
                                 const PreambleData *Preamble,
                                 const ParseInputs &ParseInput,
                                 CodeCompleteOptions Opts,
                                 SpeculativeFuzzyFind *SpecFuzzyFind) {
   auto Offset = positionToOffset(ParseInput.Contents, Pos);
   if (!Offset) {
     elog("Code completion position was invalid {0}", Offset.takeError());
     return CodeCompleteResult();
   }
+
+  auto Content = llvm::StringRef(ParseInput.Contents).take_front(*Offset);
+  if (auto OffsetBeforeComment = maybeFunctionArgumentCommentStart(Content)) {
+    // We are doing code completion of a comment, where we currently only
+    // support completing param names in function calls. To do this, we
+    // require information from Sema, but Sema's comment completion stops at
+    // parsing, so we must move back the position before running it, extract
+    // information we need and construct completion items ourselves.
+    auto CommentPrefix = Content.substr(*OffsetBeforeComment + 2).trim();
+    return codeCompleteComment(FileName, *OffsetBeforeComment, CommentPrefix,
+                               Preamble, ParseInput);
+  }
+
   auto Flow = CodeCompleteFlow(
       FileName, Preamble ? Preamble->Includes : IncludeStructure(),
       SpecFuzzyFind, Opts);
   return (!Preamble || Opts.RunParser == CodeCompleteOptions::NeverParse)
              ? std::move(Flow).runWithoutSema(ParseInput.Contents, *Offset,
                                               *ParseInput.TFS)
              : std::move(Flow).run({FileName, *Offset, *Preamble,
                                     /*PreamblePatch=*/

 bool allowImplicitCompletion(llvm::StringRef Content, unsigned Offset) {
   // Look at last line before completion point only.
   Content = Content.take_front(Offset);
   auto Pos = Content.rfind('\n');
   if (Pos != llvm::StringRef::npos)
     Content = Content.substr(Pos + 1);
 
   // Complete after scope operators.
-  if (Content.endswith(".") || Content.endswith("->") || Content.endswith("::"))
+  if (Content.endswith(".") || Content.endswith("->") ||
+      Content.endswith("::") || Content.endswith("/*"))
     return true;
   // Complete after `#include <` and #include `<foo/`.
   if ((Content.endswith("<") || Content.endswith("\"") ||
        Content.endswith("/")) &&
       isIncludeFile(Content))
     return true;
 
   // Complete words. Give non-ascii characters the benefit of the doubt.
   return !Content.empty() && (isAsciiIdentifierContinue(Content.back()) ||
                               !llvm::isASCII(Content.back()));
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/test/initialize-params.test

 # CHECK-NEXT:        ],
 # CHECK-NEXT:        "resolveProvider": false,
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          "<",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":",
 # CHECK-NEXT:          "\"",
-# CHECK-NEXT:          "/"
+# CHECK-NEXT:          "/",
+# CHECK-NEXT:          "*"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "declarationProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
 # CHECK-NEXT:      "documentHighlightProvider": true,
 # CHECK-NEXT:      "documentLinkProvider": {
 # CHECK-NEXT:        "resolveProvider": false

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

TEST(CompletionTest, CompletionRange) {
   const char *EmptyRange = "auto x = [[]]^";
   Completions = completions(EmptyRange);
   EXPECT_EQ(Completions.CompletionRange, Annotations(EmptyRange).range());
   Completions = completionsNoCompile(EmptyRange);
   EXPECT_EQ(Completions.CompletionRange, Annotations(EmptyRange).range());
 
   // Sema doesn't trigger at all here, while the no-sema completion runs
   // heuristics as normal and reports a range. It'd be nice to be consistent.
-  const char *NoCompletion = "/* [[]]^ */";
+  const char *NoCompletion = "/* foo [[]]^ */";
   Completions = completions(NoCompletion);
   EXPECT_EQ(Completions.CompletionRange, llvm::None);
   Completions = completionsNoCompile(NoCompletion);
   EXPECT_EQ(Completions.CompletionRange, Annotations(NoCompletion).range());
 }
 
 TEST(NoCompileCompletionTest, Basic) {
   auto Results = completionsNoCompile(R"cpp(
TEST(CompletionTest, PreambleCodeComplete) {
 
   MockFS FS;
   auto Inputs = ModifiedTU.inputs(FS);
   auto Result = codeComplete(testPath(ModifiedTU.Filename), Test.point(),
                              BaselineTU.preamble().get(), Inputs, {});
   EXPECT_THAT(Result.Completions, Not(testing::IsEmpty()));
 }
 
+TEST(CompletionTest, CommentParamName) {
+  clangd::CodeCompleteOptions Opts;
+  const std::string Code = R"cpp(
+    void fun(int foo, int bar);
+    void overloaded(int param_int);
+    void overloaded(int param_int, int param_other);
+    void overloaded(char param_char);
+    int main() {
+  )cpp";
+
+  EXPECT_THAT(completions(Code + "fun(/*^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("foo=")));
+  EXPECT_THAT(completions(Code + "fun(1, /*^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("bar=")));
+  EXPECT_THAT(completions(Code + "/*^", {}, Opts).Completions, IsEmpty());
+  // Test de-duplication.
+  EXPECT_THAT(
+      completions(Code + "overloaded(/*^", {}, Opts).Completions,
+      UnorderedElementsAre(Labeled("param_int="), Labeled("param_char=")));
+  // Comment already has some text in it.
+  EXPECT_THAT(completions(Code + "fun(/*  ^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("foo=")));
+  EXPECT_THAT(completions(Code + "fun(/* f^", {}, Opts).Completions,
+              UnorderedElementsAre(Labeled("foo=")));
+  EXPECT_THAT(completions(Code + "fun(/* x^", {}, Opts).Completions, IsEmpty());
+  EXPECT_THAT(completions(Code + "fun(/* f ^", {}, Opts).Completions,
+              IsEmpty());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang