refactor used_mut_nodes result

[nikomatsakis]

At present, when you run borrowck, it also has the side-effect of modifying the used_mut_nodes set in the tcx:

    /// Set of nodes which mark locals as mutable which end up getting used at
    /// some point. Local variable definitions not in this set can be warned
    /// about.
    pub used_mut_nodes: RefCell<NodeSet>,

This set is used later by the lint machinery: any mut declaration not in this set gets a warning.

This is not good, because that field is not tracking by the incremental system. The other problem is just that this very setup -- one big set -- is sort of anti-incremental, since we can't readily track which contributions to the set came from where.

I think we should refactor this so that the borrowck query, instead of yielding a unit result, as it does now, returns a NodeSet containing the list of used mut nodes within the body. Note that whether a mut is used is purely determinde by the function it is in, so we don't need to combine results from multiple borrowck results.

In other words, we would remove that field from the tcx and change the entry in src/librustc/ty/maps.rs for the borrowck from this:

    [] borrowck: BorrowCheck(DefId) -> (),

to something like this:

    [] borrowck: BorrowCheck(DefId) -> Rc<BorrowCheckResult>,

where BorrowCheckResult would be a new struct defined (presumably) in src/librustc/ty.rs, looking something like this:

struct BorrowCheckResult {
    /// contains the node-ids for variables within this function where the `mut`
    /// declaration was used in some way (e.g., by modifying the variable's value,
    /// or taking an `&mut` borrow of it).
    used_mut_nodes: NodeSet
}

We would then modify the borrow checker code (borrowck() in src/librustc_borrowck/borrowck/mod.rs) so that instead of modifying the tcx directly, it modifies a set defined per-fn, and constructs the struct when it is done.

Finally, we would have to modify the lint code that currently reads from this set (found in src/librustc_lint/unused.rs). This is a bit annoying, since late's don't seem to naturally want to carry state, and we don't have quite the helpers you would need to make it stateless.

One option -- adding state -- would be to add a field to the UnusedMut struct that tricks the innermost body (it's type might be Vec<Rc<BorrowCheckResult>>). Then we would want to modify the LateLintPass impl, which implements a "visitor-like" pattern, where you get callbacks as we descend the HIR. We would want to override check_body, so that each time we enter a fn body (etc), we can load up the borrowck results for that fn (we can keep them in a stack). Something like this:

fn check_body(&mut self, cx: &LateContext, body: &'tcx hir::Body) {
    // Get the def-id of the owner of this body, typically a function.
    let body_owner_def_id = cx.tcx().hir.body_owner_def_id(body.id());
    
    // Get the borrowck tables.
    self.borrowck_tables.push(cx.tcx().borrowck(body_owner_def_id));
}

fn check_body_post(&mut self, cx: &LateContext, body: &'tcx hir::Body) {
    self.borrowck_tables.pop();
}

then we can check whether a given mut binding is "used" by invoking self.borrowck_tables.last().unwrap().contains(...) (the vector ought to be non-empty, I think, though that could be wrong for cases like function parameters).

[nikhilshagri]

I'll take this up!

[nikomatsakis]

@cynicaldevil great! Please let me know how you're doing. I try to monitor GitHub notifications, but if you want a quick answer to a question, I find it works best if you ping me on IRC (if you have a persistent connection, especially) or else gitter. And of course there is also #rustc on IRC.

[nikomatsakis]

@cynicaldevil how goes it?

[nikhilshagri]

Still working on it, I have a few questions to ask via IRC later :)

[nikhilshagri]

@nikomatsakis I've changed the borrowck entry in define_maps to return Rc<BorrowCheckResult>.
Now, I'm refactoring out the used_mut_nodes field, which I've removed. For example, I've changed self.tcx().used_mut_nodes.borrow_mut().insert(local_id); to self.tcx().borrowck(_).get_mut().used_mut_nodes.insert(local_id);. Problem is, the borrowck call requires a DefId argument, and I can't figure out how to obtain one.

[nikomatsakis]

@cynicaldevil hmm, that doesn't sound quite right. I would expect borrowck to return an immutable result, for one thing. Maybe post a link to your WIP branch and I can take a look?

[nikhilshagri]

@nikomatsakis nikhilshagri@9713020

[nikomatsakis]

@cynicaldevil gah sorry, please do ping me on IRC or gitter in the future if I have overlooked a comment of yours! I left these notes, hope it is helpful.

[nikhilshagri]

I've modified the commit acc. to comments: nikhilshagri@df15479, and I've started working on the lint code now.