Skip to content

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

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
May 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b487ea
[Clang][Sema] Revise the transformation of CTAD parameters of nested …
zyn0217 May 9, 2024
a5a52ba
PR -> GH
zyn0217 May 10, 2024
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
25 changes: 19 additions & 6 deletions clang/lib/Sema/SemaTemplate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

if (!NewParam)
return QualType();
ParamTypes.push_back(NewParam->getType());
Expand Down
14 changes: 14 additions & 0 deletions clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 GH88142 {

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
Loading