[clangd] Don't produce snippets when completion location is followed by parenthesis

[kbobyrev]

Summary

Prevent a second pair of parenthesis from being added when there already is one
right after cursor.

Related issue and more context: https://github.com/clangd/clangd/issues/387

[sammccall]

Thanks, this is nice!

clang-tools-extra/clangd/CodeComplete.cpp

1240

I'd suggest rather storing the token kind as NextToken, and then deferring the actual "is it l_paren" check until toCodeCompletion.
That way the should-we-generate-snippets logic is more localized and IMO easier to read.

1295

need to decide what to do when it returns none, or add an explicit assert with a message ("code completing in macro?")

1714

This is a bit lengthy for describing an implementation strategy we're *not* using. Do you think we're very likely to do this, and this comment would save a lot of the work of finding out how?

I'd rather add a comment explaining what we *are* doing
e.g. Suppress function argument snippets if args are already present, or not needed (using decl).
And at most // Should we consider sometimes replacing parens with the snippet instead?

1720

how sure are we that paren-after is the right condition in all cases? Does "snippet" cover template snippets (std::vector<{$0}>)?

Don't need to handle this but it'd be nice to cover in tests.

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

2878

nit: I don't really follow this test name, but it seems to be explaining part of the implementation.

What about FunctionArgsExist which rather explains the scenario?

2890

can we have fo^o(42) as a case as well?

[kbobyrev]

Make sure snippets are omitted only in the function/method/constructor calls
for safety and extend the testset.

clang-tools-extra/clangd/CodeComplete.cpp

1295

Yeah, forgot about that -_- I think just doing nothing is OK here?

1714

I'd like to do that next patch (this patch is actually a simplified version of what I *wanted* to do, but I figured there are too many corner cases and complications. I would like to get to that very soon but I'm just afraid I'll be distracted or something. I should probably just write this down for myself, that would be fine.

1720

Yeah templates are hard :( The problem here is that this is a heuristic and I was considering suppressing pieces of the CC snippets (e.g. the templated parts) upon having < token ahead but this can also be "less than" sign and the heuristics are probably going to be harder for this. Maybe some semantic analysis would do, but I'm not sure about all the details right now.

I've added a test which shows how the template snippet will be omitted, is that OK with you?

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

2890

We can, but it wouldn't be handled "as expected" :) The problem would be that the next token is an identifier (o) and hence this wouldn't work. I'd like to fix that too (see if the next token is an identifier and a suffix of inserted text), but I think this should be a separate patch.

Do you want me to add this case with a FIXME?

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

1295

Yup EOF LGTM.

1718

now you've got the "should we generate snippets" logic split a different way :-) No-snippets-for-using-decl is expressed here, but no-args-if-parens-exist is inside the builder.

I think it's slightly neater (less plumbing) to compute it here - it's safe to do so based on the current item because we won't produce a bundle containing functions together with other things.

Also fine to pass in IsUsingDeclaration, but it should be to a parameter called "IsUsingDeclaration" rather than one called GenerateSnippets, as it's only part of the equation.

1720

Yes, adding a test showing we don't handle this edge case is fine.

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

2890

Yes, no need to handle it, but let's add a test and mark it as bad.

2918

Add a fixme: should be "<${1:typename T}>"

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

1720

BTW it might be possible to handle this robustly by having Preprocessor::Lex look ahead once it reaches the CC token and call setCodeCompletionTokenRange appropriately. You'd then have to look for the token at the endpoint of this range, rather than the one after the code completion point.

Clangd is the only (in-tree) user of the code completion token range, we use it to decide what to replace.

[kbobyrev]

Address most comments.

clang-tools-extra/clangd/CodeComplete.cpp

1718

Also fine to pass in IsUsingDeclaration, but it should be to a parameter called "IsUsingDeclaration" rather than one called GenerateSnippets, as it's only part of the equation.

Yeah I thought it might be a bit confusing to split this up, but couldn't figure out what would be best option. This sounds logical, will do!

I think it's slightly neater (less plumbing) to compute it here - it's safe to do so based on the current item because we won't produce a bundle containing functions together with other things.

I'm a bit confused: I moved the l_brace token detection logic into the builder because it resolves the completion kind, so computing it here would require the completion item kind to be resolved at this point. So are you suggesting moving the completion kind resolution out of the builder here and then passing it to the builder along with GenerateSnippets?

[kbobyrev]

Don't split the snippet omitting logic between the builder and its caller.

[sammccall]

Ship it!

clang-tools-extra/clangd/CodeComplete.cpp

1718

Yeah, I didn't see that we don't yet have access to the kind here.

clang-tools-extra/clangd/CodeComplete.cpp

 #include "support/Logger.h"
 #include "support/Threading.h"
 #include "support/Trace.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/ExternalPreprocessorSource.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"

 // 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> QueryScopes,
                         const IncludeInserter &Includes,
                         llvm::StringRef FileName,
                         CodeCompletionContext::Kind ContextKind,
-                        const CodeCompleteOptions &Opts, bool GenerateSnippets)
+                        const CodeCompleteOptions &Opts,
+                        bool IsUsingDeclaration, tok::TokenKind NextTokenKind)
       : ASTCtx(ASTCtx), ExtractDocumentation(Opts.IncludeComments),
         EnableFunctionArgSnippets(Opts.EnableFunctionArgSnippets),
