Skip to content

Detect situations where a visitor implementation is skipped by accidentally directly calling the corresponding walk function #129859

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
compiler-errors opened this issue Sep 1, 2024 · 0 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Sep 1, 2024

For the AST (but also other visitors in the compiler), visitor functions typically come in two flavors:

  • visit_X - Overrideable part of the trait invocation. If you override it, you should call walk_X if you want to recurse or do visit_Y calls on the sub-components (for some other Y) you want to walk into, skipping the rest.
  • walk_X - Non-overrideable, free function which actually recurses on the structure by calling visit_Y on all of the sub-components (for some other Y) of the thing. Sometimes this is called super_visit_X.

It is typically a bug to call walk_Y on a sub-component within a visit_X invocation. For example, see:

#129858

Which fixed a bug where we called walk_expr on the async closure's body when visiting the async closure in visit_fn. This should've been a visit_expr, since visit_expr had meaningful side-effects (collecting macro invocations).

We should have some sort of lint to prevent this pattern outside of regular visit_X -> walk_X situation, and compiler devs who need to do something special can do #[allow(whatever_lint_name_for_sketchy_visitor_pattern)] to justify that what they're doing is okay. We already have some comments that document when this happens (I think, I feel like I've seen them around), but it can be done by accident which causes bugs!

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 1, 2024
@fmease fmease added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 1, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants