tint/resolver: Make member attribute diagnostics consistent

Standardize how we refer to @size, @align and @offset.

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

src/tint/resolver/attribute_validation_test.cc

@@ -673,7 +673,7 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_ConstNegative) {
                               "a", ty.f32(), utils::Vector{MemberAlign(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
     EXPECT_EQ(r()->error(),
-              R"(12:34 error: 'align' value must be a positive, power-of-two integer)");
+              R"(12:34 error: @align value must be a positive, power-of-two integer)");
 }
 
 TEST_F(StructMemberAttributeTest, Align_Attribute_ConstPowerOfTwo) {
@@ -683,7 +683,7 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_ConstPowerOfTwo) {
                               "a", ty.f32(), utils::Vector{MemberAlign(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
     EXPECT_EQ(r()->error(),
-              R"(12:34 error: 'align' value must be a positive, power-of-two integer)");
+              R"(12:34 error: @align value must be a positive, power-of-two integer)");
 }
 
 TEST_F(StructMemberAttributeTest, Align_Attribute_ConstF32) {
@@ -692,7 +692,7 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_ConstF32) {
     Structure("mystruct", utils::Vector{Member(
                               "a", ty.f32(), utils::Vector{MemberAlign(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'align' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @align must be an i32 or u32 value)");
 }
 
 TEST_F(StructMemberAttributeTest, Align_Attribute_ConstU32) {
@@ -717,7 +717,7 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_ConstAFloat) {
     Structure("mystruct", utils::Vector{Member(
                               "a", ty.f32(), utils::Vector{MemberAlign(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'align' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @align must be an i32 or u32 value)");
 }
 
 TEST_F(StructMemberAttributeTest, Align_Attribute_Var) {
@@ -757,7 +757,7 @@ TEST_F(StructMemberAttributeTest, Size_Attribute_ConstNegative) {
     Structure("mystruct", utils::Vector{Member(
                               "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'size' attribute must be positive)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @size must be a positive integer)");
 }
 
 TEST_F(StructMemberAttributeTest, Size_Attribute_ConstF32) {
@@ -766,7 +766,7 @@ TEST_F(StructMemberAttributeTest, Size_Attribute_ConstF32) {
     Structure("mystruct", utils::Vector{Member(
                               "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'size' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @size must be an i32 or u32 value)");
 }
 
 TEST_F(StructMemberAttributeTest, Size_Attribute_ConstU32) {
@@ -791,7 +791,7 @@ TEST_F(StructMemberAttributeTest, Size_Attribute_ConstAFloat) {
     Structure("mystruct", utils::Vector{Member(
                               "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'size' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @size must be an i32 or u32 value)");
 }
 
 TEST_F(StructMemberAttributeTest, Size_Attribute_Var) {

src/tint/resolver/resolver.cc

@@ -2837,112 +2837,128 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
         std::optional<uint32_t> location;
         for (auto* attr : member->attributes) {
             Mark(attr);
-            if (auto* o = attr->As<ast::StructMemberOffsetAttribute>()) {
+            bool ok = Switch(
-                // Offset attributes are not part of the WGSL spec, but are emitted
+                attr,  //
-                // by the SPIR-V reader.
+                [&](const ast::StructMemberOffsetAttribute* o) {
+                    // Offset attributes are not part of the WGSL spec, but are emitted
+                    // by the SPIR-V reader.
+                    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant,
+                                                       "@offset value"};
+                    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
 
-                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant,
+                    auto* materialized = Materialize(Expression(o->expr));
-                                                   "@offset value"};
+                    if (!materialized) {
-                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+                        return false;
-
-                auto* materialized = Materialize(Expression(o->expr));
-                if (!materialized) {
-                    return nullptr;
-                }
-                auto const_value = materialized->ConstantValue();
-                if (!const_value) {
-                    AddError("'offset' must be constant expression", o->expr->source);
-                    return nullptr;
-                }
-                offset = const_value->As<uint64_t>();
-
-                if (offset < struct_size) {
-                    AddError("offsets must be in ascending order", o->source);
-                    return nullptr;
-                }
-                align = 1;
-                has_offset_attr = true;
-            } else if (auto* a = attr->As<ast::StructMemberAlignAttribute>()) {
-                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@align"};
-                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
-
-                auto* materialized = Materialize(Expression(a->expr));
-                if (!materialized) {
-                    return nullptr;
-                }
-                if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
-                    AddError("'align' must be an i32 or u32 value", a->source);
-                    return nullptr;
-                }
-
-                auto const_value = materialized->ConstantValue();
-                if (!const_value) {
-                    AddError("'align' must be constant expression", a->source);
-                    return nullptr;
-                }
-                auto value = const_value->As<AInt>();
-
-                if (value <= 0 || !utils::IsPowerOfTwo(value)) {
-                    AddError("'align' value must be a positive, power-of-two integer", a->source);
-                    return nullptr;
-                }
-                align = u32(value);
-                has_align_attr = true;
-            } else if (auto* s = attr->As<ast::StructMemberSizeAttribute>()) {
-                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@size"};
-                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
-
-                auto* materialized = Materialize(Expression(s->expr));
-                if (!materialized) {
-                    return nullptr;
-                }
-                if (!materialized->Type()->IsAnyOf<sem::U32, sem::I32>()) {
-                    AddError("'size' must be an i32 or u32 value", s->source);
-                    return nullptr;
-                }
-
-                auto const_value = materialized->ConstantValue();
-                if (!const_value) {
-                    AddError("'size' must be constant expression", s->expr->source);
-                    return nullptr;
-                }
-                {
-                    auto value = const_value->As<AInt>();
-                    if (value <= 0) {
-                        AddError("'size' attribute must be positive", s->source);
-                        return nullptr;
                     }
-                }
+                    auto const_value = materialized->ConstantValue();
-                auto value = const_value->As<uint64_t>();
+                    if (!const_value) {
-                if (value < size) {
+                        AddError("@offset must be constant expression", o->expr->source);
-                    AddError("'size' must be at least as big as the type's size (" +
+                        return false;
-                                 std::to_string(size) + ")",
+                    }
-                             s->source);
+                    offset = const_value->As<uint64_t>();
-                    return nullptr;
-                }
-                size = u32(value);
-                has_size_attr = true;
-            } else if (auto* l = attr->As<ast::LocationAttribute>()) {
-                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@location"};
-                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
 
-                auto* materialize = Materialize(Expression(l->expr));
+                    if (offset < struct_size) {
-                if (!materialize) {
+                        AddError("offsets must be in ascending order", o->source);
-                    return nullptr;
+                        return false;
-                }
+                    }
-                auto* c = materialize->ConstantValue();
+                    align = 1;
-                if (!c) {
+                    has_offset_attr = true;
-                    // TODO(crbug.com/tint/1633): Add error message about invalid materialization
+                    return true;
-                    // when location can be an expression.
+                },
-                    return nullptr;
+                [&](const ast::StructMemberAlignAttribute* a) {
-                }
+                    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@align"};
-                location = c->As<uint32_t>();
+                    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
+                    auto* materialized = Materialize(Expression(a->expr));
+                    if (!materialized) {
+                        return false;
+                    }
+                    if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
+                        AddError("@align must be an i32 or u32 value", a->source);
+                        return false;
+                    }
+
+                    auto const_value = materialized->ConstantValue();
+                    if (!const_value) {
+                        AddError("@align must be constant expression", a->source);
+                        return false;
+                    }
+                    auto value = const_value->As<AInt>();
+
+                    if (value <= 0 || !utils::IsPowerOfTwo(value)) {
+                        AddError("@align value must be a positive, power-of-two integer",
+                                 a->source);
+                        return false;
+                    }
+                    align = u32(value);
+                    has_align_attr = true;
+                    return true;
+                },
+                [&](const ast::StructMemberSizeAttribute* s) {
+                    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@size"};
+                    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
+                    auto* materialized = Materialize(Expression(s->expr));
+                    if (!materialized) {
+                        return false;
+                    }
+                    if (!materialized->Type()->IsAnyOf<sem::U32, sem::I32>()) {
+                        AddError("@size must be an i32 or u32 value", s->source);
+                        return false;
+                    }
+
+                    auto const_value = materialized->ConstantValue();
+                    if (!const_value) {
+                        AddError("@size must be constant expression", s->expr->source);
+                        return false;
+                    }
+                    {
+                        auto value = const_value->As<AInt>();
+                        if (value <= 0) {
+                            AddError("@size must be a positive integer", s->source);
+                            return false;
+                        }
+                    }
+                    auto value = const_value->As<uint64_t>();
+                    if (value < size) {
+                        AddError("@size must be at least as big as the type's size (" +
+                                     std::to_string(size) + ")",
+                                 s->source);
+                        return false;
+                    }
+                    size = u32(value);
+                    has_size_attr = true;
+                    return true;
+                },
+                [&](const ast::LocationAttribute* l) {
+                    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant,
+                                                       "@location"};
+                    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
+                    auto* materialize = Materialize(Expression(l->expr));
+                    if (!materialize) {
+                        return false;
+                    }
+                    auto* c = materialize->ConstantValue();
+                    if (!c) {
+                        // TODO(crbug.com/tint/1633): Add error message about invalid
+                        // materialization when location can be an expression.
+                        return false;
+                    }
+                    location = c->As<uint32_t>();
+                    return true;
+                },
+                [&](Default) {
+                    // The validator will check attributes can be applied to the struct member.
+                    return true;
+                });
+            if (!ok) {
+                return nullptr;
             }
         }
 
         if (has_offset_attr && (has_align_attr || has_size_attr)) {
-            AddError("offset attributes cannot be used with align or size attributes",
+            AddError("@offset cannot be used with @align or @size", member->source);
-                     member->source);
             return nullptr;
         }
 

src/tint/resolver/validation_test.cc

@@ -1243,7 +1243,7 @@ TEST_F(ResolverValidationTest, NegativeStructMemberAlignAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), "12:34 error: 'align' value must be a positive, power-of-two integer");
+    EXPECT_EQ(r()->error(), "12:34 error: @align value must be a positive, power-of-two integer");
 }
 
 TEST_F(ResolverValidationTest, NonPOTStructMemberAlignAttribute) {
@@ -1252,7 +1252,7 @@ TEST_F(ResolverValidationTest, NonPOTStructMemberAlignAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), "12:34 error: 'align' value must be a positive, power-of-two integer");
+    EXPECT_EQ(r()->error(), "12:34 error: @align value must be a positive, power-of-two integer");
 }
 
 TEST_F(ResolverValidationTest, ZeroStructMemberAlignAttribute) {
@@ -1261,7 +1261,7 @@ TEST_F(ResolverValidationTest, ZeroStructMemberAlignAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), "12:34 error: 'align' value must be a positive, power-of-two integer");
+    EXPECT_EQ(r()->error(), "12:34 error: @align value must be a positive, power-of-two integer");
 }
 
 TEST_F(ResolverValidationTest, ZeroStructMemberSizeAttribute) {
@@ -1270,7 +1270,7 @@ TEST_F(ResolverValidationTest, ZeroStructMemberSizeAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), "12:34 error: 'size' must be at least as big as the type's size (4)");
+    EXPECT_EQ(r()->error(), "12:34 error: @size must be at least as big as the type's size (4)");
 }
 
 TEST_F(ResolverValidationTest, OffsetAndSizeAttribute) {
@@ -1280,9 +1280,7 @@ TEST_F(ResolverValidationTest, OffsetAndSizeAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(),
+    EXPECT_EQ(r()->error(), "12:34 error: @offset cannot be used with @align or @size");
-              "12:34 error: offset attributes cannot be used with align or size "
-              "attributes");
 }
 
 TEST_F(ResolverValidationTest, OffsetAndAlignAttribute) {
@@ -1292,9 +1290,7 @@ TEST_F(ResolverValidationTest, OffsetAndAlignAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(),
+    EXPECT_EQ(r()->error(), "12:34 error: @offset cannot be used with @align or @size");
-              "12:34 error: offset attributes cannot be used with align or size "
-              "attributes");
 }
 
 TEST_F(ResolverValidationTest, OffsetAndAlignAndSizeAttribute) {
@@ -1304,9 +1300,7 @@ TEST_F(ResolverValidationTest, OffsetAndAlignAndSizeAttribute) {
                    });
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(),
+    EXPECT_EQ(r()->error(), "12:34 error: @offset cannot be used with @align or @size");
-              "12:34 error: offset attributes cannot be used with align or size "
-              "attributes");
 }
 
 TEST_F(ResolverTest, Expr_Constructor_Cast_Pointer) {