Skip to content

Revert "[Clang] Instantiate local constexpr functions eagerly (#95660)" #98991

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

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jul 16, 2024

Unfortunately, #95660 has caused a regression in DeduceReturnType(), which is a problem in that some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this adds an offending case to the test.

Closes #98526

…5660)"

Unfortunately, this has caused a regression in DeduceReturnType(), which
causes a problem in that some of the local recursive lambdas are incorrectly
rejected.

This reverts commit 5548ea3. Also,
this adds an offending case to the test.
@zyn0217 zyn0217 requested a review from cor3ntin July 16, 2024 05:09
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Unfortunately, this has caused a regression in DeduceReturnType(), which causes a problem in that some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this adds an offending case to the test.

Closes #98526


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (-1)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+6-7)
  • (modified) clang/test/SemaTemplate/instantiate-local-class.cpp (+20-13)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 838cb69f647ee..077063a828cbb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -960,7 +960,6 @@ Bug Fixes to C++ Support
 - Fixed several bugs in capturing variables within unevaluated contexts. (#GH63845), (#GH67260), (#GH69307),
   (#GH88081), (#GH89496), (#GH90669) and (#GH91633).
 - Fixed handling of brace ellison when building deduction guides. (#GH64625), (#GH83368).
-- Clang now instantiates local constexpr functions eagerly for constant evaluators. (#GH35052), (#GH94849)
 - Fixed a failed assertion when attempting to convert an integer representing the difference
   between the addresses of two labels (a GNU extension) to a pointer within a constant expression. (#GH95366).
 - Fix immediate escalation bugs in the presence of dependent call arguments. (#GH94935)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 852344d895ffd..9723de6f46fc7 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -17938,17 +17938,16 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
 
         if (FirstInstantiation || TSK != TSK_ImplicitInstantiation ||
             Func->isConstexpr()) {
-          if (Func->isConstexpr())
+          if (isa<CXXRecordDecl>(Func->getDeclContext()) &&
+              cast<CXXRecordDecl>(Func->getDeclContext())->isLocalClass() &&
+              CodeSynthesisContexts.size())
+            PendingLocalImplicitInstantiations.push_back(
+                std::make_pair(Func, PointOfInstantiation));
+          else if (Func->isConstexpr())
             // Do not defer instantiations of constexpr functions, to avoid the
             // expression evaluator needing to call back into Sema if it sees a
             // call to such a function.
             InstantiateFunctionDefinition(PointOfInstantiation, Func);
-          else if (isa<CXXRecordDecl>(Func->getDeclContext()) &&
-                   cast<CXXRecordDecl>(Func->getDeclContext())
-                       ->isLocalClass() &&
-                   CodeSynthesisContexts.size())
-            PendingLocalImplicitInstantiations.push_back(
-                std::make_pair(Func, PointOfInstantiation));
           else {
             Func->setInstantiationIsPending(true);
             PendingInstantiations.push_back(
diff --git a/clang/test/SemaTemplate/instantiate-local-class.cpp b/clang/test/SemaTemplate/instantiate-local-class.cpp
index 7eee131e28d60..a313bdd9d2ea5 100644
--- a/clang/test/SemaTemplate/instantiate-local-class.cpp
+++ b/clang/test/SemaTemplate/instantiate-local-class.cpp
@@ -512,24 +512,31 @@ namespace LambdaInDefaultMemberInitializer {
 }
 
 #if __cplusplus >= 201703L
-namespace GH35052 {
 
-template <typename F> constexpr int func(F f) {
-  if constexpr (f(1UL)) {
-    return 1;
+namespace local_recursive_lambda {
+
+template <typename F> struct recursive_lambda {
+  template <typename... Args> auto operator()(Args &&...args) const {
+    return fn(*this, args...);
   }
-  return 0;
-}
+  F fn;
+};
 
-int main() {
-  auto predicate = [](auto v) /*implicit constexpr*/ -> bool {
-    return v == 1;
-  };
+template <typename F> recursive_lambda(F) -> recursive_lambda<F>;
+
+struct Tree {
+  Tree *left, *right;
+};
+
+int sumSize(Tree *tree) {
+  auto accumulate =
+      recursive_lambda{[&](auto &self_fn, Tree *element_node) -> int {
+        return 1 + self_fn(tree->left) + self_fn(tree->right);
+      }};
 
-  static_assert(predicate(1));
-  return func(predicate);
+  return accumulate(tree);
 }
 
-} // namespace GH35052
+} // namespace local_recursive_lambda
 
 #endif

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.

This seems like the best course of action for now... sorry!
Thanks for the PR and the work on these issues

@zyn0217 zyn0217 merged commit 862715e into llvm:main Jul 16, 2024
5 of 7 checks passed
@zyn0217 zyn0217 deleted the revert-95660 branch July 16, 2024 11:07
ahatanaka pushed a commit to swiftlang/llvm-project that referenced this pull request Jul 18, 2024
…5660)" (llvm#98991)

Unfortunately, llvm#95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes llvm#98526

(cherry picked from commit 862715e)
ahatanaka pushed a commit to swiftlang/llvm-project that referenced this pull request Jul 18, 2024
…5660)" (llvm#98991)

Unfortunately, llvm#95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes llvm#98526

(cherry picked from commit 862715e)
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…" (#98991)

Summary:
Unfortunately, #95660 has caused a regression in DeduceReturnType(),
where some of the local recursive lambdas are getting incorrectly rejected.

This reverts commit 5548ea3. Also, this
adds an offending case to the test.

Closes #98526

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251570
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 regression: "error: function 'operator()' with deduced return type cannot be used before it is defined"
3 participants