Add used_underscore_binding lint

[devonhollowood]

Implement #488

[devonhollowood]

One thing I noticed is that this lint, as it currently stands, is not quite compatible with the unused_variables built-in lint. This can be seen in tests/compile_fail/for_loop.rs, line 128:

let mut _index = 0;
for _v in &vec { _index += 1 } //~ERROR the variable `_index` is used as a loop counter         

Here, we consider the variable _index to be used, so we lint on the seconds line. However, the unused_variables lint considers _index to be unused (because nothing is done with it besides the increment), so if we change _index to index, it sets off that lint. I'm not sure how best to move our definition of a "used variable" inline with the unused_variables lint, but I'm guessing it will involve liveness analysis?

[Manishearth]

Looks good so far. I'd like to do something other than checking for __ though. And we need to see if we can avoid the mismatch with unused_variables. One way of doing that is to use an ExprUseVisitor. Another is to only lint when the path is on the right hand side, i.e. the parent expr is not an ExprAssign or += or whatever with the path on the lhs.

[oli-obk]

This should be y, not _y

[devonhollowood]

Okay, I made @oli-obk's suggested changes. I have a couple questions about @Manishearth's comments:

I'd like to do something other than checking for __ though.

I think we should still only lint single underscores, but I agree it would be more satisfying to have a better check here. Is there some way to check to see if something appears in a macro definition vs. in its expansion? I think it would make sense to lint the use of underscore-prefixed bindings within a macro definition, but not within its expansion.

And we need to see if we can avoid the mismatch with unused_variables. One way of doing that is to use an ExprUseVisitor. Another is to only lint when the path is on the right hand side, i.e. the parent expr is not an ExprAssign or += or whatever with the path on the lhs.

I think we probably want to do something more advanced than just checking to make sure the binding never appears on the LHS of an ExprAssign. For example, in

fn increment(_x: u32) -> u32 {
    _x + 1
}

we probably want to lint, but _x never appears on the LHS of an ExprAssign. I'd be happy to try using an ExprUseVisitor, but I can't really find any documentation on how to use it anywhere, and its methods seem to all be side-effect based so it's hard to follow what they do. If we want to use that, I would need some help, but I can always ask around on IRC for that. My other thought was to see how the built-in unused_variables lint does it, which would let us make sure we have parity. Unfortunately I'm having trouble finding the source for that lint in the rust compiler. If you let me know where to look for that I could use that approach instead.

[Manishearth]

we probably want to lint, but _x never appears on the LHS of an ExprAssign

Exactly! I was saying that we change the heuristics to:

• We have an ExprPath starting with only one underscore
• Its parent expr is not an ExprAssign or ExprAssignOp where the path doesn't appear on the RHS

All other Exprs "use" the variable, so we should lint in that case.

The built in analysis is in librustc/middle/liveness.rs (It's a hardwired lint), but IIRC it's intertwined with other analyses, so it's harder to figure out.

If you want to try ExprUseVisitor, you basically create a Delegate (see the escape analysis lint) impl, and check for uses of a variable with the borrow/consume methods. I don't think this is necessary here.

Is there some way to check to see if something appears in a macro definition vs. in its expansion?

If an Ident's name and hygenic name match then it wasn't created by a macro. I think. This stuff is in flux right now. I'm fine with checking for __ for the scope of this PR, but if you can find something better that works, that would be great 😄

[devonhollowood]

Exactly! I was saying that we change the heuristics to:

• We have an ExprPath starting with only one underscore
• Its parent expr is not an ExprAssign or ExprAssignOp where the path doesn't appear on the RHS

All other Exprs "use" the variable, so we should lint in that case

Ah okay that makes sense! I'll go implement that.

This still doesn't mean it's a local variable; you want to .get(&expr.id) and then match if the obtained def is a deflocal.

I'll go make this change, but I think the fact that I didn't already catch that I'm doing this wrong means my test coverage isn't good enough. I expanded test coverage by adding underscore-prefixed functions, structs, enums, and enum variants. All the above were correctly not linted with the code as is, so could you give me an example of something that the code as-is would lint (due to the DefLocal thing), but that it shouldn't lint?

[Manishearth]

something that the code as-is would lint (due to the DefLocal thing), but that it shouldn't lint?

Probably something like let x = foo_ where foo_ is a function. The DefLocal check isn't too important, really.

[devonhollowood]

Probably something like let x = foo_ where foo_ is a function.

I added a test for this, and it seems to work with the code as-is. It also seems to work with that check taken out completely.

[Manishearth]

Meh, get rid of that check then :)Manishearth/rust-clippy wrote:
Probably something like let x = foo_ where foo_ is a function.

I added a test for this, and it seems to work with the code as-is. It also seems to work with that check taken out completely.

—Reply to this email directly or view it on GitHub.

[devonhollowood]

Okay, I think the most recent pull requests address all the above issues. If this looks good to you too, I'll go ahead and rebase this all into one commit.

[Manishearth]

Oh I like this way of calculating is_used; smart 😄 !

[Manishearth]

Great work! Thanks!

[llogiq]

I think we may want to reuse some of the techniques in other lints. Great work indeed! 👍