Suggest for loop instead of while-let when looping over iterators

[fhartwig]

Fixes #387
I think this is ready for review. I even remembered to update the readme for once 😄

[fhartwig]

Argh. I just realised that I misread rust-lang/rust#8372 . This issue means that there are cases where while let is actually what you want (I thought that for had been fixed). I guess I will have to also check that the iterator isn't used in the loop body to avoid false positives.

[Manishearth]

Since these examples depend on a temporary, they are all actually infinite loops 😄

I'd prefer if we used vector iteration here or stored the ranges in an external mutable variable.

[fhartwig]

I'm not sure what I was thinking there 😆 Thanks, I'll fix that.

[fhartwig]

I've made fixes for your comments, fixed the false positives where the iterator is used inside the loop body, and rebased the branch onto the current master. Ready for more feedback 😄

[llogiq]

This looks wrong.

[fhartwig]

Looks like I messed up when resolving a conflict for the rebase. Fixed.

[llogiq]

Happens to the best of us :-)

[fhartwig]

Regarding the discussion about iterators being used after the while-let loop:

On the other hand, it shouldn't be very hard to fix.

Ok, I have to eat my words here, I'm a bit stumped. I want to have a function that checks whether the an iterator variable is used again after being used in a while-let loop. I have written a visitor that, given a block, the iterator variable and the id of the while-let loop, checks whether the iterator variable appears after the beginning of the while let loop (i.e. in the loop body or after the loop). But now I need a block to run it on. I thought I could just use get_parent_node with the node id of the variable declaration to get the relevant block, but the parent node isn't necessarily a NodeBlock, it might be a NodeItem or a NodeImpltem or whatever Node variant can contain a Block. I'm guessing there must be some straightforward way to get the block (or a list of statements or something along those lines) that a given binding is in scope, but I can't seem to find it.

[fhartwig]

Ok, I just found get_enclosing_scope in rustc, I guess that's something that could be useful.

[Manishearth]

Note: for tracking uses, try ExprUseVisitor

[fhartwig]

I've fixed the false positives when an iterator is used after the loop. I've not used ExprUseVisitor since that ended up being more complex than what I do now. On the other hand, ExprUseVisitor would catch certain false negatives, e.g. the following:

let mut iter = 0u32..20;
while let Some(x) = iter().next {}
iter = 1..5;
println!("iter: {:?}" iter);

This should strictly speaking warn since it can be written as a for loop, but it currently doesn't since iter is used after the loop. I think this is too much of an edge case to make the code more complicated for it, but I'm not sure.

Regarding the dogfood.sh failure: I think this is a false positive in the needless_lifetime lint, because when I remove the lifetime, rustc complains about missing lifetimes. But it's late now and I'll look into it tomorrow.

[Manishearth]

 pub fn get_enclosing_block(cx: &LateContext, node: NodeId) -> Option<&Block> {

should work, really.

[fhartwig]

@Manishearth That's what I thought, but it doesn't (Probably because of the lifetimes on LateContext? I don't really know the elision rules off the top of my head). So as far as I can tell, this is a false positive. I've just filed #417

[Manishearth]

Oh, LateContext has a tcx lifetime. Right.

[Manishearth]

Since you had done most of the review, r? @llogiq

(If you're busy let me know and I'll do it tomorrow)

[llogiq]

Looks good to me; just slap an #[allow(needless_lifetimes)] on the offending function until we've fixed #417, and I'll be happy to merge.

[fhartwig]

Done 😄

[llogiq]

Thanks a bunch! 😃