[AST] RAV doesn't traverse explicitly instantiated function bodies by default

[sammccall]

Summary

This is consistent with the idea of shouldVisitTemplateInstantiations():
when false, we should traverse the code the user wrote but not the code
clang synthesized. Therefore we should be able to traverse the
signature of the function without traversing its body.
This is also consistent with how class and variable templates behave.

Note that currently RAV does not traverse *any* part of the these when
shouldVisitTemplateInstantiations() is false (which is another bug).
So currently this change is only observable when the user passes the
instantiated FunctionDecl to TraverseDecl themselves.

This is what the caller in CodeGenPGO does, and it wants to traverse the
instantiated body. I believe not traversing instantiations in general in
this context is an oversight.

[sammccall]

@davidxl does the one-line CodeGenPGO change look reasonable? If you're not the person to ask, could you recommend someone? Thanks!

(Applying the patch without the CodeGenPGO change causes Profile/cxx-templates.cpp and CoverageMapping/branch-templates.cpp to fail as when mapRegionCounters() gets an instantiated template function it skips traversing the body)

[hokein]

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp

168

What does the Head mean?

209

There are many declaration in the testcode, hard to figure out which one we are chosen, can you add a comment about which declaration is chosen as the traversal root? I think it is function decl instantiated by the template int foo<4>();?

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

2107

moving this part of code to if (VisitBody) seems like a different fix (and I think it is reasonable, from the grammar, the ctor-initializer is part of the function-body), can we separate it and add a test?

[sammccall]

clang/include/clang/AST/RecursiveASTVisitor.h

2107

I can do it as a separate patch before if you like, but it's NFC.

Behavior only differs if there is an init list and VisitBody is false.
However !VisitBody requires one of:

• this is a declaration only => no init list
• this is a defaulted function => no init list

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp

168

Within the function

int foo<4>() { return 0; }.

I've called int foo<4>() the "head" and {return 0;} the "body".
The distinction is that the head was written, and the body was instantiated.

are there better terms? (I'd probably like this "head" better than "declaration part" so maybe we should just define these terms in the shouldVisitTemplateInstantiations() comment)

[davidxl]

clang/lib/CodeGen/CodeGenPGO.cpp

157

This one line change looks ok to me.

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

 
 TEST(TypeHints, Smoke) {
   assertTypeHints(R"cpp(
     auto $waldo[[waldo]] = 42;
   )cpp",
                   ExpectedHint{": int", "waldo"});
 }
 
+TEST(TypeHints, FunctionTemplateExplicitInstantiation) {
+  // We used to erroneously emit bad type hints in the template for every
+  // explicit specialization: https://github.com/clangd/clangd/issues/1034
+  assertTypeHints(R"cpp(
+    template <class T> void foo() {
+      auto x = T{};
+    }
+    template void foo<char>();
+    template void foo<float>();
+  )cpp");
+}
+
 TEST(TypeHints, Decorations) {
   assertTypeHints(R"cpp(
     int x = 42;
     auto* $var1[[var1]] = &x;
     auto&& $var2[[var2]] = x;
     const auto& $var3[[var3]] = x;
   )cpp",
                   ExpectedHint{": int *", "var1"},

clang/include/clang/AST/RecursiveASTVisitor.h

 })
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
   TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
   TRY_TO(TraverseDeclarationNameInfo(D->getNameInfo()));
 
+  bool BodyWasInstantiated = false;
   // If we're an explicit template specialization, iterate over the
   // template args that were explicitly specified.  If we were doing
   // this in typing order, we'd do it between the return type and
   // the function args, but both are handled by the FunctionTypeLoc
   // above, so we have to choose one side.  I've decided to do before.
   if (const FunctionTemplateSpecializationInfo *FTSI =
           D->getTemplateSpecializationInfo()) {
-    if (FTSI->getTemplateSpecializationKind() != TSK_Undeclared &&
-        FTSI->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) {
+    if (FTSI->isExplicitInstantiationOrSpecialization()) {
       // A specialization might not have explicit template arguments if it has
       // a templated return type and concrete arguments.
       if (const ASTTemplateArgumentListInfo *TALI =
               FTSI->TemplateArgumentsAsWritten) {
         TRY_TO(TraverseTemplateArgumentLocsHelper(TALI->getTemplateArgs(),
                                                   TALI->NumTemplateArgs));
       }
     }
+    BodyWasInstantiated =
+        isTemplateInstantiation(FTSI->getTemplateSpecializationKind());
   }
 
   // Visit the function type itself, which can be either
   // FunctionNoProtoType or FunctionProtoType, or a typedef.  This
   // also covers the return type and the function parameters,
   // including exception specifications.
   if (TypeSourceInfo *TSI = D->getTypeSourceInfo()) {
     TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));
   } else if (getDerived().shouldVisitImplicitCode()) {
     // Visit parameter variable declarations of the implicit function
     // if the traverser is visiting implicit code. Parameter variable
     // declarations do not have valid TypeSourceInfo, so to visit them
     // we need to traverse the declarations explicitly.
     for (ParmVarDecl *Parameter : D->parameters()) {
       TRY_TO(TraverseDecl(Parameter));
     }
   }
 
   // Visit the trailing requires clause, if any.
   if (Expr *TrailingRequiresClause = D->getTrailingRequiresClause()) {
     TRY_TO(TraverseStmt(TrailingRequiresClause));
   }
 
