[clangd] Fix implicit template instatiations appearing as topLevelDecls.

[jvikstrom]

Summary

The parser gives implicit template instantiations to the action's HandleTopLevelDecls callback. This makes semantic highlighting highlight these templated functions multiple times. Fixed by filtering on if a Decl is an implicit function/variable/class instantiation. Also added a testcase to semantic highlighting on this.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

69

We also want to skip TSK_ExplicitInstantiationDeclaration and TSK_ExplicitInstantiationDefinition here.
This covers cases like (not sure which one of the two enum values we get, though):

template <class T>
int foo(T) { ... }

template int foo(int); // we'd rather not traverse these, highlightings will run into the same problems.

Semantics I'm describing are roughly similar to isImplicitInstatiation(D) == !isExplicitInstantion(D), where isExplicitInstantiation is taken from CodeComplete.cpp. (If we ignore TSK_Undeclared, which, I believe, should never be encountered in decls passed to HandleTopLevelDecl).

Please extract the helper from code complete and this one into a separate file (e.g. clangd/AST.h) and possibly implement one through the other

[hokein]

clang-tools-extra/clangd/ClangdUnit.cpp

69

Semantics I'm describing are roughly similar to isImplicitInstatiation(D) == !isExplicitInstantion(D),

I think there is a typo here, I believe you mean isImplicitInstantiation(D) == !isExplicitSpecialization(D) (in CodeComplete.cpp, it checks whether a Decl is an explicit specialization).

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

219 ↗

(On Diff #212569)

instead adding a highlighting test, could we add this testcase to ClangdUnitTests.cpp instead?

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

69

Yes, there's a typo thanks!

[jvikstrom]

Moved test to ClangdUnitTests.

[jvikstrom]

clang-tools-extra/clangd/ClangdUnit.cpp

69

Still want me to move this and the helper from CodeComplete to AST.h as it isn't used anywhere else? (esp. when we can't implement them through of each other)

[hokein]

the change looks good to me, leave the final approval to Ilya as he knows more context.

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

116

nit: clang-format, the length seems > 80 column.

[jvikstrom]

Formatted.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

69

Yes, it's better to share this code between codeComplete and ClangdUnit. It might pop up in more places and it's not trivial.

77

Could we expose the following non-template function instead?

bool isImplicitTemplateInstantiation(NamedDecl *);

So that the users don't need to specify which Decl they are interested in (I believe there's no use for it anyway)

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

69

Sorry, I mean share functions from the same place between code complete and clangd unit.
They might not be implementable through each other, but they are very similar and having them in the same place means that we will have easier time keeping them both correct in case we need to change any of them.

[jvikstrom]

Move isImplicitTemplateInstantiation and isExplicitTemplateSpecialization, also share implementation.

[ilya-biryukov]

clang-tools-extra/clangd/AST.h

86

Could you add a small comment with an example?

/// Indicates \p D is a template instantiation implicitly generated by the compiler, e.g.
///     template <class T> struct vector {};
///     vector<int> v; // 'vector<int>' is an implicit instantiation
bool isImplicitTemplateInstantiation(const NamedDecl *D);

/// Indicates \p D is an explicit template specialization, e.g.
///   template <class T> struct vector {};
///   template <> struct vector<bool> {}; // <-- explicit specialization
///
/// Note that explicit instantiations are NOT explicit specializations, albeit they look similar.
///   template struct vector<bool>; // <-- explicit instantiation, NOT an explicit specialization.
bool isExplicitTemplateSpecialization(const NamedDecl *D);

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

110

Could you also check that:

1. explicit specializations are present

template <>
void f(bool) {}

2. explicit instantiations are absent

template void f(bool);

3. partial specializations are present (they should not be affected by this change, but testing those here seems appropriate)

template <class T>
struct vector {};

template <class T>
struct vector<T*> {}; // partial specialization, should be present

4. template variables and classes are also handled:

template <class T>
T foo = 10; // (!) requires C++17

[jvikstrom]

