[clang] Add break/continue stmts with missing lable to the AST

[tbaederr]

Fixes #156801

We can't return a RecoveryExpr here since the functions returns a StmtResult. Just add those statements to the AST without the target.

I'm not entirely sure if we should do the same thing in Sema::ActOnBreakStmt, but I think so.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Fixes #156801

We can't return a RecoveryExpr here since the functions returns a StmtResult. Just add those statements to the AST without the target.

I'm not entirely sure if we should do the same thing in Sema::ActOnBreakStmt, but I think so.

---

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

2 Files Affected:

• (modified) clang/lib/Parse/ParseStmt.cpp (+1-3)
• (modified) clang/test/SemaCXX/labeled-break-continue-constexpr.cpp (+9-1)

diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 7e73d89c2a18c..f9141cabf0e6a 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -2313,10 +2313,8 @@ StmtResult Parser::ParseBreakOrContinueStatement(bool IsContinue) {
       // TODO: Make this a compatibility/extension warning instead once the
       // syntax of this feature is finalised.
       Diag(LabelLoc, diag::err_c2y_labeled_break_continue) << IsContinue;
-    if (!Target) {
+    if (!Target)
       Diag(LabelLoc, diag::err_break_continue_label_not_found) << IsContinue;
-      return StmtError();
-    }
   }
 
   if (IsContinue)
diff --git a/clang/test/SemaCXX/labeled-break-continue-constexpr.cpp b/clang/test/SemaCXX/labeled-break-continue-constexpr.cpp
index d1b57adca91c1..cc45887ab3817 100644
--- a/clang/test/SemaCXX/labeled-break-continue-constexpr.cpp
+++ b/clang/test/SemaCXX/labeled-break-continue-constexpr.cpp
@@ -1,6 +1,5 @@
 // RUN: %clang_cc1 -fnamed-loops -std=c++23 -fsyntax-only -verify %s
 // RUN: %clang_cc1 -fnamed-loops -std=c++23 -fsyntax-only -verify %s -fexperimental-new-constant-interpreter
-// expected-no-diagnostics
 
 struct Tracker {
   bool& destroyed;
@@ -168,3 +167,12 @@ constexpr T f12() {
 }
 static_assert(f12<int>() == 93);
 static_assert(f12<unsigned>() == 93u);
+
+
+constexpr int labelNotFound() {
+  a: for (;;) {
+    break azzz; // expected-error {{'break' label does not name an enclosing loop or 'switch'}}
+  }
+  return 1;
+}
+static_assert(labelNotFound() == 1);

[Sirraide]

Can you add some more tests:

constexpr int f13() {
  a: break aa;
  return 1;
}

static_assert(f13() == 1);

constexpr int f14() {
  for (;;) { a: break aa; }
  return 1;
}

static_assert(f14() == 1);

constexpr int f15() {
  a: switch (1) {
    case 1: break aa;
  }
  return 1;
}

static_assert(f15() == 1);

constexpr int f16() {
  a: for (;;) {
    for (;;) break aa;
  }
  return 1;
}

static_assert(f16() == 1);

[Sirraide]

I'm not entirely sure if we should do the same thing in Sema::ActOnBreakStmt, but I think so.

If we’re doing this then that should be fine so long as we still drop the break/continue if were not inside a loop or switch statement, because trying to evaluate e.g. int f() { break; } would definitely crash.

[Sirraide]

because trying to evaluate e.g. int f() { break; } would definitely crash.

As would

constexpr int f() {
  switch (1) {
    case 1: continue a;
  }
  return 1;
}

[AaronBallman]

I'm not certain this is correct; we're emitting an error. I think that branch should return StmtError() rather than a valid ContinueStmt or BreakStmt. The prior branch not returning is a bit more reasonable when we have a valid target because the semantics the AST models will still be what the programmer intended. The same is not true for the case where we couldn't find the target; we could break to the nearest loop as a way to recover, but that may end up generating confusing false positives with CFG-based diagnostics, so I think a RecoveryExpr makes more sense. WDYT?

[tbaederr]

How do you create a RecoveryExpr for a statement?

[Sirraide]

I feel like the main issue here is the fact that we’re trying to constant-evaluate a function that contains an error... which is a bit questionable imo.

we could break to the nearest loop as a way to recover, but that may end up generating confusing false positives with CFG-based diagnostics

I don’t believe CFG-based diagnostics run if there was an error (at least the ones in the static analyser don’t)?

[tbaederr]

Yes, so how do we communicate that there was an error at all?

[Sirraide]

Yes, so how do we communicate that there was an error at all?

In the static analyser, we just check if there was an error anywhere in the TU (iirc it calls Sema::hasUnrecoverableErrorOccurred() or whatever it it’s called); I’m not sure we have a way of tracking this on a per-function basis at the moment but I feel like it should be possible.

[AaronBallman]

CFG::buildCFG() has no checks that anything is valid; I was thinking the check was based on whether there was an invalid bit set somewhere, but that's not the case for constructing the CFG. Instead, AnalysisBasedWarnings::IssueWarnings() looks for whether there's been any compilation errors before deciding whether to construct the CFG. So this change "works" for anyone holding the CFG that way, but anyone with custom CFG use will get odd behavior. (I didn't check how the static analyzer uses it, how clang-tidy uses it, there's plugins to consider as well.)

I feel like the main issue here is the fact that we’re trying to constant-evaluate a function that contains an error... which is a bit questionable imo.

+1, I would expect us to fail early rather than try to continue compilation in this case.

[tbaederr]

Yes, so how do we communicate that there was an error at all?

In the static analyser, we just check if there was an error anywhere in the TU (iirc it calls Sema::hasUnrecoverableErrorOccurred() or whatever it it’s called); I’m not sure we have a way of tracking this on a per-function basis at the moment but I feel like it should be possible.

Well what we have is RecoveryExpr and Expr::containsErrors, but none of those are used for broken break statements (one of the reasons being that they aren't expressions).
We can also mark declarations as invalid but that has other side-effects like not taking part in later name lookup, which causes weird diagnostics.

[Sirraide]

Well what we have is RecoveryExpr and Expr::containsErrors, but none of those are used for broken break statements (one of the reasons being that they aren't expressions).

That and they’re also just not created if -frecovery-AST isn’t passed.

We can also mark declarations as invalid but that has other side-effects like not taking part in later name lookup, which causes weird diagnostics.

Yeah, in this case it’s not really the declaration that is invalid but rather the body.

[AaronBallman]

Well what we have is RecoveryExpr and Expr::containsErrors, but none of those are used for broken break statements (one of the reasons being that they aren't expressions).

As best I can tell, we usually don't have to mark the statement as invalid because 1) either was syntactically invalid in which case we usually don't add the statement to the AST to begin with (https://godbolt.org/z/WjGYn4cGe), or 2) it's an expression that's the problem. The same is true here, just in a bit of a weird way: the label name is a kind of DeclRefExpr (notionally), so break unknown should be handled the same as if (unknown) in that we'd form the BreakStmt but mark the operand as an invalid expression (https://godbolt.org/z/xdsnWb79s). But we don't implement the feature that way, we model it after goto, both of which skip a notional DeclRefExpr and just refer to the LabelDecl being jumped to directly; maybe the way forward is to gin up an invalid LabelDecl in this case?