[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

68 ↗

(On Diff #212569)

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

68 ↗

(On Diff #212569)

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

68 ↗

(On Diff #212569)

Yes, there's a typo thanks!

[jvikstrom]

Moved test to ClangdUnitTests.

[jvikstrom]

clang-tools-extra/clangd/ClangdUnit.cpp

68 ↗

(On Diff #212569)

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 ↗

(On Diff #213041)

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

[jvikstrom]

Formatted.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

82 ↗

(On Diff #213331)

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)

68 ↗

(On Diff #212569)

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

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

68 ↗

(On Diff #212569)

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 ↗

(On Diff #213388)

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 ↗

(On Diff #213388)

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 ↗

(On Diff #213388)

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

134 ↗

(On Diff #213608)

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

template <>
struct V<bool> {};

137 ↗

(On Diff #213608)

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 ↗

(On Diff #213608)

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.

110 ↗

(On Diff #213388)

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

[jvikstrom]

Added more ways to specialize/instantiate templates to test.

[jvikstrom]

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

144 ↗

(On Diff #213608)

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 ↗

(On Diff #213608)

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

85 ↗

(On Diff #213878)

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

91 ↗

(On Diff #213878)

NIT: remove braces

97 ↗

(On Diff #213878)

NIT: remove braces

186 ↗

(On Diff #213878)

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

188 ↗

(On Diff #213878)

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

[jvikstrom]

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

144 ↗

(On Diff #213608)

Will fix the partial specialization in a separate CL

[jvikstrom]

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

188 ↗

(On Diff #213878)

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/trunk/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,
 
+/// Indicates if \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 if \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);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_

clang-tools-extra/trunk/clangd/AST.cpp

 
 #include "SourceCode.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);
+}
+
 bool isImplementationDetail(const Decl *D) {
   return !isSpelledInSource(D->getLocation(),
                             D->getASTContext().getSourceManager());
 }
 
+// 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();
+  auto Loc = D->getLocation();
+  // FIXME: Revisit the strategy, the heuristic is limitted when handling
+  // macros, we should use the location where the whole definition occurs.
+  if (Loc.isMacroID()) {
+    std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
+    if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
+        llvm::StringRef(PrintLoc).startswith("<command line>"))
+      return false;
+  }
+  return true;
+}
+
 SourceLocation findName(const clang::Decl *D) {
   return D->getLocation();
 }
 
 std::string printQualifiedName(const NamedDecl &ND) {
   std::string QName;
   llvm::raw_string_ostream OS(QName);
   PrintingPolicy Policy(ND.getASTContext().getLangOpts());

clang-tools-extra/trunk/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/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.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"
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/trunk/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/trunk/clangd/unittests/ClangdUnitTests.cpp

 //===-- ClangdUnitTests.cpp - ClangdUnit tests ------------------*- 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 "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
 #include "TestTU.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::ElementsAre;
+using ::testing::ElementsAreArray;
+using ::testing::AllOf;
 
 TEST(ClangdUnitTest, GetBeginningOfIdentifier) {
   std::string Preamble = R"cpp(
 struct Bar { int func(); };
 #define MACRO(X) void f() { X; }
 Bar* bar;
   )cpp";
   // First ^ is the expected beginning, last is the search position.
if (ND->getName() == Name)
       return true;
   if (auto *Stream = result_listener->stream()) {
     llvm::raw_os_ostream OS(*Stream);
     arg->dump(OS);
   }
   return false;
 }
 
+// Matches if the Decl has template args equal to ArgName. If the decl is a
+// NamedDecl and ArgName is an empty string it also matches.
+MATCHER_P(WithTemplateArgs, ArgName, "") {
+  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(arg)) {
+    if (const auto *Args = FD->getTemplateSpecializationArgs()) {
+      std::string SpecializationArgs;
+      // Without the PrintingPolicy "bool" will be printed as "_Bool".
+      LangOptions LO;
+      PrintingPolicy Policy(LO);
+      Policy.adjustForCPlusPlus();
+      for (const auto Arg : Args->asArray()) {
+        if (SpecializationArgs.size() > 0)
+          SpecializationArgs += ",";
+        SpecializationArgs += Arg.getAsType().getAsString(Policy);
+      }
+      if (Args->size() == 0)
+        return ArgName == SpecializationArgs;
+      return ArgName == "<" + SpecializationArgs + ">";
+    }
+  }
+  if (const NamedDecl *ND = dyn_cast<NamedDecl>(arg))
+    return printTemplateSpecializationArgs(*ND) == ArgName;
+  return false;
+}
+
 TEST(ClangdUnitTest, TopLevelDecls) {
   TestTU TU;
   TU.HeaderCode = R"(
     int header1();
     int header2;
   )";
   TU.Code = "int main();";
   auto AST = TU.build();
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,
+     GetsExplicitInstantiationAndSpecializationTemplateTopDecls) {
+  TestTU TU;
+  TU.Code = R"cpp(
+    template <typename T>
+    void f(T) {}
+    template<>
+    void f(bool);
+    template void f(double);
+
+    template <class T>
+    struct V {};
+    template<class T>
+    struct V<T*> {};
+    template <>
+    struct V<bool> {};
+
+    template<class T>
+    T foo = T(10);
+    int i = foo<int>;
+    double d = foo<double>;
+
+    template <class T>
+    int foo<T*> = 0;
+    template <>
+    int foo<bool> = 0;
+  )cpp";
+
+  auto AST = TU.build();
+  EXPECT_THAT(
+      AST.getLocalTopLevelDecls(),
+      ElementsAreArray({AllOf(DeclNamed("f"), WithTemplateArgs("")),
+                        AllOf(DeclNamed("f"), WithTemplateArgs("<bool>")),
+                        AllOf(DeclNamed("f"), WithTemplateArgs("<double>")),
+                        AllOf(DeclNamed("V"), WithTemplateArgs("")),
+                        AllOf(DeclNamed("V"), WithTemplateArgs("<T *>")),
+                        AllOf(DeclNamed("V"), WithTemplateArgs("<bool>")),
+                        AllOf(DeclNamed("foo"), WithTemplateArgs("")),
+                        AllOf(DeclNamed("i"), WithTemplateArgs("")),
+                        AllOf(DeclNamed("d"), WithTemplateArgs("")),
+                        AllOf(DeclNamed("foo"), WithTemplateArgs("<>")),
+                        AllOf(DeclNamed("foo"), WithTemplateArgs("<bool>"))}));
+}
+
 TEST(ClangdUnitTest, TokensAfterPreamble) {
   TestTU TU;
   TU.AdditionalFiles["foo.h"] = R"(
     int foo();
   )";
   TU.Code = R"cpp(
       #include "foo.h"
       first_token;

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

R"cpp(
       template<template<class> class $TemplateParameter[[T]], class $TemplateParameter[[C]]>
       $TemplateParameter[[T]]<$TemplateParameter[[C]]> $Function[[f]]();
 
       template<typename>
       class $Class[[Foo]] {};
 
       template<typename $TemplateParameter[[T]]>
       void $Function[[foo]]($TemplateParameter[[T]] ...);
+    )cpp",
+    R"cpp(
+      template <class $TemplateParameter[[T]]>
+      struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
+      extern template struct $Class[[Tmpl]]<float>;
+      template struct $Class[[Tmpl]]<double>;
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
 }
 
 TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
   class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {