Unify AST Visitors with a macro like MIR Visitors #128974

maxcabrajac · 2024-08-11T15:35:50Z

This is a PR to work on #127615

rustbot · 2024-08-11T15:35:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

lcnr · 2024-08-12T08:38:08Z

r? compiler

lcnr · 2024-08-12T08:38:20Z

r? @cjgillot woops

cjgillot

Wow, that's some great work you did @maxcabrajac!
Please take this as an opportunity to simplify stuff. If some distinction has you make weird corner cases, don't hesitate to remove it. My comments are suggestions in this direction.

compiler/rustc_ast/src/visitors.rs

cjgillot · 2024-08-27T00:51:32Z

compiler/rustc_ast/src/visitors.rs

+            make_visit!{P!(Local), visit_local, walk_local}
+            make_visit!{P!(Pat), visit_pat, walk_pat}
+            make_visit!{P!(Expr), visit_expr, walk_expr}
+            make_visit!{P!(Ty), visit_ty, walk_ty}
+            make_visit!{P!(Block), visit_block, walk_block}
+            make_visit!{P!(FnDecl), visit_fn_decl, walk_fn_decl}


Should we remove the P! from all these?

I put these P! to maintain all function signatures the same, thus avoiding changes on implementers. I don't know why the original MutVisitor received &mut P<T> instead of &mut T for these types, but I just kept what was being done.

maxcabrajac · 2024-08-27T01:32:10Z

I think it's better to do two passes. Do what can be done without changing the trait API, after that, change what is weird about their original implementation and change all implementers accordingly. What do you think about it?
There is a pretty big roadblock now... Everything around walk_item seems to be pretty different from Visitor to MutVisitor and I really don't know where to start =/. Do you have any advice on that?

maxcabrajac · 2024-10-15T18:19:13Z

Hey @cjgillot!
I just did a major history-rewrite of what I had done.
At first, I kept on the branch a bunch of changes that either didn't make to the final cut or were a bit unrelated and could be done in follow up PRs. (I have two tiny follow-up PRs in mind)
Now the commit history is much cleaner! I think that If anyone wants to check if nothing was missed (what is a immense, but doable task) going through the diffs should be ok.
Could you please have a look at it? =)

Hey @oli-obk!
Your input would also be highly appreciated!

maxcabrajac · 2024-10-23T17:27:16Z

Hey @cjgillot !
Could you comment "hi"/"ACK"/anything when you see this?
I think my messages might have gotten buried or something like that and you missed them. =(
Thanks =)

petrochenkov · 2024-10-23T19:52:23Z

I was also going to look at this PR, but didn't because it's too large, and I couldn't allocate enough time.
If some refactorings can be reasonably split into separate PRs, then doing that should speed up the reviewing/landing process.

maxcabrajac · 2024-10-23T21:39:27Z

Hi @petrochenkov!

Thanks for the reply =)
I'm in no rush, I would love for you to have a look at it. =)
Sadly, I don't know how I to reasonably split this into PRs that don't look out of place...
Assuming that leaving it looking like a WIP inbetween PRs is ok (which I think it isn't) this is the best I can do is:

Remove visit_expr_post (d6a89c1 this seems reasonable to be its own PR, but has be done before the f097c35)
Unify all of the visit/walks that are already mostly uniform (everything up to commit 089d0ee)
Unify the problematic ones, namely, *_item and *_fn (from fcb93eb to 0e75ea0)
Unify walk_expr as it depends on *_fn being uniform (f097c35)
Unify flatmaps (from a6d7ecc to 24465fd)

There were some changes I did on my first try/think can be done that I pulled out to do in follow up PR's:

Add flatmaps for some suitable nodes (eg. Attributes)
Use visit_coroutine_kind in rustc_lint/src/early.rs
Overload visit_* instead of flatmap_* in rustc_builtin_macros/src/cfg_eval.rs

What do you think about it?

petrochenkov · 2024-10-24T12:59:07Z

Assuming that leaving it looking like a WIP inbetween PRs is ok

It's ok, anything is better than a PR with 3k+ diff.

I suggest first landing changes unifying interfaces between mutable and immutable visitors.
No macros, no mass moving code between files/modules, etc, 900 lines diff is a limit for a PR.

If something is changed in visiting interfaces of Idents, Symbols, Spans or NodeIds then it needs to be submitted separately for benchmarking.

Feel free to assign such PRs to me.

(Also, splitting "introduce X" and "use X to do something" into separate commits is not very useful, especially if X is not large.)

petrochenkov · 2024-10-24T13:02:26Z

The macros here look pretty terrifying, TBH.
With the old code it's trivial to understand what happens, while the new code requires time and some macro expertise.
Perhaps using proc macros would be more readable in this case.
But we can decide on that after the first part is merged.

maxcabrajac · 2024-10-24T13:07:43Z

Ok! I'll do that then!

rustbot · 2024-10-24T13:10:17Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

491794f

petrochenkov · 2024-10-24T15:08:12Z

@maxcabrajac
To clarify, could you keep the full set of changes in this PR, and make new PRs for the partial changes?

Upd: could you also refer to this PR from PR descriptions of the other PRs?

maxcabrajac · 2024-10-24T17:39:16Z

@maxcabrajac To clarify, could you keep the full set of changes in this PR, and make new PRs for the partial changes?

That force push to this PR was just because I missclicked "Sync fork" on the wrong branch and wanted to revert it. I'm keeping this as reference for what to do in the other PRs.

Now I'm doing what can be done before introducing macros, ok?

Upd: could you also refer to this PR from PR descriptions of the other PRs?

Added "related to #this PR" to them.

One last thing:

The macros here look pretty terrifying, TBH.

Is make_visit the one that bothers? One my first try I didn't use this approach, but opted for it when there were 500 lines of just:

fn visit_t(&mut self, t: &$($lt)? $($mut)? T)) -> result!() {
    walk_t(self, t)
}

After already using make_visit for things, I needed to make it pass additional arguments around, which might not have come out great...
That being said, we can either remove or improve make_visit if it fits. =)

…trochenkov Remove visit_expr_post from ast Visitor `visit_expr_post` is only present in the immutable version of ast Visitors and its default implementation is a noop. Given that its only implementer is on `rustc_lint/src/early.rs` and its name follows the same naming convention as some other lints (`_post`), it seems that `visit_expr_post` being in `Visitor` was a little mistake. r? `@petrochenkov` related to rust-lang#128974

petrochenkov · 2024-10-24T19:07:10Z

Now I'm doing what can be done before introducing macros, ok?

👍

Is make_visit the one that bothers?

I didn't look into much detail yet, just a general feeling (e.g. there are multiple layers of macros, a dozen of helper macros, etc).

Rollup merge of rust-lang#132107 - maxcabrajac:remove_expr_post, r=petrochenkov Remove visit_expr_post from ast Visitor `visit_expr_post` is only present in the immutable version of ast Visitors and its default implementation is a noop. Given that its only implementer is on `rustc_lint/src/early.rs` and its name follows the same naming convention as some other lints (`_post`), it seems that `visit_expr_post` being in `Visitor` was a little mistake. r? `@petrochenkov` related to rust-lang#128974

bors · 2024-10-24T23:23:04Z

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

Pass Ident by reference in ast Visitor `MutVisitor`'s version of `visit_ident` passes around `&Ident`, but `Visitor` copies `Ident`. This PR changes that r? `@petrochenkov` related to rust-lang#128974

Rollup merge of rust-lang#132106 - maxcabrajac:ident_ref, r=petrochenkov Pass Ident by reference in ast Visitor `MutVisitor`'s version of `visit_ident` passes around `&Ident`, but `Visitor` copies `Ident`. This PR changes that r? `@petrochenkov` related to rust-lang#128974

rustbot assigned lcnr Aug 11, 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 Aug 11, 2024

This comment has been minimized.

Sign in to view

cjgillot self-assigned this Aug 11, 2024

