remove `visit_clobber` and move `DummyAstNode` to `rustc_expand` #141430

fee1-dead · 2025-05-23T09:49:32Z

visit_clobber is not really useful except for one niche purpose
involving generic code. We should just use the replace logic where we
can.

rustbot · 2025-05-23T09:49:36Z

r? @oli-obk

rustbot has assigned @oli-obk.
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

fmease · 2025-05-23T10:00:09Z

FYI: Something similar was tried in an early version of PR #121829. Discussion: #121829 (comment), not sure if still relevant / relevant in general. I only skimmed your & the other PR.

fee1-dead · 2025-05-23T10:12:12Z

not sure if still relevant / relevant in general

seems irrelevant to me. There has been a year since the previous comment saying there could be additional uses of DummyAstNode but new use still has not appeared.

If we want to get a dummy of something why should we use a trait? just put it e.g. as an inherent method on P<Expr> would suffice too.

fee1-dead · 2025-05-23T12:05:18Z

🤦 Sorry for the spam. I really should stop being lazy and do more local testing before pushing.

fmease · 2025-05-23T12:06:08Z

Nah, stage2 tests are always a pain ^^'

petrochenkov · 2025-05-23T13:15:04Z

I don't think this is an improvement, invocation collector works with nodes generically, abstracting a number of small operations for every node, including clobbering.
trait InvocationCollectorNode is mostly used for that, but InvocationCollectorNode + DummyAstNode for some subset as well.
The current scheme also allows to do the clobbering only when strictly necessary (unlike this PR).
I'd rather prefer to keep things as is.

DummyAstNode and visit_clobber can be moved to expand.rs though.
Or even merged into InvocationCollectorNode with panic as the default implementation, but that would shift some errors from compile time to runtime.
Maybe some specific impls of DummyAstNode can also be removed now.

compiler/rustc_expand/src/expand.rs

tests/ui-fulldeps/pprust-expr-roundtrip.rs

petrochenkov · 2025-05-23T13:20:31Z

r? @petrochenkov
@rustbot author

rustbot · 2025-05-23T13:20:34Z

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

fee1-dead · 2025-05-23T14:16:01Z

I don't think this is an improvement, invocation collector works with nodes generically, abstracting a number of small operations for every node, including clobbering.

As I said previously, I don't think having that generic behavior is necessary.

visit_node only needs the input type to implement DummyAstNode for clobbering because it takes in a &mut Node, when it doesn't have to. When it simply returns the new node created, clobbering is no longer needed.

The bridge from MutVisitor to InvocationCollectorNode is the clobbering. This PR makes the clobbering explicit and specific to each bridged type. InvocationCollectorNode does not need clobbering.

petrochenkov · 2025-05-23T14:53:34Z

@bors try @rust-timer queue

remove `visit_clobber` and `DummyAstNode` used for one specific niche purpose, turns out we can just remove them and inline dummy values into callsites. this cleans them up.

bors · 2025-05-23T14:54:44Z

⌛ Trying commit 477d572 with merge ec406cc...

fee1-dead · 2025-05-27T11:57:39Z

@bors r=petrochenkov

bors · 2025-05-27T11:57:41Z

📌 Commit 09d4189 has been approved by petrochenkov

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - #141312 (Add From<TryLockError> for io::Error) - #141495 (Rename `{GenericArg,Term}::unpack()` to `kind()`) - #141602 (triagebot: label LLVM submodule changes with `A-LLVM`) - #141632 (remove `visit_mt` from `ast::mut_visit`) - #141640 (test: convert version_check ui test to run-make) - #141645 (bump fluent-* crates) - #141650 (coverage: Revert "unused local file IDs" due to empty function names) - #141654 (tests: mark option-niche-eq as fixed on LLVM 21) Failed merges: - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - #141636 (avoid some usages of `&mut P<T>` in AST visitors) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-05-27T19:49:32Z

