From e80887d14cbf44a984f84052723b96dcd27357e1 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 29 Apr 2021 15:49:44 +0000 Subject: [PATCH] reader/wgsl: Handle parentheses inside expect_argument_expression_list This simplifies the callsites, which were previously each having to handle the "empty list" case (and soon: trailing commas). This is also a better match for the grammar rules in the WGSL spec. Change-Id: I88ed54f94964f7b23a0fd9b584659037abb567ff Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/49465 Commit-Queue: James Price Auto-Submit: James Price Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/reader/wgsl/parser_impl.cc | 116 +++++++----------- src/reader/wgsl/parser_impl.h | 4 +- ...rser_impl_argument_expression_list_test.cc | 47 +++++-- src/reader/wgsl/parser_impl_call_stmt_test.cc | 4 +- src/reader/wgsl/parser_impl_error_msg_test.cc | 13 +- .../parser_impl_singular_expression_test.cc | 4 +- 6 files changed, 93 insertions(+), 95 deletions(-) diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc index 7bbad45c9a..874e05b4f2 100644 --- a/src/reader/wgsl/parser_impl.cc +++ b/src/reader/wgsl/parser_impl.cc @@ -2131,31 +2131,20 @@ Maybe ParserImpl::for_stmt() { } // func_call_stmt -// : IDENT PAREN_LEFT argument_expression_list* PAREN_RIGHT +// : IDENT argument_expression_list Maybe ParserImpl::func_call_stmt() { auto t = peek(); auto t2 = peek(1); if (!t.IsIdentifier() || !t2.IsParenLeft()) return Failure::kNoMatch; + next(); // Consume the first peek + auto source = t.source(); - - next(); // Consume the peek - next(); // Consume the 2nd peek - auto name = t.to_str(); - ast::ExpressionList params; - - t = peek(); - if (!t.IsParenRight() && !t.IsEof()) { - auto list = expect_argument_expression_list(); - if (list.errored) - return Failure::kErrored; - params = std::move(list.value); - } - - if (!expect("call statement", Token::Type::kParenRight)) + auto params = expect_argument_expression_list("function call"); + if (params.errored) return Failure::kErrored; return create( @@ -2163,7 +2152,7 @@ Maybe ParserImpl::func_call_stmt() { source, create( source, builder_.Symbols().Register(name)), - std::move(params))); + std::move(params.value))); } // break_stmt @@ -2197,7 +2186,7 @@ Maybe ParserImpl::continuing_stmt() { // primary_expression // : IDENT argument_expression_list? -// | type_decl PAREN_LEFT argument_expression_list* PAREN_RIGHT +// | type_decl argument_expression_list // | const_literal // | paren_rhs_stmt // | BITCAST LESS_THAN type_decl GREATER_THAN paren_rhs_stmt @@ -2239,23 +2228,13 @@ Maybe ParserImpl::primary_expression() { auto* ident = create( t.source(), builder_.Symbols().Register(t.to_str())); - if (match(Token::Type::kParenLeft, &source)) { - return sync(Token::Type::kParenRight, [&]() -> Maybe { - ast::ExpressionList params; + if (peek().IsParenLeft()) { + auto params = expect_argument_expression_list("function call"); + if (params.errored) + return Failure::kErrored; - auto t2 = peek(); - if (!t2.IsParenRight() && !t2.IsEof()) { - auto list = expect_argument_expression_list(); - if (list.errored) - return Failure::kErrored; - params = list.value; - } - - if (!expect("call expression", Token::Type::kParenRight)) - return Failure::kErrored; - - return create(source, ident, params); - }); + return create(source, ident, + std::move(params.value)); } return ident; @@ -2265,25 +2244,12 @@ Maybe ParserImpl::primary_expression() { if (type.errored) return Failure::kErrored; if (type.matched) { - auto expr = expect_paren_block( - "type constructor", [&]() -> Expect { - t = peek(); - if (t.IsParenRight() || t.IsEof()) - return create( - source, type.value, ast::ExpressionList{}); - - auto params = expect_argument_expression_list(); - if (params.errored) - return Failure::kErrored; - - return create(source, type.value, - params.value); - }); - - if (expr.errored) + auto params = expect_argument_expression_list("type constructor"); + if (params.errored) return Failure::kErrored; - return expr.value; + return create(source, type.value, + std::move(params.value)); } return Failure::kNoMatch; @@ -2339,28 +2305,34 @@ Maybe ParserImpl::singular_expression() { } // argument_expression_list -// : (logical_or_expression COMMA)* logical_or_expression -Expect ParserImpl::expect_argument_expression_list() { - auto arg = logical_or_expression(); - if (arg.errored) - return Failure::kErrored; - if (!arg.matched) - return add_error(peek(), "unable to parse argument expression"); - - ast::ExpressionList ret; - ret.push_back(arg.value); - - while (match(Token::Type::kComma)) { - arg = logical_or_expression(); - if (arg.errored) - return Failure::kErrored; - if (!arg.matched) { - return add_error(peek(), - "unable to parse argument expression after comma"); +// : PAREN_LEFT (logical_or_expression COMMA)* logical_or_expression +// PAREN_RIGHT +Expect ParserImpl::expect_argument_expression_list( + const std::string& use) { + return expect_paren_block(use, [&]() -> Expect { + // Check for empty list. + // TODO(crbug.com/tint/739): Remove this (handled by !arg.matched). + if (peek().IsParenRight()) { + return ast::ExpressionList{}; } - ret.push_back(arg.value); - } - return ret; + + ast::ExpressionList ret; + while (synchronized_) { + auto arg = logical_or_expression(); + if (arg.errored) + return Failure::kErrored; + if (!arg.matched) { + // TODO(crbug.com/tint/739): remove error to allow trailing commas. + return add_error(peek(), "unable to parse argument expression"); + } + ret.push_back(arg.value); + + if (!match(Token::Type::kComma)) { + break; + } + } + return ret; + }); } // unary_expression diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h index 49a2d2699b..a18550c390 100644 --- a/src/reader/wgsl/parser_impl.h +++ b/src/reader/wgsl/parser_impl.h @@ -552,8 +552,10 @@ class ParserImpl { Maybe primary_expression(); /// Parses a `argument_expression_list` grammar element, erroring on parse /// failure. + /// @param use a description of what was being parsed if an error was raised /// @returns the list of arguments - Expect expect_argument_expression_list(); + Expect expect_argument_expression_list( + const std::string& use); /// Parses the recursive portion of the postfix_expression /// @param prefix the left side of the expression /// @returns the parsed expression or nullptr diff --git a/src/reader/wgsl/parser_impl_argument_expression_list_test.cc b/src/reader/wgsl/parser_impl_argument_expression_list_test.cc index 926abc023a..ca8ed6c259 100644 --- a/src/reader/wgsl/parser_impl_argument_expression_list_test.cc +++ b/src/reader/wgsl/parser_impl_argument_expression_list_test.cc @@ -20,8 +20,8 @@ namespace wgsl { namespace { TEST_F(ParserImplTest, ArgumentExpressionList_Parses) { - auto p = parser("a"); - auto e = p->expect_argument_expression_list(); + auto p = parser("(a)"); + auto e = p->expect_argument_expression_list("argument list"); ASSERT_FALSE(p->has_error()) << p->error(); ASSERT_FALSE(e.errored); @@ -29,9 +29,18 @@ TEST_F(ParserImplTest, ArgumentExpressionList_Parses) { ASSERT_TRUE(e.value[0]->Is()); } +TEST_F(ParserImplTest, ArgumentExpressionList_ParsesEmptyList) { + auto p = parser("()"); + auto e = p->expect_argument_expression_list("argument list"); + ASSERT_FALSE(p->has_error()) << p->error(); + ASSERT_FALSE(e.errored); + + ASSERT_EQ(e.value.size(), 0u); +} + TEST_F(ParserImplTest, ArgumentExpressionList_ParsesMultiple) { - auto p = parser("a, -33, 1+2"); - auto e = p->expect_argument_expression_list(); + auto p = parser("(a, -33, 1+2)"); + auto e = p->expect_argument_expression_list("argument list"); ASSERT_FALSE(p->has_error()) << p->error(); ASSERT_FALSE(e.errored); @@ -41,20 +50,36 @@ TEST_F(ParserImplTest, ArgumentExpressionList_ParsesMultiple) { ASSERT_TRUE(e.value[2]->Is()); } -TEST_F(ParserImplTest, ArgumentExpressionList_HandlesMissingExpression) { - auto p = parser("a, "); - auto e = p->expect_argument_expression_list(); +TEST_F(ParserImplTest, ArgumentExpressionList_HandlesMissingLeftParen) { + auto p = parser("a)"); + auto e = p->expect_argument_expression_list("argument list"); ASSERT_TRUE(p->has_error()); ASSERT_TRUE(e.errored); - EXPECT_EQ(p->error(), "1:4: unable to parse argument expression after comma"); + EXPECT_EQ(p->error(), "1:1: expected '(' for argument list"); +} + +TEST_F(ParserImplTest, ArgumentExpressionList_HandlesMissingRightParen) { + auto p = parser("(a"); + auto e = p->expect_argument_expression_list("argument list"); + ASSERT_TRUE(p->has_error()); + ASSERT_TRUE(e.errored); + EXPECT_EQ(p->error(), "1:3: expected ')' for argument list"); +} + +TEST_F(ParserImplTest, ArgumentExpressionList_HandlesMissingExpression) { + auto p = parser("(a, )"); + auto e = p->expect_argument_expression_list("argument list"); + ASSERT_TRUE(p->has_error()); + ASSERT_TRUE(e.errored); + EXPECT_EQ(p->error(), "1:5: unable to parse argument expression"); } TEST_F(ParserImplTest, ArgumentExpressionList_HandlesInvalidExpression) { - auto p = parser("if(a) {}"); - auto e = p->expect_argument_expression_list(); + auto p = parser("(if(a) {})"); + auto e = p->expect_argument_expression_list("argument list"); ASSERT_TRUE(p->has_error()); ASSERT_TRUE(e.errored); - EXPECT_EQ(p->error(), "1:1: unable to parse argument expression"); + EXPECT_EQ(p->error(), "1:2: unable to parse argument expression"); } } // namespace diff --git a/src/reader/wgsl/parser_impl_call_stmt_test.cc b/src/reader/wgsl/parser_impl_call_stmt_test.cc index 5a1dfc298a..3a4d2068ac 100644 --- a/src/reader/wgsl/parser_impl_call_stmt_test.cc +++ b/src/reader/wgsl/parser_impl_call_stmt_test.cc @@ -65,7 +65,7 @@ TEST_F(ParserImplTest, Statement_Call_Missing_RightParen) { EXPECT_TRUE(p->has_error()); EXPECT_TRUE(e.errored); EXPECT_FALSE(e.matched); - EXPECT_EQ(p->error(), "1:3: expected ')' for call statement"); + EXPECT_EQ(p->error(), "1:3: unable to parse argument expression"); } TEST_F(ParserImplTest, Statement_Call_Missing_Semi) { @@ -83,7 +83,7 @@ TEST_F(ParserImplTest, Statement_Call_Bad_ArgList) { EXPECT_TRUE(p->has_error()); EXPECT_TRUE(e.errored); EXPECT_FALSE(e.matched); - EXPECT_EQ(p->error(), "1:5: expected ')' for call statement"); + EXPECT_EQ(p->error(), "1:5: expected ')' for function call"); } } // namespace diff --git a/src/reader/wgsl/parser_impl_error_msg_test.cc b/src/reader/wgsl/parser_impl_error_msg_test.cc index 843309a1c3..17d5cd45dc 100644 --- a/src/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/reader/wgsl/parser_impl_error_msg_test.cc @@ -123,14 +123,14 @@ TEST_F(ParserImplErrorTest, BreakStmtMissingSemicolon) { TEST_F(ParserImplErrorTest, CallExprMissingRParen) { EXPECT("fn f() { x = f(1.; }", - "test.wgsl:1:18 error: expected ')' for call expression\n" + "test.wgsl:1:18 error: expected ')' for function call\n" "fn f() { x = f(1.; }\n" " ^\n"); } TEST_F(ParserImplErrorTest, CallStmtMissingRParen) { EXPECT("fn f() { f(1.; }", - "test.wgsl:1:14 error: expected ')' for call statement\n" + "test.wgsl:1:14 error: expected ')' for function call\n" "fn f() { f(1.; }\n" " ^\n"); } @@ -143,11 +143,10 @@ TEST_F(ParserImplErrorTest, CallStmtInvalidArgument0) { } TEST_F(ParserImplErrorTest, CallStmtInvalidArgument1) { - EXPECT( - "fn f() { f(1.0, <); }", - "test.wgsl:1:17 error: unable to parse argument expression after comma\n" - "fn f() { f(1.0, <); }\n" - " ^\n"); + EXPECT("fn f() { f(1.0, <); }", + "test.wgsl:1:17 error: unable to parse argument expression\n" + "fn f() { f(1.0, <); }\n" + " ^\n"); } TEST_F(ParserImplErrorTest, CallStmtMissingSemicolon) { diff --git a/src/reader/wgsl/parser_impl_singular_expression_test.cc b/src/reader/wgsl/parser_impl_singular_expression_test.cc index 30e206738c..2d40f248c9 100644 --- a/src/reader/wgsl/parser_impl_singular_expression_test.cc +++ b/src/reader/wgsl/parser_impl_singular_expression_test.cc @@ -145,7 +145,7 @@ TEST_F(ParserImplTest, SingularExpression_Call_HangingComma) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:6: unable to parse argument expression after comma"); + EXPECT_EQ(p->error(), "1:6: unable to parse argument expression"); } TEST_F(ParserImplTest, SingularExpression_Call_MissingRightParen) { @@ -155,7 +155,7 @@ TEST_F(ParserImplTest, SingularExpression_Call_MissingRightParen) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:3: expected ')' for call expression"); + EXPECT_EQ(p->error(), "1:3: unable to parse argument expression"); } TEST_F(ParserImplTest, SingularExpression_MemberAccessor) {