From 65f88d6f1d347d6a2b10ec434f51def562bc3a2f Mon Sep 17 00:00:00 2001 From: Sarah Mashayekhi Date: Thu, 6 Aug 2020 21:24:14 +0000 Subject: [PATCH] [Validation] v-0006: variables must be defined before use Bug:tint 6 Change-Id: I22f3117a8d59eaba97166de1f188156a9e3cd7a0 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/26381 Reviewed-by: David Neto Commit-Queue: Sarah Mashayekhi --- src/validator_impl.cc | 50 +++- src/validator_impl.h | 13 + src/validator_test.cc | 281 ++++++++++++++++++++-- test/undefined-variable-global-scope.wgsl | 23 ++ test/undefined-variable-inner-block.wgsl | 20 ++ 5 files changed, 370 insertions(+), 17 deletions(-) create mode 100644 test/undefined-variable-global-scope.wgsl create mode 100644 test/undefined-variable-inner-block.wgsl diff --git a/src/validator_impl.cc b/src/validator_impl.cc index 923ffa86c9..ae7bbef031 100644 --- a/src/validator_impl.cc +++ b/src/validator_impl.cc @@ -66,6 +66,9 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) { } bool ValidatorImpl::ValidateStatements(const ast::BlockStatement* block) { + if (!block) { + return false; + } for (const auto& stmt : *block) { // TODO(sarahM0): move the folowing to a function if (stmt->IsVariableDecl()) { @@ -80,13 +83,35 @@ bool ValidatorImpl::ValidateStatements(const ast::BlockStatement* block) { } bool ValidatorImpl::ValidateStatement(const ast::Statement* stmt) { - if (stmt->IsAssign() && !ValidateAssign(stmt->AsAssign())) + if (!stmt) { return false; - + } + if (stmt->IsAssign()) { + return ValidateAssign(stmt->AsAssign()); + } return true; } bool ValidatorImpl::ValidateAssign(const ast::AssignmentStatement* a) { + if (!a) { + return false; + } + if (!(ValidateExpression(a->lhs()) && ValidateExpression(a->rhs()))) { + return false; + } + if (!ValidateResultTypes(a)) { + return false; + } + + return true; +} + +bool ValidatorImpl::ValidateResultTypes(const ast::AssignmentStatement* a) { + if (!a->lhs()->result_type() || !a->rhs()->result_type()) { + set_error(a->source(), "result_type() is nullptr"); + return false; + } + auto* lhs_result_type = a->lhs()->result_type()->UnwrapAliasPtrAlias(); auto* rhs_result_type = a->rhs()->result_type()->UnwrapAliasPtrAlias(); if (lhs_result_type != rhs_result_type) { @@ -99,6 +124,27 @@ bool ValidatorImpl::ValidateAssign(const ast::AssignmentStatement* a) { return true; } +bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) { + if (!expr) { + return false; + } + if (expr->IsIdentifier()) { + return ValidateIdentifier(expr->AsIdentifier()); + } + + return true; +} + +bool ValidatorImpl::ValidateIdentifier(const ast::IdentifierExpression* ident) { + ast::Variable* var; + if (!variable_stack_.get(ident->name(), &var)) { + set_error(ident->source(), + "v-0006: '" + ident->name() + "' is not declared"); + return false; + } + return true; +} + bool ValidatorImpl::CheckImports(const ast::Module* module) { for (const auto& import : module->imports()) { if (import->path() != "GLSL.std.450") { diff --git a/src/validator_impl.h b/src/validator_impl.h index d4f4534600..1fa0205f2f 100644 --- a/src/validator_impl.h +++ b/src/validator_impl.h @@ -19,6 +19,7 @@ #include "src/ast/assignment_statement.h" #include "src/ast/expression.h" +#include "src/ast/identifier_expression.h" #include "src/ast/module.h" #include "src/ast/statement.h" #include "src/ast/variable.h" @@ -72,6 +73,18 @@ class ValidatorImpl { /// @param module the modele to check imports /// @returns ture if input complies with v-0001 rule bool CheckImports(const ast::Module* module); + /// Validates an expression + /// @param expr the expression to check + /// @return true if the expresssion 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 if the input follows type checking rules + /// @param 'a' the assignment to check + /// @returns ture if successful + bool ValidateResultTypes(const ast::AssignmentStatement* a); private: std::string error_; diff --git a/src/validator_test.cc b/src/validator_test.cc index 994db44a32..fe485fc529 100644 --- a/src/validator_test.cc +++ b/src/validator_test.cc @@ -20,6 +20,7 @@ #include "src/ast/as_expression.h" #include "src/ast/assignment_statement.h" #include "src/ast/binary_expression.h" +#include "src/ast/bool_literal.h" #include "src/ast/break_statement.h" #include "src/ast/call_expression.h" #include "src/ast/case_statement.h" @@ -48,6 +49,8 @@ #include "src/ast/type/struct_type.h" #include "src/ast/type/vector_type.h" #include "src/ast/type_constructor_expression.h" +#include "src/ast/variable.h" +#include "src/ast/variable_decl_statement.h" #include "src/type_determiner.h" namespace tint { @@ -59,6 +62,7 @@ class TypeDeterminerHelper { : td_(std::make_unique(&ctx_, &mod_)) {} TypeDeterminer* td() const { return td_.get(); } + ast::Module* mod() { return &mod_; } private: Context ctx_; @@ -105,7 +109,7 @@ TEST_F(ValidatorTest, DISABLED_AssignToScalar_Fail) { auto lhs = std::make_unique( std::make_unique(&i32, 1)); auto rhs = std::make_unique("my_var"); - ast::AssignmentStatement assign(Source{12, 32}, std::move(lhs), + ast::AssignmentStatement assign(Source{12, 34}, std::move(lhs), std::move(rhs)); tint::ValidatorImpl v; @@ -115,13 +119,79 @@ TEST_F(ValidatorTest, DISABLED_AssignToScalar_Fail) { EXPECT_EQ(v.error(), "12:34: v-000x: invalid assignment"); } +TEST_F(ValidatorTest, UsingUndefinedVariable_Fail) { + // b = 2; + ast::type::I32Type i32; + + auto lhs = std::make_unique(Source{12, 34}, "b"); + auto rhs = std::make_unique( + std::make_unique(&i32, 2)); + auto assign = std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs)); + + EXPECT_TRUE(td()->DetermineResultType(assign.get())) << td()->error(); + tint::ValidatorImpl v; + EXPECT_FALSE(v.ValidateStatement(assign.get())); + EXPECT_EQ(v.error(), "12:34: v-0006: 'b' is not declared"); +} + +TEST_F(ValidatorTest, UsingUndefinedVariableInBlockStatement_Fail) { + // { + // b = 2; + // } + ast::type::I32Type i32; + + auto lhs = std::make_unique(Source{12, 34}, "b"); + auto rhs = std::make_unique( + std::make_unique(&i32, 2)); + + auto body = std::make_unique(); + body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + + EXPECT_TRUE(td()->DetermineStatements(body.get())) << td()->error(); + tint::ValidatorImpl v; + EXPECT_FALSE(v.ValidateStatements(body.get())); + EXPECT_EQ(v.error(), "12:34: v-0006: 'b' is not declared"); +} + +TEST_F(ValidatorTest, AssignCompatibleTypes_Pass) { + // var a :i32 = 2; + // a = 2 + ast::type::I32Type i32; + auto var = + std::make_unique("a", ast::StorageClass::kNone, &i32); + var->set_constructor(std::make_unique( + std::make_unique(&i32, 2))); + + auto lhs = std::make_unique("a"); + auto* lhs_ptr = lhs.get(); + auto rhs = std::make_unique( + std::make_unique(&i32, 2)); + auto* rhs_ptr = rhs.get(); + + ast::AssignmentStatement assign(Source{12, 34}, std::move(lhs), + std::move(rhs)); + td()->RegisterVariableForTesting(var.get()); + EXPECT_TRUE(td()->DetermineResultType(&assign)) << td()->error(); + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); + tint::ValidatorImpl v; + EXPECT_TRUE(v.ValidateResultTypes(&assign)); +} + TEST_F(ValidatorTest, AssignIncompatibleTypes_Fail) { - // var a :i32; - // a = 2.3; + // { + // var a :i32 = 2; + // a = 2.3; + // } ast::type::F32Type f32; ast::type::I32Type i32; - ast::Variable var("a", ast::StorageClass::kPrivate, &i32); + auto var = + std::make_unique("a", ast::StorageClass::kNone, &i32); + var->set_constructor(std::make_unique( + std::make_unique(&i32, 2))); auto lhs = std::make_unique("a"); auto* lhs_ptr = lhs.get(); auto rhs = std::make_unique( @@ -130,36 +200,217 @@ TEST_F(ValidatorTest, AssignIncompatibleTypes_Fail) { ast::AssignmentStatement assign(Source{12, 34}, std::move(lhs), std::move(rhs)); - td()->RegisterVariableForTesting(&var); + td()->RegisterVariableForTesting(var.get()); EXPECT_TRUE(td()->DetermineResultType(&assign)) << td()->error(); ASSERT_NE(lhs_ptr->result_type(), nullptr); ASSERT_NE(rhs_ptr->result_type(), nullptr); tint::ValidatorImpl v; - EXPECT_FALSE(v.ValidateAssign(&assign)); + EXPECT_FALSE(v.ValidateResultTypes(&assign)); ASSERT_TRUE(v.has_error()); // TODO(sarahM0): figure out what should be the error number. EXPECT_EQ(v.error(), "12:34: v-000x: invalid assignment of '__i32' to '__f32'"); } -TEST_F(ValidatorTest, AssignCompatibleTypes_Pass) { - // var a :i32; - // a = 2; +TEST_F(ValidatorTest, AssignCompatibleTypesInBlockStatement_Pass) { + // { + // var a :i32 = 2; + // a = 2 + // } ast::type::I32Type i32; + auto var = + std::make_unique("a", ast::StorageClass::kNone, &i32); + var->set_constructor(std::make_unique( + std::make_unique(&i32, 2))); - ast::Variable var("a", ast::StorageClass::kPrivate, &i32); auto lhs = std::make_unique("a"); + auto* lhs_ptr = lhs.get(); auto rhs = std::make_unique( std::make_unique(&i32, 2)); + auto* rhs_ptr = rhs.get(); - ast::AssignmentStatement assign(Source{12, 34}, std::move(lhs), - std::move(rhs)); + auto body = std::make_unique(); + body->append(std::make_unique(std::move(var))); + body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + + EXPECT_TRUE(td()->DetermineStatements(body.get())) << td()->error(); + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); - td()->RegisterVariableForTesting(&var); - EXPECT_TRUE(td()->DetermineResultType(&assign)) << td()->error(); tint::ValidatorImpl v; - EXPECT_TRUE(v.ValidateAssign(&assign)); + EXPECT_TRUE(v.ValidateStatements(body.get())); +} + +TEST_F(ValidatorTest, AssignIncompatibleTypesInBlockStatement_Fail) { + // { + // var a :i32 = 2; + // a = 2.3; + // } + ast::type::F32Type f32; + ast::type::I32Type i32; + + auto var = + std::make_unique("a", ast::StorageClass::kNone, &i32); + var->set_constructor(std::make_unique( + std::make_unique(&i32, 2))); + auto lhs = std::make_unique("a"); + auto* lhs_ptr = lhs.get(); + auto rhs = std::make_unique( + std::make_unique(&f32, 2.3f)); + auto* rhs_ptr = rhs.get(); + + ast::BlockStatement block; + block.append(std::make_unique(std::move(var))); + block.append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + + EXPECT_TRUE(td()->DetermineStatements(&block)) << td()->error(); + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); + + tint::ValidatorImpl v; + EXPECT_FALSE(v.ValidateStatements(&block)); + ASSERT_TRUE(v.has_error()); + // TODO(sarahM0): figure out what should be the error number. + EXPECT_EQ(v.error(), + "12:34: v-000x: invalid assignment of '__i32' to '__f32'"); +} + +TEST_F(ValidatorTest, UnsingUndefinedVariableGlobalVariable_Fail) { + ast::type::F32Type f32; + auto global_var = std::make_unique( + "global_var", ast::StorageClass::kPrivate, &f32); + global_var->set_constructor( + std::make_unique( + std::make_unique(&f32, 2.1))); + mod()->AddGlobalVariable(std::move(global_var)); + + auto lhs = std::make_unique(Source{12, 34}, + "not_global_var"); + auto rhs = std::make_unique( + std::make_unique(&f32, 3.14f)); + + ast::VariableList params; + auto func = + std::make_unique("my_func", std::move(params), &f32); + + auto body = std::make_unique(); + body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + func->set_body(std::move(body)); + mod()->AddFunction(std::move(func)); + + tint::ValidatorImpl v; + EXPECT_FALSE(v.Validate(mod())); + EXPECT_EQ(v.error(), "12:34: v-0006: 'not_global_var' is not declared"); +} + +TEST_F(ValidatorTest, UnsingUndefinedVariableGlobalVariable_Pass) { + ast::type::F32Type f32; + auto global_var = std::make_unique( + "global_var", ast::StorageClass::kPrivate, &f32); + global_var->set_constructor( + std::make_unique( + std::make_unique(&f32, 2.1))); + mod()->AddGlobalVariable(std::move(global_var)); + + auto lhs = std::make_unique("global_var"); + auto* lhs_ptr = lhs.get(); + auto rhs = std::make_unique( + std::make_unique(&f32, 3.14f)); + auto* rhs_ptr = rhs.get(); + + ast::VariableList params; + auto func = + std::make_unique("my_func", std::move(params), &f32); + + auto body = std::make_unique(); + body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + func->set_body(std::move(body)); + auto* func_ptr = func.get(); + mod()->AddFunction(std::move(func)); + + EXPECT_TRUE(td()->Determine()) << td()->error(); + EXPECT_TRUE(td()->DetermineFunction(func_ptr)) << td()->error(); + tint::ValidatorImpl v; + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); + EXPECT_TRUE(v.Validate(mod())) << v.error(); +} + +TEST_F(ValidatorTest, UnsingUndefinedVariableInnerScope_Fail) { + // { + // if (true) { var a : f32 = 2.0; } + // a = 3.14; + // } + ast::type::F32Type f32; + auto var = + std::make_unique("a", ast::StorageClass::kNone, &f32); + var->set_constructor(std::make_unique( + std::make_unique(&f32, 2.0))); + + ast::type::BoolType bool_type; + auto cond = std::make_unique( + std::make_unique(&bool_type, true)); + auto body = std::make_unique(); + body->append(std::make_unique(std::move(var))); + + auto lhs = std::make_unique(Source{12, 34}, "a"); + auto* lhs_ptr = lhs.get(); + auto rhs = std::make_unique( + std::make_unique(&f32, 3.14f)); + auto* rhs_ptr = rhs.get(); + + auto outer_body = std::make_unique(); + outer_body->append( + std::make_unique(std::move(cond), std::move(body))); + outer_body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + + EXPECT_TRUE(td()->DetermineStatements(outer_body.get())) << td()->error(); + tint::ValidatorImpl v; + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); + EXPECT_FALSE(v.ValidateStatements(outer_body.get())); + EXPECT_EQ(v.error(), "12:34: v-0006: 'a' is not declared"); +} + +TEST_F(ValidatorTest, UnsingUndefinedVariableOuterScope_Pass) { + // { + // var a : f32 = 2.0; + // if (true) { a = 3.14; } + // } + ast::type::F32Type f32; + auto var = + std::make_unique("a", ast::StorageClass::kNone, &f32); + var->set_constructor(std::make_unique( + std::make_unique(&f32, 2.0))); + + auto lhs = std::make_unique(Source{12, 34}, "a"); + auto* lhs_ptr = lhs.get(); + auto rhs = std::make_unique( + std::make_unique(&f32, 3.14f)); + auto* rhs_ptr = rhs.get(); + ast::type::BoolType bool_type; + auto cond = std::make_unique( + std::make_unique(&bool_type, true)); + auto body = std::make_unique(); + body->append(std::make_unique( + Source{12, 34}, std::move(lhs), std::move(rhs))); + + auto outer_body = std::make_unique(); + outer_body->append( + std::make_unique(std::move(var))); + outer_body->append( + std::make_unique(std::move(cond), std::move(body))); + EXPECT_TRUE(td()->DetermineStatements(outer_body.get())) << td()->error(); + tint::ValidatorImpl v; + ASSERT_NE(lhs_ptr->result_type(), nullptr); + ASSERT_NE(rhs_ptr->result_type(), nullptr); + EXPECT_TRUE(v.ValidateStatements(outer_body.get())) << v.error(); } TEST_F(ValidatorTest, DISABLED_AssignToConstant_Fail) { diff --git a/test/undefined-variable-global-scope.wgsl b/test/undefined-variable-global-scope.wgsl new file mode 100644 index 0000000000..5fa9fb2314 --- /dev/null +++ b/test/undefined-variable-global-scope.wgsl @@ -0,0 +1,23 @@ +# 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. + +# Vertex shader +var a : i32; + +fn main() -> void { + a = 2; + return; +} +entry_point vertex as "main" = main; + diff --git a/test/undefined-variable-inner-block.wgsl b/test/undefined-variable-inner-block.wgsl new file mode 100644 index 0000000000..06fc420004 --- /dev/null +++ b/test/undefined-variable-inner-block.wgsl @@ -0,0 +1,20 @@ +# 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. + +fn main() -> void { + var a : f32 = 2.0; + { a = 3.14;} + return; +} +entry_point fragment = main;