Move VariableDecl validation from Validator to Resolver
Moved tests and fixed now broken tests.
Bug: tint:642
Change-Id: Iaa4483abde101f3963ca20e51c1069b2d64bbb5c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46661
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index e7c512d..12fca4e 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -524,7 +524,6 @@
validator/validator_decoration_test.cc
validator/validator_function_test.cc
validator/validator_test.cc
- validator/validator_type_test.cc
validator/validator_test_helper.cc
validator/validator_test_helper.h
writer/append_vector_test.cc
diff --git a/src/resolver/assignment_validation_test.cc b/src/resolver/assignment_validation_test.cc
index f4e8489..0b248d1 100644
--- a/src/resolver/assignment_validation_test.cc
+++ b/src/resolver/assignment_validation_test.cc
@@ -81,9 +81,9 @@
create<ast::VariableDeclStatement>(var),
create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs),
});
- WrapInFunction(var, body);
+ WrapInFunction(body);
- ASSERT_TRUE(r()->Resolve());
+ ASSERT_TRUE(r()->Resolve()) << r()->error();
}
TEST_F(ResolverAssignmentValidationTest,
@@ -101,7 +101,7 @@
create<ast::VariableDeclStatement>(var),
create<ast::AssignmentStatement>(Source{{12, 34}}, lhs, rhs),
});
- WrapInFunction(var, block);
+ WrapInFunction(block);
ASSERT_FALSE(r()->Resolve());
@@ -132,7 +132,7 @@
inner_block,
});
- WrapInFunction(var, outer_block);
+ WrapInFunction(outer_block);
ASSERT_FALSE(r()->Resolve());
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 0c79dd7..79173e8 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -224,20 +224,24 @@
return true;
}
-bool Resolver::ValidateParameter(const ast::Variable* param) {
- auto* type = variable_to_info_[param]->type;
+bool Resolver::ValidateVariable(const ast::Variable* var) {
+ auto* type = variable_to_info_[var]->type;
if (auto* r = 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());
+ var->source());
return false;
}
}
return true;
}
+bool Resolver::ValidateParameter(const ast::Variable* param) {
+ return ValidateVariable(param);
+}
+
bool Resolver::ValidateFunction(const ast::Function* func) {
if (symbol_to_function_.find(func->symbol()) != symbol_to_function_.end()) {
diagnostics_.add_error("v-0016",
@@ -1278,6 +1282,16 @@
ast::Variable* var = stmt->variable();
type::Type* type = var->declared_type();
+ bool is_global = false;
+ if (variable_stack_.get(var->symbol(), nullptr, &is_global)) {
+ const char* error_code = is_global ? "v-0013" : "v-0014";
+ diagnostics_.add_error(error_code,
+ "redeclared identifier '" +
+ builder_->Symbols().NameFor(var->symbol()) + "'",
+ stmt->source());
+ return false;
+ }
+
if (auto* ctor = stmt->variable()->constructor()) {
if (!Expression(ctor)) {
return false;
@@ -1304,6 +1318,10 @@
variable_stack_.set(var->symbol(), info);
current_block_->decls.push_back(var);
+ if (!ValidateVariable(var)) {
+ return false;
+ }
+
if (!var->is_const()) {
if (info->storage_class != ast::StorageClass::kFunction) {
if (info->storage_class != ast::StorageClass::kNone) {
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index 709b2be..0c0700b 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -231,6 +231,7 @@
// AST and Type validation methods
// Each return true on success, false on failure.
bool ValidateBinary(ast::BinaryExpression* expr);
+ bool ValidateVariable(const ast::Variable* param);
bool ValidateParameter(const ast::Variable* param);
bool ValidateFunction(const ast::Function* func);
bool ValidateStructure(const type::Struct* st);
diff --git a/src/resolver/type_validation_test.cc b/src/resolver/type_validation_test.cc
index dc43bcf..a7c2394 100644
--- a/src/resolver/type_validation_test.cc
+++ b/src/resolver/type_validation_test.cc
@@ -26,6 +26,199 @@
class ResolverTypeValidationTest : public resolver::TestHelper,
public testing::Test {};
+TEST_F(ResolverTypeValidationTest,
+ GlobalVariableFunctionVariableNotUnique_Fail) {
+ // var a: f32 = 2.1;
+ // fn my_func -> void {
+ // var a: f32 = 2.0;
+ // return 0;
+ // }
+
+ Global("a", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
+
+ auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+ Func("my_func", ast::VariableList{}, ty.void_(),
+ ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var),
+ },
+ ast::DecorationList{});
+
+ EXPECT_FALSE(r()->Resolve()) << r()->error();
+ EXPECT_EQ(r()->error(), "12:34 error v-0013: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifier_Fail) {
+ // fn my_func() -> void {
+ // var a :i32 = 2;
+ // var a :f21 = 2.0;
+ // }
+ auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
+
+ auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(0.1f));
+
+ Func("my_func", ast::VariableList{}, ty.void_(),
+ ast::StatementList{
+ create<ast::VariableDeclStatement>(var),
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var_a_float),
+ },
+ ast::DecorationList{});
+
+ EXPECT_FALSE(r()->Resolve()) << r()->error();
+ EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScope_Pass) {
+ // {
+ // if (true) { var a : f32 = 2.0; }
+ // var a : f32 = 3.14;
+ // }
+ auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+ auto* cond = Expr(true);
+ auto* body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(var),
+ });
+
+ auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
+
+ auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var_a_float),
+ });
+
+ WrapInFunction(outer_body);
+
+ EXPECT_TRUE(r()->Resolve());
+}
+
+TEST_F(ResolverTypeValidationTest,
+ DISABLED_RedeclaredIdentifierInnerScope_False) {
+ // TODO(sarahM0): remove DISABLED after implementing ValidateIfStatement
+ // and it should just work
+ // {
+ // var a : f32 = 3.14;
+ // if (true) { var a : f32 = 2.0; }
+ // }
+ auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
+
+ auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+ auto* cond = Expr(true);
+ auto* body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}}, var),
+ });
+
+ auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(var_a_float),
+ create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
+ });
+
+ WrapInFunction(outer_body);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScopeBlock_Pass) {
+ // {
+ // { var a : f32; }
+ // var a : f32;
+ // }
+ auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
+ auto* inner = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var_inner),
+ });
+
+ auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
+ auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+ inner,
+ create<ast::VariableDeclStatement>(var_outer),
+ });
+
+ WrapInFunction(outer_body);
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverTypeValidationTest, RedeclaredIdentifierInnerScopeBlock_Fail) {
+ // {
+ // var a : f32;
+ // { var a : f32; }
+ // }
+ auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
+ auto* inner = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var_inner),
+ });
+
+ auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
+ auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
+ create<ast::VariableDeclStatement>(var_outer),
+ inner,
+ });
+
+ WrapInFunction(outer_body);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(), "12:34 error v-0014: redeclared identifier 'a'");
+}
+
+TEST_F(ResolverTypeValidationTest,
+ RedeclaredIdentifierDifferentFunctions_Pass) {
+ // func0 { var a : f32 = 2.0; return; }
+ // func1 { var a : f32 = 3.0; return; }
+ auto* var0 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
+
+ auto* var1 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(1.0f));
+
+ Func("func0", ast::VariableList{}, ty.void_(),
+ ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
+ var0),
+ create<ast::ReturnStatement>(),
+ },
+ ast::DecorationList{});
+
+ Func("func1", ast::VariableList{}, ty.void_(),
+ ast::StatementList{
+ create<ast::VariableDeclStatement>(Source{Source::Location{13, 34}},
+ var1),
+ create<ast::ReturnStatement>(),
+ },
+ ast::DecorationList{
+ create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+ });
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverTypeValidationTest, RuntimeArrayInFunction_Fail) {
+ /// [[stage(vertex)]]
+ // fn func -> void { var a : array<i32>; }
+
+ auto* var =
+ Var(Source{{12, 34}}, "a", ty.array<i32>(), ast::StorageClass::kNone);
+
+ Func("func", ast::VariableList{}, ty.void_(),
+ ast::StatementList{
+ create<ast::VariableDeclStatement>(var),
+ },
+ ast::DecorationList{
+ create<ast::StageDecoration>(ast::PipelineStage::kVertex),
+ });
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error v-0015: runtime arrays may only appear as the last member "
+ "of a struct");
+}
+
TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLast_Pass) {
// [[Block]]
// struct Foo {
diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc
index acc10fa..4952995 100644
--- a/src/validator/validator_impl.cc
+++ b/src/validator/validator_impl.cc
@@ -195,31 +195,7 @@
bool ValidatorImpl::ValidateDeclStatement(
const ast::VariableDeclStatement* decl) {
auto symbol = decl->variable()->symbol();
- bool is_global = false;
- if (variable_stack_.get(symbol, nullptr, &is_global)) {
- const char* error_code = "v-0014";
- if (is_global) {
- error_code = "v-0013";
- }
- add_error(
- decl->source(), error_code,
- "redeclared identifier '" + program_->Symbols().NameFor(symbol) + "'");
- return false;
- }
- // TODO(dneto): Check type compatibility of the initializer.
- // - if it's non-constant, then is storable or can be dereferenced to be
- // storable.
- // - types match or the RHS can be dereferenced to equal the LHS type.
variable_stack_.set(symbol, decl->variable());
- if (auto* arr =
- decl->variable()->declared_type()->UnwrapAll()->As<type::Array>()) {
- if (arr->IsRuntimeArray()) {
- add_error(
- decl->source(), "v-0015",
- "runtime arrays may only appear as the last member of a struct");
- return false;
- }
- }
return true;
}
diff --git a/src/validator/validator_test.cc b/src/validator/validator_test.cc
index 4a69b34..d496233 100644
--- a/src/validator/validator_test.cc
+++ b/src/validator/validator_test.cc
@@ -120,188 +120,6 @@
EXPECT_TRUE(v.Validate()) << v.error();
}
-TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Fail) {
- // var a: f32 = 2.1;
- // fn my_func -> void {
- // var a: f32 = 2.0;
- // return 0;
- // }
-
- Global("a", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f));
-
- auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
- Func("my_func", ast::VariableList{}, ty.void_(),
- ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var),
- },
- ast::DecorationList{});
-
- ValidatorImpl& v = Build();
-
- EXPECT_FALSE(v.Validate()) << v.error();
- EXPECT_EQ(v.error(), "12:34 v-0013: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifier_Fail) {
- // fn my_func() -> void {
- // var a :i32 = 2;
- // var a :f21 = 2.0;
- // }
- auto* var = Var("a", ty.i32(), ast::StorageClass::kNone, Expr(2));
-
- auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(0.1f));
-
- Func("my_func", ast::VariableList{}, ty.void_(),
- ast::StatementList{
- create<ast::VariableDeclStatement>(var),
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var_a_float),
- },
- ast::DecorationList{});
-
- ValidatorImpl& v = Build();
-
- EXPECT_FALSE(v.Validate());
- EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScope_Pass) {
- // {
- // if (true) { var a : f32 = 2.0; }
- // var a : f32 = 3.14;
- // }
- auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
- auto* cond = Expr(true);
- auto* body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(var),
- });
-
- auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
-
- auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var_a_float),
- });
-
- WrapInFunction(outer_body);
-
- ValidatorImpl& v = Build();
-
- EXPECT_TRUE(v.ValidateStatements(outer_body)) << v.error();
-}
-
-TEST_F(ValidatorTest, DISABLED_RedeclaredIdentifierInnerScope_False) {
- // TODO(sarahM0): remove DISABLED after implementing ValidateIfStatement
- // and it should just work
- // {
- // var a : f32 = 3.14;
- // if (true) { var a : f32 = 2.0; }
- // }
- auto* var_a_float = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(3.1f));
-
- auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
- auto* cond = Expr(true);
- auto* body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}}, var),
- });
-
- auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(var_a_float),
- create<ast::IfStatement>(cond, body, ast::ElseStatementList{}),
- });
-
- WrapInFunction(outer_body);
-
- ValidatorImpl& v = Build();
-
- EXPECT_FALSE(v.ValidateStatements(outer_body));
- EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScopeBlock_Pass) {
- // {
- // { var a : f32; }
- // var a : f32;
- // }
- auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
- auto* inner = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var_inner),
- });
-
- auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
- auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
- inner,
- create<ast::VariableDeclStatement>(var_outer),
- });
-
- WrapInFunction(outer_body);
-
- ValidatorImpl& v = Build();
-
- EXPECT_TRUE(v.ValidateStatements(outer_body));
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierInnerScopeBlock_Fail) {
- // {
- // var a : f32;
- // { var a : f32; }
- // }
- auto* var_inner = Var("a", ty.f32(), ast::StorageClass::kNone);
- auto* inner = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var_inner),
- });
-
- auto* var_outer = Var("a", ty.f32(), ast::StorageClass::kNone);
- auto* outer_body = create<ast::BlockStatement>(ast::StatementList{
- create<ast::VariableDeclStatement>(var_outer),
- inner,
- });
-
- WrapInFunction(outer_body);
-
- ValidatorImpl& v = Build();
-
- EXPECT_FALSE(v.ValidateStatements(outer_body));
- EXPECT_EQ(v.error(), "12:34 v-0014: redeclared identifier 'a'");
-}
-
-TEST_F(ValidatorTest, RedeclaredIdentifierDifferentFunctions_Pass) {
- // func0 { var a : f32 = 2.0; return; }
- // func1 { var a : f32 = 3.0; return; }
- auto* var0 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f));
-
- auto* var1 = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(1.0f));
-
- Func("func0", ast::VariableList{}, ty.void_(),
- ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var0),
- create<ast::ReturnStatement>(),
- },
- ast::DecorationList{});
-
- Func("func1", ast::VariableList{}, ty.void_(),
- ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{13, 34}},
- var1),
- create<ast::ReturnStatement>(),
- },
- ast::DecorationList{
- create<ast::StageDecoration>(ast::PipelineStage::kVertex),
- });
-
- ValidatorImpl& v = Build();
-
- EXPECT_TRUE(v.Validate()) << v.error();
-}
-
TEST_F(ValidatorTest, VariableDeclNoConstructor_Pass) {
// {
// var a :i32;
diff --git a/src/validator/validator_type_test.cc b/src/validator/validator_type_test.cc
deleted file mode 100644
index de8b6f5..0000000
--- a/src/validator/validator_type_test.cc
+++ /dev/null
@@ -1,48 +0,0 @@
-// Copyright 2020 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/struct_block_decoration.h"
-#include "src/validator/validator_test_helper.h"
-
-#include "src/ast/stage_decoration.h"
-namespace tint {
-namespace {
-
-class ValidatorTypeTest : public ValidatorTestHelper, public testing::Test {};
-
-TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {
- /// [[stage(vertex)]]
- // fn func -> void { var a : array<i32>; }
-
- auto* var = Var("a", ty.array<i32>(), ast::StorageClass::kNone);
-
- Func("func", ast::VariableList{}, ty.void_(),
- ast::StatementList{
- create<ast::VariableDeclStatement>(Source{Source::Location{12, 34}},
- var),
- },
- ast::DecorationList{
- create<ast::StageDecoration>(ast::PipelineStage::kVertex),
- });
-
- ValidatorImpl& v = Build();
-
- EXPECT_FALSE(v.Validate());
- EXPECT_EQ(v.error(),
- "12:34 v-0015: runtime arrays may only appear as the last member "
- "of a struct");
-}
-
-} // namespace
-} // namespace tint
diff --git a/src/writer/spirv/builder_block_test.cc b/src/writer/spirv/builder_block_test.cc
index 54f3e0a..c30ccae 100644
--- a/src/writer/spirv/builder_block_test.cc
+++ b/src/writer/spirv/builder_block_test.cc
@@ -22,7 +22,9 @@
using BuilderTest = TestHelper;
-TEST_F(BuilderTest, Block) {
+// TODO(amaiorano): Disabled because Resolver now emits "redeclared identifier
+// 'var'".
+TEST_F(BuilderTest, DISABLED_Block) {
// Note, this test uses shadow variables which aren't allowed in WGSL but
// serves to prove the block code is pushing new scopes as needed.
auto* inner = create<ast::BlockStatement>(ast::StatementList{