Added tests for making sure explicit specializations, explicit instantiations, partial instantiations, explicit declarations and template variables are in topLevelDecls. Also added a test to SemanticHighlightingTests to make sure that explicit instantiations are being traversed (so they aren't accidentaly removed from topLevelDecls).

Also added comment for the two added functions in AST.h.

[jvikstrom]

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

110

Explicit instantiations are present in topLevelDecls though, otherwise RecursiveASTVisitor would not traverse them (so adding a test to make sure explicit instantiations are included in toplevel).

Also adding a test to SemanticHighlighting to make sure that explicit instantiations are visited in that (is some other RecursiveASTVisitor usage I should add this to instead?)

[ilya-biryukov]

One important comment about somehow distinguishing multiple decls with the same name.

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

110

Yes, sorry, I forgot about the results of your investigation yesterday.
Having them in top-level decls seems fine, just wanted to make sure we actually test it.
LG

134

Also add a full specialization (IIRC, they are represented completely differently in the AST):

template <>
struct V<bool> {};

137

Also add a partial and a full template specializations for the variable declaration:

template <class T>
int foo<T*> = 0;

template <>
int foo<bool> = 0;

144

Is there a way to check we are seeing the explicit instantiations? (e.g. by looking at template arguments?)

It's not clear whether multiple DeclNamed("foo") refer to the decls we expect.

[jvikstrom]

Added more ways to specialize/instantiate templates to test.

[jvikstrom]

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

144

Well in this case all the expressions in the top level should be in topLevelDecls so unless say void f(T) {} somehow starts duplicating while void f(bool) disappears from topLevelDecls they should all be visible.
I could add a gtest matcher that also checks the types but I think that would become a pretty large matcher (would have to handle FunctionDecls with template arguments, VarDecls, TemplateClassSpecializationDecls etc.) and I'm not sure it would really add anything to the tests.

A thing we we could do is shuffle the order of the decls in the test so we never have two decls of the same name after each other (because the matcher cares about the order of the elements). Which should give us pretty high confidence we are getting the correct decls... Or maybe I'm misunderstanding you?

[ilya-biryukov]

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

144

The tests should be easy to read and understand, we should definitely find a way to distinguish decls with different arguments.
Adding a matcher for template args should be easy enough:

AllOf(DeclNamed("f"), WithTemplateArgs("<bool>"))

Implementing the matcher is easy with printTemplateSpecializationArgs from AST.h

[jvikstrom]

Changed tests to also look at template parameters.

[ilya-biryukov]

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

80

Could you wrap the result with < and >, add , as a separator?
To make sure the output format for functions is not different for other decls

86

NIT: remove braces

92

NIT: remove braces

159

NIT: do not match WithTemplateArgs for non-template names (like i and d here)?

161

NIT: add // FIXME: this should be '<T*>', not '<>'

[jvikstrom]

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

144

Will fix the partial specialization in a separate CL

[jvikstrom]

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

161

Already have a fix for it (it's a 2 line fix without the tests, should I just add do it in a separate CL or add it to this?)

[ilya-biryukov]

LGTM, but please print the function template arguments uniformly before landing this (to avoid different forms of outputs inside the same test).

clang-tools-extra/clangd/AST.h

 /// the string that are redundant in the CurrentNamespace. This way the type
 /// idenfier become shorter and easier to read.
 /// Limitation: It only handles the qualifier of the type itself, not that of
 /// templates.
 /// FIXME: change type of parameter CurrentNamespace to DeclContext ,
 /// take in to account using directives etc
 /// Example: shortenNamespace("ns1::MyClass<ns1::OtherClass>", "ns1")
 ///    --> "MyClass<ns1::OtherClass>"
-std::string  shortenNamespace(const llvm::StringRef OriginalName,
-                              const llvm::StringRef CurrentNamespace);
+std::string shortenNamespace(const llvm::StringRef OriginalName,
 
+bool isImplicitTemplateInstantiation(const NamedDecl *D);
+bool isExplicitTemplateSpecialization(const NamedDecl *D);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_

clang-tools-extra/clangd/AST.cpp

 //===--- AST.cpp - Utility AST functions  -----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
if (auto *Func = llvm::dyn_cast<FunctionDecl>(&ND)) {
     if (auto *Args = Cls->getTemplateArgsAsWritten())
       return Args->arguments();
   } else if (auto *Var = llvm::dyn_cast<VarTemplateSpecializationDecl>(&ND))
     return Var->getTemplateArgsInfo().arguments();
   // We return None for ClassTemplateSpecializationDecls because it does not
   // contain TemplateArgumentLoc information.
   return llvm::None;
 }
+
+template <class T>
+bool isTemplateSpecializationKind(const NamedDecl *D,
+                                  TemplateSpecializationKind Kind) {
+  if (const auto *TD = dyn_cast<T>(D))
+    return TD->getTemplateSpecializationKind() == Kind;
+  return false;
+}
+
+bool isTemplateSpecializationKind(const NamedDecl *D,
+                                  TemplateSpecializationKind Kind) {
+  return isTemplateSpecializationKind<FunctionDecl>(D, Kind) ||
+         isTemplateSpecializationKind<CXXRecordDecl>(D, Kind) ||
+         isTemplateSpecializationKind<VarDecl>(D, Kind);
+}
+
 } // namespace
 
+bool isImplicitTemplateInstantiation(const NamedDecl *D) {
+  return isTemplateSpecializationKind(D, TSK_ImplicitInstantiation);
+}
+
+bool isExplicitTemplateSpecialization(const NamedDecl *D) {
+  return isTemplateSpecializationKind(D, TSK_ExplicitSpecialization);
+}
+
 // Returns true if the complete name of decl \p D is spelled in the source code.
 // This is not the case for:
 //   * symbols formed via macro concatenation, the spelling location will
 //     be "<scratch space>"
 //   * symbols controlled and defined by a compile command-line option
 //     `-DName=foo`, the spelling location will be "<command line>".
 bool isSpelledInSourceCode(const Decl *D) {
   const auto &SM = D->getASTContext().getSourceManager();

clang-tools-extra/clangd/ClangdUnit.cpp

 //===--- ClangdUnit.cpp ------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdUnit.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "AST.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "IncludeFixer.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"

 }
 
 template <class T> std::size_t getUsedBytes(const std::vector<T> &Vec) {
   return Vec.capacity() * sizeof(T);
 }
 
 class DeclTrackingASTConsumer : public ASTConsumer {
 public:
   DeclTrackingASTConsumer(std::vector<Decl *> &TopLevelDecls)
       : TopLevelDecls(TopLevelDecls) {}
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
     for (Decl *D : DG) {
       auto &SM = D->getASTContext().getSourceManager();
       if (!isInsideMainFile(D->getLocation(), SM))
         continue;
+      if (const NamedDecl *ND = dyn_cast<NamedDecl>(D))
+        if (isImplicitTemplateInstantiation(ND))
+          continue;
 
       // ObjCMethodDecl are not actually top-level decls.
       if (isa<ObjCMethodDecl>(D))
         continue;
 
       TopLevelDecls.push_back(D);
     }
     return true;

clang-tools-extra/clangd/CodeComplete.cpp

for (const auto &Item : Bundle) {
                         CCContextKind, Opts);
       else
         Builder->add(Item, SemaCCS);
     }
     return Builder->build();
   }
 };
 
