Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix let_unit_value with for loop iterating over units #1967

Merged
merged 3 commits into from
Aug 18, 2017

Conversation

koivunej
Copy link
Contributor

@koivunej koivunej commented Aug 18, 2017

This will fix #1964, which simplifies to the code below getting lint let_unit_value:

for _ in &[()] {
  // anything
}

Fix is to modify is_from_for_desugar to detect HIR-level let _ = for _ in ... {} in addition to the previously detected HIR-level let _ = for something in ... {}. Also adds comments to explain the previous desugaring detection.

Does not add the test case suggested by @oli-obk:

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.

I did not add a new case because commenting out the first check (and disabling tests/dogfood.rs) will cause a large number of tests/ui/for_loop.rs to receive for_let_unit lints such as:

error: this let-binding has unit value. Consider omitting `let for _v in hm.iter() { } =`
   --> $DIR/for_loop.rs:234:5
    |
234 |     for _v in hm.iter() { }
    |     ^^^^^^^^^^^^^^^^^^^^^^^

Only ui-test changes are included as I didn't find any test cases for the is_from_for_desugar. I'd imagine something like the example below would be awesome but might not be possible at the moment (I don't know the rustc at all).

assert!(is_from_for_desugar(hir! { for _ in 0.. {} });

This will avoid `let_unit_value` in the examples in the ui-test.
It might match too widely.
Removing the first check will break a lot of for-loop UI tests and the
dogfood test.
The previous version would had deadlocked as the Sender remained alive
and iterator would had never became complete. Just in case someone
decided to run it.
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;
}}

// This detects a variable binding in for loop to avoid `let_unit_value`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This detects a variable binding ...

Should had been something like "This detects a _ binding ..." or "This detects an underscore binding". Latter might be more searchable or greppable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. I think it would happen for others, too, like if you had for Foo { a } in bar {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot see it matching this for-loop. is_from_for_desugar returns false for the following:

#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    let b = vec![Foo { a: 42 }];

    for Foo { a } in b {
        println!("a: {}", a);
    }
}

struct Foo {
    a: u32
}
The `{:#?}` output of decl in above example
Spanned {
    node: DeclLocal(
        Local {
            pat: pat(8: b),
            ty: None,
            init: Some(
                expr(26: <[_]>::into_vec(box [Foo{a: 42,}]))
            ),
            id: NodeId(
                7
            ),
            hir_id: HirId {
                owner: DefIndex(
                    3
                ),
                local_id: ItemLocalId(
                    1
                )
            },
            span: ./example.rs:5:5: 5:32,
            attrs: ThinVec(
                None
            ),
            source: Normal
        }
    ),
    span: ./example.rs:5:5: 5:33
}

Copy link
Contributor Author

@koivunej koivunej Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the previous comment I had the println!'s wrong. There are a couple of calls to is_from_for_desugar but the example for Foo { a } in bar is handled by the original check in is_from_for_desugar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a different DeclLocal than the one we're looking for, but it should probably also contain a source other than Normal. I'll see if I can produce a PR for rustc.

I think it'll call is_from_for_desugar if you have a let a = 42; outside the for loop, which will trigger the shadowing lints.

This PR is fine as it is, since this is a non-issue for the let_unit_value lint anyway

@oli-obk
Copy link
Contributor

oli-obk commented Aug 18, 2017

Thanks! This is some great detective work you did there

@oli-obk oli-obk merged commit d2504fa into rust-lang:master Aug 18, 2017
@koivunej koivunej deleted the issue-1964 branch August 18, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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