Move identifier validation from Validator to Resolver

This was mostly already implemented in the Resolver, except for adding a
variable scope for blocks.

Moved tests and improved them to only add Source on the error node.

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

src/resolver/resolver.cc

@@ -1726,7 +1726,10 @@ template <typename F>
 bool Resolver::BlockScope(BlockInfo::Type type, F&& callback) {
   BlockInfo block_info(type, current_block_);
   ScopedAssignment<BlockInfo*> sa(current_block_, &block_info);
-  return callback();
+  variable_stack_.push_scope();
+  bool result = callback();
+  variable_stack_.pop_scope();
+  return result;
 }
 
 std::string Resolver::VectorPretty(uint32_t size, type::Type* element_type) {

src/resolver/validation_test.cc

@@ -211,8 +211,7 @@ TEST_F(ResolverValidationTest, Expr_DontCall_Intrinsic) {
 TEST_F(ResolverValidationTest, UsingUndefinedVariable_Fail) {
   // b = 2;
 
-  SetSource(Source{Source::Location{12, 34}});
+  auto* lhs = Expr(Source{{12, 34}}, "b");
-  auto* lhs = Expr("b");
   auto* rhs = Expr(2);
   auto* assign = create<ast::AssignmentStatement>(lhs, rhs);
   WrapInFunction(assign);
@@ -227,8 +226,7 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableInBlockStatement_Fail) {
   //  b = 2;
   // }
 
-  SetSource(Source{Source::Location{12, 34}});
+  auto* lhs = Expr(Source{{12, 34}}, "b");
-  auto* lhs = Expr("b");
   auto* rhs = Expr(2);
 
   auto* body = create<ast::BlockStatement>(ast::StatementList{
@@ -241,6 +239,133 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableInBlockStatement_Fail) {
             "12:34 error: v-0006: identifier must be declared before use: b");
 }
 
+TEST_F(ResolverValidationTest, UsingUndefinedVariableGlobalVariableAfter_Fail) {
+  // fn my_func() -> void {
+  //   global_var = 3.14f;
+  // }
+  // var global_var: f32 = 2.1;
+
+  auto* lhs = Expr(Source{{12, 34}}, "global_var");
+  auto* rhs = Expr(3.14f);
+
+  Func("my_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::AssignmentStatement>(lhs, rhs),
+       },
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex)});
+
+  Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: v-0006: identifier must be declared before use: "
+            "global_var");
+}
+
+TEST_F(ResolverValidationTest, UsingUndefinedVariableGlobalVariable_Pass) {
+  // var global_var: f32 = 2.1;
+  // fn my_func() -> void {
+  //   global_var = 3.14;
+  //   return;
+  // }
+
+  Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
+
+  Func("my_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::AssignmentStatement>(Source{Source::Location{12, 34}},
+                                            Expr("global_var"), Expr(3.14f)),
+           create<ast::ReturnStatement>(),
+       },
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverValidationTest, UsingUndefinedVariableInnerScope_Fail) {
+  // {
+  //   if (true) { var a : f32 = 2.0; }
+  //   a = 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),
+  });
+
+  SetSource(Source{Source::Location{12, 34}});
+  auto* lhs = Expr(Source{{12, 34}}, "a");
+  auto* rhs = Expr(3.14f);
+
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+      create<ast::AssignmentStatement>(lhs, rhs),
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: v-0006: identifier must be declared before use: a");
+}
+
+TEST_F(ResolverValidationTest, UsingUndefinedVariableOuterScope_Pass) {
+  // {
+  //   var a : f32 = 2.0;
+  //   if (true) { a = 3.14; }
+  // }
+  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+  auto* lhs = Expr(Source{{12, 34}}, "a");
+  auto* rhs = Expr(3.14f);
+
+  auto* cond = Expr(true);
+  auto* body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::AssignmentStatement>(lhs, rhs),
+  });
+
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var),
+      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverValidationTest, UsingUndefinedVariableDifferentScope_Fail) {
+  // {
+  //  { var a : f32 = 2.0; }
+  //  { a = 3.14; }
+  // }
+  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+  auto* first_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::VariableDeclStatement>(var),
+  });
+
+  auto* lhs = Expr(Source{{12, 34}}, "a");
+  auto* rhs = Expr(3.14f);
+  auto* second_body = create<ast::BlockStatement>(ast::StatementList{
+      create<ast::AssignmentStatement>(lhs, rhs),
+  });
+
+  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+      first_body,
+      second_body,
+  });
+
+  WrapInFunction(outer_body);
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: v-0006: identifier must be declared before use: a");
+}
+
 TEST_F(ResolverValidationTest, StorageClass_NonFunctionClassError) {
   auto* var = Var("var", ty.i32(), ast::StorageClass::kWorkgroup);
 

src/validator/validator_impl.cc

@@ -411,11 +411,10 @@ bool ValidatorImpl::ValidateBadAssignmentToIdentifier(
       return false;
     }
   } else {
-    // The identifier is not defined. This should already have been caught
+    // The identifier is not defined.
-    // when validating the subexpression.
+    // Shouldn't reach here. This should already have been caught when
-    add_error(ident->source(), "v-0006",
+    // validation expressions in the Resolver
-              "'" + program_->Symbols().NameFor(ident->symbol()) +
+    TINT_UNREACHABLE(diagnostics());
-                  "' is not declared");
     return false;
   }
   return true;
