[Clang][Concepts] Fix the constraint equivalence checking involving parameter packs #102131
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What's the status of that? |
|
EDIT: Not anymore. We shouldn't decouple the checking from |
zyn0217
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesWe established an instantiation scope in order for constraint equivalence checking to properly map the uninstantiated parameters. That mechanism mapped even packs to themselves. Consequently, parameter packs e.g. appearing in a function call, were not expanded. So they would end up becoming No release note as I plan to backport it to 19. Fixes #101735 Full diff: https://github.com/llvm/llvm-project/pull/102131.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index d4c9d044985e34..14a67f35a8f9f8 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -972,8 +972,15 @@ 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;
+ }
+ // Parameter packs should expand to a size-of-1 argument.
+ ScopeForParameters.MakeInstantiatedLocalArgPack(PVD);
+ ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD);
+ }
std::optional<Sema::CXXThisScopeRAII> ThisScope;
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 0142efcdc3ee86..333187b0d74ad6 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -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>;
+ } {}
+
+}
|
mizvekov
reviewed
Aug 24, 2024
clang/lib/Sema/SemaConcept.cpp
Outdated
Comment on lines
974
to
983
| 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; | ||
| } | ||
| // Parameter packs should expand to a size-of-1 argument. | ||
| ScopeForParameters.MakeInstantiatedLocalArgPack(PVD); | ||
| ScopeForParameters.InstantiatedLocalPackArg(PVD, PVD); | ||
| } |
There was a problem hiding this comment.
I think the solution itself looks good, but I think this scope is being set up at the wrong place (pre-existing issue).
So the problem here is that when we are instantiating a function template, then we will have instantiated it's parameters, and these will be in the local scope, so when we perform constraint checking, we find them, and all is good.
But when we are doing the initial template parsing, and we need to perform this constraint checking, then there is no local instantiation scope for these parameters.
So I think this is a parsing problem and this ScopeForParameters setup should be moved there.
Since this is pre-existing, I am not opposed to doing this in a follow-up patch.
There was a problem hiding this comment.
Also, this new comment should be clearer here.
We are trying to create a no-transformation scope, so in the end there is no actual pack expansion going on here: We are expanding an unexpanded pack into an expansion of one unexpanded pack, so we are back where we started and nothing was expanded. So this looks like a clever hack which avoided creating the SubstTemplateTypeParmPackType through a loophole.
Since concepts make this a valid use case, we could explore improving this abstraction, for example creating some sort of NonInstantiation scope where you just have to register which declarations are in scope, but shouldn't transform them.
There was a problem hiding this comment.
Actually, I think the better (future) approach here would be to ditch this scope and improve how FindInstantiatedDecl figures out which declarations should be part of the context being instantiated, and which are outside, so that it just naturally doesn't try to transform these declarations, just as for example it doesn't transform declarations at a template level above the context being instantiated.
There was a problem hiding this comment.
Thanks for the suggestion; I’m offline and will add more comments when I get to my computer.
IIRC, this future improvement could also benefit noexcept specifier comparison as it also requires an instantiation scope that just indicates, “We know this declaration is not instantiated; just go on and leave it an uninstantiated state”
Isolating the declaration instantiation from a TreeTransform seems to have inconvenienced other things. Maybe that is what was designed for templates in the first place, but it becomes cumbersome for cases like this and CWG2369, etc, and we need the on-demand instantiation.
cor3ntin
approved these changes
Aug 26, 2024
Comment on lines
+992
to
+995
| // 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
the only thing that should be affected is profiling.
Hmm, I see your point...
There was a problem hiding this comment.
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.
|
should we rename |
Good idea. How about |
Co-authored-by: cor3ntin <[email protected]>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
/cherry-pick e6974da |
Error: Command failed due to missing milestone. |
|
/cherry-pick e6974da |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 26, 2024
…arameter packs (llvm#102131) We established an instantiation scope in order for constraint equivalence checking to properly map the uninstantiated parameters. That mechanism mapped even packs to themselves. Consequently, parameter packs e.g. appearing in a function call, were not expanded. So they would end up becoming `SubstTemplateTypeParmPackType`s that circularly depend on the canonical declaration of the function template, which is not yet determined, hence the spurious error. No release note as I plan to backport it to 19. Fixes llvm#101735 --------- Co-authored-by: cor3ntin <[email protected]> (cherry picked from commit e6974da)
|
/pull-request #106043 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/1729 Here is the relevant piece of the build log for the reference |
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Sep 24, 2024
…arameter packs (llvm#102131) We established an instantiation scope in order for constraint equivalence checking to properly map the uninstantiated parameters. That mechanism mapped even packs to themselves. Consequently, parameter packs e.g. appearing in a function call, were not expanded. So they would end up becoming `SubstTemplateTypeParmPackType`s that circularly depend on the canonical declaration of the function template, which is not yet determined, hence the spurious error. No release note as I plan to backport it to 19. Fixes llvm#101735 --------- Co-authored-by: cor3ntin <[email protected]> (cherry picked from commit e6974da)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We established an instantiation scope in order for constraint equivalence checking to properly map the uninstantiated parameters.
That mechanism mapped even packs to themselves. Consequently, parameter packs e.g. appearing in a function call, were not expanded. So they would end up becoming
SubstTemplateTypeParmPackTypes that circularly depend on the canonical declaration of the function template, which is not yet determined, hence the spurious error.No release note as I plan to backport it to 19.
Fixes #101735