From 3b267173776b333540da587867521b97cdf9e0cd Mon Sep 17 00:00:00 2001 From: James Price Date: Wed, 9 Jun 2021 09:12:57 +0000 Subject: [PATCH] resolver: Storable types include textures/samplers Add a Resolver::IsPlain() method to check for plain types, which is then used instead of IsStorable() for validating array and struct subtypes. Remove validation of assignment and constructor RHS types, instead validating the type of the variable declaration. This catches additional errors that were previously missed, such as using a pointer for a var declaration with no constructor. Change-Id: I5786a262159d2a42cc05b44743c6c26f6b5647c0 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/53960 Auto-Submit: James Price Commit-Queue: Ben Clayton Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/ast/module_clone_test.cc | 2 +- src/resolver/assignment_validation_test.cc | 5 ++- src/resolver/resolver.cc | 49 +++++++++------------- src/resolver/resolver.h | 4 ++ src/resolver/var_let_validation_test.cc | 11 ++--- 5 files changed, 35 insertions(+), 36 deletions(-) diff --git a/src/ast/module_clone_test.cc b/src/ast/module_clone_test.cc index 37a1129d4a..e635d56fb0 100644 --- a/src/ast/module_clone_test.cc +++ b/src/ast/module_clone_test.cc @@ -66,7 +66,7 @@ fn f1(p0 : f32, p1 : i32) -> f32 { var l3 : vec2 = vec2(u32(l0), u32(l1)); var l4 : S; var l5 : u32 = l4.m1[5]; - var l6 : ptr; + let l6 : ptr = &g0; loop { l0 = (p1 + 2); if (((l0 % 4) == 0)) { diff --git a/src/resolver/assignment_validation_test.cc b/src/resolver/assignment_validation_test.cc index b908662735..b270ec9b76 100644 --- a/src/resolver/assignment_validation_test.cc +++ b/src/resolver/assignment_validation_test.cc @@ -155,7 +155,10 @@ TEST_F(ResolverAssignmentValidationTest, AssignToConstant_Fail) { EXPECT_EQ(r()->error(), "12:34 error: cannot assign to value of type 'i32'"); } -TEST_F(ResolverAssignmentValidationTest, AssignNonStorable_Fail) { +// TODO(crbug.com/tint/809): The var has an implicit access::read, and so this +// test will pass again when we start validating the access mode on the LHS of +// an assignment. +TEST_F(ResolverAssignmentValidationTest, DISABLED_AssignNonStorable_Fail) { // var a : texture_storage_1d; // var b : texture_storage_1d; // a = b; diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index ff4f455b40..fc25d8f05c 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -165,20 +165,18 @@ bool Resolver::Resolve() { return result; } -// https://gpuweb.github.io/gpuweb/wgsl.html#storable-types -bool Resolver::IsStorable(const sem::Type* type) { - if (type->is_scalar() || type->Is() || type->Is()) { +// https://gpuweb.github.io/gpuweb/wgsl/#plain-types-section +bool Resolver::IsPlain(const sem::Type* type) { + if (type->is_scalar() || type->Is() || type->Is() || + type->Is() || type->Is()) { return true; } - if (auto* arr = type->As()) { - return IsStorable(arr->ElemType()); - } - if (auto* str = type->As()) { - for (const auto* member : str->Members()) { - if (!IsStorable(member->Type())) { - return false; - } - } + return false; +} + +// https://gpuweb.github.io/gpuweb/wgsl.html#storable-types +bool Resolver::IsStorable(const sem::Type* type) { + if (IsPlain(type) || type->Is() || type->Is()) { return true; } return false; @@ -525,14 +523,6 @@ bool Resolver::ValidateVariableConstructor(const ast::Variable* var, const std::string& rhs_type_name) { auto* value_type = rhs_type->UnwrapRef(); // Implicit load of RHS - // RHS needs to be of a storable type - if (!var->is_const() && !IsStorable(value_type)) { - diagnostics_.add_error( - "'" + rhs_type_name + "' is not storable for assignment", - var->constructor()->source()); - return false; - } - // Value type has to match storage type if (storage_type != value_type) { std::string decl = var->is_const() ? "let" : "var"; @@ -769,6 +759,14 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) { bool Resolver::ValidateVariable(const VariableInfo* info) { auto* var = info->declaration; auto* storage_type = info->type->UnwrapRef(); + + if (!var->is_const() && !IsStorable(storage_type)) { + diagnostics_.add_error(storage_type->FriendlyName(builder_->Symbols()) + + " cannot be used as the type of a var", + var->source()); + return false; + } + if (auto* r = storage_type->As()) { if (r->IsRuntimeSized()) { diagnostics_.add_error( @@ -2750,7 +2748,7 @@ sem::Array* Resolver::Array(const ast::Array* arr) { return nullptr; } - if (!IsStorable(el_ty)) { // Check must come before DefaultAlignAndSize() + if (!IsPlain(el_ty)) { // Check must come before DefaultAlignAndSize() builder_->Diagnostics().add_error( el_ty->FriendlyName(builder_->Symbols()) + " cannot be used as an element type of an array", @@ -2924,7 +2922,7 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) { } // Validate member type - if (!IsStorable(type)) { + if (!IsPlain(type)) { diagnostics_.add_error( type->FriendlyName(builder_->Symbols()) + " cannot be used as the type of a structure member"); @@ -3167,13 +3165,6 @@ bool Resolver::ValidateAssignment(const ast::AssignmentStatement* a) { auto* value_type = rhs_type->UnwrapRef(); // Implicit load of RHS - // RHS needs to be of a storable type - if (!IsStorable(value_type)) { - diagnostics_.add_error("'" + TypeNameOf(a->rhs()) + "' is not storable", - a->rhs()->source()); - return false; - } - // Value type has to match storage type if (storage_type != value_type) { diagnostics_.add_error("cannot assign '" + TypeNameOf(a->rhs()) + "' to '" + diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index da5ba4b000..da9a0a4954 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -73,6 +73,10 @@ class Resolver { /// @returns true if the resolver was successful bool Resolve(); + /// @param type the given type + /// @returns true if the given type is a plain type + bool IsPlain(const sem::Type* type); + /// @param type the given type /// @returns true if the given type is storable bool IsStorable(const sem::Type* type); diff --git a/src/resolver/var_let_validation_test.cc b/src/resolver/var_let_validation_test.cc index 7e5ecf20b2..96ada209d5 100644 --- a/src/resolver/var_let_validation_test.cc +++ b/src/resolver/var_let_validation_test.cc @@ -43,18 +43,19 @@ TEST_F(ResolverVarLetValidationTest, GlobalLetNoInitializer) { "12:34 error: let declarations must have initializers"); } -TEST_F(ResolverVarLetValidationTest, VarConstructorNotStorable) { +TEST_F(ResolverVarLetValidationTest, VarTypeNotStorable) { // var i : i32; // var p : pointer = &v; auto* i = Var("i", ty.i32(), ast::StorageClass::kNone); - auto* p = Var("a", ty.i32(), ast::StorageClass::kNone, - AddressOf(Source{{12, 34}}, "i")); + auto* p = + Var(Source{{56, 78}}, "a", ty.pointer(ast::StorageClass::kFunction), + ast::StorageClass::kNone, AddressOf(Source{{12, 34}}, "i")); WrapInFunction(i, p); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: 'ptr' is not storable for " - "assignment"); + "56:78 error: ptr cannot be used as the " + "type of a var"); } TEST_F(ResolverVarLetValidationTest, LetConstructorWrongType) {