Move VariableDecl validation from Validator to Resolver

Moved tests and fixed now broken tests.

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

src/CMakeLists.txt

@@ -524,7 +524,6 @@ if(${TINT_BUILD_TESTS})
     validator/validator_decoration_test.cc
     validator/validator_function_test.cc
     validator/validator_test.cc
-    validator/validator_type_test.cc
     validator/validator_test_helper.cc
     validator/validator_test_helper.h
     writer/append_vector_test.cc

src/resolver/assignment_validation_test.cc

@@ -81,9 +81,9 @@ TEST_F(ResolverAssignmentValidationTest,
       create<ast::VariableDeclStatement>(var),
       create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs),
   });
-  WrapInFunction(var, body);
+  WrapInFunction(body);
 
-  ASSERT_TRUE(r()->Resolve());
+  ASSERT_TRUE(r()->Resolve()) << r()->error();
 }
 
 TEST_F(ResolverAssignmentValidationTest,
@@ -101,7 +101,7 @@ TEST_F(ResolverAssignmentValidationTest,
       create<ast::VariableDeclStatement>(var),
       create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs),
   });
-  WrapInFunction(var, block);
+  WrapInFunction(block);
 
   ASSERT_FALSE(r()->Resolve());
 
@@ -132,7 +132,7 @@ TEST_F(ResolverAssignmentValidationTest,
       inner_block,
   });
 
-  WrapInFunction(var, outer_block);
+  WrapInFunction(outer_block);
 
   ASSERT_FALSE(r()->Resolve());
 

src/resolver/resolver.cc

@@ -224,20 +224,24 @@ bool Resolver::ResolveInternal() {
   return true;
 }
 
-bool Resolver::ValidateParameter(const ast::Variable* param) {
+bool Resolver::ValidateVariable(const ast::Variable* var) {
-  auto* type = variable_to_info_[param]->type;
+  auto* type = variable_to_info_[var]->type;
   if (auto* r = type->UnwrapAll()->As<type::Array>()) {
     if (r->IsRuntimeArray()) {
       diagnostics_.add_error(
           "v-0015",
           "runtime arrays may only appear as the last member of a struct",
-          param->source());
+          var->source());
       return false;
     }
   }
   return true;
 }
 
