From d9d08aecf4df1216da00d92b813319a2a0df17e7 Mon Sep 17 00:00:00 2001 From: James Price Date: Wed, 23 Mar 2022 09:11:03 +0000 Subject: [PATCH] wgsl: make if/switch parentheses optional Fixed: tint:1424 Change-Id: Id135c74fbbba941cce7fb96970d3c23417bc14ec Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/84340 Kokoro: Kokoro Auto-Submit: James Price Reviewed-by: Ben Clayton Commit-Queue: Ben Clayton --- docs/tint/origin-trial-changes.md | 4 ++ src/tint/reader/wgsl/parser_impl.cc | 18 +++++-- .../reader/wgsl/parser_impl_error_msg_test.cc | 7 --- .../reader/wgsl/parser_impl_if_stmt_test.cc | 50 +++++++++++++------ .../wgsl/parser_impl_switch_stmt_test.cc | 33 ++++++++---- 5 files changed, 76 insertions(+), 36 deletions(-) diff --git a/docs/tint/origin-trial-changes.md b/docs/tint/origin-trial-changes.md index fb48d13101..8680fd33cc 100644 --- a/docs/tint/origin-trial-changes.md +++ b/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) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 20e8d3828e..d2150b191c 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -1650,15 +1650,18 @@ Maybe ParserImpl::variable_stmt() { } // if_stmt -// : IF paren_rhs_stmt body_stmt ( ELSE else_stmts ) ? +// : IF expression compound_stmt ( ELSE else_stmts ) ? Maybe 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 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 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 { diff --git a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc index f68c9730f6..021b266a4d 100644 --- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc +++ b/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 {} } diff --git a/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc index 8f97550c26..2a57452763 100644 --- a/src/tint/reader/wgsl/parser_impl_if_stmt_test.cc +++ b/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 = - parser("if (a == 4) { a = b; c = d; } else if(c) { d = 2; } else {}"); + 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); + EXPECT_FALSE(p->has_error()) << p->error(); + ASSERT_NE(e.value, nullptr); + + ASSERT_TRUE(e->Is()); + ASSERT_NE(e->condition, nullptr); + ASSERT_TRUE(e->condition->Is()); + 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()); + 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 diff --git a/src/tint/reader/wgsl/parser_impl_switch_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_switch_stmt_test.cc index 1d8fb2f4ec..f2e97dedd9 100644 --- a/src/tint/reader/wgsl/parser_impl_switch_stmt_test.cc +++ b/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()); + 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();