reader/wgsl: Error if 'struct' has attributes
Attributes were being parsed, constructed, then thrown away, when declared on a structure. This was triggering the unreachable-AST node seatbelt in the Resolver.
Replace the confusing `Maybe<bool>` return types with `Maybe<Void>`. The boolean return value was not actually being used, as logic was (correctly) using the `Maybe` error / matched state.
Bug: chromium:1352803
Change-Id: I39e4994e3e9b13201ba4f4e4820cd4b2f46e93c5
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/99100
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Auto-Submit: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>
diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc
index 1788e32..50f9a45 100644
--- a/src/tint/reader/wgsl/parser_impl.cc
+++ b/src/tint/reader/wgsl/parser_impl.cc
@@ -47,6 +47,12 @@
namespace tint::reader::wgsl {
namespace {
+using Void = ParserImpl::Void;
+
+/// An instance of Void that can be used to signal success for functions that return Expect<Void> or
+/// Maybe<NoError>.
+static constexpr Void kSuccess;
+
template <typename T>
using Expect = ParserImpl::Expect<T>;
@@ -351,7 +357,7 @@
// global_directive
// : enable_directive
-Maybe<bool> ParserImpl::global_directive(bool have_parsed_decl) {
+Maybe<Void> ParserImpl::global_directive(bool have_parsed_decl) {
auto& p = peek();
auto ed = enable_directive();
if (ed.matched && have_parsed_decl) {
@@ -362,8 +368,8 @@
// enable_directive
// : enable name SEMICLON
-Maybe<bool> ParserImpl::enable_directive() {
- auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<bool> {
+Maybe<Void> ParserImpl::enable_directive() {
+ auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<Void> {
if (!match(Token::Type::kEnable)) {
return Failure::kNoMatch;
}
@@ -403,14 +409,14 @@
}
builder_.AST().AddEnable(create<ast::Enable>(name.source, extension));
- return true;
+ return kSuccess;
});
if (decl.errored) {
return Failure::kErrored;
}
if (decl.matched) {
- return true;
+ return kSuccess;
}
return Failure::kNoMatch;
@@ -424,9 +430,9 @@
// | struct_decl
// | function_decl
// | static_assert_statement SEMICOLON
-Maybe<bool> ParserImpl::global_decl() {
+Maybe<Void> ParserImpl::global_decl() {
if (match(Token::Type::kSemicolon) || match(Token::Type::kEOF)) {
- return true;
+ return kSuccess;
}
bool errored = false;
@@ -438,7 +444,7 @@
return Failure::kErrored;
}
- auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<bool> {
+ auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<Void> {
auto gv = global_variable_decl(attrs.value);
if (gv.errored) {
return Failure::kErrored;
@@ -449,7 +455,7 @@
}
builder_.AST().AddGlobalVariable(gv.value);
- return true;
+ return kSuccess;
}
auto gc = global_constant_decl(attrs.value);
@@ -466,7 +472,7 @@
}
builder_.AST().AddGlobalVariable(gc.value);
- return true;
+ return kSuccess;
}
auto ta = type_alias_decl();
@@ -479,7 +485,7 @@
}
builder_.AST().AddTypeDecl(ta.value);
- return true;
+ return kSuccess;
}
auto assertion = static_assert_statement();
@@ -491,7 +497,7 @@
if (!expect("static assertion declaration", Token::Type::kSemicolon)) {
return Failure::kErrored;
}
- return true;
+ return kSuccess;
}
return Failure::kNoMatch;
@@ -501,7 +507,10 @@
errored = true;
}
if (decl.matched) {
- return expect_attributes_consumed(attrs.value);
+ if (!expect_attributes_consumed(attrs.value)) {
+ return Failure::kErrored;
+ }
+ return kSuccess;
}
auto str = struct_decl();
@@ -510,7 +519,10 @@
}
if (str.matched) {
builder_.AST().AddTypeDecl(str.value);
- return true;
+ if (!expect_attributes_consumed(attrs.value)) {
+ return Failure::kErrored;
+ }
+ return kSuccess;
}
auto func = function_decl(attrs.value);
@@ -519,7 +531,7 @@
}
if (func.matched) {
builder_.AST().AddFunction(func.value);
- return true;
+ return kSuccess;
}
if (errored) {
diff --git a/src/tint/reader/wgsl/parser_impl.h b/src/tint/reader/wgsl/parser_impl.h
index 244b7f4..55c3be9 100644
--- a/src/tint/reader/wgsl/parser_impl.h
+++ b/src/tint/reader/wgsl/parser_impl.h
@@ -82,6 +82,10 @@
using StructMemberList = utils::Vector<const ast::StructMember*, 8>;
//! @endcond
+ /// Empty structure used by functions that do not return a value, but need to signal success /
+ /// error with Expect<Void> or Maybe<NoError>.
+ struct Void {};
+
/// Expect is the return type of the parser methods that are expected to
/// return a parsed value of type T, unless there was an parse error.
/// In the case of a parse error the called method will have called
@@ -388,13 +392,13 @@
/// Parses the `global_directive` grammar element, erroring on parse failure.
/// @param has_parsed_decl flag indicating if the parser has consumed a global declaration.
/// @return true on parse success, otherwise an error or no-match.
- Maybe<bool> global_directive(bool has_parsed_decl);
+ Maybe<Void> global_directive(bool has_parsed_decl);
/// Parses the `enable_directive` grammar element, erroring on parse failure.
/// @return true on parse success, otherwise an error or no-match.
- Maybe<bool> enable_directive();
+ Maybe<Void> enable_directive();
/// Parses the `global_decl` grammar element, erroring on parse failure.
/// @return true on parse success, otherwise an error or no-match.
- Maybe<bool> global_decl();
+ Maybe<Void> global_decl();
/// Parses a `global_variable_decl` grammar element with the initial
/// `variable_attribute_list*` provided as `attrs`
/// @returns the variable parsed or nullptr
diff --git a/src/tint/reader/wgsl/parser_impl_global_decl_test.cc b/src/tint/reader/wgsl/parser_impl_global_decl_test.cc
index d9001c5..6c943c8 100644
--- a/src/tint/reader/wgsl/parser_impl_global_decl_test.cc
+++ b/src/tint/reader/wgsl/parser_impl_global_decl_test.cc
@@ -233,6 +233,17 @@
}
}
+TEST_F(ParserImplTest, GlobalDecl_Struct_UnexpectedAttribute) {
+ auto p = parser("@vertex struct S { i : i32 }");
+
+ auto s = p->global_decl();
+ EXPECT_TRUE(s.errored);
+ EXPECT_FALSE(s.matched);
+
+ EXPECT_TRUE(p->has_error());
+ EXPECT_EQ(p->error(), "1:2: unexpected attributes");
+}
+
TEST_F(ParserImplTest, GlobalDecl_StaticAssert_WithParen) {
auto p = parser("static_assert(true);");
p->global_decl();
