Closes #1219 false positive for explicit_counter_loop

[JoshMcguigan]

Hello,

As in #3131, I added a test case here based on the reported issue #1219, and I'm not seeing a test failure. I am new to the clippy codebase, so please let me know if I am just missing something obvious.

Thanks!

[JoshMcguigan]

It looks like the integration test for RLS timed out after exactly 10 minutes. Looking at other builds it seems that test commonly runs long, can we re-run the build or is there something I should look into?

[phansch]

Restarted the RLS build, sometimes they just don't start.

I'm not really sure why the false positive is not triggered anymore.
Maybe I'm also doing something wrong, but this snippet should trigger the lint for sure:

fn main() {
    let text = "banana";
    let mut count = 0;
    for ch in text.chars() {
        if ch == 'a' {
            println!("abc")
        }
        count += 1
    }
    println!("{}", count);
}

And it doesn't: https://play.rust-lang.org/?gist=8081f695a48d99e0fe4e3e127b2cf781&version=nightly&mode=debug&edition=2015

[phansch]

It looks like we have very low test coverage for that lint. I only found these two tests that trigger the lint:

https://github.com/rust-lang-nursery/rust-clippy/blob/ca753c4af1c0b6db9c17324695750990bd472ed2/tests/ui/for_loop.rs#L267-L277

[oli-obk]

Maybe the lint is allow-by-default and needs a warn attribute to trigger?

[JoshMcguigan]

Maybe the lint is allow-by-default and needs a warn attribute to trigger?

The playground link by @phansch includes an explicit #![deny(clippy::explicit_counter_loop)].

I also tried the other test cases listed above and they aren't triggering the lint in the playground either. Testing locally, I found that the following DO trigger the lint:

 // Loop with explicit counter variable 
 let mut _index = 0; 
 for _v in &vec { 
     _index += 1 
 } 
  
 let mut _index = 1; 
 _index = 0; 
 for _v in &vec { 
     _index += 1 
 } 

while these DO NOT trigger the lint:

    let text = "banana";
    let mut count = 0;
    for ch in text.chars() {
        if ch == 'a' {
            println!("abc")
        }
        count += 1
    }
    println!("{}", count);

    let thing = 5;
    let text = "banana";
    let mut count = 0;
    for ch in text.chars() {
        if ch == 'a' {
            continue;
        }
        count += 1
    }
    println!("{}", count);

I'll take a look and try to figure out why the lint isn't being triggered locally. For the playground, does clippy typically work normally in the playground?

[JoshMcguigan]

Alright, I've narrowed it down a bit. So running locally, the following does not trigger the lint. But if I remove the println!("{}", index);, then it does trigger the lint.

        let text = "banana";
        let mut index = 0;
        for _ch in text.chars() {
            index += 1
        }
        println!("{}", index);

[flip1995]

Ah so the lint doesn't get triggered if the count variable gets used after the loop, which makes sense. If we would define the count variable in the loop decl, it would not be usable after the loop body.

Also adding let _ = count; after the loop prevents the lint from triggering.

Yep: #473

[JoshMcguigan]

This commit corrects the bug where using the variable after the loop prevents the lint from triggering, and adds some test cases to catch it.

I have not yet checked to make sure this doesn't cause regressions elsewhere though. Unless someone else gets to it first, I'll check that tonight and then finally try to tackle the original issue (false positive when there is a continue in the loop).

[flip1995]

This commit corrects the bug where using the variable after the loop prevents the lint from triggering

This wasn't a bug, but intended. See PR #473 and corresponding issues #384 and #373

The only "bug" feature is, that it is triggered when using continue without using the variable after the loop:

fn main() {
    let text = "banana";
    let mut count = 0;
    for ch in text.chars() {
        if ch == 'a' {
            continue;
        }
        count += 1
    }
}

But if you don't use the count variable after the loop, I don't think this is a FP, because you can use .enumerate() regardless of the continue, without changing the behavior.

[JoshMcguigan]

This wasn't a bug, but intended. See PR #473 and corresponding issues #384 and #373

Thanks for setting me straight here, I misread your first post but this is obvious in hindsight.

The only "bug" feature is, that it is triggered when using continue without using the variable after the loop...But if you don't use the count variable after the loop, I don't think this is a FP...

So this issue can be closed without changing the code then? Or perhaps we just add some tests here to ensure the appropriate coverage and link them to these issues?

[flip1995]

Yes, I think this issue is resolved. But adding some tests for future reference is always good. It would be great if you could do this!

[JoshMcguigan]

I've updated the tests, but looking at it again I'm thinking there is still a false positive here. Modifying the example code from @flip1995 slightly:

let text = "banana";
let mut count = 0;
for ch in text.chars() {
    if ch == 'a' {
        continue;
    }
    count += 1;
    println!("{}", count);
}

Currently this triggers the lint, but using enumerate here would change the behavior. @flip1995 can you confirm I am thinking about this correctly?

[flip1995]

Yes this is a FP! Playground
Thanks for adding the tests and finding it! Do you like to try and fix this?

EDIT (When should the lint not get triggered in case of a continue):

• There is a continue anywhere before the increment of the counter
• The counter variable gets used anywhere else in the for-loop block (this is kinda given, because if it does not get used in the body or after the loop rustc will warn because of an unused variable)

Since the lint won't get triggered if the counter is used after the loop, the only thing to add would be to bail out if continue is used before incrementing the counter variable. Just checking for a continue statement anywhere in the loop body should be easy. Checking if it appears before the counter increment could be hard.

Does this make sense to you? Or do you think I missed something?

[JoshMcguigan]

That makes sense. So I think we'd need one additional test case.

// should NOT trigger the lint
let text = "banana";
let mut count = 0;
for ch in text.chars() {
    if ch == 'a' {
        continue;
    }
    count += 1;
    println!("{}", count);
}

// should trigger the lint <-- this would be the additional test
let text = "banana";
let mut count = 0;
for ch in text.chars() {
    count += 1;
    if ch == 'a' {
        continue;
    }
    println!("{}", count);
}

I'll try implementing a fix for this, and follow up if I run into any additional issues.

Thanks for your support!

[JoshMcguigan]

I've corrected the behavior we discussed above, as well adding two more tests and corrected handling of nested loops as shown below.

// should trigger the lint because the count is not conditional
let text = "banana";
let mut count = 0;
for ch in text.chars() {
    count += 1;
    for i in 0..2 {
        let _ = 123;
    }
    println!("{}", count);
}

// should not trigger the lint because the count is incremented multiple times
let text = "banana";
let mut count = 0;
for ch in text.chars() {
    count += 1;
    for i in 0..2 {
        count += 1;
    }
    println!("{}", count);
}

If all this looks good, I believe this is fit to be merged.

[flip1995]

Thanks!