wsgl parser: expect for variable_ident_decl
All the call sites of `variable_ident_decl()` add their own error handling, so transform this into `expect_variable_ident_decl()`.
Also makes error messages more consistent.
Bug: tint:282
Change-Id: I0b5ac984018ba78896ddec0320636f5b5c4ad0b2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/32100
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
diff --git a/src/reader/wgsl/parser_impl.cc b/src/reader/wgsl/parser_impl.cc
index 8cccd2c..d209256 100644
--- a/src/reader/wgsl/parser_impl.cc
+++ b/src/reader/wgsl/parser_impl.cc
@@ -123,7 +123,7 @@
ParserImpl::~ParserImpl() = default;
-void ParserImpl::add_error(const Token& t,
+void ParserImpl::add_error(const Source& source,
const std::string& err,
const std::string& use) {
std::stringstream msg;
@@ -131,7 +131,7 @@
if (!use.empty()) {
msg << " for " << use;
}
- add_error(t, msg.str());
+ add_error(source, msg.str());
}
void ParserImpl::add_error(const Token& t, const std::string& err) {
@@ -317,27 +317,23 @@
if (!match(Token::Type::kConst))
return nullptr;
- auto decl = variable_ident_decl();
+ const char* use = "constant declaration";
+
+ auto decl = expect_variable_ident_decl(use);
if (has_error())
return nullptr;
- if (decl.name.empty() || decl.type == nullptr) {
- add_error(peek(), "error parsing constant variable identifier");
- return nullptr;
- }
auto var = std::make_unique<ast::Variable>(
decl.source, decl.name, ast::StorageClass::kNone, decl.type);
var->set_is_const(true);
- auto t = next();
- if (!t.IsEqual()) {
- add_error(t, "missing = for const declaration");
+ if (!expect(use, Token::Type::kEqual))
return nullptr;
- }
auto init = expect_const_expr();
if (has_error())
return nullptr;
+
var->set_constructor(std::move(init));
return var;
@@ -351,15 +347,11 @@
auto sc = variable_storage_decoration();
if (has_error())
- return {};
+ return nullptr;
- auto decl = variable_ident_decl();
+ auto decl = expect_variable_ident_decl("variable declaration");
if (has_error())
return nullptr;
- if (decl.name.empty() || decl.type == nullptr) {
- add_error(peek(), "invalid identifier declaration");
- return nullptr;
- }
return std::make_unique<ast::Variable>(decl.source, decl.name, sc, decl.type);
}
@@ -753,26 +745,22 @@
// variable_ident_decl
// : IDENT COLON type_decl
-ParserImpl::TypedIdentifier ParserImpl::variable_ident_decl() {
+ParserImpl::TypedIdentifier ParserImpl::expect_variable_ident_decl(
+ const std::string& use) {
+ std::string name;
+ Source source;
+ if (!expect_ident(use, &name, &source))
+ return {};
+
+ if (!expect(use, Token::Type::kColon))
+ return {};
+
auto t = peek();
- if (!t.IsIdentifier())
- return {};
-
- auto name = t.to_str();
- auto source = t.source();
- next(); // Consume the peek
-
- t = next();
- if (!t.IsColon()) {
- add_error(t, "missing : for identifier declaration");
- return {};
- }
-
auto* type = type_decl();
if (has_error())
return {};
if (type == nullptr) {
- add_error(peek(), "invalid type for identifier declaration");
+ add_error(t.source(), "invalid type", use);
return {};
}
@@ -894,9 +882,8 @@
}
auto decos = decoration_list();
- if (has_error()) {
+ if (has_error())
return nullptr;
- }
if (match(Token::Type::kArray)) {
auto array_decos = cast_decorations<ast::ArrayDecoration>(decos);
@@ -1133,9 +1120,8 @@
return nullptr;
auto body = expect_struct_body_decl();
- if (has_error()) {
+ if (has_error())
return nullptr;
- }
return std::make_unique<ast::type::StructType>(
name, std::make_unique<ast::Struct>(source, std::move(struct_decos),
@@ -1150,6 +1136,8 @@
while (!peek().IsBraceRight() && !peek().IsEof()) {
auto decos = decoration_list();
+ if (has_error())
+ return ast::StructMemberList{};
auto mem = expect_struct_member(decos);
if (has_error())
@@ -1166,15 +1154,23 @@
// : struct_member_decoration_decl+ variable_ident_decl SEMICOLON
std::unique_ptr<ast::StructMember> ParserImpl::expect_struct_member(
ast::DecorationList& decos) {
- auto t = peek();
-
- auto decl = variable_ident_decl();
+ // FUDGE - Abort early if we enter with an error state to avoid accumulating
+ // multiple error messages. This is a work around for the unit tests that
+ // call:
+ // auto decos = p->decoration_list();
+ // auto m = p->expect_struct_member(decos);
+ // ... and expect a single error message due to bad decorations.
+ // While expect_struct_body_decl() aborts after checking for decoration parse
+ // errors (and so these tests do not currently reflect full-parse behaviour),
+ // they do test the long-term desired behavior where the parser can
+ // resynchronize at the ']]'.
+ // TODO(ben-clayton) - remove this once resynchronization is implemented.
if (has_error())
return nullptr;
- if (decl.name.empty() || decl.type == nullptr) {
- add_error(peek(), "invalid identifier declaration");
+
+ auto decl = expect_variable_ident_decl("struct member");
+ if (has_error())
return nullptr;
- }
auto member_decos = cast_decorations<ast::StructMemberDecoration>(decos);
@@ -1255,16 +1251,14 @@
// :
// | (variable_ident_decl COMMA)* variable_ident_decl
ast::VariableList ParserImpl::expect_param_list() {
- auto t = peek();
+ if (!peek().IsIdentifier()) // Empty list
+ return ast::VariableList{};
- ast::VariableList ret;
-
- auto decl = variable_ident_decl();
+ auto decl = expect_variable_ident_decl("parameter");
if (has_error())
return {};
- if (decl.name.empty() || decl.type == nullptr)
- return {};
+ ast::VariableList ret;
for (;;) {
auto var = std::make_unique<ast::Variable>(
decl.source, decl.name, ast::StorageClass::kNone, decl.type);
@@ -1275,19 +1269,12 @@
var->set_is_const(true);
ret.push_back(std::move(var));
- t = peek();
- if (!t.IsComma())
+ if (!match(Token::Type::kComma))
break;
- next(); // Consume the peek
-
- decl = variable_ident_decl();
+ decl = expect_variable_ident_decl("parameter");
if (has_error())
return {};
- if (decl.name.empty() || decl.type == nullptr) {
- add_error(t, "found , but no variable declaration");
- return {};
- }
}
return ret;
@@ -1518,19 +1505,12 @@
if (t.IsConst()) {
next(); // Consume the peek
- auto decl = variable_ident_decl();
+ auto decl = expect_variable_ident_decl("constant declaration");
if (has_error())
return nullptr;
- if (decl.name.empty() || decl.type == nullptr) {
- add_error(peek(), "unable to parse variable declaration");
- return nullptr;
- }
- t = next();
- if (!t.IsEqual()) {
- add_error(t, "missing = for constant declaration");
+ if (!expect("constant declaration", Token::Type::kEqual))
return nullptr;
- }
auto constructor = logical_or_expression();
if (has_error())
@@ -2760,9 +2740,9 @@
return false;
}
- auto t = peek();
- if (match(Token::Type::kAttrRight)) {
- add_error(t, "empty decoration list");
+ Source source;
+ if (match(Token::Type::kAttrRight, &source)) {
+ add_error(source, "empty decoration list");
return false;
}
@@ -2954,7 +2934,7 @@
bool ParserImpl::expect_sint(const std::string& use, int32_t* out) {
auto t = next();
if (!t.IsSintLiteral()) {
- add_error(t, "expected signed integer literal", use);
+ add_error(t.source(), "expected signed integer literal", use);
return false;
}
*out = t.to_i32();
@@ -2999,7 +2979,7 @@
*source = t.source();
if (!t.IsIdentifier()) {
- add_error(t, "expected identifier", use);
+ add_error(t.source(), "expected identifier", use);
return false;
}
diff --git a/src/reader/wgsl/parser_impl.h b/src/reader/wgsl/parser_impl.h
index 74d82fb..da2f724 100644
--- a/src/reader/wgsl/parser_impl.h
+++ b/src/reader/wgsl/parser_impl.h
@@ -132,11 +132,11 @@
/// @param msg the error message
void add_error(const Token& t, const std::string& msg);
/// Appends an error raised when parsing |use| at |t| with the message |msg|
- /// @param t the token to associate the error with
+ /// @param source the source to associate the error with
/// @param msg the error message
/// @param use a description of what was being parsed when the error was
/// raised.
- void add_error(const Token& t,
+ void add_error(const Source& source,
const std::string& msg,
const std::string& use);
/// Appends an error at |source| with the message |msg|
@@ -169,9 +169,11 @@
/// Parses a `variable_decl` grammar element
/// @returns the parsed variable or nullptr otherwise
std::unique_ptr<ast::Variable> variable_decl();
- /// Parses a `variable_ident_decl` grammar element
+ /// Parses a `variable_ident_decl` grammar element, erroring on parse
+ /// failure.
+ /// @param use a description of what was being parsed if an error was raised.
/// @returns the identifier and type parsed or empty otherwise
- TypedIdentifier variable_ident_decl();
+ TypedIdentifier expect_variable_ident_decl(const std::string& use);
/// Parses a `variable_storage_decoration` grammar element
/// @returns the storage class or StorageClass::kNone if none matched
ast::StorageClass variable_storage_decoration();
diff --git a/src/reader/wgsl/parser_impl_error_msg_test.cc b/src/reader/wgsl/parser_impl_error_msg_test.cc
index 641c5cc..f07df6d 100644
--- a/src/reader/wgsl/parser_impl_error_msg_test.cc
+++ b/src/reader/wgsl/parser_impl_error_msg_test.cc
@@ -169,14 +169,14 @@
TEST_F(ParserImplErrorTest, ConstVarStmtInvalid) {
EXPECT("fn f() -> void { const >; }",
- "test.wgsl:1:24 error: unable to parse variable declaration\n"
+ "test.wgsl:1:24 error: expected identifier for constant declaration\n"
"fn f() -> void { const >; }\n"
" ^\n");
}
TEST_F(ParserImplErrorTest, ConstVarStmtMissingAssignment) {
EXPECT("fn f() -> void { const a : i32; }",
- "test.wgsl:1:31 error: missing = for constant declaration\n"
+ "test.wgsl:1:31 error: expected '=' for constant declaration\n"
"fn f() -> void { const a : i32; }\n"
" ^\n");
}
@@ -416,23 +416,23 @@
TEST_F(ParserImplErrorTest, FunctionDeclParamMissingColon) {
EXPECT("fn f(x) -> void {}",
- "test.wgsl:1:7 error: missing : for identifier declaration\n"
+ "test.wgsl:1:7 error: expected ':' for parameter\n"
"fn f(x) -> void {}\n"
" ^\n");
}
TEST_F(ParserImplErrorTest, FunctionDeclParamInvalidType) {
EXPECT("fn f(x : 1) -> void {}",
- "test.wgsl:1:10 error: invalid type for identifier declaration\n"
+ "test.wgsl:1:10 error: invalid type for parameter\n"
"fn f(x : 1) -> void {}\n"
" ^\n");
}
TEST_F(ParserImplErrorTest, FunctionDeclParamMissing) {
EXPECT("fn f(x : i32, ) -> void {}",
- "test.wgsl:1:13 error: found , but no variable declaration\n"
+ "test.wgsl:1:15 error: expected identifier for parameter\n"
"fn f(x : i32, ) -> void {}\n"
- " ^\n");
+ " ^\n");
}
TEST_F(ParserImplErrorTest, FunctionDeclMissingLBrace) {
@@ -451,7 +451,7 @@
TEST_F(ParserImplErrorTest, GlobalDeclConstInvalidIdentifier) {
EXPECT("const ^ : i32 = 1;",
- "test.wgsl:1:7 error: error parsing constant variable identifier\n"
+ "test.wgsl:1:7 error: expected identifier for constant declaration\n"
"const ^ : i32 = 1;\n"
" ^\n");
}
@@ -479,7 +479,7 @@
TEST_F(ParserImplErrorTest, GlobalDeclConstMissingAssignment) {
EXPECT("const i : vec2<i32>;",
- "test.wgsl:1:20 error: missing = for const declaration\n"
+ "test.wgsl:1:20 error: expected '=' for constant declaration\n"
"const i : vec2<i32>;\n"
" ^\n");
}
@@ -702,7 +702,7 @@
TEST_F(ParserImplErrorTest, GlobalDeclStructMemberInvalidIdentifier) {
EXPECT("struct S { 1 : i32; };",
- "test.wgsl:1:12 error: invalid identifier declaration\n"
+ "test.wgsl:1:12 error: expected identifier for struct member\n"
"struct S { 1 : i32; };\n"
" ^\n");
}
@@ -988,7 +988,7 @@
TEST_F(ParserImplErrorTest, GlobalDeclVarInvalidIdentifier) {
EXPECT("var ^ : mat4x4;",
- "test.wgsl:1:5 error: invalid identifier declaration\n"
+ "test.wgsl:1:5 error: expected identifier for variable declaration\n"
"var ^ : mat4x4;\n"
" ^\n");
}
diff --git a/src/reader/wgsl/parser_impl_for_stmt_test.cc b/src/reader/wgsl/parser_impl_for_stmt_test.cc
index b599e65..a8a8128 100644
--- a/src/reader/wgsl/parser_impl_for_stmt_test.cc
+++ b/src/reader/wgsl/parser_impl_for_stmt_test.cc
@@ -216,7 +216,7 @@
// Test a for loop with an invalid initializer statement.
TEST_F(ForStmtErrorTest, InvalidInitializerAsConstDecl) {
std::string for_str = "for (const x: i32;;) { }";
- std::string error_str = "1:18: missing = for constant declaration";
+ std::string error_str = "1:18: expected '=' for constant declaration";
TestForWithError(for_str, error_str);
}
@@ -267,7 +267,7 @@
// Test a for loop with an invalid body.
TEST_F(ForStmtErrorTest, InvalidBody) {
std::string for_str = "for (;;) { const x: i32; }";
- std::string error_str = "1:24: missing = for constant declaration";
+ std::string error_str = "1:24: expected '=' for constant declaration";
TestForWithError(for_str, error_str);
}
diff --git a/src/reader/wgsl/parser_impl_function_header_test.cc b/src/reader/wgsl/parser_impl_function_header_test.cc
index dfd053c..20a94ff 100644
--- a/src/reader/wgsl/parser_impl_function_header_test.cc
+++ b/src/reader/wgsl/parser_impl_function_header_test.cc
@@ -65,7 +65,7 @@
auto f = p->function_header();
ASSERT_TRUE(p->has_error());
ASSERT_EQ(f, nullptr);
- EXPECT_EQ(p->error(), "1:15: found , but no variable declaration");
+ EXPECT_EQ(p->error(), "1:16: expected identifier for parameter");
}
TEST_F(ParserImplTest, FunctionHeader_MissingParenRight) {
diff --git a/src/reader/wgsl/parser_impl_global_constant_decl_test.cc b/src/reader/wgsl/parser_impl_global_constant_decl_test.cc
index be0535a..fd2719b 100644
--- a/src/reader/wgsl/parser_impl_global_constant_decl_test.cc
+++ b/src/reader/wgsl/parser_impl_global_constant_decl_test.cc
@@ -48,7 +48,7 @@
auto e = p->global_constant_decl();
ASSERT_TRUE(p->has_error());
ASSERT_EQ(e, nullptr);
- EXPECT_EQ(p->error(), "1:14: missing = for const declaration");
+ EXPECT_EQ(p->error(), "1:14: expected '=' for constant declaration");
}
TEST_F(ParserImplTest, GlobalConstantDecl_InvalidVariable) {
diff --git a/src/reader/wgsl/parser_impl_global_decl_test.cc b/src/reader/wgsl/parser_impl_global_decl_test.cc
index fb027b5..f85eb71 100644
--- a/src/reader/wgsl/parser_impl_global_decl_test.cc
+++ b/src/reader/wgsl/parser_impl_global_decl_test.cc
@@ -71,7 +71,7 @@
auto* p = parser("const a : vec2<i32>;");
p->expect_global_decl();
ASSERT_TRUE(p->has_error());
- EXPECT_EQ(p->error(), "1:20: missing = for const declaration");
+ EXPECT_EQ(p->error(), "1:20: expected '=' for constant declaration");
}
TEST_F(ParserImplTest, GlobalDecl_GlobalConstant_MissingSemicolon) {
diff --git a/src/reader/wgsl/parser_impl_param_list_test.cc b/src/reader/wgsl/parser_impl_param_list_test.cc
index 1af5e61..1711106 100644
--- a/src/reader/wgsl/parser_impl_param_list_test.cc
+++ b/src/reader/wgsl/parser_impl_param_list_test.cc
@@ -87,7 +87,7 @@
TEST_F(ParserImplTest, ParamList_Empty) {
auto* p = parser("");
auto e = p->expect_param_list();
- ASSERT_FALSE(p->has_error()) << p->error();
+ ASSERT_FALSE(p->has_error());
EXPECT_EQ(e.size(), 0u);
}
@@ -95,7 +95,7 @@
auto* p = parser("a : i32,");
auto e = p->expect_param_list();
ASSERT_TRUE(p->has_error());
- EXPECT_EQ(p->error(), "1:8: found , but no variable declaration");
+ EXPECT_EQ(p->error(), "1:9: expected identifier for parameter");
}
} // namespace
diff --git a/src/reader/wgsl/parser_impl_struct_body_decl_test.cc b/src/reader/wgsl/parser_impl_struct_body_decl_test.cc
index 7ea358e..7e8d7d2 100644
--- a/src/reader/wgsl/parser_impl_struct_body_decl_test.cc
+++ b/src/reader/wgsl/parser_impl_struct_body_decl_test.cc
@@ -70,7 +70,7 @@
} )");
auto m = p->expect_struct_body_decl();
ASSERT_TRUE(p->has_error());
- EXPECT_EQ(p->error(), "4:3: invalid identifier declaration");
+ EXPECT_EQ(p->error(), "4:3: expected identifier for struct member");
}
} // namespace
diff --git a/src/reader/wgsl/parser_impl_variable_decl_test.cc b/src/reader/wgsl/parser_impl_variable_decl_test.cc
index 97d0115..35edb24 100644
--- a/src/reader/wgsl/parser_impl_variable_decl_test.cc
+++ b/src/reader/wgsl/parser_impl_variable_decl_test.cc
@@ -52,7 +52,7 @@
auto v = p->variable_decl();
ASSERT_TRUE(p->has_error());
ASSERT_EQ(v, nullptr);
- ASSERT_EQ(p->error(), "1:12: missing : for identifier declaration");
+ ASSERT_EQ(p->error(), "1:12: expected ':' for variable declaration");
}
TEST_F(ParserImplTest, VariableDecl_WithStorageClass) {
diff --git a/src/reader/wgsl/parser_impl_variable_ident_decl_test.cc b/src/reader/wgsl/parser_impl_variable_ident_decl_test.cc
index dc06863..f9eb7a7 100644
--- a/src/reader/wgsl/parser_impl_variable_ident_decl_test.cc
+++ b/src/reader/wgsl/parser_impl_variable_ident_decl_test.cc
@@ -23,7 +23,7 @@
TEST_F(ParserImplTest, VariableIdentDecl_Parses) {
auto* p = parser("my_var : f32");
- auto decl = p->variable_ident_decl();
+ auto decl = p->expect_variable_ident_decl("test");
ASSERT_FALSE(p->has_error());
ASSERT_EQ(decl.name, "my_var");
ASSERT_NE(decl.type, nullptr);
@@ -37,43 +37,35 @@
TEST_F(ParserImplTest, VariableIdentDecl_MissingIdent) {
auto* p = parser(": f32");
- auto decl = p->variable_ident_decl();
- ASSERT_FALSE(p->has_error());
- ASSERT_EQ(decl.name, "");
- ASSERT_EQ(decl.type, nullptr);
-
- auto t = p->next();
- ASSERT_TRUE(t.IsColon());
+ auto decl = p->expect_variable_ident_decl("test");
+ ASSERT_TRUE(p->has_error());
+ ASSERT_EQ(p->error(), "1:1: expected identifier for test");
}
TEST_F(ParserImplTest, VariableIdentDecl_MissingColon) {
auto* p = parser("my_var f32");
- auto r = p->variable_ident_decl();
+ auto r = p->expect_variable_ident_decl("test");
ASSERT_TRUE(p->has_error());
- ASSERT_EQ(p->error(), "1:8: missing : for identifier declaration");
+ ASSERT_EQ(p->error(), "1:8: expected ':' for test");
}
TEST_F(ParserImplTest, VariableIdentDecl_MissingType) {
auto* p = parser("my_var :");
- auto r = p->variable_ident_decl();
+ auto r = p->expect_variable_ident_decl("test");
ASSERT_TRUE(p->has_error());
- ASSERT_EQ(p->error(), "1:9: invalid type for identifier declaration");
+ ASSERT_EQ(p->error(), "1:9: invalid type for test");
}
TEST_F(ParserImplTest, VariableIdentDecl_InvalidIdent) {
auto* p = parser("123 : f32");
- auto decl = p->variable_ident_decl();
- ASSERT_FALSE(p->has_error());
- ASSERT_EQ(decl.name, "");
- ASSERT_EQ(decl.type, nullptr);
-
- auto t = p->next();
- ASSERT_TRUE(t.IsSintLiteral());
+ auto decl = p->expect_variable_ident_decl("test");
+ ASSERT_TRUE(p->has_error());
+ ASSERT_EQ(p->error(), "1:1: expected identifier for test");
}
TEST_F(ParserImplTest, VariableIdentDecl_InvalidType) {
auto* p = parser("my_var : invalid");
- auto r = p->variable_ident_decl();
+ auto r = p->expect_variable_ident_decl("test");
ASSERT_TRUE(p->has_error());
ASSERT_EQ(p->error(), "1:10: unknown constructed type 'invalid'");
}
diff --git a/src/reader/wgsl/parser_impl_variable_stmt_test.cc b/src/reader/wgsl/parser_impl_variable_stmt_test.cc
index 1330a7a..8a984c9 100644
--- a/src/reader/wgsl/parser_impl_variable_stmt_test.cc
+++ b/src/reader/wgsl/parser_impl_variable_stmt_test.cc
@@ -100,7 +100,7 @@
auto e = p->variable_stmt();
ASSERT_TRUE(p->has_error());
ASSERT_EQ(e, nullptr);
- EXPECT_EQ(p->error(), "1:15: missing = for constant declaration");
+ EXPECT_EQ(p->error(), "1:15: expected '=' for constant declaration");
}
TEST_F(ParserImplTest, VariableStmt_Const_MissingConstructor) {
