Don't mark lambda non-dependent if nested in a generic lambda. #149121
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Daniel M. Katz (katzdm) ChangesFixes #118187. An instantiated The fix here proposed is a bit ugly, but the condition that it's being bolted onto already seems like a bit of a hack, so this seems no worse for wear. Note that #89702 surfaced this change because it caused the inner lambda expression to (correctly) be considered in a constant-evaluated context. The affected check for whether to mark the inner lambda as Tested: Full diff: https://github.com/llvm/llvm-project/pull/149121.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 970825c98fec1..4c902d2ab6d83 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -963,6 +963,7 @@ Bug Fixes to C++ Support
- Fix a crash with NTTP when instantiating local class.
- Fixed a crash involving list-initialization of an empty class with a
non-empty initializer list. (#GH147949)
+- Fixed spurious diagnoses of certain nested lambda expressions. (#GH149121)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 758012f894a41..5f2b93680cc3d 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -15512,6 +15512,8 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
DC = DC->getParent();
if ((getSema().isUnevaluatedContext() ||
getSema().isConstantEvaluatedContext()) &&
+ !(dyn_cast_or_null<CXXRecordDecl>(DC->getParent()) &&
+ cast<CXXRecordDecl>(DC->getParent())->isGenericLambda()) &&
(DC->isFileContext() || !DC->getParent()->isDependentContext()))
DependencyKind = CXXRecordDecl::LDK_NeverDependent;
diff --git a/clang/test/SemaCXX/cxx2a-consteval.cpp b/clang/test/SemaCXX/cxx2a-consteval.cpp
index 1474c48cda3c1..d92529b465c6f 100644
--- a/clang/test/SemaCXX/cxx2a-consteval.cpp
+++ b/clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1319,6 +1319,15 @@ namespace GH139160{
B result = (B){10, get_value(make_struct())}; // expected-error {{initializer element is not a compile-time constant}}
// expected-error@-1 {{call to consteval function 'GH139160::get_value' is not a constant expression}}
// expected-note@-2 {{non-constexpr function 'make_struct' cannot be used in a constant expression}}
-};
+} // namespace GH139160
+
+namespace GH118187 {
+template <typename T> int t() {
+ return []<typename U> consteval {
+ return [](U v) { return v; }(123);
+ }.template operator()<int>();
+}
+int v = t<int>();
+} // namespace GH118187
|
| DC = DC->getParent(); | ||
| if ((getSema().isUnevaluatedContext() || | ||
| getSema().isConstantEvaluatedContext()) && | ||
| !(dyn_cast_or_null<CXXRecordDecl>(DC->getParent()) && |
There was a problem hiding this comment.
We'd want to use isa_and_present (or isa_and_nonnull or w/e it is called) in this case.
That said, we probably would prefer to do something like extracting above the if a const auto *RD = dyn_cast_if_present<CXXRecordDecl>(DC->getParent()); then changing the check to RD && RD->isGenericLambda()` (or perhaps BETTER, pulling that whole thing out to a separate boolean with a reasonable name, so you have a place to put a comment).
Which brings me to the last thing, I'd like a comment explaining WHY this check should be here, it looks enough out of place as to why this cannot be dependent.
zyn0217
left a comment
zyn0217
left a comment
There was a problem hiding this comment.
I apologize for the long delay. After a second thought, I think the patch should work as expected.
Can you please rebase the patch and also add a test case for #156579? The added constant evaluation context reveals another bug, where we considered lambdas non-dependent too eagerly, causing template argument deduction to occur on mismatched template depths.
cc @cor3ntin we should backport this too
Thanks for taking a look! I'll try to circle back to this in the next week or so. |
|
@katzdm ping |
🐧 Linux x64 Test Results
|
Thanks! FYI for any other reviewers, I don't have merge access - would appreciate somebody helping out, if it looks good :-) |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/18428 Here is the relevant piece of the build log for the reference |
…149121) Fixes llvm#118187 Fixes llvm#156579 An instantiated `LambdaExpr` can currently be marked as `LDK_NeverDependent` if it's nested within a generic lambda. If that `LambdaExpr` in fact depends on template parameters introduced by the enclosing generic lambda, then its dependence will be misreported as "never dependent" and spurious diagnostics can result. The fix here proposed is a bit ugly, but the condition that it's being bolted onto already seems like a bit of a hack, so this seems no worse for wear. Note that llvm#89702 surfaced this change because it caused the inner lambda expression to (correctly) be considered in a constant-evaluated context. The affected check for whether to mark the inner lambda as `LDK_NeverDependent` therefore started to apply, whereas it didn't before. **Tested**: `check-clang` and `check-cxx`.
…149121) Fixes llvm#118187 Fixes llvm#156579 An instantiated `LambdaExpr` can currently be marked as `LDK_NeverDependent` if it's nested within a generic lambda. If that `LambdaExpr` in fact depends on template parameters introduced by the enclosing generic lambda, then its dependence will be misreported as "never dependent" and spurious diagnostics can result. The fix here proposed is a bit ugly, but the condition that it's being bolted onto already seems like a bit of a hack, so this seems no worse for wear. Note that llvm#89702 surfaced this change because it caused the inner lambda expression to (correctly) be considered in a constant-evaluated context. The affected check for whether to mark the inner lambda as `LDK_NeverDependent` therefore started to apply, whereas it didn't before. **Tested**: `check-clang` and `check-cxx`.
…149121) Fixes llvm#118187 Fixes llvm#156579 An instantiated `LambdaExpr` can currently be marked as `LDK_NeverDependent` if it's nested within a generic lambda. If that `LambdaExpr` in fact depends on template parameters introduced by the enclosing generic lambda, then its dependence will be misreported as "never dependent" and spurious diagnostics can result. The fix here proposed is a bit ugly, but the condition that it's being bolted onto already seems like a bit of a hack, so this seems no worse for wear. Note that llvm#89702 surfaced this change because it caused the inner lambda expression to (correctly) be considered in a constant-evaluated context. The affected check for whether to mark the inner lambda as `LDK_NeverDependent` therefore started to apply, whereas it didn't before. **Tested**: `check-clang` and `check-cxx`.
7 participants
Fixes #118187
Fixes #156579
An instantiated
LambdaExprcan currently be marked asLDK_NeverDependentif it's nested within a generic lambda. If thatLambdaExprin fact depends on template parameters introduced by the enclosing generic lambda, then its dependence will be misreported as "never dependent" and spurious diagnostics can result.The fix here proposed is a bit ugly, but the condition that it's being bolted onto already seems like a bit of a hack, so this seems no worse for wear.
Note that #89702 surfaced this change because it caused the inner lambda expression to (correctly) be considered in a constant-evaluated context. The affected check for whether to mark the inner lambda as
LDK_NeverDependenttherefore started to apply, whereas it didn't before.Tested:
check-clangandcheck-cxx.