tint/resolver: Fix failures with no error

Invalid values to @binding(), @group() and @location() would fail resolving without an error diagnostic. This later triggers and ICE.

Refactor duplicate @location resolving in 4 places to a single method.

Canonicalize the diagnostic messages for attributes.

Remove a bunch of TODOs

Bug: chromium:1380212
Bug: tint:1633
Change-Id: Id2cc6ba4b807f12f350a2a31ef87fa0f185b64c3
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/108144
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>

src/tint/resolver/attribute_validation_test.cc

@@ -1606,12 +1606,22 @@ TEST_F(GroupAndBindingTest, Const_AInt) {
     EXPECT_TRUE(r()->Resolve()) << r()->error();
 }
 
+TEST_F(GroupAndBindingTest, Binding_NonConstant) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()),
+              Binding(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a))), Group(1_i));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: @binding requires a const-expression, but expression is a runtime-expression)");
+}
+
 TEST_F(GroupAndBindingTest, Binding_Negative) {
     GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()),
               Binding(Source{{12, 34}}, -2_i), Group(1_i));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' value must be non-negative)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @binding value must be non-negative)");
 }
 
 TEST_F(GroupAndBindingTest, Binding_F32) {
@@ -1619,7 +1629,7 @@ TEST_F(GroupAndBindingTest, Binding_F32) {
               Binding(Source{{12, 34}}, 2.0_f), Group(1_u));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @binding must be an i32 or u32 value)");
 }
 
 TEST_F(GroupAndBindingTest, Binding_AFloat) {
@@ -1627,7 +1637,17 @@ TEST_F(GroupAndBindingTest, Binding_AFloat) {
               Binding(Source{{12, 34}}, 2.0_a), Group(1_u));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @binding must be an i32 or u32 value)");
+}
+
+TEST_F(GroupAndBindingTest, Group_NonConstant) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(2_u),
+              Group(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a))));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: @group requires a const-expression, but expression is a runtime-expression)");
 }
 
 TEST_F(GroupAndBindingTest, Group_Negative) {
@@ -1635,7 +1655,7 @@ TEST_F(GroupAndBindingTest, Group_Negative) {
               Group(Source{{12, 34}}, -1_i));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' value must be non-negative)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @group value must be non-negative)");
 }
 
 TEST_F(GroupAndBindingTest, Group_F32) {
@@ -1643,7 +1663,7 @@ TEST_F(GroupAndBindingTest, Group_F32) {
               Group(Source{{12, 34}}, 1.0_f));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @group must be an i32 or u32 value)");
 }
 
 TEST_F(GroupAndBindingTest, Group_AFloat) {
@@ -1651,7 +1671,7 @@ TEST_F(GroupAndBindingTest, Group_AFloat) {
               Group(Source{{12, 34}}, 1.0_a));
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @group must be an i32 or u32 value)");
 }
 
 using IdTest = ResolverTest;
@@ -1671,24 +1691,125 @@ TEST_F(IdTest, Const_AInt) {
     EXPECT_TRUE(r()->Resolve()) << r()->error();
 }
 
+TEST_F(IdTest, NonConstant) {
+    Override("val", ty.f32(),
+             utils::Vector{Id(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a)))});
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: @id requires a const-expression, but expression is a runtime-expression)");
+}
+
 TEST_F(IdTest, Negative) {
     Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, -1_i)});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'id' value must be non-negative)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @id value must be non-negative)");
 }
 
 TEST_F(IdTest, F32) {
     Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1_f)});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @id must be an i32 or u32 value)");
 }
 
 TEST_F(IdTest, AFloat) {
     Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1.0_a)});
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)");
+    EXPECT_EQ(r()->error(), R"(12:34 error: @id must be an i32 or u32 value)");
 }
 
