[Clang][Sema] Revise the transformation of CTAD parameters of nested class templates #91628

zyn0217 · 2024-05-09T17:17:19Z

This fixes a regression introduced by bee78b8.

When we form a deduction guide for a constructor, basically, we do the following work:

Collect template parameters from the constructor's surrounding class template, if present.
Collect template parameters from the constructor.
Splice these template parameters together into a new template parameter list.
Turn all the references (e.g. the function parameter list) to the invented parameter list by applying a TreeTransform to the function type.

In the previous fix, we handled cases of nested class templates by substituting the "outer" template parameters (i.e. those not declared at the surrounding class template or the constructor) with the instantiating template arguments. The approach per se makes sense, but there was a flaw in the following case:

template <typename U, typename... Us> struct X {
  template <typename V> struct Y {
    template <typename T> Y(T) {}
  };

  template <typename T> Y(T) -> Y<T>;
};

X<int>::Y y(42);

While we're transforming the parameters for Y(T), we first attempt to transform all references to V and T; then, we handle the references to outer parameters U and Us using the template arguments from X<int> by transforming the same ParamDecl. However, the first step results in the reference T being <template-param-0-1> because the invented T is the last of the parameter list of the deduction guide, and what we're substituting with is a corresponding parameter pack (which is Us, though empty). Hence we're messing up the substitution.

I think we can resolve it by reversing the substitution order, which means handling outer template parameters first and then the inner parameters.

There's no release note because this is a regression in 18, and I hope we can catch up with the last release.

Fixes #88142

…class templates Fixes llvm#88142

llvmbot · 2024-05-10T02:34:46Z

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

This fixes a regression introduced by bee78b8.

When we form a deduction guide for a constructor, basically, we do the following work:

Collect template parameters from the constructor's surrounding class template, if present.
Collect template parameters from the constructor.
Splice these template parameters together into a new template parameter list.
Turn all the references (e.g. the function parameter list) to the invented parameter list by applying a TreeTransform to the function type.

In the previous fix, we handled cases of nested class templates by substituting the "outer" template parameters (i.e. those not declared at the surrounding class template or the constructor) with the instantiating template arguments. The approach per se makes sense, but there was a flaw in the following case:

template &lt;typename U, typename... Us&gt; struct X {
  template &lt;typename V&gt; struct Y {
    template &lt;typename T&gt; Y(T) {}
  };

  template &lt;typename T&gt; Y(T) -&gt; Y&lt;T&gt;;
};

X&lt;int&gt;::Y y(42);

While we're transforming the parameters for Y(T), we first attempt to transform all references to V and T; then, we handle the references to outer parameters U and Us using the template arguments from X<int> by transforming the same ParamDecl. However, the first step results in the reference T being <template-param-0-1> because the invented T is the last of the parameter list of the deduction guide, and what we're substituting with is a corresponding parameter pack (which is Us, though empty). Hence we're messing up the substitution.

I think we can resolve it by reversing the substitution order, which means handling outer template parameters first and then the inner parameters.

There's no release note because this is a regression in 18, and I hope we can catch up with the last release.

Fixes #88142

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

2 Files Affected:

(modified) clang/lib/Sema/SemaTemplate.cpp (+19-6)
(modified) clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp (+14)

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 7e57fa0696725..63c16b3fbc279 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2491,9 +2491,6 @@ struct ConvertConstructorToDeductionGuideTransform {
       Args.addOuterRetainedLevel();
     }
 
-    if (NestedPattern)
-      Args.addOuterRetainedLevels(NestedPattern->getTemplateDepth());
-
     FunctionProtoTypeLoc FPTL = CD->getTypeSourceInfo()->getTypeLoc()
                                    .getAsAdjusted<FunctionProtoTypeLoc>();
     assert(FPTL && "no prototype for constructor declaration");
