Skip to content

Conversation

cjgillot
Copy link
Contributor

AST visitors are large and error-prone beasts. This PR attempts to write them using a derive macro.

The design uses three traits: Visitor, Visitable, Walkable.

  • Visitor is the trait implemented by downstream crates, it lists visit_stuff methods, which call Walkable::walk_ref by default;
  • Walkable is derived using the macro, the generated walk_ref method calls Visitable::visit on each component;
  • Visitable is implemented by common_visitor_and_walkers macro, to call the proper Visitor::visit_stuff method if it exists, to call Walkable::walk_ref if there is none.

I agree this is quite a lot of spaghetti macros. I'm open to suggestions on how to reduce the amount of boilerplate code.

If this PR is accepted, I believe the same design can be used for the HIR visitor.

@rustbot
Copy link
Collaborator

rustbot commented Jul 13, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
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 Jul 13, 2025
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2025
bors added a commit that referenced this pull request Jul 13, 2025
Implement AST visitors using a derive macro.

AST visitors are large and error-prone beasts. This PR attempts to write them using a derive macro.

The design uses three traits: `Visitor`, `Visitable`, `Walkable`.
- `Visitor` is the trait implemented by downstream crates, it lists `visit_stuff` methods, which call `Walkable::walk_ref` by default;
- `Walkable` is derived using the macro, the generated `walk_ref` method calls `Visitable::visit` on each component;
- `Visitable` is implemented by `common_visitor_and_walkers` macro, to call the proper `Visitor::visit_stuff` method if it exists, to call `Walkable::walk_ref` if there is none.

I agree this is quite a lot of spaghetti macros. I'm open to suggestions on how to reduce the amount of boilerplate code.

If this PR is accepted, I believe the same design can be used for the HIR visitor.
@bors
Copy link
Collaborator

bors commented Jul 13, 2025

⌛ Trying commit 8dea854 with merge 1140f76...

@bors
Copy link
Collaborator

bors commented Jul 13, 2025

☀️ Try build successful - checks-actions
Build commit: 1140f76 (1140f76b9982bc0f7d192946fc7f08237146878b)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1140f76): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@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
Regressions ❌
(secondary)
0.7% [0.6%, 0.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 2.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)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (primary 4.5%, secondary -1.2%)

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

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

Binary size

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

Bootstrap: 465.776s -> 467.271s (0.32%)
Artifact size: 374.73 MiB -> 374.77 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2025
@jieyouxu
Copy link
Member

r? compiler (review capacity)

@rustbot rustbot assigned SparrowLii and unassigned jieyouxu Jul 14, 2025
@SparrowLii
Copy link
Member

I think this need a reviewer who is familiar with AST and macros.
r? @petrochenkov I guess you have the ability to review it?

@rustbot rustbot assigned petrochenkov and unassigned SparrowLii Jul 14, 2025
@petrochenkov
Copy link
Contributor

The code in compiler/rustc_ast/src/(mut_)visit.rs is clearly write-only now, but it was already true since the visitor/mut-visitor deduplication that introduced heavy use of macros, so this PR doesn't make things significantly worse.

@petrochenkov
Copy link
Contributor

I'm open to suggestions on how to reduce the amount of boilerplate code.

I think in this PR the remaining code in mut_visit.rs can be merged into visit.rs, it will allow to get rid of macro_exporting and importing the helper macros at least.

@petrochenkov
Copy link
Contributor

At high level, there are two calls to common_visitor_and_walkers!.

