Skip to content

Fix let_unit_value with for loop iterating over units #1967

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 18, 2017
Merged

Fix let_unit_value with for loop iterating over units #1967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf8e95e
is_from_for_desugar: add match for `for _ in x`
koivunej Aug 18, 2017
a5147e8
is_from_for_loop: document what first check matches
koivunej Aug 18, 2017
171f7b4
tests/ui/let_unit: fix comment and example code
koivunej Aug 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions clippy_lints/src/utils/higher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,36 @@ pub fn range(expr: &hir::Expr) -> Option<Range> {

/// Checks if a `let` decl is from a `for` loop desugaring.
pub fn is_from_for_desugar(decl: &hir::Decl) -> bool {
// This will detect plain for-loops without an actual variable binding:
//
// ```
// for x in some_vec {
// // do stuff
// }
// ```
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 •
edited
Loading

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

// lint (see issue #1964).
//
// ```
// for _ in vec![()] {
// // anything
// }
// ```
if_let_chain! {[
let hir::DeclLocal(ref loc) = decl.node,
let hir::LocalSource::ForLoopDesugar = loc.source,
], {
return true;
}}

false
}

Expand Down
24 changes: 24 additions & 0 deletions tests/ui/let_unit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,32 @@ fn main() {
let _a = ();
}

consume_units_with_for_loop(); // should be fine as well

let_and_return!(()) // should be fine
}

// Related to issue #1964
fn consume_units_with_for_loop() {
// `for_let_unit` lint should not be triggered by consuming them using for loop.
let v = vec![(), (), ()];
let mut count = 0;
for _ in v {
count += 1;
}
assert_eq!(count, 3);

// Same for consuming from some other Iterator<Item = ()>.
let (tx, rx) = ::std::sync::mpsc::channel();
tx.send(()).unwrap();
drop(tx);

count = 0;
for _ in rx.iter() {
count += 1;
}
assert_eq!(count, 1);
}

#[derive(Copy, Clone)]
pub struct ContainsUnit(()); // should be fine