From 8250f2b850ce73c87c77277e6f136c33b9dcbeed Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 3 Feb 2022 00:12:52 +0000 Subject: [PATCH] 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 Reviewed-by: David Neto Commit-Queue: Ben Clayton --- docs/origin-trial-changes.md | 1 + src/program_builder.h | 18 +++ src/resolver/resolver.cc | 3 +- src/resolver/resolver_behavior_test.cc | 4 +- src/resolver/resolver_validation.cc | 58 +++++++++ src/resolver/validation_test.cc | 160 +++++++++++++++++++++++++ src/sem/if_statement.cc | 2 +- src/sem/if_statement.h | 7 +- 8 files changed, 248 insertions(+), 5 deletions(-) diff --git a/docs/origin-trial-changes.md b/docs/origin-trial-changes.md index 8fd00ece0c..8045445947 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 c63a5f15b6..c072b556e4 100644 --- a/src/program_builder.h +++ b/src/program_builder.h @@ -2071,6 +2071,24 @@ class ProgramBuilder { return create(nullptr, body); } + /// 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 + const ast::IfStatement* If(const Source& source, + CONDITION&& condition, + const ast::BlockStatement* body, + ELSE_STATEMENTS&&... elseStatements) { + return create( + source, Expr(std::forward(condition)), body, + ast::ElseStatementList{ + std::forward(elseStatements)...}); + } + /// Creates a ast::IfStatement with input condition, body, and optional /// variadic else statements /// @param condition the if statement condition expression diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 6ab3630cad..e5924e9561 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -965,7 +965,8 @@ sem::IfStatement* Resolver::IfStatement(const ast::IfStatement* stmt) { sem::ElseStatement* Resolver::ElseStatement(const ast::ElseStatement* stmt) { auto* sem = builder_->create( - stmt, current_compound_statement_, current_function_); + stmt, current_compound_statement_->As(), + 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 e7711635ff..bcc2a1732b 100644 --- a/src/resolver/resolver_behavior_test.cc +++ b/src/resolver/resolver_behavior_test.cc @@ -467,8 +467,8 @@ TEST_F(ResolverBehaviorTest, StmtLoopEmpty_ContEmpty_NoExit) { 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 0668bd97e1..f9dadab8c5 100644 --- a/src/resolver/resolver_validation.cc +++ b/src/resolver/resolver_validation.cc @@ -1263,6 +1263,64 @@ bool Resolver::ValidateBreakStatement(const sem::Statement* stmt) { 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()) { + auto* block_parent = block->Parent(); + auto* if_stmt = block_parent->As(); + auto* el_stmt = block_parent->As(); + 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()) { + 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 65b910867f..a7df8bfc73 100644 --- a/src/resolver/validation_test.cc +++ b/src/resolver/validation_test.cc @@ -1043,6 +1043,166 @@ TEST_F(ResolverValidationTest, Stmt_BreakInSwitch) { 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 8fdc8abc36..be01a6ecce 100644 --- a/src/sem/if_statement.cc +++ b/src/sem/if_statement.cc @@ -34,7 +34,7 @@ const ast::IfStatement* IfStatement::Declaration() const { } 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 3d927c89f0..4b60d9f484 100644 --- a/src/sem/if_statement.h +++ b/src/sem/if_statement.h @@ -67,7 +67,7 @@ class ElseStatement : public Castable { /// @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 @@ class ElseStatement : public Castable { /// @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(Statement::Parent()); + } + /// Sets the else-statement condition expression /// @param condition the else condition expression void SetCondition(const Expression* condition) { condition_ = condition; }