Skip to content

[clang] CTAD alias: Respect explicit deduction guides defined after the first use of the alias template. #125478

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
hokein merged 3 commits into from
Feb 10, 2025
Merged

[clang] CTAD alias: Respect explicit deduction guides defined after the first use of the alias template. #125478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4af8e19
[clang] CTAD alias: Respecte explicit deduction guides defined after the
hokein Feb 3, 2025
abc8882
Address review comments.
hokein Feb 3, 2025
1750551
Add a test case for explicit template deduction guides.
hokein Feb 10, 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
37 changes: 29 additions & 8 deletions clang/lib/Sema/SemaTemplateDeductionGuide.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,28 @@ bool hasDeclaredDeductionGuides(DeclarationName Name, DeclContext *DC) {
return false;
}

// Returns all source deduction guides associated with the declared
// deduction guides that have the specified deduction guide name.
llvm::DenseSet<const NamedDecl *> getSourceDeductionGuides(DeclarationName Name,
DeclContext *DC) {
assert(Name.getNameKind() ==
DeclarationName::NameKind::CXXDeductionGuideName &&
"name must be a deduction guide name");
llvm::DenseSet<const NamedDecl *> Result;
for (auto *D : DC->lookup(Name)) {
if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(D))
D = FTD->getTemplatedDecl();

if (const auto *GD = dyn_cast<CXXDeductionGuideDecl>(D)) {
assert(GD->getSourceDeductionGuide() &&
"deduction guide for alias template must have a source deduction "
"guide");
Result.insert(GD->getSourceDeductionGuide());
Copy link
Contributor

@cor3ntin cor3ntin Feb 3, 2025

Choose a reason for hiding this comment

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

This can be null right? So we'd be inserting a bunch of nullptr into the map which is harmless but also not useful

Copy link
Collaborator Author

@hokein hokein Feb 3, 2025

Choose a reason for hiding this comment

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

Added an assertion -- this cannot be nullptr in theory. The function is only called for the alias templates, if a deduction guide exists for a type alias, then it must be generated from a source deduction guide by definition.

}
}
return Result;
}

// Build the associated constraints for the alias deduction guides.
// C++ [over.match.class.deduct]p3.3:
// The associated constraints ([temp.constr.decl]) are the conjunction of the
Expand Down Expand Up @@ -1191,17 +1213,14 @@ void DeclareImplicitDeductionGuidesForTypeAlias(
if (AliasTemplate->isInvalidDecl())
return;
auto &Context = SemaRef.Context;
// FIXME: if there is an explicit deduction guide after the first use of the
// type alias usage, we will not cover this explicit deduction guide. fix this
// case.
if (hasDeclaredDeductionGuides(
Context.DeclarationNames.getCXXDeductionGuideName(AliasTemplate),
AliasTemplate->getDeclContext()))
return;
auto [Template, AliasRhsTemplateArgs] =
getRHSTemplateDeclAndArgs(SemaRef, AliasTemplate);
if (!Template)
return;
auto SourceDeductionGuides = getSourceDeductionGuides(
Context.DeclarationNames.getCXXDeductionGuideName(AliasTemplate),
AliasTemplate->getDeclContext());

DeclarationNameInfo NameInfo(
Context.DeclarationNames.getCXXDeductionGuideName(Template), Loc);
LookupResult Guides(SemaRef, NameInfo, clang::Sema::LookupOrdinaryName);
Expand All @@ -1210,6 +1229,8 @@ void DeclareImplicitDeductionGuidesForTypeAlias(

for (auto *G : Guides) {
if (auto *DG = dyn_cast<CXXDeductionGuideDecl>(G)) {
if (SourceDeductionGuides.contains(DG))
continue;
// The deduction guide is a non-template function decl, we just clone it.
auto *FunctionType =
SemaRef.Context.getTrivialTypeSourceInfo(DG->getType());
Expand Down Expand Up @@ -1252,7 +1273,7 @@ void DeclareImplicitDeductionGuidesForTypeAlias(
continue;
}
FunctionTemplateDecl *F = dyn_cast<FunctionTemplateDecl>(G);
if (!F)
if (!F || SourceDeductionGuides.contains(F->getTemplatedDecl()))
continue;
// The **aggregate** deduction guides are handled in a different code path
// (DeclareAggregateDeductionGuideFromInitList), which involves the tricky
Expand Down
18 changes: 15 additions & 3 deletions clang/test/SemaCXX/cxx20-ctad-type-alias.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,23 @@ int i = 0;
AFoo s{i};
static_assert(__is_same(decltype(s.t), int));

template<class T>
using BFoo = AFoo<T>;
Copy link
Contributor

@antangelo antangelo Feb 3, 2025

Choose a reason for hiding this comment

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

Do we also need a test case for the templated DG case?

Copy link
Collaborator Author

@hokein hokein Feb 10, 2025

Choose a reason for hiding this comment

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

Done, added one.


// template explicit deduction guide.
template<class T>
Foo(T) -> Foo<float>;
static_assert(__is_same(decltype(AFoo(i).t), float));
static_assert(__is_same(decltype(BFoo(i).t), float));

// explicit deduction guide.
Foo(int) -> Foo<X>;
AFoo s2{i};
// FIXME: the type should be X because of the above explicit deduction guide.
static_assert(__is_same(decltype(s2.t), int));
static_assert(__is_same(decltype(AFoo(i).t), X));
static_assert(__is_same(decltype(BFoo(i).t), X));

Foo(double) -> Foo<int>;
static_assert(__is_same(decltype(AFoo(1.0).t), int));
static_assert(__is_same(decltype(BFoo(1.0).t), int));
Copy link
Collaborator

@shafik shafik Feb 4, 2025

Choose a reason for hiding this comment

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

Can we get more complicated test cases, w/ multiple template params and multiple constructor arguments etc e.g.: https://compiler-explorer.com/z/x8PhEqWeo

We should really be trying harder to stress test our implementation.

Does it fix this case: https://godbolt.org/z/b84rzhjjK if not we need to open a bug.

Copy link
Contributor

@antangelo antangelo Feb 4, 2025

Choose a reason for hiding this comment

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

Does it fix this case: https://godbolt.org/z/b84rzhjjK if not we need to open a bug.

That test case doesn't look related to these changes to me, I think it's a different issue.

Copy link
Collaborator Author

@hokein hokein Feb 5, 2025

Choose a reason for hiding this comment

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

https://compiler-explorer.com/z/x8PhEqWeo

A simpler example: https://compiler-explorer.com/z/Tdx1jd1bM

I think Clang (and MSVC) are correct in rejecting this example. The candidate deduction guide (AFoo(T t, U) -> Foo<T, U>) is not selected because the is_deducible type trait is not satisfied, as indicated in the diagnostic message.

It looks like a bug in gcc.

Copy link
Collaborator Author

@hokein hokein Feb 5, 2025

Choose a reason for hiding this comment

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

Does it fix this case: https://godbolt.org/z/b84rzhjjK if not we need to open a bug.

That test case doesn't look related to these changes to me, I think it's a different issue.

This is not related to this patch. Filed #125821 for tracking (thanks for spotting it!)

} // namespace test16

namespace test17 {
Expand Down