tint/resolver: Clean up 'let' / 'override' resolving

Split the logic out of Resolver::Variable().

Primes the resolver for the introduction of 'const'

Bug: tint:1580
Change-Id: Id67280ed5c8c73a69c62728fb5a81a08f13628a8
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/93785
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>

src/tint/resolver/resolver.cc

@@ -312,6 +312,20 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
 }
 
 sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) {
+    return Switch(
+        v,  //
+        [&](const ast::Var* var) { return Var(var, is_global); },
+        [&](const ast::Let* let) { return Let(let, is_global); },
+        [&](const ast::Override* override) { return Override(override); },
+        [&](Default) {
+            TINT_ICE(Resolver, diagnostics_)
+                << "Resolver::GlobalVariable() called with a unknown variable type: "
+                << v->TypeInfo().name;
+            return nullptr;
+        });
+}
+
+sem::Variable* Resolver::Let(const ast::Let* v, bool is_global) {
     const sem::Type* ty = nullptr;
 
     // If the variable has a declared type, resolve it.
@@ -322,8 +336,58 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) {
         }
     }
 
-    auto* as_let = v->As<ast::Let>();
+    if (!v->constructor) {
-    auto* as_override = v->As<ast::Override>();
+        AddError("'let' declaration must have an initializer", v->source);
+        return nullptr;
+    }
+
+    auto* rhs = Materialize(Expression(v->constructor), ty);
+    if (!rhs) {
+        return nullptr;
+    }
+
+    // If the variable has no declared type, infer it from the RHS
+    if (!ty) {
+        ty = rhs->Type()->UnwrapRef();  // Implicit load of RHS
+    }
+
+    if (rhs &&
+        !validator_.VariableConstructorOrCast(v, ast::StorageClass::kNone, ty, rhs->Type())) {
+        return nullptr;
+    }
+
+    if (!ApplyStorageClassUsageToType(ast::StorageClass::kNone, const_cast<sem::Type*>(ty),
+                                      v->source)) {
+        AddNote("while instantiating 'let' " + builder_->Symbols().NameFor(v->symbol), v->source);
+        return nullptr;
+    }
+
+    sem::Variable* sem = nullptr;
+    if (is_global) {
+        sem = builder_->create<sem::GlobalVariable>(
+            v, ty, ast::StorageClass::kNone, ast::Access::kUndefined,
+            rhs ? rhs->ConstantValue() : sem::Constant{}, sem::BindingPoint{});
+    } else {
+        sem = builder_->create<sem::LocalVariable>(v, ty, ast::StorageClass::kNone,
+                                                   ast::Access::kUndefined, current_statement_,
+                                                   rhs ? rhs->ConstantValue() : sem::Constant{});
+    }
+
+    sem->SetConstructor(rhs);
+    builder_->Sem().Add(v, sem);
+    return sem;
+}
+
+sem::Variable* Resolver::Override(const ast::Override* v) {
+    const sem::Type* ty = nullptr;
+
+    // If the variable has a declared type, resolve it.
+    if (v->type) {
+        ty = Type(v->type);
+        if (!ty) {
+            return nullptr;
+        }
+    }
 
     const sem::Expression* rhs = nullptr;
 
@@ -338,9 +402,6 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) {
         if (!ty) {
             ty = rhs->Type()->UnwrapRef();  // Implicit load of RHS
         }
-    } else if (as_let) {
-        AddError("'let' declaration must have an initializer", v->source);
-        return nullptr;
     } else if (!ty) {
         AddError("'override' declaration requires a type or initializer", v->source);
         return nullptr;
@@ -353,28 +414,17 @@ sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) {
 
     if (!ApplyStorageClassUsageToType(ast::StorageClass::kNone, const_cast<sem::Type*>(ty),
                                       v->source)) {
-        AddNote("while instantiating variable " + builder_->Symbols().NameFor(v->symbol),
+        AddNote("while instantiating 'override' " + builder_->Symbols().NameFor(v->symbol),
                 v->source);
         return nullptr;
     }
 
-    sem::Variable* sem = nullptr;
+    auto* sem = builder_->create<sem::GlobalVariable>(v, ty, ast::StorageClass::kNone,
-    if (is_global) {
+                                                      ast::Access::kUndefined, sem::Constant{},
-        bool has_const_val = rhs && !as_override;
+                                                      sem::BindingPoint{});
-        auto* global = builder_->create<sem::GlobalVariable>(
-            v, ty, ast::StorageClass::kNone, ast::Access::kUndefined,
-            has_const_val ? rhs->ConstantValue() : sem::Constant{}, sem::BindingPoint{});
 
-        if (as_override) {
+    if (auto* id = ast::GetAttribute<ast::IdAttribute>(v->attributes)) {
-            if (auto* id = ast::GetAttribute<ast::IdAttribute>(v->attributes)) {
+        sem->SetConstantId(static_cast<uint16_t>(id->value));
-                global->SetConstantId(static_cast<uint16_t>(id->value));
-            }
-        }
-        sem = global;
-    } else {
-        sem = builder_->create<sem::LocalVariable>(
-            v, ty, ast::StorageClass::kNone, ast::Access::kUndefined, current_statement_,
-            (rhs && as_let) ? rhs->ConstantValue() : sem::Constant{});
     }
 
     sem->SetConstructor(rhs);
@@ -567,10 +617,7 @@ void Resolver::SetShadows() {
 }
 
 sem::GlobalVariable* Resolver::GlobalVariable(const ast::Variable* v) {
-    auto* as_var = v->As<ast::Var>();
+    auto* sem = As<sem::GlobalVariable>(Variable(v, /* is_global */ true));
-
-    auto* sem = As<sem::GlobalVariable>(as_var ? Var(as_var, /* is_global */ true)
-                                               : Variable(v, /* is_global */ true));
     if (!sem) {
         return nullptr;
     }
@@ -2477,10 +2524,7 @@ sem::Statement* Resolver::VariableDeclStatement(const ast::VariableDeclStatement
     return StatementScope(stmt, sem, [&] {
         Mark(stmt->variable);
 
-        auto* variable = Switch(
+        auto* variable = Variable(stmt->variable, /* is_global */ false);
-            stmt->variable,  //
-            [&](const ast::Var* var) { return Var(var, /* is_global */ false); },
-            [&](Default) { return Variable(stmt->variable, /* is_global */ false); });
         if (!variable) {
             return false;
         }
@@ -2501,10 +2545,7 @@ sem::Statement* Resolver::VariableDeclStatement(const ast::VariableDeclStatement
             sem->Behaviors() = ctor->Behaviors();
         }
 
-        return Switch(
+        return validator_.Variable(variable);
-            stmt->variable,  //
-            [&](const ast::Var*) { return validator_.Var(variable); },
-            [&](Default) { return validator_.Variable(variable); });
     });
 }
 

src/tint/resolver/resolver.h

@@ -298,6 +298,21 @@ class Resolver {
     /// @param is_global true if this is module scope, otherwise function scope
     sem::Variable* Variable(const ast::Variable* var, bool is_global);
 
+    /// @returns the semantic info for the `ast::Let` `v`. If an error is raised, nullptr is
+    /// returned.
+    /// @note this method does not resolve the attributes as these are context-dependent (global,
+    /// local)
+    /// @param var the variable
+    /// @param is_global true if this is module scope, otherwise function scope
+    sem::Variable* Let(const ast::Let* var, bool is_global);
+
+    /// @returns the semantic info for the module-scope `ast::Override` `v`. If an error is raised,
+    /// nullptr is returned.
+    /// @note this method does not resolve the attributes as these are context-dependent (global,
+    /// local)
+    /// @param override the variable
+    sem::Variable* Override(const ast::Override* override);
+
     /// @returns the semantic info for the `ast::Var` `var`. If an error is raised, nullptr is
     /// returned.
     /// @note this method does not resolve the attributes as these are context-dependent (global,

src/tint/resolver/validator.cc

@@ -558,7 +558,7 @@ bool Validator::GlobalVariable(
                     return false;
                 }
             }
-            return Variable(global);
+            return Override(global);
         },
         [&](const ast::Let*) {
             if (!decl->attributes.empty()) {
@@ -566,7 +566,7 @@ bool Validator::GlobalVariable(
                          decl->attributes[0]->source);
                 return false;
             }
-            return Variable(global);
+            return Let(global);
         },
         [&](const ast::Var* var) {
             if (global->StorageClass() == ast::StorageClass::kNone) {
@@ -684,21 +684,53 @@ bool Validator::AtomicVariable(
 
 bool Validator::Variable(const sem::Variable* v) const {
     auto* decl = v->Declaration();
-    auto* storage_ty = v->Type()->UnwrapRef();
+    return Switch(
+        decl,                                               //
+        [&](const ast::Var*) { return Var(v); },            //
+        [&](const ast::Let*) { return Let(v); },            //
+        [&](const ast::Override*) { return Override(v); },  //
+        [&](Default) {
+            TINT_ICE(Resolver, diagnostics_)
+                << "Validator::Variable() called with a unknown variable type: "
+                << decl->TypeInfo().name;
+            return false;
+        });
+}
 
-    auto* kind = decl->Is<ast::Override>() ? "'override'" : "'let'";
+bool Validator::Let(const sem::Variable* v) const {
+    auto* decl = v->Declaration();
+    auto* storage_ty = v->Type()->UnwrapRef();
 
     if (v->Is<sem::GlobalVariable>()) {
         auto name = symbols_.NameFor(decl->symbol);
         if (sem::ParseBuiltinType(name) != sem::BuiltinType::kNone) {
-            AddError("'" + name + "' is a builtin and cannot be redeclared as a " + kind,
+            AddError("'" + name + "' is a builtin and cannot be redeclared as a 'let'",
                      decl->source);
             return false;
         }
     }
 
     if (!(storage_ty->IsConstructible() || storage_ty->Is<sem::Pointer>())) {
-        AddError(sem_.TypeNameOf(storage_ty) + " cannot be used as the type of a " + kind,
+        AddError(sem_.TypeNameOf(storage_ty) + " cannot be used as the type of a 'let'",
+                 decl->source);
+        return false;
+    }
+    return true;
+}
+
+bool Validator::Override(const sem::Variable* v) const {
+    auto* decl = v->Declaration();
+    auto* storage_ty = v->Type()->UnwrapRef();
+
+    auto name = symbols_.NameFor(decl->symbol);
+    if (sem::ParseBuiltinType(name) != sem::BuiltinType::kNone) {
+        AddError("'" + name + "' is a builtin and cannot be redeclared as a 'override'",
+                 decl->source);
+        return false;
+    }
+
+    if (!storage_ty->is_scalar()) {
+        AddError(sem_.TypeNameOf(storage_ty) + " cannot be used as the type of a 'override'",
                  decl->source);
         return false;
     }

src/tint/resolver/validator.h

@@ -356,6 +356,16 @@ class Validator {
     /// @returns true on success, false otherwise.
     bool Variable(const sem::Variable* v) const;
 
+    /// Validates a 'let' variable declaration
+    /// @param v the variable to validate
+    /// @returns true on success, false otherwise.
+    bool Let(const sem::Variable* v) const;
+
+    /// Validates a 'override' variable declaration
+    /// @param v the variable to validate
+    /// @returns true on success, false otherwise.
+    bool Override(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/var_let_validation_test.cc

@@ -74,6 +74,22 @@ TEST_F(ResolverVarLetValidationTest, LetTypeNotConstructible) {
     EXPECT_EQ(r()->error(), "56:78 error: texture_2d<f32> cannot be used as the type of a 'let'");
 }
 
+TEST_F(ResolverVarLetValidationTest, OverrideExplicitTypeNotScalar) {
+    // override o : vec3<f32>;
+    Override(Source{{56, 78}}, "o", ty.vec3<f32>(), nullptr);
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), "56:78 error: vec3<f32> cannot be used as the type of a 'override'");
+}
+
+TEST_F(ResolverVarLetValidationTest, OverrideInferedTypeNotScalar) {
+    // override o = vec3(1.0f);
+    Override(Source{{56, 78}}, "o", nullptr, vec3<f32>(1.0_f));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), "56:78 error: vec3<f32> cannot be used as the type of a 'override'");
+}
+
 TEST_F(ResolverVarLetValidationTest, LetConstructorWrongType) {
     // var v : i32 = 2u
     WrapInFunction(Let(Source{{3, 3}}, "v", ty.i32(), Expr(2_u)));