Make ast `MutVisitor` have the same method name and style as `Visitor` #127524

oli-obk · 2024-07-09T16:19:30Z

It doesn't map 100% because some MutVisitor methods can filter or even expand to multiple items, but consistency seems nicer.

tracking issue: #127615

rustbot · 2024-07-09T16:19:40Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

fmease · 2024-07-09T17:30:00Z

This allows ast lowering to just assume things are error or valid, and avoids having to redo some checks, delaying bugs or checking the global error counter.

That would be wonderful!

compiler/rustc_ast/src/mut_visit.rs

cjgillot · 2024-07-13T11:59:16Z

@bors try @rust-timer queue

Make ast `MutVisitor` have the same method name and style as `Visitor` It doesn't map 100% because some `MutVisitor` methods can filter or even expand to multiple items, but consistency seems nicer. The last commit showcases how similar they are by changing ast validation to a `MutVisitor` (without actually doing any mutation yet). My plan is to replace all nodes that support it with error nodes if validation failed on them. This allows ast lowering to just assume things are error or valid, and avoids having to redo some checks, delaying bugs or checking the global error counter. tracking issue: rust-lang#127615

bors · 2024-07-13T12:00:28Z

⌛ Trying commit e89df61 with merge abb4640...

bors · 2024-07-13T13:50:14Z

☀️ Try build successful - checks-actions
Build commit: abb4640 (abb464056e77c63d95009762fc5d5cc064a7dd8e)

rust-timer · 2024-07-13T15:04:54Z

Finished benchmarking commit (abb4640): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.7%]	79
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	28
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.7%]	79

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 705.609s -> 703.847s (-0.25%)
Artifact size: 328.61 MiB -> 328.70 MiB (0.03%)

oli-obk · 2024-07-13T15:30:35Z

Instruction count changes are unsurprising considering 46c6db8, but it looks like cycles are unaffected. I'll do some local profiling

oli-obk · 2024-07-16T11:43:40Z


 4,759,893  ???:rustc_ast::mut_visit::walk_block::<rustc_ast_passes::ast_validation::AstValidator>
 4,498,211  ???:rustc_ast::mut_visit::walk_expr::<rustc_ast_passes::ast_validation::AstValidator>
-2,594,747  ???:rustc_ast::mut_visit::noop_visit_block::<rustc_expand::placeholders::PlaceholderExpander>
 2,588,572  ???:rustc_ast::mut_visit::walk_block::<rustc_expand::placeholders::PlaceholderExpander>
 2,283,081  ???:<rustc_expand::placeholders::PlaceholderExpander as rustc_ast::mut_visit::MutVisitor>::visit_expr
-2,008,291  ???:rustc_ast::visit::walk_expr::<rustc_ast_passes::ast_validation::AstValidator>
-1,516,918  ???:rustc_ast::mut_visit::noop_visit_block::<rustc_expand::expand::InvocationCollector>
 1,507,507  ???:rustc_ast::mut_visit::walk_block::<rustc_expand::expand::InvocationCollector>
