Resolver: Validate <storage> var types

https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration
Variables in the storage storage class and variables with a storage
texture type must have an access attribute applied to the store type.

https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
A variable in the storage storage class is a storage buffer variable. Its
store type must be a host-shareable structure type with block attribute,
satisfying the storage class constraints.

Fixup tests, including those that were producing warnings about `var <in>`

The WGSL writer seems to want to put a newline after every decoration block, leading to some ugly output. I'll fix this as a separate change.

Fixes: tint:531
Fixes: tint:692
Change-Id: If09d987477247ab4a7c635f6ee6e616a06061515
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47763
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>

src/resolver/host_shareable_validation_test.cc

@@ -25,32 +25,10 @@ namespace {
 
 using ResolverHostShareableValidationTest = ResolverTest;
 
-TEST_F(ResolverHostShareableValidationTest, Bool) {
-  Global(Source{{56, 78}}, "g", ty.bool_(), ast::StorageClass::kStorage);
-
-  ASSERT_FALSE(r()->Resolve());
-
-  EXPECT_EQ(
-      r()->error(),
-      R"(56:78 error: Type 'bool' cannot be used in storage class 'storage' as it is non-host-shareable
-56:78 note: while instantiating variable g)");
-}
-
-TEST_F(ResolverHostShareableValidationTest, Pointer) {
-  Global(Source{{56, 78}}, "g", ty.pointer<i32>(ast::StorageClass::kInput),
-         ast::StorageClass::kStorage);
-
-  ASSERT_FALSE(r()->Resolve());
-
-  EXPECT_EQ(
-      r()->error(),
-      R"(56:78 error: Type 'ptr<in, i32>' cannot be used in storage class 'storage' as it is non-host-shareable
-56:78 note: while instantiating variable g)");
-}
-
 TEST_F(ResolverHostShareableValidationTest, BoolMember) {
   auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.bool_())});
-  Global(Source{{56, 78}}, "g", s, ast::StorageClass::kStorage);
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
 
   ASSERT_FALSE(r()->Resolve());
 
@@ -63,7 +41,8 @@ TEST_F(ResolverHostShareableValidationTest, BoolMember) {
 
 TEST_F(ResolverHostShareableValidationTest, BoolVectorMember) {
   auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.vec3<bool>())});
-  Global(Source{{56, 78}}, "g", s, ast::StorageClass::kStorage);
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
 
   ASSERT_FALSE(r()->Resolve());
 
@@ -77,7 +56,8 @@ TEST_F(ResolverHostShareableValidationTest, BoolVectorMember) {
 TEST_F(ResolverHostShareableValidationTest, Aliases) {
   auto* a1 = ty.alias("a1", ty.bool_());
   auto* s = Structure("S", {Member(Source{{12, 34}}, "x", a1)});
-  auto* a2 = ty.alias("a2", s);
+  auto* ac = ty.access(ast::AccessControl::kReadOnly, s);
+  auto* a2 = ty.alias("a2", ac);
   Global(Source{{56, 78}}, "g", a2, ast::StorageClass::kStorage);
 
   ASSERT_FALSE(r()->Resolve());
@@ -89,27 +69,14 @@ TEST_F(ResolverHostShareableValidationTest, Aliases) {
 56:78 note: while instantiating variable g)");
 }
 
-TEST_F(ResolverHostShareableValidationTest, AccessControl) {
-  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.bool_())});
-  auto* a = create<type::AccessControl>(ast::AccessControl::kReadOnly, s);
-  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
-
-  ASSERT_FALSE(r()->Resolve());
-
-  EXPECT_EQ(
-      r()->error(),
-      R"(56:78 error: Type 'bool' cannot be used in storage class 'storage' as it is non-host-shareable
-12:34 note: while analysing structure member S.x
-56:78 note: while instantiating variable g)");
-}
-
 TEST_F(ResolverHostShareableValidationTest, NestedStructures) {
   auto* i1 = Structure("I1", {Member(Source{{1, 2}}, "x", ty.bool_())});
   auto* i2 = Structure("I2", {Member(Source{{3, 4}}, "y", i1)});
   auto* i3 = Structure("I3", {Member(Source{{5, 6}}, "z", i2)});
 
   auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)});
-  Global(Source{{9, 10}}, "g", s, ast::StorageClass::kStorage);
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{9, 10}}, "g", a, ast::StorageClass::kStorage);
 
   ASSERT_FALSE(r()->Resolve());
 
@@ -143,7 +110,8 @@ TEST_F(ResolverHostShareableValidationTest, NoError) {
                       });
 
   auto* s = Structure("S", {Member(Source{{7, 8}}, "m", i3)});
-  Global(Source{{9, 10}}, "g", s, ast::StorageClass::kStorage);
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{9, 10}}, "g", a, ast::StorageClass::kStorage);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 }

src/resolver/resolver.cc

@@ -253,6 +253,27 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     return false;
   }
 