+bool Resolver::ValidateParameter(const ast::Variable* param) {
+  return ValidateVariable(param);
+}
+
 bool Resolver::ValidateFunction(const ast::Function* func) {
   if (symbol_to_function_.find(func->symbol()) != symbol_to_function_.end()) {
     diagnostics_.add_error("v-0016",
@@ -1278,6 +1282,16 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
   ast::Variable* var = stmt->variable();
   type::Type* type = var->declared_type();
 
+  bool is_global = false;
+  if (variable_stack_.get(var->symbol(), nullptr, &is_global)) {
+    const char* error_code = is_global ? "v-0013" : "v-0014";
+    diagnostics_.add_error(error_code,
+                           "redeclared identifier '" +
+                               builder_->Symbols().NameFor(var->symbol()) + "'",
+                           stmt->source());
+    return false;
+  }
+
   if (auto* ctor = stmt->variable()->constructor()) {
     if (!Expression(ctor)) {
       return false;
@@ -1304,6 +1318,10 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
   variable_stack_.set(var->symbol(), info);
   current_block_->decls.push_back(var);
 
+  if (!ValidateVariable(var)) {
+    return false;
+  }
+
   if (!var->is_const()) {
     if (info->storage_class != ast::StorageClass::kFunction) {
       if (info->storage_class != ast::StorageClass::kNone) {

src/resolver/resolver.h

@@ -231,6 +231,7 @@ class Resolver {
   // AST and Type validation methods
   // Each return true on success, false on failure.
   bool ValidateBinary(ast::BinaryExpression* expr);
+  bool ValidateVariable(const ast::Variable* param);
   bool ValidateParameter(const ast::Variable* param);
   bool ValidateFunction(const ast::Function* func);
   bool ValidateStructure(const type::Struct* st);

src/resolver/type_validation_test.cc

@@ -26,6 +26,199 @@ namespace {
 class ResolverTypeValidationTest : public resolver::TestHelper,
                                    public testing::Test {};
 
+TEST_F(ResolverTypeValidationTest,
+       GlobalVariableFunctionVariableNotUnique_Fail) {
+  // var a: f32 = 2.1;
+  // fn my_func -> void {
+  //   var a: f32 = 2.0;
+  //   return 0;
+  // }
+
+  Global("a", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
+
+  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+  Func("my_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                              var),
+       },
+       ast::DecorationList{});
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(r()->error(), "12:34 error v-0013: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifier_Fail) {
+  // fn my_func() -> void {
+  //  var a :i32 = 2;
+  //  var a :f21 = 2.0;
+  // }
+  auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+
+  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(0.1f));
+
+  Func("my_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(var),
+           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                              var_a_float),
+       },
+       ast::DecorationList{});
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScope_Pass) {
+  // {
+  // if (true) { var a : f32 = 2.0; }
+  // var a : f32 = 3.14;
+  // }
+  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+  auto* cond = Expr(true);
+  auto* body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var),
+  });
+
+  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
+
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                         var_a_float),
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_TRUE(r()->Resolve());
+}
+
+TEST_F(ResolverTypeValidationTest,
+       DISABLED_RedeclaredIdentifierInnerScope_False) {
+  // TODO(sarahM0): remove DISABLED after implementing ValidateIfStatement
+  // and it should just work
+  // {
+  // var a : f32 = 3.14;
+  // if (true) { var a : f32 = 2.0; }
+  // }
+  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
+
+  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+  auto* cond = Expr(true);
+  auto* body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}}, var),
+  });
+
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var_a_float),
+      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScopeBlock_Pass) {
+  // {
+  //  { var a : f32; }
+  //  var a : f32;
+  // }
+  auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
+  auto* inner = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                         var_inner),
+  });
+
+  auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      inner,
+      create<ast::VariableDeclStatement>(var_outer),
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScopeBlock_Fail) {
+  // {
+  //  var a : f32;
+  //  { var a : f32; }
+  // }
+  auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
+  auto* inner = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                         var_inner),
+  });
+
+  auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var_outer),
+      inner,
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest,
+       RedeclaredIdentifierDifferentFunctions_Pass) {
+  // func0 { var a : f32 = 2.0; return; }
+  // func1 { var a : f32 = 3.0; return; }
+  auto* var0 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+  auto* var1 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(1.0f));
+
+  Func("func0", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+                                              var0),
+           create<ast::ReturnStatement>(),
+       },
+       ast::DecorationList{});
+
+  Func("func1", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(Source{Source::Location{13, 34}},
+                                              var1),
+           create<ast::ReturnStatement>(),
+       },
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverTypeValidationTest, RuntimeArrayInFunction_Fail) {
+  /// [[stage(vertex)]]
+  // fn func -> void { var a : array<i32>; }
+
+  auto* var =
+      Var(Source{{12, 34}}, "a", ty.array<i32>(), ast::StorageClass::kNone);
+
+  Func("func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(var),
+       },
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(
+      r()->error(),
+      "12:34 error v-0015: runtime arrays may only appear as the last member "
+      "of a struct");
+}
+
 TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLast_Pass) {
   // [[Block]]
   // struct Foo {

src/validator/validator_impl.cc

@@ -195,31 +195,7 @@ bool ValidatorImpl::ValidateStatements(const ast::BlockStatement* block) {
 bool ValidatorImpl::ValidateDeclStatement(
     const ast::VariableDeclStatement* decl) {
   auto symbol = decl->variable()->symbol();
-  bool is_global = false;
-  if (variable_stack_.get(symbol, nullptr, &is_global)) {
-    const char* error_code = "v-0014";
-    if (is_global) {
-      error_code = "v-0013";
-    }
-    add_error(
-        decl->source(), error_code,
-        "redeclared identifier '" + program_->Symbols().NameFor(symbol) + "'");
-    return false;
-  }
-  // TODO(dneto): Check type compatibility of the initializer.
-  //  - if it's non-constant, then is storable or can be dereferenced to be
-  //    storable.
-  //  - types match or the RHS can be dereferenced to equal the LHS type.
   variable_stack_.set(symbol, decl->variable());
-  if (auto* arr =
-          decl->variable()->declared_type()->UnwrapAll()->As<type::Array>()) {
-    if (arr->IsRuntimeArray()) {
-      add_error(
-          decl->source(), "v-0015",
-          "runtime arrays may only appear as the last member of a struct");
-      return false;
-    }
-  }
   return true;
 }
 

src/validator/validator_test.cc

@@ -120,188 +120,6 @@ TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Pass) {
   EXPECT_TRUE(v.Validate()) << v.error();
 }
 
-TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Fail) {
-  // var a: f32 = 2.1;
-  // fn my_func -> void {
-  //   var a: f32 = 2.0;
-  //   return 0;
-  // }
-
-  Global("a", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
-
-  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
-  Func("my_func", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                              var),
-       },
-       ast::DecorationList{});
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.Validate()) << v.error();
-  EXPECT_EQ(v.error(), "12:34 v-0013: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifier_Fail) {
-  // fn my_func() -> void {
-  //  var a :i32 = 2;
-  //  var a :f21 = 2.0;
-  // }
-  auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
-
-  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(0.1f));
-
-  Func("my_func", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::VariableDeclStatement>(var),
-           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                              var_a_float),
-       },
-       ast::DecorationList{});
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.Validate());
-  EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScope_Pass) {
-  // {
-  // if (true) { var a : f32 = 2.0; }
-  // var a : f32 = 3.14;
-  // }
-  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
-  auto* cond = Expr(true);
-  auto* body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var),
-  });
-
-  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
-
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
-      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                         var_a_float),
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.ValidateStatements(outer_body)) << v.error();
-}
-
-TEST_F(ValidatorTest, DISABLED_RedeclaredIdentifierInnerScope_False) {
-  // TODO(sarahM0): remove DISABLED after implementing ValidateIfStatement
-  // and it should just work
-  // {
-  // var a : f32 = 3.14;
-  // if (true) { var a : f32 = 2.0; }
-  // }
-  auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
-
-  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
-  auto* cond = Expr(true);
-  auto* body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}}, var),
-  });
-
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var_a_float),
-      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.ValidateStatements(outer_body));
-  EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScopeBlock_Pass) {
-  // {
-  //  { var a : f32; }
-  //  var a : f32;
-  // }
-  auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
-  auto* inner = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                         var_inner),
-  });
-
-  auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      inner,
-      create<ast::VariableDeclStatement>(var_outer),
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.ValidateStatements(outer_body));
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScopeBlock_Fail) {
-  // {
-  //  var a : f32;
-  //  { var a : f32; }
-  // }
-  auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
-  auto* inner = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                         var_inner),
-  });
-
-  auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var_outer),
-      inner,
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.ValidateStatements(outer_body));
-  EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierDifferentFunctions_Pass) {
-  // func0 { var a : f32 = 2.0; return; }
-  // func1 { var a : f32 = 3.0; return; }
-  auto* var0 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
-  auto* var1 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(1.0f));
-
-  Func("func0", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                              var0),
-           create<ast::ReturnStatement>(),
-       },
-       ast::DecorationList{});
-
-  Func("func1", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::VariableDeclStatement>(Source{Source::Location{13, 34}},
-                                              var1),
-           create<ast::ReturnStatement>(),
-       },
-       ast::DecorationList{
-           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
-       });
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.Validate()) << v.error();
-}
-
 TEST_F(ValidatorTest, VariableDeclNoConstructor_Pass) {
   // {
   // var a :i32;

src/validator/validator_type_test.cc

@@ -1,48 +0,0 @@
-// Copyright 2020 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/struct_block_decoration.h"
-#include "src/validator/validator_test_helper.h"
-
-#include "src/ast/stage_decoration.h"
-namespace tint {
-namespace {
-
-class ValidatorTypeTest : public ValidatorTestHelper, public testing::Test {};
-
-TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {
-  /// [[stage(vertex)]]
-  // fn func -> void { var a : array<i32>; }
-
-  auto* var = Var("a", ty.array<i32>(), ast::StorageClass::kNone);
-
-  Func("func", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
-                                              var),
-       },
-       ast::DecorationList{
-           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
-       });
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_FALSE(v.Validate());
-  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_block_test.cc

@@ -22,7 +22,9 @@ namespace {
 
 using BuilderTest = TestHelper;
 
-TEST_F(BuilderTest, Block) {
+// TODO(amaiorano): Disabled because Resolver now emits "redeclared identifier
+// 'var'".
+TEST_F(BuilderTest, DISABLED_Block) {
   // Note, this test uses shadow variables which aren't allowed in WGSL but
   // serves to prove the block code is pushing new scopes as needed.
   auto* inner = create<ast::BlockStatement>(ast::StatementList{