[clang] Remove written template args from implicit var tpl spec #156329

bolshakov-a · 2025-09-01T13:33:54Z

VarTemplateSpecializationDecl::getTemplateArgsAsWritten() function should return nullptr in the case of implicit instantiation, as its ClassTemplateSpecializationDecl counterpart does, and not the arguments written in DeclRefExpr referencing the specialization in the first place. Otherwise, for such code:

template <typename>
int VarTpl;

template <typename T>
void tplFn() {
  (void)VarTpl<T>;  // (1)
}

void fn() {
  tplFn<char>();
}

Clang treats the char argument of the VarTpl specialization as if it were written in the line marked as (1), which is misleading and hardly makes sense.

Moreover, "template args as written" are stored inside ExplicitInfo field of VarTemplateSpecializationDecl, but it is documented that it is not for implicit instantiations.

Moreover, it is assumed in TraverseVarTemplateSpecializationDecl method of RecursiveASTVisitor that getTemplateArgsAsWritten() returns nullptr for implicit instantiations, as it is stated in the comment there.

That said, setTemplateArgsAsWritten should be called only for variable template explicit specializations (it is already done inside Sema::ActOnVarTemplateSpecialization) and explicit instantiations (hence true is passed to the new SetWrittenArgs parameter of CheckVarTemplateId function inside Sema::ActOnExplicitInstantiation, but not when handling expressions referencing a variable template specialization). InstantiateVariableDefinition function just passes the arguments from the corresponding declaration. I'm not sure about instantiating a class template containing a variable template explicit specialization and thus have tried to leave the logic of the first overload of TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl as it was.

llvmbot · 2025-09-01T13:34:27Z

@llvm/pr-subscribers-clang

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

VarTemplateSpecializationDecl::getTemplateArgsAsWritten() function should return nullptr in the case of implicit instantiation, as its ClassTemplateSpecializationDecl counterpart does, and not the arguments written in DeclRefExpr referencing the specialization in the first place. Otherwise, for such code:

template &lt;typename&gt;
int VarTpl;

template &lt;typename T&gt;
void tplFn() {
  (void)VarTpl&lt;T&gt;;  // (1)
}

void fn() {
  tplFn&lt;char&gt;();
}

Clang treats the char argument of the VarTpl specialization as if it were written in the line marked as (1), which is misleading and hardly makes sense.

Moreover, "template args as written" are stored inside ExplicitInfo field of VarTemplateSpecializationDecl, but it is documented that it is not for implicit instantiations.

Moreover, it is assumed in TraverseVarTemplateSpecializationDecl method of RecursiveASTVisitor that getTemplateArgsAsWritten() returns nullptr for implicit instantiations, as it is stated in the comment there.

That said, setTemplateArgsAsWritten should be called only for variable template explicit specializations (it is already done inside Sema::ActOnVarTemplateSpecialization) and explicit instantiations (hence true is passed to the new SetWrittenArgs parameter of CheckVarTemplateId function inside Sema::ActOnExplicitInstantiation, but not when handling expressions referencing a variable template specialization). InstantiateVariableDefinition function just passes the arguments from the corresponding declaration. I'm not sure about instantiating a class template containing a variable template explicit specialization and thus have tried to leave the logic of the first overload of TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl as it was.

Full diff: https://github.com/llvm/llvm-project/pull/156329.diff

6 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+2-2)
(modified) clang/include/clang/Sema/Template.h (+1-2)
(modified) clang/lib/Sema/SemaExprMember.cpp (+3-2)
(modified) clang/lib/Sema/SemaTemplate.cpp (+10-6)
(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+18-13)
(modified) clang/unittests/AST/DeclTest.cpp (+19)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index c3fb57774c8dc..224095070cbe3 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11662,7 +11662,8 @@ class Sema final : public SemaBase {
   DeclResult CheckVarTemplateId(VarTemplateDecl *Template,
                                 SourceLocation TemplateLoc,
                                 SourceLocation TemplateNameLoc,
-                                const TemplateArgumentListInfo &TemplateArgs);
+                                const TemplateArgumentListInfo &TemplateArgs,
+                                bool SetWrittenArgs);
 
   /// Form a reference to the specialization of the given variable template
   /// corresponding to the specified argument list, or a null-but-valid result
