immutable loop condition lint gives up on mutated upvars

[ExpHP]

Currently the immutable loop condition lint gives up on code like the following, where the variable being mutated is an upvar:

let (mut a, mut b) = (0, 0);

// this should be okay
// (and it is, but only because clippy gives up; it doesn't
//  know how to compare the 'a' in the body to the 'a' in the condition)
let mut f = || {
    while a < 5 {
        a += 1;
    }
};
f();

// this should produce an error (but clippy doesn't even try)
let mut f = || {
    while a < 5 {
        b += 1;
    }
};
f();

This is a known problem with a FIXME in the source, but I think that fixing it (or at least, discovering how to fix it) will be a useful step towards implementing #2914, which requires much more reliable ways of discovering whether a given variable is used.

[ExpHP]

Notes on my failed noobish attempt to fix this:

• The currently documented reasoning for giving up is that a NodeId can't be obtained from an UpvarId.
• However... it is easy enough to obtain an HirId instead. Any existing code path in clippy_lints/src/utils/usage.rs with access to a NodeId can just call cx.tcx.hir.node_to_hir_id(id), and the Categorization::Upvar branch can use upvar.id.var_id.
• (with this change, mutated_variables will always succeed and no longer needs to return Option)

If you try this, you'll find that it does fix the above examples... but it is still easily defeated by the following simple example, which becomes a false positive: (very ungood for a deny-by-default lint, so I won't be submitting a PR 😝 !)

let (mut a, mut b) = (0, 0);

while a < 5 {  // <-- the HirId we get here is for the local var `a`
    let mut f = || {
        a += 1;  // <-- ...which doesn't match the one here, which is an upvar.  Oops!
    };
    f();
}

I find it really hard to believe that there's no way to figure out the local that an Upvar ultimately corresponds to. There's gotta be a way... right?

[flip1995]

I think I did the "fix" of bailing out on Upvars..

I find it really hard to believe that there's no way to figure out the local that an Upvar ultimately corresponds to. There's gotta be a way... right?

If I remember correctly this was the point where I failed fixing it and decided to bail out...

Edit: Yep #2572

[ExpHP]

I think I am losing my mind here

• The UpvarId has the closure's LocalDefId.
• LocalDefId can be converted to a DefId.
• TyCtxt has methods for obtaining the Mir of a DefId. (defined via macro in rustc::ty::query)
• a Mir has an Vec<UpvarDecl> which I think is constructed with HirIds from the outer scope? I'm really not sure... (they're constructed in librustc_mir/build/mod.rs)
• The indices into this vec are...
  the indices... are...
  ...I don't know. Some places that access it use values of a newtyped index called Field. I don't know how to get one.

😫 🆘

[ExpHP]

I am beginning to believe that the compiler does not have this functionality implemented anywhere because it does not need it. Everything the compiler cares about is integrated directly into borrowck or similar, probably so that it can be computed efficiently.

Consider some of the specific error messages the compiler gives:

|| {
    let x = vec![()];
    || {
        || {
            x.sort();
        };
    };
};

   Compiling playground v0.0.1 (file:///playground)
error[E0595]: closure cannot assign to immutable local variable `x`
 --> src/main.rs:5:9
  |
4 |         let x = vec![()];
  |             - consider changing this to `mut x`
5 |         || {
  |         ^^ cannot borrow mutably

Pay close attention and you'll notice that the spans in the error suggest local reasoning on just the statements/expressions that lie immediately in the body of the first closure (treating the nested closures as opaque). It is not the let x and the x += 1 that it points to, but rather, the let x and a closure.

---

In another example, the compiler does evidently see through the layers of closures:

move || {
    let x = vec![()];
    move || {
        move || {
            vec![0,1,2].into_iter().map(|_| drop(x));
        }
    }
};

error[E0507]: cannot move out of captured outer variable in an `FnMut` closure
 --> src/main.rs:7:54
  |
4 |         let x = vec![()];
  |             - captured outer variable
...
7 |                 vec![0,1,2].into_iter().map(|_| drop(x));
  |                                                      ^ cannot move out of captured outer variable in an `FnMut` closure

The "captured outer variable" comes from the note field on a cmt, which sounds like literally the most unstable thing ever and almost certainly not something that we should use or rely on.

---

I do still hope this is just me being a noob and not seeing the solution right under my nose. Because otherwise it does not bode well for #[clippy::close]...

[flip1995]

Your investigations go way beyond what I tried back then. 😄 Which means I'm afraid, that I can't help you much here...

Anyway, maybe this could help: There is a method for getting the ast::NodeId from a hir::def_id::DefId: tcx.hir.as_local_node_id(def_id). I'm not sure if this will return the NodeId that we're looking for, though.

Maybe this also helps, in case you haven't found it yet: rustc-guide: Identifiers in the HIR

Big Thanks for taking your time and looking into this! 👍