-        GenerateSnippets(GenerateSnippets) {
+        IsUsingDeclaration(IsUsingDeclaration), NextTokenKind(NextTokenKind) {
     add(C, SemaCCS);
     if (C.SemaResult) {
       assert(ASTCtx);
       Completion.Origin |= SymbolOrigin::AST;
       Completion.Name = std::string(llvm::StringRef(SemaCCS->getTypedText()));
       if (Completion.Scope.empty()) {
         if ((C.SemaResult->Kind == CodeCompletionResult::RK_Declaration) ||
             (C.SemaResult->Kind == CodeCompletionResult::RK_Pattern))
private:
 
   std::string summarizeReturnType() const {
     if (auto *RT = onlyValue<&BundledEntry::ReturnType>())
       return *RT;
     return "";
   }
 
   std::string summarizeSnippet() const {
-    if (!GenerateSnippets)
+    if (IsUsingDeclaration)
+      return "";
+    // Suppress function argument snippets if args are already present.
+    if ((Completion.Kind == CompletionItemKind::Function ||
+         Completion.Kind == CompletionItemKind::Method ||
+         Completion.Kind == CompletionItemKind::Constructor) &&
+        NextTokenKind == tok::l_paren)
       return "";
     auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
     if (!Snippet)
       // All bundles are function calls.
       // FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
       // we need to complete 'forward<$1>($0)'.
       return "($0)";
     if (EnableFunctionArgSnippets)
private:
   }
 
   // ASTCtx can be nullptr if not run with sema.
   ASTContext *ASTCtx;
   CodeCompletion Completion;
   llvm::SmallVector<BundledEntry, 1> Bundled;
   bool ExtractDocumentation;
   bool EnableFunctionArgSnippets;
-  /// When false, no snippets are generated argument lists.
-  bool GenerateSnippets;
+  // No snippets will be generated for using declarations and when the function
+  // arguments are already present.
+  bool IsUsingDeclaration;
+  tok::TokenKind NextTokenKind;
 };
 
 // Determine the symbol ID for a Sema code completion result, if possible.
 llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R,
                                      const SourceManager &SM) {
   switch (R.Kind) {
   case CodeCompletionResult::RK_Declaration:
   case CodeCompletionResult::RK_Pattern: {
class CodeCompleteFlow {
   IncludeStructure Includes;           // Complete once the compiler runs.
   SpeculativeFuzzyFind *SpecFuzzyFind; // Can be nullptr.
   const CodeCompleteOptions &Opts;
 
   // Sema takes ownership of Recorder. Recorder is valid until Sema cleanup.
   CompletionRecorder *Recorder = nullptr;
   CodeCompletionContext::Kind CCContextKind = CodeCompletionContext::CCC_Other;
   bool IsUsingDeclaration = false;
+  // The snippets will not be generated if the token following completion
+  // location is an opening parenthesis (tok::l_paren) because this would add
+  // extra parenthesis.
+  tok::TokenKind NextTokenKind = tok::eof;
   // Counters for logging.
   int NSema = 0, NIndex = 0, NSemaAndIndex = 0, NIdent = 0;
   bool Incomplete = false; // Would more be available with a higher limit?
   CompletionPrefix HeuristicPrefix;
   llvm::Optional<FuzzyMatcher> Filter; // Initialized once Sema runs.
   Range ReplacedRange;
   std::vector<std::string> QueryScopes; // Initialized once Sema runs.
   // Initialized once QueryScopes is initialized, if there are scopes.
CodeCompleteResult run(const SemaCompleteInput &SemaCCInput) && {
     CodeCompleteResult Output;
     auto RecorderOwner = std::make_unique<CompletionRecorder>(Opts, [&]() {
       assert(Recorder && "Recorder is not set");
       CCContextKind = Recorder->CCContext.getKind();
       IsUsingDeclaration = Recorder->CCContext.isUsingDeclaration();
       auto Style = getFormatStyleForFile(
           SemaCCInput.FileName, SemaCCInput.ParseInput.Contents,
           SemaCCInput.ParseInput.FSProvider->getFileSystem().get());
+      const auto NextToken = Lexer::findNextToken(
+          Recorder->CCSema->getPreprocessor().getCodeCompletionLoc(),
+          Recorder->CCSema->getSourceManager(), Recorder->CCSema->LangOpts);
+      if (NextToken)
+        NextTokenKind = NextToken->getKind();
       // If preprocessor was run, inclusions from preprocessor callback should
       // already be added to Includes.
       Inserter.emplace(
           SemaCCInput.FileName, SemaCCInput.ParseInput.Contents, Style,
           SemaCCInput.ParseInput.CompileCommand.Directory,
           &Recorder->CCSema->getPreprocessor().getHeaderSearchInfo());
       for (const auto &Inc : Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
private:
   }
 
   CodeCompletion toCodeCompletion(const CompletionCandidate::Bundle &Bundle) {
     llvm::Optional<CodeCompletionBuilder> Builder;
     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, QueryScopes, *Inserter, FileName,
-                        CCContextKind, Opts,
-                        /*GenerateSnippets=*/!IsUsingDeclaration);
+                        CCContextKind, Opts, IsUsingDeclaration, NextTokenKind);
       else
         Builder->add(Item, SemaCCS);
     }
     return Builder->build();
   }
 };
 
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   clang::CodeCompleteOptions Result;

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

for (const char *Test : Yes) {
     EXPECT_TRUE(allowImplicitCompletion(A.code(), A.point())) << Test;
   }
   for (const char *Test : No) {
     llvm::Annotations A(Test);
     EXPECT_FALSE(allowImplicitCompletion(A.code(), A.point())) << Test;
   }
 }
 
+TEST(CompletionTest, FunctionArgsExist) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.EnableSnippets = true;
+  std::string Context = R"cpp(
+    int foo(int A);
+    int bar();
+    struct Object {
+      Object(int B) {}
+    };
+    template <typename T>
+    struct Container {
+      Container(int Size) {}
+    };
+  )cpp";
+  EXPECT_THAT(completions(Context + "int y = fo^", {}, Opts).Completions,
+              UnorderedElementsAre(
+                  AllOf(Labeled("foo(int A)"), SnippetSuffix("(${1:int A})"))));
+  EXPECT_THAT(
+      completions(Context + "int y = fo^(42)", {}, Opts).Completions,
+      UnorderedElementsAre(AllOf(Labeled("foo(int A)"), SnippetSuffix(""))));
+  // FIXME(kirillbobyrev): No snippet should be produced here.
+  EXPECT_THAT(completions(Context + "int y = fo^o(42)", {}, Opts).Completions,
+              UnorderedElementsAre(
+                  AllOf(Labeled("foo(int A)"), SnippetSuffix("(${1:int A})"))));
+  EXPECT_THAT(
+      completions(Context + "int y = ba^", {}, Opts).Completions,
+      UnorderedElementsAre(AllOf(Labeled("bar()"), SnippetSuffix("()"))));
+  EXPECT_THAT(completions(Context + "int y = ba^()", {}, Opts).Completions,
+              UnorderedElementsAre(AllOf(Labeled("bar()"), SnippetSuffix(""))));
+  EXPECT_THAT(
+      completions(Context + "Object o = Obj^", {}, Opts).Completions,
+      Contains(AllOf(Labeled("Object(int B)"), SnippetSuffix("(${1:int B})"),
+                     Kind(CompletionItemKind::Constructor))));
+  EXPECT_THAT(completions(Context + "Object o = Obj^()", {}, Opts).Completions,
+              Contains(AllOf(Labeled("Object(int B)"), SnippetSuffix(""),
+                             Kind(CompletionItemKind::Constructor))));
+  EXPECT_THAT(
+      completions(Context + "Container c = Cont^", {}, Opts).Completions,
+      Contains(AllOf(Labeled("Container<typename T>(int Size)"),
+                     SnippetSuffix("<${1:typename T}>(${2:int Size})"),
+                     Kind(CompletionItemKind::Constructor))));
+  // FIXME(kirillbobyrev): It would be nice to still produce the template
+  // snippet part: in this case it should be "<${1:typename T}>".
+  EXPECT_THAT(
+      completions(Context + "Container c = Cont^()", {}, Opts).Completions,
+      Contains(AllOf(Labeled("Container<typename T>(int Size)"),
+                     SnippetSuffix(""),
+                     Kind(CompletionItemKind::Constructor))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang