Skip to content
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

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jan 14, 2025

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.
@zyn0217 zyn0217 requested a review from cor3ntin January 14, 2025 09:14
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 14, 2025
@zyn0217 zyn0217 requested a review from erichkeane January 14, 2025 09:15
@llvmbot
Copy link
Member

llvmbot commented Jan 14, 2025

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/122875.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1-1)
  • (modified) clang/lib/Sema/TreeTransform.h (+1-1)
  • (modified) clang/test/SemaCXX/fold_lambda_with_variadics.cpp (+6)
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] { }(), ...);
   }();

@cor3ntin cor3ntin requested a review from mizvekov January 14, 2025 10:37
Copy link
Contributor

@cor3ntin cor3ntin left a 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).
Copy link
Contributor

@cor3ntin cor3ntin Jan 14, 2025

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? 😄

Copy link
Contributor Author

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>; }(), ...);
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor

@cor3ntin cor3ntin left a 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

Copy link
Contributor

@mizvekov mizvekov left a 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.

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jan 20, 2025

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.

@zyn0217 zyn0217 merged commit 6f0a627 into llvm:main Jan 20, 2025
12 checks passed
@zyn0217 zyn0217 deleted the 122417-lambda-expansion-flag branch January 20, 2025 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang++ 19.1.6 crash
5 participants