tint/resolver: Shuffle validation code

Restructure the logic so there's less, pointless dynamic casting, and the complexity is reduced.

This alters the order in which variables are validated, hence the change of test.

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

src/tint/program_builder.h

@@ -779,6 +779,17 @@ class ProgramBuilder {
             return pointer(Of<T>(), storage_class, access);
         }
 
+        /// @param source the Source of the node
+        /// @param storage_class the storage class of the pointer
+        /// @param access the optional access control of the pointer
+        /// @return the pointer to type `T` with the given ast::StorageClass.
+        template <typename T>
+        const ast::Pointer* pointer(const Source& source,
+                                    ast::StorageClass storage_class,
+                                    ast::Access access = ast::Access::kUndefined) const {
+            return pointer(source, Of<T>(), storage_class, access);
+        }
+
         /// @param source the Source of the node
         /// @param type the type of the atomic
         /// @return the atomic to `type`

src/tint/resolver/resolver.cc

@@ -2575,7 +2575,7 @@ sem::Statement* Resolver::VariableDeclStatement(const ast::VariableDeclStatement
             sem->Behaviors() = ctor->Behaviors();
         }
 
-        return validator_.Variable(variable);
+        return validator_.LocalVariable(variable);
     });
 }
 

src/tint/resolver/validator.cc

@@ -533,6 +533,32 @@ bool Validator::StorageClassLayout(const sem::Variable* var,
     return true;
 }
 
+bool Validator::LocalVariable(const sem::Variable* v) const {
+    auto* decl = v->Declaration();
+    return Switch(
+        decl,  //
+        [&](const ast::Var* var) {
+            if (IsValidationEnabled(var->attributes,
+                                    ast::DisabledValidation::kIgnoreStorageClass)) {
+                if (!v->Type()->UnwrapRef()->IsConstructible()) {
+                    AddError("function-scope 'var' must have a constructible type",
+                             var->type ? var->type->source : var->source);
+                    return false;
+                }
+            }
+            return Var(v);
+        },                                                  //
+        [&](const ast::Let*) { return Let(v); },            //
+        [&](const ast::Override*) { return Override(v); },  //
+        [&](const ast::Const*) { return true; },            //
+        [&](Default) {
+            TINT_ICE(Resolver, diagnostics_)
+                << "Validator::Variable() called with a unknown variable type: "
+                << decl->TypeInfo().name;
+            return false;
+        });
+}
+
 bool Validator::GlobalVariable(
     const sem::GlobalVariable* global,
     std::unordered_map<uint32_t, const sem::Variable*> constant_ids,
@@ -583,6 +609,14 @@ bool Validator::GlobalVariable(
                 return false;
             }
 
+            auto name = symbols_.NameFor(var->symbol);
+            if (sem::ParseBuiltinType(name) != sem::BuiltinType::kNone) {
+                AddError(
+                    "'" + name + "' is a builtin and cannot be redeclared as a module-scope 'var'",
+                    var->source);
+                return false;
+            }
+
             return Var(global);
         },
         [&](const ast::Override*) {
@@ -702,49 +736,15 @@ bool Validator::AtomicVariable(
     return true;
 }
 
-bool Validator::Variable(const sem::Variable* v) const {
-    auto* decl = v->Declaration();
-    return Switch(
-        decl,                                               //
-        [&](const ast::Var*) { return Var(v); },            //
-        [&](const ast::Let*) { return Let(v); },            //
-        [&](const ast::Override*) { return Override(v); },  //
-        [&](const ast::Const*) { return true; },            //
-        [&](Default) {
-            TINT_ICE(Resolver, diagnostics_)
-                << "Validator::Variable() called with a unknown variable type: "
-                << decl->TypeInfo().name;
-            return false;
-        });
-}
-
 bool Validator::Var(const sem::Variable* v) const {
     auto* var = v->Declaration()->As<ast::Var>();
     auto* storage_ty = v->Type()->UnwrapRef();
 
-    if (v->Is<sem::GlobalVariable>()) {
-        auto name = symbols_.NameFor(var->symbol);
-        if (sem::ParseBuiltinType(name) != sem::BuiltinType::kNone) {
-            AddError("'" + name + "' is a builtin and cannot be redeclared as a module-scope 'var'",
-                     var->source);
-            return false;
-        }
-    }
-
     if (!IsStorable(storage_ty)) {
         AddError(sem_.TypeNameOf(storage_ty) + " cannot be used as the type of a var", var->source);
         return false;
     }
 
-    if (v->Is<sem::LocalVariable>() &&
-        IsValidationEnabled(var->attributes, ast::DisabledValidation::kIgnoreStorageClass)) {
-        if (!v->Type()->UnwrapRef()->IsConstructible()) {
-            AddError("function-scope 'var' must have a constructible type",
-                     var->type ? var->type->source : var->source);
-            return false;
-        }
-    }
-
     if (storage_ty->is_handle() && var->declared_storage_class != ast::StorageClass::kNone) {
         // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
         // If the store type is a texture type or a sampler type, then the

src/tint/resolver/validator.h

@@ -264,6 +264,11 @@ class Validator {
     /// @returns true on success, false otherwise.
     bool BuiltinCall(const sem::Call* call) const;
 
+    /// Validates a local variable
+    /// @param v the variable to validate
+    /// @returns true on success, false otherwise.
+    bool LocalVariable(const sem::Variable* v) const;
+
     /// Validates a location attribute
     /// @param location the location attribute to validate
     /// @param type the variable type
@@ -354,11 +359,6 @@ class Validator {
     /// @returns true on success, false otherwise
     bool SwitchStatement(const ast::SwitchStatement* s);
 
-    /// Validates a variable
-    /// @param v the variable to validate
-    /// @returns true on success, false otherwise.
-    bool Variable(const sem::Variable* v) const;
-
     /// Validates a 'var' variable declaration
     /// @param v the variable to validate
     /// @returns true on success, false otherwise.

src/tint/resolver/variable_validation_test.cc

@@ -59,18 +59,16 @@ TEST_F(ResolverVariableValidationTest, OverrideNoInitializerNoType) {
     EXPECT_EQ(r()->error(), "12:34 error: override declaration requires a type or initializer");
 }
 
-TEST_F(ResolverVariableValidationTest, VarTypeNotStorable) {
+TEST_F(ResolverVariableValidationTest, VarTypeNotConstructible) {
     // var i : i32;
     // var p : pointer<function, i32> = &v;
     auto* i = Var("i", ty.i32(), ast::StorageClass::kNone);
-    auto* p = Var(Source{{56, 78}}, "a", ty.pointer<i32>(ast::StorageClass::kFunction),
+    auto* p = Var("a", ty.pointer<i32>(Source{{56, 78}}, ast::StorageClass::kFunction),
                   ast::StorageClass::kNone, AddressOf(Source{{12, 34}}, "i"));
     WrapInFunction(i, p);
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(),
-              "56:78 error: ptr<function, i32, read_write> cannot be used as the "
-              "type of a var");
+    EXPECT_EQ(r()->error(), "56:78 error: function-scope 'var' must have a constructible type");
 }
 
 TEST_F(ResolverVariableValidationTest, LetTypeNotConstructible) {