Move struct validation from Validator to Resolver

* Moved Validator::ValidateConstructedType, which only validated
structs, to Resolver as ValidateStructure.
* Moved relevant tests to new files, and also updated all failing tests
to validate Source location.
* Fixed other tests that broke now that we're validating structs.

Bug: tint:642
Change-Id: Iefc08ef548f52d8c3798d814d2183c56d1236c2d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45160
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/CMakeLists.txt

@@ -467,6 +467,7 @@ if(${TINT_BUILD_TESTS})
     inspector/inspector_test.cc
     intrinsic_table_test.cc
     program_test.cc
+    resolver/decoration_validation_test.cc
     resolver/intrinsic_test.cc
     resolver/is_storeable_test.cc
     resolver/resolver_test_helper.cc
@@ -474,6 +475,7 @@ if(${TINT_BUILD_TESTS})
     resolver/resolver_test.cc
     resolver/struct_layout_test.cc
     resolver/struct_storage_class_use_test.cc
+    resolver/type_validation_test.cc
     resolver/validation_test.cc
     scope_stack_test.cc
     semantic/sem_intrinsic_test.cc

src/diagnostic/diagnostic.h

@@ -110,21 +110,35 @@ class List {
 
   /// adds the error message without a source to the end of this list.
   /// @param err_msg the error message
-  void add_error(const std::string& err_msg) {
+  void add_error(std::string err_msg) {
     diag::Diagnostic error{};
     error.severity = diag::Severity::Error;
-    error.message = err_msg;
+    error.message = std::move(err_msg);
     add(std::move(error));
   }
 
   /// adds the error message with the given Source to the end of this list.
   /// @param err_msg the error message
   /// @param source the source of the error diagnostic
-  void add_error(const std::string& err_msg, const Source& source) {
+  void add_error(std::string err_msg, const Source& source) {
     diag::Diagnostic error{};
     error.severity = diag::Severity::Error;
     error.source = source;
-    error.message = err_msg;
+    error.message = std::move(err_msg);
+    add(std::move(error));
+  }
+
+  /// adds the error message with the given code and Source to the end of this
+  /// list.
+  /// @param code the error code
+  /// @param err_msg the error message
+  /// @param source the source of the error diagnostic
+  void add_error(const char* code, std::string err_msg, const Source& source) {
+    diag::Diagnostic error{};
+    error.code = code;
+    error.severity = diag::Severity::Error;
+    error.source = source;
+    error.message = std::move(err_msg);
     add(std::move(error));
   }
 

src/inspector/inspector_test.cc

@@ -222,6 +222,17 @@ class InspectorHelper : public ProgramBuilder {
     return struct_type;
   }
 
+  /// Returns true if the struct with `member_types` requires a block decoration
+  /// @param member_types a vector of member types
+  /// @returns true if block decoration is required
+  bool StructRequiresBlockDecoration(
+      std::vector<type::Type*> member_types) const {
+    // Structure needs a [[block]] attribute if the last member is a
+    // dynamically-sized array.
+    return member_types.back()->Is<type::Array>(
+        [](auto&& a) { return a->IsRuntimeArray(); });
+  }
+
   /// Generates types appropriate for using in a storage buffer
   /// @param name name for the type
   /// @param member_types a vector of member types
@@ -231,7 +242,8 @@ class InspectorHelper : public ProgramBuilder {
   std::tuple<type::Struct*, type::AccessControl*> MakeStorageBufferTypes(
       const std::string& name,
       std::vector<type::Type*> member_types) {
-    auto* struct_type = MakeStructType(name, member_types, false);
+    bool is_block = StructRequiresBlockDecoration(member_types);
+    auto* struct_type = MakeStructType(name, member_types, is_block);
     auto* access_type = create<type::AccessControl>(
         ast::AccessControl::kReadWrite, struct_type);
     return {struct_type, std::move(access_type)};
@@ -246,7 +258,8 @@ class InspectorHelper : public ProgramBuilder {
   std::tuple<type::Struct*, type::AccessControl*>
   MakeReadOnlyStorageBufferTypes(const std::string& name,
                                  std::vector<type::Type*> member_types) {
-    auto* struct_type = MakeStructType(name, member_types, false);
+    bool is_block = StructRequiresBlockDecoration(member_types);
+    auto* struct_type = MakeStructType(name, member_types, is_block);
     auto* access_type =
         create<type::AccessControl>(ast::AccessControl::kReadOnly, struct_type);
     return {struct_type, std::move(access_type)};

src/resolver/decoration_validation_test.cc

@@ -0,0 +1,205 @@
+// 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/ast/access_decoration.h"
+#include "src/ast/constant_id_decoration.h"
+#include "src/ast/stage_decoration.h"
+#include "src/ast/struct_block_decoration.h"
+#include "src/ast/workgroup_decoration.h"
+#include "src/resolver/resolver.h"
+#include "src/resolver/resolver_test_helper.h"
+
+#include "gmock/gmock.h"
+
+namespace tint {
+namespace {
+
+enum class DecorationKind {
+  kAccess,
+  kAlign,
+  kBinding,
+  kBuiltin,
+  kConstantId,
+  kGroup,
+  kLocation,
+  kOffset,
+  kSize,
+  kStage,
+  kStride,
+  kStructBlock,
+  kWorkgroup,
+};
+struct TestParams {
+  DecorationKind kind;
+  bool should_pass;
+};
+class TestWithParams : public resolver::TestHelper,
+                       public testing::TestWithParam<TestParams> {};
+
+ast::Decoration* createDecoration(const Source& source,
+                                  ProgramBuilder& builder,
+                                  DecorationKind kind) {
+  switch (kind) {
+    case DecorationKind::kAccess:
+      return builder.create<ast::AccessDecoration>(
+          source, ast::AccessControl::kReadOnly);
+    case DecorationKind::kAlign:
+      return builder.create<ast::StructMemberAlignDecoration>(source, 4u);
+    case DecorationKind::kBinding:
+      return builder.create<ast::BindingDecoration>(source, 1);
+    case DecorationKind::kBuiltin:
+      return builder.create<ast::BuiltinDecoration>(source,
+                                                    ast::Builtin::kPosition);
+    case DecorationKind::kConstantId:
+      return builder.create<ast::ConstantIdDecoration>(source, 0u);
+    case DecorationKind::kGroup:
+      return builder.create<ast::GroupDecoration>(source, 1u);
+    case DecorationKind::kLocation:
+      return builder.create<ast::LocationDecoration>(source, 1);
+    case DecorationKind::kOffset:
+      return builder.create<ast::StructMemberOffsetDecoration>(source, 4u);
+    case DecorationKind::kSize:
+      return builder.create<ast::StructMemberSizeDecoration>(source, 4u);
+    case DecorationKind::kStage:
+      return builder.create<ast::StageDecoration>(source,
+                                                  ast::PipelineStage::kCompute);
+    case DecorationKind::kStride:
+      return builder.create<ast::StrideDecoration>(source, 4u);
+    case DecorationKind::kStructBlock:
+      return builder.create<ast::StructBlockDecoration>(source);
+    case DecorationKind::kWorkgroup:
+      return builder.create<ast::WorkgroupDecoration>(source, 1u, 1u, 1u);
+  }
+  return nullptr;
+}
+
+using ArrayDecorationTest = TestWithParams;
+
+TEST_P(ArrayDecorationTest, IsValid) {
+  auto params = GetParam();
+
+  ast::StructMemberList members{Member(
+      "a", create<type::Array>(ty.f32(), 0,
+                               ast::DecorationList{createDecoration(
+                                   Source{{12, 34}}, *this, params.kind)}))};
+  auto* s = create<ast::Struct>(
+      members, ast::DecorationList{create<ast::StructBlockDecoration>()});
+  auto* s_ty = ty.struct_("mystruct", s);
+  AST().AddConstructedType(s_ty);
+
+  WrapInFunction();
+
+  if (params.should_pass) {
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+  } else {
+    EXPECT_FALSE(r()->Resolve()) << r()->error();
+    EXPECT_EQ(r()->error(),
+              "12:34 error: decoration is not valid for array types");
+  }
+}
+INSTANTIATE_TEST_SUITE_P(
+    ResolverDecorationValidationTest,
+    ArrayDecorationTest,
+    testing::Values(TestParams{DecorationKind::kAccess, false},
+                    TestParams{DecorationKind::kAlign, false},
+                    TestParams{DecorationKind::kBinding, false},
+                    TestParams{DecorationKind::kBuiltin, false},
+                    TestParams{DecorationKind::kConstantId, false},
+                    TestParams{DecorationKind::kGroup, false},
+                    TestParams{DecorationKind::kLocation, false},
+                    TestParams{DecorationKind::kOffset, false},
+                    TestParams{DecorationKind::kSize, false},
+                    TestParams{DecorationKind::kStage, false},
+                    TestParams{DecorationKind::kStride, true},
+                    TestParams{DecorationKind::kStructBlock, false},
+                    TestParams{DecorationKind::kWorkgroup, false}));
+
+using StructDecorationTest = TestWithParams;
+TEST_P(StructDecorationTest, IsValid) {
+  auto params = GetParam();
+
+  auto* s = create<ast::Struct>(ast::StructMemberList{},
+                                ast::DecorationList{createDecoration(
+                                    Source{{12, 34}}, *this, params.kind)});
+  auto* s_ty = ty.struct_("mystruct", s);
+  AST().AddConstructedType(s_ty);
+
+  WrapInFunction();
+
+  if (params.should_pass) {
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+  } else {
+    EXPECT_FALSE(r()->Resolve()) << r()->error();
+    EXPECT_EQ(r()->error(),
+              "12:34 error: decoration is not valid for struct declarations");
+  }
+}
+INSTANTIATE_TEST_SUITE_P(
+    ResolverDecorationValidationTest,
+    StructDecorationTest,
+    testing::Values(TestParams{DecorationKind::kAccess, false},
+                    TestParams{DecorationKind::kAlign, false},
+                    TestParams{DecorationKind::kBinding, false},
+                    TestParams{DecorationKind::kBuiltin, false},
+                    TestParams{DecorationKind::kConstantId, false},
+                    TestParams{DecorationKind::kGroup, false},
+                    TestParams{DecorationKind::kLocation, false},
+                    TestParams{DecorationKind::kOffset, false},
+                    TestParams{DecorationKind::kSize, false},
+                    TestParams{DecorationKind::kStage, false},
+                    TestParams{DecorationKind::kStride, false},
+                    TestParams{DecorationKind::kStructBlock, true},
+                    TestParams{DecorationKind::kWorkgroup, false}));
+
+using StructMemberDecorationTest = TestWithParams;
+TEST_P(StructMemberDecorationTest, IsValid) {
+  auto params = GetParam();
+
+  ast::StructMemberList members{
+      Member("a", ty.i32(),
+             ast::DecorationList{
+                 createDecoration(Source{{12, 34}}, *this, params.kind)})};
+  auto* s = create<ast::Struct>(members, ast::DecorationList{});
+  auto* s_ty = ty.struct_("mystruct", s);
+  AST().AddConstructedType(s_ty);
+
+  WrapInFunction();
+
+  if (params.should_pass) {
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+  } else {
+    EXPECT_FALSE(r()->Resolve()) << r()->error();
+    EXPECT_EQ(r()->error(),
+              "12:34 error: decoration is not valid for structure members");
+  }
+}
+INSTANTIATE_TEST_SUITE_P(
+    ResolverDecorationValidationTest,
+    StructMemberDecorationTest,
+    testing::Values(TestParams{DecorationKind::kAccess, false},
+                    TestParams{DecorationKind::kAlign, true},
+                    TestParams{DecorationKind::kBinding, false},
+                    TestParams{DecorationKind::kBuiltin, true},
+                    TestParams{DecorationKind::kConstantId, false},
+                    TestParams{DecorationKind::kGroup, false},
+                    TestParams{DecorationKind::kLocation, true},
+                    TestParams{DecorationKind::kOffset, true},
+                    TestParams{DecorationKind::kSize, true},
+                    TestParams{DecorationKind::kStage, false},
+                    TestParams{DecorationKind::kStride, false},
+                    TestParams{DecorationKind::kStructBlock, false},
+                    TestParams{DecorationKind::kWorkgroup, false}));
+
+}  // namespace
+}  // namespace tint

src/resolver/resolver.cc

@@ -27,6 +27,7 @@
 #include "src/ast/if_statement.h"
 #include "src/ast/loop_statement.h"
 #include "src/ast/return_statement.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/ast/switch_statement.h"
 #include "src/ast/unary_op_expression.h"
 #include "src/ast/variable_decl_statement.h"
@@ -1391,6 +1392,61 @@ const semantic::Array* Resolver::Array(type::Array* arr) {
   return create_semantic(utils::RoundUp(el_align, el_size));
 }
 
+bool Resolver::ValidateStructure(const type::Struct* st) {
+  for (auto* member : st->impl()->members()) {
+    if (auto* r = member->type()->UnwrapAll()->As<type::Array>()) {
+      if (r->IsRuntimeArray()) {
+        if (member != st->impl()->members().back()) {
+          diagnostics_.add_error(
+              "v-0015",
+              "runtime arrays may only appear as the last member of a struct",
+              member->source());
+          return false;
+        }
+        if (!st->IsBlockDecorated()) {
+          diagnostics_.add_error("v-0015",
+                                 "a struct containing a runtime-sized array "
+                                 "requires the [[block]] attribute: '" +
+                                     builder_->Symbols().NameFor(st->symbol()) +
+                                     "'",
+                                 member->source());
+          return false;
+        }
+
+        for (auto* deco : r->decorations()) {
+          if (!deco->Is<ast::StrideDecoration>()) {
+            diagnostics_.add_error("decoration is not valid for array types",
+                                   deco->source());
+            return false;
+          }
+        }
+      }
+    }
+
+    for (auto* deco : member->decorations()) {
+      if (!(deco->Is<ast::BuiltinDecoration>() ||
+            deco->Is<ast::LocationDecoration>() ||
+            deco->Is<ast::StructMemberOffsetDecoration>() ||
+            deco->Is<ast::StructMemberSizeDecoration>() ||
+            deco->Is<ast::StructMemberAlignDecoration>())) {
+        diagnostics_.add_error("decoration is not valid for structure members",
+                               deco->source());
+        return false;
+      }
+    }
+  }
+
+  for (auto* deco : st->impl()->decorations()) {
+    if (!(deco->Is<ast::StructBlockDecoration>())) {
+      diagnostics_.add_error("decoration is not valid for struct declarations",
+                             deco->source());
+      return false;
+    }
+  }
+
+  return true;
+}
+
 Resolver::StructInfo* Resolver::Structure(type::Struct* str) {
   auto info_it = struct_info_.find(str);
   if (info_it != struct_info_.end()) {
@@ -1398,6 +1454,10 @@ Resolver::StructInfo* Resolver::Structure(type::Struct* str) {
     return info_it->second;
   }
 
+  if (!ValidateStructure(str)) {
+    return nullptr;
+  }
+
   semantic::StructMemberList sem_members;
   sem_members.reserve(str->impl()->members().size());
 

src/resolver/resolver.h

@@ -216,6 +216,10 @@ class Resolver {
   /// returned.
   const semantic::Array* Array(type::Array*);
 
+  /// @returns returns true if input struct is valid
+  /// @param st the struct to validate
+  bool ValidateStructure(const type::Struct* st);
+
   /// @returns the StructInfo for the structure `str`, building it if it hasn't
   /// been constructed already. If an error is raised, nullptr is returned.
   StructInfo* Structure(type::Struct* str);

src/resolver/struct_layout_test.cc

@@ -15,6 +15,7 @@
 #include "src/resolver/resolver.h"
 
 #include "gmock/gmock.h"
+#include "src/ast/struct_block_decoration.h"
 #include "src/resolver/resolver_test_helper.h"
 #include "src/semantic/struct.h"
 
@@ -125,9 +126,12 @@ TEST_F(ResolverStructLayoutTest, ExplicitStrideArrayStaticSize) {
 }
 
 TEST_F(ResolverStructLayoutTest, ImplicitStrideArrayRuntimeSized) {
-  auto* s = Structure("S", {
+  auto* s =
-                               Member("c", ty.array<f32>()),
+      Structure("S",
-                           });
+                {
+                    Member("c", ty.array<f32>()),
+                },
+                ast::DecorationList{create<ast::StructBlockDecoration>()});
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
@@ -143,9 +147,12 @@ TEST_F(ResolverStructLayoutTest, ImplicitStrideArrayRuntimeSized) {
 }
 
 TEST_F(ResolverStructLayoutTest, ExplicitStrideArrayRuntimeSized) {
-  auto* s = Structure("S", {
+  auto* s =
-                               Member("c", ty.array<f32>(/*stride*/ 32)),
+      Structure("S",
-                           });
+                {
+                    Member("c", ty.array<f32>(/*stride*/ 32)),
+                },
+                ast::DecorationList{create<ast::StructBlockDecoration>()});
 
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 

src/resolver/type_validation_test.cc

@@ -0,0 +1,152 @@
+// 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/ast/stage_decoration.h"
+#include "src/ast/struct_block_decoration.h"
+#include "src/resolver/resolver.h"
+#include "src/resolver/resolver_test_helper.h"
+
+#include "gmock/gmock.h"
+
+namespace tint {
+namespace {
+
+class ResolverTypeValidationTest : public resolver::TestHelper,
+                                   public testing::Test {};
+
+TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLast_Pass) {
+  // [[Block]]
+  // struct Foo {
+  //   vf: f32;
+  //   rt: array<f32>;
+  // };
+
+  ast::DecorationList decos;
+  decos.push_back(create<ast::StructBlockDecoration>());
+  auto* st =
+      create<ast::Struct>(ast::StructMemberList{Member("vf", ty.f32()),
+                                                Member("rt", ty.array<f32>())},
+                          decos);
+
+  auto* struct_type = ty.struct_("Foo", st);
+  AST().AddConstructedType(struct_type);
+
+  WrapInFunction();
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLastNoBlock_Fail) {
+  // struct Foo {
+  //   vf: f32;
+  //   rt: array<f32>;
+  // };
+
+  ast::DecorationList decos;
+  auto* st = create<ast::Struct>(
+      ast::StructMemberList{Member("vf", ty.f32()),
+                            Member(Source{{12, 34}}, "rt", ty.array<f32>())},
+      decos);
+
+  auto* struct_type = ty.struct_("Foo", st);
+  AST().AddConstructedType(struct_type);
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-0015: a struct containing a runtime-sized array "
+            "requires the [[block]] attribute: 'Foo'");
+}
+
+TEST_F(ResolverTypeValidationTest, RuntimeArrayIsNotLast_Fail) {
+  // [[Block]]
+  // struct Foo {
+  //   rt: array<f32>;
+  //   vf: f32;
+  // };
+
+  ast::DecorationList decos;
+  decos.push_back(create<ast::StructBlockDecoration>());
+
+  auto* rt = Member(Source{{12, 34}}, "rt", ty.array<f32>());
+  auto* st = create<ast::Struct>(
+      ast::StructMemberList{rt, Member("vf", ty.f32())}, decos);
+
+  auto* struct_type = ty.struct_("Foo", st);
+
+  AST().AddConstructedType(struct_type);
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-0015: runtime arrays may only appear as the last "
+            "member of a struct");
+}
+
+TEST_F(ResolverTypeValidationTest, AliasRuntimeArrayIsNotLast_Fail) {
+  // [[Block]]
+  // type RTArr = array<u32>;
+  // struct s {
+  //  b: RTArr;
+  //  a: u32;
+  //}
+
+  auto* alias = ty.alias("RTArr", ty.array<u32>());
+
+  ast::DecorationList decos;
+  decos.push_back(create<ast::StructBlockDecoration>());
+  auto* st = create<ast::Struct>(
+      ast::StructMemberList{Member(Source{{12, 34}}, "b", alias),
+                            Member("a", ty.u32())},
+      decos);
+
+  auto* struct_type = ty.struct_("s", st);
+  AST().AddConstructedType(struct_type);
+
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(
+      r()->error(),
+      "12:34 error v-0015: runtime arrays may only appear as the last member "
+      "of a struct");
+}
+
+TEST_F(ResolverTypeValidationTest, AliasRuntimeArrayIsLast_Pass) {
+  // [[Block]]
+  // type RTArr = array<u32>;
+  // struct s {
+  //  a: u32;
+  //  b: RTArr;
+  //}
+
+  auto* alias = ty.alias("RTArr", ty.array<u32>());
+
+  ast::DecorationList decos;
+  decos.push_back(create<ast::StructBlockDecoration>());
+  auto* st = create<ast::Struct>(
+      ast::StructMemberList{Member("a", ty.u32()), Member("b", alias)}, decos);
+
+  auto* struct_type = ty.struct_("s", st);
+  AST().AddConstructedType(struct_type);
+
+  WrapInFunction();
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+}  // namespace
+}  // namespace tint

src/transform/bound_array_accessors_test.cc

@@ -535,6 +535,7 @@ TEST_F(BoundArrayAccessorsTest, DISABLED_Matrix_Row_Constant_Id_Clamps) {
 
 TEST_F(BoundArrayAccessorsTest, RuntimeArray_Clamps) {
   auto* src = R"(
+[[block]]
 struct S {
   a : f32;
   b : array<f32>;
@@ -547,6 +548,7 @@ fn f() -> void {
 )";
 
   auto* expect = R"(
+[[block]]
 struct S {
   a : f32;
   b : array<f32>;

src/validator/validator_decoration_test.cc

@@ -86,44 +86,6 @@ ast::Decoration* createDecoration(ProgramBuilder& builder,
   return nullptr;
 }
 
-using ArrayDecorationTest = ValidatorDecorationsTestWithParams;
-TEST_P(ArrayDecorationTest, Decoration_IsValid) {
-  auto params = GetParam();
-
-  ast::StructMemberList members{Member(
-      "a", create<type::Array>(
-               ty.f32(), 0,
-               ast::DecorationList{createDecoration(*this, params.kind)}))};
-  auto* s = create<ast::Struct>(
-      members, ast::DecorationList{create<ast::StructBlockDecoration>()});
-  auto* s_ty = ty.struct_("mystruct", s);
-
-  ValidatorImpl& v = Build();
-
-  if (params.should_pass) {
-    EXPECT_TRUE(v.ValidateConstructedType(s_ty));
-  } else {
-    EXPECT_FALSE(v.ValidateConstructedType(s_ty));
-    EXPECT_EQ(v.error(), "decoration is not valid for array types");
-  }
-}
-INSTANTIATE_TEST_SUITE_P(
-    ValidatorTest,
-    ArrayDecorationTest,
-    testing::Values(DecorationTestParams{DecorationKind::kAccess, false},
-                    DecorationTestParams{DecorationKind::kAlign, false},
-                    DecorationTestParams{DecorationKind::kBinding, false},
-                    DecorationTestParams{DecorationKind::kBuiltin, false},
-                    DecorationTestParams{DecorationKind::kConstantId, false},
-                    DecorationTestParams{DecorationKind::kGroup, false},
-                    DecorationTestParams{DecorationKind::kLocation, false},
-                    DecorationTestParams{DecorationKind::kOffset, false},
-                    DecorationTestParams{DecorationKind::kSize, false},
-                    DecorationTestParams{DecorationKind::kStage, false},
-                    DecorationTestParams{DecorationKind::kStride, true},
-                    DecorationTestParams{DecorationKind::kStructBlock, false},
-                    DecorationTestParams{DecorationKind::kWorkgroup, false}));
-
 using FunctionDecorationTest = ValidatorDecorationsTestWithParams;
 TEST_P(FunctionDecorationTest, Decoration_IsValid) {
   auto params = GetParam();
@@ -195,77 +157,6 @@ INSTANTIATE_TEST_SUITE_P(
                     DecorationTestParams{DecorationKind::kStructBlock, false},
                     DecorationTestParams{DecorationKind::kWorkgroup, false}));
 
-using StructDecorationTest = ValidatorDecorationsTestWithParams;
-TEST_P(StructDecorationTest, Decoration_IsValid) {
-  auto params = GetParam();
-
-  auto* s = create<ast::Struct>(
-      ast::StructMemberList{},
-      ast::DecorationList{createDecoration(*this, params.kind)});
-  auto* s_ty = ty.struct_("mystruct", s);
-
-  ValidatorImpl& v = Build();
-
-  if (params.should_pass) {
-    EXPECT_TRUE(v.ValidateConstructedType(s_ty));
-  } else {
-    EXPECT_FALSE(v.ValidateConstructedType(s_ty));
-    EXPECT_EQ(v.error(), "decoration is not valid for struct declarations");
-  }
-}
-INSTANTIATE_TEST_SUITE_P(
-    ValidatorTest,
-    StructDecorationTest,
-    testing::Values(DecorationTestParams{DecorationKind::kAccess, false},
-                    DecorationTestParams{DecorationKind::kAlign, false},
-                    DecorationTestParams{DecorationKind::kBinding, false},
-                    DecorationTestParams{DecorationKind::kBuiltin, false},
-                    DecorationTestParams{DecorationKind::kConstantId, false},
-                    DecorationTestParams{DecorationKind::kGroup, false},
-                    DecorationTestParams{DecorationKind::kLocation, false},
-                    DecorationTestParams{DecorationKind::kOffset, false},
-                    DecorationTestParams{DecorationKind::kSize, false},
-                    DecorationTestParams{DecorationKind::kStage, false},
-                    DecorationTestParams{DecorationKind::kStride, false},
-                    DecorationTestParams{DecorationKind::kStructBlock, true},
-                    DecorationTestParams{DecorationKind::kWorkgroup, false}));
-
-using StructMemberDecorations = ValidatorDecorationsTestWithParams;
-TEST_P(StructMemberDecorations, Decoration_IsValid) {
-  auto params = GetParam();
-
-  ast::StructMemberList members{
-      Member("a", ty.i32(),
-             ast::DecorationList{createDecoration(*this, params.kind)})};
-  auto* s = create<ast::Struct>(members, ast::DecorationList{});
-  auto* s_ty = ty.struct_("mystruct", s);
-
-  ValidatorImpl& v = Build();
-
-  if (params.should_pass) {
-    EXPECT_TRUE(v.ValidateConstructedType(s_ty));
-  } else {
-    EXPECT_FALSE(v.ValidateConstructedType(s_ty));
-    EXPECT_EQ(v.error(), "decoration is not valid for structure members");
-  }
-}
-INSTANTIATE_TEST_SUITE_P(
-    ValidatorTest,
-    StructMemberDecorations,
-    testing::Values(DecorationTestParams{DecorationKind::kAccess, false},
-                    DecorationTestParams{DecorationKind::kAlign, true},
-                    DecorationTestParams{DecorationKind::kBinding, false},
-                    DecorationTestParams{DecorationKind::kBuiltin, true},
-                    DecorationTestParams{DecorationKind::kConstantId, false},
-                    DecorationTestParams{DecorationKind::kGroup, false},
-                    DecorationTestParams{DecorationKind::kLocation, true},
-                    DecorationTestParams{DecorationKind::kOffset, true},
-                    DecorationTestParams{DecorationKind::kSize, true},
-                    DecorationTestParams{DecorationKind::kStage, false},
-                    DecorationTestParams{DecorationKind::kStride, false},
-                    DecorationTestParams{DecorationKind::kStructBlock, false},
-                    DecorationTestParams{DecorationKind::kWorkgroup, false}));
-
 using VariableDecorationTest = ValidatorDecorationsTestWithParams;
 TEST_P(VariableDecorationTest, Decoration_IsValid) {
   auto params = GetParam();

src/validator/validator_impl.cc

@@ -72,9 +72,8 @@ bool ValidatorImpl::Validate() {
   // Validate global declarations in the order they appear in the module.
   for (auto* decl : program_->AST().GlobalDeclarations()) {
     if (auto* ty = decl->As<type::Type>()) {
-      if (!ValidateConstructedType(ty)) {
+      // Validated by Resolver (Struct types only)
-        return false;
+      return true;
-      }
     } else if (auto* func = decl->As<ast::Function>()) {
       current_function_ = func;
       if (!ValidateFunction(func)) {
@@ -97,60 +96,6 @@ bool ValidatorImpl::Validate() {
   return true;
 }
 
-bool ValidatorImpl::ValidateConstructedType(const type::Type* type) {
-  if (auto* st = type->As<type::Struct>()) {
-    for (auto* member : st->impl()->members()) {
-      if (auto* r = member->type()->UnwrapAll()->As<type::Array>()) {
-        if (r->IsRuntimeArray()) {
-          if (member != st->impl()->members().back()) {
-            add_error(member->source(), "v-0015",
-                      "runtime arrays may only appear as the last member of "
-                      "a struct");
-            return false;
-          }
-          if (!st->IsBlockDecorated()) {
-            add_error(member->source(), "v-0015",
-                      "a struct containing a runtime-sized array "
-                      "requires the [[block]] attribute: '" +
-                          program_->Symbols().NameFor(st->symbol()) + "'");
-            return false;
-          }
-
-          for (auto* deco : r->decorations()) {
-            if (!deco->Is<ast::StrideDecoration>()) {
-              add_error(deco->source(),
-                        "decoration is not valid for array types");
-              return false;
-            }
-          }
-        }
-      }
-
-      for (auto* deco : member->decorations()) {
-        if (!(deco->Is<ast::BuiltinDecoration>() ||
-              deco->Is<ast::LocationDecoration>() ||
-              deco->Is<ast::StructMemberOffsetDecoration>() ||
-              deco->Is<ast::StructMemberSizeDecoration>() ||
-              deco->Is<ast::StructMemberAlignDecoration>())) {
-          add_error(deco->source(),
-                    "decoration is not valid for structure members");
-          return false;
-        }
-      }
-    }
-
-    for (auto* deco : st->impl()->decorations()) {
-      if (!(deco->Is<ast::StructBlockDecoration>())) {
-        add_error(deco->source(),
-                  "decoration is not valid for struct declarations");
-        return false;
-      }
-    }
-  }
-
-  return true;
-}
-
 bool ValidatorImpl::ValidateGlobalVariable(const ast::Variable* var) {
   auto* sem = program_->Sem().Get(var);
   if (!sem) {

src/validator/validator_impl.h

@@ -131,12 +131,6 @@ class ValidatorImpl {
   /// @param funcs the functions to check
   /// @returns true if the valdiation was successful
   bool ValidateEntryPoint(const ast::FunctionList& funcs);
-
-  /// Validates a constructed type
-  /// @param type the type to check
-  /// @returns true if the valdiation was successful
-  bool ValidateConstructedType(const type::Type* type);
-
   /// Returns true if the given type is storable. This uses and
   /// updates `storable_` and `not_storable_`.
   /// @param type the given type

src/validator/validator_type_test.cc

@@ -21,129 +21,6 @@ namespace {
 
 class ValidatorTypeTest : public ValidatorTestHelper, public testing::Test {};
 
-TEST_F(ValidatorTypeTest, RuntimeArrayIsLast_Pass) {
-  // [[Block]]
-  // struct Foo {
-  //   vf: f32;
-  //   rt: array<f32>;
-  // };
-
-  ast::DecorationList decos;
-  decos.push_back(create<ast::StructBlockDecoration>());
-  auto* st =
-      create<ast::Struct>(ast::StructMemberList{Member("vf", ty.f32()),
-                                                Member("rt", ty.array<f32>())},
-                          decos);
-
-  auto* struct_type = ty.struct_("Foo", st);
-
-  AST().AddConstructedType(struct_type);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.ValidateConstructedType(struct_type));
-}
-
-TEST_F(ValidatorTypeTest, RuntimeArrayIsLastNoBlock_Fail) {
-  // struct Foo {
-  //   vf: f32;
-  //   rt: array<f32>;
-  // };
-
-  ast::DecorationList decos;
-  auto* st =
-      create<ast::Struct>(ast::StructMemberList{Member("vf", ty.f32()),
-                                                Member("rt", ty.array<f32>())},
-                          decos);
-
-  auto* struct_type = ty.struct_("Foo", st);
-  AST().AddConstructedType(struct_type);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
-  EXPECT_EQ(v.error(),
-            "v-0015: a struct containing a runtime-sized array requires the "
-            "[[block]] attribute: 'Foo'");
-}
-
-TEST_F(ValidatorTypeTest, RuntimeArrayIsNotLast_Fail) {
-  // [[Block]]
-  // struct Foo {
-  //   rt: array<f32>;
-  //   vf: f32;
-  // };
-
-  ast::DecorationList decos;
-  decos.push_back(create<ast::StructBlockDecoration>());
-
-  SetSource(Source::Location{12, 34});
-  auto* rt = Member("rt", ty.array<f32>());
-  SetSource(Source{});
-  auto* st = create<ast::Struct>(
-      ast::StructMemberList{rt, Member("vf", ty.f32())}, decos);
-
-  auto* struct_type = ty.struct_("Foo", st);
-
-  AST().AddConstructedType(struct_type);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
-  EXPECT_EQ(v.error(),
-            "12:34 v-0015: runtime arrays may only appear as the last member "
-            "of a struct");
-}
-
-TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) {
-  // [[Block]]
-  // type RTArr = array<u32>;
-  // struct s {
-  //  b: RTArr;
-  //  a: u32;
-  //}
-
-  auto* alias = ty.alias("RTArr", ty.array<u32>());
-
-  ast::DecorationList decos;
-  decos.push_back(create<ast::StructBlockDecoration>());
-  auto* st = create<ast::Struct>(
-      ast::StructMemberList{Member("b", alias), Member("a", ty.u32())}, decos);
-
-  auto* struct_type = ty.struct_("s", st);
-  AST().AddConstructedType(struct_type);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
-  EXPECT_EQ(v.error(),
-            "v-0015: runtime arrays may only appear as the last member "
-            "of a struct");
-}
-
-TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsLast_Pass) {
-  // [[Block]]
-  // type RTArr = array<u32>;
-  // struct s {
-  //  a: u32;
-  //  b: RTArr;
-  //}
-
-  auto* alias = ty.alias("RTArr", ty.array<u32>());
-
-  ast::DecorationList decos;
-  decos.push_back(create<ast::StructBlockDecoration>());
-  auto* st = create<ast::Struct>(
-      ast::StructMemberList{Member("a", ty.u32()), Member("b", alias)}, decos);
-
-  auto* struct_type = ty.struct_("s", st);
-  AST().AddConstructedType(struct_type);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.ValidateConstructedType(struct_type));
-}
-
 TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {
   /// [[stage(vertex)]]
   // fn func -> void { var a : array<i32>; }

src/writer/msl/generator_impl_type_test.cc

@@ -410,14 +410,17 @@ TEST_F(MslGeneratorImplTest, EmitType_Struct_Layout_ArrayDefaultStride) {
   // array_z: size(4), align(4)
   auto* array_z = ty.array<f32>();
 
-  auto* s = Structure("S", {
+  auto* s =
-                               Member("a", ty.i32()),
+      Structure("S",
-                               Member("b", array_x),
+                {
-                               Member("c", ty.f32()),
+                    Member("a", ty.i32()),
-                               Member("d", array_y),
+                    Member("b", array_x),
-                               Member("e", ty.f32()),
+                    Member("c", ty.f32()),
-                               Member("f", array_z),
+                    Member("d", array_y),
-                           });
+                    Member("e", ty.f32()),
+                    Member("f", array_z),
+                },
+                ast::DecorationList{create<ast::StructBlockDecoration>()});
 
   Global("G", s, ast::StorageClass::kStorage);
 

src/writer/spirv/builder_type_test.cc

@@ -438,11 +438,14 @@ TEST_F(BuilderTest_Type, GenerateStruct_DecoratedMembers_LayoutArraysOfMatrix) {
   auto* arr_arr_mat2x3 = ty.array(ty.mat2x3<f32>(), 1);  // Doubly nested array
   auto* rtarr_mat4x4 = ty.array(ty.mat4x4<f32>(), 0);    // Runtime array
 
-  auto* s = Structure("S", {
+  auto* s =
-                               Member("a", arr_mat2x2),
+      Structure("S",
-                               Member("b", arr_arr_mat2x3),
+                {
-                               Member("c", rtarr_mat4x4),
+                    Member("a", arr_mat2x2),
-                           });
+                    Member("b", arr_arr_mat2x3),
+                    Member("c", rtarr_mat4x4),
+                },
+                ast::DecorationList{create<ast::StructBlockDecoration>()});
 
   spirv::Builder& b = Build();
 
@@ -469,7 +472,8 @@ OpMemberName %1 0 "a"
 OpMemberName %1 1 "b"
 OpMemberName %1 2 "c"
 )");
-  EXPECT_EQ(DumpInstructions(b.annots()), R"(OpMemberDecorate %1 0 Offset 0
+  EXPECT_EQ(DumpInstructions(b.annots()), R"(OpDecorate %1 Block
+OpMemberDecorate %1 0 Offset 0
 OpMemberDecorate %1 0 ColMajor
 OpMemberDecorate %1 0 MatrixStride 8
 OpDecorate %2 ArrayStride 16

test/BUILD.gn

@@ -169,13 +169,15 @@ source_set("tint_unittests_core_src") {
     "../src/intrinsic_table_test.cc",
     "../src/program_builder_test.cc",
     "../src/program_test.cc",
+    "../src/resolver/decoration_validation_test.cc",
     "../src/resolver/intrinsic_test.cc",
     "../src/resolver/is_storeable_test.cc",
+    "../src/resolver/resolver_test.cc",
     "../src/resolver/resolver_test_helper.cc",
     "../src/resolver/resolver_test_helper.h",
-    "../src/resolver/resolver_test.cc",
     "../src/resolver/struct_layout_test.cc",
     "../src/resolver/struct_storage_class_use_test.cc",
+    "../src/resolver/type_validation_test.cc",
     "../src/resolver/validation_test.cc",
     "../src/scope_stack_test.cc",
     "../src/semantic/sem_intrinsic_test.cc",