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>

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 @@ void ParserImpl::translation_unit() {
 
 // 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 @@ Maybe<bool> ParserImpl::global_directive(bool have_parsed_decl) {
 
 // enable_directive
 //  : enable name SEMICLON
-Maybe<bool> ParserImpl::enable_directive() {
+Maybe<Void> ParserImpl::enable_directive() {
-    auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<bool> {
+    auto decl = sync(Token::Type::kSemicolon, [&]() -> Maybe<Void> {
         if (!match(Token::Type::kEnable)) {
             return Failure::kNoMatch;
         }
@@ -403,14 +409,14 @@ Maybe<bool> ParserImpl::enable_directive() {
         }
         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 @@ Maybe<bool> ParserImpl::enable_directive() {
 //  | 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 @@ Maybe<bool> ParserImpl::global_decl() {
         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 @@ Maybe<bool> ParserImpl::global_decl() {
             }
 
             builder_.AST().AddGlobalVariable(gv.value);
-            return true;
+            return kSuccess;
         }
 
         auto gc = global_constant_decl(attrs.value);
@@ -466,7 +472,7 @@ Maybe<bool> ParserImpl::global_decl() {
             }
 
             builder_.AST().AddGlobalVariable(gc.value);
-            return true;
+            return kSuccess;
         }
 
         auto ta = type_alias_decl();
@@ -479,7 +485,7 @@ Maybe<bool> ParserImpl::global_decl() {
             }
 
             builder_.AST().AddTypeDecl(ta.value);
-            return true;
+            return kSuccess;
         }
 
         auto assertion = static_assert_statement();
@@ -491,7 +497,7 @@ Maybe<bool> ParserImpl::global_decl() {
             if (!expect("static assertion declaration", Token::Type::kSemicolon)) {
                 return Failure::kErrored;
             }
-            return true;
+            return kSuccess;
         }
 
         return Failure::kNoMatch;
@@ -501,7 +507,10 @@ Maybe<bool> ParserImpl::global_decl() {
         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 @@ Maybe<bool> ParserImpl::global_decl() {
     }
     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 @@ Maybe<bool> ParserImpl::global_decl() {
     }
     if (func.matched) {
         builder_.AST().AddFunction(func.value);
-        return true;
+        return kSuccess;
     }
 
     if (errored) {

src/tint/reader/wgsl/parser_impl.h

@@ -82,6 +82,10 @@ class ParserImpl {
     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 @@ class ParserImpl {
     /// 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

src/tint/reader/wgsl/parser_impl_global_decl_test.cc

@@ -233,6 +233,17 @@ TEST_F(ParserImplTest, GlobalDecl_Struct_Invalid) {
     }
 }
 
+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();