[clang] Fixed Constant Evaluation don't Call Destructor

[Mr-Anyone]

Within the condition statement of the for block, the destructor doesn't get when evaluating compile time constants.

resolves #139818

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vincent (Mr-Anyone)

Changes

Within the condition statement of the for block, the destructor doesn't get when evaluating compile time constants.

resolves #139818

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/AST/ExprConstant.cpp (+5-1)
• (modified) clang/test/SemaCXX/static-assert-cxx26.cpp (+35)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0c12091a90add..76c139632b31c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -623,6 +623,7 @@ Bug Fixes in This Version
 - Fix crash due to unknown references and pointer implementation and handling of
   base classes. (GH139452)
 - Fixed an assertion failure in serialization of constexpr structs containing unions. (#GH140130)
+- Fix constant evaluation in for loop not calling destructor (#GH139818)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index ca1fbdf7e652f..d8364977258a1 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -5742,8 +5742,12 @@ static EvalStmtResult EvaluateStmt(StmtResult &Result, EvalInfo &Info,
       if (FS->getCond() && !EvaluateCond(Info, FS->getConditionVariable(),
                                          FS->getCond(), Continue))
         return ESR_Failed;
-      if (!Continue)
+
+      if (!Continue) {
+        if (!IterScope.destroy())
+          return ESR_Failed;
         break;
+      }
 
       EvalStmtResult ESR = EvaluateLoopBody(Result, Info, FS->getBody());
       if (ESR != ESR_Continue) {
diff --git a/clang/test/SemaCXX/static-assert-cxx26.cpp b/clang/test/SemaCXX/static-assert-cxx26.cpp
index b53c67ee67932..7a47f2784ceb3 100644
--- a/clang/test/SemaCXX/static-assert-cxx26.cpp
+++ b/clang/test/SemaCXX/static-assert-cxx26.cpp
@@ -416,3 +416,38 @@ static_assert(
       // expected-note@-1 {{read of dereferenced one-past-the-end pointer is not allowed in a constant expression}}
 );
 }
+
+// taken from: https://github.com/llvm/llvm-project/issues/139818
+namespace GH139818{
+    struct A {
+      constexpr ~A() { ref = false; }
+      constexpr operator bool() {
+        return b;
+      }
+      bool b;
+      bool& ref;
+    };
+
+    constexpr bool f1() {
+      bool ret = true;
+      for (bool b = false; A x{b, ret}; b = true) {}
+      return ret;
+    }
+
+    static_assert(!f1());
+
+    struct Y {
+      constexpr ~Y() noexcept(false) { throw "oops"; }  // expected-error {{cannot use 'throw' with exceptions disabled}}
+                                                        // expected-note@-1 {{subexpression not valid in a constant expression}}
+      constexpr operator bool() {
+        return b;
+      }
+      bool b;
+    };
+    constexpr bool f2() {
+      for (bool b = false; Y x = {b}; b = true) {} // expected-note {{in call to 'x.~Y()'}}
+      return true;
+    }
+    static_assert(f2()); // expected-error {{static assertion expression is not an integral constant expression}}
+                         // expected-note@-1 {{in call to 'f2()'}}
+};

[Sirraide]

Er, there’s no reason for these tests to be in this file; maybe try and find a file containing tests around destructors during constant evaluation (in either test/AST or test/SemaCXX); if you can’t find one, then just make a new file and call it gh139818.cpp.

[Mr-Anyone]

I have moved the test to cxx2a-consteval.cpp is that ok? Also, thanks for reviewing.

[Sirraide]

No, that’s for the consteval keyword. Maybe just make a new file for this then.

And in that case, can you also check if this works with the new constant interpreter by adding a second RUN line with -fexperimental-new-constant-interpreter. If that also passes then that’s great, if not, then just remove that second RUN line again (you don’t have to update the new interpreter as well as part of this pr), but it’d be nice to check whether this is also a bug in the new interpreter if we’re already adding tests for this.

[Mr-Anyone]

I just moved it to gh139818.cpp in test/SemaCXX

[zyn0217]

GH139818 already reflects the source.

Suggested change

	// taken from: https://github.com/llvm/llvm-project/issues/139818
	namespace GH139818{
	namespace GH139818 {

[Mr-Anyone]

done, thanks.

[Mr-Anyone]

ping

[Mr-Anyone]

Documentation fail seems to be fixed by #142387

[Sirraide]

One comment but otherwise this LGTM.

[Sirraide]

Documentation fail seems to be fixed by #142387

Yeah, the changes to the release notes look fine, so if CI is unhappy about them then that’s for some other reason.