+  if (info->storage_class == ast::StorageClass::kStorage) {
+    // https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration
+    // Variables in the storage storage class and variables with a storage
+    // texture type must have an access attribute applied to the store type.
+
+    // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
+    // A variable in the storage storage class is a storage buffer variable. Its
+    // store type must be a host-shareable structure type with block attribute,
+    // satisfying the storage class constraints.
+
+    auto* access = info->type->As<type::AccessControl>();
+    auto* str = access ? access->type()->As<type::Struct>() : nullptr;
+    if (!str) {
+      diagnostics_.add_error(
+          "variables declared in the <storage> storage class must be of an "
+          "[[access]] qualified structure type",
+          var->source());
+      return false;
+    }
+  }
+
   for (auto* deco : var->decorations()) {
     if (!(deco->Is<ast::BindingDecoration>() ||
           deco->Is<ast::BuiltinDecoration>() ||

src/resolver/resolver_test.cc

@@ -763,9 +763,12 @@ TEST_F(ResolverTest, Function_Parameters) {
 }
 
 TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
+  auto* s = Structure("S", {Member("m", ty.u32())});
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+
   auto* in_var = Global("in_var", ty.f32(), ast::StorageClass::kInput);
   auto* out_var = Global("out_var", ty.f32(), ast::StorageClass::kOutput);
-  auto* sb_var = Global("sb_var", ty.f32(), ast::StorageClass::kStorage);
+  auto* sb_var = Global("sb_var", a, ast::StorageClass::kStorage);
   auto* wg_var = Global("wg_var", ty.f32(), ast::StorageClass::kWorkgroup);
   auto* priv_var = Global("priv_var", ty.f32(), ast::StorageClass::kPrivate);
 
@@ -795,9 +798,12 @@ TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
 }
 
 TEST_F(ResolverTest, Function_RegisterInputOutputVariables_SubFunction) {
+  auto* s = Structure("S", {Member("m", ty.u32())});
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+
   auto* in_var = Global("in_var", ty.f32(), ast::StorageClass::kInput);
   auto* out_var = Global("out_var", ty.f32(), ast::StorageClass::kOutput);
-  auto* sb_var = Global("sb_var", ty.f32(), ast::StorageClass::kStorage);
+  auto* sb_var = Global("sb_var", a, ast::StorageClass::kStorage);
   auto* wg_var = Global("wg_var", ty.f32(), ast::StorageClass::kWorkgroup);
   auto* priv_var = Global("priv_var", ty.f32(), ast::StorageClass::kPrivate);
 

src/resolver/storage_class_validation_test.cc

@@ -0,0 +1,123 @@
+// Copyright 2021 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/resolver/resolver.h"
+
+#include "gmock/gmock.h"
+#include "src/resolver/resolver_test_helper.h"
+#include "src/semantic/struct.h"
+#include "src/type/access_control_type.h"
+
+namespace tint {
+namespace resolver {
+namespace {
+
+using ResolverStorageClassValidationTest = ResolverTest;
+
+TEST_F(ResolverStorageClassValidationTest, GlobalVariableNoStorageClass_Fail) {
+  // var g : f32;
+  Global(Source{{12, 34}}, "g", ty.f32(), ast::StorageClass::kNone);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-0022: global variables must have a storage class");
+}
+
+TEST_F(ResolverStorageClassValidationTest, Bool) {
+  // var<storage> g : bool;
+  Global(Source{{56, 78}}, "g", ty.bool_(), ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, Pointer) {
+  // var<storage> g : ptr<i32, input>;
+  Global(Source{{56, 78}}, "g", ty.pointer<i32>(ast::StorageClass::kInput),
+         ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, Array) {
+  // var<storage> g : [[access(read)]] array<S, 3>;
+  auto* s = Structure("S", {Member("a", ty.f32())});
+  auto* a = ty.array(s, 3);
+  auto* ac = ty.access(ast::AccessControl::kReadOnly, a);
+  Global(Source{{56, 78}}, "g", ac, ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, BoolAlias) {
+  // type a = bool;
+  // var<storage> g : [[access(read)]] a;
+  auto* a = ty.alias("a", ty.bool_());
+  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, NoAccessControl) {
+  // var<storage> g : S;
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
+  Global(Source{{56, 78}}, "g", s, ast::StorageClass::kStorage);
+
+  ASSERT_FALSE(r()->Resolve());
+
+  EXPECT_EQ(
+      r()->error(),
+      R"(56:78 error: variables declared in the <storage> storage class must be of an [[access]] qualified structure type)");
+}
+
+TEST_F(ResolverStorageClassValidationTest, NoError_Basic) {
+  // var<storage> g : [[access(read)]] S;
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
+  auto* a = ty.access(ast::AccessControl::kReadOnly, s);
+  Global(Source{{56, 78}}, "g", a, ast::StorageClass::kStorage);
+
+  ASSERT_TRUE(r()->Resolve());
+}
+
+TEST_F(ResolverStorageClassValidationTest, NoError_Aliases) {
+  // type a1 = S;
+  // type a2 = [[access(read)]] a1;
+  // var<storage> g : a2;
+  auto* s = Structure("S", {Member(Source{{12, 34}}, "x", ty.i32())});
+  auto* a1 = ty.alias("a1", s);
+  auto* ac = ty.access(ast::AccessControl::kReadOnly, a1);
+  auto* a2 = ty.alias("a2", ac);
+  Global(Source{{56, 78}}, "g", a2, ast::StorageClass::kStorage);
+
+  ASSERT_TRUE(r()->Resolve());
+}
+
+}  // namespace
+}  // namespace resolver
+}  // namespace tint

src/resolver/struct_storage_class_use_test.cc

@@ -65,53 +65,53 @@ TEST_F(ResolverStorageClassUseTest, StructReachableFromReturnType) {
 TEST_F(ResolverStorageClassUseTest, StructReachableFromGlobal) {
   auto* s = Structure("S", {Member("a", ty.f32())});
 
-  Global("g", s, ast::StorageClass::kStorage);
+  Global("g", s, ast::StorageClass::kUniform);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
   auto* sem = Sem().Get(s);
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->StorageClassUsage(),
-              UnorderedElementsAre(ast::StorageClass::kStorage));
+              UnorderedElementsAre(ast::StorageClass::kUniform));
 }
 
 TEST_F(ResolverStorageClassUseTest, StructReachableViaGlobalAlias) {
   auto* s = Structure("S", {Member("a", ty.f32())});
   auto* a = ty.alias("A", s);
-  Global("g", a, ast::StorageClass::kStorage);
+  Global("g", a, ast::StorageClass::kUniform);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
   auto* sem = Sem().Get(s);
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->StorageClassUsage(),
-              UnorderedElementsAre(ast::StorageClass::kStorage));
+              UnorderedElementsAre(ast::StorageClass::kUniform));
 }
 
 TEST_F(ResolverStorageClassUseTest, StructReachableViaGlobalStruct) {
   auto* s = Structure("S", {Member("a", ty.f32())});
   auto* o = Structure("O", {Member("a", s)});
-  Global("g", o, ast::StorageClass::kStorage);
+  Global("g", o, ast::StorageClass::kUniform);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
   auto* sem = Sem().Get(s);
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->StorageClassUsage(),
-              UnorderedElementsAre(ast::StorageClass::kStorage));
+              UnorderedElementsAre(ast::StorageClass::kUniform));
 }
 
 TEST_F(ResolverStorageClassUseTest, StructReachableViaGlobalArray) {
   auto* s = Structure("S", {Member("a", ty.f32())});
   auto* a = ty.array(s, 3);
-  Global("g", a, ast::StorageClass::kStorage);
+  Global("g", a, ast::StorageClass::kUniform);
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
   auto* sem = Sem().Get(s);
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->StorageClassUsage(),
-              UnorderedElementsAre(ast::StorageClass::kStorage));
+              UnorderedElementsAre(ast::StorageClass::kUniform));
 }
 
 TEST_F(ResolverStorageClassUseTest, StructReachableFromLocal) {
@@ -168,8 +168,9 @@ TEST_F(ResolverStorageClassUseTest, StructReachableViaLocalArray) {
 
 TEST_F(ResolverStorageClassUseTest, StructMultipleStorageClassUses) {
   auto* s = Structure("S", {Member("a", ty.f32())});
-  Global("x", s, ast::StorageClass::kStorage);
-  Global("y", s, ast::StorageClass::kUniform);
+  auto* ac = ty.access(ast::AccessControl::kReadOnly, s);
+  Global("x", s, ast::StorageClass::kUniform);
+  Global("y", ac, ast::StorageClass::kStorage);
   WrapInFunction(Var("g", s, ast::StorageClass::kFunction));
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
@@ -177,8 +178,8 @@ TEST_F(ResolverStorageClassUseTest, StructMultipleStorageClassUses) {
   auto* sem = Sem().Get(s);
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->StorageClassUsage(),
-              UnorderedElementsAre(ast::StorageClass::kStorage,
-                                   ast::StorageClass::kUniform,
+              UnorderedElementsAre(ast::StorageClass::kUniform,
+                                   ast::StorageClass::kStorage,
                                    ast::StorageClass::kFunction));
 }
 

src/resolver/type_validation_test.cc

@@ -56,15 +56,6 @@ TEST_F(ResolverTypeValidationTest, GlobalVariableWithStorageClass_Pass) {
   EXPECT_TRUE(r()->Resolve()) << r()->error();
 }
 
-TEST_F(ResolverTypeValidationTest, GlobalVariableNoStorageClass_Fail) {
-  // var global_var: f32;
-  Global(Source{{12, 34}}, "global_var", ty.f32(), ast::StorageClass::kNone);
-
-  EXPECT_FALSE(r()->Resolve());
-  EXPECT_EQ(r()->error(),
-            "12:34 error v-0022: global variables must have a storage class");
-}
-
 TEST_F(ResolverTypeValidationTest, GlobalConstantWithStorageClass_Fail) {
   // const<in> global_var: f32;
   AST().AddGlobalVariable(