[AST] Fix clang RecursiveASTVisitor for definition of XXXTemplateSpecializationDecl

[daiyousei-qz]

Summary

The original discussion starts at this issue: https://github.com/clangd/clangd/issues/1132
where clangd's semantic highlighting is missing for symbols of a template specialization definition. It turns out the visitor didn't traverse the base classes of Class/Var##TemplateSpecializationDecl, i.e. CXXRecordDecl/VarDecl. This patch adds them back as what is done in DEF_TRAVERSE_TMPL_PART_SPEC_DECL.

I'm not sure who should be included as reviewers. Please edit as needed.

[nridge]

Thanks for the patch!

Could you add a semantic highlighting test case to SemanticHighlighting.GetsCorrectTokens as well please? The test cases there are pieces of code annotated with the type of highlighting kind each token should get.

clang/include/clang/AST/RecursiveASTVisitor.h

2036–2037

We should move this call to TraverseNestedNameSpecifierLoc into the else branch, since Traverse...Helper already does that:

• TraverseCXXRecordHelper does it via TraverseRecordHelper here
• TraverseVarHelper does it via TraverseDeclaratorHelper here

[daiyousei-qz]

added unit test
moved redundant visit

[nridge]

It seems you have uploaded just the changes relative to the previous patch. If you made a second commit, you need to squash it into the first and resubmit.

Otherwise the test looks good, thanks!

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

811

nit: // Explicit template specialization to be a bit more specific

[daiyousei-qz]

squashed commits.
updated comments.

[rsmith]

clang/include/clang/AST/RecursiveASTVisitor.h

2040

This should be done automatically by the WalkUpFrom* implementation, not here. Do you know why that's not happening?

[rsmith]

clang/include/clang/AST/RecursiveASTVisitor.h

2040

Oh, never mind, this is doing the "traverse" part not the "visit" part. Ignore that comment!

[nridge]

Thanks, the patch looks good to me, but someone else should probably take a look and approve.

Adding @sammccall who has looked at RecursiveASTVisitor recently in D120498.

[nridge]

Thanks Richard for the review!

@daiyousei-qz would you like me to commit the patch for you? (If so, I need a name + email for attribution, there doesn't seem to be one in the revision metadata.)

[daiyousei-qz]

Yes, please do so. I'm using the web interface since the arc tool isn't very friendly to Windows user :(

Name: Qingyuan Zheng
Email: [e-mail]

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

)cpp",
         <:<:deprecated:>:> void $Function_decl_deprecated[[Foo]](int* $Parameter_decl[[x]]);
         void $Function_decl[[Foo]](int $Parameter_decl[[x]]);
         template <typename $TemplateParameter_decl[[T]]>
         void $Function_decl[[Bar]]($TemplateParameter[[T]] $Parameter_decl[[x]]) {
             $Function_deprecated[[Foo]]($Parameter[[x]]); 
             $Function_deprecated[[Foo]]($Parameter[[x]]); 
             $Function_deprecated[[Foo]]($Parameter[[x]]); 
         }
+      )cpp",
+      // Explicit template specialization
+      R"cpp(
+        struct $Class_decl[[Base]]{};
+        template <typename $TemplateParameter_decl[[T]]>
+        struct $Class_decl[[S]] : public $Class[[Base]] {};
+        template <> 
+        struct $Class_decl[[S]]<void> : public $Class[[Base]] {};
+
+        template <typename $TemplateParameter_decl[[T]]>
+        $TemplateParameter[[T]] $Variable_decl[[x]] = {};
+        template <>
+        int $Variable_decl[[x]]<int> = (int)sizeof($Class[[Base]]);
       )cpp"};
   for (const auto &TestCase : TestCases)
     // Mask off scope modifiers to keep the tests manageable.
     // They're tested separately.
     checkHighlightings(TestCase, {}, ~ScopeModifierMask);
 
   checkHighlightings(R"cpp(
     class $Class_decl[[A]] {

clang/include/clang/AST/RecursiveASTVisitor.h

bool RecursiveASTVisitor<Derived>::TraverseCXXRecordHelper(CXXRecordDecl *D) {
   }
   return true;
 }
 
 DEF_TRAVERSE_DECL(RecordDecl, { TRY_TO(TraverseRecordHelper(D)); })
 
 DEF_TRAVERSE_DECL(CXXRecordDecl, { TRY_TO(TraverseCXXRecordHelper(D)); })
 
-#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND)                              \
+#define DEF_TRAVERSE_TMPL_SPEC_DECL(TMPLDECLKIND, DECLKIND)                    \
   DEF_TRAVERSE_DECL(TMPLDECLKIND##TemplateSpecializationDecl, {                \
     /* For implicit instantiations ("set<int> x;"), we don't want to           \
        recurse at all, since the instatiated template isn't written in         \
        the source code anywhere.  (Note the instatiated *type* --              \
        set<int> -- is written, and will still get a callback of                \
        TemplateSpecializationType).  For explicit instantiations               \
        ("template set<int>;"), we do need a callback, since this               \
        is the only callback that's made for this instantiation.                \
        We use getTypeAsWritten() to distinguish. */                            \
     if (TypeSourceInfo *TSI = D->getTypeAsWritten())                           \
       TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));                              \
                                                                                \
-    TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));              \
-    if (!getDerived().shouldVisitTemplateInstantiations() &&                   \
-        D->getTemplateSpecializationKind() != TSK_ExplicitSpecialization)      \
+    if (getDerived().shouldVisitTemplateInstantiations() ||                    \
+        D->getTemplateSpecializationKind() == TSK_ExplicitSpecialization) {    \
+      /* Traverse base definition for explicit specializations */              \
+      TRY_TO(Traverse##DECLKIND##Helper(D));                                   \
+    } else {                                                                   \
+      TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));            \
+                                                                               \
       /* Returning from here skips traversing the                              \
          declaration context of the *TemplateSpecializationDecl                \
          (embedded in the DEF_TRAVERSE_DECL() macro)                           \
          which contains the instantiated members of the template. */           \
       return true;                                                             \
+    }                                                                          \
   })
 
-DEF_TRAVERSE_TMPL_SPEC_DECL(Class)
-DEF_TRAVERSE_TMPL_SPEC_DECL(Var)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Class, CXXRecord)
+DEF_TRAVERSE_TMPL_SPEC_DECL(Var, Var)
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseTemplateArgumentLocsHelper(
     const TemplateArgumentLoc *TAL, unsigned Count) {
   for (unsigned I = 0; I < Count; ++I) {
     TRY_TO(TraverseTemplateArgumentLoc(TAL[I]));
   }
   return true;