From 94518a8a17ba38907cabbfac688183aae9024264 Mon Sep 17 00:00:00 2001
From: Alan Baker <alanbaker@google.com>
Date: Fri, 19 Apr 2024 13:54:37 -0400
Subject: [PATCH 1/2] Validation for address space and access mode in var decls

* Check that only storage allows an access mode (read or read_write)
* Tests no access mode parameter and a trailing comma after address
  space separately
---
 src/webgpu/listing_meta.json                  |  1 +
 src/webgpu/shader/validation/decl/var.spec.ts | 39 +++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/src/webgpu/listing_meta.json b/src/webgpu/listing_meta.json
index 1710db137b4b..e4da9127b439 100644
--- a/src/webgpu/listing_meta.json
+++ b/src/webgpu/listing_meta.json
@@ -1901,6 +1901,7 @@
   "webgpu:shader,validation,decl,ptr_spelling:ptr_bad_store_type:*": { "subcaseMS": 0.967 },
   "webgpu:shader,validation,decl,ptr_spelling:ptr_handle_space_invalid:*": { "subcaseMS": 1.000 },
   "webgpu:shader,validation,decl,ptr_spelling:ptr_not_instantiable:*": { "subcaseMS": 1.310 },
+  "webgpu:shader,validation,decl,var:address_space_access_mode:*": { "subcaseMS": 14.399 },
   "webgpu:shader,validation,decl,var:binding_collision_unused_helper:*": { "subcaseMS": 1.000 },
   "webgpu:shader,validation,decl,var:binding_collisions:*": { "subcaseMS": 1.000 },
   "webgpu:shader,validation,decl,var:binding_point_on_function_var:*": { "subcaseMS": 1.000 },
diff --git a/src/webgpu/shader/validation/decl/var.spec.ts b/src/webgpu/shader/validation/decl/var.spec.ts
index 95e7417d2387..76e510983fcf 100644
--- a/src/webgpu/shader/validation/decl/var.spec.ts
+++ b/src/webgpu/shader/validation/decl/var.spec.ts
@@ -527,3 +527,42 @@ g.test('binding_collision_unused_helper')
 
     t.expectCompileResult(true, wgsl);
   });
+
+g.test('address_space_access_mode')
+  .desc('Test that only storage accepts an access mode')
+  .params(u =>
+    u
+      .combine('address_space', ['private', 'storage', 'uniform', 'function', 'workgroup'] as const)
+      .combine('access_mode', ['', ',', ',read', ',write', ',read_write'] as const)
+  )
+  .fn(t => {
+    let fdecl = ``;
+    let mdecl = ``;
+    // Most address spaces do not accept an access mode, but should accept no
+    // template argument or a trailing comma.
+    let shouldPass = t.params.access_mode === '' || t.params.access_mode === ',';
+    // 'handle' unchecked since it is untypable.
+    switch (t.params.address_space) {
+      case 'private':
+        mdecl = `var<private${t.params.access_mode}> x : u32;`;
+        break;
+      case 'storage':
+        mdecl = `@group(0) @binding(0) var<storage${t.params.access_mode}> x : u32;`;
+        shouldPass = t.params.access_mode !== ',write';
+        break;
+      case 'uniform':
+        mdecl = `@group(0) @binding(0) var<uniform${t.params.access_mode}> x : u32;`;
+        break;
+      case 'workgroup':
+        mdecl = `var<private${t.params.access_mode}> x : u32;`;
+        break;
+      case 'function':
+        fdecl = `var<function${t.params.access_mode}> x : u32;`;
+        break;
+    }
+    const code = `${mdecl}
+    fn foo() {
+      ${fdecl}
+    }`;
+    t.expectCompileResult(shouldPass, code);
+  });

From 14bdefb3a187fd68a2f7ac2fe6b7f5d289dfbaac Mon Sep 17 00:00:00 2001
From: Alan Baker <alanbaker@google.com>
Date: Fri, 19 Apr 2024 14:20:05 -0400
Subject: [PATCH 2/2] Changes for review

* Test trailing comma more generally.
---
 src/webgpu/shader/validation/decl/var.spec.ts | 24 ++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/webgpu/shader/validation/decl/var.spec.ts b/src/webgpu/shader/validation/decl/var.spec.ts
index 76e510983fcf..81426e1e21fe 100644
--- a/src/webgpu/shader/validation/decl/var.spec.ts
+++ b/src/webgpu/shader/validation/decl/var.spec.ts
@@ -533,31 +533,39 @@ g.test('address_space_access_mode')
   .params(u =>
     u
       .combine('address_space', ['private', 'storage', 'uniform', 'function', 'workgroup'] as const)
-      .combine('access_mode', ['', ',', ',read', ',write', ',read_write'] as const)
+      .combine('access_mode', ['', 'read', 'write', 'read_write'] as const)
+      .combine('trailing_comma', [true, false] as const)
   )
   .fn(t => {
     let fdecl = ``;
     let mdecl = ``;
     // Most address spaces do not accept an access mode, but should accept no
     // template argument or a trailing comma.
-    let shouldPass = t.params.access_mode === '' || t.params.access_mode === ',';
+    let shouldPass = t.params.access_mode === '';
+    let suffix = ``;
+    if (t.params.access_mode === '') {
+      suffix += t.params.trailing_comma ? ',' : '';
+    } else {
+      suffix += `,${t.params.access_mode}`;
+      suffix += t.params.trailing_comma ? ',' : '';
+    }
     // 'handle' unchecked since it is untypable.
     switch (t.params.address_space) {
       case 'private':
-        mdecl = `var<private${t.params.access_mode}> x : u32;`;
+        mdecl = `var<private${suffix}> x : u32;`;
         break;
       case 'storage':
-        mdecl = `@group(0) @binding(0) var<storage${t.params.access_mode}> x : u32;`;
-        shouldPass = t.params.access_mode !== ',write';
+        mdecl = `@group(0) @binding(0) var<storage${suffix}> x : u32;`;
+        shouldPass = t.params.access_mode !== 'write';
         break;
       case 'uniform':
-        mdecl = `@group(0) @binding(0) var<uniform${t.params.access_mode}> x : u32;`;
+        mdecl = `@group(0) @binding(0) var<uniform${suffix}> x : u32;`;
         break;
       case 'workgroup':
-        mdecl = `var<private${t.params.access_mode}> x : u32;`;
+        mdecl = `var<workgroup${suffix}> x : u32;`;
         break;
       case 'function':
-        fdecl = `var<function${t.params.access_mode}> x : u32;`;
+        fdecl = `var<function${suffix}> x : u32;`;
         break;
     }
     const code = `${mdecl}