Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 24, 2025
+34 −3
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32be32e
[Clang] Fix a partial ordering bug involving CTAD injected template a…
zyn0217 Jul 21, 2025
0fa71d3
Apply suggestions
zyn0217 Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions clang/lib/Sema/SemaTemplateDeduction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

QualType InstP = S.SubstType(P.getCanonicalType(), MLTAL, FTD->getLocation(),
FTD->getDeclName(), &IsIncompleteSubstitution);
if (InstP.isNull() && !IsIncompleteSubstitution)
Expand All @@ -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;
}
Expand Down
16 changes: 16 additions & 0 deletions clang/test/SemaTemplate/deduction-guide.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}