+enum class LocationAttributeType {
+    kEntryPointParameter,
+    kEntryPointReturnType,
+    kStructureMember,
+};
+
+struct LocationTest : ResolverTestWithParam<LocationAttributeType> {
+    void Build(const ast::Expression* location_value) {
+        switch (GetParam()) {
+            case LocationAttributeType::kEntryPointParameter:
+                Func("main",
+                     utils::Vector{Param(Source{{12, 34}}, "a", ty.i32(),
+                                         utils::Vector{
+                                             Location(Source{{12, 34}}, location_value),
+                                             Flat(),
+                                         })},
+                     ty.void_(), utils::Empty,
+                     utils::Vector{
+                         Stage(ast::PipelineStage::kFragment),
+                     });
+                return;
+            case LocationAttributeType::kEntryPointReturnType:
+                Func("main", utils::Empty, ty.f32(),
+                     utils::Vector{
+                         Return(1_a),
+                     },
+                     utils::Vector{
+                         Stage(ast::PipelineStage::kFragment),
+                     },
+                     utils::Vector{
+                         Location(Source{{12, 34}}, location_value),
+                     });
+                return;
+            case LocationAttributeType::kStructureMember:
+                Structure("S", utils::Vector{
+                                   Member("m", ty.f32(),
+                                          utils::Vector{
+                                              Location(Source{{12, 34}}, location_value),
+                                          }),
+                               });
+                return;
+        }
+    }
+};
+
+TEST_P(LocationTest, Const_I32) {
+    Build(Expr(0_i));
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_P(LocationTest, Const_U32) {
+    Build(Expr(0_u));
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_P(LocationTest, Const_AInt) {
+    Build(Expr(0_a));
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_P(LocationTest, NonConstant) {
+    Build(Construct(ty.u32(), Call(Source{{12, 34}}, "dpdx", 1_a)));
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: @location value requires a const-expression, but expression is a runtime-expression)");
+}
+
+TEST_P(LocationTest, Negative) {
+    Build(Expr(-1_a));
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: @location value must be non-negative)");
+}
+
+TEST_P(LocationTest, F32) {
+    Build(Expr(1_f));
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: @location must be an i32 or u32 value)");
+}
+
+TEST_P(LocationTest, AFloat) {
+    Build(Expr(1.0_a));
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: @location must be an i32 or u32 value)");
+}
+
+INSTANTIATE_TEST_SUITE_P(LocationTest,
+                         LocationTest,
+                         testing::Values(LocationAttributeType::kEntryPointParameter,
+                                         LocationAttributeType::kEntryPointReturnType,
+                                         LocationAttributeType::kStructureMember));
+
 }  // namespace
 }  // namespace InterpolateTests
 

src/tint/resolver/override_test.cc

@@ -91,7 +91,7 @@ TEST_F(ResolverOverrideTest, DuplicateIds) {
 
     EXPECT_FALSE(r()->Resolve());
 
-    EXPECT_EQ(r()->error(), R"(56:78 error: override IDs must be unique
+    EXPECT_EQ(r()->error(), R"(56:78 error: @id values must be unique
 12:34 note: a override with an ID of 7 was previously declared here:)");
 }
 
@@ -100,7 +100,7 @@ TEST_F(ResolverOverrideTest, IdTooLarge) {
 
     EXPECT_FALSE(r()->Resolve());
 
-    EXPECT_EQ(r()->error(), "12:34 error: override IDs must be between 0 and 65535");
+    EXPECT_EQ(r()->error(), "12:34 error: @id value must be between 0 and 65535");
 }
 
 TEST_F(ResolverOverrideTest, F16_TemporallyBan) {

src/tint/resolver/resolver.cc

@@ -461,18 +461,18 @@ sem::Variable* Resolver::Override(const ast::Override* v) {
             return nullptr;
         }
         if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
-            AddError("'id' must be an i32 or u32 value", id_attr->source);
+            AddError("@id must be an i32 or u32 value", id_attr->source);
             return nullptr;
         }
 
         auto const_value = materialized->ConstantValue();
         auto value = const_value->As<AInt>();
         if (value < 0) {
-            AddError("'id' value must be non-negative", id_attr->source);
+            AddError("@id value must be non-negative", id_attr->source);
             return nullptr;
         }
         if (value > std::numeric_limits<decltype(OverrideId::value)>::max()) {
-            AddError("override IDs must be between 0 and " +
+            AddError("@id value must be between 0 and " +
                          std::to_string(std::numeric_limits<decltype(OverrideId::value)>::max()),
                      id_attr->source);
             return nullptr;
@@ -638,14 +638,14 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
                     return nullptr;
                 }
                 if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
-                    AddError("'binding' must be an i32 or u32 value", attr->source);
+                    AddError("@binding must be an i32 or u32 value", attr->source);
                     return nullptr;
                 }
 
                 auto const_value = materialized->ConstantValue();
                 auto value = const_value->As<AInt>();
                 if (value < 0) {
-                    AddError("'binding' value must be non-negative", attr->source);
+                    AddError("@binding value must be non-negative", attr->source);
                     return nullptr;
                 }
                 binding = u32(value);
@@ -662,14 +662,14 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
                     return nullptr;
                 }
                 if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