@@ -463,9 +462,6 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) {
   if (!expr) {
     return false;
   }
-  if (auto* i = expr->As<ast::IdentifierExpression>()) {
-    return ValidateIdentifier(i);
-  }
 
   if (auto* c = expr->As<ast::CallExpression>()) {
     return ValidateCallExpr(c);
@@ -473,17 +469,6 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) {
   return true;
 }
 
-bool ValidatorImpl::ValidateIdentifier(const ast::IdentifierExpression* ident) {
-  const ast::Variable* var;
-  if (!variable_stack_.get(ident->symbol(), &var)) {
-    add_error(ident->source(), "v-0006",
-              "'" + program_->Symbols().NameFor(ident->symbol()) +
-                  "' is not declared");
-    return false;
-  }
-  return true;
-}
-
 bool ValidatorImpl::IsStorable(type::Type* type) {
   if (type == nullptr) {
     return false;

src/validator/validator_impl.h

@@ -98,10 +98,6 @@ class ValidatorImpl {
   /// @param expr the expression to check
   /// @return true if the expression is valid
   bool ValidateExpression(const ast::Expression* expr);
-  /// Validates v-0006:Variables must be defined before use
-  /// @param ident the identifer to check if its in the scope
-  /// @return true if idnet was defined
-  bool ValidateIdentifier(const ast::IdentifierExpression* ident);
   /// 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

@@ -185,155 +185,6 @@ TEST_F(ValidatorTest, GlobalConstNoStorageClass_Pass) {
   EXPECT_FALSE(v.Validate()) << v.error();
 }
 
-TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariableAfter_Fail) {
-  // fn my_func() -> void {
-  //   global_var = 3.14f;
-  // }
-  // var global_var: f32 = 2.1;
-
-  SetSource(Source{Source::Location{12, 34}});
-  auto* lhs = Expr("global_var");
-  auto* rhs = Expr(3.14f);
-
-  Func("my_func", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::AssignmentStatement>(lhs, rhs),
-       },
-       ast::DecorationList{
-           create<ast::StageDecoration>(ast::PipelineStage::kVertex)});
-
-  Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
-
-  // TODO(amaiorano): Move to resolver tests. Program is invalid now because
-  // Resolver catches this. ValidatorImpl& v = Build();
-
-  // EXPECT_FALSE(v.Validate());
-  // EXPECT_EQ(v.error(), "12:34 v-0006: 'global_var' is not declared");
-}
-
-TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Pass) {
-  // var global_var: f32 = 2.1;
-  // fn my_func() -> void {
-  //   global_var = 3.14;
-  //   return;
-  // }
-
-  Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
-
-  Func("my_func", ast::VariableList{}, ty.void_(),
-       ast::StatementList{
-           create<ast::AssignmentStatement>(Source{Source::Location{12, 34}},
-                                            Expr("global_var"), Expr(3.14f)),
-           create<ast::ReturnStatement>(),
-       },
-       ast::DecorationList{
-           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
-       });
-
-  ValidatorImpl& v = Build();
-
-  EXPECT_TRUE(v.Validate()) << v.error();
-}
-
-TEST_F(ValidatorTest, UsingUndefinedVariableInnerScope_Fail) {
-  // {
-  //   if (true) { var a : f32 = 2.0; }
-  //   a = 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),
-  });
-
-  SetSource(Source{Source::Location{12, 34}});
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(3.14f);
-
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
-      create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
-                                       rhs),
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_FALSE(v.ValidateStatements(outer_body));
-  EXPECT_EQ(v.error(), "12:34 v-0006: 'a' is not declared");
-}
-
-TEST_F(ValidatorTest, UsingUndefinedVariableOuterScope_Pass) {
-  // {
-  //   var a : f32 = 2.0;
-  //   if (true) { a = 3.14; }
-  // }
-  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
-  SetSource(Source{Source::Location{12, 34}});
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(3.14f);
-
-  auto* cond = Expr(true);
-  auto* body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
-                                       rhs),
-  });
-
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var),
-      create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_TRUE(v.ValidateStatements(outer_body)) << v.error();
-}
-
-TEST_F(ValidatorTest, UsingUndefinedVariableDifferentScope_Fail) {
-  // {
-  //  { var a : f32 = 2.0; }
-  //  { a = 3.14; }
-  // }
-  auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-  auto* first_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::VariableDeclStatement>(var),
-  });
-
-  SetSource(Source{Source::Location{12, 34}});
-  auto* lhs = Expr("a");
-  auto* rhs = Expr(3.14f);
-  auto* second_body = create<ast::BlockStatement>(ast::StatementList{
-      create<ast::AssignmentStatement>(Source{Source::Location{12, 34}}, lhs,
-                                       rhs),
-  });
-
-  auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
-      first_body,
-      second_body,
-  });
-
-  WrapInFunction(outer_body);
-
-  ValidatorImpl& v = Build();
-
-  ASSERT_NE(TypeOf(lhs), nullptr);
-  ASSERT_NE(TypeOf(rhs), nullptr);
-
-  EXPECT_FALSE(v.ValidateStatements(outer_body));
-  EXPECT_EQ(v.error(), "12:34 v-0006: 'a' is not declared");
-}
-
 TEST_F(ValidatorTest, GlobalVariableUnique_Pass) {
   // var global_var0 : f32 = 0.1;
   // var global_var1 : i32 = 0;