☔ The latest upstream changes (presumably #141644) made this pull request unmergeable. Please resolve the merge conflicts.

Rollup of 8 pull requests Successful merges: - rust-lang/rust#141312 (Add From<TryLockError> for io::Error) - rust-lang/rust#141495 (Rename `{GenericArg,Term}::unpack()` to `kind()`) - rust-lang/rust#141602 (triagebot: label LLVM submodule changes with `A-LLVM`) - rust-lang/rust#141632 (remove `visit_mt` from `ast::mut_visit`) - rust-lang/rust#141640 (test: convert version_check ui test to run-make) - rust-lang/rust#141645 (bump fluent-* crates) - rust-lang/rust#141650 (coverage: Revert "unused local file IDs" due to empty function names) - rust-lang/rust#141654 (tests: mark option-niche-eq as fixed on LLVM 21) Failed merges: - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang/rust#141636 (avoid some usages of `&mut P<T>` in AST visitors) r? `@ghost` `@rustbot` modify labels: rollup

`visit_clobber` is not really useful except for one niche purpose involving generic code. We should just use the replace logic where we can.

fee1-dead · 2025-05-29T04:55:54Z

@bors r=petrochenkov

bors · 2025-05-29T04:55:57Z

📌 Commit 5e71855 has been approved by petrochenkov

It is now in the queue for this repository.

Rollup of 4 pull requests Successful merges: - #138285 (Stabilize `repr128`) - #139994 (add `CStr::display`) - #141571 (coretests: extend and simplify float tests) - #141656 (CI: Add cargo tests to aarch64-apple-darwin) Failed merges: - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - #141636 (avoid some usages of `&mut P<T>` in AST visitors) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 8 pull requests Successful merges: - #133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - #141004 (Report text_direction_codepoint_in_literal when parsing) - #141407 (Refactor the two-phase check for impls and impl items) - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - #141507 (atomic_load intrinsic: use const generic parameter for ordering) - #141538 (implement `va_arg` for x86_64 systemv) - #141669 (float: Replace some approximate assertions with exact) - #141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #141430 - fee1-dead-contrib:push-nmzoprvtsvww, r=petrochenkov remove `visit_clobber` and move `DummyAstNode` to `rustc_expand` `visit_clobber` is not really useful except for one niche purpose involving generic code. We should just use the replace logic where we can.

Rollup of 4 pull requests Successful merges: - rust-lang#138285 (Stabilize `repr128`) - rust-lang#139994 (add `CStr::display`) - rust-lang#141571 (coretests: extend and simplify float tests) - rust-lang#141656 (CI: Add cargo tests to aarch64-apple-darwin) Failed merges: - rust-lang#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang#141636 (avoid some usages of `&mut P<T>` in AST visitors) r? `@ghost` `@rustbot` modify labels: rollup

Kobzol · 2025-05-30T15:19:45Z

@rust-timer build 7c866b4

(Trying perf. due to #141753)

rust-timer · 2025-05-30T21:14:48Z

Finished benchmarking commit (7c866b4): comparison URL.

Overall result: ❌ regressions - 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 is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (secondary 0.1%)

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)	1.4%	[0.5%, 3.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-1.1%, -0.5%]	6
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 0.2%)

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)	1.3%	[0.4%, 3.4%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-3.6%, -0.4%]	9
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: missing data
Artifact size: 370.26 MiB -> 370.22 MiB (-0.01%)

