Refactor visit.rs with a `Visitor` trait #7081

pnkfelix · 2013-06-12T11:52:38Z

Currently libsyntax/visit.rs provides a record that holds a collection of @fn's, using a mix of lambda-lifting and an explicit virtual method table to define traversals over the AST.

pub struct Visitor<E> {
    visit_mod: @fn(&_mod, span, node_id, E, vt<E>),
    visit_view_item: @fn(@view_item, E, vt<E>),
    ...
}

where

pub enum vt<E> { mk_vt(visitor<E>), }

So then a use of the visitor looks like this (taken from freevars.rs):

    let v = visit::mk_vt(@visit::Visitor {visit_item: ignore_item,
                                          visit_expr: walk_expr,
                                          .. *visit::default_visitor()});
    (v.visit_block)(blk, 1, v);

where ignore_item and walk_expr are defined accordingly.

The above is awkward. A more natural way to encode this would be via a Visitor trait that defined the visit methods; then instances of the trait would correspond to the accumulated state denoted above by E in the struct definition and 1 in the freevars example.

(I originally had written a @Visitor object, but there is no need to commit to using objects in the Visitor interface.)

The text was updated successfully, but these errors were encountered:

pnkfelix · 2013-06-12T15:20:35Z

(The original description for this ticket used an @visitor object, but that was not what Niko intended. I'm rewriting the description now to reflect a more generic approach.)

nikomatsakis · 2013-06-12T15:20:59Z

I don't think we should use objects here. I envision something like this:

trait Visitor {
    fn visit_expr(&mut self, expr: @ast::expr) {
        super_expr(self, expr);
    }
    fn visit_fn(&mut self, ...);
}

// It occurs to me that we have to break "super functions" into their own
// top-level functions if we want them to be callable.
fn super_expr<V:Visitor>(
    this: &mut V,
    expr: @ast::expr)
{
        match expr.node {
            ast::expr_binary(op, left, right) => {
                self.visit_expr(left); self.visit_expr(right);
            }
            ...
        }
}

and then we implement individual passes follows:

struct MyContext {
    tcx: ty::ctxt, ...
}

impl visit::Visitor for MyContext {
    fn visit_expr(&mut self, expr: @ast::expr) {
        visit::super_expr(self, expr);
    }
}

nikomatsakis · 2013-06-12T15:24:06Z

Incidentally, @msullivan has been working on fixing bugs in default methods. For all I know this approach may be feasible now, or there may be bugs standing in the way.

This step 1 in my plan for issue #7081: Refactor visit.rs with a `@Visitor` trait.

pnkfelix · 2013-06-15T11:22:29Z

Default methods, which block this, are tracked in #2794

sanxiyn · 2013-07-04T01:12:36Z

Referencing @Aatch's branch here.

pnkfelix · 2013-07-23T06:05:54Z

So apparently we have @pnkfelix working on this problem on branches labelled fsk-issue7081 like this, as well as @Aatch's branch that sanxiyn mentioned above, and now there is also @pcwalton working on a visit-trait branch as well, as he announced in the #rust irc channel.

Though I think pcwalton's code is putting in an @Trait style visitor; Niko previously pointed out on this isssue that it shouldn't be necessary to use an @Trait object here.

pnkfelix · 2013-07-23T07:30:38Z

�One detail I meant to include in my last comment: the rewrite from my branch is attempting to support the transition from (E, vt<E>) to VisitorTrait in an incremental fashion, by putting in a impl<E> VisitorTrait for (E, vt<E>) as part of visit.rs.

I saw such incremental development as a virtue of my strategy, but its possible that getting that right is just slowing my progress down.

nikomatsakis · 2013-07-24T01:21:25Z

Indeed. If we're going to rewrite all the visitor passes, it'd be a shame not to remove the managed boxes!

huonw · 2013-07-24T01:34:08Z

I guess having more info is useful: a while ago I used @Aatch's branch to start rewriting lint.

pnkfelix · 2013-08-14T20:34:20Z

Okay, I've ported all of the visitor passes in a pretty mechanical fashion. The results of the mechanical rewrites are not pretty, nor are they meant to be.

I'm going to land this in a couple of incremental pull requests. The first few will be focused on landing the mechanical rewrites, with an emphasis on diffs that make it semi-obvious that they are refactorings. Code-cleanup will come later, on a file by file basis.

@nikomatsakis

…r=nikomatsakis Rewriting visit.rs to operate on a borrowed `&mut V` where `<V:Visitor>` r? @nikomatsakis r? @pcwalton This is the first in a planned series of incremental pull requests. (There will probably be five pull requests including this one, though they can be combined or split as necessary.) Part of #7081. (But definitely does *not* complete it, not on its own, and not even after all five parts land; there are still a few loose ends to tie up or trim afterwards.) The bulk of this change for this particular PR is pnkfelix/rust@3d83010, which has the changes necessary to visit.rs to support everything else that comes later. The other commits are illustrating the standard mechanical transformation that I am applying. One important point for nearly *all* of these pull requests: I was deliberately *not* trying to be intelligent in the transformation. * My goal was to minimize code churn, and make the transformation as mechanical as possible. * For example, I kept the separation between the Visitor struct (corresponding to the earlier vtable of functions that were potentially closed over local state) and the explicitly passed (and clones) visitor Env. I am certain that this is almost always unnecessary, and a later task will be to go through an meld the Env's into the Visitors as appropriate. (My original goal had been to make such melding part of this task; that's why I turned them into a (Env, vtable) tuple way back when. But I digress.) * Also, my main goal here was to get rid of the record of `@fn`'s as described by the oldvisit.rs API. (This series gets rid of all but one such case; I'm still investigating that.) There is *still* plenty of `@`-boxing left to be removed, I'm sure, and even still some `@fn`'s too; removing all of those is not the goal here; its just to get rid of the encoded protocol of `@fn`'s in the (old)visit API. To see where things will be going in the future (i.e., to get a sneak-preview of future pull-requests in the series), see: * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step1 (that's this one) * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step2 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step3 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step4 * https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step5 * Note that between step 4 and step 5 there is just a single commit, but its a doozy because its the only case where my mechanical transformation did not apply, and thus more serious rewriting was necessary. See commit pnkfelix/rust@da902b2

pnkfelix · 2013-08-19T06:56:52Z

To see where things will be going in the future (i.e., to get a sneak-preview of future pull-requests in the series), see:

https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step1 (that's PR Visitor trait rewrite, step 1. #8527; done)
https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step2 (that's PR visitor trait rewrite step2 #8539; done)
https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step3 (PR visitor trait rewrite step 3 (of 5). #8619; done)
https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step4 (PR visitor trait rewrite step 4 (of 5). #8623; done)
https://github.com/pnkfelix/rust/commits/fsk-visitor-vpar-defaults-step5 (PR visitor trait rewrite step 5 (of 5). non-mechanical #8638)
- Note that between step 4 and step 5 there is just a single commit, but its a doozy because its the only case where my mechanical transformation did not apply, and thus more serious rewriting was necessary. See commit pnkfelix/rust@da902b2

@nikomatsakis

…r=graydon,nikomatsakis r? @nikomatsakis Follow up to #8527 (which was step 1 of 5). See that for overall description Part of #7081

…r=nmatsakis Follow up to #8539 (step 2 of 5). (See #8527, which was step 1 of 5, for the full outline.) Part of #7081.

…r=nmatsakis Follow up to #8619 (step 3 of 5). (See #8527, which was step 1 of 5, for the full outline.) Part of #7081.

@huonw

…r=huonw "non-mechanical" : there was lots more hacking than the other more-mechanical ports Felix did. r? @huonw. (Or @nikomatsakis ; I just want someone to sanity-check this. Its not a thing of beauty.) Followup to #8623. (See #8527, which was step 1 of 5, for the full outline. Part of #7081.) Notes on the change follow. There's also a strange pattern that I hacked in to accommodate the Outer/Inner traversal structure of the existing code (which was previously encoding this by untying the Y-combinator style knot of the vtable, and then retying it but superimposing new methods that "stop at items"). I hope either I or someone else can come back in the future and replace this ugliness with something more natural. Added boilerplate macro; all the OuterLint definitions are the same (but must be abstracted over implementing struct, thus the macro). Revised lint.rs use declarations to make ast references explicit. Also removed unused imports.

msullivan · 2013-08-22T23:30:55Z

So is this done now?

pnkfelix · 2013-08-23T06:21:18Z

No it turns out there are a couple more oldvisit clients that need to be ported. One I knew about, the others I somehow overlooked before.

Further followup on #7081. There still remains writeback.rs, but I want to wait to investigate that one because I've seen `make check` issues with it in the past.

…Ctxt,Visitor}.

…rLoan{Ctxt,Visitor}.

…text,Visitor}.

r? anyone Remove some trivial Visitor structs, using their non-trivial Contexts as the Visitor implementation instead. Removed a little bit of `@boxing` as well. Part of ongoing work on #7081.

sanxiyn · 2013-09-13T11:41:27Z

Is this done now?

pnkfelix · 2013-09-13T11:51:58Z

@sanxiyn I think it would be good to first finish unifying as many Ctxt+Visitors as possible, as illustrated in the commits with log messages of the form unify .*{C(on)?t(e)?xt,Visitor}

…s, r=alexcrichton r? anyone. Part of #7081. More refactorings of the syntax::visit::Visitor implementations, folding so-called "environments" into the visitor impl when the latter was previously a trivial unit struct. As usual, this refactoring only applies when the environments are not actually carrying state that is meant to be pushed and popped as we traverse the expression. (For an example where the environment *isn't* just passed through, see the `visit_fn` in `liveness.rs`.) Got rid of a bit of @-allocation in borrowck. Both cases should be pure-refactorings.

pnkfelix · 2013-09-24T10:41:15Z

As a step towards closing this bug, here is a list of impls of visit::Visitor, and whether they have trivial environments (Visitor<()>) or cannot have trivial environments currently (with an explanation why in the latter case). (This is assuming PR #9453 lands.)

I spent a while this morning/afternoon looking at the typeck/regionck cases below. Unfortunately the assumption that we have a @mut FnCtxt goes pretty deep into the code, and the most immediate refactorings I applied hit some snags, the first being in vtable::early_resolve_expr, where we have a loop over type substitutions in the fcx while we also modify the vtables of the fcx. (And that is just the first snag.) So resolving @mut FnCtxt properly may be a worthy of a separate ticket. (Then again, the more conservative approach of just getting rid of VtableResolveVisitor, without worrying about the @mut itself, may still be viable.) I'll look at the others indendent of FnCtxt later.

middle/typeck/check/regionck.rs:167:impl Visitor<@mut Rcx> for RegionckVisitor
middle/typeck/check/vtable.rs:776:impl visit::Visitor<@mut FnCtxt> for VtableResolveVisitor
middle/typeck/check/writeback.rs:323:impl Visitor<@mut WbCtxt> for WbVisitor

The lints are a special case; I suspect this code needs a rewrite anyway, and its probably not worth trying to further bandaid it with refactorings like this. Still, here's the list.

…ir visitor structs.

…-typeck, r=huonw r? anyone Also got rid of a bit of `@mut` allocation. (Though not the monster that is `@mut FnCtxt`; that case is documented already on #7081; if we attack it, it will probably be its own ticket, not part of #7081.)

@allocation

…er, r=huonw r? anyone Part of #7081. Removed many unnecessary context arguments, turning them into visitors. Removed some @allocation. If this lands, then I think the only thing left that is unaddressed are: * the various lint visitors, and * middle/privacy.rs, which has `impl<'self> Visitor<&'self method_map> for PrivacyVisitor`

pnkfelix · 2013-09-25T16:06:03Z

okay, looks like just the privacy.rs and the lints are left.

And privacy isn't as hard to deal with as I had thought it would be (I should have tried harder to get it into that last PR). I'll probably close this ticket after I put up a PR for privacy.

pnkfelix · 2013-09-25T16:21:38Z

@alexcrichton is going to be mucking with the privacy code, so I'm going to yield that battle ground to him and close this ticket. :)

pnkfelix mentioned this issue Jun 12, 2013

Visitor refactoring: Step 1, couple (Env, vt<Env>) together in a tuple. #7084

Closed

ghost assigned pnkfelix Jun 12, 2013

bors added a commit that referenced this issue Jun 12, 2013

auto merge of #7084 : pnkfelix/rust/fsk-visitor-refactoring, r=pnkfelix

df483e8

This step 1 in my plan for issue #7081: Refactor visit.rs with a `@Visitor` trait.

pnkfelix mentioned this issue Jun 16, 2013

default method of T that calls generic fn<S:T>(&S) on self does not work #7183

Closed

pnkfelix mentioned this issue Aug 15, 2013

Visitor trait rewrite, step 1. #8527

Closed

pnkfelix mentioned this issue Aug 15, 2013

visitor trait rewrite step2 #8539

Closed

bors added a commit that referenced this issue Aug 19, 2013

auto merge of #8539 : pnkfelix/rust/fsk-visitor-vpar-defaults-step2, …

d597f54

…r=graydon,nikomatsakis r? @nikomatsakis Follow up to #8527 (which was step 1 of 5). See that for overall description Part of #7081

pnkfelix mentioned this issue Aug 19, 2013

visitor trait rewrite step 3 (of 5). #8619

Merged

bors added a commit that referenced this issue Aug 19, 2013

auto merge of #8619 : pnkfelix/rust/fsk-visitor-vpar-defaults-step3, …

2246d56

…r=nmatsakis Follow up to #8539 (step 2 of 5). (See #8527, which was step 1 of 5, for the full outline.) Part of #7081.

pnkfelix mentioned this issue Aug 19, 2013

visitor trait rewrite step 4 (of 5). #8623

Closed

bors added a commit that referenced this issue Aug 19, 2013

auto merge of #8623 : pnkfelix/rust/fsk-visitor-vpar-defaults-step4, …

e6e678f

…r=nmatsakis Follow up to #8619 (step 3 of 5). (See #8527, which was step 1 of 5, for the full outline.) Part of #7081.

pnkfelix mentioned this issue Aug 20, 2013

visitor trait rewrite step 5 (of 5). non-mechanical #8638

Closed

pnkfelix mentioned this issue Sep 3, 2013

porting writeback.rs to new <V:Visitor> injects rt/uv failure #8791

Closed

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 10, 2013

Remove oldvisit.rs (yay!); part of rust-lang#7081 refactoring.

8b00004

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 10, 2013

check_loans.rs Visitor refactoring (rust-lang#7081): unify CheckLoan{…

959d9d6

…Ctxt,Visitor}.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 10, 2013

gather_loans/mod.rs Visitor refactoring (rust-lang#7081): unify Gathe…

6724317

…rLoan{Ctxt,Visitor}.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 10, 2013

middle/entry.rs Visitor refactoring (rust-lang#7081): unify Entry{Con…

ed37da2

…text,Visitor}.

pnkfelix mentioned this issue Sep 10, 2013

Some Visitor+Context unifications #9094

Closed

This was referenced Sep 14, 2013

Refactor libsyntax Visitor impls to use default methods. #9181

Closed

Get rid of unused SimpleVisitor stuff. #9187

Closed

A little more libsyntax cleanup #9200

Closed

pnkfelix mentioned this issue Sep 24, 2013

#7081: Further syntax::visit refactors #9453

Merged

pnkfelix added a commit to pnkfelix/rust that referenced this issue Sep 24, 2013

Part of rust-lang#7081: Fold remainder of typeck's visit env into the…

0e95c34

…ir visitor structs.

pnkfelix mentioned this issue Sep 24, 2013

Part of #7081: Fold remaining typeck's visit envs into visitor structs #9463

Merged

pnkfelix mentioned this issue Sep 25, 2013

#7081 syntax::visit refactor remainder #9492

Merged

pnkfelix closed this as completed Sep 25, 2013

pnkfelix mentioned this issue Oct 27, 2013

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

Closed

pnkfelix removed their assignment Jun 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor visit.rs with a `Visitor` trait #7081

Refactor visit.rs with a `Visitor` trait #7081

pnkfelix commented Jun 12, 2013

pnkfelix commented Jun 12, 2013

nikomatsakis commented Jun 12, 2013

nikomatsakis commented Jun 12, 2013

pnkfelix commented Jun 15, 2013

sanxiyn commented Jul 4, 2013

pnkfelix commented Jul 23, 2013

pnkfelix commented Jul 23, 2013

nikomatsakis commented Jul 24, 2013

huonw commented Jul 24, 2013

pnkfelix commented Aug 14, 2013

pnkfelix commented Aug 19, 2013

msullivan commented Aug 22, 2013

pnkfelix commented Aug 23, 2013

sanxiyn commented Sep 13, 2013

pnkfelix commented Sep 13, 2013

pnkfelix commented Sep 24, 2013

pnkfelix commented Sep 25, 2013

pnkfelix commented Sep 25, 2013

Refactor visit.rs with a Visitor trait #7081

Refactor visit.rs with a Visitor trait #7081

Comments

pnkfelix commented Jun 12, 2013

pnkfelix commented Jun 12, 2013

nikomatsakis commented Jun 12, 2013

nikomatsakis commented Jun 12, 2013

pnkfelix commented Jun 15, 2013

sanxiyn commented Jul 4, 2013

pnkfelix commented Jul 23, 2013

pnkfelix commented Jul 23, 2013

nikomatsakis commented Jul 24, 2013

huonw commented Jul 24, 2013

pnkfelix commented Aug 14, 2013

pnkfelix commented Aug 19, 2013

msullivan commented Aug 22, 2013

pnkfelix commented Aug 23, 2013

sanxiyn commented Sep 13, 2013

pnkfelix commented Sep 13, 2013

pnkfelix commented Sep 24, 2013

pnkfelix commented Sep 25, 2013

pnkfelix commented Sep 25, 2013

Refactor visit.rs with a `Visitor` trait #7081

Refactor visit.rs with a `Visitor` trait #7081