Add optional attribute trailing commas.

This CL allows optional trailing commas on attribute lists.

Bug: tint:1423
Change-Id: I80f3b1580c95cac328b4ab846c6a3d4999fe34cb
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/96907
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Auto-Submit: Dan Sinclair <dsinclair@chromium.org>

src/tint/reader/wgsl/parser_impl.cc

@@ -3167,6 +3167,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::LocationAttribute>(t.source(), val.value);
         });
@@ -3179,6 +3180,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::BindingAttribute>(t.source(), val.value);
         });
@@ -3191,6 +3193,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::GroupAttribute>(t.source(), val.value);
         });
@@ -3213,15 +3216,19 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             }
 
             if (match(Token::Type::kComma)) {
-                auto sampling_tok = next();
+                if (!peek_is(Token::Type::kParenRight)) {
-                if (sampling_tok == "center") {
+                    auto sampling_tok = next();
-                    sampling = ast::InterpolationSampling::kCenter;
+                    if (sampling_tok == "center") {
-                } else if (sampling_tok == "centroid") {
+                        sampling = ast::InterpolationSampling::kCenter;
-                    sampling = ast::InterpolationSampling::kCentroid;
+                    } else if (sampling_tok == "centroid") {
-                } else if (sampling_tok == "sample") {
+                        sampling = ast::InterpolationSampling::kCentroid;
-                    sampling = ast::InterpolationSampling::kSample;
+                    } else if (sampling_tok == "sample") {
-                } else {
+                        sampling = ast::InterpolationSampling::kSample;
-                    return add_error(sampling_tok, "invalid interpolation sampling");
+                    } else {
+                        return add_error(sampling_tok, "invalid interpolation sampling");
+                    }
+
+                    match(Token::Type::kComma);
                 }
             }
 
@@ -3240,6 +3247,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
                 return Failure::kErrored;
             }
 
+            match(Token::Type::kComma);
             return create<ast::BuiltinAttribute>(t.source(), builtin.value);
         });
     }
@@ -3259,22 +3267,28 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             x = std::move(expr.value);
 
             if (match(Token::Type::kComma)) {
-                expr = primary_expression();
+                if (!peek_is(Token::Type::kParenRight)) {
-                if (expr.errored) {
-                    return Failure::kErrored;
-                } else if (!expr.matched) {
-                    return add_error(peek(), "expected workgroup_size y parameter");
-                }
-                y = std::move(expr.value);
-
-                if (match(Token::Type::kComma)) {
                     expr = primary_expression();
                     if (expr.errored) {
                         return Failure::kErrored;
                     } else if (!expr.matched) {
-                        return add_error(peek(), "expected workgroup_size z parameter");
+                        return add_error(peek(), "expected workgroup_size y parameter");
+                    }
+                    y = std::move(expr.value);
+
+                    if (match(Token::Type::kComma)) {
+                        if (!peek_is(Token::Type::kParenRight)) {
+                            expr = primary_expression();
+                            if (expr.errored) {
+                                return Failure::kErrored;
+                            } else if (!expr.matched) {
+                                return add_error(peek(), "expected workgroup_size z parameter");
+                            }
+                            z = std::move(expr.value);
+
+                            match(Token::Type::kComma);
+                        }
                     }
-                    z = std::move(expr.value);
                 }
             }
 
@@ -3326,6 +3340,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::StructMemberSizeAttribute>(t.source(), val.value);
         });
@@ -3338,6 +3353,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::StructMemberAlignAttribute>(t.source(), val.value);
         });
@@ -3350,6 +3366,7 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
             if (val.errored) {
                 return Failure::kErrored;
             }
+            match(Token::Type::kComma);
 
             return create<ast::IdAttribute>(t.source(), val.value);
         });

src/tint/reader/wgsl/parser_impl_error_msg_test.cc

