[Clang] Fix a partial ordering bug involving CTAD injected template arguments

[zyn0217]

The synthesized deduction guides use injected template arguments for distinguishment of explicit and implicit deduction guides.

In partial ordering, we may substitute into these injected types when checking consistency. Properly substituting them needs the instantiated class template specializations which isn't the case at that point. So instead, we check their template specialization types.

No release note because I think we want a backport, after baking it for a couple of days.

Fixes #134613

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

The synthesized deduction guides use injected template arguments for distinguishment of explicit and implicit deduction guides.

In partial ordering, we may substitute into these injected types when checking consistency. Properly substituting them needs the instantiated class template specializations which isn't the case at that point. So instead, we check their template specialization types.

No release note because I think we want a backport, after baking it for a couple of days.

Fixes #134613

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+9-3)
• (modified) clang/test/SemaTemplate/deduction-guide.cpp (+16)

diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index d09a72b71b805..f990e609d9823 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -5525,6 +5525,8 @@ static TemplateDeductionResult CheckDeductionConsistency(
   // FIXME: A substitution can be incomplete on a non-structural part of the
   // type. Use the canonical type for now, until the TemplateInstantiator can
   // deal with that.
+  if (auto *Injected = P->getAs<InjectedClassNameType>())
+    P = Injected->getInjectedSpecializationType();
   QualType InstP = S.SubstType(P.getCanonicalType(), MLTAL, FTD->getLocation(),
                                FTD->getDeclName(), &IsIncompleteSubstitution);
   if (InstP.isNull() && !IsIncompleteSubstitution)
@@ -5539,9 +5541,13 @@ static TemplateDeductionResult CheckDeductionConsistency(
   if (auto *PA = dyn_cast<PackExpansionType>(A);
       PA && !isa<PackExpansionType>(InstP))
     A = PA->getPattern();
-  if (!S.Context.hasSameType(
-          S.Context.getUnqualifiedArrayType(InstP.getNonReferenceType()),
-          S.Context.getUnqualifiedArrayType(A.getNonReferenceType())))
+  auto T1 = S.Context.getUnqualifiedArrayType(InstP.getNonReferenceType());
+  auto T2 = S.Context.getUnqualifiedArrayType(A.getNonReferenceType());
+  if (auto *Injected = T1->getAs<InjectedClassNameType>())
+    T1 = Injected->getInjectedSpecializationType();
+  if (auto *Injected = T2->getAs<InjectedClassNameType>())
+    T2 = Injected->getInjectedSpecializationType();
+  if (!S.Context.hasSameType(T1, T2))
     return TemplateDeductionResult::NonDeducedMismatch;
   return TemplateDeductionResult::Success;
 }
diff --git a/clang/test/SemaTemplate/deduction-guide.cpp b/clang/test/SemaTemplate/deduction-guide.cpp
index 0953f647426fa..f6bc6ee3673c2 100644
--- a/clang/test/SemaTemplate/deduction-guide.cpp
+++ b/clang/test/SemaTemplate/deduction-guide.cpp
@@ -966,3 +966,19 @@ Expand<Type, Invocable<>> _{};
 // CHECK-NEXT:   | `-ParmVarDecl {{.+}} 'T...' pack
 
 }
+
+namespace GH134613 {
+template <typename R> struct Foo {
+  using value_type = R;
+
+  Foo() = default;
+  Foo(Foo<Foo<R>> &&rhs) {}
+};
+
+void main() {
+  auto r1 = Foo(Foo<Foo<int>>{});
+
+  static_assert(__is_same(decltype(r1)::value_type, int));
+}
+
+}

[mizvekov]

It's not immediately clear to me these injected class template types only appear at the top level type of the parameters, that they can't appear nested within, and there is no explanation for that.

If we can't fix this at the deduction guide level, I think the next best thing would be to make sure the substitution here produces the right injected vs non-injected type to match the other side, by manipulating the current context.

[zyn0217]

I agree with you that fixing the deduction guide generation looks more optimal than current way, but that seemed to going to mess up more things, see the comment in transformFunctionProtoType:

    // We use the injected-class-name type of the primary template instead.
    // This has the convenient property that it is different from any type that
    // the user can write in a deduction-guide (because they cannot enter the
    // context of the template), so implicit deduction guides can never collide
    // with explicit ones.

So staying with injected types looks like the only option to me. (For now, we just want to restore the 19's behavior)

The bug happens when the generated CTAD guide (which is generated from a copy constructor) comes along with another deduction guide that is generated from a user-defined constructor; since the first guide uses the injected class types, the default substitution would fail, hence the failure of type comparision in consistency checking, resulting in wrong partial ordering.

Besides CTAD guides, we don't appear to suffer from the issue because we don't model user-written function parameters with InjectedClassNameType - which is also what the above comment reflects.

So to me, they can't be nested - we promised it when forming the the deduction guide.

I tried to fix it at the substitution level, but unfortunately it didn't work very well. Some regressions surfaced, and all of them are due to the inconsistencies between the types in A, DeducedArgs. That's why I tweaked types at here in the end.

I'm happy to add some comments, as long as the approach makes sense to you.

[mizvekov]

Alright, if we want a quick solution which can be back ported, that will do.

Can we enable this workaround only on the side which is the implicit deduction guide?

This would make this self-documented as a workaround, with a clear link to implicit deduction guides, and it would limit it to applying to known situations.

A comment calling this out as a workaround is also in order.

[mizvekov]

LGTM, thanks!

[mizvekov]

Yeah, the PR description says so.