@@ -14022,7 +14023,6 @@ class Sema final : public SemaBase {
   VarTemplateSpecializationDecl *BuildVarTemplateInstantiation(
       VarTemplateDecl *VarTemplate, VarDecl *FromVar,
       const TemplateArgumentList *PartialSpecArgs,
-      const TemplateArgumentListInfo &TemplateArgsInfo,
       SmallVectorImpl<TemplateArgument> &Converted,
       SourceLocation PointOfInstantiation,
       LateInstantiatedAttrVec *LateAttrs = nullptr,
diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
index fe907078af275..115c19d4f1540 100644
--- a/clang/include/clang/Sema/Template.h
+++ b/clang/include/clang/Sema/Template.h
@@ -723,9 +723,8 @@ enum class TemplateSubstitutionKind : char {
     bool SubstQualifier(const TagDecl *OldDecl,
                         TagDecl *NewDecl);
 
-    Decl *VisitVarTemplateSpecializationDecl(
+    VarTemplateSpecializationDecl *VisitVarTemplateSpecializationDecl(
         VarTemplateDecl *VarTemplate, VarDecl *FromVar,
-        const TemplateArgumentListInfo &TemplateArgsInfo,
         ArrayRef<TemplateArgument> Converted,
         VarTemplateSpecializationDecl *PrevDecl = nullptr);
 
diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index 4a31a139eaf4f..aedfc5e88b1c6 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -1126,8 +1126,9 @@ Sema::BuildMemberReferenceExpr(Expr *BaseExpr, QualType BaseExprType,
       return ExprError();
     }
 
-    DeclResult VDecl = CheckVarTemplateId(VarTempl, TemplateKWLoc,
-                                          MemberNameInfo.getLoc(), *TemplateArgs);
+    DeclResult VDecl =
+        CheckVarTemplateId(VarTempl, TemplateKWLoc, MemberNameInfo.getLoc(),
+                           *TemplateArgs, /*SetWrittenArgs=*/false);
     if (VDecl.isInvalid())
       return ExprError();
 
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 3d8416ac7dc1b..3533871ad4dad 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -4542,7 +4542,8 @@ static bool IsLibstdcxxStdFormatKind(Preprocessor &PP, VarDecl *Var) {
 DeclResult
 Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
                          SourceLocation TemplateNameLoc,
-                         const TemplateArgumentListInfo &TemplateArgs) {
+                         const TemplateArgumentListInfo &TemplateArgs,
+                         bool SetWrittenArgs) {
   assert(Template && "A variable template id without template?");
 
   // Check that the template argument list is well-formed for this template.
@@ -4725,10 +4726,12 @@ Sema::CheckVarTemplateId(VarTemplateDecl *Template, SourceLocation TemplateLoc,
   // in DoMarkVarDeclReferenced().
   // FIXME: LateAttrs et al.?
   VarTemplateSpecializationDecl *Decl = BuildVarTemplateInstantiation(
-      Template, InstantiationPattern, PartialSpecArgs, TemplateArgs,
-      CTAI.CanonicalConverted, TemplateNameLoc /*, LateAttrs, StartingScope*/);
+      Template, InstantiationPattern, PartialSpecArgs, CTAI.CanonicalConverted,
+      TemplateNameLoc /*, LateAttrs, StartingScope*/);
   if (!Decl)
     return true;
+  if (SetWrittenArgs)
+    Decl->setTemplateArgsAsWritten(TemplateArgs);
 
   if (AmbiguousPartialSpec) {
     // Partial ordering did not produce a clear winner. Complain.
@@ -4760,7 +4763,7 @@ ExprResult Sema::CheckVarTemplateId(
     const TemplateArgumentListInfo *TemplateArgs) {
 
   DeclResult Decl = CheckVarTemplateId(Template, TemplateLoc, NameInfo.getLoc(),
-                                       *TemplateArgs);
+                                       *TemplateArgs, /*SetWrittenArgs=*/false);
   if (Decl.isInvalid())
     return ExprError();
 
@@ -10707,8 +10710,9 @@ DeclResult Sema::ActOnExplicitInstantiation(Scope *S,
       TemplateArgumentListInfo TemplateArgs =
           makeTemplateArgumentListInfo(*this, *D.getName().TemplateId);
 
-      DeclResult Res = CheckVarTemplateId(PrevTemplate, TemplateLoc,
-                                          D.getIdentifierLoc(), TemplateArgs);
+      DeclResult Res =
+          CheckVarTemplateId(PrevTemplate, TemplateLoc, D.getIdentifierLoc(),
+                             TemplateArgs, /*SetWrittenArgs=*/true);
       if (Res.isInvalid())
         return true;
 
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index ee1b520fa46e9..910f99d3eb160 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4542,14 +4542,17 @@ Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
                       PrevDecl->getPointOfInstantiation(), Ignored))
     return nullptr;
 
-  return VisitVarTemplateSpecializationDecl(InstVarTemplate, D,
-                                            VarTemplateArgsInfo,
-                                            CTAI.CanonicalConverted, PrevDecl);
+  if (VarTemplateSpecializationDecl *VTSD = VisitVarTemplateSpecializationDecl(
+          InstVarTemplate, D, CTAI.CanonicalConverted, PrevDecl)) {
+    VTSD->setTemplateArgsAsWritten(VarTemplateArgsInfo);
+    return VTSD;
+  }
+  return nullptr;
 }
 
-Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
+VarTemplateSpecializationDecl *
+TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
     VarTemplateDecl *VarTemplate, VarDecl *D,
-    const TemplateArgumentListInfo &TemplateArgsInfo,
     ArrayRef<TemplateArgument> Converted,
     VarTemplateSpecializationDecl *PrevDecl) {
 
@@ -4570,7 +4573,6 @@ Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
   VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
       SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
       VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
-  Var->setTemplateArgsAsWritten(TemplateArgsInfo);
   if (!PrevDecl) {
     void *InsertPos = nullptr;
     VarTemplate->findSpecialization(Converted, InsertPos);
@@ -5880,7 +5882,6 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
 VarTemplateSpecializationDecl *Sema::BuildVarTemplateInstantiation(
     VarTemplateDecl *VarTemplate, VarDecl *FromVar,
     const TemplateArgumentList *PartialSpecArgs,
-    const TemplateArgumentListInfo &TemplateArgsInfo,
     SmallVectorImpl<TemplateArgument> &Converted,
     SourceLocation PointOfInstantiation, LateInstantiatedAttrVec *LateAttrs,
     LocalInstantiationScope *StartingScope) {
@@ -5922,9 +5923,8 @@ VarTemplateSpecializationDecl *Sema::BuildVarTemplateInstantiation(
 
   // TODO: Set LateAttrs and StartingScope ...
 
-  return cast_or_null<VarTemplateSpecializationDecl>(
-      Instantiator.VisitVarTemplateSpecializationDecl(
-          VarTemplate, FromVar, TemplateArgsInfo, Converted));
+  return Instantiator.VisitVarTemplateSpecializationDecl(VarTemplate, FromVar,
+                                                         Converted);
 }
 
 VarTemplateSpecializationDecl *Sema::CompleteVarTemplateSpecializationDecl(
@@ -6340,10 +6340,15 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
         TemplateArgInfo.addArgument(Arg);
     }
 
-    Var = cast_or_null<VarDecl>(Instantiator.VisitVarTemplateSpecializationDecl(
-        VarSpec->getSpecializedTemplate(), Def, TemplateArgInfo,
-        VarSpec->getTemplateArgs().asArray(), VarSpec));
+    VarTemplateSpecializationDecl *VTSD =
+        Instantiator.VisitVarTemplateSpecializationDecl(
+            VarSpec->getSpecializedTemplate(), Def,
+            VarSpec->getTemplateArgs().asArray(), VarSpec);
+    Var = VTSD;
+
     if (Var) {
+      VTSD->setTemplateArgsAsWritten(TemplateArgInfo);
+
       llvm::PointerUnion<VarTemplateDecl *,
                          VarTemplatePartialSpecializationDecl *> PatternPtr =
           VarSpec->getSpecializedTemplateOrPartial();
diff --git a/clang/unittests/AST/DeclTest.cpp b/clang/unittests/AST/DeclTest.cpp
index 4c83ff9142e87..e76edbfe95b68 100644
--- a/clang/unittests/AST/DeclTest.cpp
+++ b/clang/unittests/AST/DeclTest.cpp
@@ -586,3 +586,22 @@ namespace x::y {
   ASSERT_NE(FD, nullptr);
   ASSERT_EQ(FD->getQualifiedNameAsString(), "x::y::Foo::Foo<T>");
 }
+
+TEST(Decl, NoWrittenArgsInImplicitlyInstantiatedVarSpec) {
+  const char *Code = R"cpp(
+    template <typename>
+    int VarTpl;
+
+    void fn() {
+      (void)VarTpl<char>;
+    }
+  )cpp";
+
+  auto AST = tooling::buildASTFromCode(Code);
+  ASTContext &Ctx = AST->getASTContext();
+
+  const auto *VTSD = selectFirst<VarTemplateSpecializationDecl>(
+      "id", match(varDecl(isTemplateInstantiation()).bind("id"), Ctx));
+  ASSERT_NE(VTSD, nullptr);
+  EXPECT_EQ(VTSD->getTemplateArgsAsWritten(), nullptr);
+}

bolshakov-a · 2025-09-01T13:36:35Z

@erichkeane, @mizvekov, @shafik

zyn0217

I think we need a release note, maybe 'Potential AST breaking changes' would be a good fit.

bolshakov-a · 2025-09-02T09:08:22Z

I always forget about release notes, thanks! However, there isn't "Potential AST breaking changes" section but there is "AST Dumping Potentially Breaking Changes" one. But this PR doesn't change AST dumps. Maybe, "C++ Specific Potentially Breaking Changes"?

erichkeane · 2025-09-02T13:49:38Z

clang/docs/ReleaseNotes.rst

@@ -84,6 +84,9 @@ C++ Specific Potentially Breaking Changes
    static_assert((b.*mp)() == 1); // newly rejected
    static_assert((c.*mp)() == 1); // accepted

+- ``VarTemplateSpecializationDecl::getTemplateArgsAsWritten()`` method returns


This isn't really a useful release note for someone who uses our compiler. We have to explain WHAT is happening from a user perspective.

I don't think there is any change from user's perspective. I didn't succeeded in finding any related user-observable bug. In fact, this change fixes a bug in the IWYU tool. However, those who use Clang as a library (like IWYU) are users as well, aren't they? And this PR fixes the bug in the Clang library interface (or at least makes it more consistent).

There is a related bug that RecursiveASTVisitor traverses (by default) implicitly instantiated variable template specializations and calls VisitVarTemplateSpecializationDecl for them, which manifests itself in the ExtractAPI, AFAIU. Namely, for such code:

template <typename> int var_tpl; inline void Fn(){ (void)var_tpl<double>; }

Clang writes "Global Variable Template Specialization" record in the JSON file which looks like a bug because the specialization is implicit. CC @zixu-w, @daniel-grumberg.

I don't mind if you suggest just to remove the relnote or move it into some other section.

also CC @QuietMisdreavus for the ExtractAPI impact. At a glance I believe this is also fine for ExtractAPI (that the template specialization record is not expected in the symbol graph output for this one).

I believe this is also fine for ExtractAPI

Do you mean this PR? It doesn't fix this bug. (But fixing it would also fix the problem with IWYU, tbh)

Should probably be: now returns nullptr. Also, I THINK this probably belongs in teh ABI changes section then?

@erichkeane, I think that ABI is about name mangling and other codegen-related stuff? Maybe, such a small change in the internal AST representation just don't need mentioning in the relnotes?

mizvekov · 2025-09-02T16:32:56Z

clang/include/clang/Sema/Sema.h

@@ -11662,7 +11662,8 @@ class Sema final : public SemaBase {
  DeclResult CheckVarTemplateId(VarTemplateDecl *Template,
                                SourceLocation TemplateLoc,
                                SourceLocation TemplateNameLoc,
-                                const TemplateArgumentListInfo &TemplateArgs);
+                                const TemplateArgumentListInfo &TemplateArgs,
+                                bool SetWrittenArgs);


Rather than adding a new bool parameter, I think we better move the responsibility of setting the written args entirely out of CheckVarTemplateId, which would make this more similar to CheckTypeTemplateId.

Setting the args to the CheckVarTemplateId result may change some previous declaration stored in the ASTContext when CheckVarTemplateId returns it and not a new one.

I cannot find CheckTypeTemplateId in the project (typo?)

Err I meant CheckTemplateIdType, pretty unfortunate inconsistency in naming formulation.

Right, that's another inconsistency with how CheckTemplateIdType operates and is used.

This may need some though, I don't want to propose a large refactoring to support such a simple change, but the bool parameter does not feel right.

Seems like the difference is because CheckTemplateIdType returns template specialization type and not the specialization declaration.

mizvekov

LGTM.

I don't love the new bool parameter, but I don't see a simple way to avoid it, and this change is too simple to be worth asking for such a change.

bolshakov-a · 2025-09-04T02:11:08Z

Ok, thanks! Can you merge it please if you don't think that something should be done with #156329 (comment)?

mizvekov · 2025-09-04T02:18:43Z

I'd suggest wait for the other reviewers, or until next week if no response.

erichkeane

1 nit, else lgtm. Fix that and re-ping and I'll merge you.

erichkeane · 2025-09-04T14:00:18Z

clang/docs/ReleaseNotes.rst

@@ -84,6 +84,9 @@ C++ Specific Potentially Breaking Changes
    static_assert((b.*mp)() == 1); // newly rejected
    static_assert((c.*mp)() == 1); // accepted

+- ``VarTemplateSpecializationDecl::getTemplateArgsAsWritten()`` method returns


Should probably be: now returns nullptr. Also, I THINK this probably belongs in teh ABI changes section then?

VarTemplateSpecializationDecl::getTemplateArgsAsWritten() function should return nullptr in the case of implicit instantiation, as its ClassTemplateSpecializationDecl counterpart does, and not the arguments written in DeclRefExpr referencing the specialization in the first place. Otherwise, for such code: template <typename> int VarTpl; template <typename T> void tplFn() { (void)VarTpl<T>; // (1) } void fn() { tplFn<char>(); } Clang treats the 'char' argument of the VarTpl specialization as if it were written in the line marked as (1), which is misleading and hardly makes sense. Moreover, "template args as written" are stored inside ExplicitInfo field of VarTemplateSpecializationDecl, but it is documented that it is not for implicit instantiations. Moreover, it is assumed in TraverseVarTemplateSpecializationDecl method of RecursiveASTVisitor that getTemplateArgsAsWritten() returns nullptr for implicit instantiations, as it is stated in the comment there. That said, setTemplateArgsAsWritten should be called only for variable template explicit specializations (it is already done inside Sema::ActOnVarTemplateSpecialization) and explicit instantiations (hence 'true' is passed to the new SetWrittenArgs parameter of CheckVarTemplateId function inside Sema::ActOnExplicitInstantiation, but not when handling expressions referencing a variable template specialization). InstantiateVariableDefinition function just passes the arguments from the corresponding declaration. I'm not sure about instantiating a class template containing a variable template explicit specialization and thus have tried to leave the logic of the first overload of TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl as it was.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 1, 2025

cor3ntin requested a review from mizvekov September 1, 2025 14:49

zyn0217 reviewed Sep 2, 2025

View reviewed changes

bolshakov-a force-pushed the no_written_args_impl_inst_spec branch from 801d872 to 69887b3 Compare September 2, 2025 09:13

erichkeane reviewed Sep 2, 2025

View reviewed changes

mizvekov reviewed Sep 2, 2025

View reviewed changes

mizvekov approved these changes Sep 4, 2025

View reviewed changes

erichkeane approved these changes Sep 4, 2025

View reviewed changes

bolshakov-a force-pushed the no_written_args_impl_inst_spec branch from 69887b3 to 2e0a5fa Compare September 4, 2025 15:00

erichkeane enabled auto-merge (squash) September 4, 2025 15:01

erichkeane merged commit 1cb47c1 into llvm:main Sep 4, 2025
10 checks passed

bolshakov-a deleted the no_written_args_impl_inst_spec branch September 4, 2025 16:20

[clang] Remove written template args from implicit var tpl spec #156329

[clang] Remove written template args from implicit var tpl spec #156329

Conversation

bolshakov-a commented Sep 1, 2025

Uh oh!

llvmbot commented Sep 1, 2025

Uh oh!

bolshakov-a commented Sep 1, 2025

Uh oh!

zyn0217 left a comment

Choose a reason for hiding this comment

Uh oh!

bolshakov-a commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mizvekov Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

bolshakov-a commented Sep 4, 2025

Uh oh!

mizvekov commented Sep 4, 2025

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mizvekov Sep 2, 2025 •

edited

Loading