From 4682e3fc3132a36eb0a33a21bf2db50e20771e5e Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Mon, 22 Mar 2021 17:38:45 +0000 Subject: [PATCH] Move identifier validation from Validator to Resolver This was mostly already implemented in the Resolver, except for adding a variable scope for blocks. Moved tests and improved them to only add Source on the error node. Bug: tint:642 Change-Id: I175dd22c873df5933133bc92276101aeab3021ed Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45460 Commit-Queue: Antonio Maiorano Reviewed-by: Ben Clayton --- src/resolver/resolver.cc | 5 +- src/resolver/validation_test.cc | 133 +++++++++++++++++++++++++++- src/validator/validator_impl.cc | 23 +---- src/validator/validator_impl.h | 4 - src/validator/validator_test.cc | 149 -------------------------------- 5 files changed, 137 insertions(+), 177 deletions(-) diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 00cf333573..3b43fc1f68 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -1726,7 +1726,10 @@ template bool Resolver::BlockScope(BlockInfo::Type type, F&& callback) { BlockInfo block_info(type, current_block_); ScopedAssignment sa(current_block_, &block_info); - return callback(); + variable_stack_.push_scope(); + bool result = callback(); + variable_stack_.pop_scope(); + return result; } std::string Resolver::VectorPretty(uint32_t size, type::Type* element_type) { diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc index f9366e3dbc..30c5ba96f0 100644 --- a/src/resolver/validation_test.cc +++ b/src/resolver/validation_test.cc @@ -211,8 +211,7 @@ TEST_F(ResolverValidationTest, Expr_DontCall_Intrinsic) { TEST_F(ResolverValidationTest, UsingUndefinedVariable_Fail) { // b = 2; - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("b"); + auto* lhs = Expr(Source{{12, 34}}, "b"); auto* rhs = Expr(2); auto* assign = create(lhs, rhs); WrapInFunction(assign); @@ -227,8 +226,7 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableInBlockStatement_Fail) { // b = 2; // } - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("b"); + auto* lhs = Expr(Source{{12, 34}}, "b"); auto* rhs = Expr(2); auto* body = create(ast::StatementList{ @@ -241,6 +239,133 @@ TEST_F(ResolverValidationTest, UsingUndefinedVariableInBlockStatement_Fail) { "12:34 error: v-0006: identifier must be declared before use: b"); } +TEST_F(ResolverValidationTest, UsingUndefinedVariableGlobalVariableAfter_Fail) { + // fn my_func() -> void { + // global_var = 3.14f; + // } + // var global_var: f32 = 2.1; + + auto* lhs = Expr(Source{{12, 34}}, "global_var"); + auto* rhs = Expr(3.14f); + + Func("my_func", ast::VariableList{}, ty.void_(), + ast::StatementList{ + create(lhs, rhs), + }, + ast::DecorationList{ + create(ast::PipelineStage::kVertex)}); + + Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: v-0006: identifier must be declared before use: " + "global_var"); +} + +TEST_F(ResolverValidationTest, UsingUndefinedVariableGlobalVariable_Pass) { + // var global_var: f32 = 2.1; + // fn my_func() -> void { + // global_var = 3.14; + // return; + // } + + Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f)); + + Func("my_func", ast::VariableList{}, ty.void_(), + ast::StatementList{ + create(Source{Source::Location{12, 34}}, + Expr("global_var"), Expr(3.14f)), + create(), + }, + ast::DecorationList{ + create(ast::PipelineStage::kVertex), + }); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(ResolverValidationTest, UsingUndefinedVariableInnerScope_Fail) { + // { + // if (true) { var a : f32 = 2.0; } + // a = 3.14; + // } + auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); + + auto* cond = Expr(true); + auto* body = create(ast::StatementList{ + create(var), + }); + + SetSource(Source{Source::Location{12, 34}}); + auto* lhs = Expr(Source{{12, 34}}, "a"); + auto* rhs = Expr(3.14f); + + auto* outer_body = create(ast::StatementList{ + create(cond, body, ast::ElseStatementList{}), + create(lhs, rhs), + }); + + WrapInFunction(outer_body); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: v-0006: identifier must be declared before use: a"); +} + +TEST_F(ResolverValidationTest, UsingUndefinedVariableOuterScope_Pass) { + // { + // var a : f32 = 2.0; + // if (true) { a = 3.14; } + // } + auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); + + auto* lhs = Expr(Source{{12, 34}}, "a"); + auto* rhs = Expr(3.14f); + + auto* cond = Expr(true); + auto* body = create(ast::StatementList{ + create(lhs, rhs), + }); + + auto* outer_body = create(ast::StatementList{ + create(var), + create(cond, body, ast::ElseStatementList{}), + }); + + WrapInFunction(outer_body); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(ResolverValidationTest, UsingUndefinedVariableDifferentScope_Fail) { + // { + // { var a : f32 = 2.0; } + // { a = 3.14; } + // } + auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); + auto* first_body = create(ast::StatementList{ + create(var), + }); + + auto* lhs = Expr(Source{{12, 34}}, "a"); + auto* rhs = Expr(3.14f); + auto* second_body = create(ast::StatementList{ + create(lhs, rhs), + }); + + auto* outer_body = create(ast::StatementList{ + first_body, + second_body, + }); + + WrapInFunction(outer_body); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + "12:34 error: v-0006: identifier must be declared before use: a"); +} + TEST_F(ResolverValidationTest, StorageClass_NonFunctionClassError) { auto* var = Var("var", ty.i32(), ast::StorageClass::kWorkgroup); diff --git a/src/validator/validator_impl.cc b/src/validator/validator_impl.cc index 05e7f9e585..021456cb02 100644 --- a/src/validator/validator_impl.cc +++ b/src/validator/validator_impl.cc @@ -411,11 +411,10 @@ bool ValidatorImpl::ValidateBadAssignmentToIdentifier( return false; } } else { - // The identifier is not defined. This should already have been caught - // when validating the subexpression. - add_error(ident->source(), "v-0006", - "'" + program_->Symbols().NameFor(ident->symbol()) + - "' is not declared"); + // The identifier is not defined. + // Shouldn't reach here. This should already have been caught when + // validation expressions in the Resolver + TINT_UNREACHABLE(diagnostics()); return false; } return true; @@ -463,9 +462,6 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) { if (!expr) { return false; } - if (auto* i = expr->As()) { - return ValidateIdentifier(i); - } if (auto* c = expr->As()) { return ValidateCallExpr(c); @@ -473,17 +469,6 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) { return true; } -bool ValidatorImpl::ValidateIdentifier(const ast::IdentifierExpression* ident) { - const ast::Variable* var; - if (!variable_stack_.get(ident->symbol(), &var)) { - add_error(ident->source(), "v-0006", - "'" + program_->Symbols().NameFor(ident->symbol()) + - "' is not declared"); - return false; - } - return true; -} - bool ValidatorImpl::IsStorable(type::Type* type) { if (type == nullptr) { return false; diff --git a/src/validator/validator_impl.h b/src/validator/validator_impl.h index 93c1d8db20..102fb4bb7c 100644 --- a/src/validator/validator_impl.h +++ b/src/validator/validator_impl.h @@ -98,10 +98,6 @@ class ValidatorImpl { /// @param expr the expression to check /// @return true if the expression is valid bool ValidateExpression(const ast::Expression* expr); - /// Validates v-0006:Variables must be defined before use - /// @param ident the identifer to check if its in the scope - /// @return true if idnet was defined - bool ValidateIdentifier(const ast::IdentifierExpression* ident); /// Validates declaration name uniqueness /// @param decl is the new declaration to be added /// @returns true if no previous declaration with the `decl` 's name diff --git a/src/validator/validator_test.cc b/src/validator/validator_test.cc index 84ce862486..b5d8dba6ed 100644 --- a/src/validator/validator_test.cc +++ b/src/validator/validator_test.cc @@ -185,155 +185,6 @@ TEST_F(ValidatorTest, GlobalConstNoStorageClass_Pass) { EXPECT_FALSE(v.Validate()) << v.error(); } -TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariableAfter_Fail) { - // fn my_func() -> void { - // global_var = 3.14f; - // } - // var global_var: f32 = 2.1; - - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("global_var"); - auto* rhs = Expr(3.14f); - - Func("my_func", ast::VariableList{}, ty.void_(), - ast::StatementList{ - create(lhs, rhs), - }, - ast::DecorationList{ - create(ast::PipelineStage::kVertex)}); - - Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f)); - - // TODO(amaiorano): Move to resolver tests. Program is invalid now because - // Resolver catches this. ValidatorImpl& v = Build(); - - // EXPECT_FALSE(v.Validate()); - // EXPECT_EQ(v.error(), "12:34 v-0006: 'global_var' is not declared"); -} - -TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Pass) { - // var global_var: f32 = 2.1; - // fn my_func() -> void { - // global_var = 3.14; - // return; - // } - - Global("global_var", ty.f32(), ast::StorageClass::kPrivate, Expr(2.1f)); - - Func("my_func", ast::VariableList{}, ty.void_(), - ast::StatementList{ - create(Source{Source::Location{12, 34}}, - Expr("global_var"), Expr(3.14f)), - create(), - }, - ast::DecorationList{ - create(ast::PipelineStage::kVertex), - }); - - ValidatorImpl& v = Build(); - - EXPECT_TRUE(v.Validate()) << v.error(); -} - -TEST_F(ValidatorTest, UsingUndefinedVariableInnerScope_Fail) { - // { - // if (true) { var a : f32 = 2.0; } - // a = 3.14; - // } - auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); - - auto* cond = Expr(true); - auto* body = create(ast::StatementList{ - create(var), - }); - - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("a"); - auto* rhs = Expr(3.14f); - - auto* outer_body = create(ast::StatementList{ - create(cond, body, ast::ElseStatementList{}), - create(Source{Source::Location{12, 34}}, lhs, - rhs), - }); - - WrapInFunction(outer_body); - - ValidatorImpl& v = Build(); - - ASSERT_NE(TypeOf(lhs), nullptr); - ASSERT_NE(TypeOf(rhs), nullptr); - - EXPECT_FALSE(v.ValidateStatements(outer_body)); - EXPECT_EQ(v.error(), "12:34 v-0006: 'a' is not declared"); -} - -TEST_F(ValidatorTest, UsingUndefinedVariableOuterScope_Pass) { - // { - // var a : f32 = 2.0; - // if (true) { a = 3.14; } - // } - auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); - - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("a"); - auto* rhs = Expr(3.14f); - - auto* cond = Expr(true); - auto* body = create(ast::StatementList{ - create(Source{Source::Location{12, 34}}, lhs, - rhs), - }); - - auto* outer_body = create(ast::StatementList{ - create(var), - create(cond, body, ast::ElseStatementList{}), - }); - - WrapInFunction(outer_body); - - ValidatorImpl& v = Build(); - - ASSERT_NE(TypeOf(lhs), nullptr); - ASSERT_NE(TypeOf(rhs), nullptr); - - EXPECT_TRUE(v.ValidateStatements(outer_body)) << v.error(); -} - -TEST_F(ValidatorTest, UsingUndefinedVariableDifferentScope_Fail) { - // { - // { var a : f32 = 2.0; } - // { a = 3.14; } - // } - auto* var = Var("a", ty.f32(), ast::StorageClass::kNone, Expr(2.0f)); - auto* first_body = create(ast::StatementList{ - create(var), - }); - - SetSource(Source{Source::Location{12, 34}}); - auto* lhs = Expr("a"); - auto* rhs = Expr(3.14f); - auto* second_body = create(ast::StatementList{ - create(Source{Source::Location{12, 34}}, lhs, - rhs), - }); - - auto* outer_body = create(ast::StatementList{ - first_body, - second_body, - }); - - WrapInFunction(outer_body); - - ValidatorImpl& v = Build(); - - ASSERT_NE(TypeOf(lhs), nullptr); - ASSERT_NE(TypeOf(rhs), nullptr); - - EXPECT_FALSE(v.ValidateStatements(outer_body)); - EXPECT_EQ(v.error(), "12:34 v-0006: 'a' is not declared"); -} - TEST_F(ValidatorTest, GlobalVariableUnique_Pass) { // var global_var0 : f32 = 0.1; // var global_var1 : i32 = 0;