-template <class T> bool isExplicitTemplateSpecialization(const NamedDecl &ND) {
-  if (const auto *TD = dyn_cast<T>(&ND))
-    if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
-      return true;
-  return false;
-}
-
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
   clang::CodeCompleteOptions Result;
   Result.IncludeCodePatterns = EnableSnippets && IncludeCodePatterns;
   Result.IncludeMacros = IncludeMacros;
   Result.IncludeGlobals = true;
   // We choose to include full comments and not do doxygen parsing in
case Decl::LinkageSpec:
       return true;
     default:
       break;
     };
     return false;
   };
   // We only complete symbol's name, which is the same as the name of the
   // *primary* template in case of template specializations.
-  if (isExplicitTemplateSpecialization<FunctionDecl>(ND) ||
-      isExplicitTemplateSpecialization<CXXRecordDecl>(ND) ||
-      isExplicitTemplateSpecialization<VarDecl>(ND))
+  if (isExplicitTemplateSpecialization(&ND))
     return false;
 
   if (InTopLevelScope(ND))
     return true;
 
   if (const auto *EnumDecl = dyn_cast<clang::EnumDecl>(ND.getDeclContext()))
     return InTopLevelScope(*EnumDecl) && !EnumDecl->isScoped();
 

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

MATCHER_P(DeclNamed, Name, "") {
   return false;
 }
 
 TEST(ClangdUnitTest, TopLevelDecls) {
   TestTU TU;
   TU.HeaderCode = R"(
     int header1();
     int header2;
   )";
   TU.Code = "int main();";
   auto AST = TU.build();
   EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
 }
 
 TEST(ClangdUnitTest, DoesNotGetIncludedTopDecls) {
   TestTU TU;
   TU.HeaderCode = R"cpp(
     #define LL void foo(){}
     template<class T>
     struct H {
       H() {}
       LL
     };
   )cpp";
   TU.Code = R"cpp(
     int main() {
       H<int> h;
       h.foo();
     }
   )cpp";
   auto AST = TU.build();
   EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
 }
 
