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

[oli-obk]

In contrast to the MIR mut and immut visitors

rust/compiler/rustc_middle/src/mir/visit.rs

Lines 1235 to 1236 in c92a8e4

	make_mir_visitor!(Visitor,);
	make_mir_visitor!(MutVisitor, mut);

which are generated from a single macro, and are thus exactly the same except for mutation, the ast visitors are each handwritten. Their method naming scheme doesn't even match up, so changing from a Visitor to a MutVisitor requires extensive changes.

Work items

• Adjust names so everything has the visit_* vs walk_* naming scheme
• Give every MutVisitor::visit_* method a corresponding flat_map_* method. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
  • we can make it so implementors can override either visit_foo or flat_map_foo without there being problems. The walk_bar functions must invoke flat_map_foo, so that they can pick up on more/fewer items being produced. So walk_flat_map_foo (which only ever maps to a single output element) needs to invoke visit_foo, not walk_foo. Otherwise overriding visit_foo will do nothing.
  • This should be using macros similar to what [WIP] Try optimizing noop AST mut visiting #127371 did, so you don't repeat yourself
• (Optional): move all the walk_ functions to defaulted methods on the trait (not sure this should be done, discuss on zulip first!) discussed, should not be done
• Give every MutVisitor::visit_* method a corresponding Visitor method and vice versa E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
• Create a macro that generates both visitors, including the walk_ functions in one go, just like with the MIR visitors E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot.

Previous work

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

[GrigorenkoPV]

Give every MutVisitor::visit_* method a corresponding Visitor method and vice versa

• Missing in Visitor::visit_*
  • angle_bracketed_parameter_data
  • coroutine_kind
  • fn_decl
  • foreign_mod
  • id
  • macro_def
  • meta_item
  • meta_list_item
  • mt
  • parenthesized_parameter_data
  • qself
  • span
  • where_clause
• Missing in MutVisitor::visit_*
  • arm
  • assoc_item
  • enum_def
  • expr_field
  • expr_post
  • field_def
  • fn_ret_ty
  • foreign_item
  • generic_param
  • item
  • mac_def
  • param
  • pat_field
  • stmt
  • variant
  • variant_discr

All of these should be added, correct?

[oli-obk]

For the ones that only exist in Visitor but not in MutVisitor, it may be useful to see if anyone actually overrides these and potentially entirely remove them.

For the ones that only exist in MutVisitor but not Visitor, there are some that don't really make sense for Visitor, like the one for ids or the one for spans. There won't be any immutable visitors that just want to see all spans I think. So it seems ok to have such differences for leaf functions (where the Walker is a noop)

[oli-obk]

Also sometimes MutVisitor has things like ItemKind visiting, but not Item...

Looks like my idea was a bit naive. We basically need to analyze every missing method and see if there isn't one almost like it and if it can reasonably be refactored or if the mut visitor is just too different because of the mutation

[Borgerr]

Someone can correct me if I'm wrong, but I don't think claiming this issue is entirely appropriate, as it involves tracking sub-issues. I for one would be wanting to pick the low hanging fruit for the 4th suggestion for example, but leave the others alone.

[maxcabrajac]

I think I can begin to implement the macro that generates both traits and walk_* functions by merging one function at a time and seek mentoring to unify the more complex walk functions as needed. I'm thinking about making the macro generate a module so the path to walk_* functions stays the same.

[maxcabrajac]

I did some digging and this is what I found out:

• Missing in Visitor::visit_*
  • coroutine_kind
  • id
  • meta_item
  • meta_list_item
  • span
  • There is a walk_ but no visit_
    • fn_decl
    • qself
      • Normally the caller unwraps options, but not here (?)
  • Visitors do the same manually
    • angle_bracketed_parameter_data
    • parenthesized_parameter_data
    • foreign_mod
    • mt
    • where_clause
• Missing in MutVisitor::visit_*
  • enum_def
    • MutVisitors do the same manually
  • expr_post
    • Noop on default implementation
    • Only implementer is rustc_lint/src/early.rs. Looking at it, I think this might be a missplaced function as they have a bunch of *_post functions.
  • variant_discr
    • Default implementation is to delegate to visit_anon_const
    • Only implementer is rustc_resolve/src/late.rs. They use it to inject a AnonConstKind for a proper resolver. It seems like proper use.
  • fn_ret_ty
    • There is a walk_ but no visit_
  • Methods that already have a flat_map_ variant (also these are all flat_map_*s that exist (?)):
    • arm
    • assoc_item
    • expr_field
    • field_def
    • foreign_item
    • generic_param
    • item
    • param
    • pat_field
    • stmt
    • variant
• Visitor::mac_def and MutVisitor::macro_def
  • Signatures make me think they are supposed to be the same, but mac_def is a noop while macro_def is not (it recurses by using non-trait visit_ functions)
• Some functions have aditional parameters on Visitor
  • lifetime
  • path
  • use_tree
• Visitor::visit_precise_capturing_arg doesn't return V::Result. bug?

[maxcabrajac]

I was digging around to find out what to do with methods that have a flat_map_* variant and got confused... Why do flat_map_*s exist? All of them are just one to one conversions (their signatures are simalar to: fn(_: T) -> SmallVec<[T; 1]>). Is there a reason not to have just visit_*s and do something like what's done in mut_visit::{visit_vec, visit_thin_vec} or list.iter_mut().map(|t| vis.visit_t(t))?

[oli-obk]

There are implementors of MutVisitor that produce multiple items. E.g. when expanding a macro in item position, it may yield multiple items

[maxcabrajac]

Ohhh, now I get it. Sorry, I didn't read the docs for SmallVec well enough.

[noahmbright]

Is there a master list of what visit_* methods exist? If there isn't but there's a way to find all of them I could go through and do that. I think that would make it easier to track progress on each flat_map and if either the MutVisitor or Visitor method needs implemented. Without a centralized list I'm having trouble telling what's left, especially if the MutVisitor should be treated differently from the immutable one

[maxcabrajac]

Hi @noahmbright!
Using rustdoc is a good way of getting a master list of what each trait offers.
I did a bunch of work in #128974 and tackled the last two tasks of this issue (you can have look at it there, your input would be greatly appreciated!). One of the things I did is a macro that quickly writes flat_map and walk_flat_map implementations, so doing the first task after #128974 is merged would be more of a chore than anything.

I didn't do it immediately as I think it deserves a bit more discussion. Sorry for not starting it until now. =/

I noticed that every type whose visit_ has a flat_map_ variant in MutVisitor is always stored in the AST as a list, so 1-to-many conversions make perfect sense. On the other hand, types whose visit_ doesn't have a flat_map_ variant are stored directly most of the time, so 1-to-1 conversions are required.
At first, I thought that adding flat_map_ to anything that is stored at least once in a list and use flat_map_ whenever possible would be a good idea, but the very next day I thought that this way of doing things requires the implementer to know of this distinction between 1-to-1 visit_ functions and 1-to-many flat_map_s and to know when each of them is be called by the parent node's walk_.
Should we make the distinction between required 1-to-1 conversions and possible 1-to-many conversions? Or should we leave it as is, allowing flat_map_ only when it is always used?

[lucarlig]

not entirely clear what is left to do, which easy issue is still left, and how can I claim it?