Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MutVisitor's default implementations to visit Stmt's and BinOp's spans #133784

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 3, 2024

The Stmt case is a bug introduced almost certainly unintentionally by #126993. The code used to visit and mutate span correctly, but got changed as follows by that PR. Notice how span is copied into the output by |kind| Stmt { id, kind, span } which happens after the mutation in the correct code (red) and before the mutation in the incorrect code (green).

  pub fn noop_flat_map_stmt<T: MutVisitor>(
      Stmt { kind, mut span, mut id }: Stmt,
      vis: &mut T,
  ) -> SmallVec<[Stmt; 1]> {
      vis.visit_id(&mut id);
-     vis.visit_span(&mut span);
      let stmts: SmallVec<_> = noop_flat_map_stmt_kind(kind, vis)
          .into_iter()
          .map(|kind| Stmt { id, kind, span })
          .collect();
      if stmts.len() > 1 {
          panic!(...);
      }
+     vis.visit_span(&mut span);
      stmts
  }

@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 3, 2024
Comment on lines 1631 to 1635
ExprKind::Binary(binop, lhs, rhs) => {
vis.visit_span(&mut binop.span);
vis.visit_expr(lhs);
vis.visit_expr(rhs);
}
Copy link

@tjallingt tjallingt Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ExprKind::Binary(binop, lhs, rhs) => {
vis.visit_span(&mut binop.span);
vis.visit_expr(lhs);
vis.visit_expr(rhs);
}
ExprKind::Binary(binop, lhs, rhs) => {
vis.visit_expr(lhs);
vis.visit_expr(rhs);
vis.visit_span(&mut binop.span);
}

To match the order defined by #126993

@compiler-errors
Copy link
Member

r? compiler-errors @bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2024

📌 Commit a3cfe2f has been approved by compiler-errors

It is now in the queue for this repository.

@rustbot rustbot assigned compiler-errors and unassigned chenyukang Dec 3, 2024
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#133651 (Update `NonZero` and `NonNull` to not field-project (per MCP#807))
 - rust-lang#133764 (rustdoc: Rename `set_back_info` to `restore_module_data`.)
 - rust-lang#133784 (Fix MutVisitor's default implementations to visit Stmt's and BinOp's spans)
 - rust-lang#133798 (stop replacing bivariant args with `'static` when computing closure requirements)
 - rust-lang#133804 (Improve code for FileName retrieval in rustdoc)
 - rust-lang#133817 (Use `eprintln` instead of `println` in bootstrap/compiletest/tidy)

Failed merges:

 - rust-lang#133810 (remove unnecessary `eval_verify_bound`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45088fd into rust-lang:master Dec 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
Rollup merge of rust-lang#133784 - dtolnay:visitspans, r=compiler-errors

Fix MutVisitor's default implementations to visit Stmt's and BinOp's spans

The `Stmt` case is a bug introduced almost certainly unintentionally by rust-lang#126993. The code _used_ to visit and mutate `span` correctly, but got changed as follows by that PR. Notice how `span` is **copied** into the output by `|kind| Stmt { id, kind, span }` which happens after the mutation in the correct code (red) and before the mutation in the incorrect code (green).

```diff
  pub fn noop_flat_map_stmt<T: MutVisitor>(
      Stmt { kind, mut span, mut id }: Stmt,
      vis: &mut T,
  ) -> SmallVec<[Stmt; 1]> {
      vis.visit_id(&mut id);
-     vis.visit_span(&mut span);
      let stmts: SmallVec<_> = noop_flat_map_stmt_kind(kind, vis)
          .into_iter()
          .map(|kind| Stmt { id, kind, span })
          .collect();
      if stmts.len() > 1 {
          panic!(...);
      }
+     vis.visit_span(&mut span);
      stmts
  }
```
@dtolnay dtolnay deleted the visitspans branch December 22, 2024 17:02
@dtolnay dtolnay removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants