Resolver: improve array handling

type::Array was the only type with special handling in that we'd resolve
all array types first in ResolverInternal, then proceed with resolving
the AST in order of declaration. This CL removes this special handling,
and instead, we now process Arrays as we process ast::Variables of
array type. This change also allows us to pass down a Source location
for validation messages when processing Arrays.

Updated some Builder tests that weren't creating a variable of the array
type they declared.

Bug: tint:707
Change-Id: I8483b3a979bc1e5e04feb1ca4d281e96e9e654be
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47825
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>

src/resolver/resolver.cc

@@ -178,16 +178,6 @@ bool Resolver::IsValidAssignment(type::Type* lhs, type::Type* rhs) {
 }
 
 bool Resolver::ResolveInternal() {
-  // Process non-user-defined types (arrays). The rest will be processed in
-  // order of declaration below.
-  for (auto* ty : builder_->Types()) {
-    if (auto* arr = ty->As<type::Array>()) {
-      if (!Array(arr)) {
-        return false;
-      }
-    }
-  }
-
   // Process everything else in the order they appear in the module. This is
   // necessary for validation of use-before-declaration.
   for (auto* decl : builder_->AST().GlobalDeclarations()) {
@@ -196,10 +186,6 @@ bool Resolver::ResolveInternal() {
         if (!Structure(str)) {
           return false;
         }
-      } else if (auto* arr = decl->As<type::Array>()) {
-        if (!Array(arr)) {
-          return false;
-        }
       }
     } else if (auto* func = decl->As<ast::Function>()) {
       if (!Function(func)) {
@@ -209,12 +195,37 @@ bool Resolver::ResolveInternal() {
       if (!GlobalVariable(var)) {
         return false;
       }
+    } else {
+      TINT_UNREACHABLE(diagnostics_)
+          << "unhandled global declaration: " << decl->TypeInfo().name;
+      return false;
     }
   }
 
   return true;
 }
 
+Resolver::VariableInfo* Resolver::Variable(ast::Variable* var,
+                                           type::Type* type /*=nullptr*/) {
+  auto it = variable_to_info_.find(var);
+  if (it != variable_to_info_.end()) {
+    return it->second;
+  }
+
+  auto* ctype = Canonical(type ? type : var->declared_type());
+  auto* info = variable_infos_.Create(var, ctype);
+  variable_to_info_.emplace(var, info);
+
+  // Resolve variable's type
+  if (auto* arr = info->type->As<type::Array>()) {
+    if (!Array(arr, var->source())) {
+      return nullptr;
+    }
+  }
+
+  return info;
+}
+
 bool Resolver::GlobalVariable(ast::Variable* var) {
   if (variable_stack_.has(var->symbol())) {
     diagnostics_.add_error("v-0011",
@@ -224,7 +235,10 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     return false;
   }
 
-  auto* info = CreateVariableInfo(var);
+  auto* info = Variable(var);
+  if (!info) {
+    return false;
+  }
   variable_stack_.set_global(var->symbol(), info);
 
   if (!var->is_const() && info->storage_class == ast::StorageClass::kNone) {
@@ -535,7 +549,10 @@ bool Resolver::Function(ast::Function* func) {
 
   variable_stack_.push_scope();
   for (auto* param : func->params()) {
-    auto* param_info = CreateVariableInfo(param);
+    auto* param_info = Variable(param);
+    if (!param_info) {
+      return false;
+    }
     variable_stack_.set(param->symbol(), param_info);
     func_info->parameters.emplace_back(param_info);
 
@@ -1565,7 +1582,12 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
     }
   }
 
-  auto* info = CreateVariableInfo(var);
+  auto* info = Variable(var, type);
+  if (!info) {
+    return false;
+  }
+  // TODO(amaiorano): Remove this and fix tests. We're overriding the semantic
+  // type stored in info->type here with a possibly non-canonicalized type.
   info->type = type;
   variable_stack_.set(var->symbol(), info);
   current_block_->decls.push_back(var);
@@ -1597,12 +1619,6 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
   return true;
 }
 
-Resolver::VariableInfo* Resolver::CreateVariableInfo(ast::Variable* var) {
-  auto* info = variable_infos_.Create(var, Canonical(var->declared_type()));
-  variable_to_info_.emplace(var, info);
-  return info;
-}
-
 type::Type* Resolver::TypeOf(ast::Expression* expr) {
   auto it = expr_info_.find(expr);
   if (it != expr_info_.end()) {
@@ -1730,7 +1746,8 @@ void Resolver::CreateSemanticNodes() const {
 
 bool Resolver::DefaultAlignAndSize(type::Type* ty,
                                    uint32_t& align,
-                                   uint32_t& size) {
+                                   uint32_t& size,
+                                   const Source& source) {
   static constexpr uint32_t vector_size[] = {
       /* padding */ 0,
       /* padding */ 0,
@@ -1779,7 +1796,8 @@ bool Resolver::DefaultAlignAndSize(type::Type* ty,
     }
     return false;
   } else if (cty->Is<type::Array>()) {
-    if (auto* sem = Array(ty->UnwrapAliasIfNeeded()->As<type::Array>())) {
+    if (auto* sem =
+            Array(ty->UnwrapAliasIfNeeded()->As<type::Array>(), source)) {
       align = sem->Align();
       size = sem->Size();
       return true;
@@ -1790,7 +1808,7 @@ bool Resolver::DefaultAlignAndSize(type::Type* ty,
   return false;
 }
 
-const semantic::Array* Resolver::Array(type::Array* arr) {
+const semantic::Array* Resolver::Array(type::Array* arr, const Source& source) {
   if (auto* sem = builder_->Sem().Get(arr)) {
     // Semantic info already constructed for this array type
     return sem;
@@ -1800,15 +1818,16 @@ const semantic::Array* Resolver::Array(type::Array* arr) {
   auto* el_ty = arr->type();
   if (!IsStorable(el_ty)) {
     builder_->Diagnostics().add_error(
-        std::string(el_ty->FriendlyName(builder_->Symbols())) +
+        el_ty->FriendlyName(builder_->Symbols()) +
-        " cannot be used as an element type of an array");
+            " cannot be used as an element type of an array",
+        source);
     return nullptr;
   }
 
   auto create_semantic = [&](uint32_t stride) -> semantic::Array* {
     uint32_t el_align = 0;
     uint32_t el_size = 0;
-    if (!DefaultAlignAndSize(arr->type(), el_align, el_size)) {
+    if (!DefaultAlignAndSize(arr->type(), el_align, el_size, source)) {
       return nullptr;
     }
 
@@ -1831,7 +1850,7 @@ const semantic::Array* Resolver::Array(type::Array* arr) {
   // Calculate implicit stride
   uint32_t el_align = 0;
   uint32_t el_size = 0;
-  if (!DefaultAlignAndSize(el_ty, el_align, el_size)) {
+  if (!DefaultAlignAndSize(el_ty, el_align, el_size, source)) {
     return nullptr;
   }
 
@@ -1933,7 +1952,7 @@ Resolver::StructInfo* Resolver::Structure(type::Struct* str) {
     uint32_t offset = struct_size;
     uint32_t align = 0;
     uint32_t size = 0;
-    if (!DefaultAlignAndSize(member->type(), align, size)) {
+    if (!DefaultAlignAndSize(member->type(), align, size, member->source())) {
       return nullptr;
     }
 
@@ -2189,7 +2208,7 @@ bool Resolver::Assignment(ast::AssignmentStatement* a) {
 
 bool Resolver::ApplyStorageClassUsageToType(ast::StorageClass sc,
                                             type::Type* ty,
-                                            Source usage) {
+                                            const Source& usage) {
   ty = ty->UnwrapIfNeeded();
 
   if (auto* str = ty->As<type::Struct>()) {

src/resolver/resolver.h

@@ -251,12 +251,21 @@ class Resolver {
   /// @returns the semantic information for the array `arr`, building it if it
   /// hasn't been constructed already. If an error is raised, nullptr is
   /// returned.
-  const semantic::Array* Array(type::Array*);
+  /// @param arr the Array to get semantic information for
+  /// @param source the Source of the ast node with this array as its type
+  const semantic::Array* Array(type::Array* arr, const Source& source);
 
   /// @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);
 
+  /// @returns the VariableInfo for the variable `var`, building it if it hasn't
+  /// been constructed already. If an error is raised, nullptr is returned.
+  /// @param var the variable to create or return the `VariableInfo` for
+  /// @param type optional type of `var` to use instead of
+  /// `var->declared_type()`. For type inference.
+  VariableInfo* Variable(ast::Variable* var, type::Type* type = nullptr);
+
   /// Records the storage class usage for the given type, and any transient
   /// dependencies of the type. Validates that the type can be used for the
   /// given storage class, erroring if it cannot.
@@ -267,14 +276,16 @@ class Resolver {
   /// @returns true on success, false on error
   bool ApplyStorageClassUsageToType(ast::StorageClass sc,
                                     type::Type* ty,
-                                    Source usage);
+                                    const Source& usage);
 
   /// @param align the output default alignment in bytes for the type `ty`
   /// @param size the output default size in bytes for the type `ty`
+  /// @param source the Source of the variable declaration of type `ty`
   /// @returns true on success, false on error
-  bool DefaultAlignAndSize(type::Type* ty, uint32_t& align, uint32_t& size);
+  bool DefaultAlignAndSize(type::Type* ty,
-
+                           uint32_t& align,
-  VariableInfo* CreateVariableInfo(ast::Variable*);
+                           uint32_t& size,
+                           const Source& source);
 
   /// @returns the resolved type of the ast::Expression `expr`
   /// @param expr the expression

src/writer/spirv/builder_type_test.cc

@@ -59,6 +59,7 @@ TEST_F(BuilderTest_Type, ReturnsGeneratedAlias) {
 
 TEST_F(BuilderTest_Type, GenerateRuntimeArray) {
   auto* ary = ty.array(ty.i32(), 0);
+  Global("a", ary, ast::StorageClass::kInput);
 
   spirv::Builder& b = Build();
 
@@ -73,6 +74,7 @@ TEST_F(BuilderTest_Type, GenerateRuntimeArray) {
 
 TEST_F(BuilderTest_Type, ReturnsGeneratedRuntimeArray) {
   auto* ary = ty.array(ty.i32(), 0);
+  Global("a", ary, ast::StorageClass::kInput);
 
   spirv::Builder& b = Build();
 
@@ -87,6 +89,7 @@ TEST_F(BuilderTest_Type, ReturnsGeneratedRuntimeArray) {
 
 TEST_F(BuilderTest_Type, GenerateArray) {
   auto* ary = ty.array(ty.i32(), 4);
+  Global("a", ary, ast::StorageClass::kInput);
 
   spirv::Builder& b = Build();
 
@@ -103,6 +106,7 @@ TEST_F(BuilderTest_Type, GenerateArray) {
 
 TEST_F(BuilderTest_Type, GenerateArray_WithStride) {
   auto* ary = ty.array(ty.i32(), 4, 16u);
+  Global("a", ary, ast::StorageClass::kInput);
 
   spirv::Builder& b = Build();
 
@@ -122,6 +126,7 @@ TEST_F(BuilderTest_Type, GenerateArray_WithStride) {
 
 TEST_F(BuilderTest_Type, ReturnsGeneratedArray) {
   auto* ary = ty.array(ty.i32(), 4);
+  Global("a", ary, ast::StorageClass::kInput);
 
   spirv::Builder& b = Build();