Skip to content

visit.rs: consider making visit_ty default be a walk_ty #10108

New issue

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

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

Already on GitHub? Sign in to your account

Closed
pnkfelix opened this issue Oct 27, 2013 · 2 comments
Closed

visit.rs: consider making visit_ty default be a walk_ty #10108

pnkfelix opened this issue Oct 27, 2013 · 2 comments
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR)

Comments

@pnkfelix
Copy link
Member

Most of the Visitor default methods in visit.rs do a walk over the substructure of the ast node being visited. E.g. a sample of the code in visit.rs:

    fn visit_item(&mut self, i:@item, e:E) { walk_item(self, i, e) }
    fn visit_local(&mut self, l:@Local, e:E) { walk_local(self, l, e) }
    fn visit_block(&mut self, b:&Block, e:E) { walk_block(self, b, e) }

but there are exceptions, and one of them is visit_ty:

    fn visit_ty(&mut self, _t:&Ty, _e:E) { }

I deliberately made that function a no-op when I introduced the Visitor refactoring (#7081), because I was attempting to do a pure refactoring of the existing record-of-closures syntax visitor, which also skipped traversing types by default, see e.g. oldvisit.rs.

@jbclements recently asked why we do this, and I do not have an answer. At first I thought "well maybe we want to force people to opt-into traversing over type structure, since in the common case you'll only want to look at value-producing expressions in many visitors." The problem with that logic is that we do allow some expressions within a type expression (namely const expressions, such as within the bounds of a fixed-length vector type), and thus if you want to traverse all of the subexpressions, you still need to traverse the types.

One tricky side of this is that I think in my early attempts at the visit.rs refactoring, I did attempt to include ty-traversal as the visit_ty default, and this led to some sort of errors. (i don't recall what sort of errors; all I remember is eventually narrowing the problem down to the visit_ty implementation, and at that point I threw my hands up.)

But even so, we can easily change all of the existing visitor implementations that don't override the default visit_ty to now override it with a no-op. That would trivially lead the way to making visit_ty call walk_ty, which might be a better default behavior for future visitor code.

@jbclements
Copy link
Contributor

+1

@alexcrichton
Copy link
Member

This was enabled in #10862, and #10894 is tracking the remaining locations where this caused problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR)
Projects
None yet
Development

No branches or pull requests

3 participants