Validate that runtime arrays aren't used as parameters

Remove the logic around `arrayLength()` being passed anything but a member accessor. Runtime arrays types cannot be used for variables nor parameters.

Add validator logic and test for runtime array parameters.

Adjust the validator error message so it doesn't include the field / variable name. This read weird, and the same information is already provided by the source.

Bug: tint:266
Bug: tint:252
Change-Id: Iecedb0524e10a67b4f8ad15635d67fe61e9d69d9
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/36420
Auto-Submit: Ben Clayton <bclayton@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/validator/validator_impl.cc

@@ -91,9 +91,8 @@ bool ValidatorImpl::ValidateConstructedTypes(
           if (r->IsRuntimeArray()) {
             if (member != st->impl()->members().back()) {
               add_error(member->source(), "v-0015",
-                        "runtime arrays may only appear as the last "
+                        "runtime arrays may only appear as the last member of "
-                        "member of a struct: '" +
+                        "a struct");
-                            member->name() + "'");
               return false;
             }
             if (!st->IsBlockDecorated()) {
@@ -199,6 +198,9 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) {
 
   for (auto* param : func->params()) {
     variable_stack_.set(param->name(), param);
+    if (!ValidateParameter(param)) {
+      return false;
+    }
   }
   if (!ValidateStatements(func->body())) {
     return false;
@@ -216,6 +218,18 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) {
   return true;
 }
 
+bool ValidatorImpl::ValidateParameter(const ast::Variable* param) {
+  if (auto* r = param->type()->UnwrapAll()->As<ast::type::Array>()) {
+    if (r->IsRuntimeArray()) {
+      add_error(
+          param->source(), "v-0015",
+          "runtime arrays may only appear as the last member of a struct");
+      return false;
+    }
+  }
+  return true;
+}
+
 bool ValidatorImpl::ValidateReturnStatement(const ast::ReturnStatement* ret) {
   // TODO(sarahM0): update this when this issue resolves:
   // https://github.com/gpuweb/gpuweb/issues/996
@@ -266,10 +280,9 @@ bool ValidatorImpl::ValidateDeclStatement(
   if (auto* arr =
           decl->variable()->type()->UnwrapAll()->As<ast::type::Array>()) {
     if (arr->IsRuntimeArray()) {
-      add_error(decl->source(), "v-0015",
+      add_error(
-                "runtime arrays may only appear as the last "
+          decl->source(), "v-0015",
-                "member of a struct: '" +
+          "runtime arrays may only appear as the last member of a struct");
-                    decl->variable()->name() + "'");
       return false;
     }
   }

src/validator/validator_impl.h

@@ -83,6 +83,10 @@ class ValidatorImpl {
   /// @param func the function to check
   /// @returns true if the validation was successful
   bool ValidateFunction(const ast::Function* func);
+  /// Validates a function parameter
+  /// @param param the function parameter to check
+  /// @returns true if the validation was successful
+  bool ValidateParameter(const ast::Variable* param);
   /// Validates a block of statements
   /// @param block the statements to check
   /// @returns true if the validation was successful

src/validator/validator_type_test.cc

@@ -84,18 +84,20 @@ TEST_F(ValidatorTypeTest, RuntimeArrayIsNotLast_Fail) {
 
   ast::StructDecorationList decos;
   decos.push_back(create<ast::StructBlockDecoration>());
-  auto* st =
+
-      create<ast::Struct>(ast::StructMemberList{Member("rt", ty.array<f32>()),
+  SetSource(Source::Location{12, 34});
-                                                Member("vf", ty.f32)},
+  auto* rt = Member("rt", ty.array<f32>());
-                          decos);
+  SetSource(Source{});
+  auto* st = create<ast::Struct>(
+      ast::StructMemberList{rt, Member("vf", ty.f32)}, decos);
 
   auto* struct_type = ty.struct_("Foo", st);
 
   mod->AddConstructedType(struct_type);
   EXPECT_FALSE(v()->ValidateConstructedTypes(mod->constructed_types()));
   EXPECT_EQ(v()->error(),
-            "v-0015: runtime arrays may only appear as the last member "
+            "12:34 v-0015: runtime arrays may only appear as the last member "
-            "of a struct: 'rt'");
+            "of a struct");
 }
 
 TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) {
@@ -118,7 +120,7 @@ TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) {
   EXPECT_FALSE(v()->ValidateConstructedTypes(mod->constructed_types()));
   EXPECT_EQ(v()->error(),
             "v-0015: runtime arrays may only appear as the last member "
-            "of a struct: 'b'");
+            "of a struct");
 }
 
 TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsLast_Pass) {
@@ -161,8 +163,39 @@ TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {
   EXPECT_FALSE(v()->Validate(mod));
   EXPECT_EQ(v()->error(),
             "12:34 v-0015: runtime arrays may only appear as the last member "
-            "of a struct: 'a'");
+            "of a struct");
 }
 
+TEST_F(ValidatorTypeTest, RuntimeArrayAsParameter_Fail) {
+  // fn func(a : array<u32>) {}
+  // [[stage(vertex)]] fn main() {}
+
+  auto* param =
+      Var(Source{Source::Location{12, 34}}, "a", ast::StorageClass::kNone,
+          ty.array<i32>(), nullptr, ast::VariableDecorationList{});
+
+  auto* func = Func("func", ast::VariableList{param}, ty.void_,
+                    ast::StatementList{
+                        create<ast::ReturnStatement>(),
+                    },
+                    ast::FunctionDecorationList{});
+  mod->AddFunction(func);
+
+  auto* main =
+      Func("main", ast::VariableList{}, ty.void_,
+           ast::StatementList{
+               create<ast::ReturnStatement>(),
+           },
+           ast::FunctionDecorationList{
+               create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+           });
+  mod->AddFunction(main);
+
+  EXPECT_TRUE(td()->Determine()) << td()->error();
+  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_EQ(v()->error(),
+            "12:34 v-0015: runtime arrays may only appear as the last member "
+            "of a struct");
+}
 }  // namespace
 }  // namespace tint

