Skip to content

[Clang][Concepts] Fix the constraint equivalence checking involving parameter packs #102131

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 4 commits into from
Aug 26, 2024
Merged

[Clang][Concepts] Fix the constraint equivalence checking involving parameter packs #102131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa99ac4
[Clang][Concepts] Fix the constraint equivalence checking for Templat…
zyn0217 Aug 6, 2024
339ab57
Clarify the reason & add a FIXME
zyn0217 Aug 25, 2024
811bfc9
Apply Corentin's feedback
zyn0217 Aug 26, 2024
908804f
Fix comments
zyn0217 Aug 26, 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
26 changes: 24 additions & 2 deletions clang/lib/Sema/SemaConcept.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,8 +972,30 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
// equivalence.
LocalInstantiationScope ScopeForParameters(S);
if (auto *FD = DeclInfo.getDecl()->getAsFunction())
for (auto *PVD : FD->parameters())
ScopeForParameters.InstantiatedLocal(PVD, PVD);
for (auto *PVD : FD->parameters()) {
if (!PVD->isParameterPack()) {
ScopeForParameters.InstantiatedLocal(PVD, PVD);
continue;
}
// This is hacky: we're mapping the parameter pack to a size-of-1 argument
// to avoid building SubstTemplateTypeParmPackTypes for
// PackExpansionTypes. The SubstTemplateTypeParmPackType node would
// otherwise reference the AssociatedDecl of the template arguments, which
// is, in this case, the template declaration.
//
// However, as we are in the process of comparing potential
// re-declarations, the canonical declaration is the declaration itself at
// this point. So if we didn't expand these packs, we would end up with an
// incorrect profile difference because we will be profiling the
// canonical types!
//
// FIXME: Improve the "no-transform" machinery in FindInstantiatedDecl so
// that we can eliminate the Scope in the cases where the declarations are
// not necessarily instantiated. It would also benefit the noexcept
// specifier comparison.
Comment on lines +992 to +995
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could temporarily pretend that the declarations are equivalent while checking their constraint so that their canonical type matches.
That way we could just do a normal parameter substitution, I think.
Or maybe @mizvekov solution would be better.

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 wonder if we could temporarily pretend that the declarations are equivalent while checking their constraint so that their canonical type matches.

I tried that approach in 0727e47. While it works, I don't think I like it quite much because it looks unnatural/dirty to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think both approach are equally... in search of a better 3rd approach :D
I like the Decl approach because it doesn't mess too much with substitution at all, the only thing that should be affected is profiling. So I am somewhat more confident about its correctness, if that makes sense.

Any preference @mizvekov ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only thing that should be affected is profiling.

Hmm, I see your point...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the solution in this patch improves on the existing one, even if interim, as the idea is to not have TransformDecl change these inserted declarations at all, which we failed to do for packs. So I like it more, as it just adds to existing workarounds, instead of adding a new one.

ScopeForParameters.MakeInstantiatedLocalArgPack(PVD);
ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD);
}

std::optional<Sema::CXXThisScopeRAII> ThisScope;

Expand Down
23 changes: 23 additions & 0 deletions clang/test/SemaTemplate/concepts-out-of-line-def.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,3 +599,26 @@ template <class DerT>
unsigned long DerivedCollection<DerTs...>::index() {}

} // namespace GH72557

namespace GH101735 {

template <class, class>
concept True = true;

template <typename T>
class A {
template <typename... Ts>
void method(Ts&... ts)
requires requires (T t) {
{ t.method(static_cast<Ts &&>(ts)...) } -> True<void>;
};
};

template <typename T>
template <typename... Ts>
void A<T>::method(Ts&... ts)
requires requires (T t) {
{ t.method(static_cast<Ts &&>(ts)...) } -> True<void>;
} {}

}
Loading