+TEST(ClangdUnitTest, DoesNotGetImplicitTemplateTopDecls) {
+  TestTU TU;
+  TU.Code = R"cpp(
+    template<typename T>
+    void f(T) {}
+    void s() {
+      f(10UL);
+    }
+  )cpp";
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+              ElementsAre(DeclNamed("f"), DeclNamed("s")));
+}
+
 TEST(ClangdUnitTest, TokensAfterPreamble) {
   TestTU TU;
   TU.AdditionalFiles["foo.h"] = R"(
     int foo();
   )";
   TU.Code = R"cpp(
       #include "foo.h"
       first_token;
       void test() {
       }
       last_token
 )cpp";
   auto AST = TU.build();
   const syntax::TokenBuffer &T = AST.getTokens();
   const auto &SM = AST.getSourceManager();
 
   ASSERT_GT(T.expandedTokens().size(), 2u);
   // Check first token after the preamble.
   EXPECT_EQ(T.expandedTokens().front().text(SM), "first_token");
   // Last token is always 'eof'.
   EXPECT_EQ(T.expandedTokens().back().kind(), tok::eof);
   // Check the token before 'eof'.
   EXPECT_EQ(T.expandedTokens().drop_back().back().text(SM), "last_token");
 
   // The spelled tokens for the main file should have everything.
   auto Spelled = T.spelledTokens(SM.getMainFileID());
   ASSERT_FALSE(Spelled.empty());
   EXPECT_EQ(Spelled.front().kind(), tok::hash);
   EXPECT_EQ(Spelled.back().text(SM), "last_token");
 }
 
 
 TEST(ClangdUnitTest, NoCrashOnTokensWithTidyCheck) {
   TestTU TU;
   // this check runs the preprocessor, we need to make sure it does not break
   // our recording logic.
   TU.ClangTidyChecks = "modernize-use-trailing-return-type";
   TU.Code = "inline int foo() {}";
 
   auto AST = TU.build();
   const syntax::TokenBuffer &T = AST.getTokens();
   const auto &SM = AST.getSourceManager();
 
   ASSERT_GT(T.expandedTokens().size(), 7u);
   // Check first token after the preamble.
   EXPECT_EQ(T.expandedTokens().front().text(SM), "inline");
   // Last token is always 'eof'.
   EXPECT_EQ(T.expandedTokens().back().kind(), tok::eof);
   // Check the token before 'eof'.
   EXPECT_EQ(T.expandedTokens().drop_back().back().text(SM), "}");
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang