Skip to content

refactor used_mut_nodes result #42384

Closed
Closed
@nikomatsakis

Description

@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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-incr-compArea: Incremental compilationC-cleanupCategory: PRs that clean code up or issues documenting cleanup.E-mediumCall for participation: Medium difficulty. Experience needed to fix: Intermediate.E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions