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 <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 14f5abb..6eb4470 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -2043,15 +2043,15 @@
}
return true;
}
- if (stmt->Is<ast::ContinueStatement>()) {
+ if (auto* c = stmt->As<ast::ContinueStatement>()) {
// 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<sem::LoopBlockStatement>()) {
- if (loop_block->FirstContinue() == size_t(~0)) {
+ if (!loop_block->FirstContinue()) {
const_cast<sem::LoopBlockStatement*>(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 @@
->FindFirstParent<sem::LoopContinuingBlockStatement>()) {
auto* loop_block =
continuing_block->FindFirstParent<sem::LoopBlockStatement>();
- 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 @@
if (iter != decls.end()) {
auto var_decl_index =
static_cast<size_t>(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 1ee8f6f..1fe106a 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc
@@ -572,17 +572,21 @@
// }
// }
- auto error_loc = Source{Source::Location{12, 34}};
- auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
- 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<ast::ContinueStatement>(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 @@
// }
// }
- auto error_loc = Source{Source::Location{12, 34}};
- auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
- 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<ast::ContinueStatement>(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 @@
// }
// }
- auto error_loc = Source{Source::Location{12, 34}};
- auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
- 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<ast::ContinueStatement>(cont_loc))),
+ Decl(Var(decl_loc, "z", ty.i32(), ast::StorageClass::kNone)));
auto* compare = create<ast::BinaryExpression>(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 @@
// }
// }
- auto error_loc = Source{Source::Location{12, 34}};
- auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
- 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<ast::ContinueStatement>(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 614fd06..70cdd0b 100644
--- a/src/sem/block_statement.cc
+++ b/src/sem/block_statement.cc
@@ -51,8 +51,11 @@
}
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 cc76148..7a99fee 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 @@
/// 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