@@ -357,18 +357,18 @@ TEST_F(ParserImplErrorTest, FunctionDeclWorkgroupSizeXInvalid) {
 }
 
 TEST_F(ParserImplErrorTest, FunctionDeclWorkgroupSizeYInvalid) {
-    EXPECT("@workgroup_size(1, ) fn f() {}",
+    EXPECT("@workgroup_size(1, fn) fn f() {}",
            R"(test.wgsl:1:20 error: expected workgroup_size y parameter
-@workgroup_size(1, ) fn f() {}
+@workgroup_size(1, fn) fn f() {}
-                   ^
+                   ^^
 )");
 }
 
 TEST_F(ParserImplErrorTest, FunctionDeclWorkgroupSizeZInvalid) {
-    EXPECT("@workgroup_size(1, 2, ) fn f() {}",
+    EXPECT("@workgroup_size(1, 2, fn) fn f() {}",
            R"(test.wgsl:1:23 error: expected workgroup_size z parameter
-@workgroup_size(1, 2, ) fn f() {}
+@workgroup_size(1, 2, fn) fn f() {}
-                      ^
+                      ^^
 )");
 }
 

src/tint/reader/wgsl/parser_impl_function_attribute_test.cc

@@ -41,6 +41,38 @@ TEST_F(ParserImplTest, Attribute_Workgroup) {
     EXPECT_EQ(values[2], nullptr);
 }
 
+TEST_F(ParserImplTest, Attribute_Workgroup_1Param_TrailingComma) {
+    auto p = parser("workgroup_size(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr) << p->error();
+    ASSERT_FALSE(p->has_error());
+    auto* func_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(func_attr, nullptr);
+    ASSERT_TRUE(func_attr->Is<ast::WorkgroupAttribute>());
+
+    auto values = func_attr->As<ast::WorkgroupAttribute>()->Values();
+
+    ASSERT_TRUE(values[0]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->value, 4);
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+
+    EXPECT_EQ(values[1], nullptr);
+    EXPECT_EQ(values[2], nullptr);
+}
+
+TEST_F(ParserImplTest, Attribute_Workgroup_1Param_TrailingComma_Double) {
+    auto p = parser("workgroup_size(4,,)");
+    auto attr = p->attribute();
+    EXPECT_FALSE(attr.matched);
+    EXPECT_TRUE(attr.errored);
+    EXPECT_EQ(attr.value, nullptr);
+    EXPECT_TRUE(p->has_error());
+    EXPECT_EQ(p->error(), "1:18: expected workgroup_size y parameter");
+}
+
 TEST_F(ParserImplTest, Attribute_Workgroup_2Param) {
     auto p = parser("workgroup_size(4, 5)");
     auto attr = p->attribute();
@@ -67,6 +99,42 @@ TEST_F(ParserImplTest, Attribute_Workgroup_2Param) {
     EXPECT_EQ(values[2], nullptr);
 }
 
+TEST_F(ParserImplTest, Attribute_Workgroup_2Param_TrailingComma) {
+    auto p = parser("workgroup_size(4, 5,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr) << p->error();
+    ASSERT_FALSE(p->has_error());
+    auto* func_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(func_attr, nullptr) << p->error();
+    ASSERT_TRUE(func_attr->Is<ast::WorkgroupAttribute>());
+
+    auto values = func_attr->As<ast::WorkgroupAttribute>()->Values();
+
+    ASSERT_TRUE(values[0]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->value, 4);
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+
+    ASSERT_TRUE(values[1]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[1]->As<ast::IntLiteralExpression>()->value, 5);
+    EXPECT_EQ(values[1]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+
+    EXPECT_EQ(values[2], nullptr);
+}
+
+TEST_F(ParserImplTest, Attribute_Workgroup21Param_TrailingComma_Double) {
+    auto p = parser("workgroup_size(4,5,,)");
+    auto attr = p->attribute();
+    EXPECT_FALSE(attr.matched);
+    EXPECT_TRUE(attr.errored);
+    EXPECT_EQ(attr.value, nullptr);
+    EXPECT_TRUE(p->has_error());
+    EXPECT_EQ(p->error(), "1:20: expected workgroup_size z parameter");
+}
+
 TEST_F(ParserImplTest, Attribute_Workgroup_3Param) {
     auto p = parser("workgroup_size(4, 5, 6)");
     auto attr = p->attribute();
@@ -96,6 +164,35 @@ TEST_F(ParserImplTest, Attribute_Workgroup_3Param) {
               ast::IntLiteralExpression::Suffix::kNone);
 }
 
+TEST_F(ParserImplTest, Attribute_Workgroup_3Param_TrailingComma) {
+    auto p = parser("workgroup_size(4, 5, 6,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr) << p->error();
+    ASSERT_FALSE(p->has_error());
+    auto* func_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(func_attr, nullptr);
+    ASSERT_TRUE(func_attr->Is<ast::WorkgroupAttribute>());
+
+    auto values = func_attr->As<ast::WorkgroupAttribute>()->Values();
+
+    ASSERT_TRUE(values[0]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->value, 4);
+    EXPECT_EQ(values[0]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+
+    ASSERT_TRUE(values[1]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[1]->As<ast::IntLiteralExpression>()->value, 5);
+    EXPECT_EQ(values[1]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+
+    ASSERT_TRUE(values[2]->Is<ast::IntLiteralExpression>());
+    EXPECT_EQ(values[2]->As<ast::IntLiteralExpression>()->value, 6);
+    EXPECT_EQ(values[2]->As<ast::IntLiteralExpression>()->suffix,
+              ast::IntLiteralExpression::Suffix::kNone);
+}
+
 TEST_F(ParserImplTest, Attribute_Workgroup_WithIdent) {
     auto p = parser("workgroup_size(4, height)");
     auto attr = p->attribute();
@@ -129,7 +226,7 @@ TEST_F(ParserImplTest, Attribute_Workgroup_TooManyValues) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:23: expected ')' for workgroup_size attribute");
+    EXPECT_EQ(p->error(), "1:25: expected ')' for workgroup_size attribute");
 }
 
 TEST_F(ParserImplTest, Attribute_Workgroup_MissingLeftParam) {
@@ -202,16 +299,6 @@ TEST_F(ParserImplTest, Attribute_Workgroup_Missing_Z_Comma) {
     EXPECT_EQ(p->error(), "1:21: expected ')' for workgroup_size attribute");
 }
 
-TEST_F(ParserImplTest, Attribute_Workgroup_Missing_Z_Value) {
-    auto p = parser("workgroup_size(1, 2, )");
-    auto attr = p->attribute();
-    EXPECT_FALSE(attr.matched);
-    EXPECT_TRUE(attr.errored);
-    EXPECT_EQ(attr.value, nullptr);
-    EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:22: expected workgroup_size z parameter");
-}
-
 // TODO(crbug.com/tint/1503): Remove when @stage is removed
 TEST_F(ParserImplTest, Attribute_Stage) {
     auto p = parser("stage(compute)");

src/tint/reader/wgsl/parser_impl_global_constant_decl_test.cc

@@ -204,6 +204,36 @@ TEST_F(ParserImplTest, GlobalOverrideDecl_WithId) {
     EXPECT_EQ(override_attr->value, 7u);
 }
 
+TEST_F(ParserImplTest, GlobalOverrideDecl_WithId_TrailingComma) {
+    auto p = parser("@id(7,) override a : f32 = 1.");
+    auto attrs = p->attribute_list();
+    EXPECT_FALSE(attrs.errored);
+    EXPECT_TRUE(attrs.matched);
+
+    auto e = p->global_constant_decl(attrs.value);
+    EXPECT_FALSE(p->has_error()) << p->error();
+    EXPECT_TRUE(e.matched);
+    EXPECT_FALSE(e.errored);
+    auto* override = e.value->As<ast::Override>();
+    ASSERT_NE(override, nullptr);
+
+    EXPECT_EQ(override->symbol, p->builder().Symbols().Get("a"));
+    ASSERT_NE(override->type, nullptr);
+    EXPECT_TRUE(override->type->Is<ast::F32>());
+
+    EXPECT_EQ(override->source.range.begin.line, 1u);
+    EXPECT_EQ(override->source.range.begin.column, 18u);
+    EXPECT_EQ(override->source.range.end.line, 1u);
+    EXPECT_EQ(override->source.range.end.column, 19u);
+
+    ASSERT_NE(override->constructor, nullptr);
+    EXPECT_TRUE(override->constructor->Is<ast::LiteralExpression>());
+
+    auto* override_attr = ast::GetAttribute<ast::IdAttribute>(override->attributes);
+    ASSERT_NE(override_attr, nullptr);
+    EXPECT_EQ(override_attr->value, 7u);
+}
+
 TEST_F(ParserImplTest, GlobalOverrideDecl_WithoutId) {
     auto p = parser("override a : f32 = 1.");
     auto attrs = p->attribute_list();

src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc

@@ -33,6 +33,22 @@ TEST_F(ParserImplTest, Attribute_Size) {
     EXPECT_EQ(o->size, 4u);
 }
 
+TEST_F(ParserImplTest, Attribute_Size_TrailingComma) {
+    auto p = parser("size(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    ASSERT_FALSE(p->has_error());
+
+    auto* member_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(member_attr, nullptr);
+    ASSERT_TRUE(member_attr->Is<ast::StructMemberSizeAttribute>());
+
+    auto* o = member_attr->As<ast::StructMemberSizeAttribute>();
+    EXPECT_EQ(o->size, 4u);
+}
+
 TEST_F(ParserImplTest, Attribute_Size_MissingLeftParen) {
     auto p = parser("size 4)");
     auto attr = p->attribute();
@@ -89,6 +105,22 @@ TEST_F(ParserImplTest, Attribute_Align) {
     EXPECT_EQ(o->align, 4u);
 }
 
+TEST_F(ParserImplTest, Attribute_Align_TrailingComma) {
+    auto p = parser("align(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    ASSERT_FALSE(p->has_error());
+
+    auto* member_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(member_attr, nullptr);
+    ASSERT_TRUE(member_attr->Is<ast::StructMemberAlignAttribute>());
+
+    auto* o = member_attr->As<ast::StructMemberAlignAttribute>();
+    EXPECT_EQ(o->align, 4u);
+}
+
 TEST_F(ParserImplTest, Attribute_Align_MissingLeftParen) {
     auto p = parser("align 4)");
     auto attr = p->attribute();

src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc

@@ -32,6 +32,21 @@ TEST_F(ParserImplTest, Attribute_Location) {
     EXPECT_EQ(loc->value, 4u);
 }
 
+TEST_F(ParserImplTest, Attribute_Location_TrailingComma) {
+    auto p = parser("location(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_FALSE(p->has_error());
+    ASSERT_TRUE(var_attr->Is<ast::LocationAttribute>());
+
+    auto* loc = var_attr->As<ast::LocationAttribute>();
+    EXPECT_EQ(loc->value, 4u);
+}
+
 TEST_F(ParserImplTest, Attribute_Location_MissingLeftParen) {
     auto p = parser("location 4)");
     auto attr = p->attribute();
@@ -99,6 +114,22 @@ TEST_P(BuiltinTest, Attribute_Builtin) {
     auto* builtin = var_attr->As<ast::BuiltinAttribute>();
     EXPECT_EQ(builtin->builtin, params.result);
 }
+TEST_P(BuiltinTest, Attribute_Builtin_TrailingComma) {
+    auto params = GetParam();
+    auto p = parser(std::string("builtin(") + params.input + ",)");
+
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_FALSE(p->has_error()) << p->error();
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_TRUE(var_attr->Is<ast::BuiltinAttribute>());
+
+    auto* builtin = var_attr->As<ast::BuiltinAttribute>();
+    EXPECT_EQ(builtin->builtin, params.result);
+}
 INSTANTIATE_TEST_SUITE_P(
     ParserImplTest,
     BuiltinTest,
@@ -182,6 +213,32 @@ TEST_F(ParserImplTest, Attribute_Interpolate_Flat) {
     EXPECT_EQ(interp->sampling, ast::InterpolationSampling::kNone);
 }
 
+TEST_F(ParserImplTest, Attribute_Interpolate_Single_TrailingComma) {
+    auto p = parser("interpolate(flat,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_FALSE(p->has_error());
+    ASSERT_TRUE(var_attr->Is<ast::InterpolateAttribute>());
+
+    auto* interp = var_attr->As<ast::InterpolateAttribute>();
+    EXPECT_EQ(interp->type, ast::InterpolationType::kFlat);
+    EXPECT_EQ(interp->sampling, ast::InterpolationSampling::kNone);
+}
+
+TEST_F(ParserImplTest, Attribute_Interpolate_Single_DoubleTrailingComma) {
+    auto p = parser("interpolate(flat,,)");
+    auto attr = p->attribute();
+    EXPECT_FALSE(attr.matched);
+    EXPECT_TRUE(attr.errored);
+    EXPECT_EQ(attr.value, nullptr);
+    EXPECT_TRUE(p->has_error());
+    EXPECT_EQ(p->error(), "1:18: invalid interpolation sampling");
+}
+
 TEST_F(ParserImplTest, Attribute_Interpolate_Perspective_Center) {
     auto p = parser("interpolate(perspective, center)");
     auto attr = p->attribute();
@@ -198,6 +255,22 @@ TEST_F(ParserImplTest, Attribute_Interpolate_Perspective_Center) {
     EXPECT_EQ(interp->sampling, ast::InterpolationSampling::kCenter);
 }
 
+TEST_F(ParserImplTest, Attribute_Interpolate_Double_TrailingComma) {
+    auto p = parser("interpolate(perspective, center,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_FALSE(p->has_error());
+    ASSERT_TRUE(var_attr->Is<ast::InterpolateAttribute>());
+
+    auto* interp = var_attr->As<ast::InterpolateAttribute>();
+    EXPECT_EQ(interp->type, ast::InterpolationType::kPerspective);
+    EXPECT_EQ(interp->sampling, ast::InterpolationSampling::kCenter);
+}
+
 TEST_F(ParserImplTest, Attribute_Interpolate_Perspective_Centroid) {
     auto p = parser("interpolate(perspective, centroid)");
     auto attr = p->attribute();
@@ -270,16 +343,6 @@ TEST_F(ParserImplTest, Attribute_Interpolate_InvalidFirstValue) {
     EXPECT_EQ(p->error(), "1:13: invalid interpolation type");
 }
 
-TEST_F(ParserImplTest, Attribute_Interpolate_MissingSecondValue) {
-    auto p = parser("interpolate(perspective,)");
-    auto attr = p->attribute();
-    EXPECT_FALSE(attr.matched);
-    EXPECT_TRUE(attr.errored);
-    EXPECT_EQ(attr.value, nullptr);
-    EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:25: invalid interpolation sampling");
-}
-
 TEST_F(ParserImplTest, Attribute_Interpolate_InvalidSecondValue) {
     auto p = parser("interpolate(perspective, nope)");
     auto attr = p->attribute();
@@ -305,6 +368,21 @@ TEST_F(ParserImplTest, Attribute_Binding) {
     EXPECT_EQ(binding->value, 4u);
 }
 
+TEST_F(ParserImplTest, Attribute_Binding_TrailingComma) {
+    auto p = parser("binding(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_FALSE(p->has_error());
+    ASSERT_TRUE(var_attr->Is<ast::BindingAttribute>());
+
+    auto* binding = var_attr->As<ast::BindingAttribute>();
+    EXPECT_EQ(binding->value, 4u);
+}
+
 TEST_F(ParserImplTest, Attribute_Binding_MissingLeftParen) {
     auto p = parser("binding 4)");
     auto attr = p->attribute();
@@ -360,6 +438,21 @@ TEST_F(ParserImplTest, Attribute_group) {
     EXPECT_EQ(group->value, 4u);
 }
 
+TEST_F(ParserImplTest, Attribute_group_TrailingComma) {
+    auto p = parser("group(4,)");
+    auto attr = p->attribute();
+    EXPECT_TRUE(attr.matched);
+    EXPECT_FALSE(attr.errored);
+    ASSERT_NE(attr.value, nullptr);
+    auto* var_attr = attr.value->As<ast::Attribute>();
+    ASSERT_FALSE(p->has_error());
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_TRUE(var_attr->Is<ast::GroupAttribute>());
+
+    auto* group = var_attr->As<ast::GroupAttribute>();
+    EXPECT_EQ(group->value, 4u);
+}
+
 TEST_F(ParserImplTest, Attribute_Group_MissingLeftParen) {
     auto p = parser("group 2)");
     auto attr = p->attribute();