Skip to content

Clean up AST visitor some more - #162315

Merged
rust-bors[bot] merged 11 commits into
rust-lang:mainfrom
nnethercote:clean-up-AST-visitor-some-more
Sep 14, 2026
Merged

Clean up AST visitor some more#162315
rust-bors[bot] merged 11 commits into
rust-lang:mainfrom
nnethercote:clean-up-AST-visitor-some-more

Conversation

@nnethercote

@nnethercote nnethercote commented Sep 4, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

A sequel to #162226. Details in individual commits.

r? @fee1-dead

@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 Sep 4, 2026
@nnethercote

nnethercote commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

LLM disclosure: most of these clean-ups were suggested by an LLM. I made all the code and text changes myself.

@nnethercote

nnethercote commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Shouldn't affect perf, but just in case:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 4, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 4, 2026
@bors
Auto merge of #162315 - nnethercote:clean-up-AST-visitor-some-more, r…
2a4114a 
…=<try>

Clean up AST visitor some more
@rust-bors rust-bors Bot 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 Sep 5, 2026
@rust-bors

rust-bors Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

💥 Test timed out after 21600s

@nnethercote

nnethercote commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

💥 Test timed out after 21600s

Not sure what happened. Let's try again:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 5, 2026
@bors
Auto merge of #162315 - nnethercote:clean-up-AST-visitor-some-more, r…
d804d79 
…=<try>

Clean up AST visitor some more
@rust-bors

rust-bors Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: d804d79 (d804d7975d767ccd03c93b9f34541ae4bc41c8ee)
Base parent: 0ed41eb (0ed41eb4142dda2df61eb1145a312c1a9d62eb56)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d804d79): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 2.2%, secondary 6.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (secondary -2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 476.718s -> 480.953s (0.89%)
Artifact size: 403.29 MiB -> 403.40 MiB (0.03%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 5, 2026
@nnethercote
nnethercote marked this pull request as draft September 5, 2026 13:16
@nnethercote

nnethercote commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Slight regressions on html5ever. I'll investigate on Monday.

@rust-bors

This comment has been minimized.

Sign in to view
@nnethercote
nnethercote force-pushed the clean-up-AST-visitor-some-more branch from f9e5acf to ad3c224 Compare September 6, 2026 23:34
@nnethercote

nnethercote commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 6, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 6, 2026
@bors
Auto merge of #162315 - nnethercote:clean-up-AST-visitor-some-more, r…
e8a15ce 
…=<try>

Clean up AST visitor some more
@nnethercote
Remove need for allow(unused_parens)
d3b4e4c 
The AST visitor code uses `($($extra_ty)?)` which expands to `()` in
some cases and `(T)` in others, which requires `allow(unused_parens)` to
avoid warnings.

This commit splits the two `impl_visitable!` macros into two rules: one
for the "no-extra" case and one for the "with-extra" case. There's a
small amount of duplication between the two rules but there are multiple
advantages.

- The `allow(unused_parens)` is removed.

- `impl_visitable!` callers can omit the extra param if it's `()`.

- When the extra argument might or might not be present, a more standard
  `$(, $extra: $extra_ty)?` is now used.

- No weird `let ($($extra)?) = extra;` destructuring.

The `Visitable` derive also gets a tweak to avoid unnecessary parens
around extra args.
@nnethercote
Avoid #[macro_export]
75fd853 
These macros aren't needed outside the crate.
@nnethercote
Don't pass in $Visitor/$lt to various macros
ad22c2b 
We can just hardwire `'a` and `Visitor`/`MutVisitor`. This simplifies
the inputs for these macros from "leading thing + repeating elements" to
just "repeating elements".
@nnethercote
Use tidy-alphabetical markers on various lists
cd21f63 
A lot of these lists are already almost in alphabetical order.
@nnethercote
Inline visit/visit_mut in impl_visitable
712e322
@nnethercote
Rename ty macro parameters
44aee39 
As requested by the reviewer.
@nnethercote
nnethercote force-pushed the clean-up-AST-visitor-some-more branch from ad3c224 to 44aee39 Compare September 13, 2026 23:41
@rustbot

rustbot commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@nnethercote

nnethercote commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

I addressed the nit.

@bors r=fee1-dead

@rust-bors

rust-bors Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 44aee39 has been approved by fee1-dead

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Sep 13, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 14, 2026
@bors
Auto merge of #162315 - nnethercote:clean-up-AST-visitor-some-more, r…
dd3ef79 
…=fee1-dead

Clean up AST visitor some more



A sequel to #162226. Details in individual commits.

r? @fee1-dead
@rust-log-analyzer

rust-log-analyzer commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

The job dist-x86_64-msvc failed! Check out the build log: (web) (plain enhanced) (plain)

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

@rust-bors rust-bors Bot 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 Sep 14, 2026
@rust-bors

rust-bors Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

💔 Test for dd3ef79 failed: CI. Failed job:

@jieyouxu

jieyouxu commented Sep 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

The hosted runner lost communication with the server. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

@jieyouxu

jieyouxu commented Sep 14, 2026

Copy link
Copy Markdown
Member

@bors retry

@rust-bors rust-bors Bot 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 Sep 14, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@rust-bors rust-bors Bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 14, 2026
@rust-bors

rust-bors Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

☀️ Test successful - CI
Approved by: fee1-dead
Duration: 3h 12m 2s
Pushing ed77b7b to main...

@rust-bors
rust-bors Bot merged commit ed77b7b into rust-lang:main Sep 14, 2026
14 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Sep 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing b722b55 (parent) -> ed77b7b (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard ed77b7b8699e342c2dc842c83cb5fe4025252ee8 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. test-x86_64-gnu-miri: 1h 30m -> 53m 9s (-41.6%)
  2. test-x86_64-msvc-ext2: 1h 54m -> 1h 9m (-39.7%)
  3. dist-android: 32m 56s -> 22m 17s (-32.3%)
  4. test-x86_64-msvc-ext3: 2h -> 1h 22m (-31.6%)
  5. dist-arm-linux-musl: 1h 50m -> 1h 17m (-29.8%)
  6. test-i686-gnu-nopt-2: 2h 33m -> 1h 48m (-29.2%)
  7. test-pr-check-2: 35m 14s -> 45m 22s (+28.8%)
  8. dist-powerpc64le-linux-musl: 1h 15m -> 1h 36m (+28.6%)
  9. test-x86_64-gnu-llvm-22-2: 1h 46m -> 1h 16m (-28.2%)
  10. dist-i686-linux: 1h 31m -> 1h 55m (+27.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer

rust-timer commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (ed77b7b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

Results (primary -2.2%, secondary 9.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 9.4% [9.1%, 9.7%] 2
Improvements ✅
(primary)		 -2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -2.2% [-2.2%, -2.2%] 1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 498.957s -> 495.332s (-0.73%)
Artifact size: 406.94 MiB -> 406.96 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fee1-dead fee1-dead fee1-dead requested changes

Assignees

@fee1-dead fee1-dead

Labels

merged-by-bors This PR was explicitly merged by bors. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.100.0

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote @rust-timer @rustbot @rust-log-analyzer @jieyouxu @fee1-dead