Change HIR visitor pattern

[P-E-P]

HIR is in a bad state, multiple efforts could be started to improve the current situation. One of those could reduce the amount of code by removing boilerplate code within HIR visitors.

Most visitors currently have to redefine over and over traversal functions even when the visitor per se should only act on a few components.

Some proposals have been made to improve the general state of visitors within the project:

• https://github.com/Rust-GCC/gccrs/pull/2904#issuecomment-1995337505
• Separate walk from visit
• Provide a default walk/visit implementation to fall back on when no implementation is provided.

In the long run we should probably separate visit from walk in all our visitors but this requires an extensive refactor of multiple parts of the code base and multiple question remains (some AST visitors require infix operations).

We already provide an interface for HIR visitors: HIRFullVisitor.

• [ ] Take a look at the AST visitor mechanisms (ASTDefaultVisitor, ASTVisitor, ASTContextualVisitor...)
• [ ] Provide a default implementation for the HIR visitor.
• [ ] Remove empty visits functions from HIR visitor implementors
• [ ] Identify implementations in HIR visitor implementors where it could be replaced by a single call to parents' visit function.

[tamaroning]

Hi, I recently encountered this issue and reached the same opinion that walk and visit should be separated. https://github.com/Rust-GCC/gccrs/pull/3115#discussion_r1747104532

multiple question remains (some AST visitors require infix operations)

Do you mean mutating AST nodes by "infix" operations?

[P-E-P]

Hi, I recently encountered this issue and reached the same opinion that walk and visit should be separated. #3115 (comment)

multiple question remains (some AST visitors require infix operations)

Do you mean mutating AST nodes by "infix" operations?

We really need to be sure to where we're heading so I'll put a maximum of informations here from what has been discussed previously:

• We need a default traversal operation ("walk")
  • to prevent huge file of copy/paste children visitor dispatch
  • or worse empty implementation
    • not visiting everything is faster but this means in the future modifying a child node might lead to unexpected results: the node won't be visited and recreating the traversal will be prone to errors
• in #2904 @GuillaumeGomez suggested that we separate walk and visit
  • this would clear up visit functions and separate mutations/actions done around the AST
    • I agree on those points but something is off: it puts some constraints on the traversal type
      • We could provide visit_prefix and visit_suffix instead of visit but I feel it is not "worth" the additional complexity
      • Not all traversals are prefix/suffix: some visitors make use of infix (rust ast collector), infix on N-tree means we cannot provide an easy interface
        • this edge case could be solved by providing a way to override "walk" (unexpected)
• @CohenArthur would like in the long run to switch to a system with less side effects, every visit function should return it's own modifications, errors would be reported as error nodes instead of being collected alongside the traversal.
  • Incompatible with some visitors ? (Globbing visitor ?)
• some bits are currently processed though visit but are not part of the visitor, it is incoherent

Our trees are quite big, this means any move will take some time to implement and will be tedious.

[CohenArthur]

@CohenArthur would like in the long run to switch to a system with less side effects, every visit function should return it's own modifications, errors would be reported as error nodes instead of being collected alongside the traversal.

for what it's worth I'm not sure this is a good idea considering C++. this pattern works very well in Rust because move semantics are the default, and make sense. but it is a lot more annoying to do in C++

I really think that having separate walks and visits functions could clean things up, but it would be a huge change

[P-E-P]

For now we should probably focus on the initial goal of this PR: provide a default visitor, we'll then be able to move it to some walk funcitons in another issue/pr.

[Kamiinarii78]

@P-E-P Can you assign it to me ?