Move assignment validation from Validator to Resolver
* With this change, ProgramBuilder::WrapInStatement(expr), which
produced an "expr = expr" assignment expression, would fail validation
in some cases like for call expressions. Replaced this with a
declaration of a variable with type inferred from expr.
* Moved existing validation tests to resolver\assignment_validation.cc,
and added missing tests: AssignFromPointer_Fail.
* Fixed broken tests as a result of this change.
Bug: tint:642
Change-Id: I6a738b67edb0e116a46e936d652c2dcb4389281e
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46161
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/program_builder.cc b/src/program_builder.cc
index bebb8be..ac01248 100644
--- a/src/program_builder.cc
+++ b/src/program_builder.cc
@@ -15,6 +15,7 @@
#include "src/program_builder.h"
#include "src/ast/assignment_statement.h"
+#include "src/ast/call_statement.h"
#include "src/ast/variable_decl_statement.h"
#include "src/demangler.h"
#include "src/semantic/expression.h"
@@ -126,9 +127,8 @@
}
ast::Statement* ProgramBuilder::WrapInStatement(ast::Expression* expr) {
- // TODO(ben-clayton): This is valid enough for the Resolver, but the LHS
- // may not be assignable, and so may not validate.
- return create<ast::AssignmentStatement>(expr, expr);
+ // Create a temporary variable of inferred type from expr.
+ return Decl(Var(symbols_.New(), nullptr, ast::StorageClass::kFunction, expr));
}
ast::Statement* ProgramBuilder::WrapInStatement(ast::Statement* stmt) {
diff --git a/src/resolver/assignment_validation_test.cc b/src/resolver/assignment_validation_test.cc
index ce975ee..f4e8489 100644
--- a/src/resolver/assignment_validation_test.cc
+++ b/src/resolver/assignment_validation_test.cc
@@ -16,6 +16,8 @@
#include "gmock/gmock.h"
#include "src/resolver/resolver_test_helper.h"
+#include "src/type/access_control_type.h"
+#include "src/type/storage_texture_type.h"
namespace tint {
namespace resolver {
@@ -139,6 +141,143 @@
R"(12:34 error: invalid assignment: cannot assign value of type 'f32' to a variable of type 'i32')");
}
+TEST_F(ResolverAssignmentValidationTest, AssignToScalar_Fail) {
+ // var my_var : i32 = 2;
+ // 1 = my_var;
+
+ auto* var = Var("my_var", ty.i32(), ast::StorageClass::kNone, Expr(2));
+ auto* lhs = Expr(1);
+ auto* rhs = Expr("my_var");
+
+ auto* assign = create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var), assign);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "12:34 error v-000x: invalid assignment: left-hand-side does not "
+ "reference storage: i32");
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignCompatibleTypes_Pass) {
+ // var a :i32 = 2;
+ // a = 2
+ auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+
+ auto* lhs = Expr("a");
+ auto* rhs = Expr(2);
+
+ auto* assign = create<ast::AssignmentStatement>(
+ Source{Source::Location{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var), assign);
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest,
+ AssignCompatibleTypesThroughAlias_Pass) {
+ // alias myint = i32;
+ // var a :myint = 2;
+ // a = 2
+ auto* myint = ty.alias("myint", ty.i32());
+ auto* var = Var("a", myint, ast::StorageClass::kNone, Expr(2));
+
+ auto* lhs = Expr("a");
+ auto* rhs = Expr(2);
+
+ auto* assign = create<ast::AssignmentStatement>(
+ Source{Source::Location{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var), assign);
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest,
+ AssignCompatibleTypesInferRHSLoad_Pass) {
+ // var a : i32 = 2;
+ // var b : i32 = 3;
+ // a = b;
+ auto* var_a = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+ auto* var_b = Var("b", ty.i32(), ast::StorageClass::kNone, Expr(3));
+
+ auto* lhs = Expr("a");
+ auto* rhs = Expr("b");
+
+ auto* assign = create<ast::AssignmentStatement>(
+ Source{Source::Location{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignThroughPointer_Pass) {
+ // var a :i32;
+ // const b : ptr<function,i32> = a;
+ // b = 2;
+ const auto func = ast::StorageClass::kFunction;
+ auto* var_a = Var("a", ty.i32(), func, Expr(2), {});
+ auto* var_b = Const("b", ty.pointer<int>(func), Expr("a"), {});
+
+ auto* lhs = Expr("b");
+ auto* rhs = Expr(2);
+
+ auto* assign = create<ast::AssignmentStatement>(
+ Source{Source::Location{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignToConstant_Fail) {
+ // {
+ // const a :i32 = 2;
+ // a = 2
+ // }
+ auto* var = Const("a", ty.i32(), Expr(2));
+
+ auto* lhs = Expr("a");
+ auto* rhs = Expr(2);
+
+ auto* body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(var),
+ create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
+ rhs),
+ });
+
+ WrapInFunction(body);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "12:34 error v-0021: cannot re-assign a constant: 'a'");
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignFromPointer_Fail) {
+ // var a : [[access(read)]] texture_storage_1d<rgba8unorm>;
+ // var b : [[access(read)]] texture_storage_1d<rgba8unorm>;
+ // a = b;
+
+ auto* tex_type = create<type::StorageTexture>(
+ type::TextureDimension::k1d, type::ImageFormat::kRgba8Unorm,
+ type::StorageTexture::SubtypeFor(type::ImageFormat::kRgba8Unorm,
+ Types()));
+ auto* tex_ac =
+ create<type::AccessControl>(ast::AccessControl::kReadOnly, tex_type);
+
+ auto* var_a = Var("a", tex_ac, ast::StorageClass::kFunction);
+ auto* var_b = Var("b", tex_ac, ast::StorageClass::kFunction);
+
+ auto* lhs = Expr("a");
+ auto* rhs = Expr("b");
+
+ auto* assign = create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs);
+ WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "12:34 error v-000x: invalid assignment: right-hand-side is not "
+ "storable: ptr<function, [[access(read)]] "
+ "texture_storage_1d<rgba8unorm>>");
+}
+
} // namespace
} // namespace resolver
} // namespace tint
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 38ed168..3c8dfcc 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -333,23 +333,7 @@
ScopedAssignment<semantic::Statement*> sa(current_statement_, sem_statement);
if (auto* a = stmt->As<ast::AssignmentStatement>()) {
- if (!Expression(a->lhs()) || !Expression(a->rhs())) {
- return false;
- }
- // TODO(crbug.com/tint/659): This logic needs updating once pointers are
- // pinned down in the WGSL spec.
- auto* lhs_type = TypeOf(a->lhs())->UnwrapAll();
- auto* rhs_type = TypeOf(a->rhs());
- if (!IsValidAssignment(lhs_type, rhs_type)) {
- diagnostics_.add_error(
- "invalid assignment: cannot assign value of type '" +
- rhs_type->FriendlyName(builder_->Symbols()) +
- "' to a variable of type '" +
- lhs_type->FriendlyName(builder_->Symbols()) + "'",
- stmt->source());
- return false;
- }
- return true;
+ return Assignment(a);
}
if (auto* b = stmt->As<ast::BlockStatement>()) {
return BlockStatement(b);
@@ -1770,6 +1754,74 @@
return true;
}
+bool Resolver::ValidateAssignment(const ast::AssignmentStatement* a) {
+ auto* lhs = a->lhs();
+ auto* rhs = a->rhs();
+
+ // TODO(crbug.com/tint/659): This logic needs updating once pointers are
+ // pinned down in the WGSL spec.
+ auto* lhs_type = TypeOf(lhs)->UnwrapAll();
+ auto* rhs_type = TypeOf(rhs);
+ if (!IsValidAssignment(lhs_type, rhs_type)) {
+ diagnostics_.add_error("invalid assignment: cannot assign value of type '" +
+ rhs_type->FriendlyName(builder_->Symbols()) +
+ "' to a variable of type '" +
+ lhs_type->FriendlyName(builder_->Symbols()) +
+ "'",
+ a->source());
+ return false;
+ }
+
+ // Pointers are not storable in WGSL, but the right-hand side must be
+ // storable. The raw right-hand side might be a pointer value which must be
+ // loaded (dereferenced) to provide the value to be stored.
+ auto* rhs_result_type = TypeOf(rhs)->UnwrapAll();
+ if (!IsStorable(rhs_result_type)) {
+ diagnostics_.add_error(
+ "v-000x",
+ "invalid assignment: right-hand-side is not storable: " +
+ TypeOf(rhs)->FriendlyName(builder_->Symbols()),
+ a->source());
+ return false;
+ }
+
+ // lhs must be a pointer or a constant
+ auto* lhs_result_type = TypeOf(lhs)->UnwrapIfNeeded();
+ if (!lhs_result_type->Is<type::Pointer>()) {
+ // In case lhs is a constant identifier, output a nicer message as it's
+ // likely to be a common programmer error.
+ if (auto* ident = lhs->As<ast::IdentifierExpression>()) {
+ VariableInfo* var;
+ if (variable_stack_.get(ident->symbol(), &var) &&
+ var->declaration->is_const()) {
+ diagnostics_.add_error(
+ "v-0021",
+ "cannot re-assign a constant: '" +
+ builder_->Symbols().NameFor(ident->symbol()) + "'",
+ a->source());
+ return false;
+ }
+ }
+
+ // Issue a generic error.
+ diagnostics_.add_error(
+ "v-000x",
+ "invalid assignment: left-hand-side does not reference storage: " +
+ TypeOf(lhs)->FriendlyName(builder_->Symbols()),
+ a->source());
+ return false;
+ }
+
+ return true;
+}
+
+bool Resolver::Assignment(ast::AssignmentStatement* a) {
+ if (!Expression(a->lhs()) || !Expression(a->rhs())) {
+ return false;
+ }
+ return ValidateAssignment(a);
+}
+
bool Resolver::ApplyStorageClassUsageToType(ast::StorageClass sc,
type::Type* ty,
Source usage) {
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index 3f39d73..2aa09874 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -223,6 +223,7 @@
bool VariableDeclStatement(const ast::VariableDeclStatement*);
bool Return(ast::ReturnStatement* ret);
bool Switch(ast::SwitchStatement* s);
+ bool Assignment(ast::AssignmentStatement* a);
// AST and Type validation methods
// Each return true on success, false on failure.
@@ -232,6 +233,7 @@
bool ValidateStructure(const type::Struct* st);
bool ValidateReturn(const ast::ReturnStatement* ret);
bool ValidateSwitch(const ast::SwitchStatement* s);
+ bool ValidateAssignment(const ast::AssignmentStatement* a);
/// @returns the semantic information for the array `arr`, building it if it
/// hasn't been constructed already. If an error is raised, nullptr is
diff --git a/src/resolver/resolver_test.cc b/src/resolver/resolver_test.cc
index 3aa69a5..fa0e933 100644
--- a/src/resolver/resolver_test.cc
+++ b/src/resolver/resolver_test.cc
@@ -639,14 +639,13 @@
TEST_F(ResolverTest, Expr_Identifier_FunctionVariable_Const) {
auto* my_var_a = Expr("my_var");
- auto* my_var_b = Expr("my_var");
auto* var = Const("my_var", ty.f32());
- auto* assign = create<ast::AssignmentStatement>(my_var_a, my_var_b);
+ auto* decl = Decl(Var("b", ty.f32(), ast::StorageClass::kFunction, my_var_a));
Func("my_func", ast::VariableList{}, ty.void_(),
ast::StatementList{
create<ast::VariableDeclStatement>(var),
- assign,
+ decl,
},
ast::DecorationList{});
@@ -654,11 +653,8 @@
ASSERT_NE(TypeOf(my_var_a), nullptr);
EXPECT_TRUE(TypeOf(my_var_a)->Is<type::F32>());
- EXPECT_EQ(StmtOf(my_var_a), assign);
- ASSERT_NE(TypeOf(my_var_b), nullptr);
- EXPECT_TRUE(TypeOf(my_var_b)->Is<type::F32>());
- EXPECT_EQ(StmtOf(my_var_b), assign);
- EXPECT_TRUE(CheckVarUsers(var, {my_var_a, my_var_b}));
+ EXPECT_EQ(StmtOf(my_var_a), decl);
+ EXPECT_TRUE(CheckVarUsers(var, {my_var_a}));
}
TEST_F(ResolverTest, Expr_Identifier_FunctionVariable) {
diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc
index a00c5fa..b817892 100644
--- a/src/validator/validator_impl.cc
+++ b/src/validator/validator_impl.cc
@@ -238,9 +238,6 @@
if (auto* v = stmt->As<ast::VariableDeclStatement>()) {
return ValidateDeclStatement(v);
}
- if (auto* a = stmt->As<ast::AssignmentStatement>()) {
- return ValidateAssign(a);
- }
if (auto* s = stmt->As<ast::SwitchStatement>()) {
return ValidateSwitch(s);
}
@@ -272,66 +269,6 @@
return true;
}
-bool ValidatorImpl::ValidateBadAssignmentToIdentifier(
- const ast::AssignmentStatement* assign) {
- auto* ident = assign->lhs()->As<ast::IdentifierExpression>();
- if (!ident) {
- // It wasn't an identifier in the first place.
- return true;
- }
- const ast::Variable* var;
- if (variable_stack_.get(ident->symbol(), &var)) {
- // Give a nicer message if the LHS of the assignment is a const identifier.
- // It's likely to be a common programmer error.
- if (var->is_const()) {
- add_error(assign->source(), "v-0021",
- "cannot re-assign a constant: '" +
- program_->Symbols().NameFor(ident->symbol()) + "'");
- return false;
- }
- } else {
- // The identifier is not defined.
- // Shouldn't reach here. This should already have been caught when
- // validation expressions in the Resolver
- TINT_UNREACHABLE(diagnostics());
- return false;
- }
- return true;
-}
-
-bool ValidatorImpl::ValidateAssign(const ast::AssignmentStatement* assign) {
- if (!assign) {
- return false;
- }
- auto* lhs = assign->lhs();
- auto* rhs = assign->rhs();
-
- // Pointers are not storable in WGSL, but the right-hand side must be
- // storable. The raw right-hand side might be a pointer value which must be
- // loaded (dereferenced) to provide the value to be stored.
- auto* rhs_result_type = program_->Sem().Get(rhs)->Type()->UnwrapAll();
- if (!IsStorable(rhs_result_type)) {
- add_error(assign->source(), "v-000x",
- "invalid assignment: right-hand-side is not storable: " +
- program_->Sem().Get(rhs)->Type()->type_name());
- return false;
- }
- auto* lhs_result_type = program_->Sem().Get(lhs)->Type()->UnwrapIfNeeded();
- if (!Is<type::Pointer>(lhs_result_type)) {
- if (!ValidateBadAssignmentToIdentifier(assign)) {
- return false;
- }
- // Issue a generic error.
- add_error(
- assign->source(), "v-000x",
- "invalid assignment: left-hand-side does not reference storage: " +
- program_->Sem().Get(lhs)->Type()->type_name());
- return false;
- }
-
- return true;
-}
-
bool ValidatorImpl::IsStorable(type::Type* type) {
if (type == nullptr) {
return false;
diff --git a/src/validator/validator_impl.h b/src/validator/validator_impl.h
index 04edf95..e84de4e 100644
--- a/src/validator/validator_impl.h
+++ b/src/validator/validator_impl.h
@@ -84,16 +84,6 @@
/// @param stmt the statement to check
/// @returns true if the validation was successful
bool ValidateStatement(const ast::Statement* stmt);
- /// Validates an assignment
- /// @param assign the assignment to check
- /// @returns true if the validation was successful
- bool ValidateAssign(const ast::AssignmentStatement* assign);
- /// Validates a bad assignment to an identifier. Issues an error
- /// and returns false if the left hand side is an identifier.
- /// @param assign the assignment to check
- /// @returns true if the LHS of theassignment is not an identifier expression
- bool ValidateBadAssignmentToIdentifier(
- const ast::AssignmentStatement* assign);
/// Validates declaration name uniqueness
/// @param decl is the new declaration to be added
/// @returns true if no previous declaration with the `decl` 's name
diff --git a/src/validator/validator_test.cc b/src/validator/validator_test.cc
index b5d8dba..1e3b5c7 100644
--- a/src/validator/validator_test.cc
+++ b/src/validator/validator_test.cc
@@ -21,123 +21,6 @@
class ValidatorTest : public ValidatorTestHelper, public testing::Test {};
-TEST_F(ValidatorTest, AssignToScalar_Fail) {
- // var my_var : i32 = 2;
- // 1 = my_var;
-
- auto* var = Var("my_var", ty.i32(), ast::StorageClass::kNone, Expr(2));
- RegisterVariable(var);
-
- auto* lhs = Expr(1);
- auto* rhs = Expr("my_var");
-
- SetSource(Source{Source::Location{12, 34}});
- auto* assign = create<ast::AssignmentStatement>(lhs, rhs);
- WrapInFunction(assign);
-
- ValidatorImpl& v = Build();
-
- // TODO(sarahM0): Invalidate assignment to scalar.
- EXPECT_FALSE(v.ValidateAssign(assign));
- ASSERT_TRUE(v.has_error());
- // TODO(sarahM0): figure out what should be the error number.
- EXPECT_EQ(v.error(),
- "12:34 v-000x: invalid assignment: left-hand-side does not "
- "reference storage: __i32");
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypes_Pass) {
- // var a :i32 = 2;
- // a = 2
- auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- RegisterVariable(var);
-
- auto* lhs = Expr("a");
- auto* rhs = Expr(2);
-
- auto* assign = create<ast::AssignmentStatement>(
- Source{Source::Location{12, 34}}, lhs, rhs);
- WrapInFunction(assign);
-
- ValidatorImpl& v = Build();
-
- ASSERT_NE(TypeOf(lhs), nullptr);
- ASSERT_NE(TypeOf(rhs), nullptr);
-
- EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypesThroughAlias_Pass) {
- // alias myint = i32;
- // var a :myint = 2;
- // a = 2
- auto* myint = ty.alias("myint", ty.i32());
- auto* var = Var("a", myint, ast::StorageClass::kNone, Expr(2));
- RegisterVariable(var);
-
- auto* lhs = Expr("a");
- auto* rhs = Expr(2);
-
- auto* assign = create<ast::AssignmentStatement>(
- Source{Source::Location{12, 34}}, lhs, rhs);
- WrapInFunction(assign);
-
- ValidatorImpl& v = Build();
-
- ASSERT_NE(TypeOf(lhs), nullptr);
- ASSERT_NE(TypeOf(rhs), nullptr);
-
- EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypesInferRHSLoad_Pass) {
- // var a :i32 = 2;
- // var b :i32 = 3;
- // a = b;
- auto* var_a = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
- auto* var_b = Var("b", ty.i32(), ast::StorageClass::kNone, Expr(3));
- RegisterVariable(var_a);
- RegisterVariable(var_b);
-
- auto* lhs = Expr("a");
- auto* rhs = Expr("b");
-
- auto* assign = create<ast::AssignmentStatement>(
- Source{Source::Location{12, 34}}, lhs, rhs);
- WrapInFunction(assign);
-
- ValidatorImpl& v = Build();
-
- ASSERT_NE(TypeOf(lhs), nullptr);
- ASSERT_NE(TypeOf(rhs), nullptr);
-
- EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignThroughPointer_Pass) {
- // var a :i32;
- // const b : ptr<function,i32> = a;
- // b = 2;
- const auto func = ast::StorageClass::kFunction;
- auto* var_a = Var("a", ty.i32(), func, Expr(2), {});
- auto* var_b = Const("b", ty.pointer<int>(func), Expr("a"), {});
- RegisterVariable(var_a);
- RegisterVariable(var_b);
-
- auto* lhs = Expr("b");
- auto* rhs = Expr(2);
-
- auto* assign = create<ast::AssignmentStatement>(
- Source{Source::Location{12, 34}}, lhs, rhs);
- WrapInFunction(assign);
-
- ValidatorImpl& v = Build();
-
- ASSERT_NE(TypeOf(lhs), nullptr);
- ASSERT_NE(TypeOf(rhs), nullptr);
-
- EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
TEST_F(ValidatorTest, GlobalVariableWithStorageClass_Pass) {
// var<in> global_var: f32;
@@ -215,33 +98,6 @@
"12:34 v-0011: redeclared global identifier 'global_var'");
}
-TEST_F(ValidatorTest, AssignToConstant_Fail) {
- // {
- // const a :i32 = 2;
- // a = 2
- // }
- auto* var = Const("a", ty.i32(), Expr(2));
-
- auto* lhs = Expr("a");
- auto* rhs = Expr(2);
-
- auto* body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(var),
- create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
- rhs),
- });
-
- WrapInFunction(body);
-
- ValidatorImpl& v = Build();
-
- ASSERT_NE(TypeOf(lhs), nullptr);
- ASSERT_NE(TypeOf(rhs), nullptr);
-
- EXPECT_FALSE(v.ValidateStatements(body));
- EXPECT_EQ(v.error(), "12:34 v-0021: cannot re-assign a constant: 'a'");
-}
-
TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Pass) {
// fn my_func -> void {
// var a: f32 = 2.0;