-1,342,389  ???:<rustc_parse::parser::Parser>::collect_tokens_for_expr::<<rustc_parse::parser::Parser>::parse_expr_prefix::{closure
 1,321,664  ???:<rustc_expand::expand::InvocationCollector as rustc_ast::mut_visit::MutVisitor>::visit_expr
 1,241,869  ???:<rustc_parse::parser::Parser>::parse_expr_prefix::{closure
  -964,508  ???:rustc_ast::mut_visit::noop_flat_map_stmt::<rustc_expand::expand::InvocationCollector>
   962,066  ???:rustc_ast::mut_visit::walk_flat_map_stmt::<rustc_expand::expand::InvocationCollector>
   936,985  ???:<rustc_expand::expand::MacroExpander>::fully_expand_fragment
  -568,177  ???:rustc_ast::visit::walk_stmt::<rustc_ast_passes::ast_validation::AstValidator>
   403,558  ???:<smallvec::SmallVec<[rustc_ast::ast::StmtKind; 1]>>::push
   214,188  ???:<thin_vec::ThinVec<rustc_ast::ptr::P<rustc_ast::ast::Expr>> as rustc_data_structures::flat_map_in_place::FlatMapInPlace<rustc_ast::ptr::P<rustc_ast::ast::Expr>>>::flat_map_in_place::<rustc_ast::mut_visit::visit_thin_exprs<rustc_expand::expand::InvocationCollector>::{closure
   183,629  ???:rustc_ast::mut_visit::walk_pat::<rustc_ast_passes::ast_validation::AstValidator>
   172,075  ???:<rustc_ast_passes::ast_validation::AstValidator as rustc_ast::mut_visit::MutVisitor>::visit_ty
   159,591  ???:rustc_ast::mut_visit::walk_expr::<rustc_expand::expand::InvocationCollector>
  -136,407  ???:rustc_ast::visit::walk_pat::<rustc_ast_passes::ast_validation::AstValidator>
  -132,325  ???:<rustc_ast_passes::ast_validation::AstValidator as rustc_ast::visit::Visitor>::visit_ty
   124,987  ???:rustc_ast::mut_visit::walk_expr::<rustc_expand::placeholders::PlaceholderExpander>
  -124,566  ???:rustc_ast::mut_visit::noop_visit_expr::<rustc_expand::expand::InvocationCollector>

unsure what's the exact cause. Should probably benchmark without the last few commits

petrochenkov · 2024-07-16T14:28:58Z

I'm skeptical about these parts of the motivation

changing from a Visitor to a MutVisitor requires extensive changes
changing ast validation to a MutVisitor

because we should probably not introduce new mutable visitors, which is a part of the larger discussion in https://rust-lang.zulipchat.com/#narrow/stream/315146-t-compiler.2Fetc.2Fincremental-hir/topic/Outlining.20modules.20from.20AST.

But having consistent naming is good in any case.

compiler/rustc_ast/src/mut_visit.rs

petrochenkov · 2024-07-16T15:07:39Z

The PR does many changes of different kinds in addition to the visitor renamings, they should probably be all landed and reviewed separately.
I didn't review the parts starting from the AST validator rewrite in detail.

…ions, r=petrochenkov Various ast validation simplifications Changes pulled out of rust-lang#127524 These are needed to make ast validation a mutable visitor, as we can't keep immutable references to the AST around in that case. But I think they are simplifying things in general and can stand on their own

Make ast `MutVisitor` have the same method name and style as `Visitor` It doesn't map 100% because some `MutVisitor` methods can filter or even expand to multiple items, but consistency seems nicer. tracking issue: rust-lang#127615

bors · 2024-07-22T14:41:03Z

⌛ Trying commit e9f32d0 with merge e58bcf0...

bors · 2024-07-22T16:32:16Z

☀️ Try build successful - checks-actions
Build commit: e58bcf0 (e58bcf072da9620b7fe78ff80b695a548ba0132e)

rust-timer · 2024-07-22T18:13:42Z

Finished benchmarking commit (e58bcf0): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.7%	[-2.7%, -2.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 771.169s -> 771.842s (0.09%)
Artifact size: 328.89 MiB -> 328.90 MiB (0.00%)

petrochenkov · 2024-07-23T11:05:50Z

@bors r+

bors · 2024-07-23T11:05:53Z

📌 Commit e9f32d0 has been approved by petrochenkov

It is now in the queue for this repository.

…nkov Make ast `MutVisitor` have the same method name and style as `Visitor` It doesn't map 100% because some `MutVisitor` methods can filter or even expand to multiple items, but consistency seems nicer. tracking issue: rust-lang#127615

bors · 2024-07-23T17:21:30Z

⌛ Testing commit e9f32d0 with merge a897baa...

bors · 2024-07-23T19:44:02Z

💔 Test failed - checks-actions

rust-log-analyzer · 2024-07-23T19:44:07Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

petrochenkov · 2024-07-23T20:26:28Z

@bors retry

bors · 2024-07-24T09:37:01Z

⌛ Testing commit e9f32d0 with merge d24930c...

bors · 2024-07-24T11:58:03Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing d24930c to master...

rust-timer · 2024-07-24T13:14:50Z

Finished benchmarking commit (d24930c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.3%	[3.3%, 3.3%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.585s -> 771.472s (0.25%)
Artifact size: 328.88 MiB -> 328.92 MiB (0.01%)

…nkov Make ast `MutVisitor` have the same method name and style as `Visitor` It doesn't map 100% because some `MutVisitor` methods can filter or even expand to multiple items, but consistency seems nicer. tracking issue: rust-lang#127615

rustbot assigned compiler-errors Jul 9, 2024

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 Jul 9, 2024

cjgillot reviewed Jul 9, 2024

View reviewed changes

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the feed_item_attrs2 branch from e8d7873 to 9844d13 Compare July 10, 2024 10:39

petrochenkov self-assigned this Jul 10, 2024

oli-obk mentioned this pull request Jul 11, 2024

ast::mut_visit::MutVisitor and ast::visit::Visitor do not have corresponding methods for all their methods #127615

Open

5 tasks

oli-obk unassigned compiler-errors Jul 11, 2024

oli-obk force-pushed the feed_item_attrs2 branch from 7f18720 to e89df61 Compare July 11, 2024 16:14

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 13, 2024

petrochenkov reviewed Jul 16, 2024

View reviewed changes

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved

compiler/rustc_ast/src/mut_visit.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2024

oli-obk mentioned this pull request Jul 16, 2024

Various ast validation simplifications #127818

Merged

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 22, 2024

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jul 22, 2024

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 Jul 23, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 23, 2024

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 Jul 23, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 24, 2024

bors merged commit d24930c into rust-lang:master Jul 24, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Jul 24, 2024

bors mentioned this pull request Jul 24, 2024

Remove Nonterminal and TokenKind::Interpolated #124141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ast `MutVisitor` have the same method name and style as `Visitor` #127524

Make ast `MutVisitor` have the same method name and style as `Visitor` #127524

oli-obk commented Jul 9, 2024 •

edited

Loading

rustbot commented Jul 9, 2024

fmease commented Jul 9, 2024

cjgillot commented Jul 13, 2024

This comment has been minimized.

bors commented Jul 13, 2024

bors commented Jul 13, 2024

This comment has been minimized.

rust-timer commented Jul 13, 2024

oli-obk commented Jul 13, 2024 •

edited

Loading

oli-obk commented Jul 16, 2024

petrochenkov commented Jul 16, 2024

petrochenkov commented Jul 16, 2024

This comment has been minimized.

bors commented Jul 22, 2024

bors commented Jul 22, 2024

This comment has been minimized.

rust-timer commented Jul 22, 2024

petrochenkov commented Jul 23, 2024

bors commented Jul 23, 2024

bors commented Jul 23, 2024

bors commented Jul 23, 2024

rust-log-analyzer commented Jul 23, 2024

petrochenkov commented Jul 23, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

rust-timer commented Jul 24, 2024

Make ast MutVisitor have the same method name and style as Visitor #127524

Make ast MutVisitor have the same method name and style as Visitor #127524

Conversation

oli-obk commented Jul 9, 2024 • edited Loading

rustbot commented Jul 9, 2024

fmease commented Jul 9, 2024

cjgillot commented Jul 13, 2024

This comment has been minimized.

bors commented Jul 13, 2024

bors commented Jul 13, 2024

This comment has been minimized.

rust-timer commented Jul 13, 2024

Overall result: ❌ regressions - ACTION NEEDED

oli-obk commented Jul 13, 2024 • edited Loading

oli-obk commented Jul 16, 2024

petrochenkov commented Jul 16, 2024

petrochenkov commented Jul 16, 2024

This comment has been minimized.

bors commented Jul 22, 2024

bors commented Jul 22, 2024

This comment has been minimized.

rust-timer commented Jul 22, 2024

Overall result: no relevant changes - no action needed

petrochenkov commented Jul 23, 2024

bors commented Jul 23, 2024

bors commented Jul 23, 2024

bors commented Jul 23, 2024

rust-log-analyzer commented Jul 23, 2024

petrochenkov commented Jul 23, 2024

bors commented Jul 24, 2024

bors commented Jul 24, 2024

rust-timer commented Jul 24, 2024

Overall result: no relevant changes - no action needed

Make ast `MutVisitor` have the same method name and style as `Visitor` #127524

Make ast `MutVisitor` have the same method name and style as `Visitor` #127524

oli-obk commented Jul 9, 2024 •

edited

Loading

oli-obk commented Jul 13, 2024 •

edited

Loading