[clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl.

[hokein]

Summary

Given the following VarTemplateDecl AST,

VarTemplateDecl col:26 X
|-TemplateTypeParmDecl typename depth 0 index 0
`-VarDecl X 'bool' cinit
  `-CXXBoolLiteralExpr 'bool' true

previously, we returned the VarDecl as the top-level decl, which was not
correct, the top-level decl should be VarTemplateDecl.

[sammccall]

Nice catch, LG with readability nits

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

127

(you could fold this into the above test if you want)

132

Is WithTemplateArgs here testing what you want (args vs params)?
Seems like it would be clearer to assert on DeclKind

clang/lib/Parse/ParseDecl.cpp

2212–2214

The purpose of the renaming is less than obvious here.

Can we rename VarTemplateDecl to OuterDecl or so? (And change the type to Decl)

[sammccall]

Is this https://github.com/clangd/clangd/issues/554 ? :-)

[hokein]

address review comments.

[hokein]

Is this https://github.com/clangd/clangd/issues/554 ? :-)

Yeah, the github issue exposes multiple bugs, this is part of the fix.

The AST of VarTemplateSpeicalizationDecl is a bit unusual, given the follow code,

template <typename> bool X = true;
bool Z = X<int>;

TranslationUnitDecl 0x8a2ec28 <<invalid sloc>> <invalid sloc>
|-VarTemplateDecl 0x8a6faf8 </tmp/t.cpp:1:1, col:30> col:26 X
| |-TemplateTypeParmDecl 0x8a6f9c0 <col:11> col:19 typename depth 0 index 0
| |-VarDecl 0x8a6fa90 <col:21, col:30> col:26 X 'bool' cinit
| | `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true
| `-VarTemplateSpecializationDecl 0x8a6fd08 <col:21, col:30> col:26 used X 'bool' cinit
|   |-TemplateArgument type 'int'
|   | `-BuiltinType 0x8a2ed20 'int'
|   `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true
|-VarDecl 0x8a6fbb8 <line:2:1, col:15> col:6 Z 'bool' cinit
| `-ImplicitCastExpr 0x8a6ff28 <col:10, col:15> 'bool' <LValueToRValue>
|   `-DeclRefExpr 0x8a6fed8 <col:10, col:15> 'bool' lvalue VarTemplateSpecialization 0x8a6fd08 'X' 'bool'
`-VarTemplateSpecializationDecl 0x8a6fd08 <line:1:21, col:30> col:26 used X 'bool' cinit   <---- here
  |-TemplateArgument type 'int'
  | `-BuiltinType 0x8a2ed20 'int'
  `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true

Note that the implicitly-instantiated VarTemplateSpecializationDecl is one of TranslationUnitDecl::decls(), this is different than function/class templates -- for function/class templates, only *explicit* template specializations will be in the TranslationUnitDecl::decls()

Related code is at https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L5015. @rsmith do you think this is expected? or we should fix that? like making it align with function/class templates?

[nridge]

I posted about this a while ago, but wasn't sure how to fix. Thanks for fixing!

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

if (ND->getName() == Name)
       return true;
   if (auto *Stream = result_listener->stream()) {
     llvm::raw_os_ostream OS(*Stream);
     arg->dump(OS);
   }
   return false;
 }
 
+MATCHER_P(DeclKind, Kind, "") {
+  if (NamedDecl *ND = dyn_cast<NamedDecl>(arg))
+    return ND->getDeclKindName() == Kind;
+  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;

 }
 
 TEST(ParsedASTTest, TopLevelDecls) {
   TestTU TU;
   TU.HeaderCode = R"(
     int header1();
     int header2;
   )";
-  TU.Code = "int main();";
+  TU.Code = R"cpp(
+    int main();
+    template <typename> bool X = true;
+  )cpp";
   auto AST = TU.build();
-  EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
+  EXPECT_THAT(
+      AST.getLocalTopLevelDecls(),
+      ElementsAreArray({AllOf(DeclNamed("main"), DeclKind("Function")),
+                        AllOf(DeclNamed("X"), DeclKind("VarTemplate"))}));
+}
+
+TEST(ParsedASTTest, VarTemplateDecl) {
+  TestTU TU;
+  TU.Code = "template <typename> bool X = true;";
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+              ElementsAre(AllOf(DeclNamed("X"), WithTemplateArgs(""))));
 }
 
 TEST(ParsedASTTest, 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();

clang/lib/Parse/ParseDecl.cpp

void pop() {
           P.ExitScope();
       }
       ThisDecl = nullptr;
     }
   };
 
   // Inform the current actions module that we just parsed this declarator.
   Decl *ThisDecl = nullptr;
+  Decl *OuterDecl = nullptr;
   switch (TemplateInfo.Kind) {
   case ParsedTemplateInfo::NonTemplate:
     ThisDecl = Actions.ActOnDeclarator(getCurScope(), D);
     break;
 
   case ParsedTemplateInfo::Template:
   case ParsedTemplateInfo::ExplicitSpecialization: {
     ThisDecl = Actions.ActOnTemplateDeclarator(getCurScope(),
                                                *TemplateInfo.TemplateParams,
                                                D);
-    if (VarTemplateDecl *VT = dyn_cast_or_null<VarTemplateDecl>(ThisDecl))
+    if (VarTemplateDecl *VT = dyn_cast_or_null<VarTemplateDecl>(ThisDecl)) {
       // Re-direct this decl to refer to the templated decl so that we can
       // initialize it.
       ThisDecl = VT->getTemplatedDecl();
+      OuterDecl = VT;
+    }
     break;
   }
   case ParsedTemplateInfo::ExplicitInstantiation: {
     if (Tok.is(tok::semi)) {
       DeclResult ThisRes = Actions.ActOnExplicitInstantiation(
           getCurScope(), TemplateInfo.ExternLoc, TemplateInfo.TemplateLoc, D);
       if (ThisRes.isInvalid()) {
         SkipUntil(tok::semi, StopBeforeMatch);
if (isTokenEqualOrEqualTypo()) {
     } else
       Actions.AddInitializerToDecl(ThisDecl, Init.get(), /*DirectInit=*/true);
 
   } else {
     Actions.ActOnUninitializedDecl(ThisDecl);
   }
 
   Actions.FinalizeDeclaration(ThisDecl);
-
-  return ThisDecl;
+  return OuterDecl ? OuterDecl : ThisDecl;
 }
 
 /// ParseSpecifierQualifierList
 ///        specifier-qualifier-list:
 ///          type-specifier specifier-qualifier-list[opt]
 ///          type-qualifier specifier-qualifier-list[opt]
 /// [GNU]    attributes     specifier-qualifier-list[opt]
 ///