[Clang] Substitute for the type aliases inside of a CTAD guide #94740

zyn0217 · 2024-06-07T10:22:17Z

Similar to the approach of handling nested class templates when building a CTAD guide, we substitute the template parameters of a type alias declaration with the instantiating template arguments in order to ensure the guide eventually doesn't reference any outer template parameters.

For example,

template <class T> struct Outer {
  using Alias = S<T>;
  template <class U> struct Inner {
    Inner(Alias);
  };
};

we used to retain the reference to T accidentally because the TreeTransform does nothing on type alias Decls by default.

Fixes #94614

llvmbot · 2024-06-07T13:14:13Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Similar to the approach of handling nested class templates when building a CTAD guide, we substitute the template parameters of a type alias declaration with the instantiating template arguments in order to ensure the guide eventually doesn't reference any outer template parameters.

For example,

template &lt;class T&gt; struct Outer {
  using Alias = S&lt;T&gt;;
  template &lt;class U&gt; struct Inner {
    Inner(Alias);
  };
};

we used to retain the reference to T accidentally because the TreeTransform does nothing on type alias Decls by default.

Fixes #94614

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/lib/Sema/SemaTemplate.cpp (+91-7)
(modified) clang/test/SemaTemplate/nested-deduction-guides.cpp (+70)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0c700d23257bf..3f6d040b0ddf1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -823,6 +823,7 @@ Bug Fixes to C++ Support
   differering by their constraints when only one of these function was variadic.
 - Fix a crash when a variable is captured by a block nested inside a lambda. (Fixes #GH93625).
 - Fixed a type constraint substitution issue involving a generic lambda expression. (#GH93821)
+- Fixed a CTAD substitution bug involving type aliases that reference outer template parameters. (#GH94614).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 40a759ea330de..baf2f12bcbb07 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2220,23 +2220,103 @@ namespace {
 class ExtractTypeForDeductionGuide
   : public TreeTransform<ExtractTypeForDeductionGuide> {
   llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs;
+  ClassTemplateDecl *NestedPattern;
+  const MultiLevelTemplateArgumentList *OuterInstantiationArgs;
 
 public:
   typedef TreeTransform<ExtractTypeForDeductionGuide> Base;
   ExtractTypeForDeductionGuide(
       Sema &SemaRef,
-      llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs)
-      : Base(SemaRef), MaterializedTypedefs(MaterializedTypedefs) {}
+      llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs,
+      ClassTemplateDecl *NestedPattern,
+      const MultiLevelTemplateArgumentList *OuterInstantiationArgs)
+      : Base(SemaRef), MaterializedTypedefs(MaterializedTypedefs),
+        NestedPattern(NestedPattern),
+        OuterInstantiationArgs(OuterInstantiationArgs) {}
 
   TypeSourceInfo *transform(TypeSourceInfo *TSI) { return TransformType(TSI); }
 
+  /// Returns true if it's safe to substitute \p Typedef with
+  /// \p OuterInstantiationArgs.
+  bool mightReferToOuterTemplateParameters(TypedefNameDecl *Typedef) {
+    if (!NestedPattern)
+      return false;
+
+    static auto WalkUp = [](DeclContext *DC, DeclContext *TargetDC) {
+      if (DC->Equals(TargetDC))
+        return true;
+      while (DC->isRecord()) {
+        if (DC->Equals(TargetDC))
+          return true;
+        DC = DC->getParent();
+      }
+      return false;
+    };
+
+    if (WalkUp(Typedef->getDeclContext(), NestedPattern->getTemplatedDecl()))
+      return true;
+    if (WalkUp(NestedPattern->getTemplatedDecl(), Typedef->getDeclContext()))
+      return true;
+    return false;
+  }
+
+  QualType
+  RebuildTemplateSpecializationType(TemplateName Template,
+                                    SourceLocation TemplateNameLoc,
+                                    TemplateArgumentListInfo &TemplateArgs) {
+    if (!OuterInstantiationArgs ||
+        !isa_and_present<TypeAliasTemplateDecl>(Template.getAsTemplateDecl()))
+      return Base::RebuildTemplateSpecializationType(Template, TemplateNameLoc,
+                                                     TemplateArgs);
+
+    auto *TATD = cast<TypeAliasTemplateDecl>(Template.getAsTemplateDecl());
+    auto *Pattern = TATD;
+    while (Pattern->getInstantiatedFromMemberTemplate())
+      Pattern = Pattern->getInstantiatedFromMemberTemplate();
+    if (!mightReferToOuterTemplateParameters(Pattern->getTemplatedDecl()))
+      return Base::RebuildTemplateSpecializationType(Template, TemplateNameLoc,
+                                                     TemplateArgs);
+
+    Decl *NewD = SemaRef.SubstDecl(
+        TATD, SemaRef.getASTContext().getTranslationUnitDecl(),
+        *OuterInstantiationArgs);
+    if (!NewD)
+      return QualType();
+
+    auto *NewTATD = cast<TypeAliasTemplateDecl>(NewD);
+    MaterializedTypedefs.push_back(NewTATD->getTemplatedDecl());
+
+    return Base::RebuildTemplateSpecializationType(
+        TemplateName(NewTATD), TemplateNameLoc, TemplateArgs);
+  }
+
   QualType TransformTypedefType(TypeLocBuilder &TLB, TypedefTypeLoc TL) {
     ASTContext &Context = SemaRef.getASTContext();
     TypedefNameDecl *OrigDecl = TL.getTypedefNameDecl();
     TypedefNameDecl *Decl = OrigDecl;
     // Transform the underlying type of the typedef and clone the Decl only if
     // the typedef has a dependent context.
-    if (OrigDecl->getDeclContext()->isDependentContext()) {
+    bool InDependentContext = OrigDecl->getDeclContext()->isDependentContext();
+
+    // A typedef/alias Decl within the NestedPattern may reference the outer
+    // template parameters. They're substituted with corresponding instantiation
+    // arguments here and in RebuildTemplateSpecializationType() above.
+    // Otherwise, we would have a CTAD guide with "dangling" template
+    // parameters.
+    // For example,
+    //   template <class T> struct Outer {
+    //     using Alias = S<T>;
+    //     template <class U> struct Inner {
+    //       Inner(Alias);
+    //     };
+    //   };
+    if (OuterInstantiationArgs && InDependentContext) {
+      Decl = cast_if_present<TypedefNameDecl>(SemaRef.SubstDecl(
+          OrigDecl, Context.getTranslationUnitDecl(), *OuterInstantiationArgs));
+      if (!Decl)
+        return QualType();
+      MaterializedTypedefs.push_back(Decl);
+    } else if (InDependentContext) {
       TypeLocBuilder InnerTLB;
       QualType Transformed =
           TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
@@ -2577,8 +2657,9 @@ struct ConvertConstructorToDeductionGuideTransform {
       // defined outside of the surrounding class template. That is T in the
       // above example.
       if (NestedPattern) {
-        NewParam = transformFunctionTypeParam(NewParam, OuterInstantiationArgs,
-                                              MaterializedTypedefs);
+        NewParam = transformFunctionTypeParam(
+            NewParam, OuterInstantiationArgs, MaterializedTypedefs,
+            /*TransformingOuterPatterns=*/true);
         if (!NewParam)
           return QualType();
       }
@@ -2630,7 +2711,8 @@ struct ConvertConstructorToDeductionGuideTransform {
 
   ParmVarDecl *transformFunctionTypeParam(
       ParmVarDecl *OldParam, MultiLevelTemplateArgumentList &Args,
-      llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs) {
+      llvm::SmallVectorImpl<TypedefNameDecl *> &MaterializedTypedefs,
+      bool TransformingOuterPatterns = false) {
     TypeSourceInfo *OldDI = OldParam->getTypeSourceInfo();
     TypeSourceInfo *NewDI;
     if (auto PackTL = OldDI->getTypeLoc().getAs<PackExpansionTypeLoc>()) {
@@ -2653,7 +2735,9 @@ struct ConvertConstructorToDeductionGuideTransform {
     // members of the current instantiations with the definitions of those
     // typedefs, avoiding triggering instantiation of the deduced type during
     // deduction.
-    NewDI = ExtractTypeForDeductionGuide(SemaRef, MaterializedTypedefs)
+    NewDI = ExtractTypeForDeductionGuide(
+                SemaRef, MaterializedTypedefs, NestedPattern,
+                TransformingOuterPatterns ? &Args : nullptr)
                 .transform(NewDI);
 
     // Resolving a wording defect, we also inherit default arguments from the
diff --git a/clang/test/SemaTemplate/nested-deduction-guides.cpp b/clang/test/SemaTemplate/nested-deduction-guides.cpp
index 38410b93ead3b..913042810ae82 100644
--- a/clang/test/SemaTemplate/nested-deduction-guides.cpp
+++ b/clang/test/SemaTemplate/nested-deduction-guides.cpp
@@ -16,3 +16,73 @@ using T = A<void>::B<int>;
 
 using Copy = decltype(copy);
 using Copy = A<void>::B<int>;
+
+namespace GH94614 {
+
+template <class, class> struct S {};
+
+struct trouble_1 {
+} constexpr t1;
+struct trouble_2 {
+} constexpr t2;
+struct trouble_3 {
+} constexpr t3;
+struct trouble_4 {
+} constexpr t4;
+struct trouble_5 {
+} constexpr t5;
+struct trouble_6 {
+} constexpr t6;
+struct trouble_7 {
+} constexpr t7;
+struct trouble_8 {
+} constexpr t8;
+struct trouble_9 {
+} constexpr t9;
+
+template <class U, class... T> struct Unrelated {
+  using Trouble = S<U, T...>;
+
+  template <class... V> using Trouble2 = S<V..., T...>;
+};
+
+template <class T, class U> struct Outer {
+  using Trouble = S<U, T>;
+
+  template <class V> using Trouble2 = S<V, T>;
+
+  template <class V> using Trouble3 = S<U, T>;
+
+  template <class V> struct Inner {
+    template <class W> struct Paranoid {
+      using Trouble4 = S<W, T>;
+
+      template <class... X> using Trouble5 = S<X..., T>;
+    };
+
+    Inner(trouble_1, V v, Trouble trouble) {}
+    Inner(trouble_2, V v, Trouble2<V> trouble) {}
+    Inner(trouble_3, V v, Trouble3<V> trouble) {}
+    Inner(trouble_4, V v, typename Unrelated<U, T>::template Trouble2<V> trouble) {}
+    Inner(trouble_5, V v, typename Unrelated<U, T>::Trouble trouble) {}
+    Inner(trouble_6, V v, typename Unrelated<V, T>::Trouble trouble) {}
+    Inner(trouble_7, V v, typename Paranoid<V>::Trouble4 trouble) {}
+    Inner(trouble_8, V v, typename Paranoid<V>::template Trouble5<V> trouble) {}
+    template <class W>
+    Inner(trouble_9, V v, W w, typename Paranoid<V>::template Trouble5<W> trouble) {}
+  };
+};
+
+S<int, char> s;
+
+Outer<char, int>::Inner _1(t1, 42, s);
+Outer<char, int>::Inner _2(t2, 42, s);
+Outer<char, int>::Inner _3(t3, 42, s);
+Outer<char, int>::Inner _4(t4, 42, s);
+Outer<char, int>::Inner _5(t5, 42, s);
+Outer<char, int>::Inner _6(t6, 42, s);
+Outer<char, int>::Inner _7(t7, 42, s);
+Outer<char, int>::Inner _8(t8, 42, s);
+Outer<char, int>::Inner _9(t9, 42, 24, s);
+
+} // namespace GH94614

hokein

Looks roughly good to me.

hokein · 2024-06-11T10:44:08Z

clang/lib/Sema/SemaTemplate.cpp

+                                                     TemplateArgs);
+
+    Decl *NewD = SemaRef.SubstDecl(
+        TATD, SemaRef.getASTContext().getTranslationUnitDecl(),


Passing the TranslationUnitDecl seems wrong -- the NewD will be attached to the TU, I think we should use the DC member in ConvertConstructorToDeductionGuideTransform.

This is what we've done since D80743, when @zygoloid suggested that we delay setting the DeclContexts until the CXXDeductionGuideDecl is formed. This way, I guess (I didn't dig into the details) we could have template parameters attach to the right depths (e.g. presumably 1) rather than the depth relative to the outer class templates (DC).

I see, thanks for the link.

There is a subtle difference:

In D80743, we create a Decl by calling TypeAliasDecl::Create(.., TU, ...), which only sets the member TypeAliasDecl::DeclCtx to TU.

While here, we pass the TU to the SubstDecl. This has the side effect of adding the NewD into the TU (in the substitution implementation, TU->addDecl(NewD) is called). Consequently, in the AST dump, you will see NewD under the TranslationUnit, which doesn't look right. I guess we can fix this by calling TU->removeDecl(NewD).

Thinking more about this, we should have this information in the AST.

Given the example:

template <class T> struct Outer { using Alias = S<T>; template <class U> struct Inner { Inner(Alias); }; }; Outer<int>::Inner inner(S<int>());

We're performing a substitution for the Alias with T = int here. At this point, the Outer<int> class specialization should be available. Inside this specialization, we should have the TypeAliasDecl S<int>.
So an alternative would be to reuse this Decl rather than creating a new one. I'm not sure if this is feasible. It looks like we don't have a good way to get it from the primary TypeAliasTemplateDecl (no specializations in the AST node). A hacky way is to perform a lookup from Outer<int>.

There is a subtle difference:

Thanks for spotting that! This is indeed something I've overlooked.
Looking at the SubstDecl again, I think we can take the middle-ground approach: we call TemplateDeclInstantiator::InstantiateTypeAliasTemplateDecl() directly when we want to instantiate a TypedefNameDecl.

This is a plausible approach to me because

We could save the recursion into the DeclVisitor from SubstDecl because we already know the Decl is a TypedefDecl.

This avoids the accidental Decl injection, although we have to tweak the TemplateDeclInstantiator a bit.

Re: So an alternative would be to reuse this Decl rather than creating a new one: granted that we could find that instantiated Decl, we still have to copy the Decl, which is in part a duplicate work of the instantiation logic, because we want the transformed Decl to live in the CXXDeductionGuideDecl - I think we can still leverage this instantiation mechanism to keep it intelligible.

hokein · 2024-06-11T10:44:26Z

clang/lib/Sema/SemaTemplate.cpp

+    //   };
+    if (OuterInstantiationArgs && InDependentContext) {
+      Decl = cast_if_present<TypedefNameDecl>(SemaRef.SubstDecl(
+          OrigDecl, Context.getTranslationUnitDecl(), *OuterInstantiationArgs));


and here as well.

clang/lib/Sema/SemaTemplate.cpp

hokein · 2024-06-11T10:59:56Z

clang/lib/Sema/SemaTemplate.cpp

+    //     };
+    //   };
+    if (OuterInstantiationArgs && InDependentContext) {
+      Decl = cast_if_present<TypedefNameDecl>(SemaRef.SubstDecl(


We will perform the substitution even if the typedef/alias decl doesn't have any dependent template parameter references (e.g. using Alias = S<int>.), this seems unnecessary.

A simple idea to avoid it is to perform the substitution only if the type of TypedefNameDecl is dependent.

Done. We now substitute these only if they involve any template parameters.

hokein · 2024-06-11T11:06:23Z

clang/test/SemaTemplate/nested-deduction-guides.cpp

+
+namespace GH94614 {
+
+template <class, class> struct S {};


I wonder whether it is feasible to add an ast-dump test for this issue, I find seeing and verifying the shape of AST deduction guide is clearer. However, the ast-dump doesn't seem to dump much information about the type of the function parameter decl, which is the information we want to verify.

Yeah, and I've even tried #pragma clang __debug dump param, which doesn't work either...

I was expecting we could dump the TypeLoc through some command lines, but unfortunately, this isn't the case.

hokein

Thanks, it looks much better.

zyn0217 · 2024-07-10T04:48:58Z

Thanks for the review.

…lvm#94740) Similar to the approach of handling nested class templates when building a CTAD guide, we substitute the template parameters of a type alias declaration with the instantiating template arguments in order to ensure the guide eventually doesn't reference any outer template parameters. For example, ```cpp template <class T> struct Outer { using Alias = S<T>; template <class U> struct Inner { Inner(Alias); }; }; ``` we used to retain the reference to T accidentally because the TreeTransform does nothing on type alias Decls by default. Fixes llvm#94614

zyn0217 added 3 commits June 7, 2024 18:12

[Clang] Substitute for the type aliases inside of a CTAD guide

2f60e51

Add prior 'typename's

70b754e

Give up if we exit a RecordDecl

f5a196a

zyn0217 requested review from cor3ntin, hokein and erichkeane June 7, 2024 13:13

zyn0217 marked this pull request as ready for review June 7, 2024 13:13

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 7, 2024

erichkeane requested review from mizvekov and sdkrystian June 7, 2024 13:21

hokein reviewed Jun 11, 2024

View reviewed changes

zyn0217 added 4 commits June 13, 2024 23:22

Merge branch 'main' into GH94614

02b2fbc

[Clang] Instantiate local constexpr functions eagerly

fe4a7cc

Merge branch 'main' into GH94614

9230ca3

Address review feedback

c553fd0

zyn0217 requested a review from hokein June 29, 2024 10:00

Avoid an unwanted Decl injection

927e2fe

hokein approved these changes Jul 5, 2024

View reviewed changes

Merge branch 'main' into GH94614

94750e6

zyn0217 merged commit 869ac40 into llvm:main Jul 10, 2024
5 of 7 checks passed

zyn0217 deleted the GH94614 branch July 10, 2024 04:51

EugeneZelenko mentioned this pull request Jul 12, 2024

Crash: clangd while indexing #98428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Substitute for the type aliases inside of a CTAD guide #94740

[Clang] Substitute for the type aliases inside of a CTAD guide #94740

Uh oh!

zyn0217 commented Jun 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 7, 2024

Uh oh!

hokein left a comment

Uh oh!

hokein Jun 11, 2024

Uh oh!

zyn0217 Jun 11, 2024

Uh oh!

hokein Jul 4, 2024

Uh oh!

zyn0217 Jul 4, 2024

Uh oh!

hokein Jun 11, 2024

Uh oh!

Uh oh!

hokein Jun 11, 2024

Uh oh!

zyn0217 Jun 29, 2024

Uh oh!

hokein Jun 11, 2024

Uh oh!

zyn0217 Jun 29, 2024 •

edited

Loading

Uh oh!

hokein left a comment

Uh oh!

zyn0217 commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

[Clang] Substitute for the type aliases inside of a CTAD guide #94740

[Clang] Substitute for the type aliases inside of a CTAD guide #94740

Uh oh!

Conversation

zyn0217 commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 7, 2024

Uh oh!

hokein left a comment

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!

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!

zyn0217 Jun 29, 2024 • 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!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

zyn0217 commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented Jun 7, 2024 •

edited

Loading

zyn0217 Jun 29, 2024 •

edited

Loading