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>
diff --git a/src/tint/reader/wgsl/parser_impl_texture_sampler_test.cc b/src/tint/reader/wgsl/parser_impl_texture_sampler_test.cc
index ce31276..a13998a 100644
--- a/src/tint/reader/wgsl/parser_impl_texture_sampler_test.cc
+++ b/src/tint/reader/wgsl/parser_impl_texture_sampler_test.cc
@@ -239,7 +239,8 @@
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')");
}
diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc
index bd965f7..30615bb 100644
--- a/src/tint/resolver/attribute_validation_test.cc
+++ b/src/tint/resolver/attribute_validation_test.cc
@@ -738,7 +738,9 @@
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
diff --git a/src/tint/resolver/builtin_test.cc b/src/tint/resolver/builtin_test.cc
index 2917f4d..b28521d 100644
--- a/src/tint/resolver/builtin_test.cc
+++ b/src/tint/resolver/builtin_test.cc
@@ -67,7 +67,9 @@
// 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
diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc
index 85adaf8..b8ba0fe 100644
--- a/src/tint/resolver/resolver.cc
+++ b/src/tint/resolver/resolver.cc
@@ -416,6 +416,8 @@
// 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 @@
}
auto* c = materialize->ConstantValue();
if (!c) {
- // TODO(crbug.com/tint/1633): Handle invalid materialization when expressions
- // are supported.
+ // TODO(crbug.com/tint/1633): Handle invalid materialization when expressions are
+ // supported.
return nullptr;
}
@@ -493,9 +495,14 @@
return nullptr;
}
- const auto* rhs = Expression(c->constructor);
- if (!rhs) {
- return nullptr;
+ 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) {
@@ -509,12 +516,6 @@
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 @@
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 @@
// 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::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 @@
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 @@
// 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 @@
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 @@
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 @@
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 @@
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;
diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h
index 5c413f9..f699996 100644
--- a/src/tint/resolver/resolver.h
+++ b/src/tint/resolver/resolver.h
@@ -414,6 +414,15 @@
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 @@
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;
diff --git a/src/tint/resolver/static_assert_test.cc b/src/tint/resolver/static_assert_test.cc
index 6d8423d..6e8ecb7 100644
--- a/src/tint/resolver/static_assert_test.cc
+++ b/src/tint/resolver/static_assert_test.cc
@@ -88,7 +88,8 @@
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) {
diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc
index e8128bb..3066315 100644
--- a/src/tint/resolver/validator.cc
+++ b/src/tint/resolver/validator.cc
@@ -1391,6 +1391,30 @@
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()) {
diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h
index 2b84ad1..1297a9a 100644
--- a/src/tint/resolver/validator.h
+++ b/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 @@
/// @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
diff --git a/src/tint/resolver/variable_validation_test.cc b/src/tint/resolver/variable_validation_test.cc
index b1c7bac..65cfd51 100644
--- a/src/tint/resolver/variable_validation_test.cc
+++ b/src/tint/resolver/variable_validation_test.cc
@@ -423,7 +423,9 @@
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 @@
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 @@
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
diff --git a/src/tint/transform/robustness_test.cc b/src/tint/transform/robustness_test.cc
index fa04e47..fe2f343 100644
--- a/src/tint/transform/robustness_test.cc
+++ b/src/tint/transform/robustness_test.cc
@@ -1311,7 +1311,8 @@
}
)";
- 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);