common_visitor_and_walkers!(Visitor<'a>);
common_visitor_and_walkers!((mut) MutVisitor);

At the same time the logic in helper macros is duplicated for mutable OR immutable visitor, e.g. like this

#[macro_export]
macro_rules! impl_visitable_noop {
    (<$lt:lifetime> $($ty:ty,)*) => {
        $(
            impl_visitable!(|&$lt self: $ty, _vis: &mut V, _extra: ()| {
                V::Result::output()
            });
        )*
    };
    (<mut> $($ty:ty,)*) => {
        $(
            impl_visitable!(|&mut self: $ty, _vis: &mut V, _extra: ()| {});
        )*
    }
}

If the two visitors are placed into the same file, then perhaps the common_visitor_and_walkers macro can be removed, and both its calls inlined, and the logic in helper macros can be changed to produce code for mutable AND immutable visitor?

macro_rules! impl_visitable_noop {
    (<$lt:lifetime> $($ty:ty,)*) => {
        $(
            impl_visitable!(|&$lt self: $ty, _vis: &mut V, _extra: ()| {
                V::Result::output()
            });
        )*

        $(
            impl_visitable!(|&mut self: $ty, _vis: &mut V, _extra: ()| {});
        )*
    };
}

@petrochenkov 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 14, 2025
@cjgillot
Copy link
Contributor Author

@petrochenkov Merging both files seems a bit tricky, as they are used by downstream code to chose which version of walk_* to call. The churn seems a bit too high. Instead I changed the macros to just have a version in visit and another one in mut_visit. Do you want me to duplicate contents of common_visitor_and_walkers too?

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

rustbot commented Jul 21, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@cjgillot
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Jul 22, 2025

📌 Commit 3c81fae has been approved by petrochenkov

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2025
@bors
Copy link
Collaborator

bors commented Jul 22, 2025

⌛ Testing commit 3c81fae with merge 55d9909...

bors added a commit that referenced this pull request Jul 22, 2025
Implement AST visitors using a derive macro.

AST visitors are large and error-prone beasts. This PR attempts to write them using a derive macro.

The design uses three traits: `Visitor`, `Visitable`, `Walkable`.
- `Visitor` is the trait implemented by downstream crates, it lists `visit_stuff` methods, which call `Walkable::walk_ref` by default;
- `Walkable` is derived using the macro, the generated `walk_ref` method calls `Visitable::visit` on each component;
- `Visitable` is implemented by `common_visitor_and_walkers` macro, to call the proper `Visitor::visit_stuff` method if it exists, to call `Walkable::walk_ref` if there is none.

I agree this is quite a lot of spaghetti macros. I'm open to suggestions on how to reduce the amount of boilerplate code.

If this PR is accepted, I believe the same design can be used for the HIR visitor.
@jieyouxu
Copy link
Member

Not sure why arm-android is taking so long. I'll requeue this if it times out.

@bors
Copy link
Collaborator

bors commented Jul 23, 2025

💥 Test timed out

@bors 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, 2025
@jieyouxu
Copy link
Member

arm-android timeout
@bors retry

@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 Jul 23, 2025
@bors
Copy link
Collaborator

bors commented Jul 23, 2025

⌛ Testing commit 3c81fae with merge 20aa182...

@bors
Copy link
Collaborator

bors commented Jul 23, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 23, 2025
@bors bors merged commit 20aa182 into rust-lang:master Jul 23, 2025
12 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 23, 2025
Copy link
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 a7a1618 (parent) -> 20aa182 (this PR)

Test differences

Show 14 test diffs

14 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 20aa182235d6b27ecee519f1d18ee30f0d1c4a61 --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. dist-x86_64-apple: 11145.7s -> 15319.3s (37.4%)
  2. dist-apple-various: 9221.3s -> 6265.9s (-32.1%)
  3. dist-aarch64-apple: 7155.9s -> 5128.4s (-28.3%)
  4. aarch64-apple: 4297.7s -> 5254.3s (22.3%)
  5. pr-check-2: 2166.1s -> 2594.8s (19.8%)
  6. x86_64-gnu-llvm-20-1: 3290.4s -> 3684.6s (12.0%)
  7. x86_64-rust-for-linux: 2600.4s -> 2908.0s (11.8%)
  8. i686-gnu-2: 5314.6s -> 5904.0s (11.1%)
  9. x86_64-apple-1: 7215.2s -> 8007.0s (11.0%)
  10. armhf-gnu: 4904.1s -> 5417.4s (10.5%)
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
Copy link
Collaborator

Finished benchmarking commit (20aa182): 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.1% [0.1%, 0.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

Results (primary 2.4%, secondary -0.8%)

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

mean range count
Regressions ❌
(primary)
2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)
4.7% [4.5%, 4.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.4% [-6.8%, -2.9%] 3
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Binary size

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

Bootstrap: 463.694s -> 469.114s (1.17%)
Artifact size: 374.58 MiB -> 374.63 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

7 participants