[clang][Index] Fix usage of IndexImplicitInstantiation

[kadircet]

Summary

Indexing context was skipping explicit template instantiations as well.
This patch makes sure it only skips implicit ones.

[ilya-biryukov]

Only have a few NITs, will dig deeper into the change tomorrow.
Added @arphaman too as an owner of the index library. Alex, feel free to reassign if you're the wrong person to take a look at this

unittests/Index/IndexTests.cpp

57

NIT: initialize with 0 to avoid UB.

66

Why do we need to static_cast to int? Can we leave out the cast?

71

NIT: maybe make it a free-standing function, accepting two parameters:

struct Position {
  friend bool operator==(const Pos& L, const Pos& R) {
    // ...
  }
};

Doesn't really matter much here, though, just a general best practice.

98

NIT: initialize with null to make UB less likely

[ilya-biryukov]

unittests/Index/IndexTests.cpp

56

NIT: put all of the decls of a file into an anonymous namespace

[ilya-biryukov]

LGTM

[kadircet]

Address comments

[kadircet]

unittests/Index/IndexTests.cpp

66

it was left over of a copy paste, we don't even need subtraction.

[kadircet]

• Get rid of redundant test

lib/Index/IndexTypeSourceInfo.cpp

for (unsigned i = 0, e = TL.getNumProtocols(); i != e; ++i) {
                                Parent, ParentDC, SymbolRoleSet(), Relations);
     }
     return true;
   }
 
   template<typename TypeLocType>
   bool HandleTemplateSpecializationTypeLoc(TypeLocType TL) {
     if (const auto *T = TL.getTypePtr()) {
-      if (IndexCtx.shouldIndexImplicitInstantiation()) {
-        if (CXXRecordDecl *RD = T->getAsCXXRecordDecl()) {
-          IndexCtx.handleReference(RD, TL.getTemplateNameLoc(),
-                                   Parent, ParentDC, SymbolRoleSet(), Relations);
+        if (!RD->isImplicit() || IndexCtx.shouldIndexImplicitInstantiation()) {
+          IndexCtx.handleReference(RD, TL.getTemplateNameLoc(), Parent,
+                                   ParentDC, SymbolRoleSet(), Relations);
           return true;
         }
       }
       if (const TemplateDecl *D = T->getTemplateName().getAsTemplateDecl())
         IndexCtx.handleReference(D, TL.getTemplateNameLoc(), Parent, ParentDC,
                                  SymbolRoleSet(), Relations);
     }
     return true;

test/Index/Core/index-source.cpp

 // CHECK-NEXT: RelSpecialization | InnerClass | c:@ST>2#T#T@PseudoOverridesInSpecializations@ST>1#T@InnerClass
 };
 
 template<typename U>
 class PseudoOverridesInSpecializations<double, int>::InnerClass {
 };
 // CHECK: [[@LINE-2]]:54 | class(Gen)/C++ | InnerClass | c:@S@PseudoOverridesInSpecializations>#d#I@ST>1#T@InnerClass | <no-cgname> | Def,RelChild | rel: 1
 // CHECK-NEXT: RelChild
-// CHECK: [[@LINE-4]]:7 | class(Gen)/C++ | PseudoOverridesInSpecializations | c:@ST>2#T#T@PseudoOverridesInSpecializations | <no-cgname> | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-4]]:7 | class(Gen,TS)/C++ | PseudoOverridesInSpecializations | c:@S@PseudoOverridesInSpecializations>#d#I | <no-cgname> | Ref,RelCont | rel: 1
 // CHECK-NEXT: RelCont
 
 template<typename S>
 class PseudoOverridesInSpecializations<float, S> {
   typedef float TypealiasOrRecord;
 // CHECK: [[@LINE-1]]:17 | type-alias/C | TypealiasOrRecord | c:index-source.cpp@SP>1#T@PseudoOverridesInSpecializations>#f#t0.0@T@TypealiasOrRecord | <no-cgname> | Def,RelChild,RelSpecialization | rel: 2
 // CHECK-NEXT: RelChild
 // CHECK-NEXT: RelSpecialization | TypealiasOrRecord | c:@ST>2#T#T@PseudoOverridesInSpecializations@TypealiasOrRecord

 template<typename T>
 class SpecializationDecl { };
 // CHECK: [[@LINE-1]]:7 | class(Gen)/C++ | SpecializationDecl | c:@ST>1#T@SpecializationDecl | <no-cgname> | Def | rel: 0
 
 template<>
 class SpecializationDecl<int>;
 // CHECK: [[@LINE-1]]:7 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#I | <no-cgname> | Decl,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | SpecializationDecl | c:@ST>1#T@SpecializationDecl
-// CHECK: [[@LINE-3]]:7 | class(Gen)/C++ | SpecializationDecl | c:@ST>1#T@SpecializationDecl | <no-cgname> | Ref | rel: 0
+// CHECK: [[@LINE-3]]:7 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#I | <no-cgname> | Ref | rel: 0
 
 template<>
 class SpecializationDecl<int> { };
 // CHECK: [[@LINE-1]]:7 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#I | <no-cgname> | Def,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | SpecializationDecl | c:@ST>1#T@SpecializationDecl
-// CHECK-NEXT: [[@LINE-3]]:7 | class(Gen)/C++ | SpecializationDecl | c:@ST>1#T@SpecializationDecl | <no-cgname> | Ref | rel: 0
+// CHECK-NEXT: [[@LINE-3]]:7 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#I | <no-cgname> | Ref | rel: 0
 
 template<typename T>
 class PartialSpecilizationClass<Cls, T>;
 // CHECK: [[@LINE-1]]:7 | class(Gen,TPS)/C++ | PartialSpecilizationClass | c:@SP>1#T@PartialSpecilizationClass>#$@S@Cls#t0.0 | <no-cgname> | Decl,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass
 // CHECK: [[@LINE-3]]:7 | class(Gen)/C++ | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass | <no-cgname> | Ref | rel: 0
 // CHECK-NEXT: [[@LINE-4]]:33 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref | rel: 0
 
 template<>
 class PartialSpecilizationClass<Cls, Cls> : Cls { };
 // CHECK: [[@LINE-1]]:7 | class(Gen,TS)/C++ | PartialSpecilizationClass | c:@S@PartialSpecilizationClass>#$@S@Cls#S0_ | <no-cgname> | Def,RelSpecialization | rel: 1
 // CHECK-NEXT: RelSpecialization | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass
 // CHECK-NEXT: [[@LINE-3]]:45 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref,RelBase,RelCont | rel: 1
 // CHECK-NEXT: RelBase,RelCont | PartialSpecilizationClass | c:@S@PartialSpecilizationClass>#$@S@Cls#S0_
-// CHECK-NEXT: [[@LINE-5]]:7 | class(Gen)/C++ | PartialSpecilizationClass | c:@ST>2#T#T@PartialSpecilizationClass | <no-cgname> | Ref | rel: 0
+// CHECK-NEXT: [[@LINE-5]]:7 | class(Gen,TS)/C++ | PartialSpecilizationClass | c:@S@PartialSpecilizationClass>#$@S@Cls#S0_ | <no-cgname> | Ref | rel: 0
 // CHECK-NEXT: [[@LINE-6]]:33 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref | rel: 0
 // CHECK-NEXT: [[@LINE-7]]:38 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref | rel: 0
 
 template<typename T, int x>
 void functionSp() { }
 
 struct Record {
   constexpr static int C = 2;
 };
 
 template<>
 void functionSp<SpecializationDecl<Cls>, Record::C>() {
 // CHECK: [[@LINE-1]]:6 | function(Gen,TS)/C++ | functionSp | c:@F@functionSp<#$@S@SpecializationDecl>#$@S@Cls#VI2># | __Z10functionSpI18SpecializationDeclI3ClsELi2EEvv | Def,RelSpecialization | rel: 1
 // CHECK:   RelSpecialization | functionSp | c:@FT@>2#T#NIfunctionSp#v#
-// CHECK: [[@LINE-3]]:17 | class(Gen)/C++ | SpecializationDecl | c:@ST>1#T@SpecializationDecl | <no-cgname> | Ref,RelCont | rel: 1
+// CHECK: [[@LINE-3]]:17 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#$@S@Cls | <no-cgname> | Ref,RelCont | rel: 1
 // CHECK: [[@LINE-4]]:36 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref,RelCont | rel: 1
 // CHECK: [[@LINE-5]]:50 | static-property/C++ | C | c:@S@Record@C | __ZN6Record1CE | Ref,RelCont | rel: 1
 // CHECK: [[@LINE-6]]:42 | struct/C++ | Record | c:@S@Record | <no-cgname> | Ref,RelCont | rel: 1
 }
 
 template<typename T, int x>
 class ClassWithCorrectSpecialization { };
 
 template<>
 class ClassWithCorrectSpecialization<SpecializationDecl<Cls>, Record::C> { };
-// CHECK: [[@LINE-1]]:38 | class(Gen)/C++ | SpecializationDecl | c:@ST>1#T@SpecializationDecl | <no-cgname> | Ref | rel: 0
+// CHECK: [[@LINE-1]]:38 | class(Gen,TS)/C++ | SpecializationDecl | c:@S@SpecializationDecl>#$@S@Cls | <no-cgname> | Ref | rel: 0
 // CHECK: [[@LINE-2]]:57 | class/C++ | Cls | c:@S@Cls | <no-cgname> | Ref | rel: 0
 // CHECK: [[@LINE-3]]:71 | static-property/C++ | C | c:@S@Record@C | __ZN6Record1CE | Ref,Read | rel: 0
 // CHECK: [[@LINE-4]]:63 | struct/C++ | Record | c:@S@Record | <no-cgname> | Ref | rel: 0
 
 namespace ns {
 // CHECK: [[@LINE-1]]:11 | namespace/C++ | ns | c:@N@ns | <no-cgname> | Decl | rel: 0
 namespace inner {
 // CHECK: [[@LINE-1]]:11 | namespace/C++ | inner | c:@N@ns@N@inner | <no-cgname> | Decl,RelChild | rel: 1

 void localStructuredBindingAndRef() {
   int ref = structuredBinding1;
 // CHECK: [[@LINE-1]]:13 | variable/C++ | structuredBinding1 | c:@N@cpp17structuredBinding@structuredBinding1 | <no-cgname> | Ref,Read,RelCont | rel: 1
 // CHECK-NEXT: RelCont | localStructuredBindingAndRef | c:@N@cpp17structuredBinding@F@localStructuredBindingAndRef#
   auto [localBinding1, localBinding2] = Cpp17StructuredBinding(ref, structuredBinding2);
 // CHECK: [[@LINE-1]]:69 | variable/C++ | structuredBinding2 | c:@N@cpp17structuredBinding@structuredBinding2 | <no-cgname> | Ref,Read,RelCont | rel: 1
 // CHECK-NEXT: RelCont | localStructuredBindingAndRef | c:@N@cpp17structuredBinding@F@localStructuredBindingAndRef#
 // CHECK-NOT: localBinding
-// LOCAL: [[@LINE-4]]:9 | variable(local)/C++ | localBinding1 | c:index-source.cpp@25382@N@cpp17structuredBinding@F@localStructuredBindingAndRef#@localBinding1
+// LOCAL: [[@LINE-4]]:9 | variable(local)/C++ | localBinding1 | c:index-source.cpp@25408@N@cpp17structuredBinding@F@localStructuredBindingAndRef#@localBinding1
 }
 
 }
 
 template<typename T>
 struct Guided { T t; };
 // CHECK: [[@LINE-1]]:8 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | <no-cgname> | Def | rel: 0
 // CHECK-NEXT: [[@LINE-2]]:19 | field/C++ | t | c:@ST>1#T@Guided@FI@t | <no-cgname> | Def,RelChild | rel: 1
 // CHECK-NEXT: RelChild | Guided | c:@ST>1#T@Guided
 Guided(double) -> Guided<float>;
-// CHECK: [[@LINE-1]]:19 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | <no-cgname> | Ref | rel: 0
+// CHECK: [[@LINE-1]]:19 | struct(Gen,TS)/C++ | Guided | c:@S@Guided>#f | <no-cgname> | Ref | rel: 0
 // CHECK-NEXT: [[@LINE-2]]:1 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | <no-cgname> | Ref | rel: 0
 auto guided = Guided{1.0};
 // CHECK: [[@LINE-1]]:6 | variable/C | guided | c:@guided | _guided | Def | rel: 0
 // CHECK-NEXT: [[@LINE-2]]:15 | struct(Gen)/C++ | Guided | c:@ST>1#T@Guided | <no-cgname> | Ref,RelCont | rel: 1
 // CHECK-NEXT: RelCont | guided | c:@guided
 
 namespace rd33122110 {
 

test/Index/index-refs.cpp

 /* when indexing implicit instantiations
   [indexDeclaration]: kind: struct-template-spec | name: TS | USR: c:@S@TS>#I | {{.*}} | loc: 50:8
   [indexDeclaration]: kind: typedef | name: MyInt | USR: c:index-refs.cpp@593@S@TS>#I@T@MyInt | {{.*}} | loc: 51:15 | semantic-container: [TS:50:8] | lexical-container: [TS:50:8]
  */
 // CHECK-NEXT: [indexDeclaration]: kind: function | name: foo3
 /* when indexing implicit instantiations
   [indexEntityReference]: kind: struct-template-spec | name: TS | USR: c:@S@TS>#I | {{.*}} | loc: 55:3
 */
-// CHECK-NEXT: [indexEntityReference]: kind: c++-class-template | name: TS | USR: c:@ST>2#T#T@TS | {{.*}} | loc: 55:3
+// CHECK-NEXT: [indexEntityReference]: kind: struct-template-partial-spec | name: TS | USR: c:@SP>1#T@TS>#t0.0#I | {{.*}} | loc: 55:3
 
 // CHECK:      [indexEntityReference]: kind: variable | name: array_size | {{.*}} | loc: 59:22
 // CHECK:      [indexEntityReference]: kind: variable | name: default_param | {{.*}} | loc: 62:19 | {{.*}} | role: ref read
 // CHECK-NOT:  [indexEntityReference]: kind: variable | name: default_param | {{.*}} | loc: 62:19
 
 // CHECK:      [indexEntityReference]: kind: field | name: y | {{.*}} | loc: 69:20
 // CHECK-NEXT: [indexEntityReference]: kind: field | name: x | {{.*}} | loc: 69:28
 // CHECK-NOT:  [indexEntityReference]: kind: field | name: y | {{.*}} | loc: 69:20

unittests/Index/IndexTests.cpp

 //===--- IndexTests.cpp - Test indexing actions -----------------*- 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 "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <memory>
 
 namespace clang {
 namespace index {
+namespace {
+struct Position {
+  size_t Line = 0;
+  size_t Column = 0;
+
+  Position(size_t Line = 0, size_t Column = 0) : Line(Line), Column(Column) {}
+
+  static Position fromSourceLocation(SourceLocation Loc,
+                                     const SourceManager &SM) {
+    FileID FID;
+    unsigned Offset;
+    std::tie(FID, Offset) = SM.getDecomposedSpellingLoc(Loc);
+    Position P;
+    P.Line = SM.getLineNumber(FID, Offset);
+    P.Column = SM.getColumnNumber(FID, Offset);
+    return P;
+  }
+};
+
+bool operator==(const Position &LHS, const Position &RHS) {
+  return std::tie(LHS.Line, LHS.Column) == std::tie(RHS.Line, RHS.Column);
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Position &Pos) {
+  return OS << Pos.Line << ':' << Pos.Column;
+}
 
 struct TestSymbol {
   std::string QName;
+  Position WrittenPos;
+  Position DeclPos;
   // FIXME: add more information.
 };
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const TestSymbol &S) {
-  return OS << S.QName;
+  return OS << S.QName << '[' << S.WrittenPos << ']' << '@' << S.DeclPos;
 }
 
-namespace {
 class Indexer : public IndexDataConsumer {
 public:
+  void initialize(ASTContext &Ctx) override {
+    AST = &Ctx;
+    IndexDataConsumer::initialize(Ctx);
+  }
+
   bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
-                           ArrayRef<SymbolRelation>, SourceLocation,
+                           ArrayRef<SymbolRelation>, SourceLocation Loc,
                            ASTNodeInfo) override {
     const auto *ND = llvm::dyn_cast<NamedDecl>(D);
     if (!ND)
       return true;
     TestSymbol S;
     S.QName = ND->getQualifiedNameAsString();
+    S.WrittenPos = Position::fromSourceLocation(Loc, AST->getSourceManager());
+    S.DeclPos =
+        Position::fromSourceLocation(D->getLocation(), AST->getSourceManager());
     Symbols.push_back(std::move(S));
     return true;
   }
 
   bool handleMacroOccurence(const IdentifierInfo *Name, const MacroInfo *,
                             SymbolRoleSet, SourceLocation) override {
     TestSymbol S;
     S.QName = Name->getName();
     Symbols.push_back(std::move(S));
     return true;
   }
 
   std::vector<TestSymbol> Symbols;
+  const ASTContext *AST = nullptr;
 };
 
 class IndexAction : public ASTFrontendAction {
 public:
   IndexAction(std::shared_ptr<Indexer> Index,
               IndexingOptions Opts = IndexingOptions())
       : Index(std::move(Index)), Opts(Opts) {}
 
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
     return llvm::make_unique<Consumer>(Index, CI.getPreprocessorPtr(), Opts);
   }
 
 private:
   std::shared_ptr<Indexer> Index;
   IndexingOptions Opts;
 };
 
+using testing::AllOf;
 using testing::Contains;
 using testing::Not;
 using testing::UnorderedElementsAre;
 
 MATCHER_P(QName, Name, "") { return arg.QName == Name; }
+MATCHER_P(WrittenAt, Pos, "") { return arg.WrittenPos == Pos; }
+MATCHER_P(DeclAt, Pos, "") { return arg.DeclPos == Pos; }
 
 TEST(IndexTest, Simple) {
   auto Index = std::make_shared<Indexer>();
   tooling::runToolOnCode(new IndexAction(Index), "class X {}; void f() {}");
   EXPECT_THAT(Index->Symbols, UnorderedElementsAre(QName("X"), QName("f")));
 }
 
 TEST(IndexTest, IndexPreprocessorMacros) {
TEST(IndexTest, IndexParametersInDecls) {
   EXPECT_THAT(Index->Symbols, Contains(QName("bar")));
 
   Opts.IndexParametersInDeclarations = false;
   Index->Symbols.clear();
   tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar"))));
 }
 
+TEST(IndexTest, IndexExplicitTemplateInstantiation) {
+  std::string Code = R"cpp(
+    template <typename T>
+    struct Foo { void bar() {} };
+    template <>
+    struct Foo<int> { void bar() {} };
+    void foo() {
+      Foo<char> abc;
+      Foo<int> b;
+    }
+  )cpp";
+  auto Index = std::make_shared<Indexer>();
+  IndexingOptions Opts;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+              AllOf(Contains(AllOf(QName("Foo"), WrittenAt(Position(8, 7)),
+                                   DeclAt(Position(5, 12)))),
+                    Contains(AllOf(QName("Foo"), WrittenAt(Position(7, 7)),
+                                   DeclAt(Position(3, 12))))));
+}
+
+TEST(IndexTest, IndexTemplateInstantiationPartial) {
+  std::string Code = R"cpp(
+    template <typename T1, typename T2>
+    struct Foo { void bar() {} };
+    template <typename T>
+    struct Foo<T, int> { void bar() {} };
+    void foo() {
+      Foo<char, char> abc;
+      Foo<int, int> b;
+    }
+  )cpp";
+  auto Index = std::make_shared<Indexer>();
+  IndexingOptions Opts;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+              Contains(AllOf(QName("Foo"), WrittenAt(Position(8, 7)),
+                             DeclAt(Position(5, 12)))));
+}
+
 } // namespace
 } // namespace index
 } // namespace clang