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>

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
 

src/program_builder.h

@@ -2071,6 +2071,24 @@ class ProgramBuilder {
     return create<ast::ElseStatement>(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 <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

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<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);

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) {
+TEST_F(ResolverBehaviorTest, StmtLoopEmpty_ContIfTrueBreak) {
-  auto* stmt = Loop(Block(), Block(Break()));
+  auto* stmt = Loop(Block(), Block(If(true, Block(Break()))));
   WrapInFunction(stmt);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();

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<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;
 }
 

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());

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) {}
 

src/sem/if_statement.h

@@ -67,7 +67,7 @@ class ElseStatement : public Castable<ElseStatement, CompoundStatement> {
   /// @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<ElseStatement, CompoundStatement> {
   /// @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; }