@@ -2583,11 +2580,27 @@ struct ConvertConstructorToDeductionGuideTransform {
 
     //    -- The types of the function parameters are those of the constructor.
     for (auto *OldParam : TL.getParams()) {
-      ParmVarDecl *NewParam =
-          transformFunctionTypeParam(OldParam, Args, MaterializedTypedefs);
-      if (NestedPattern && NewParam)
+      ParmVarDecl *NewParam = OldParam;
+      // Given
+      //   template <class T> struct C {
+      //     template <class U> struct D {
+      //       template <class V> D(U, V);
+      //     };
+      //   };
+      // First, transform all the references to template parameters that are
+      // defined outside of the surrounding class template. That is T in the
+      // above example.
+      if (NestedPattern) {
         NewParam = transformFunctionTypeParam(NewParam, OuterInstantiationArgs,
                                               MaterializedTypedefs);
+        if (!NewParam)
+          return QualType();
+      }
+      // Then, transform all the references to template parameters that are
+      // defined at the class template and the constructor. In this example,
+      // they're U and V, respectively.
+      NewParam =
+          transformFunctionTypeParam(NewParam, Args, MaterializedTypedefs);
       if (!NewParam)
         return QualType();
       ParamTypes.push_back(NewParam->getType());
diff --git a/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp b/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
index 38b6706595a11..5428a4eab01cf 100644
--- a/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
+++ b/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
@@ -84,3 +84,17 @@ nested_init_list<int>::concept_fail nil_invalid{1, ""};
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate template ignored: substitution failure [with F = const char *]: constraints not satisfied for class template 'concept_fail' [with F = const char *]}}
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate function template not viable: requires 1 argument, but 2 were provided}}
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate function template not viable: requires 0 arguments, but 2 were provided}}
+
+namespace PR88142 {
+
+template <typename, typename...> struct X {
+  template <typename> struct Y {
+    template <typename T> Y(T) {}
+  };
+
+  template <typename T> Y(T) -> Y<T>;
+};
+
+X<int>::Y y(42);
+
+} // namespace PR88142

antangelo

These changes make sense to me, but please wait for the others to review as well.

hokein

The fix looks good to me, and thanks for the comprehensive explanation in the description.

hokein · 2024-05-10T08:14:25Z

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

@@ -84,3 +84,17 @@ nested_init_list<int>::concept_fail nil_invalid{1, ""};
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate template ignored: substitution failure [with F = const char *]: constraints not satisfied for class template 'concept_fail' [with F = const char *]}}
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate function template not viable: requires 1 argument, but 2 were provided}}
 // expected-note@#INIT_LIST_INNER_INVALID {{candidate function template not viable: requires 0 arguments, but 2 were provided}}
