wgsl: make if/switch parentheses optional

Fixed: tint:1424
Change-Id: Id135c74fbbba941cce7fb96970d3c23417bc14ec
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/84340
Kokoro: Kokoro <noreply+kokoro@google.com>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

docs/tint/origin-trial-changes.md

@@ -2,6 +2,10 @@
 
 ## Changes for M102
 
+### New Features
+
+* Parentheses are no longer required around expressions for if and switch statements [tint:1424](crbug.com/tint/1424)
+
 ### Breaking changes
 
 * The `@block` attribute has been removed. [tint:1324](crbug.com/tint/1324)

src/tint/reader/wgsl/parser_impl.cc

@@ -1650,15 +1650,18 @@ Maybe<const ast::VariableDeclStatement*> ParserImpl::variable_stmt() {
 }
 
 // if_stmt
-//   : IF paren_rhs_stmt body_stmt ( ELSE else_stmts ) ?
+//   : IF expression compound_stmt ( ELSE else_stmts ) ?
 Maybe<const ast::IfStatement*> ParserImpl::if_stmt() {
   Source source;
   if (!match(Token::Type::kIf, &source))
     return Failure::kNoMatch;
 
-  auto condition = expect_paren_rhs_stmt();
+  auto condition = logical_or_expression();
   if (condition.errored)
     return Failure::kErrored;
+  if (!condition.matched) {
+    return add_error(peek(), "unable to parse condition expression");
+  }
 
   auto body = expect_body_stmt();
   if (body.errored)
@@ -1690,10 +1693,14 @@ Expect<ast::ElseStatementList> ParserImpl::else_stmts() {
 
     const ast::Expression* cond = nullptr;
     if (else_if) {
-      auto condition = expect_paren_rhs_stmt();
+      auto condition = logical_or_expression();
       if (condition.errored) {
         return Failure::kErrored;
       }
+      if (!condition.matched) {
+        return add_error(peek(), "unable to parse condition expression");
+      }
+
       cond = condition.value;
     }
 
@@ -1716,9 +1723,12 @@ Maybe<const ast::SwitchStatement*> ParserImpl::switch_stmt() {
   if (!match(Token::Type::kSwitch, &source))
     return Failure::kNoMatch;
 
-  auto condition = expect_paren_rhs_stmt();
+  auto condition = logical_or_expression();
   if (condition.errored)
     return Failure::kErrored;
+  if (!condition.matched) {
+    return add_error(peek(), "unable to parse selector expression");
+  }
 
   auto body = expect_brace_block("switch statement",
                                  [&]() -> Expect<ast::CaseStatementList> {

src/tint/reader/wgsl/parser_impl_error_msg_test.cc

@@ -1033,13 +1033,6 @@ var i : vec3<1>;
 )");
 }
 
-TEST_F(ParserImplErrorTest, IfStmtMissingLParen) {
-  EXPECT("fn f() { if true) {} }", R"(test.wgsl:1:13 error: expected '('
-fn f() { if true) {} }
-            ^^^^
-)");
-}
-
 TEST_F(ParserImplErrorTest, IfStmtMissingRParen) {
   EXPECT("fn f() { if (true {} }", R"(test.wgsl:1:19 error: expected ')'
 fn f() { if (true {} }

src/tint/reader/wgsl/parser_impl_if_stmt_test.cc

@@ -20,7 +20,7 @@ namespace wgsl {
 namespace {
 
 TEST_F(ParserImplTest, IfStmt) {
-  auto p = parser("if (a == 4) { a = b; c = d; }");
+  auto p = parser("if a == 4 { a = b; c = d; }");
   auto e = p->if_stmt();
   EXPECT_TRUE(e.matched);
   EXPECT_FALSE(e.errored);
@@ -35,8 +35,30 @@ TEST_F(ParserImplTest, IfStmt) {
 }
 
 TEST_F(ParserImplTest, IfStmt_WithElse) {
-  auto p =
+  auto p = parser("if a == 4 { a = b; c = d; } else if(c) { d = 2; } else {}");
-      parser("if (a == 4) { a = b; c = d; } else if(c) { d = 2; } else {}");
+  auto e = p->if_stmt();
+  EXPECT_TRUE(e.matched);
+  EXPECT_FALSE(e.errored);
+  EXPECT_FALSE(p->has_error()) << p->error();
+  ASSERT_NE(e.value, nullptr);
+
+  ASSERT_TRUE(e->Is<ast::IfStatement>());
+  ASSERT_NE(e->condition, nullptr);
+  ASSERT_TRUE(e->condition->Is<ast::BinaryExpression>());
+  EXPECT_EQ(e->body->statements.size(), 2u);
+
+  ASSERT_EQ(e->else_statements.size(), 2u);
+  ASSERT_NE(e->else_statements[0]->condition, nullptr);
+  ASSERT_TRUE(
+      e->else_statements[0]->condition->Is<ast::IdentifierExpression>());
+  EXPECT_EQ(e->else_statements[0]->body->statements.size(), 1u);
+
+  ASSERT_EQ(e->else_statements[1]->condition, nullptr);
+  EXPECT_EQ(e->else_statements[1]->body->statements.size(), 0u);
+}
+
+TEST_F(ParserImplTest, IfStmt_WithElse_WithParens) {
+  auto p = parser("if(a==4) { a = b; c = d; } else if(c) { d = 2; } else {}");
   auto e = p->if_stmt();
   EXPECT_TRUE(e.matched);
   EXPECT_FALSE(e.errored);
@@ -59,13 +81,13 @@ TEST_F(ParserImplTest, IfStmt_WithElse) {
 }
 
 TEST_F(ParserImplTest, IfStmt_InvalidCondition) {
-  auto p = parser("if (a = 3) {}");
+  auto p = parser("if a = 3 {}");
   auto e = p->if_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:7: expected ')'");
+  EXPECT_EQ(p->error(), "1:6: expected '{'");
 }
 
 TEST_F(ParserImplTest, IfStmt_MissingCondition) {
@@ -75,47 +97,47 @@ TEST_F(ParserImplTest, IfStmt_MissingCondition) {
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:4: expected '('");
+  EXPECT_EQ(p->error(), "1:4: unable to parse condition expression");
 }
 
 TEST_F(ParserImplTest, IfStmt_InvalidBody) {
-  auto p = parser("if (a) { fn main() {}}");
+  auto p = parser("if a { fn main() {}}");
   auto e = p->if_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:10: expected '}'");
+  EXPECT_EQ(p->error(), "1:8: expected '}'");
 }
 
 TEST_F(ParserImplTest, IfStmt_MissingBody) {
-  auto p = parser("if (a)");
+  auto p = parser("if a");
   auto e = p->if_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:7: expected '{'");
+  EXPECT_EQ(p->error(), "1:5: expected '{'");
 }
 
 TEST_F(ParserImplTest, IfStmt_InvalidElseif) {
-  auto p = parser("if (a) {} else if (a) { fn main() -> a{}}");
+  auto p = parser("if a {} else if a { fn main() -> a{}}");
   auto e = p->if_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:25: expected '}'");
+  EXPECT_EQ(p->error(), "1:21: expected '}'");
 }
 
 TEST_F(ParserImplTest, IfStmt_InvalidElse) {
-  auto p = parser("if (a) {} else { fn main() -> a{}}");
+  auto p = parser("if a {} else { fn main() -> a{}}");
   auto e = p->if_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:18: expected '}'");
+  EXPECT_EQ(p->error(), "1:16: expected '}'");
 }
 
 }  // namespace

src/tint/reader/wgsl/parser_impl_switch_stmt_test.cc

@@ -20,7 +20,7 @@ namespace wgsl {
 namespace {
 
 TEST_F(ParserImplTest, SwitchStmt_WithoutDefault) {
-  auto p = parser(R"(switch(a) {
+  auto p = parser(R"(switch a {
   case 1: {}
   case 2: {}
 })");
@@ -36,7 +36,7 @@ TEST_F(ParserImplTest, SwitchStmt_WithoutDefault) {
 }
 
 TEST_F(ParserImplTest, SwitchStmt_Empty) {
-  auto p = parser("switch(a) { }");
+  auto p = parser("switch a { }");
   auto e = p->switch_stmt();
   EXPECT_TRUE(e.matched);
   EXPECT_FALSE(e.errored);
@@ -47,7 +47,7 @@ TEST_F(ParserImplTest, SwitchStmt_Empty) {
 }
 
 TEST_F(ParserImplTest, SwitchStmt_DefaultInMiddle) {
-  auto p = parser(R"(switch(a) {
+  auto p = parser(R"(switch a {
   case 1: {}
   default: {}
   case 2: {}
@@ -65,14 +65,25 @@ TEST_F(ParserImplTest, SwitchStmt_DefaultInMiddle) {
   ASSERT_FALSE(e->body[2]->IsDefault());
 }
 
+TEST_F(ParserImplTest, SwitchStmt_WithParens) {
+  auto p = parser("switch(a+b) { }");
+  auto e = p->switch_stmt();
+  EXPECT_TRUE(e.matched);
+  EXPECT_FALSE(e.errored);
+  EXPECT_FALSE(p->has_error()) << p->error();
+  ASSERT_NE(e.value, nullptr);
+  ASSERT_TRUE(e->Is<ast::SwitchStatement>());
+  ASSERT_EQ(e->body.size(), 0u);
+}
+
 TEST_F(ParserImplTest, SwitchStmt_InvalidExpression) {
-  auto p = parser("switch(a=b) {}");
+  auto p = parser("switch a=b {}");
   auto e = p->switch_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:9: expected ')'");
+  EXPECT_EQ(p->error(), "1:9: expected '{' for switch statement");
 }
 
 TEST_F(ParserImplTest, SwitchStmt_MissingExpression) {
@@ -82,31 +93,31 @@ TEST_F(ParserImplTest, SwitchStmt_MissingExpression) {
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:8: expected '('");
+  EXPECT_EQ(p->error(), "1:8: unable to parse selector expression");
 }
 
 TEST_F(ParserImplTest, SwitchStmt_MissingBracketLeft) {
-  auto p = parser("switch(a) }");
+  auto p = parser("switch a }");
   auto e = p->switch_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:11: expected '{' for switch statement");
+  EXPECT_EQ(p->error(), "1:10: expected '{' for switch statement");
 }
 
 TEST_F(ParserImplTest, SwitchStmt_MissingBracketRight) {
-  auto p = parser("switch(a) {");
+  auto p = parser("switch a {");
   auto e = p->switch_stmt();
   EXPECT_FALSE(e.matched);
   EXPECT_TRUE(e.errored);
   EXPECT_EQ(e.value, nullptr);
   EXPECT_TRUE(p->has_error());
-  EXPECT_EQ(p->error(), "1:12: expected '}' for switch statement");
+  EXPECT_EQ(p->error(), "1:11: expected '}' for switch statement");
 }
 
 TEST_F(ParserImplTest, SwitchStmt_InvalidBody) {
-  auto p = parser(R"(switch(a) {
+  auto p = parser(R"(switch a {
   case: {}
 })");
   auto e = p->switch_stmt();