tint/resolver: Improve errors for expr eval-stages

Raise the error on the inner-most expression that violates the required evaluation stage.

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

src/tint/reader/wgsl/parser_impl_texture_sampler_test.cc

@@ -239,7 +239,8 @@ TEST_F(ParserImplTest, TextureSamplerTypes_StorageTexture_InvalidAccess) {
     EXPECT_EQ(t.value, nullptr);
     EXPECT_FALSE(t.matched);
     EXPECT_TRUE(t.errored);
-    EXPECT_EQ(p->error(), R"(1:30: expected access control for storage texture type. Did you mean 'read'?
+    EXPECT_EQ(p->error(),
+              R"(1:30: expected access control for storage texture type. Did you mean 'read'?
 Possible values: 'read', 'read_write', 'write')");
 }
 

src/tint/resolver/attribute_validation_test.cc

@@ -738,7 +738,9 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_Override) {
     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"(error: @align requires a const-expression, but expression is an override-expression)");
 }
 
 }  // namespace StructAndStructMemberTests

src/tint/resolver/builtin_test.cc

@@ -67,7 +67,9 @@ TEST_F(ResolverBuiltinTest, ModuleScopeUsage) {
 
     // TODO(crbug.com/tint/1581): Once 'abs' is implemented as @const, this will no longer be an
     // error.
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'const' initializer must be const-expression)");
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)");
 }
 
 // Tests for Logical builtins

src/tint/resolver/resolver.cc

