-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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.
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
[Clang] Correctly propagate type aliases' unexpanded flags up to lambda #122875
Conversation
We should have been checking desugar() for the type of the right-hand side of a typedef declaration, instead of using getCanonicalType(), which points to the end of the type alias chain.
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesWe should have been checking desugar() for the type of the right-hand side of a typedef declaration, instead of using getCanonicalType(), which points to the end of the type alias chain. Fixes #122417 Full diff: https://github.com/llvm/llvm-project/pull/122875.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 07a1a4195427d8..a5da6c9c88328c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -810,7 +810,7 @@ Bug Fixes to C++ Support
module imports in those situations. (#GH60336)
- Fix init-capture packs having a size of one before being instantiated. (#GH63677)
- Clang now preserves the unexpanded flag in a lambda transform used for pack expansion. (#GH56852), (#GH85667),
- (#GH99877).
+ (#GH99877), (#GH122417).
- Fixed a bug when diagnosing ambiguous explicit specializations of constrained member functions.
- Fixed an assertion failure when selecting a function from an overload set that includes a
specialization of a conversion function template.
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 4a3c739ecbeab8..79df5cf4f2bb56 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -8496,7 +8496,7 @@ TreeTransform<Derived>::TransformDeclStmt(DeclStmt *S) {
getSema()
.getASTContext()
.getTypeDeclType(TD)
- .getCanonicalType()
+ .getSingleStepDesugaredType(getSema().getASTContext())
->containsUnexpandedParameterPack();
if (auto *VD = dyn_cast<VarDecl>(Transformed))
diff --git a/clang/test/SemaCXX/fold_lambda_with_variadics.cpp b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
index 2257a4c2d975a8..69572bea3664a7 100644
--- a/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
+++ b/clang/test/SemaCXX/fold_lambda_with_variadics.cpp
@@ -7,6 +7,8 @@ struct identity {
using type = T;
};
+template <class> using ElementType = int;
+
template <class = void> void f() {
static_assert([]<class... Is>(Is... x) {
@@ -47,6 +49,10 @@ template <class = void> void f() {
}(), ...);
}(1, 2);
+ []<class... Is>(Is...) {
+ ([] { using T = ElementType<Is>; }(), ...);
+ }(1);
+
[](auto ...y) {
([y] { }(), ...);
}();
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd love @mizvekov to double check
@@ -810,7 +810,7 @@ Bug Fixes to C++ Support | |||
module imports in those situations. (#GH60336) | |||
- Fix init-capture packs having a size of one before being instantiated. (#GH63677) | |||
- Clang now preserves the unexpanded flag in a lambda transform used for pack expansion. (#GH56852), (#GH85667), | |||
(#GH99877). | |||
(#GH99877), (#GH122417). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with that formatting? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing wrong with that, this is just another case falling into that entry - we should have fixed this one in that PR :)
@@ -47,6 +49,10 @@ template <class = void> void f() { | |||
}(), ...); | |||
}(1, 2); | |||
|
|||
[]<class... Is>(Is...) { | |||
([] { using T = ElementType<Is>; }(), ...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity were applying the getCanonicalType
to T
and getting int
while we really wanted ElementType<Is>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly why the issue arises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM again, feel free to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is ok, but I don't think this fixes the underlying issue. There is something generally wrong with type aliases which don't use all of their parameters, and this needs more thought.
The current approach - bubbling up the containsUnexpanded flag within a transform - is admittedly not ideal. However, it's the only way to make Clang correctly handle nested packs, within lambdas. I remembered @cor3ntin once told me Richard had an idea of completely refactoring the pack model as in #9395 (comment). If implemented, we could get rid of this 'workaround'. Maybe we could explore that (probably starting from lambdas?) in the future. |
We should have been checking desugar() for the type of the right-hand side of a typedef declaration, instead of using getCanonicalType(), which points to the end of the type alias chain.
Fixes #122417