From 5ffae32177e13661554320606583f4f25b5ba451 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 30 Sep 2021 17:29:50 +0000 Subject: [PATCH] validation: Improve continue-bypasses-decl message Attach the error to the continue statement, and add notes to show where the variable is both declared and used. Change-Id: Ie9939a5ca674e7216069bbb1d8dc82ab6949367c Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/65521 Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/resolver/resolver.cc | 21 ++++++----- src/resolver/validation_test.cc | 64 ++++++++++++++++++++------------- src/sem/block_statement.cc | 5 ++- src/sem/block_statement.h | 35 +++++++++++------- 4 files changed, 80 insertions(+), 45 deletions(-) diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index 14f5abbdae..6eb4470197 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -2043,15 +2043,15 @@ bool Resolver::Statement(ast::Statement* stmt) { } return true; } - if (stmt->Is()) { + if (auto* c = stmt->As()) { // Set if we've hit the first continue statement in our parent loop if (auto* block = current_block_->FindFirstParent< sem::LoopBlockStatement, sem::LoopContinuingBlockStatement>()) { if (auto* loop_block = block->As()) { - if (loop_block->FirstContinue() == size_t(~0)) { + if (!loop_block->FirstContinue()) { const_cast(loop_block) - ->SetFirstContinue(loop_block->Decls().size()); + ->SetFirstContinue(c, loop_block->Decls().size()); } } else { AddError("continuing blocks must not contain a continue statement", @@ -2980,7 +2980,7 @@ bool Resolver::Identifier(ast::IdentifierExpression* expr) { ->FindFirstParent()) { auto* loop_block = continuing_block->FindFirstParent(); - if (loop_block->FirstContinue() != size_t(~0)) { + if (loop_block->FirstContinue()) { auto& decls = loop_block->Decls(); // If our identifier is in loop_block->decls, make sure its index is // less than first_continue @@ -2990,11 +2990,16 @@ bool Resolver::Identifier(ast::IdentifierExpression* expr) { if (iter != decls.end()) { auto var_decl_index = static_cast(std::distance(decls.begin(), iter)); - if (var_decl_index >= loop_block->FirstContinue()) { + if (var_decl_index >= loop_block->NumDeclsAtFirstContinue()) { AddError("continue statement bypasses declaration of '" + - builder_->Symbols().NameFor(symbol) + - "' in continuing block", - expr->source()); + builder_->Symbols().NameFor(symbol) + "'", + loop_block->FirstContinue()->source()); + AddNote("identifier '" + builder_->Symbols().NameFor(symbol) + + "' declared here", + (*iter)->source()); + AddNote("identifier '" + builder_->Symbols().NameFor(symbol) + + "' referenced in continuing block here", + expr->source()); return false; } } diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc index 1ee8f6f460..1fe106a84b 100644 --- a/src/resolver/validation_test.cc +++ b/src/resolver/validation_test.cc @@ -572,17 +572,21 @@ TEST_F(ResolverValidationTest, // } // } - auto error_loc = Source{Source::Location{12, 34}}; - auto* body = Block(If(Expr(true), Block(create())), - Decl(Var("z", ty.i32(), ast::StorageClass::kNone))); - auto* continuing = Block(Assign(Expr(error_loc, "z"), 2)); + auto cont_loc = Source{Source::Location{12, 34}}; + auto decl_loc = Source{Source::Location{56, 78}}; + auto ref_loc = Source{Source::Location{90, 12}}; + auto* body = + Block(If(Expr(true), Block(create(cont_loc))), + Decl(Var(decl_loc, "z", ty.i32(), ast::StorageClass::kNone))); + auto* continuing = Block(Assign(Expr(ref_loc, "z"), 2)); auto* loop_stmt = Loop(body, continuing); WrapInFunction(loop_stmt); EXPECT_FALSE(r()->Resolve()) << r()->error(); EXPECT_EQ(r()->error(), - "12:34 error: continue statement bypasses declaration of 'z' in " - "continuing block"); + R"(12:34 error: continue statement bypasses declaration of 'z' +56:78 note: identifier 'z' declared here +90:12 note: identifier 'z' referenced in continuing block here)"); } TEST_F( @@ -600,19 +604,23 @@ TEST_F( // } // } - auto error_loc = Source{Source::Location{12, 34}}; - auto* body = Block(If(Expr(true), Block(create())), - Decl(Var("z", ty.i32(), ast::StorageClass::kNone))); + auto cont_loc = Source{Source::Location{12, 34}}; + auto decl_loc = Source{Source::Location{56, 78}}; + auto ref_loc = Source{Source::Location{90, 12}}; + auto* body = + Block(If(Expr(true), Block(create(cont_loc))), + Decl(Var(decl_loc, "z", ty.i32(), ast::StorageClass::kNone))); auto* continuing = - Block(If(Expr(true), Block(Assign(Expr(error_loc, "z"), 2)))); + Block(If(Expr(true), Block(Assign(Expr(ref_loc, "z"), 2)))); auto* loop_stmt = Loop(body, continuing); WrapInFunction(loop_stmt); EXPECT_FALSE(r()->Resolve()) << r()->error(); EXPECT_EQ(r()->error(), - "12:34 error: continue statement bypasses declaration of 'z' in " - "continuing block"); + R"(12:34 error: continue statement bypasses declaration of 'z' +56:78 note: identifier 'z' declared here +90:12 note: identifier 'z' referenced in continuing block here)"); } TEST_F(ResolverValidationTest, @@ -630,19 +638,23 @@ TEST_F(ResolverValidationTest, // } // } - auto error_loc = Source{Source::Location{12, 34}}; - auto* body = Block(If(Expr(true), Block(create())), - Decl(Var("z", ty.i32(), ast::StorageClass::kNone))); + auto cont_loc = Source{Source::Location{12, 34}}; + auto decl_loc = Source{Source::Location{56, 78}}; + auto ref_loc = Source{Source::Location{90, 12}}; + auto* body = + Block(If(Expr(true), Block(create(cont_loc))), + Decl(Var(decl_loc, "z", ty.i32(), ast::StorageClass::kNone))); auto* compare = create(ast::BinaryOp::kLessThan, - Expr(error_loc, "z"), Expr(2)); + Expr(ref_loc, "z"), Expr(2)); auto* continuing = Block(If(compare, Block())); auto* loop_stmt = Loop(body, continuing); WrapInFunction(loop_stmt); EXPECT_FALSE(r()->Resolve()) << r()->error(); EXPECT_EQ(r()->error(), - "12:34 error: continue statement bypasses declaration of 'z' in " - "continuing block"); + R"(12:34 error: continue statement bypasses declaration of 'z' +56:78 note: identifier 'z' declared here +90:12 note: identifier 'z' referenced in continuing block here)"); } TEST_F(ResolverValidationTest, @@ -659,18 +671,22 @@ TEST_F(ResolverValidationTest, // } // } - auto error_loc = Source{Source::Location{12, 34}}; - auto* body = Block(If(Expr(true), Block(create())), - Decl(Var("z", ty.i32(), ast::StorageClass::kNone))); + auto cont_loc = Source{Source::Location{12, 34}}; + auto decl_loc = Source{Source::Location{56, 78}}; + auto ref_loc = Source{Source::Location{90, 12}}; + auto* body = + Block(If(Expr(true), Block(create(cont_loc))), + Decl(Var(decl_loc, "z", ty.i32(), ast::StorageClass::kNone))); - auto* continuing = Block(Loop(Block(Assign(Expr(error_loc, "z"), 2)))); + auto* continuing = Block(Loop(Block(Assign(Expr(ref_loc, "z"), 2)))); auto* loop_stmt = Loop(body, continuing); WrapInFunction(loop_stmt); EXPECT_FALSE(r()->Resolve()) << r()->error(); EXPECT_EQ(r()->error(), - "12:34 error: continue statement bypasses declaration of 'z' in " - "continuing block"); + R"(12:34 error: continue statement bypasses declaration of 'z' +56:78 note: identifier 'z' declared here +90:12 note: identifier 'z' referenced in continuing block here)"); } TEST_F(ResolverValidationTest, diff --git a/src/sem/block_statement.cc b/src/sem/block_statement.cc index 614fd06b3a..70cdd0b79f 100644 --- a/src/sem/block_statement.cc +++ b/src/sem/block_statement.cc @@ -51,8 +51,11 @@ LoopBlockStatement::LoopBlockStatement(const ast::BlockStatement* declaration, } LoopBlockStatement::~LoopBlockStatement() = default; -void LoopBlockStatement::SetFirstContinue(size_t first_continue) { +void LoopBlockStatement::SetFirstContinue( + const ast::ContinueStatement* first_continue, + size_t num_decls) { first_continue_ = first_continue; + num_decls_at_first_continue_ = num_decls; } } // namespace sem diff --git a/src/sem/block_statement.h b/src/sem/block_statement.h index cc76148e5b..7a99feee5c 100644 --- a/src/sem/block_statement.h +++ b/src/sem/block_statement.h @@ -24,6 +24,7 @@ namespace tint { namespace ast { class BlockStatement; +class ContinueStatement; class Function; class Variable; } // namespace ast @@ -90,21 +91,31 @@ class LoopBlockStatement : public Castable { /// Destructor ~LoopBlockStatement() override; - /// @returns the index of the first variable declared after the first continue - /// statement - size_t FirstContinue() const { return first_continue_; } + /// @returns the first continue statement in this loop block, or nullptr if + /// there are no continue statements in the block + const ast::ContinueStatement* FirstContinue() const { + return first_continue_; + } - /// Requires that this is a loop block. - /// Allows the resolver to set the index of the first variable declared after - /// the first continue statement. - /// @param first_continue index of the relevant variable - void SetFirstContinue(size_t first_continue); + /// @returns the number of variables declared before the first continue + /// statement + size_t NumDeclsAtFirstContinue() const { + return num_decls_at_first_continue_; + } + + /// Allows the resolver to record the first continue statement in the block + /// and the number of variables declared prior to that statement. + /// @param first_continue the first continue statement in the block + /// @param num_decls the number of variable declarations before that continue + void SetFirstContinue(const ast::ContinueStatement* first_continue, + size_t num_decls); private: - // first_continue is set to the index of the first variable in decls - // declared after the first continue statement in a loop block, if any. - constexpr static size_t kNoContinue = size_t(~0); - size_t first_continue_ = kNoContinue; + /// The first continue statement in this loop block. + const ast::ContinueStatement* first_continue_ = nullptr; + + /// The number of variables declared before the first continue statement. + size_t num_decls_at_first_continue_ = 0; }; } // namespace sem