@@ -416,6 +416,8 @@ sem::Variable* Resolver::Override(const ast::Override* v) {
 
     // Does the variable have a constructor?
     if (v->constructor) {
+        ExprEvalStageConstraint constraint{sem::EvaluationStage::kOverride, "override initializer"};
+        TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
         rhs = Materialize(Expression(v->constructor), ty);
         if (!rhs) {
             return nullptr;
@@ -453,8 +455,8 @@ sem::Variable* Resolver::Override(const ast::Override* v) {
         }
         auto* c = materialize->ConstantValue();
         if (!c) {
-            // TODO(crbug.com/tint/1633): Handle invalid materialization when expressions
+            // TODO(crbug.com/tint/1633): Handle invalid materialization when expressions are
-            // are supported.
+            // supported.
             return nullptr;
         }
 
@@ -493,10 +495,15 @@ sem::Variable* Resolver::Const(const ast::Const* c, bool is_global) {
         return nullptr;
     }
 
-    const auto* rhs = Expression(c->constructor);
+    const sem::Expression* rhs = nullptr;
+    {
+        ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "const initializer"};
+        TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+        rhs = Expression(c->constructor);
         if (!rhs) {
             return nullptr;
         }
+    }
 
     if (ty) {
         // If an explicit type was specified, materialize to that type
@@ -509,12 +516,6 @@ sem::Variable* Resolver::Const(const ast::Const* c, bool is_global) {
         ty = rhs->Type();
     }
 
-    const auto value = rhs->ConstantValue();
-    if (!value) {
-        AddError("'const' initializer must be const-expression", c->constructor->source);
-        return nullptr;
-    }
-
     if (!validator_.VariableInitializer(c, ast::AddressSpace::kNone, ty, rhs)) {
         return nullptr;
     }
@@ -525,6 +526,7 @@ sem::Variable* Resolver::Const(const ast::Const* c, bool is_global) {
         return nullptr;
     }
 
+    const auto value = rhs->ConstantValue();
     auto* sem = is_global ? static_cast<sem::Variable*>(builder_->create<sem::GlobalVariable>(
                                 c, ty, sem::EvaluationStage::kConstant, ast::AddressSpace::kNone,
                                 ast::Access::kUndefined, value, sem::BindingPoint{}, std::nullopt))
@@ -552,6 +554,12 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
 
     // Does the variable have a constructor?
     if (var->constructor) {
+        ExprEvalStageConstraint constraint{
+            is_global ? sem::EvaluationStage::kOverride : sem::EvaluationStage::kRuntime,
+            "var initializer",
+        };
+        TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
         rhs = Materialize(Expression(var->constructor), storage_ty);
         if (!rhs) {
             return nullptr;
@@ -858,16 +866,13 @@ sem::GlobalVariable* Resolver::GlobalVariable(const ast::Variable* v) {
 }
 
 sem::Statement* Resolver::StaticAssert(const ast::StaticAssert* assertion) {
+    ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "static assertion"};
+    TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
     auto* expr = Expression(assertion->condition);
     if (!expr) {
         return nullptr;
     }
     auto* cond = expr->ConstantValue();
-    if (!cond) {
-        AddError("static assertion condition must be a const-expression",
-                 assertion->condition->source);
-        return nullptr;
-    }
     if (auto* ty = cond->Type(); !ty->Is<sem::Bool>()) {
         AddError(
             "static assertion condition must be a bool, got '" + builder_->FriendlyName(ty) + "'",
@@ -1458,6 +1463,13 @@ sem::Expression* Resolver::Expression(const ast::Expression* root) {
             return nullptr;
         }
 
+        if (auto* constraint = expr_eval_stage_constraint_.constraint) {
+            if (!validator_.EvaluationStage(sem_expr, expr_eval_stage_constraint_.stage,
+                                            constraint)) {
+                return nullptr;
+            }
+        }
+
         builder_->Sem().Add(expr, sem_expr);
         if (expr == root) {
             return sem_expr;
@@ -2818,13 +2830,17 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
                 // 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);
+
                 auto* materialized = Materialize(Expression(o->expr));
                 if (!materialized) {
                     return nullptr;
                 }
                 auto const_value = materialized->ConstantValue();
                 if (!const_value) {
-                    AddError("'offset' must be const-expression", o->expr->source);
+                    AddError("'offset' must be constant expression", o->expr->source);
                     return nullptr;
                 }
                 offset = const_value->As<uint64_t>();
@@ -2836,6 +2852,9 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
                 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;
@@ -2847,7 +2866,7 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
 
                 auto const_value = materialized->ConstantValue();
                 if (!const_value) {
-                    AddError("'align' must be const-expression", a->source);
+                    AddError("'align' must be constant expression", a->source);
                     return nullptr;
                 }
                 auto value = const_value->As<AInt>();
@@ -2856,16 +2875,19 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
                     AddError("'align' value must be a positive, power-of-two integer", a->source);
                     return nullptr;
                 }
-                align = const_value->As<u32>();
+                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;
                 }
                 auto const_value = materialized->ConstantValue();
                 if (!const_value) {
-                    AddError("'size' must be const-expression", s->expr->source);
+                    AddError("'size' must be constant expression", s->expr->source);
                     return nullptr;
                 }
                 auto value = const_value->As<uint64_t>();
@@ -2876,9 +2898,12 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
                              s->source);
                     return nullptr;
                 }
-                size = const_value->As<u32>();
+                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 (!materialize) {
                     return nullptr;

src/tint/resolver/resolver.h

@@ -414,6 +414,15 @@ class Resolver {
     using StructConstructorSig =
         utils::UnorderedKeyWrapper<std::tuple<const sem::Struct*, size_t, sem::EvaluationStage>>;
 
+    /// ExprEvalStageConstraint describes a constraint on when expressions can be evaluated.
+    struct ExprEvalStageConstraint {
+        /// The latest stage that the expression can be evaluated
+        sem::EvaluationStage stage = sem::EvaluationStage::kRuntime;
+        /// The 'thing' that is imposing the contraint. e.g. "var declaration"
+        /// If nullptr, then there is no constraint
+        const char* constraint = nullptr;
+    };
+
     ProgramBuilder* const builder_;
     diag::List& diagnostics_;
     ConstEval const_eval_;
@@ -425,10 +434,10 @@ class Resolver {
     std::vector<sem::Function*> entry_points_;
     std::unordered_map<const sem::Type*, const Source&> atomic_composite_info_;
     utils::Bitset<0> marked_;
+    ExprEvalStageConstraint expr_eval_stage_constraint_;
     std::unordered_map<OverrideId, const sem::Variable*> override_ids_;
     std::unordered_map<ArrayConstructorSig, sem::CallTarget*> array_ctors_;
     std::unordered_map<StructConstructorSig, sem::CallTarget*> struct_ctors_;
-
     sem::Function* current_function_ = nullptr;
     sem::Statement* current_statement_ = nullptr;
     sem::CompoundStatement* current_compound_statement_ = nullptr;

src/tint/resolver/static_assert_test.cc

@@ -88,7 +88,8 @@ TEST_F(ResolverStaticAssertTest, Local_NonConst) {
     WrapInFunction(StaticAssert(Expr(Source{{12, 34}}, "V")));
     EXPECT_FALSE(r()->Resolve());
     EXPECT_EQ(r()->error(),
-              "12:34 error: static assertion condition must be a const-expression");
+              "12:34 error: static assertion requires a const-expression, but expression is a "
+              "runtime-expression");
 }
 
 TEST_F(ResolverStaticAssertTest, Local_LessThan_Pass) {

src/tint/resolver/validator.cc

@@ -1391,6 +1391,30 @@ bool Validator::EntryPoint(const sem::Function* func, ast::PipelineStage stage)
     return true;
 }
 
+bool Validator::EvaluationStage(const sem::Expression* expr,
+                                sem::EvaluationStage latest_stage,
+                                std::string_view constraint) const {
+    if (expr->Stage() > latest_stage) {
+        auto stage_name = [](sem::EvaluationStage stage) -> std::string {
+            switch (stage) {
+                case sem::EvaluationStage::kRuntime:
+                    return "a runtime-expression";
+                case sem::EvaluationStage::kOverride:
+                    return "an override-expression";
+                case sem::EvaluationStage::kConstant:
+                    return "a const-expression";
+            }
+            return "<unknown>";
+        };
+
+        AddError(std::string(constraint) + " requires " + stage_name(latest_stage) +
+                     ", but expression is " + stage_name(expr->Stage()),
+                 expr->Declaration()->source);
+        return false;
+    }
+    return true;
+}
+
 bool Validator::Statements(utils::VectorRef<const ast::Statement*> stmts) const {
     for (auto* stmt : stmts) {
         if (!sem_.Get(stmt)->IsReachable()) {

src/tint/resolver/validator.h

@@ -25,6 +25,7 @@
 #include "src/tint/ast/pipeline_stage.h"
 #include "src/tint/program_builder.h"
 #include "src/tint/resolver/sem_helper.h"
+#include "src/tint/sem/evaluation_stage.h"
 #include "src/tint/source.h"
 
 // Forward declarations
@@ -209,6 +210,15 @@ class Validator {
     /// @returns true on success, false otherwise
     bool EntryPoint(const sem::Function* func, ast::PipelineStage stage) const;
 
+    /// Validates that the expression must not be evaluated any later than @p latest_stage
+    /// @param expr the expression to check
+    /// @param latest_stage the latest evaluation stage that the expression can be evaluated
+    /// @param constraint the 'thing' that is imposing the contraint. e.g. "var declaration"
+    /// @returns true if @p expr is evaluated in or before @p latest_stage, false otherwise
+    bool EvaluationStage(const sem::Expression* expr,
+                         sem::EvaluationStage latest_stage,
+                         std::string_view constraint) const;
+
     /// Validates a for loop
     /// @param stmt the for loop statement to validate
     /// @returns true on success, false otherwise

src/tint/resolver/variable_validation_test.cc

@@ -423,7 +423,9 @@ TEST_F(ResolverVariableValidationTest, ConstInitWithVar) {
     WrapInFunction(v, c);
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'const' initializer must be const-expression)");
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)");
 }
 
 TEST_F(ResolverVariableValidationTest, ConstInitWithOverride) {
@@ -432,7 +434,9 @@ TEST_F(ResolverVariableValidationTest, ConstInitWithOverride) {
     WrapInFunction(c);
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'const' initializer must be const-expression)");
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression)");
 }
 
 TEST_F(ResolverVariableValidationTest, ConstInitWithLet) {
@@ -441,7 +445,30 @@ TEST_F(ResolverVariableValidationTest, ConstInitWithLet) {
     WrapInFunction(l, c);
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(), R"(12:34 error: 'const' initializer must be const-expression)");
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)");
+}
+
+TEST_F(ResolverVariableValidationTest, ConstInitWithRuntimeExpr) {
+    // const c = clamp(2, dpdx(0.5), 3);
+    WrapInFunction(Const("c", Call("clamp", 2_a, Call(Source{{12, 34}}, "dpdx", 0.5_a), 3_a)));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)");
+}
+
+TEST_F(ResolverVariableValidationTest, ConstInitWithOverrideExpr) {
+    auto* o = Override("v", Expr(1_i));
+    auto* c = Const("c", Add(10_a, Expr(Source{{12, 34}}, o)));
+    WrapInFunction(c);
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(
+        r()->error(),
+        R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression)");
 }
 
 }  // namespace

src/tint/transform/robustness_test.cc

@@ -1311,7 +1311,8 @@ fn f() {
 }
 )";
 
-    auto* expect = R"(error: array size is an override-expression, when expected a constant-expression.
+    auto* expect =
+        R"(error: array size is an override-expression, when expected a constant-expression.
 Was the SubstituteOverride transform run?)";
 
     auto got = Run<Robustness>(src);