ast: Standardize visiting order#126993
Conversation
Id, attributes, inner nodes in source order if possible, tokens, span. Also always use exhaustive matching in visiting infra, and visit some missing nodes.
rustbot
added
S-waiting-on-review
|
Changes to the size of AST and/or HIR nodes. cc @nnethercote |
| ast-stats-1 - Struct 72 ( 1.1%) 1 | ||
| ast-stats-1 - Lit 144 ( 2.2%) 2 | ||
| ast-stats-1 - Block 216 ( 3.3%) 3 | ||
| ast-stats-1 PathSegment 720 (10.9%) 30 24 |
There was a problem hiding this comment.
PathSegment hasn't changed in size. Something about the visitor must have changed, but I can't see what. Do you know?
There was a problem hiding this comment.
This comment is relevant, it describes how the AST node measurements aren't perfect:
rust/compiler/rustc_passes/src/hir_stats.rs
Lines 47 to 63 in 4bc39f0
There was a problem hiding this comment.
Paths in attributes are now visited.
|
@bors r+ rollup=never cool :) visitor boilerplate is always kinda sketch when it just uses field accesses instead of exhaustively destructuring |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (036b38c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 696.635s -> 697.296s (0.09%) |
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 } ```
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 } ```
6 participants
Order: ID, attributes, inner nodes in source order if possible, tokens, span.
Also always use exhaustive matching in visiting infra, and visit some discovered missing nodes.
Unlike #125741 this shouldn't affect anything serious like
macro_rulesscopes.