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>

src/resolver/resolver.cc

@@ -2043,15 +2043,15 @@ bool Resolver::Statement(ast::Statement* stmt) {
     }
     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 @@ bool Resolver::Identifier(ast::IdentifierExpression* expr) {
                   ->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 @@ bool Resolver::Identifier(ast::IdentifierExpression* expr) {
           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) +
+                           builder_->Symbols().NameFor(symbol) + "'",
-                           "' in continuing block",
+                       loop_block->FirstContinue()->source());
-                       expr->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;
             }
           }

src/resolver/validation_test.cc

@@ -572,17 +572,21 @@ TEST_F(ResolverValidationTest,
   //     }
   // }
 
-  auto error_loc = Source{Source::Location{12, 34}};
+  auto cont_loc = Source{Source::Location{12, 34}};
-  auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
+  auto decl_loc = Source{Source::Location{56, 78}};
-                     Decl(Var("z", ty.i32(), ast::StorageClass::kNone)));
+  auto ref_loc = Source{Source::Location{90, 12}};
-  auto* continuing = Block(Assign(Expr(error_loc, "z"), 2));
+  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 "
+            R"(12:34 error: continue statement bypasses declaration of 'z'
-            "continuing block");
+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 cont_loc = Source{Source::Location{12, 34}};
-  auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
+  auto decl_loc = Source{Source::Location{56, 78}};
-                     Decl(Var("z", ty.i32(), ast::StorageClass::kNone)));
+  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 "
+            R"(12:34 error: continue statement bypasses declaration of 'z'
-            "continuing block");
+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 cont_loc = Source{Source::Location{12, 34}};
-  auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
+  auto decl_loc = Source{Source::Location{56, 78}};
-                     Decl(Var("z", ty.i32(), ast::StorageClass::kNone)));
+  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 "
+            R"(12:34 error: continue statement bypasses declaration of 'z'
-            "continuing block");
+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 cont_loc = Source{Source::Location{12, 34}};
-  auto* body = Block(If(Expr(true), Block(create<ast::ContinueStatement>())),
+  auto decl_loc = Source{Source::Location{56, 78}};
-                     Decl(Var("z", ty.i32(), ast::StorageClass::kNone)));
+  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 "
+            R"(12:34 error: continue statement bypasses declaration of 'z'
-            "continuing block");
+56:78 note: identifier 'z' declared here
+90:12 note: identifier 'z' referenced in continuing block here)");
 }
 
 TEST_F(ResolverValidationTest,

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

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<LoopBlockStatement, BlockStatement> {
   /// Destructor
   ~LoopBlockStatement() override;
 
-  /// @returns the index of the first variable declared after the first continue
+  /// @returns the first continue statement in this loop block, or nullptr if
-  /// statement
+  /// there are no continue statements in the block
-  size_t FirstContinue() const { return first_continue_; }
+  const ast::ContinueStatement* FirstContinue() const {
+    return first_continue_;
+  }
 
-  /// Requires that this is a loop block.
+  /// @returns the number of variables declared before the first continue
-  /// Allows the resolver to set the index of the first variable declared after
+  /// statement
-  /// the first continue statement.
+  size_t NumDeclsAtFirstContinue() const {
-  /// @param first_continue index of the relevant variable
+    return num_decls_at_first_continue_;
-  void SetFirstContinue(size_t 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
+  /// The first continue statement in this loop block.
-  // declared after the first continue statement in a loop block, if any.
+  const ast::ContinueStatement* first_continue_ = nullptr;
-  constexpr static size_t kNoContinue = size_t(~0);
+
-  size_t first_continue_ = kNoContinue;
+  /// The number of variables declared before the first continue statement.
+  size_t num_decls_at_first_continue_ = 0;
 };
 
 }  // namespace sem