-                    AddError("'group' must be an i32 or u32 value", attr->source);
+                    AddError("@group must be an i32 or u32 value", attr->source);
                     return nullptr;
                 }
 
                 auto const_value = materialized->ConstantValue();
                 auto value = const_value->As<AInt>();
                 if (value < 0) {
-                    AddError("'group' value must be non-negative", attr->source);
+                    AddError("@group value must be non-negative", attr->source);
                     return nullptr;
                 }
                 group = u32(value);
@@ -679,22 +679,11 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
 
         std::optional<uint32_t> location;
         if (auto* attr = ast::GetAttribute<ast::LocationAttribute>(var->attributes)) {
-            auto* materialized = Materialize(Expression(attr->expr));
+            auto value = LocationAttribute(attr);
-            if (!materialized) {
+            if (!value) {
                 return nullptr;
             }
-            if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
+            location = value.Get();
-                AddError("'location' must be an i32 or u32 value", attr->source);
-                return nullptr;
-            }
-
-            auto const_value = materialized->ConstantValue();
-            auto value = const_value->As<AInt>();
-            if (value < 0) {
-                AddError("'location' value must be non-negative", attr->source);
-                return nullptr;
-            }
-            location = u32(value);
         }
 
         sem = builder_->create<sem::GlobalVariable>(
@@ -748,48 +737,36 @@ sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index)
     sem::BindingPoint binding_point;
     if (param->HasBindingPoint()) {
         {
+            ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@binding value"};
+            TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
             auto* attr = ast::GetAttribute<ast::BindingAttribute>(param->attributes);
-            auto* materialize = Materialize(Expression(attr->expr));
+            auto* materialized = Materialize(Expression(attr->expr));
-            if (!materialize) {
+            if (!materialized) {
                 return nullptr;
             }
-            auto* c = materialize->ConstantValue();
+            binding_point.binding = materialized->ConstantValue()->As<uint32_t>();
-            if (!c) {
-                // TODO(crbug.com/tint/1633): Add error message about invalid materialization when
-                // binding can be an expression.
-                return nullptr;
-            }
-            binding_point.binding = c->As<uint32_t>();
         }
         {
+            ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@group value"};
+            TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
             auto* attr = ast::GetAttribute<ast::GroupAttribute>(param->attributes);
-            auto* materialize = Materialize(Expression(attr->expr));
+            auto* materialized = Materialize(Expression(attr->expr));
-            if (!materialize) {
+            if (!materialized) {
                 return nullptr;
             }
-            auto* c = materialize->ConstantValue();
+            binding_point.group = materialized->ConstantValue()->As<uint32_t>();
-            if (!c) {
-                // TODO(crbug.com/tint/1633): Add error message about invalid materialization when
-                // binding can be an expression.
-                return nullptr;
-            }
-            binding_point.group = c->As<uint32_t>();
         }
     }
 
     std::optional<uint32_t> location;
-    if (auto* l = ast::GetAttribute<ast::LocationAttribute>(param->attributes)) {
+    if (auto* attr = ast::GetAttribute<ast::LocationAttribute>(param->attributes)) {
-        auto* materialize = Materialize(Expression(l->expr));
+        auto value = LocationAttribute(attr);
-        if (!materialize) {
+        if (!value) {
             return nullptr;
         }
-        auto* c = materialize->ConstantValue();
+        location = value.Get();
-        if (!c) {
-            // TODO(crbug.com/tint/1633): Add error message about invalid materialization when
-            // location can be an expression.
-            return nullptr;
-        }
-        location = c->As<uint32_t>();
     }
 
     auto* sem = builder_->create<sem::Parameter>(
@@ -799,6 +776,30 @@ sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index)
     return sem;
 }
 
+utils::Result<uint32_t> Resolver::LocationAttribute(const ast::LocationAttribute* attr) {
+    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@location value"};
+    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
+    auto* materialized = Materialize(Expression(attr->expr));
+    if (!materialized) {
+        return utils::Failure;
+    }
+
+    if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
+        AddError("@location must be an i32 or u32 value", attr->source);
+        return utils::Failure;
+    }
+
+    auto const_value = materialized->ConstantValue();
+    auto value = const_value->As<AInt>();
+    if (value < 0) {
+        AddError("@location value must be non-negative", attr->source);
+        return utils::Failure;
+    }
+
+    return static_cast<uint32_t>(value);
+}
+
 ast::Access Resolver::DefaultAccessForAddressSpace(ast::AddressSpace address_space) {
     // https://gpuweb.github.io/gpuweb/wgsl/#storage-class
     switch (address_space) {
@@ -984,18 +985,12 @@ sem::Function* Resolver::Function(const ast::Function* decl) {
     for (auto* attr : decl->return_type_attributes) {
         Mark(attr);
 
-        if (auto* a = attr->As<ast::LocationAttribute>()) {
+        if (auto* loc_attr = attr->As<ast::LocationAttribute>()) {
-            auto* materialize = Materialize(Expression(a->expr));
+            auto value = LocationAttribute(loc_attr);
-            if (!materialize) {
+            if (!value) {
                 return nullptr;
             }
-            auto* c = materialize->ConstantValue();
+            return_location = value.Get();
-            if (!c) {
-                // TODO(crbug.com/tint/1633): Add error message about invalid materialization when
-                // location can be an expression.
-                return nullptr;
-            }
-            return_location = c->As<uint32_t>();
         }
     }
     if (!validator_.NoDuplicateAttributes(decl->attributes)) {
@@ -2969,22 +2964,12 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
                     has_size_attr = true;
                     return true;
                 },
-                [&](const ast::LocationAttribute* l) {
+                [&](const ast::LocationAttribute* loc_attr) {
-                    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant,
+                    auto value = LocationAttribute(loc_attr);
-                                                       "@location"};
+                    if (!value) {
-                    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
-
-                    auto* materialize = Materialize(Expression(l->expr));
-                    if (!materialize) {
                         return false;
                     }
-                    auto* c = materialize->ConstantValue();
+                    location = value.Get();
-                    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) {

src/tint/resolver/resolver.h

@@ -349,6 +349,10 @@ class Resolver {
     /// @param index the index of the parameter
     sem::Parameter* Parameter(const ast::Parameter* param, uint32_t index);
 
+    /// @returns the location value for a `@location` attribute, validating the value's range and
+    /// type.
+    utils::Result<uint32_t> LocationAttribute(const ast::LocationAttribute* attr);
+
     /// Records the address space usage for the given type, and any transient
     /// dependencies of the type. Validates that the type can be used for the
     /// given address space, erroring if it cannot.

src/tint/resolver/validator.cc

@@ -797,7 +797,7 @@ bool Validator::Override(
         if (attr->Is<ast::IdAttribute>()) {
             auto id = v->OverrideId();
             if (auto it = override_ids.find(id); it != override_ids.end() && it->second != v) {
-                AddError("override IDs must be unique", attr->source);
+                AddError("@id values must be unique", attr->source);
                 AddNote("a override with an ID of " + std::to_string(id.value) +
                             " was previously declared here:",
                         ast::GetAttribute<ast::IdAttribute>(it->second->Declaration()->attributes)