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 <jrprice@google.com>
Auto-Submit: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc
index 7bbad45..874e05b 100644
--- a/src/reader/wgsl/parser_impl.cc
+++ b/src/reader/wgsl/parser_impl.cc
@@ -2131,31 +2131,20 @@
}
// func_call_stmt
-// : IDENT PAREN_LEFT argument_expression_list* PAREN_RIGHT
+// : IDENT argument_expression_list
Maybe<ast::CallStatement*> 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<ast::CallStatement>(
@@ -2163,7 +2152,7 @@
source,
create<ast::IdentifierExpression>(
source, builder_.Symbols().Register(name)),
- std::move(params)));
+ std::move(params.value)));
}
// break_stmt
@@ -2197,7 +2186,7 @@
// 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 @@
auto* ident = create<ast::IdentifierExpression>(
t.source(), builder_.Symbols().Register(t.to_str()));
- if (match(Token::Type::kParenLeft, &source)) {
- return sync(Token::Type::kParenRight, [&]() -> Maybe<ast::Expression*> {
- 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<ast::CallExpression>(source, ident, params);
- });
+ return create<ast::CallExpression>(source, ident,
+ std::move(params.value));
}
return ident;
@@ -2265,25 +2244,12 @@
if (type.errored)
return Failure::kErrored;
if (type.matched) {
- auto expr = expect_paren_block(
- "type constructor", [&]() -> Expect<ast::TypeConstructorExpression*> {
- t = peek();
- if (t.IsParenRight() || t.IsEof())
- return create<ast::TypeConstructorExpression>(
- source, type.value, ast::ExpressionList{});
-
- auto params = expect_argument_expression_list();
- if (params.errored)
- return Failure::kErrored;
-
- return create<ast::TypeConstructorExpression>(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<ast::TypeConstructorExpression>(source, type.value,
+ std::move(params.value));
}
return Failure::kNoMatch;
@@ -2339,28 +2305,34 @@
}
// argument_expression_list
-// : (logical_or_expression COMMA)* logical_or_expression
-Expect<ast::ExpressionList> 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<ast::ExpressionList> ParserImpl::expect_argument_expression_list(
+ const std::string& use) {
+ return expect_paren_block(use, [&]() -> Expect<ast::ExpressionList> {
+ // 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 49a2d26..a18550c 100644
--- a/src/reader/wgsl/parser_impl.h
+++ b/src/reader/wgsl/parser_impl.h
@@ -552,8 +552,10 @@
Maybe<ast::Expression*> 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<ast::ExpressionList> expect_argument_expression_list();
+ Expect<ast::ExpressionList> 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 926abc0..ca8ed6c 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 {
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 @@
ASSERT_TRUE(e.value[0]->Is<ast::IdentifierExpression>());
}
+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 @@
ASSERT_TRUE(e.value[2]->Is<ast::BinaryExpression>());
}
-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 5a1dfc2..3a4d206 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 @@
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 @@
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 843309a..17d5cd4 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, 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, 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 30e2067..2d40f24 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 @@
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 @@
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) {
