resolver: Correctly validate 'break' inside 'continuing'
We haven't been correctly checking the esoteric set of rules around breaks in continuing statements.
Bug: chromium:1288919
Change-Id: Ica6a0e71d06d9b204c359fea5f778db2383e6fa1
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/78860
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/docs/origin-trial-changes.md b/docs/origin-trial-changes.md
index 8fd00ec..8045445 100644
--- a/docs/origin-trial-changes.md
+++ b/docs/origin-trial-changes.md
@@ -6,6 +6,7 @@
* The `@interpolate(flat)` attribute must now be specified on integral user-defined IO. [tint:1224](crbug.com/tint/1224)
* The `ignore()` intrinsic has been removed. Use phoney-assignment instead: `ignore(expr);` -> `_ = expr;`.
+* `break` statements in `continuing` blocks are now correctly validated.
## Changes for M99
diff --git a/src/program_builder.h b/src/program_builder.h
index c63a5f1..c072b55 100644
--- a/src/program_builder.h
+++ b/src/program_builder.h
@@ -2073,6 +2073,24 @@
/// Creates a ast::IfStatement with input condition, body, and optional
/// variadic else statements
+ /// @param source the source information for the if statement
+ /// @param condition the if statement condition expression
+ /// @param body the if statement body
+ /// @param elseStatements optional variadic else statements
+ /// @returns the if statement pointer
+ template <typename CONDITION, typename... ELSE_STATEMENTS>
+ const ast::IfStatement* If(const Source& source,
+ CONDITION&& condition,
+ const ast::BlockStatement* body,
+ ELSE_STATEMENTS&&... elseStatements) {
+ return create<ast::IfStatement>(
+ source, Expr(std::forward<CONDITION>(condition)), body,
+ ast::ElseStatementList{
+ std::forward<ELSE_STATEMENTS>(elseStatements)...});
+ }
+
+ /// Creates a ast::IfStatement with input condition, body, and optional
+ /// variadic else statements
/// @param condition the if statement condition expression
/// @param body the if statement body
/// @param elseStatements optional variadic else statements
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 6ab3630..e5924e9 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -965,7 +965,8 @@
sem::ElseStatement* Resolver::ElseStatement(const ast::ElseStatement* stmt) {
auto* sem = builder_->create<sem::ElseStatement>(
- stmt, current_compound_statement_, current_function_);
+ stmt, current_compound_statement_->As<sem::IfStatement>(),
+ current_function_);
return StatementScope(stmt, sem, [&] {
if (auto* cond_expr = stmt->condition) {
auto* cond = Expression(cond_expr);
diff --git a/src/resolver/resolver_behavior_test.cc b/src/resolver/resolver_behavior_test.cc
index e771163..bcc2a17 100644
--- a/src/resolver/resolver_behavior_test.cc
+++ b/src/resolver/resolver_behavior_test.cc
@@ -467,8 +467,8 @@
EXPECT_EQ(r()->error(), "12:34 error: loop does not exit");
}
-TEST_F(ResolverBehaviorTest, StmtLoopEmpty_ContBreak) {
- auto* stmt = Loop(Block(), Block(Break()));
+TEST_F(ResolverBehaviorTest, StmtLoopEmpty_ContIfTrueBreak) {
+ auto* stmt = Loop(Block(), Block(If(true, Block(Break()))));
WrapInFunction(stmt);
ASSERT_TRUE(r()->Resolve()) << r()->error();
diff --git a/src/resolver/resolver_validation.cc b/src/resolver/resolver_validation.cc
index 0668bd9..f9dadab 100644
--- a/src/resolver/resolver_validation.cc
+++ b/src/resolver/resolver_validation.cc
@@ -1263,6 +1263,64 @@
stmt->Declaration()->source);
return false;
}
+ if (auto* continuing = ClosestContinuing(/*stop_at_loop*/ true)) {
+ auto fail = [&](const char* note_msg, const Source& note_src) {
+ constexpr const char* kErrorMsg =
+ "break statement in a continuing block must be the single statement "
+ "of an if statement's true or false block, and that if statement "
+ "must be the last statement of the continuing block";
+ AddError(kErrorMsg, stmt->Declaration()->source);
+ AddNote(note_msg, note_src);
+ return false;
+ };
+
+ if (auto* block = stmt->Parent()->As<sem::BlockStatement>()) {
+ auto* block_parent = block->Parent();
+ auto* if_stmt = block_parent->As<sem::IfStatement>();
+ auto* el_stmt = block_parent->As<sem::ElseStatement>();
+ if (el_stmt) {
+ if_stmt = el_stmt->Parent();
+ }
+ if (!if_stmt) {
+ return fail("break statement is not directly in if statement block",
+ stmt->Declaration()->source);
+ }
+ if (block->Declaration()->statements.size() != 1) {
+ return fail("if statement block contains multiple statements",
+ block->Declaration()->source);
+ }
+ for (auto* el : if_stmt->Declaration()->else_statements) {
+ if (el->condition) {
+ return fail("else has condition", el->condition->source);
+ }
+ bool el_contains_break = el_stmt && el == el_stmt->Declaration();
+ if (el_contains_break) {
+ if (auto* true_block = if_stmt->Declaration()->body;
+ !true_block->Empty()) {
+ return fail("non-empty true block", true_block->source);
+ }
+ } else {
+ if (!el->body->Empty()) {
+ return fail("non-empty false block", el->body->source);
+ }
+ }
+ }
+ if (if_stmt->Parent()->Declaration() != continuing) {
+ return fail(
+ "if statement containing break statement is not directly in "
+ "continuing block",
+ if_stmt->Declaration()->source);
+ }
+ if (auto* cont_block = continuing->As<ast::BlockStatement>()) {
+ if (if_stmt->Declaration() != cont_block->Last()) {
+ return fail(
+ "if statement containing break statement is not the last "
+ "statement of the continuing block",
+ if_stmt->Declaration()->source);
+ }
+ }
+ }
+ }
return true;
}
diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc
index 65b9108..a7df8bf 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc
@@ -1043,6 +1043,166 @@
EXPECT_TRUE(r()->Resolve()) << r()->error();
}
+TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block( // if(true) {
+ Break(Source{{12, 34}})))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfElseInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block(), // if(true) {
+ Else(Block( // } else {
+ Break(Source{{12, 34}}))))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInContinuing) {
+ auto* cont = Block( // continuing {
+ Block(Break(Source{{12, 34}}))); // break;
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "12:34 note: break statement is not directly in if statement block");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfInIfInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block( // if(true) {
+ If(Source{{56, 78}}, true, // if(true) {
+ Block(Break(Source{{12, 34}})))))); // break;
+ // }
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: if statement containing break statement is not directly in "
+ "continuing block");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfTrueMultipleStmtsInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block(Source{{56, 78}}, // if(true) {
+ Assign(Phony(), 1), // _ = 1;
+ Break(Source{{12, 34}})))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: if statement block contains multiple statements");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfElseMultipleStmtsInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block(), // if(true) {
+ Else(Block(Source{{56, 78}}, // } else {
+ Assign(Phony(), 1), // _ = 1;
+ Break(Source{{12, 34}}))))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: if statement block contains multiple statements");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfElseIfInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, Block(), // if(true) {
+ Else(Expr(Source{{56, 78}}, true), // } else if (true) {
+ Block(Break(Source{{12, 34}}))))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: else has condition");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfNonEmptyElseInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, // if(true) {
+ Block(Break(Source{{12, 34}})), // break;
+ Else(Block(Source{{56, 78}}, // } else {
+ Assign(Phony(), 1))))); // _ = 1;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: non-empty false block");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfElseNonEmptyTrueInContinuing) {
+ auto* cont = Block( // continuing {
+ If(true, // if(true) {
+ Block(Source{{56, 78}}, Assign(Phony(), 1)), // _ = 1;
+ Else(Block( // } else {
+ Break(Source{{12, 34}}))))); // break;
+ // }
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: non-empty true block");
+}
+
+TEST_F(ResolverValidationTest, Stmt_BreakInIfInContinuingNotLast) {
+ auto* cont = Block( // continuing {
+ If(Source{{56, 78}}, true, // if(true) {
+ Block(Break(Source{{12, 34}}))), // break;
+ // }
+ Assign(Phony(), 1)); // _ = 1;
+ // }
+ WrapInFunction(Loop(Block(), cont));
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: break statement in a continuing block must be the single "
+ "statement of an if statement's true or false block, and that if "
+ "statement must be the last statement of the continuing block\n"
+ "56:78 note: if statement containing break statement is not the last "
+ "statement of the continuing block");
+}
+
TEST_F(ResolverValidationTest, Stmt_BreakNotInLoopOrSwitch) {
WrapInFunction(Break(Source{{12, 34}}));
EXPECT_FALSE(r()->Resolve());
diff --git a/src/sem/if_statement.cc b/src/sem/if_statement.cc
index 8fdc8ab..be01a6e 100644
--- a/src/sem/if_statement.cc
+++ b/src/sem/if_statement.cc
@@ -34,7 +34,7 @@
}
ElseStatement::ElseStatement(const ast::ElseStatement* declaration,
- const CompoundStatement* parent,
+ const IfStatement* parent,
const sem::Function* function)
: Base(declaration, parent, function) {}
diff --git a/src/sem/if_statement.h b/src/sem/if_statement.h
index 3d927c8..4b60d9f 100644
--- a/src/sem/if_statement.h
+++ b/src/sem/if_statement.h
@@ -67,7 +67,7 @@
/// @param parent the owning statement
/// @param function the owning function
ElseStatement(const ast::ElseStatement* declaration,
- const CompoundStatement* parent,
+ const IfStatement* parent,
const sem::Function* function);
/// Destructor
@@ -76,6 +76,11 @@
/// @returns the else-statement condition expression
const Expression* Condition() const { return condition_; }
+ /// @return the statement that encloses this statement
+ const IfStatement* Parent() const {
+ return static_cast<const IfStatement*>(Statement::Parent());
+ }
+
/// Sets the else-statement condition expression
/// @param condition the else condition expression
void SetCondition(const Expression* condition) { condition_ = condition; }