src/writer/spirv/builder.cc

@@ -1871,15 +1871,15 @@ uint32_t Builder::GenerateIntrinsic(ast::IdentifierExpression* ident,
     if (call->params().empty()) {
       error_ = "missing param for runtime array length";
       return 0;
-    } else if (!call->params()[0]->Is<ast::MemberAccessorExpression>()) {
+    }
-      if (call->params()[0]->result_type()->Is<ast::type::Pointer>()) {
+    auto* arg = call->params()[0];
-        error_ = "pointer accessors not supported yet";
+
-      } else {
+    auto* accessor = arg->As<ast::MemberAccessorExpression>();
-        error_ = "invalid accessor for runtime array length";
+    if (accessor == nullptr) {
-      }
+      error_ = "invalid expression for array length";
       return 0;
     }
-    auto* accessor = call->params()[0]->As<ast::MemberAccessorExpression>();
+
     auto struct_id = GenerateExpression(accessor->structure());
     if (struct_id == 0) {
       return 0;

src/writer/spirv/builder_intrinsic_test.cc

@@ -1285,8 +1285,7 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength) {
                           ast::StructDecorationList{});
   auto* s_type = ty.struct_("my_struct", s);
   auto* var = Var("b", ast::StorageClass::kPrivate, s_type);
-  auto* expr = Call("arrayLength", create<ast::MemberAccessorExpression>(
+  auto* expr = Call("arrayLength", MemberAccessor("b", "a"));
-                                       Expr("b"), Expr("a")));
 
   ASSERT_TRUE(td.DetermineResultType(expr)) << td.error();
 
@@ -1321,8 +1320,7 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength_OtherMembersInStruct) {
 
   auto* s_type = ty.struct_("my_struct", s);
   auto* var = Var("b", ast::StorageClass::kPrivate, s_type);
-  auto* expr = Call("arrayLength", create<ast::MemberAccessorExpression>(
+  auto* expr = Call("arrayLength", MemberAccessor("b", "a"));
-                                       Expr("b"), Expr("a")));
 
   ASSERT_TRUE(td.DetermineResultType(expr)) << td.error();
 
@@ -1350,36 +1348,6 @@ TEST_F(IntrinsicBuilderTest, Call_ArrayLength_OtherMembersInStruct) {
 )");
 }
 
-// TODO(dsinclair): https://bugs.chromium.org/p/tint/issues/detail?id=266
-TEST_F(IntrinsicBuilderTest, DISABLED_Call_ArrayLength_Ptr) {
-  ast::type::Pointer ptr(ty.array<f32>(), ast::StorageClass::kStorageBuffer);
-
-  auto* s = create<ast::Struct>(
-      ast::StructMemberList{Member("z", ty.f32), Member("a", ty.array<f32>())},
-      ast::StructDecorationList{});
-  auto* s_type = ty.struct_("my_struct", s);
-  auto* var = Var("b", ast::StorageClass::kPrivate, s_type);
-
-  Var("ptr_var", ast::StorageClass::kPrivate, &ptr,
-      create<ast::MemberAccessorExpression>(Expr("b"), Expr("a")), {});
-
-  auto* expr = Call("arrayLength", "ptr_var");
-  ASSERT_TRUE(td.DetermineResultType(expr)) << td.error();
-
-  auto* func = Func("a_func", ast::VariableList{}, ty.void_,
-                    ast::StatementList{}, ast::FunctionDecorationList{});
-
-  ASSERT_TRUE(b.GenerateFunction(func)) << b.error();
-  ASSERT_TRUE(b.GenerateGlobalVariable(var)) << b.error();
-  EXPECT_EQ(b.GenerateExpression(expr), 11u) << b.error();
-
-  EXPECT_EQ(DumpInstructions(b.types()), R"( ... )");
-
-  EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(%11 = OpArrayLength %12 %5 1
-)");
-}
-
 }  // namespace
 }  // namespace spirv
 }  // namespace writer