Resolver: Move common logic into Variable()

Variable() is called for globals, locals and parameters. Much of the logic is the same.

Move all the common logic down into Variable(). This:
* Removes some yucky default parameters
* Adds type validation that was missing for globals (broken tests fixed)
* Gives me a single place to implement the Reference type wrapping

Bug: tint:727
Change-Id: I70f4a3603d7fa781da938508aa2a1bc80ec15d77
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50580
Auto-Submit: Ben Clayton <bclayton@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

src/resolver/resolver.cc

@@ -413,20 +413,57 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
   return s;
 }
 
-Resolver::VariableInfo* Resolver::Variable(
+Resolver::VariableInfo* Resolver::Variable(ast::Variable* var,
-    ast::Variable* var,
+                                           bool is_parameter) {
-    const sem::Type* type, /* = nullptr */
+  if (variable_to_info_.count(var)) {
-    std::string type_name /* = "" */) {
+    TINT_ICE(diagnostics_) << "Variable "
-  auto it = variable_to_info_.find(var);
+                           << builder_->Symbols().NameFor(var->symbol())
-  if (it != variable_to_info_.end()) {
+                           << " already resolved";
-    return it->second;
+    return nullptr;
   }
 
-  if (type == nullptr && var->type()) {
+  // If the variable has a declared type, resolve it.
-    type = Type(var->type());
+  std::string type_name;
-    type_name = var->type()->FriendlyName(builder_->Symbols());
+  const sem::Type* type = nullptr;
+  if (auto* ty = var->type()) {
+    type_name = ty->FriendlyName(builder_->Symbols());
+    type = Type(ty);
+    if (!type) {
+      return nullptr;
     }
+  }
+
+  // Does the variable have a constructor?
+  if (auto* ctor = var->constructor()) {
+    Mark(var->constructor());
+    if (!Expression(var->constructor())) {
+      return nullptr;
+    }
+
+    // Fetch the constructor's type
+    auto* rhs_type = TypeOf(ctor);
+    if (!rhs_type) {
+      return nullptr;
+    }
+
+    // If the variable has no declared type, infer it from the RHS
     if (type == nullptr) {
+      type_name = TypeNameOf(ctor);
+      type = rhs_type->UnwrapPtr();
+    }
+
+    if (!IsValidAssignment(type, rhs_type)) {
+      diagnostics_.add_error(
+          "variable of type '" + type_name +
+              "' cannot be initialized with a value of type '" +
+              TypeNameOf(ctor) + "'",
+          var->source());
+      return nullptr;
+    }
+  } else if (var->is_const() && !is_parameter &&
+             !ast::HasDecoration<ast::OverrideDecoration>(var->decorations())) {
+    diagnostics_.add_error("let declarations must have initializers",
+                           var->source());
     return nullptr;
   }
 
@@ -446,7 +483,7 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     return false;
   }
 
-  auto* info = Variable(var);
+  auto* info = Variable(var, /* is_parameter */ false);
   if (!info) {
     return false;
   }
@@ -472,20 +509,6 @@ bool Resolver::GlobalVariable(ast::Variable* var) {
     info->binding_point = {bp.group->value(), bp.binding->value()};
   }
 
-  if (var->has_constructor()) {
-    Mark(var->constructor());
-    if (!Expression(var->constructor())) {
-      return false;
-    }
-  } else {
-    if (var->is_const() &&
-        !ast::HasDecoration<ast::OverrideDecoration>(var->decorations())) {
-      diagnostics_.add_error("let declarations must have initializers",
-                             var->source());
-      return false;
-    }
-  }
-
   if (!ValidateGlobalVariable(info)) {
     return false;
   }
@@ -1020,7 +1043,7 @@ bool Resolver::Function(ast::Function* func) {
   variable_stack_.push_scope();
   for (auto* param : func->params()) {
     Mark(param);
-    auto* param_info = Variable(param);
+    auto* param_info = Variable(param, /* is_parameter */ true);
     if (!param_info) {
       return false;
     }
@@ -2072,17 +2095,6 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
   ast::Variable* var = stmt->variable();
   Mark(var);
 
-  // If the variable has a declared type, resolve it.
-  std::string type_name;
-  const sem::Type* type = nullptr;
-  if (auto* ast_ty = var->type()) {
-    type_name = ast_ty->FriendlyName(builder_->Symbols());
-    type = Type(ast_ty);
-    if (!type) {
-      return false;
-    }
-  }
-
   bool is_global = false;
   if (variable_stack_.get(var->symbol(), nullptr, &is_global)) {
     const char* error_code = is_global ? "v-0013" : "v-0014";
@@ -2093,47 +2105,16 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
     return false;
   }
 
-  if (auto* ctor = stmt->variable()->constructor()) {
+  auto* info = Variable(var, /* is_parameter */ false);
-    Mark(ctor);
+  if (!info) {
-    if (!Expression(ctor)) {
     return false;
   }
-    auto* rhs_type = TypeOf(ctor);
-
-    // If the variable has no type, infer it from the rhs
-    if (type == nullptr) {
-      type_name = TypeNameOf(ctor);
-      type = rhs_type->UnwrapPtr();
-    }
-
-    if (!IsValidAssignment(type, rhs_type)) {
-      diagnostics_.add_error(
-          "variable of type '" + type_name +
-              "' cannot be initialized with a value of type '" +
-              TypeNameOf(ctor) + "'",
-          stmt->source());
-      return false;
-    }
-  } else {
-    if (stmt->variable()->is_const()) {
-      diagnostics_.add_error("let declarations must have initializers",
-                             var->source());
-      return false;
-    }
-  }
 
   for (auto* deco : var->decorations()) {
     // TODO(bclayton): Validate decorations
     Mark(deco);
   }
 
-  auto* info = Variable(var, type, type_name);
-  if (!info) {
-    return false;
-  }
-  // TODO(bclayton): Remove this and fix tests. We're overriding the semantic
-  // type stored in info->type here with a possibly non-canonicalized type.
-  info->type = const_cast<sem::Type*>(type);
   variable_stack_.set(var->symbol(), info);
   current_block_->decls.push_back(var);
 
@@ -2212,8 +2193,7 @@ void Resolver::SetType(ast::Expression* expr,
     TINT_ASSERT(type.sem);
   }
   if (expr_info_.count(expr)) {
-    TINT_ICE(builder_->Diagnostics())
+    TINT_ICE(diagnostics_) << "SetType() called twice for the same expression";
-        << "SetType() called twice for the same expression";
   }
   expr_info_.emplace(expr, ExpressionInfo{type, type_name, current_statement_});
 }
@@ -2260,8 +2240,7 @@ void Resolver::CreateSemanticNodes() const {
       } else {
         auto* sem_user = sem_expr->As<sem::VariableUser>();
         if (!sem_user) {
-          TINT_ICE(builder_->Diagnostics())
+          TINT_ICE(diagnostics_) << "expected sem::VariableUser, got "
-              << "expected sem::VariableUser, got "
                                  << sem_expr->TypeInfo().name;
         }
         sem_var->AddUser(sem_user);

src/resolver/resolver.h

@@ -262,13 +262,11 @@ class Resolver {
 
   /// @returns the VariableInfo for the variable `var`, building it if it hasn't
   /// been constructed already. If an error is raised, nullptr is returned.
+  /// @note this method does not resolve the decorations as these are
+  /// context-dependent (global, local, parameter)
   /// @param var the variable to create or return the `VariableInfo` for
-  /// @param type optional type of `var` to use instead of `var->type()`.
+  /// @param is_parameter true if the variable represents a parameter
-  /// @param type_name optional type name of `var` to use instead of
+  VariableInfo* Variable(ast::Variable* var, bool is_parameter);
-  /// `var->type()->FriendlyName()`.
-  VariableInfo* Variable(ast::Variable* var,
-                         const sem::Type* type = nullptr,
-                         std::string type_name = "");
 
   /// 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

src/resolver/type_validation_test.cc

@@ -90,8 +90,8 @@ TEST_F(ResolverTypeValidationTest, GlobalConstantWithStorageClass_Fail) {
   // const<in> global_var: f32;
   AST().AddGlobalVariable(
       create<ast::Variable>(Source{{12, 34}}, Symbols().Register("global_var"),
-                            ast::StorageClass::kInput, ty.f32(), true, nullptr,
+                            ast::StorageClass::kInput, ty.f32(), true,
-                            ast::DecorationList{}));
+                            Expr(1.23f), ast::DecorationList{}));
 
   EXPECT_FALSE(r()->Resolve());
   EXPECT_EQ(r()->error(),
@@ -113,7 +113,7 @@ TEST_F(ResolverTypeValidationTest, GlobalVariableUnique_Pass) {
   Global("global_var0", ty.f32(), ast::StorageClass::kPrivate, Expr(0.1f));
 
   Global(Source{{12, 34}}, "global_var1", ty.f32(), ast::StorageClass::kPrivate,
-         Expr(0));
+         Expr(1.0f));
 
   EXPECT_TRUE(r()->Resolve()) << r()->error();
 }

src/resolver/validation_test.cc

@@ -164,10 +164,8 @@ TEST_F(ResolverValidationTest, Stmt_Else_NonBool) {
 TEST_F(ResolverValidationTest,
        Stmt_VariableDecl_MismatchedTypeScalarConstructor) {
   u32 unsigned_value = 2u;  // Type does not match variable type
-  auto* var =
+  auto* decl = Decl(Var(Source{{3, 3}}, "my_var", ty.i32(),
-      Var("my_var", ty.i32(), ast::StorageClass::kNone, Expr(unsigned_value));
+                        ast::StorageClass::kNone, Expr(unsigned_value)));
-
-  auto* decl = Decl(Source{{{3, 3}, {3, 22}}}, var);
   WrapInFunction(decl);
 
   EXPECT_FALSE(r()->Resolve());
@@ -181,10 +179,8 @@ TEST_F(ResolverValidationTest,
   auto* my_int = ty.alias("MyInt", ty.i32());
   AST().AddConstructedType(my_int);
   u32 unsigned_value = 2u;  // Type does not match variable type
-  auto* var =
+  auto* decl = Decl(Var(Source{{3, 3}}, "my_var", my_int,
-      Var("my_var", my_int, ast::StorageClass::kNone, Expr(unsigned_value));
+                        ast::StorageClass::kNone, Expr(unsigned_value)));
-
-  auto* decl = Decl(Source{{{3, 3}, {3, 22}}}, var);
   WrapInFunction(decl);
 
   EXPECT_FALSE(r()->Resolve());

src/writer/spirv/builder_function_variable_test.cc

@@ -45,7 +45,7 @@ TEST_F(BuilderTest, FunctionVar_NoStorageClass) {
 TEST_F(BuilderTest, FunctionVar_WithConstantConstructor) {
   auto* init = vec3<f32>(1.f, 1.f, 3.f);
 
-  auto* v = Global("var", ty.f32(), ast::StorageClass::kOutput, init);
+  auto* v = Global("var", ty.vec3<f32>(), ast::StorageClass::kOutput, init);
 
   spirv::Builder& b = Build();
 
@@ -60,8 +60,8 @@ TEST_F(BuilderTest, FunctionVar_WithConstantConstructor) {
 %3 = OpConstant %2 1
 %4 = OpConstant %2 3
 %5 = OpConstantComposite %1 %3 %3 %4
-%7 = OpTypePointer Function %2
+%7 = OpTypePointer Function %1
-%8 = OpConstantNull %2
+%8 = OpConstantNull %1
 )");
   EXPECT_EQ(DumpInstructions(b.functions()[0].variables()),
             R"(%6 = OpVariable %7 Function %8

src/writer/spirv/builder_global_variable_test.cc

@@ -57,7 +57,7 @@ TEST_F(BuilderTest, GlobalVar_WithStorageClass_Input) {
 TEST_F(BuilderTest, GlobalVar_WithConstructor) {
   auto* init = vec3<f32>(1.f, 1.f, 3.f);
 
-  auto* v = Global("var", ty.f32(), ast::StorageClass::kOutput, init);
+  auto* v = Global("var", ty.vec3<f32>(), ast::StorageClass::kOutput, init);
 
   spirv::Builder& b = Build();
 
@@ -71,7 +71,7 @@ TEST_F(BuilderTest, GlobalVar_WithConstructor) {
 %3 = OpConstant %2 1
 %4 = OpConstant %2 3
 %5 = OpConstantComposite %1 %3 %3 %4
-%7 = OpTypePointer Output %2
+%7 = OpTypePointer Output %1
 %6 = OpVariable %7 Output %5
 )");
 }
@@ -79,7 +79,7 @@ TEST_F(BuilderTest, GlobalVar_WithConstructor) {
 TEST_F(BuilderTest, GlobalVar_Const) {
   auto* init = vec3<f32>(1.f, 1.f, 3.f);
 
-  auto* v = GlobalConst("var", ty.f32(), init);
+  auto* v = GlobalConst("var", ty.vec3<f32>(), init);
 
   spirv::Builder& b = Build();
 
@@ -99,7 +99,7 @@ TEST_F(BuilderTest, GlobalVar_Const) {
 TEST_F(BuilderTest, GlobalVar_Complex_Constructor) {
   auto* init = vec3<f32>(ast::ExpressionList{Expr(1.f), Expr(2.f), Expr(3.f)});
 
-  auto* v = GlobalConst("var", ty.f32(), init);
+  auto* v = GlobalConst("var", ty.vec3<f32>(), init);
 
   spirv::Builder& b = Build();
 
@@ -118,7 +118,7 @@ TEST_F(BuilderTest, GlobalVar_Complex_Constructor) {
 TEST_F(BuilderTest, GlobalVar_Complex_ConstructorWithExtract) {
   auto* init = vec3<f32>(vec2<f32>(1.f, 2.f), 3.f);
 
-  auto* v = GlobalConst("var", ty.f32(), init);
+  auto* v = GlobalConst("var", ty.vec3<f32>(), init);
 
   spirv::Builder& b = Build();
 

src/writer/spirv/builder_ident_expression_test.cc

@@ -25,7 +25,7 @@ using BuilderTest = TestHelper;
 TEST_F(BuilderTest, IdentifierExpression_GlobalConst) {
   auto* init = vec3<f32>(1.f, 1.f, 3.f);
 
-  auto* v = GlobalConst("var", ty.f32(), init);
+  auto* v = GlobalConst("var", ty.vec3<f32>(), init);
 
   auto* expr = Expr("var");
   WrapInFunction(expr);