rustbot assigned davidtwco and unassigned cjgillot and lcnr Aug 12, 2024

rustbot assigned cjgillot and unassigned davidtwco Aug 12, 2024

This comment has been minimized.

Sign in to view

cjgillot reviewed Aug 27, 2024

View reviewed changes

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

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

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

cjgillot reviewed Aug 27, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

maxcabrajac force-pushed the visitor_macro branch from 6613670 to 2d751e6 Compare August 28, 2024 17:54

This comment has been minimized.

Sign in to view

Add make_walk_flat_map

24465fd

maxcabrajac force-pushed the visitor_macro branch from 4fba12f to 24465fd Compare October 15, 2024 17:23

maxcabrajac added 3 commits October 15, 2024 14:54

Unify visit_method_receiver_expr

6440429

Unify comment

fdc62e4

Add FIXME asking for removal of some P!s

87e6380

petrochenkov self-assigned this Oct 23, 2024

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 Oct 24, 2024

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Oct 24, 2024

maxcabrajac marked this pull request as draft October 24, 2024 13:31

maxcabrajac force-pushed the visitor_macro branch from 491794f to 87e6380 Compare October 24, 2024 13:43

This was referenced Oct 24, 2024

Remove P<> from visit_s in ast MutVisitor #132112

Draft

Remove visit_expr_post from ast Visitor #132107

Merged

Pass Ident by reference in ast Visitor #132106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify AST Visitors with a macro like MIR Visitors #128974

Unify AST Visitors with a macro like MIR Visitors #128974

maxcabrajac commented Aug 11, 2024 •

edited

Loading

rustbot commented Aug 11, 2024

This comment has been minimized.

lcnr commented Aug 12, 2024

lcnr commented Aug 12, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cjgillot left a comment

cjgillot Aug 27, 2024

maxcabrajac Aug 27, 2024 •

edited

Loading

maxcabrajac commented Aug 27, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

maxcabrajac commented Oct 15, 2024

maxcabrajac commented Oct 23, 2024 •

edited

Loading

petrochenkov commented Oct 23, 2024 •

edited

Loading

maxcabrajac commented Oct 23, 2024 •

edited

Loading

petrochenkov commented Oct 24, 2024

petrochenkov commented Oct 24, 2024

maxcabrajac commented Oct 24, 2024

rustbot commented Oct 24, 2024

petrochenkov commented Oct 24, 2024 •

edited

Loading

maxcabrajac commented Oct 24, 2024

petrochenkov commented Oct 24, 2024

bors commented Oct 24, 2024

Unify AST Visitors with a macro like MIR Visitors #128974

Are you sure you want to change the base?

Unify AST Visitors with a macro like MIR Visitors #128974

Conversation

maxcabrajac commented Aug 11, 2024 • edited Loading

rustbot commented Aug 11, 2024

This comment has been minimized.

lcnr commented Aug 12, 2024

lcnr commented Aug 12, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Aug 27, 2024

Choose a reason for hiding this comment

maxcabrajac Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

maxcabrajac commented Aug 27, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

maxcabrajac commented Oct 15, 2024

maxcabrajac commented Oct 23, 2024 • edited Loading

petrochenkov commented Oct 23, 2024 • edited Loading

maxcabrajac commented Oct 23, 2024 • edited Loading

petrochenkov commented Oct 24, 2024

petrochenkov commented Oct 24, 2024

maxcabrajac commented Oct 24, 2024

rustbot commented Oct 24, 2024

petrochenkov commented Oct 24, 2024 • edited Loading

maxcabrajac commented Oct 24, 2024

petrochenkov commented Oct 24, 2024

bors commented Oct 24, 2024

maxcabrajac commented Aug 11, 2024 •

edited

Loading

maxcabrajac Aug 27, 2024 •

edited

Loading

maxcabrajac commented Oct 23, 2024 •

edited

Loading

petrochenkov commented Oct 23, 2024 •

edited

Loading

maxcabrajac commented Oct 23, 2024 •

edited

Loading

petrochenkov commented Oct 24, 2024 •

edited

Loading