Rollup of 8 pull requests Successful merges: - rust-lang/rust#133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion) - rust-lang/rust#141004 (Report text_direction_codepoint_in_literal when parsing) - rust-lang/rust#141407 (Refactor the two-phase check for impls and impl items) - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang/rust#141507 (atomic_load intrinsic: use const generic parameter for ordering) - rust-lang/rust#141538 (implement `va_arg` for x86_64 systemv) - rust-lang/rust#141669 (float: Replace some approximate assertions with exact) - rust-lang/rust#141747 (rustdoc: display doc(cfg(false)) properly) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 4 pull requests Successful merges: - rust-lang/rust#138285 (Stabilize `repr128`) - rust-lang/rust#139994 (add `CStr::display`) - rust-lang/rust#141571 (coretests: extend and simplify float tests) - rust-lang/rust#141656 (CI: Add cargo tests to aarch64-apple-darwin) Failed merges: - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`) - rust-lang/rust#141636 (avoid some usages of `&mut P<T>` in AST visitors) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned oli-obk May 23, 2025

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 May 23, 2025

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the push-nmzoprvtsvww branch from 219f808 to b84f556 Compare May 23, 2025 10:10

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the push-nmzoprvtsvww branch from b84f556 to c7ee736 Compare May 23, 2025 11:10

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the push-nmzoprvtsvww branch from c7ee736 to dbaf7ae Compare May 23, 2025 11:53

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the push-nmzoprvtsvww branch from dbaf7ae to 477d572 Compare May 23, 2025 12:04

petrochenkov self-assigned this May 23, 2025

petrochenkov reviewed May 23, 2025

View reviewed changes

compiler/rustc_expand/src/expand.rs Show resolved Hide resolved

tests/ui-fulldeps/pprust-expr-roundtrip.rs Show resolved Hide resolved

rustbot 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 May 23, 2025

petrochenkov unassigned oli-obk May 23, 2025

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 May 23, 2025

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2025

matthiaskrgr mentioned this pull request May 27, 2025

Rollup of 8 pull requests #141662

Merged

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 27, 2025

jhpratt mentioned this pull request May 29, 2025

Rollup of 4 pull requests #141717

Merged

remove visit_clobber and move DummyAstNode to rustc_expand

5e71855

`visit_clobber` is not really useful except for one niche purpose involving generic code. We should just use the replace logic where we can.

fee1-dead force-pushed the push-nmzoprvtsvww branch from 09d4189 to 5e71855 Compare May 29, 2025 04:46

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2025

matthiaskrgr mentioned this pull request May 30, 2025

Rollup of 8 pull requests #141753

Merged

bors merged commit a87bc9d into rust-lang:master May 30, 2025
9 checks passed

rustbot added this to the 1.89.0 milestone May 30, 2025

This comment has been minimized.

Sign in to view

rustbot removed the perf-regression Performance regression. label May 30, 2025

remove visit_clobber and move DummyAstNode to rustc_expand #141430

remove visit_clobber and move DummyAstNode to rustc_expand #141430

Uh oh!

Conversation

fee1-dead commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 23, 2025

Uh oh!

fmease commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

fee1-dead commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fee1-dead commented May 23, 2025

Uh oh!

fmease commented May 23, 2025

Uh oh!

petrochenkov commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented May 23, 2025

Uh oh!

rustbot commented May 23, 2025

Uh oh!

fee1-dead commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented May 23, 2025

Uh oh!

This comment has been minimized.

bors commented May 23, 2025

Uh oh!

fee1-dead commented May 27, 2025

Uh oh!

bors commented May 27, 2025

Uh oh!

bors commented May 27, 2025

Uh oh!

fee1-dead commented May 29, 2025

Uh oh!

bors commented May 29, 2025

Uh oh!

Uh oh!

Kobzol commented May 30, 2025

Uh oh!

This comment has been minimized.

rust-timer commented May 30, 2025

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Uh oh!

remove `visit_clobber` and move `DummyAstNode` to `rustc_expand` #141430

remove `visit_clobber` and move `DummyAstNode` to `rustc_expand` #141430

fee1-dead commented May 23, 2025 •

edited

Loading

fmease commented May 23, 2025 •

edited

Loading

fee1-dead commented May 23, 2025 •

edited

Loading

petrochenkov commented May 23, 2025 •

edited

Loading

fee1-dead commented May 23, 2025 •

edited

Loading