tint/resolver: Clean up parameter resolving

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

src/tint/resolver/function_validation_test.cc

@@ -718,9 +718,7 @@ TEST_F(ResolverFunctionValidationTest, ParameterStoreType_NonAtomicFree) {
     Func("f", {bar}, ty.void_(), {});
 
     EXPECT_FALSE(r()->Resolve());
-    EXPECT_EQ(r()->error(),
+    EXPECT_EQ(r()->error(), "12:34 error: type of function parameter must be constructible");
-              "12:34 error: store type of function parameter must be a "
-              "constructible type");
 }
 
 TEST_F(ResolverFunctionValidationTest, ParameterSotreType_AtomicFree) {

src/tint/resolver/resolver.cc

@@ -311,9 +311,7 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
     return s;
 }
 
-sem::Variable* Resolver::Variable(const ast::Variable* v,
+sem::Variable* Resolver::Variable(const ast::Variable* v, bool is_global) {
-                                  bool is_global,
-                                  uint32_t index /* = 0 */) {
     const sem::Type* storage_ty = nullptr;
 
     // If the variable has a declared type, resolve it.
@@ -327,7 +325,6 @@ sem::Variable* Resolver::Variable(const ast::Variable* v,
     auto* as_var = v->As<ast::Var>();
     auto* as_let = v->As<ast::Let>();
     auto* as_override = v->As<ast::Override>();
-    auto* as_param = v->As<ast::Parameter>();
 
     const sem::Expression* rhs = nullptr;
 
@@ -402,29 +399,11 @@ sem::Variable* Resolver::Variable(const ast::Variable* v,
     }
 
     if (!ApplyStorageClassUsageToType(storage_class, const_cast<sem::Type*>(var_ty), v->source)) {
-        AddNote(std::string("while instantiating ") + ((as_param) ? "parameter " : "variable ") +
+        AddNote("while instantiating variable " + builder_->Symbols().NameFor(v->symbol),
-                    builder_->Symbols().NameFor(v->symbol),
                 v->source);
         return nullptr;
     }
 
-    if (as_param) {
-        if (auto* ptr = var_ty->As<sem::Pointer>()) {
-            // For MSL, we push module-scope variables into the entry point as pointer
-            // parameters, so we also need to handle their store type.
-            if (!ApplyStorageClassUsageToType(
-                    ptr->StorageClass(), const_cast<sem::Type*>(ptr->StoreType()), v->source)) {
-                AddNote("while instantiating parameter " + builder_->Symbols().NameFor(v->symbol),
-                        v->source);
-                return nullptr;
-            }
-        }
-        auto* param =
-            builder_->create<sem::Parameter>(as_param, index, var_ty, storage_class, access);
-        builder_->Sem().Add(as_param, param);
-        return param;
-    }
-
     if (is_global) {
         sem::BindingPoint binding_point;
         if (as_var) {
@@ -457,6 +436,45 @@ sem::Variable* Resolver::Variable(const ast::Variable* v,
     return local;
 }
 
+sem::Parameter* Resolver::Parameter(const ast::Parameter* param, uint32_t index) {
+    auto add_note = [&] {
+        AddNote("while instantiating parameter " + builder_->Symbols().NameFor(param->symbol),
+                param->source);
+    };
+
+    for (auto* attr : param->attributes) {
+        Mark(attr);
+    }
+    if (!validator_.NoDuplicateAttributes(param->attributes)) {
+        return nullptr;
+    }
+
+    sem::Type* ty = Type(param->type);
+    if (!ty) {
+        return nullptr;
+    }
+
+    if (!ApplyStorageClassUsageToType(ast::StorageClass::kNone, ty, param->source)) {
+        add_note();
+        return nullptr;
+    }
+
+    if (auto* ptr = ty->As<sem::Pointer>()) {
+        // For MSL, we push module-scope variables into the entry point as pointer
+        // parameters, so we also need to handle their store type.
+        if (!ApplyStorageClassUsageToType(
+                ptr->StorageClass(), const_cast<sem::Type*>(ptr->StoreType()), param->source)) {
+            add_note();
+            return nullptr;
+        }
+    }
+
+    auto* sem = builder_->create<sem::Parameter>(param, index, ty, ast::StorageClass::kNone,
+                                                 ast::Access::kUndefined);
+    builder_->Sem().Add(param, sem);
+    return sem;
+}
+
 ast::Access Resolver::DefaultAccessForStorageClass(ast::StorageClass storage_class) {
     // https://gpuweb.github.io/gpuweb/wgsl/#storage-class
     switch (storage_class) {
@@ -574,15 +592,12 @@ sem::Function* Resolver::Function(const ast::Function* decl) {
             }
         }
 
-        auto* p = As<sem::Parameter>(Variable(param, false, parameter_index++));
+        auto* p = Parameter(param, parameter_index++);
         if (!p) {
             return nullptr;
         }
 
-        for (auto* attr : param->attributes) {
+        if (!validator_.Parameter(decl, p)) {
-            Mark(attr);
-        }
-        if (!validator_.NoDuplicateAttributes(param->attributes)) {
             return nullptr;
         }
 

src/tint/resolver/resolver.h

@@ -290,14 +290,20 @@ class Resolver {
     /// raised.
     sem::Struct* Structure(const ast::Struct* str);
 
-    /// @returns the semantic info for the variable `var`. If an error is
+    /// @returns the semantic info for the variable `v`. If an error is raised, nullptr is
-    /// raised, nullptr is returned.
+    /// returned.
-    /// @note this method does not resolve the attributes as these are
+    /// @note this method does not resolve the attributes as these are context-dependent (global,
-    /// context-dependent (global, local, parameter)
+    /// local)
-    /// @param var the variable to create or return the `VariableInfo` for
+    /// @param var the variable
     /// @param is_global true if this is module scope, otherwise function scope
-    /// @param index the index of the parameter, if this variable is a parameter
+    sem::Variable* Variable(const ast::Variable* var, bool is_global);
-    sem::Variable* Variable(const ast::Variable* var, bool is_global, uint32_t index = 0);
+
+    /// @returns the semantic info for the function parameter `param`. If an error is raised,
+    /// nullptr is returned.
+    /// @note the caller is expected to validate the parameter
+    /// @param param the AST parameter
+    /// @param index the index of the parameter
+    sem::Parameter* Parameter(const ast::Parameter* param, uint32_t index);
 
     /// Records the storage class usage for the given type, and any transient
     /// dependencies of the type. Validates that the type can be used for the

src/tint/resolver/validator.cc

@@ -739,11 +739,7 @@ bool Validator::Variable(const sem::Variable* v) const {
     return true;
 }
 
-bool Validator::FunctionParameter(const ast::Function* func, const sem::Variable* var) const {
+bool Validator::Parameter(const ast::Function* func, const sem::Variable* var) const {
-    if (!Variable(var)) {
-        return false;
-    }
-
     auto* decl = var->Declaration();
 
     if (IsValidationDisabled(decl->attributes, ast::DisabledValidation::kFunctionParameter)) {
@@ -779,11 +775,11 @@ bool Validator::FunctionParameter(const ast::Function* func, const sem::Variable
 
     if (IsPlain(var->Type())) {
         if (!var->Type()->IsConstructible()) {
-            AddError("store type of function parameter must be a constructible type", decl->source);
+            AddError("type of function parameter must be constructible", decl->source);
             return false;
         }
     } else if (!var->Type()->IsAnyOf<sem::Texture, sem::Sampler, sem::Pointer>()) {
-        AddError("store type of function parameter cannot be " + sem_.TypeNameOf(var->Type()),
+        AddError("type of function parameter cannot be " + sem_.TypeNameOf(var->Type()),
                  decl->source);
         return false;
     }
@@ -948,12 +944,6 @@ bool Validator::Function(const sem::Function* func, ast::PipelineStage stage) co
         return false;
     }
 
-    for (size_t i = 0; i < decl->params.size(); i++) {
-        if (!FunctionParameter(decl, func->Parameters()[i])) {
-            return false;
-        }
-    }
-
     if (!func->ReturnType()->Is<sem::Void>()) {
         if (!func->ReturnType()->IsConstructible()) {
             AddError("function return type must be a constructible type",

src/tint/resolver/validator.h

@@ -298,7 +298,7 @@ class Validator {
     /// @param func the function the variable is for
     /// @param var the variable to validate
     /// @returns true on success, false otherwise
-    bool FunctionParameter(const ast::Function* func, const sem::Variable* var) const;
+    bool Parameter(const ast::Function* func, const sem::Variable* var) const;
 
     /// Validates a return
     /// @param ret the return statement to validate