[clang][Sema] Fix the continue and break scope for while loops

[ojhunt]

Make sure we don't push the break and continue scope for a while loop until after we have evaluated the condition.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

Changes

Make sure we don't push the break and continue scope for a while loop until after we have evaluated the condition.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Parse/ParseStmt.cpp (+2-1)
• (added) clang/test/Sema/while-loop-condition-scope.c (+11)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0e9fcaa5fac6a..4062c4b7a6fdb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -161,6 +161,8 @@ Bug Fixes in This Version
   targets that treat ``_Float16``/``__fp16`` as native scalar types. Previously
   the warning was silently lost because the operands differed only by an implicit
   cast chain. (#GH149967).
+- Correct the continue and break scope for while statements to be after the
+  condition is evaluated.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index bf1978c22ee9f..b1b700951231f 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -1734,7 +1734,6 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
                  Scope::DeclScope  | Scope::ControlScope;
   else
     ScopeFlags = Scope::BreakScope | Scope::ContinueScope;
-  ParseScope WhileScope(this, ScopeFlags);
 
   // Parse the condition.
   Sema::ConditionResult Cond;
@@ -1744,6 +1743,8 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) {
                                 Sema::ConditionKind::Boolean, LParen, RParen))
     return StmtError();
 
+  ParseScope WhileScope(this, ScopeFlags);
+
   // OpenACC Restricts a while-loop inside of certain construct/clause
   // combinations, so diagnose that here in OpenACC mode.
   SemaOpenACC::LoopInConstructRAII LCR{getActions().OpenACC()};
diff --git a/clang/test/Sema/while-loop-condition-scope.c b/clang/test/Sema/while-loop-condition-scope.c
new file mode 100644
index 0000000000000..d87362bdc668d
--- /dev/null
+++ b/clang/test/Sema/while-loop-condition-scope.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s 
+
+void f() {
+  while (({ continue; 1; })) {
+    // expected-error@-1 {{'continue' statement not in loop statement}}
+
+  }
+  while (({ break; 1; })) {
+    // expected-error@-1 {{'break' statement not in loop or switch statement}}
+  }
+}

[Sirraide]

The main thing I’m still not sure about since I noticed that we allow this is whether it’s a bug or a ‘feature’ (I could imagine some horrible macros maybe making use of this but there ought to be better alternatives...), because we also allow this for for and do while loops, and at least in the case of the former it’s quite intentional apparently: a875721

If it is intentional for the other loops too we should definitely document that though because it’s rather odd out of context...

CC @zygoloid: Also, the commit above cites GCC compatibility as a reason for allowing this in for loops, but I can’t seem to find a GCC version on godbolt that actually accepts this or any of our test cases that that commit added. So unless I’m missing something, it doesn’t seem like GCC ever actually supported this?

[zygoloid]

It looks like GCC changed their behavior in version 9 onwards. In prior versions, a continue in the condition or increment of a for loop (or the condition of a while) would branch to the continue block of that for (or while) loop. But, only in C++ (in C it branches to the continue block of the outer loop), and only if there is an outer loop (otherwise it gets rejected early).

Prior to C++11, various major libraries (including both boost and Qt, as I recall) provided foreach macros that relied on this behavior, so Clang had to follow it. And instead of following it only in C++ and only when there's an enclosing loop, we chose to behave more consistently and allow it in both C and C++, regardless of whether there's an enclosing loop.

It looks like GCC 9 onwards finally converged on the more sensible behavior -- that break and continue in a loop increment / condition are not in the scope of that loop. I guess we should follow suit, but this is a breaking change for code that old GCC and Clang supported.

[Sirraide]

But, only in C++ (in C it branches to the continue block of the outer loop), and only if there is an outer loop (otherwise it gets rejected early).

Ah, I didn’t think that having an outer loop mattered; that’s why I couldn’t get it to work.

[Sirraide]

I guess we should follow suit, but this is a breaking change for code that old GCC and Clang supported.

I’d say we should at least try to get rid of it; considering that GCC doesn’t support it either anymore it’s probably fine.

@ojhunt It’d be nice to also remove support for this from do while and for loops, though iirc it might be a bit more work in case of the latter...