[include-cleaner] Filter template instantiations from AST roots.

[hokein]

clang-tools-extra/include-cleaner/lib/Record.cpp

377

as we already do a dyn_cast inside the isImplicitTemplateInstantiation, we can get rid of this NamedDecl cast by changing the isImplicitTemplateInstantiation to accept a Decl*.

387

we can move it to the above anonymous namespace as well (it is fine to have two functions with same name because of C++ overloads). In this case, I'd probably inline it in HandleTopLevelDecl.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

105

maybe DropImplicitTemplateTopDecl?

120

this example is a nice example to show the original bug (where we inserted unexpected additional headers) in include-cleaner.

The scope of this patch is to filter out the implicit template instantiation, a simple testcase like below could work. (the current testcase is fine as well, up to you)

template<typename T>
int func(T) { return 0; } 
auto s = func<int>(1);

123

nit: instead doing a negative verification, I'd check the captured root decl, which is (a MyGetter CXXRecordDecl, and a v var decl), and with a comment saying the implicitly-instantiated method decl is filtered out.

[VitaNuo]

Address review comments.

clang-tools-extra/include-cleaner/lib/Record.cpp

377

Oh right, I didn't notice. Thanks.

387

I am not sure I fully understand this comment, since I cannot both move this method to the anonymous namespace above and inline it at the same time. I have inlined it now, hopefully it looks fine to you.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

105

Not sure, since the rest of tests above only use nouns, e.g., "Macros", "Inclusion", "Namespace", etc. I've called it "ImplicitTemplates" now, hopefully it's fine.

120

You're right, but IMHO I would rather keep the current example. The current example, although simplified, still highly resembles the pattern we've found it real code. The snippet above does not anymore. Since it's a bug fix for a real pattern, I'd rather keep it as is. Let me know if you disagree.

123

Sure, I have no preference.

[hokein]

thanks looks good!

Please remember to add a FIX: <link to github issue> to the commit message.

clang-tools-extra/include-cleaner/lib/Record.cpp

387

the current code looks good. (I meant either, not both, sorry for the confusion)

clang-tools-extra/include-cleaner/lib/Record.cpp

 //===--- Record.cpp - Record compiler events ------------------------------===//
 //
 // 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-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclGroup.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Inclusions/HeaderAnalysis.h"
 
 namespace clang::include_cleaner {
 namespace {

 bool PragmaIncludes::isSelfContained(const FileEntry *FE) const {
   return !NonSelfContainedFiles.contains(FE->getUniqueID());
 }
 
 bool PragmaIncludes::isPrivate(const FileEntry *FE) const {
   return IWYUPublic.find(FE->getUniqueID()) != IWYUPublic.end();
 }
 
+namespace {
+template <typename T>
+bool isImplicitTemplateSpecialization(const NamedDecl *D) {
+  if (const auto *TD = dyn_cast<T>(D))
+    return TD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation;
+  return false;
+}
+} // namespace
+
 std::unique_ptr<ASTConsumer> RecordedAST::record() {
   class Recorder : public ASTConsumer {
     RecordedAST *Out;
 
   public:
     Recorder(RecordedAST *Out) : Out(Out) {}
     void Initialize(ASTContext &Ctx) override { Out->Ctx = &Ctx; }
     bool HandleTopLevelDecl(DeclGroupRef DG) override {
       const auto &SM = Out->Ctx->getSourceManager();
       for (Decl *D : DG) {
         if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation())))
           continue;
-        // FIXME: Filter out certain Obj-C and template-related decls.
+        if (const NamedDecl *ND = dyn_cast<NamedDecl>(D))
+          if (isImplicitTemplateInstantiation(ND))
+            continue;
+        // FIXME: Filter out certain Obj-C as well.
         Out->Roots.push_back(D);
       }
       return ASTConsumer::HandleTopLevelDecl(DG);
     }
+
+  private:
+  bool isImplicitTemplateInstantiation(const NamedDecl *D) {   
+    return isImplicitTemplateSpecialization<FunctionDecl>(D) ||
+         isImplicitTemplateSpecialization<CXXRecordDecl>(D) ||
+         isImplicitTemplateSpecialization<VarDecl>(D);
+  }
   };
 
   return std::make_unique<Recorder>(this);
 }
 
 std::unique_ptr<PPCallbacks> RecordedPP::record(const Preprocessor &PP) {
   return std::make_unique<PPRecorder>(*this, PP);
 }
 
 } // namespace clang::include_cleaner

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp

TEST_F(RecordASTTest, Macros) {
   Inputs.Code = R"cpp(
     #include "header.h"
     X
   )cpp";
   auto AST = build();
   EXPECT_THAT(Recorded.Roots, testing::ElementsAre(named("x")));
 }
 
+// Decl from template instantiation is filtered out from roots.
+TEST_F(RecordASTTest, TemplateInstantiations) {
+  Inputs.ExtraFiles["dispatch.h"] = R"cpp(
+  struct A {
+    static constexpr int value = 1;
+  };
+  template <class Getter>
+  int dispatch() {
+    return Getter::template get<A>();
+  }
+  )cpp";
+  Inputs.Code = R"cpp(
+  #include "dispatch.h"  
+  struct MyGetter {
+    template <class T> static int get() { return T::value; }
+  };
+  int v = dispatch<MyGetter>();
+  )cpp";
+  auto AST = build();
+  EXPECT_THAT(Recorded.Roots, testing::Not(testing::ElementsAre(named("get"))));
+}
+
 class RecordPPTest : public ::testing::Test {
 protected:
   TestInputs Inputs;
   RecordedPP Recorded;
 
   RecordPPTest() {
     struct RecordAction : public PreprocessOnlyAction {
       RecordedPP &Out;