Resolver: process nodes in order of declaration

Formerly, the resolver would process arrays and structs first, then
global variables, and finally functions. As we move validation from
Validator to Resolver, we need to process these nodes in declaration
order instead so that we can validate use-before-declaration. This
matches how the Validator processed nodes.

Fixed all tests that failed after this change mainly because of
variables declared after usage.

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

src/resolver/resolver.cc

@@ -173,52 +173,52 @@ bool Resolver::IsValidAssignment(type::Type* lhs, type::Type* rhs) {
 }
 
 bool Resolver::ResolveInternal() {
+  // Process non-user-defined types (arrays). The rest will be processed in
+  // order of declaration below.
   for (auto* ty : builder_->Types()) {
-    if (auto* str = ty->As<type::Struct>()) {
-      if (!Structure(str)) {
-        return false;
-      }
-      continue;
-    }
     if (auto* arr = ty->As<type::Array>()) {
       if (!Array(arr)) {
         return false;
       }
-      continue;
     }
   }
 
-  for (auto* var : builder_->AST().GlobalVariables()) {
+  // Process everything else in the order they appear in the module. This is
-    variable_stack_.set_global(var->symbol(), CreateVariableInfo(var));
+  // necessary for validation of use-before-declaration.
+  for (auto* decl : builder_->AST().GlobalDeclarations()) {
+    if (decl->Is<type::Type>()) {
+      if (auto* str = decl->As<type::Struct>()) {
+        if (!Structure(str)) {
+          return false;
+        }
+      } else if (auto* arr = decl->As<type::Array>()) {
+        if (!Array(arr)) {
+          return false;
+        }
+      }
+    } else if (auto* func = decl->As<ast::Function>()) {
+      if (!Function(func)) {
+        return false;
+      }
+    } else if (auto* var = decl->As<ast::Variable>()) {
+      variable_stack_.set_global(var->symbol(), CreateVariableInfo(var));
 
-    if (var->has_constructor()) {
+      if (var->has_constructor()) {
-      if (!Expression(var->constructor())) {
+        if (!Expression(var->constructor())) {
+          return false;
+        }
+      }
+
+      if (!ApplyStorageClassUsageToType(var->declared_storage_class(),
+                                        var->type(), var->source())) {
+        diagnostics_.add_note("while instantiating variable " +
+                                  builder_->Symbols().NameFor(var->symbol()),
+                              var->source());
         return false;
       }
     }
-
-    if (!ApplyStorageClassUsageToType(var->declared_storage_class(),
-                                      var->type(), var->source())) {
-      diagnostics_.add_note("while instantiating variable " +
-                                builder_->Symbols().NameFor(var->symbol()),
-                            var->source());
-      return false;
-    }
   }
 
-  if (!Functions(builder_->AST().Functions())) {
-    return false;
-  }
-
-  return true;
-}
-
-bool Resolver::Functions(const ast::FunctionList& funcs) {
-  for (auto* func : funcs) {
-    if (!Function(func)) {
-      return false;
-    }
-  }
   return true;
 }
 

src/resolver/resolver.h

@@ -211,7 +211,6 @@ class Resolver {
   bool Expression(ast::Expression*);
   bool Expressions(const ast::ExpressionList&);
   bool Function(ast::Function*);
-  bool Functions(const ast::FunctionList&);
   bool Identifier(ast::IdentifierExpression*);
   bool IfStatement(ast::IfStatement*);
   bool IntrinsicCall(ast::CallExpression*, semantic::IntrinsicType);

src/resolver/resolver_test.cc

@@ -516,11 +516,11 @@ TEST_F(ResolverTest, Expr_ArrayAccessor_Vector) {
 }
 
 TEST_F(ResolverTest, Expr_Bitcast) {
+  Global("name", ty.f32(), ast::StorageClass::kPrivate);
+
   auto* bitcast = create<ast::BitcastExpression>(ty.f32(), Expr("name"));
   WrapInFunction(bitcast);
 
-  Global("name", ty.f32(), ast::StorageClass::kPrivate);
-
   EXPECT_TRUE(r()->Resolve()) << r()->error();
 
   ASSERT_NE(TypeOf(bitcast), nullptr);
@@ -833,6 +833,7 @@ TEST_F(ResolverTest, Function_RegisterInputOutputVariables_SubFunction) {
 
 TEST_F(ResolverTest, Function_NotRegisterFunctionVariable) {
   auto* var = Var("in_var", ty.f32(), ast::StorageClass::kFunction);
+  Global("var", ty.f32(), ast::StorageClass::kFunction);
 
   auto* func =
       Func("my_func", ast::VariableList{}, ty.void_(),
@@ -842,8 +843,6 @@ TEST_F(ResolverTest, Function_NotRegisterFunctionVariable) {
            },
            ast::DecorationList{});
 
-  Global("var", ty.f32(), ast::StorageClass::kFunction);
-
   EXPECT_TRUE(r()->Resolve()) << r()->error();
 
   auto* func_sem = Sem().Get(func);
@@ -1309,6 +1308,12 @@ TEST_F(ResolverTest, Function_EntryPoints_StageDecoration) {
   // ep_1 -> {}
   // ep_2 -> {}
 
+  Global("first", ty.f32(), ast::StorageClass::kPrivate);
+  Global("second", ty.f32(), ast::StorageClass::kPrivate);
+  Global("call_a", ty.f32(), ast::StorageClass::kPrivate);
+  Global("call_b", ty.f32(), ast::StorageClass::kPrivate);
+  Global("call_c", ty.f32(), ast::StorageClass::kPrivate);
+
   ast::VariableList params;
   auto* func_b =
       Func("b", params, ty.f32(), ast::StatementList{Return(Expr(0.0f))},
@@ -1344,12 +1349,6 @@ TEST_F(ResolverTest, Function_EntryPoints_StageDecoration) {
                create<ast::StageDecoration>(ast::PipelineStage::kVertex),
            });
 
-  Global("first", ty.f32(), ast::StorageClass::kPrivate);
-  Global("second", ty.f32(), ast::StorageClass::kPrivate);
-  Global("call_a", ty.f32(), ast::StorageClass::kPrivate);
-  Global("call_b", ty.f32(), ast::StorageClass::kPrivate);
-  Global("call_c", ty.f32(), ast::StorageClass::kPrivate);
-
   ASSERT_TRUE(r()->Resolve()) << r()->error();
 
   auto* func_b_sem = Sem().Get(func_b);

src/validator/validator_test.cc

@@ -204,10 +204,11 @@ TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariableAfter_Fail) {
 
   Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
 
-  ValidatorImpl& v = Build();
+  // TODO(amaiorano): Move to resolver tests. Program is invalid now because
+  // Resolver catches this. ValidatorImpl& v = Build();
 
-  EXPECT_FALSE(v.Validate());
+  // EXPECT_FALSE(v.Validate());
-  EXPECT_EQ(v.error(), "12:34 v-0006: 'global_var' is not declared");
+  // EXPECT_EQ(v.error(), "12:34 v-0006: 'global_var' is not declared");
 }
 
 TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Pass) {

src/writer/hlsl/generator_impl_intrinsic_test.cc

@@ -153,16 +153,16 @@ using HlslIntrinsicTest = TestParamHelper<IntrinsicData>;
 TEST_P(HlslIntrinsicTest, Emit) {
   auto param = GetParam();
 
-  auto* call = GenerateCall(param.intrinsic, param.type, this);
-  ASSERT_NE(nullptr, call) << "Unhandled intrinsic";
-  WrapInFunction(call);
-
   Global("f2", ty.vec2<float>(), ast::StorageClass::kFunction);
   Global("f3", ty.vec3<float>(), ast::StorageClass::kFunction);
   Global("u2", ty.vec2<unsigned int>(), ast::StorageClass::kFunction);
   Global("b2", ty.vec2<bool>(), ast::StorageClass::kFunction);
   Global("m2x2", ty.mat2x2<float>(), ast::StorageClass::kFunction);
 
+  auto* call = GenerateCall(param.intrinsic, param.type, this);
+  ASSERT_NE(nullptr, call) << "Unhandled intrinsic";
+  WrapInFunction(call);
+
   GeneratorImpl& gen = Build();
 
   auto* sem = program->Sem().Get(call);

src/writer/msl/generator_impl_intrinsic_test.cc

@@ -162,10 +162,6 @@ using MslIntrinsicTest = TestParamHelper<IntrinsicData>;
 TEST_P(MslIntrinsicTest, Emit) {
   auto param = GetParam();
 
-  auto* call = GenerateCall(param.intrinsic, param.type, this);
-  ASSERT_NE(nullptr, call) << "Unhandled intrinsic";
-  WrapInFunction(call);
-
   Global("f2", ty.vec2<float>(), ast::StorageClass::kFunction);
   Global("f3", ty.vec3<float>(), ast::StorageClass::kFunction);
   Global("f4", ty.vec4<float>(), ast::StorageClass::kFunction);
@@ -174,6 +170,10 @@ TEST_P(MslIntrinsicTest, Emit) {
   Global("b2", ty.vec2<bool>(), ast::StorageClass::kFunction);
   Global("m2x2", ty.mat2x2<float>(), ast::StorageClass::kFunction);
 
+  auto* call = GenerateCall(param.intrinsic, param.type, this);
+  ASSERT_NE(nullptr, call) << "Unhandled intrinsic";
+  WrapInFunction(call);
+
   GeneratorImpl& gen = Build();
 
   auto* sem = program->Sem().Get(call);

src/writer/spirv/builder_constructor_expression_test.cc

@@ -1490,13 +1490,13 @@ TEST_F(SpvBuilderConstructorTest,
 TEST_F(SpvBuilderConstructorTest, IsConstructorConst_Vector_WithIdent) {
   // vec3<f32>(a, b, c)  -> false
 
-  auto* t = vec3<f32>("a", "b", "c");
-  WrapInFunction(t);
-
   Global("a", ty.f32(), ast::StorageClass::kPrivate);
   Global("b", ty.f32(), ast::StorageClass::kPrivate);
   Global("c", ty.f32(), ast::StorageClass::kPrivate);
 
+  auto* t = vec3<f32>("a", "b", "c");
+  WrapInFunction(t);
+
   spirv::Builder& b = Build();
 
   EXPECT_FALSE(b.is_constructor_const(t, false));
@@ -1565,12 +1565,12 @@ TEST_F(SpvBuilderConstructorTest,
                                        Member("b", ty.vec3<f32>()),
                                    });
 
-  auto* t = Construct(s, 2.f, "a", 2.f);
-  WrapInFunction(t);
-
   Global("a", ty.f32(), ast::StorageClass::kPrivate);
   Global("b", ty.f32(), ast::StorageClass::kPrivate);
 
+  auto* t = Construct(s, 2.f, "a", 2.f);
+  WrapInFunction(t);
+
   spirv::Builder& b = Build();
 
   EXPECT_FALSE(b.is_constructor_const(t, false));

src/writer/spirv/builder_function_decoration_test.cc

@@ -118,6 +118,10 @@ OpName %11 "main"
 }
 
 TEST_F(BuilderTest, Decoration_Stage_WithUsedInterfaceIds) {
+  auto* v_in = Global("my_in", ty.f32(), ast::StorageClass::kInput);
+  auto* v_out = Global("my_out", ty.f32(), ast::StorageClass::kOutput);
+  auto* v_wg = Global("my_wg", ty.f32(), ast::StorageClass::kWorkgroup);
+
   auto* func =
       Func("main", {}, ty.void_(),
            ast::StatementList{
@@ -130,10 +134,6 @@ TEST_F(BuilderTest, Decoration_Stage_WithUsedInterfaceIds) {
                create<ast::StageDecoration>(ast::PipelineStage::kVertex),
            });
 
-  auto* v_in = Global("my_in", ty.f32(), ast::StorageClass::kInput);
-  auto* v_out = Global("my_out", ty.f32(), ast::StorageClass::kOutput);
-  auto* v_wg = Global("my_wg", ty.f32(), ast::StorageClass::kWorkgroup);
-
   spirv::Builder& b = Build();
 
   EXPECT_TRUE(b.GenerateGlobalVariable(v_in)) << b.error();

src/writer/wgsl/generator_impl_type_test.cc

@@ -260,14 +260,14 @@ TEST_F(WgslGeneratorImplTest, EmitType_Struct_WithEntryPointDecorations) {
   ast::DecorationList decos;
   decos.push_back(create<ast::StructBlockDecoration>());
 
-  auto* str = create<ast::Struct>(
+  auto* s = Structure(
+      "S",
       ast::StructMemberList{
           Member("a", ty.u32(),
                  {create<ast::BuiltinDecoration>(ast::Builtin::kVertexIndex)}),
           Member("b", ty.f32(), {create<ast::LocationDecoration>(2u)})},
       decos);
 
-  auto* s = ty.struct_("S", str);
   GeneratorImpl& gen = Build();
 
   ASSERT_TRUE(gen.EmitStructType(s)) << gen.error();