Move assignment validation from Validator to Resolver

* With this change, ProgramBuilder::WrapInStatement(expr), which
produced an "expr = expr" assignment expression, would fail validation
in some cases like for call expressions. Replaced this with a
declaration of a variable with type inferred from expr.

* Moved existing validation tests to resolver\assignment_validation.cc,
and added missing tests: AssignFromPointer_Fail.

* Fixed broken tests as a result of this change.

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

src/program_builder.cc

@@ -15,6 +15,7 @@
 #include "src/program_builder.h"
 
 #include "src/ast/assignment_statement.h"
+#include "src/ast/call_statement.h"
 #include "src/ast/variable_decl_statement.h"
 #include "src/demangler.h"
 #include "src/semantic/expression.h"
@@ -126,9 +127,8 @@ ast::VariableDeclStatement* ProgramBuilder::WrapInStatement(ast::Variable* v) {
 }
 
 ast::Statement* ProgramBuilder::WrapInStatement(ast::Expression* expr) {
-  // TODO(ben-clayton): This is valid enough for the Resolver, but the LHS
+  // Create a temporary variable of inferred type from expr.
-  // may not be assignable, and so may not validate.
+  return Decl(Var(symbols_.New(), nullptr, ast::StorageClass::kFunction, expr));
-  return create<ast::AssignmentStatement>(expr, expr);
 }
 
 ast::Statement* ProgramBuilder::WrapInStatement(ast::Statement* stmt) {

src/resolver/assignment_validation_test.cc

@@ -16,6 +16,8 @@
 
 #include "gmock/gmock.h"
 #include "src/resolver/resolver_test_helper.h"
+#include "src/type/access_control_type.h"
+#include "src/type/storage_texture_type.h"
 
 namespace tint {
 namespace resolver {
@@ -139,6 +141,143 @@ TEST_F(ResolverAssignmentValidationTest,
       R"(12:34 error: invalid assignment: cannot assign value of type 'f32' to a variable of type 'i32')");
 }
 
+TEST_F(ResolverAssignmentValidationTest, AssignToScalar_Fail) {
+  // var my_var : i32 = 2;
+  // 1 = my_var;
+
+  auto* var = Var("my_var", ty.i32(), ast::StorageClass::kNone, Expr(2));
+  auto* lhs = Expr(1);
+  auto* rhs = Expr("my_var");
+
+  auto* assign = create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var), assign);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-000x: invalid assignment: left-hand-side does not "
+            "reference storage: i32");
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignCompatibleTypes_Pass) {
+  // var a :i32 = 2;
+  // a = 2
+  auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+
+  auto* lhs = Expr("a");
+  auto* rhs = Expr(2);
+
+  auto* assign = create<ast::AssignmentStatement>(
+      Source{Source::Location{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var), assign);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest,
+       AssignCompatibleTypesThroughAlias_Pass) {
+  // alias myint = i32;
+  // var a :myint = 2;
+  // a = 2
+  auto* myint = ty.alias("myint", ty.i32());
+  auto* var = Var("a", myint, ast::StorageClass::kNone, Expr(2));
+
+  auto* lhs = Expr("a");
+  auto* rhs = Expr(2);
+
+  auto* assign = create<ast::AssignmentStatement>(
+      Source{Source::Location{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var), assign);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest,
+       AssignCompatibleTypesInferRHSLoad_Pass) {
+  // var a : i32 = 2;
+  // var b : i32 = 3;
+  // a = b;
+  auto* var_a = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+  auto* var_b = Var("b", ty.i32(), ast::StorageClass::kNone, Expr(3));
+
+  auto* lhs = Expr("a");
+  auto* rhs = Expr("b");
+
+  auto* assign = create<ast::AssignmentStatement>(
+      Source{Source::Location{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignThroughPointer_Pass) {
+  // var a :i32;
+  // const b : ptr<function,i32> = a;
+  // b = 2;
+  const auto func = ast::StorageClass::kFunction;
+  auto* var_a = Var("a", ty.i32(), func, Expr(2), {});
+  auto* var_b = Const("b", ty.pointer<int>(func), Expr("a"), {});
+
+  auto* lhs = Expr("b");
+  auto* rhs = Expr(2);
+
+  auto* assign = create<ast::AssignmentStatement>(
+      Source{Source::Location{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignToConstant_Fail) {
+  // {
+  //  const a :i32 = 2;
+  //  a = 2
+  // }
+  auto* var = Const("a", ty.i32(), Expr(2));
+
+  auto* lhs = Expr("a");
+  auto* rhs = Expr(2);
+
+  auto* body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var),
+      create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
+                                       rhs),
+  });
+
+  WrapInFunction(body);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-0021: cannot re-assign a constant: 'a'");
+}
+
+TEST_F(ResolverAssignmentValidationTest, AssignFromPointer_Fail) {
+  // var a : [[access(read)]] texture_storage_1d<rgba8unorm>;
+  // var b : [[access(read)]] texture_storage_1d<rgba8unorm>;
+  // a = b;
+
+  auto* tex_type = create<type::StorageTexture>(
+      type::TextureDimension::k1d, type::ImageFormat::kRgba8Unorm,
+      type::StorageTexture::SubtypeFor(type::ImageFormat::kRgba8Unorm,
+                                       Types()));
+  auto* tex_ac =
+      create<type::AccessControl>(ast::AccessControl::kReadOnly, tex_type);
+
+  auto* var_a = Var("a", tex_ac, ast::StorageClass::kFunction);
+  auto* var_b = Var("b", tex_ac, ast::StorageClass::kFunction);
+
+  auto* lhs = Expr("a");
+  auto* rhs = Expr("b");
+
+  auto* assign = create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs);
+  WrapInFunction(Decl(var_a), Decl(var_b), assign);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-000x: invalid assignment: right-hand-side is not "
+            "storable: ptr<function, [[access(read)]] "
+            "texture_storage_1d<rgba8unorm>>");
+}
+
 }  // namespace
 }  // namespace resolver
 }  // namespace tint

src/resolver/resolver.cc

@@ -333,23 +333,7 @@ bool Resolver::Statement(ast::Statement* stmt) {
   ScopedAssignment<semantic::Statement*> sa(current_statement_, sem_statement);
 
   if (auto* a = stmt->As<ast::AssignmentStatement>()) {
-    if (!Expression(a->lhs()) || !Expression(a->rhs())) {
+    return Assignment(a);
-      return false;
-    }
-    // TODO(crbug.com/tint/659): This logic needs updating once pointers are
-    // pinned down in the WGSL spec.
-    auto* lhs_type = TypeOf(a->lhs())->UnwrapAll();
-    auto* rhs_type = TypeOf(a->rhs());
-    if (!IsValidAssignment(lhs_type, rhs_type)) {
-      diagnostics_.add_error(
-          "invalid assignment: cannot assign value of type '" +
-              rhs_type->FriendlyName(builder_->Symbols()) +
-              "' to a variable of type '" +
-              lhs_type->FriendlyName(builder_->Symbols()) + "'",
-          stmt->source());
-      return false;
-    }
-    return true;
   }
   if (auto* b = stmt->As<ast::BlockStatement>()) {
     return BlockStatement(b);
@@ -1770,6 +1754,74 @@ bool Resolver::Switch(ast::SwitchStatement* s) {
   return true;
 }
 
+bool Resolver::ValidateAssignment(const ast::AssignmentStatement* a) {
+  auto* lhs = a->lhs();
+  auto* rhs = a->rhs();
+
+  // TODO(crbug.com/tint/659): This logic needs updating once pointers are
+  // pinned down in the WGSL spec.
+  auto* lhs_type = TypeOf(lhs)->UnwrapAll();
+  auto* rhs_type = TypeOf(rhs);
+  if (!IsValidAssignment(lhs_type, rhs_type)) {
+    diagnostics_.add_error("invalid assignment: cannot assign value of type '" +
+                               rhs_type->FriendlyName(builder_->Symbols()) +
+                               "' to a variable of type '" +
+                               lhs_type->FriendlyName(builder_->Symbols()) +
+                               "'",
+                           a->source());
+    return false;
+  }
+
+  // Pointers are not storable in WGSL, but the right-hand side must be
+  // storable. The raw right-hand side might be a pointer value which must be
+  // loaded (dereferenced) to provide the value to be stored.
+  auto* rhs_result_type = TypeOf(rhs)->UnwrapAll();
+  if (!IsStorable(rhs_result_type)) {
+    diagnostics_.add_error(
+        "v-000x",
+        "invalid assignment: right-hand-side is not storable: " +
+            TypeOf(rhs)->FriendlyName(builder_->Symbols()),
+        a->source());
+    return false;
+  }
+
+  // lhs must be a pointer or a constant
+  auto* lhs_result_type = TypeOf(lhs)->UnwrapIfNeeded();
+  if (!lhs_result_type->Is<type::Pointer>()) {
+    // In case lhs is a constant identifier, output a nicer message as it's
+    // likely to be a common programmer error.
+    if (auto* ident = lhs->As<ast::IdentifierExpression>()) {
+      VariableInfo* var;
+      if (variable_stack_.get(ident->symbol(), &var) &&
+          var->declaration->is_const()) {
+        diagnostics_.add_error(
+            "v-0021",
+            "cannot re-assign a constant: '" +
+                builder_->Symbols().NameFor(ident->symbol()) + "'",
+            a->source());
+        return false;
+      }
+    }
+
+    // Issue a generic error.
+    diagnostics_.add_error(
+        "v-000x",
+        "invalid assignment: left-hand-side does not reference storage: " +
+            TypeOf(lhs)->FriendlyName(builder_->Symbols()),
+        a->source());
+    return false;
+  }
+
+  return true;
+}
+
+bool Resolver::Assignment(ast::AssignmentStatement* a) {
+  if (!Expression(a->lhs()) || !Expression(a->rhs())) {
+    return false;
+  }
+  return ValidateAssignment(a);
+}
+
 bool Resolver::ApplyStorageClassUsageToType(ast::StorageClass sc,
                                             type::Type* ty,
                                             Source usage) {

src/resolver/resolver.h

@@ -223,6 +223,7 @@ class Resolver {
   bool VariableDeclStatement(const ast::VariableDeclStatement*);
   bool Return(ast::ReturnStatement* ret);
   bool Switch(ast::SwitchStatement* s);
+  bool Assignment(ast::AssignmentStatement* a);
 
   // AST and Type validation methods
   // Each return true on success, false on failure.
@@ -232,6 +233,7 @@ class Resolver {
   bool ValidateStructure(const type::Struct* st);
   bool ValidateReturn(const ast::ReturnStatement* ret);
   bool ValidateSwitch(const ast::SwitchStatement* s);
+  bool ValidateAssignment(const ast::AssignmentStatement* a);
 
   /// @returns the semantic information for the array `arr`, building it if it
   /// hasn't been constructed already. If an error is raised, nullptr is

src/resolver/resolver_test.cc

@@ -639,14 +639,13 @@ TEST_F(ResolverTest, Expr_Identifier_GlobalConstant) {
 
 TEST_F(ResolverTest, Expr_Identifier_FunctionVariable_Const) {
   auto* my_var_a = Expr("my_var");
-  auto* my_var_b = Expr("my_var");
   auto* var = Const("my_var", ty.f32());
-  auto* assign = create<ast::AssignmentStatement>(my_var_a, my_var_b);
+  auto* decl = Decl(Var("b", ty.f32(), ast::StorageClass::kFunction, my_var_a));
 
   Func("my_func", ast::VariableList{}, ty.void_(),
        ast::StatementList{
            create<ast::VariableDeclStatement>(var),
-           assign,
+           decl,
        },
        ast::DecorationList{});
 
@@ -654,11 +653,8 @@ TEST_F(ResolverTest, Expr_Identifier_FunctionVariable_Const) {
 
   ASSERT_NE(TypeOf(my_var_a), nullptr);
   EXPECT_TRUE(TypeOf(my_var_a)->Is<type::F32>());
-  EXPECT_EQ(StmtOf(my_var_a), assign);
+  EXPECT_EQ(StmtOf(my_var_a), decl);
-  ASSERT_NE(TypeOf(my_var_b), nullptr);
+  EXPECT_TRUE(CheckVarUsers(var, {my_var_a}));
-  EXPECT_TRUE(TypeOf(my_var_b)->Is<type::F32>());
-  EXPECT_EQ(StmtOf(my_var_b), assign);
-  EXPECT_TRUE(CheckVarUsers(var, {my_var_a, my_var_b}));
 }
 
 TEST_F(ResolverTest, Expr_Identifier_FunctionVariable) {

src/validator/validator_impl.cc

@@ -238,9 +238,6 @@ bool ValidatorImpl::ValidateStatement(const ast::Statement* stmt) {
   if (auto* v = stmt->As<ast::VariableDeclStatement>()) {
     return ValidateDeclStatement(v);
   }
-  if (auto* a = stmt->As<ast::AssignmentStatement>()) {
-    return ValidateAssign(a);
-  }
   if (auto* s = stmt->As<ast::SwitchStatement>()) {
     return ValidateSwitch(s);
   }
@@ -272,66 +269,6 @@ bool ValidatorImpl::ValidateCase(const ast::CaseStatement* c) {
   return true;
 }
 
-bool ValidatorImpl::ValidateBadAssignmentToIdentifier(
-    const ast::AssignmentStatement* assign) {
-  auto* ident = assign->lhs()->As<ast::IdentifierExpression>();
-  if (!ident) {
-    // It wasn't an identifier in the first place.
-    return true;
-  }
-  const ast::Variable* var;
-  if (variable_stack_.get(ident->symbol(), &var)) {
-    // Give a nicer message if the LHS of the assignment is a const identifier.
-    // It's likely to be a common programmer error.
-    if (var->is_const()) {
-      add_error(assign->source(), "v-0021",
-                "cannot re-assign a constant: '" +
-                    program_->Symbols().NameFor(ident->symbol()) + "'");
-      return false;
-    }
-  } else {
-    // The identifier is not defined.
-    // Shouldn't reach here. This should already have been caught when
-    // validation expressions in the Resolver
-    TINT_UNREACHABLE(diagnostics());
-    return false;
-  }
-  return true;
-}
-
-bool ValidatorImpl::ValidateAssign(const ast::AssignmentStatement* assign) {
-  if (!assign) {
-    return false;
-  }
-  auto* lhs = assign->lhs();
-  auto* rhs = assign->rhs();
-
-  // Pointers are not storable in WGSL, but the right-hand side must be
-  // storable. The raw right-hand side might be a pointer value which must be
-  // loaded (dereferenced) to provide the value to be stored.
-  auto* rhs_result_type = program_->Sem().Get(rhs)->Type()->UnwrapAll();
-  if (!IsStorable(rhs_result_type)) {
-    add_error(assign->source(), "v-000x",
-              "invalid assignment: right-hand-side is not storable: " +
-                  program_->Sem().Get(rhs)->Type()->type_name());
-    return false;
-  }
-  auto* lhs_result_type = program_->Sem().Get(lhs)->Type()->UnwrapIfNeeded();
-  if (!Is<type::Pointer>(lhs_result_type)) {
-    if (!ValidateBadAssignmentToIdentifier(assign)) {
-      return false;
-    }
-    // Issue a generic error.
-    add_error(
-        assign->source(), "v-000x",
-        "invalid assignment: left-hand-side does not reference storage: " +
-            program_->Sem().Get(lhs)->Type()->type_name());
-    return false;
-  }
-
-  return true;
-}
-
 bool ValidatorImpl::IsStorable(type::Type* type) {
   if (type == nullptr) {
     return false;

src/validator/validator_impl.h

@@ -84,16 +84,6 @@ class ValidatorImpl {
   /// @param stmt the statement to check
   /// @returns true if the validation was successful
   bool ValidateStatement(const ast::Statement* stmt);
-  /// Validates an assignment
-  /// @param assign the assignment to check
-  /// @returns true if the validation was successful
-  bool ValidateAssign(const ast::AssignmentStatement* assign);
-  /// Validates a bad assignment to an identifier. Issues an error
-  /// and returns false if the left hand side is an identifier.
-  /// @param assign the assignment to check
-  /// @returns true if the LHS of theassignment is not an identifier expression
-  bool ValidateBadAssignmentToIdentifier(
-      const ast::AssignmentStatement* assign);
   /// Validates declaration name uniqueness
   /// @param decl is the new declaration to be added
   /// @returns true if no previous declaration with the `decl` 's name

src/validator/validator_test.cc

@@ -21,123 +21,6 @@ namespace {
 
 class ValidatorTest : public ValidatorTestHelper, public testing::Test {};
 
-TEST_F(ValidatorTest, AssignToScalar_Fail) {
-  // var my_var : i32 = 2;
-  // 1 = my_var;
-
-  auto* var = Var("my_var", ty.i32(), ast::StorageClass::kNone, Expr(2));
-  RegisterVariable(var);
-
-  auto* lhs = Expr(1);
-  auto* rhs = Expr("my_var");
-
-  SetSource(Source{Source::Location{12, 34}});
-  auto* assign = create<ast::AssignmentStatement>(lhs, rhs);
-  WrapInFunction(assign);
-
-  ValidatorImpl& v = Build();
-
-  // TODO(sarahM0): Invalidate assignment to scalar.
-  EXPECT_FALSE(v.ValidateAssign(assign));
-  ASSERT_TRUE(v.has_error());
-  // TODO(sarahM0): figure out what should be the error number.
-  EXPECT_EQ(v.error(),
-            "12:34 v-000x: invalid assignment: left-hand-side does not "
-            "reference storage: __i32");
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypes_Pass) {
-  // var a :i32 = 2;
-  // a = 2
-  auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
-  RegisterVariable(var);
-
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(2);
-
-  auto* assign = create<ast::AssignmentStatement>(
-      Source{Source::Location{12, 34}}, lhs, rhs);
-  WrapInFunction(assign);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypesThroughAlias_Pass) {
-  // alias myint = i32;
-  // var a :myint = 2;
-  // a = 2
-  auto* myint = ty.alias("myint", ty.i32());
-  auto* var = Var("a", myint, ast::StorageClass::kNone, Expr(2));
-  RegisterVariable(var);
-
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(2);
-
-  auto* assign = create<ast::AssignmentStatement>(
-      Source{Source::Location{12, 34}}, lhs, rhs);
-  WrapInFunction(assign);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignCompatibleTypesInferRHSLoad_Pass) {
-  // var a :i32 = 2;
-  // var b :i32 = 3;
-  // a = b;
-  auto* var_a = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
-  auto* var_b = Var("b", ty.i32(), ast::StorageClass::kNone, Expr(3));
-  RegisterVariable(var_a);
-  RegisterVariable(var_b);
-
-  auto* lhs = Expr("a");
-  auto* rhs = Expr("b");
-
-  auto* assign = create<ast::AssignmentStatement>(
-      Source{Source::Location{12, 34}}, lhs, rhs);
-  WrapInFunction(assign);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
-
-TEST_F(ValidatorTest, AssignThroughPointer_Pass) {
-  // var a :i32;
-  // const b : ptr<function,i32> = a;
-  // b = 2;
-  const auto func = ast::StorageClass::kFunction;
-  auto* var_a = Var("a", ty.i32(), func, Expr(2), {});
-  auto* var_b = Const("b", ty.pointer<int>(func), Expr("a"), {});
-  RegisterVariable(var_a);
-  RegisterVariable(var_b);
-
-  auto* lhs = Expr("b");
-  auto* rhs = Expr(2);
-
-  auto* assign = create<ast::AssignmentStatement>(
-      Source{Source::Location{12, 34}}, lhs, rhs);
-  WrapInFunction(assign);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_TRUE(v.ValidateAssign(assign)) << v.error();
-}
 
 TEST_F(ValidatorTest, GlobalVariableWithStorageClass_Pass) {
   // var<in> global_var: f32;
@@ -215,33 +98,6 @@ TEST_F(ValidatorTest, GlobalVariableNotUnique_Fail) {
             "12:34 v-0011: redeclared global identifier 'global_var'");
 }
 
-TEST_F(ValidatorTest, AssignToConstant_Fail) {
-  // {
-  //  const a :i32 = 2;
-  //  a = 2
-  // }
-  auto* var = Const("a", ty.i32(), Expr(2));
-
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(2);
-
-  auto* body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var),
-      create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
-                                       rhs),
-  });
-
-  WrapInFunction(body);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_FALSE(v.ValidateStatements(body));
-  EXPECT_EQ(v.error(), "12:34 v-0021: cannot re-assign a constant: 'a'");
-}
-
 TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Pass) {
   // fn my_func -> void {
   //   var a: f32 = 2.0;