[validation] Validate globals in declared order

Instead of validating all global variables and then functions,
validate global declarations in the order they were added to the AST.

This fixes false-positive "redeclared identifier" errors when a global
variable is declared after a function that declares a variable of the
same name, and false-negative "identifier not declared" errors when a
global variable is declared after a function that tries to use it.

Change-Id: Ibf5e5265bc2f8ca892096f0420757b70e1984525
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41302
Commit-Queue: James Price <jrprice@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

src/validator/validator_function_test.cc

@@ -116,9 +116,8 @@ TEST_F(ValidateFunctionTest, FunctionTypeMustMatchReturnStatementType_Pass) {
        });
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_TRUE(v.ValidateFunctions(program->AST().Functions())) << v.error();
+  EXPECT_TRUE(v.Validate()) << v.error();
 }
 
 TEST_F(ValidateFunctionTest, FunctionTypeMustMatchReturnStatementType_fail) {

src/validator/validator_impl.cc

@@ -286,101 +286,85 @@ void ValidatorImpl::add_error(const Source& src, const std::string& msg) {
 }
 
 bool ValidatorImpl::Validate() {
-  function_stack_.push_scope();
+  // Validate global declarations in the order they appear in the module.
-  if (!ValidateGlobalVariables(program_->AST().GlobalVariables())) {
+  for (auto* decl : program_->AST().GlobalDeclarations()) {
-    return false;
+    if (auto* ty = decl->As<type::Type>()) {
-  }
+      if (!ValidateConstructedType(ty)) {
-  if (!ValidateConstructedTypes(program_->AST().ConstructedTypes())) {
+        return false;
-    return false;
+      }
-  }
+    } else if (auto* func = decl->As<ast::Function>()) {
-  if (!ValidateFunctions(program_->AST().Functions())) {
+      current_function_ = func;
-    return false;
+      if (!ValidateFunction(func)) {
+        return false;
+      }
+      current_function_ = nullptr;
+    } else if (auto* var = decl->As<ast::Variable>()) {
+      if (!ValidateGlobalVariable(var)) {
+        return false;
+      }
+    } else {
+      assert(false /* unreachable */);
+    }
   }
   if (!ValidateEntryPoint(program_->AST().Functions())) {
     return false;
   }
-  function_stack_.pop_scope();
 
   return true;
 }
 
-bool ValidatorImpl::ValidateConstructedTypes(
+bool ValidatorImpl::ValidateConstructedType(const type::Type* type) {
-    const std::vector<type::Type*>& constructed_types) {
+  if (auto* st = type->As<type::Struct>()) {
-  for (auto* const ct : constructed_types) {
+    for (auto* member : st->impl()->members()) {
-    if (auto* st = ct->As<type::Struct>()) {
+      if (auto* r = member->type()->UnwrapAll()->As<type::Array>()) {
-      for (auto* member : st->impl()->members()) {
+        if (r->IsRuntimeArray()) {
-        if (auto* r = member->type()->UnwrapAll()->As<type::Array>()) {
+          if (member != st->impl()->members().back()) {
-          if (r->IsRuntimeArray()) {
+            add_error(member->source(), "v-0015",
-            if (member != st->impl()->members().back()) {
+                      "runtime arrays may only appear as the last member of "
-              add_error(member->source(), "v-0015",
+                      "a struct");
-                        "runtime arrays may only appear as the last member of "
+            return false;
-                        "a struct");
+          }
-              return false;
+          if (!st->IsBlockDecorated()) {
-            }
+            add_error(member->source(), "v-0015",
-            if (!st->IsBlockDecorated()) {
+                      "a struct containing a runtime-sized array "
-              add_error(member->source(), "v-0015",
+                      "requires the [[block]] attribute: '" +
-                        "a struct containing a runtime-sized array "
+                          program_->Symbols().NameFor(st->symbol()) + "'");
-                        "requires the [[block]] attribute: '" +
+            return false;
-                            program_->Symbols().NameFor(st->symbol()) + "'");
-              return false;
-            }
           }
         }
       }
     }
   }
+
   return true;
 }
 
-bool ValidatorImpl::ValidateGlobalVariables(
+bool ValidatorImpl::ValidateGlobalVariable(const ast::Variable* var) {
-    const ast::VariableList& global_vars) {
+  auto* sem = program_->Sem().Get(var);
-  for (auto* var : global_vars) {
+  if (!sem) {
-    auto* sem = program_->Sem().Get(var);
+    add_error(var->source(), "no semantic information for variable '" +
-    if (!sem) {
+                                 program_->Symbols().NameFor(var->symbol()) +
-      add_error(var->source(), "no semantic information for variable '" +
+                                 "'");
-                                   program_->Symbols().NameFor(var->symbol()) +
+    return false;
-                                   "'");
-      return false;
-    }
-
-    if (variable_stack_.has(var->symbol())) {
-      add_error(var->source(), "v-0011",
-                "redeclared global identifier '" +
-                    program_->Symbols().NameFor(var->symbol()) + "'");
-      return false;
-    }
-    if (!var->is_const() && sem->StorageClass() == ast::StorageClass::kNone) {
-      add_error(var->source(), "v-0022",
-                "global variables must have a storage class");
-      return false;
-    }
-    if (var->is_const() && !(sem->StorageClass() == ast::StorageClass::kNone)) {
-      add_error(var->source(), "v-global01",
-                "global constants shouldn't have a storage class");
-      return false;
-    }
-    variable_stack_.set_global(var->symbol(), var);
-  }
-  return true;
-}
-
-bool ValidatorImpl::ValidateFunctions(const ast::FunctionList& funcs) {
-  for (auto* func : funcs) {
-    if (function_stack_.has(func->symbol())) {
-      add_error(func->source(), "v-0016",
-                "function names must be unique '" +
-                    program_->Symbols().NameFor(func->symbol()) + "'");
-      return false;
-    }
-
-    function_stack_.set(func->symbol(), func);
-    current_function_ = func;
-    if (!ValidateFunction(func)) {
-      return false;
-    }
-    current_function_ = nullptr;
   }
 
+  if (variable_stack_.has(var->symbol())) {
+    add_error(var->source(), "v-0011",
+              "redeclared global identifier '" +
+                  program_->Symbols().NameFor(var->symbol()) + "'");
+    return false;
+  }
+  if (!var->is_const() && sem->StorageClass() == ast::StorageClass::kNone) {
+    add_error(var->source(), "v-0022",
+              "global variables must have a storage class");
+    return false;
+  }
+  if (var->is_const() && !(sem->StorageClass() == ast::StorageClass::kNone)) {
+    add_error(var->source(), "v-global01",
+              "global constants shouldn't have a storage class");
+    return false;
+  }
+  variable_stack_.set_global(var->symbol(), var);
   return true;
 }
 
@@ -425,6 +409,15 @@ bool ValidatorImpl::ValidateEntryPoint(const ast::FunctionList& funcs) {
 }
 
 bool ValidatorImpl::ValidateFunction(const ast::Function* func) {
+  if (function_stack_.has(func->symbol())) {
+    add_error(func->source(), "v-0016",
+              "function names must be unique '" +
+                  program_->Symbols().NameFor(func->symbol()) + "'");
+    return false;
+  }
+
+  function_stack_.set(func->symbol(), func);
+
   variable_stack_.push_scope();
 
   for (auto* param : func->params()) {
@@ -906,7 +899,7 @@ bool ValidatorImpl::ValidateBadAssignmentToIdentifier(
     // It wasn't an identifier in the first place.
     return true;
   }
-  ast::Variable* var;
+  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.
@@ -989,7 +982,7 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) {
 }
 
 bool ValidatorImpl::ValidateIdentifier(const ast::IdentifierExpression* ident) {
-  ast::Variable* var;
+  const ast::Variable* var;
   if (!variable_stack_.get(ident->symbol(), &var)) {
     add_error(ident->source(), "v-0006",
               "'" + program_->Symbols().NameFor(ident->symbol()) +

src/validator/validator_impl.h

@@ -76,14 +76,10 @@ class ValidatorImpl {
   /// @param msg the error message
   void add_error(const Source& src, const std::string& msg);
 
-  /// Validate global variables
+  /// Validates a global variable
-  /// @param global_vars list of global variables to check
+  /// @param var the global variable to check
   /// @returns true if the validation was successful
-  bool ValidateGlobalVariables(const ast::VariableList& global_vars);
+  bool ValidateGlobalVariable(const ast::Variable* var);
-  /// Validates Functions
-  /// @param funcs the functions to check
-  /// @returns true if the validation was successful
-  bool ValidateFunctions(const ast::FunctionList& funcs);
   /// Validates a function
   /// @param func the function to check
   /// @returns true if the validation was successful
@@ -144,11 +140,10 @@ class ValidatorImpl {
   /// @returns true if the valdiation was successful
   bool ValidateEntryPoint(const ast::FunctionList& funcs);
 
-  /// Validates constructed types
+  /// Validates a constructed type
-  /// @param constructed_types the types to check
+  /// @param type the type to check
   /// @returns true if the valdiation was successful
-  bool ValidateConstructedTypes(
+  bool ValidateConstructedType(const type::Type* type);
-      const std::vector<type::Type*>& constructed_types);
 
   /// Returns true if the given type is storable. This uses and
   /// updates `storable_` and `not_storable_`.
@@ -165,8 +160,8 @@ class ValidatorImpl {
  private:
   const Program* program_;
   diag::List diags_;
-  ScopeStack<ast::Variable*> variable_stack_;
+  ScopeStack<const ast::Variable*> variable_stack_;
-  ScopeStack<ast::Function*> function_stack_;
+  ScopeStack<const ast::Function*> function_stack_;
   ast::Function* current_function_ = nullptr;
 };
 

src/validator/validator_test.cc

@@ -375,15 +375,13 @@ TEST_F(ValidatorTest, AssignIncompatibleTypesInNestedBlockStatement_Fail) {
 
 TEST_F(ValidatorTest, GlobalVariableWithStorageClass_Pass) {
   // var<in> gloabl_var: f32;
-  Global(Source{Source::Location{12, 34}}, "global_var",
+  auto* var = Global(Source{Source::Location{12, 34}}, "global_var",
-         ast::StorageClass::kInput, ty.f32(), nullptr,
+                     ast::StorageClass::kInput, ty.f32(), nullptr,
-         ast::VariableDecorationList{});
+                     ast::VariableDecorationList{});
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_TRUE(v.ValidateGlobalVariables(program->AST().GlobalVariables()))
+  EXPECT_TRUE(v.ValidateGlobalVariable(var)) << v.error();
-      << v.error();
 }
 
 TEST_F(ValidatorTest, GlobalVariableNoStorageClass_Fail) {
@@ -449,6 +447,32 @@ TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Fail) {
   EXPECT_EQ(v.error(), "12:34 v-0006: 'not_global_var' is not declared");
 }
 
+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::FunctionDecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex)});
+
+  Global("global_var", ast::StorageClass::kPrivate, ty.f32(), Expr(2.1f),
+         ast::VariableDecorationList{});
+
+  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 {
@@ -579,18 +603,17 @@ TEST_F(ValidatorTest, UsingUndefinedVariableDifferentScope_Fail) {
 TEST_F(ValidatorTest, GlobalVariableUnique_Pass) {
   // var global_var0 : f32 = 0.1;
   // var global_var1 : i32 = 0;
-  Global("global_var0", ast::StorageClass::kPrivate, ty.f32(), Expr(0.1f),
+  auto* var0 = Global("global_var0", ast::StorageClass::kPrivate, ty.f32(),
-         ast::VariableDecorationList{});
+                      Expr(0.1f), ast::VariableDecorationList{});
 
-  Global(Source{Source::Location{12, 34}}, "global_var1",
+  auto* var1 = Global(Source{Source::Location{12, 34}}, "global_var1",
-         ast::StorageClass::kPrivate, ty.f32(), Expr(0),
+                      ast::StorageClass::kPrivate, ty.f32(), Expr(0),
-         ast::VariableDecorationList{});
+                      ast::VariableDecorationList{});
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_TRUE(v.ValidateGlobalVariables(program->AST().GlobalVariables()))
+  EXPECT_TRUE(v.ValidateGlobalVariable(var0)) << v.error();
-      << v.error();
+  EXPECT_TRUE(v.ValidateGlobalVariable(var1)) << v.error();
 }
 
 TEST_F(ValidatorTest, GlobalVariableNotUnique_Fail) {
@@ -604,9 +627,8 @@ TEST_F(ValidatorTest, GlobalVariableNotUnique_Fail) {
          ast::VariableDecorationList{});
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_FALSE(v.ValidateGlobalVariables(program->AST().GlobalVariables()));
+  EXPECT_FALSE(v.Validate());
   EXPECT_EQ(v.error(),
             "12:34 v-0011: redeclared global identifier 'global_var'");
 }
@@ -639,6 +661,30 @@ TEST_F(ValidatorTest, AssignToConstant_Fail) {
   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;
+  // }
+  // var a: f32 = 2.1;
+
+  auto* var = Var("a", ast::StorageClass::kNone, ty.f32(), Expr(2.0f),
+                  ast::VariableDecorationList{});
+
+  Func("my_func", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(var),
+       },
+       ast::FunctionDecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex)});
+
+  Global("a", ast::StorageClass::kPrivate, ty.f32(), Expr(2.1f),
+         ast::VariableDecorationList{});
+
+  ValidatorImpl& v = Build();
+
+  EXPECT_TRUE(v.Validate()) << v.error();
+}
+
 TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Fail) {
   // var a: f32 = 2.1;
   // fn my_func -> void {
@@ -665,7 +711,7 @@ TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Fail) {
   EXPECT_EQ(v.error(), "12:34 v-0013: redeclared identifier 'a'");
 }
 
-TEST_F(ValidatorTest, RedeclaredIndentifier_Fail) {
+TEST_F(ValidatorTest, RedeclaredIdentifier_Fail) {
   // fn my_func() -> void {
   //  var a :i32 = 2;
   //  var a :f21 = 2.0;

src/validator/validator_type_test.cc

@@ -54,9 +54,8 @@ TEST_F(ValidatorTypeTest, RuntimeArrayIsLast_Pass) {
   AST().AddConstructedType(struct_type);
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_TRUE(v.ValidateConstructedTypes(program->AST().ConstructedTypes()));
+  EXPECT_TRUE(v.ValidateConstructedType(struct_type));
 }
 
 TEST_F(ValidatorTypeTest, RuntimeArrayIsLastNoBlock_Fail) {
@@ -75,9 +74,8 @@ TEST_F(ValidatorTypeTest, RuntimeArrayIsLastNoBlock_Fail) {
   AST().AddConstructedType(struct_type);
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_FALSE(v.ValidateConstructedTypes(program->AST().ConstructedTypes()));
+  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
   EXPECT_EQ(v.error(),
             "v-0015: a struct containing a runtime-sized array requires the "
             "[[block]] attribute: 'Foo'");
@@ -104,9 +102,8 @@ TEST_F(ValidatorTypeTest, RuntimeArrayIsNotLast_Fail) {
   AST().AddConstructedType(struct_type);
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_FALSE(v.ValidateConstructedTypes(program->AST().ConstructedTypes()));
+  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
   EXPECT_EQ(v.error(),
             "12:34 v-0015: runtime arrays may only appear as the last member "
             "of a struct");
@@ -131,9 +128,8 @@ TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsNotLast_Fail) {
   AST().AddConstructedType(struct_type);
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_FALSE(v.ValidateConstructedTypes(program->AST().ConstructedTypes()));
+  EXPECT_FALSE(v.ValidateConstructedType(struct_type));
   EXPECT_EQ(v.error(),
             "v-0015: runtime arrays may only appear as the last member "
             "of a struct");
@@ -158,9 +154,8 @@ TEST_F(ValidatorTypeTest, AliasRuntimeArrayIsLast_Pass) {
   AST().AddConstructedType(struct_type);
 
   ValidatorImpl& v = Build();
-  const Program* program = v.program();
 
-  EXPECT_TRUE(v.ValidateConstructedTypes(program->AST().ConstructedTypes()));
+  EXPECT_TRUE(v.ValidateConstructedType(struct_type));
 }
 
 TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {