Move function validation from Validator to Resolver

* Fixed many tests that now failed validation. Most of the time,
functions declared that they returned a type, but with no return
statement.
* ProgramBuilder::WrapInFunction now returns the function is creates,
and std::moves its StatementList.
* ProgramBuilder: Added Return function to create ast::ReturnStatements
more easily.

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

src/resolver/decoration_validation_test.cc

@@ -14,6 +14,7 @@
 
 #include "src/ast/access_decoration.h"
 #include "src/ast/constant_id_decoration.h"
+#include "src/ast/return_statement.h"
 #include "src/ast/stage_decoration.h"
 #include "src/ast/struct_block_decoration.h"
 #include "src/ast/workgroup_decoration.h"
@@ -84,6 +85,41 @@ ast::Decoration* createDecoration(const Source& source,
   return nullptr;
 }
 
+using FunctionReturnTypeDecorationTest = TestWithParams;
+TEST_P(FunctionReturnTypeDecorationTest, IsValid) {
+  auto params = GetParam();
+
+  Func("main", ast::VariableList{}, ty.f32(),
+       ast::StatementList{create<ast::ReturnStatement>(Expr(1.f))},
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex)},
+       ast::DecorationList{createDecoration({}, *this, params.kind)});
+
+  if (params.should_pass) {
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+  } else {
+    EXPECT_FALSE(r()->Resolve()) << r()->error();
+    EXPECT_EQ(r()->error(),
+              "error: decoration is not valid for function return types");
+  }
+}
+INSTANTIATE_TEST_SUITE_P(
+    ResolverDecorationValidationTest,
+    FunctionReturnTypeDecorationTest,
+    testing::Values(TestParams{DecorationKind::kAccess, false},
+                    TestParams{DecorationKind::kAlign, false},
+                    TestParams{DecorationKind::kBinding, false},
+                    TestParams{DecorationKind::kBuiltin, true},
+                    TestParams{DecorationKind::kConstantId, false},
+                    TestParams{DecorationKind::kGroup, false},
+                    TestParams{DecorationKind::kLocation, true},
+                    TestParams{DecorationKind::kOffset, false},
+                    TestParams{DecorationKind::kSize, false},
+                    TestParams{DecorationKind::kStage, false},
+                    TestParams{DecorationKind::kStride, false},
+                    TestParams{DecorationKind::kStructBlock, false},
+                    TestParams{DecorationKind::kWorkgroup, false}));
+
 using ArrayDecorationTest = TestWithParams;
 
 TEST_P(ArrayDecorationTest, IsValid) {

src/resolver/function_validation_test.cc

@@ -0,0 +1,78 @@
+// Copyright 2021 The Tint Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "src/ast/return_statement.h"
+#include "src/resolver/resolver.h"
+#include "src/resolver/resolver_test_helper.h"
+
+#include "gmock/gmock.h"
+
+namespace tint {
+namespace {
+
+class ResolverFunctionValidationTest : public resolver::TestHelper,
+                                       public testing::Test {};
+
+TEST_F(ResolverFunctionValidationTest, FunctionNamesMustBeUnique_fail) {
+  // fn func -> i32 { return 2; }
+  // fn func -> i32 { return 2; }
+  Func("func", ast::VariableList{}, ty.i32(),
+       ast::StatementList{
+           create<ast::ReturnStatement>(Expr(2)),
+       },
+       ast::DecorationList{});
+
+  Func(Source{Source::Location{12, 34}}, "func", ast::VariableList{}, ty.i32(),
+       ast::StatementList{
+           create<ast::ReturnStatement>(Expr(2)),
+       },
+       ast::DecorationList{});
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error v-0016: function names must be unique 'func'");
+}
+
+TEST_F(ResolverFunctionValidationTest, FunctionEndWithoutReturnStatement_Fail) {
+  // fn func -> int { var a:i32 = 2; }
+
+  auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+
+  Func(Source{Source::Location{12, 34}}, "func", ast::VariableList{}, ty.i32(),
+       ast::StatementList{
+           create<ast::VariableDeclStatement>(var),
+       },
+       ast::DecorationList{});
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(
+      r()->error(),
+      "12:34 error v-0002: non-void function must end with a return statement");
+}
+
+TEST_F(ResolverFunctionValidationTest,
+       FunctionEndWithoutReturnStatementEmptyBody_Fail) {
+  // fn func -> int {}
+
+  Func(Source{Source::Location{12, 34}}, "func", ast::VariableList{}, ty.i32(),
+       ast::StatementList{}, ast::DecorationList{});
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(
+      r()->error(),
+      "12:34 error v-0002: non-void function must end with a return statement");
+}
+
+}  // namespace
+}  // namespace tint

src/resolver/resolver.cc

@@ -222,9 +222,65 @@ bool Resolver::Functions(const ast::FunctionList& funcs) {
   return true;
 }
 
+bool Resolver::ValidateParameter(const ast::Variable* param) {
+  if (auto* r = param->type()->UnwrapAll()->As<type::Array>()) {
+    if (r->IsRuntimeArray()) {
+      diagnostics_.add_error(
+          "v-0015",
+          "runtime arrays may only appear as the last member of a struct",
+          param->source());
+      return false;
+    }
+  }
+  return true;
+}
+
+bool Resolver::ValidateFunction(const ast::Function* func) {
+  if (symbol_to_function_.find(func->symbol()) != symbol_to_function_.end()) {
+    diagnostics_.add_error("v-0016",
+                           "function names must be unique '" +
+                               builder_->Symbols().NameFor(func->symbol()) +
+                               "'",
+                           func->source());
+    return false;
+  }
+
+  for (auto* param : func->params()) {
+    if (!ValidateParameter(param)) {
+      return false;
+    }
+  }
+
+  if (!func->return_type()->Is<type::Void>()) {
+    if (!func->get_last_statement() ||
+        !func->get_last_statement()->Is<ast::ReturnStatement>()) {
+      diagnostics_.add_error(
+          "v-0002", "non-void function must end with a return statement",
+          func->source());
+      return false;
+    }
+
+    for (auto* deco : func->return_type_decorations()) {
+      if (!(deco->Is<ast::BuiltinDecoration>() ||
+            deco->Is<ast::LocationDecoration>())) {
+        diagnostics_.add_error(
+            "decoration is not valid for function return types",
+            deco->source());
+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
 bool Resolver::Function(ast::Function* func) {
   auto* func_info = function_infos_.Create<FunctionInfo>(func);
 
+  if (!ValidateFunction(func)) {
+    return false;
+  }
+
   ScopedAssignment<FunctionInfo*> sa(current_function_, func_info);
 
   variable_stack_.push_scope();

src/resolver/resolver.h

@@ -224,16 +224,15 @@ class Resolver {
   // AST and Type validation methods
   // Each return true on success, false on failure.
   bool ValidateBinary(ast::BinaryExpression* expr);
+  bool ValidateParameter(const ast::Variable* param);
+  bool ValidateFunction(const ast::Function* func);
+  bool ValidateStructure(const type::Struct* st);
 
   /// @returns the semantic information for the array `arr`, building it if it
   /// hasn't been constructed already. If an error is raised, nullptr is
   /// returned.
   const semantic::Array* Array(type::Array*);
 
-  /// @returns returns true if input struct is valid
-  /// @param st the struct to validate
-  bool ValidateStructure(const type::Struct* st);
-
   /// @returns the StructInfo for the structure `str`, building it if it hasn't
   /// been constructed already. If an error is raised, nullptr is returned.
   StructInfo* Structure(type::Struct* str);
@@ -286,7 +285,7 @@ class Resolver {
   BlockInfo* current_block_ = nullptr;
   ScopeStack<VariableInfo*> variable_stack_;
   std::unordered_map<Symbol, FunctionInfo*> symbol_to_function_;
-  std::unordered_map<ast::Function*, FunctionInfo*> function_to_info_;
+  std::unordered_map<const ast::Function*, FunctionInfo*> function_to_info_;
   std::unordered_map<ast::Variable*, VariableInfo*> variable_to_info_;
   std::unordered_map<ast::CallExpression*, FunctionCallInfo> function_calls_;
   std::unordered_map<ast::Expression*, ExpressionInfo> expr_info_;

src/resolver/resolver_test.cc

@@ -271,7 +271,7 @@ TEST_F(ResolverTest, Stmt_Switch) {
 
 TEST_F(ResolverTest, Stmt_Call) {
   ast::VariableList params;
-  Func("my_func", params, ty.f32(), ast::StatementList{},
+  Func("my_func", params, ty.f32(), ast::StatementList{Return(Expr(0.0f))},
        ast::DecorationList{});
 
   auto* expr = Call("my_func");
@@ -325,7 +325,7 @@ TEST_F(ResolverTest, Stmt_VariableDecl_ModuleScope) {
 }
 
 TEST_F(ResolverTest, Stmt_VariableDecl_OuterScopeAfterInnerScope) {
-  // fn func_i32() -> i32 {
+  // fn func_i32() -> void {
   //   {
   //     var foo : i32 = 2;
   //     var bar : i32 = foo;
@@ -359,11 +359,11 @@ TEST_F(ResolverTest, Stmt_VariableDecl_OuterScopeAfterInnerScope) {
   auto* bar_f32_init = bar_f32->constructor();
   auto* bar_f32_decl = create<ast::VariableDeclStatement>(bar_f32);
 
-  Func("func", params, ty.f32(),
+  Func("func", params, ty.void_(),
        ast::StatementList{inner, foo_f32_decl, bar_f32_decl},
        ast::DecorationList{});
 
-  EXPECT_TRUE(r()->Resolve());
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
   ASSERT_NE(TypeOf(foo_i32_init), nullptr);
   EXPECT_TRUE(TypeOf(foo_i32_init)->Is<type::I32>());
   ASSERT_NE(TypeOf(foo_f32_init), nullptr);
@@ -381,11 +381,11 @@ TEST_F(ResolverTest, Stmt_VariableDecl_OuterScopeAfterInnerScope) {
 }
 
 TEST_F(ResolverTest, Stmt_VariableDecl_ModuleScopeAfterFunctionScope) {
-  // fn func_i32() -> i32 {
+  // fn func_i32() -> void {
   //   var foo : i32 = 2;
   // }
   // var foo : f32 = 2.0;
-  // fn func_f32() -> f32 {
+  // fn func_f32() -> void {
   //   var bar : f32 = foo;
   // }
 
@@ -395,7 +395,7 @@ TEST_F(ResolverTest, Stmt_VariableDecl_ModuleScopeAfterFunctionScope) {
   auto* fn_i32 = Var("foo", ty.i32(), ast::StorageClass::kNone, Expr(2));
   auto* fn_i32_init = fn_i32->constructor();
   auto* fn_i32_decl = create<ast::VariableDeclStatement>(fn_i32);
-  Func("func_i32", params, ty.i32(), ast::StatementList{fn_i32_decl},
+  Func("func_i32", params, ty.void_(), ast::StatementList{fn_i32_decl},
        ast::DecorationList{});
 
   // Declare f32 "foo" at module scope
@@ -407,10 +407,10 @@ TEST_F(ResolverTest, Stmt_VariableDecl_ModuleScopeAfterFunctionScope) {
   auto* fn_f32 = Var("bar", ty.f32(), ast::StorageClass::kNone, Expr("foo"));
   auto* fn_f32_init = fn_f32->constructor();
   auto* fn_f32_decl = create<ast::VariableDeclStatement>(fn_f32);
-  Func("func_f32", params, ty.f32(), ast::StatementList{fn_f32_decl},
+  Func("func_f32", params, ty.void_(), ast::StatementList{fn_f32_decl},
        ast::DecorationList{});
 
-  EXPECT_TRUE(r()->Resolve());
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
   ASSERT_NE(TypeOf(mod_init), nullptr);
   EXPECT_TRUE(TypeOf(mod_init)->Is<type::F32>());
   ASSERT_NE(TypeOf(fn_i32_init), nullptr);
@@ -529,7 +529,7 @@ TEST_F(ResolverTest, Expr_Bitcast) {
 
 TEST_F(ResolverTest, Expr_Call) {
   ast::VariableList params;
-  Func("my_func", params, ty.f32(), ast::StatementList{},
+  Func("my_func", params, ty.f32(), ast::StatementList{Return(Expr(0.0f))},
        ast::DecorationList{});
 
   auto* call = Call("my_func");
@@ -543,7 +543,8 @@ TEST_F(ResolverTest, Expr_Call) {
 
 TEST_F(ResolverTest, Expr_Call_InBinaryOp) {
   ast::VariableList params;
-  Func("func", params, ty.f32(), ast::StatementList{}, ast::DecorationList{});
+  Func("func", params, ty.f32(), ast::StatementList{Return(Expr(0.0f))},
+       ast::DecorationList{});
 
   auto* expr = Add(Call("func"), Call("func"));
   WrapInFunction(expr);
@@ -556,7 +557,7 @@ TEST_F(ResolverTest, Expr_Call_InBinaryOp) {
 
 TEST_F(ResolverTest, Expr_Call_WithParams) {
   ast::VariableList params;
-  Func("my_func", params, ty.f32(), ast::StatementList{},
+  Func("my_func", params, ty.void_(), ast::StatementList{},
        ast::DecorationList{});
 
   auto* param = Expr(2.4f);
@@ -671,7 +672,7 @@ TEST_F(ResolverTest, Expr_Identifier_FunctionVariable_Const) {
   auto* var = Const("my_var", ty.f32());
   auto* assign = create<ast::AssignmentStatement>(my_var_a, my_var_b);
 
-  Func("my_func", ast::VariableList{}, ty.f32(),
+  Func("my_func", ast::VariableList{}, ty.void_(),
        ast::StatementList{
            create<ast::VariableDeclStatement>(var),
            assign,
@@ -696,7 +697,7 @@ TEST_F(ResolverTest, Expr_Identifier_FunctionVariable) {
 
   auto* var = Var("my_var", ty.f32(), ast::StorageClass::kNone);
 
-  Func("my_func", ast::VariableList{}, ty.f32(),
+  Func("my_func", ast::VariableList{}, ty.void_(),
        ast::StatementList{
            create<ast::VariableDeclStatement>(var),
            assign,
@@ -721,7 +722,7 @@ TEST_F(ResolverTest, Expr_Identifier_Function_Ptr) {
   auto* my_var_b = Expr("my_var");
   auto* assign = create<ast::AssignmentStatement>(my_var_a, my_var_b);
 
-  Func("my_func", ast::VariableList{}, ty.f32(),
+  Func("my_func", ast::VariableList{}, ty.void_(),
        ast::StatementList{
            create<ast::VariableDeclStatement>(
                Var("my_var", ty.pointer<f32>(ast::StorageClass::kFunction),
@@ -743,8 +744,8 @@ TEST_F(ResolverTest, Expr_Identifier_Function_Ptr) {
 }
 
 TEST_F(ResolverTest, Expr_Call_Function) {
-  Func("my_func", ast::VariableList{}, ty.f32(), ast::StatementList{},
-       ast::DecorationList{});
+  Func("my_func", ast::VariableList{}, ty.f32(),
+       ast::StatementList{Return(Expr(0.0f))}, ast::DecorationList{});
 
   auto* call = Call("my_func");
   WrapInFunction(call);
@@ -770,7 +771,7 @@ TEST_F(ResolverTest, Function_RegisterInputOutputVariables) {
   auto* priv_var = Global("priv_var", ty.f32(), ast::StorageClass::kPrivate);
 
   auto* func = Func(
-      "my_func", ast::VariableList{}, ty.f32(),
+      "my_func", ast::VariableList{}, ty.void_(),
       ast::StatementList{
           create<ast::AssignmentStatement>(Expr("out_var"), Expr("in_var")),
           create<ast::AssignmentStatement>(Expr("wg_var"), Expr("wg_var")),
@@ -806,11 +807,11 @@ TEST_F(ResolverTest, Function_RegisterInputOutputVariables_SubFunction) {
            create<ast::AssignmentStatement>(Expr("wg_var"), Expr("wg_var")),
            create<ast::AssignmentStatement>(Expr("sb_var"), Expr("sb_var")),
            create<ast::AssignmentStatement>(Expr("priv_var"), Expr("priv_var")),
-       },
+           Return(Expr(0.0f))},
        ast::DecorationList{});
 
   auto* func2 = Func(
-      "func", ast::VariableList{}, ty.f32(),
+      "func", ast::VariableList{}, ty.void_(),
       ast::StatementList{
           create<ast::AssignmentStatement>(Expr("out_var"), Call("my_func")),
       },
@@ -834,7 +835,7 @@ TEST_F(ResolverTest, Function_NotRegisterFunctionVariable) {
   auto* var = Var("in_var", ty.f32(), ast::StorageClass::kFunction);
 
   auto* func =
-      Func("my_func", ast::VariableList{}, ty.f32(),
+      Func("my_func", ast::VariableList{}, ty.void_(),
            ast::StatementList{
                create<ast::VariableDeclStatement>(var),
                create<ast::AssignmentStatement>(Expr("var"), Expr(1.f)),
@@ -1276,7 +1277,7 @@ TEST_F(ResolverTest, StorageClass_SetsIfMissing) {
   auto* var = Var("var", ty.i32(), ast::StorageClass::kNone);
 
   auto* stmt = create<ast::VariableDeclStatement>(var);
-  Func("func", ast::VariableList{}, ty.i32(), ast::StatementList{stmt},
+  Func("func", ast::VariableList{}, ty.void_(), ast::StatementList{stmt},
        ast::DecorationList{});
 
   EXPECT_TRUE(r()->Resolve()) << r()->error();
@@ -1287,7 +1288,7 @@ TEST_F(ResolverTest, StorageClass_SetsIfMissing) {
 TEST_F(ResolverTest, StorageClass_DoesNotSetOnConst) {
   auto* var = Const("var", ty.i32());
   auto* stmt = create<ast::VariableDeclStatement>(var);
-  Func("func", ast::VariableList{}, ty.i32(), ast::StatementList{stmt},
+  Func("func", ast::VariableList{}, ty.void_(), ast::StatementList{stmt},
        ast::DecorationList{});
 
   EXPECT_TRUE(r()->Resolve()) << r()->error();
@@ -1310,23 +1311,22 @@ TEST_F(ResolverTest, Function_EntryPoints_StageDecoration) {
 
   ast::VariableList params;
   auto* func_b =
-      Func("b", params, ty.f32(), ast::StatementList{}, ast::DecorationList{});
-  auto* func_c =
-      Func("c", params, ty.f32(),
-           ast::StatementList{
-               create<ast::AssignmentStatement>(Expr("second"), Call("b")),
-           },
+      Func("b", params, ty.f32(), ast::StatementList{Return(Expr(0.0f))},
            ast::DecorationList{});
+  auto* func_c = Func("c", params, ty.f32(),
+                      ast::StatementList{create<ast::AssignmentStatement>(
+                                             Expr("second"), Call("b")),
+                                         Return(Expr(0.0f))},
+                      ast::DecorationList{});
 
-  auto* func_a =
-      Func("a", params, ty.f32(),
-           ast::StatementList{
-               create<ast::AssignmentStatement>(Expr("first"), Call("c")),
-           },
-           ast::DecorationList{});
+  auto* func_a = Func("a", params, ty.f32(),
+                      ast::StatementList{create<ast::AssignmentStatement>(
+                                             Expr("first"), Call("c")),
+                                         Return(Expr(0.0f))},
+                      ast::DecorationList{});
 
   auto* ep_1 =
-      Func("ep_1", params, ty.f32(),
+      Func("ep_1", params, ty.void_(),
            ast::StatementList{
                create<ast::AssignmentStatement>(Expr("call_a"), Call("a")),
                create<ast::AssignmentStatement>(Expr("call_b"), Call("b")),
@@ -1336,7 +1336,7 @@ TEST_F(ResolverTest, Function_EntryPoints_StageDecoration) {
            });
 
   auto* ep_2 =
-      Func("ep_2", params, ty.f32(),
+      Func("ep_2", params, ty.void_(),
            ast::StatementList{
                create<ast::AssignmentStatement>(Expr("call_c"), Call("c")),
            },

src/resolver/type_validation_test.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include "src/ast/return_statement.h"
 #include "src/ast/stage_decoration.h"
 #include "src/ast/struct_block_decoration.h"
 #include "src/resolver/resolver.h"
@@ -96,6 +97,34 @@ TEST_F(ResolverTypeValidationTest, RuntimeArrayIsNotLast_Fail) {
             "member of a struct");
 }
 
+TEST_F(ResolverTypeValidationTest, RuntimeArrayAsParameter_Fail) {
+  // fn func(a : array<u32>) {}
+  // [[stage(vertex)]] fn main() {}
+
+  auto* param = Var(Source{Source::Location{12, 34}}, "a", ty.array<i32>(),
+                    ast::StorageClass::kNone);
+
+  Func("func", ast::VariableList{param}, ty.void_(),
+       ast::StatementList{
+           create<ast::ReturnStatement>(),
+       },
+       ast::DecorationList{});
+
+  Func("main", ast::VariableList{}, ty.void_(),
+       ast::StatementList{
+           create<ast::ReturnStatement>(),
+       },
+       ast::DecorationList{
+           create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+       });
+
+  EXPECT_FALSE(r()->Resolve()) << r()->error();
+  EXPECT_EQ(
+      r()->error(),
+      "12:34 error v-0015: runtime arrays may only appear as the last member "
+      "of a struct");
+}
+
 TEST_F(ResolverTypeValidationTest, AliasRuntimeArrayIsNotLast_Fail) {
   // [[Block]]
   // type RTArr = array<u32>;

src/resolver/validation_test.cc

@@ -116,7 +116,7 @@ TEST_F(ResolverValidationTest, Stmt_Call_recursive) {
   auto* call_expr = Call("main");
   ast::VariableList params0;
 
-  Func("main", params0, ty.f32(),
+  Func("main", params0, ty.void_(),
        ast::StatementList{
            create<ast::CallStatement>(call_expr),
        },
@@ -245,7 +245,7 @@ TEST_F(ResolverValidationTest, StorageClass_NonFunctionClassError) {
   auto* var = Var("var", ty.i32(), ast::StorageClass::kWorkgroup);
 
   auto* stmt = create<ast::VariableDeclStatement>(var);
-  Func("func", ast::VariableList{}, ty.i32(), ast::StatementList{stmt},
+  Func("func", ast::VariableList{}, ty.void_(), ast::StatementList{stmt},
        ast::DecorationList{});
 
   EXPECT_FALSE(r()->Resolve());