let_unit_value false warning on for _ in std::sync::mpsc::Receiver::iter()

[koivunej]

Playground example with the main point being this:

let rx: ::std::sync::mpsc::Receiver<()> = rx;

for _ in rx.iter() {
    // anything
}

Todays playground and the clippy version I have installed (0.0.150) both report:

warning: this let-binding has unit value. Consider omitting `let for _ in rx.iter() {
            count += 1;
        } =`
  --> src/main.rs:10:9
   |
10 | /         for _ in rx.iter() {
11 | |             count += 1;
12 | |         }
   | |_________^
   |
   = note: #[warn(let_unit_value)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#let_unit_value

warning: this let-binding has unit value. Consider omitting `let _ =`
  --> src/main.rs:10:9
   |
10 | /         for _ in rx.iter() {
11 | |             count += 1;
12 | |         }
   | |_________^
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#let_unit_value

I don't think there is duplicate of this reported. let_unit_value related found:

• Let_unit_value false positive when return type is type parameter #1502 in the context of let _: () = ... but not in for loop
• Findings from running Clippy on Servo #164 mentions in context of derives

My real use case for using Receiver<()> is to provide a way for a worker thread to sleep with notifications. I guess this could be implemented in other ways but this was a quick solution that seems to work. std::thread::Thread might even provide a better way for this via park() and unpark().

[oli-obk]

Oh... this is inside the expansion of the for loop. Apparently higher::is_from_for_desugar doesn't work anymore.

[koivunej]

Just to clarify, the following snippet does not trigger any warnings, even with &v replaced by v:

let v = vec![0, 1, 2];
let mut count = 0;
    
for _ in &v {
    count += 1;
}
    
assert_eq!(count, 3);

I might be interested in taking a further look on why this happens and see if it's something I could possibly fix...

[oli-obk]

The issue is with iterating over units:

    let v = vec![(), (), ()];
    let mut count = 0;

    for _ in v {
        count += 1;
    }
    assert_eq!(count, 3);

I might be interested in taking a further look on why this happens and see if it's something I could possibly fix...

Rustc removes for loops during its compilation and replaces them with a loop, some let bindings and a match. We have a function is_from_for_desugar, which is supposed to detect the expanded for loops, but it doesn't seem to work anymore.

Inside the expansion of the for loop there's a let _: () = iter.next(), which triggers the lint.

To debug this you can do one or multiple of the following

• extend the inspector lint so it can dump for loop internals
• extend the author lint so it can dump for loop internals
• add lots of println! calls inside is_from_for_desugar and look at what's going on
• check whether you can make rustc produce the expanded output (google for pretting printing I think)

[koivunej]

Ok.. I'll start looking how to integrate this into the test suite. I think it needs to be as an "ui-test", tests/ui/let_unit.rs looks a bit similar.

Interesting find: I accidentially reproduced the code sample you mentioned (with vec![(), (), ()]) as:

    let v = vec![(), (), ()];
    let mut count = 0;

    for _ in &v { // <--
        count += 1;
    }
    assert_eq!(count, 3);

The above does not trigger a warning. With for _ in v { ... } it will trigger the same warnings.

Thanks for your tips. I'll try continue looking...

[oli-obk]

The above does not trigger a warning. With for _ in v { ... } it will trigger the same warnings.

It does not trigger the warnings, because the let statement is let _: &() = item;, which isn't a unit value ;)

[koivunej]

It does not trigger the warnings, because the let statement is let _: &() = item;, which isn't a unit value ;)

Ah yes. :)

It looks like modifying the utility is_from_for_desugar by adding another

 /// Checks if a `let` decl is from a `for` loop desugaring.
 pub fn is_from_for_desugar(decl: &hir::Decl) -> bool {
     if_let_chain! {[
         let hir::DeclLocal(ref loc) = decl.node,
         let Some(ref expr) = loc.init,
         let hir::ExprMatch(_, _, hir::MatchSource::ForLoopDesugar) = expr.node,
     ], {
         return true;
     }}
+    if_let_chain! {[
+        let hir::DeclLocal(ref loc) = decl.node,
+        let hir::LocalSource::ForLoopDesugar = loc.source,
+    ], {
+        return true;
+    }}
     false
 }

will remove the warning from my case. I concluded this by inspecting the {:#?} of hir::Decl and building a new match based on that. Looks like the first match cannot be removed either, as it will match line for x in y { ... } (no let binding on the for-loop expr), found by the dogfood test when I tried to remove it.

I have no idea if this will match too much but nothing is caught by the existing test cases, at least running from linux.

Any tips on how to continue from here? Should I package this diff as an PR or can you think of something getting broken by this change?

[oli-obk]

It basically looks good to me. Can you add all that you learned about this as comments before each of the two if_let_chain?

Should I package this diff as an PR

yes that would be great. Include some of the examples in this issue as unit tests to show that they aren't triggering. Also add an example that would have triggered if the first if_let_chain were removed, so we don't have to hope dogfood will catch it but have an explicit test in the suite.