+
+namespace PR88142 {


nit: PR => GH

hokein · 2024-05-10T08:30:58Z

clang/lib/Sema/SemaTemplate.cpp

+      // defined at the class template and the constructor. In this example,
+      // they're U and V, respectively.
+      NewParam =
+          transformFunctionTypeParam(NewParam, Args, MaterializedTypedefs);


We have the same pattern on Line 2438-2452 in this file, where we perform a substitution of OuterInstantiationArgs on a new transformed parameter decl, I think we should probably fix it as well.

I think that's just fine, although it looks weird: those lines are merely "creating" new template parameters rather than doing type substitution. (it's applying a TemplateDeclInstantiator with the instantiating arguments - the only valuable part from these arguments is the depth we're supposed to deduct, and the type isn't something we're concerned about at that time.)

Frankly, I have to say I didn't fully understand why we have such discrepancies (use the transformTemplateParameter first and then a TemplateDeclInstantiator for nested cases). I was expecting that perhaps we could unify them in one call. I think it warrants a separate refactoring PR, then.

The TemplateDeclInstantiator is used for that substitution to avoid evaluating constraints at that point. Without this, the constraint template argument depths on NewParam would be as if the parameter were at depth 0 (instead of the nested template depth), causing a crash when evaluated later.

@antangelo : thanks for the explanation. I was considering if we can reuse the transformTemplateTypeParam stuff instead of a TemplateDeclInstantiator (we can make the constraint evaluation opt-in in that function; I presumed this would behave like what setEvaluateConstraints(false) does.), but doing that doesn't look like it would simplify the logic here significantly, so I hesitated to do so.

The other part Depth1Args also prevents us from combining these substitution into one call - I think I have to go through it further. Anyhow, this shouldn't block a backport.

zyn0217 · 2024-05-10T12:47:06Z

I'm merging it now - if something breaks and I'm not in front of my computer, feel free to revert then.

…class templates (llvm#91628) This fixes a regression introduced by bee78b8. When we form a deduction guide for a constructor, basically, we do the following work: - Collect template parameters from the constructor's surrounding class template, if present. - Collect template parameters from the constructor. - Splice these template parameters together into a new template parameter list. - Turn all the references (e.g. the function parameter list) to the invented parameter list by applying a `TreeTransform` to the function type. In the previous fix, we handled cases of nested class templates by substituting the "outer" template parameters (i.e. those not declared at the surrounding class template or the constructor) with the instantiating template arguments. The approach per se makes sense, but there was a flaw in the following case: ```cpp template <typename U, typename... Us> struct X { template <typename V> struct Y { template <typename T> Y(T) {} }; template <typename T> Y(T) -> Y<T>; }; X<int>::Y y(42); ``` While we're transforming the parameters for `Y(T)`, we first attempt to transform all references to `V` and `T`; then, we handle the references to outer parameters `U` and `Us` using the template arguments from `X<int>` by transforming the same `ParamDecl`. However, the first step results in the reference `T` being `<template-param-0-1>` because the invented `T` is the last of the parameter list of the deduction guide, and what we're substituting with is a corresponding parameter pack (which is `Us`, though empty). Hence we're messing up the substitution. I think we can resolve it by reversing the substitution order, which means handling outer template parameters first and then the inner parameters. There's no release note because this is a regression in 18, and I hope we can catch up with the last release. Fixes llvm#88142 (cherry picked from commit 8c852ab)

[Clang][Sema] Revise the transformation of CTAD parameters of nested …

4b487ea

…class templates Fixes llvm#88142

zyn0217 requested review from cor3ntin, erichkeane, antangelo and hokein May 10, 2024 02:33

zyn0217 marked this pull request as ready for review May 10, 2024 02:34

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

antangelo approved these changes May 10, 2024

View reviewed changes

hokein reviewed May 10, 2024

View reviewed changes

erichkeane approved these changes May 10, 2024

View reviewed changes

PR -> GH

a5a52ba

zyn0217 merged commit 8c852ab into llvm:main May 10, 2024
3 of 4 checks passed

zyn0217 deleted the GH88142 branch May 10, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Sema] Revise the transformation of CTAD parameters of nested class templates #91628

[Clang][Sema] Revise the transformation of CTAD parameters of nested class templates #91628

Uh oh!

zyn0217 commented May 9, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 10, 2024

Uh oh!

antangelo left a comment

Uh oh!

hokein left a comment

Uh oh!

hokein May 10, 2024

Uh oh!

hokein May 10, 2024

Uh oh!

zyn0217 May 10, 2024

Uh oh!

antangelo May 10, 2024

Uh oh!

zyn0217 May 11, 2024

Uh oh!

zyn0217 commented May 10, 2024

Uh oh!

Uh oh!

Uh oh!

[Clang][Sema] Revise the transformation of CTAD parameters of nested class templates #91628

[Clang][Sema] Revise the transformation of CTAD parameters of nested class templates #91628

Uh oh!

Conversation

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

Uh oh!

Uh oh!

llvmbot commented May 10, 2024

Uh oh!

antangelo left a comment

Choose a reason for hiding this comment

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

hokein May 10, 2024

Choose a reason for hiding this comment

Uh oh!

hokein May 10, 2024

Choose a reason for hiding this comment

Uh oh!

zyn0217 May 10, 2024

Choose a reason for hiding this comment

Uh oh!

antangelo May 10, 2024

Choose a reason for hiding this comment

Uh oh!

zyn0217 May 11, 2024

Choose a reason for hiding this comment

Uh oh!

zyn0217 commented May 10, 2024

Uh oh!

Uh oh!

Uh oh!

zyn0217 commented May 9, 2024 •

edited

Loading