-  if (CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(D)) {
-    // Constructor initializers.
-    for (auto *I : Ctor->inits()) {
-      if (I->isWritten() || getDerived().shouldVisitImplicitCode())
-        TRY_TO(TraverseConstructorInitializer(I));
-    }
-  }
-
   bool VisitBody =
       D->isThisDeclarationADefinition() &&
+      // Don't visit the function body if it was instantiated, not written.
+      (!BodyWasInstantiated ||
+       getDerived().shouldVisitTemplateInstantiations()) &&
       // Don't visit the function body if the function definition is generated
       // by clang.
       (!D->isDefaulted() || getDerived().shouldVisitImplicitCode());
 
   if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
     if (const CXXRecordDecl *RD = MD->getParent()) {
       if (RD->isLambda() &&
           declaresSameEntity(RD->getLambdaCallOperator(), MD)) {
         VisitBody = VisitBody && getDerived().shouldVisitLambdaBody();
       }
     }
   }
 
   if (VisitBody) {
+    if (CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(D)) {
+      // Constructor initializers.
+      for (auto *I : Ctor->inits()) {
+        if (I->isWritten() || getDerived().shouldVisitImplicitCode())
+          TRY_TO(TraverseConstructorInitializer(I));
+      }
+    }
+
     TRY_TO(TraverseStmt(D->getBody()));
     // Body may contain using declarations whose shadows are parented to the
     // FunctionDecl itself.
     for (auto *Child : D->decls()) {
       if (isa<UsingShadowDecl>(Child))
         TRY_TO(TraverseDecl(Child));
     }
   }

clang/lib/CodeGen/CodeGenPGO.cpp

if (PGOReader->getVersion() <= 5)
     return PGO_HASH_V2;
   return PGO_HASH_V3;
 }
 
 /// A RecursiveASTVisitor that fills a map of statements to PGO counters.
 struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
   using Base = RecursiveASTVisitor<MapRegionCounters>;
 
+  bool shouldVisitTemplateInstantiations() { return true; }
+
   /// The next counter value to assign.
   unsigned NextCounter;
   /// The function hash.
   PGOHash Hash;
   /// The map of statements to counters.
   llvm::DenseMap<const Stmt *, unsigned> &CounterMap;
   /// The profile version.
   uint64_t ProfileVersion;

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp

const llvm::StringRef Code = R"cpp(
     // Primary template. Should be traversed.
     template <int> int foo() { return 0; }
     // Explicit specialization. Should be traversed.
     template <> int foo<1>() { return 1; }
     // Implicit instantiation. Should be traversed if instantiations are on.
     int x = foo<2>();
     // Explicit instantiation declaration. Head should be traversed (no body).
     extern template int foo<3>();
     // Explicit instantiation. Head traversed, body only with instantiations on.
     template int foo<4>();
   )cpp";
 
   // FIXME: explicit instantiation decl/definition are missing.
   checkLog("Default mode, without instantiations", Visitor, Code, R"(
 TraverseFunctionTemplateDecl
   VisitFunctionTemplateDecl
   TraverseFunctionDecl

 TraverseFunctionDecl
   VisitFunctionDecl foo ExplicitSpecialization
   VisitCompoundStmt
 TraverseVarDecl
   VisitVarDecl x
 )");
 
   Visitor.Instantiations = false;
   Visitor.ChooseTraversalRoot = [](ASTContext &Ctx) -> Decl * {
     for (auto *D : Ctx.getTranslationUnitDecl()->decls()) {
       if (auto *FTD = llvm::dyn_cast<FunctionTemplateDecl>(D)) {
         for (auto *Spec : FTD->specializations())
           if (Spec->getTemplateSpecializationKind() ==
               clang::TSK_ExplicitInstantiationDefinition)
             return Spec;
       }
     }
     ADD_FAILURE() << "Could not find an explicit instantiation!";
     return Ctx.getTranslationUnitDecl();
   };
-  // FIXME: we should not visit the body, as instantiation traversals are off.
   checkLog("Explicitly traversing an explicit instantiation", Visitor, Code, R"(
 TraverseFunctionDecl
   VisitFunctionDecl foo ExplicitInstantiationDefinition
-  VisitCompoundStmt
 )");
 }
 
 TEST(RecursiveASTVisitor, VisitsClassTemplates) {
   TemplateVisitor Visitor;
   const llvm::StringRef Code = R"cpp(
     // Primary template. Should be traversed.
     template <class> class foo { int a; };