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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1283
+          Recorder->CCSema->getSourceManager(), Recorder->CCSema->LangOpts);
+      HasParenthesisAfter = NextToken->getKind() == tok::l_paren;
       auto Style = getFormatStyleForFile(SemaCCInput.FileName,
----------------
kbobyrev wrote:
> sammccall wrote:
> > need to decide what to do when it returns none, or add an explicit assert with a message ("code completing in macro?")
> Yeah, forgot about that -_- I think just doing nothing is OK here?
Yup EOF LGTM.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1712
+            SemaCCS, QueryScopes, *Inserter, FileName, CCContextKind, Opts,
+            /*GenerateSnippets=*/!IsUsingDeclaration && !HasParenthesisAfter);
       else
----------------
kbobyrev wrote:
> sammccall wrote:
> > 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.
> 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?
Yes, adding a test showing we don't handle this edge case is fine.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1715
+                        CCContextKind, Opts, !IsUsingDeclaration,
+                        NextTokenKind);
       else
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2921
+      Contains(AllOf(Labeled("Container<typename T>(int Size)"),
+                     SnippetSuffix(""),
+                     Kind(CompletionItemKind::Constructor))));
----------------
Add a fixme: should be "<${1:typename T}>"


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2893
+  EXPECT_THAT(
+      completions(Context + "int y = fo^(42)", {}, Opts).Completions,
+      UnorderedElementsAre(AllOf(Labeled("foo(int A)"), SnippetSuffix(""))));
----------------
kbobyrev wrote:
> sammccall wrote:
> > can we have fo^o(42) as a case as well?
> 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?
Yes, no need to handle it, but let's add a test and mark it as bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81380