From cf8e95eb2208ab869df241d11c887a33e67d033f Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 18 Aug 2017 17:07:39 +0300 Subject: [PATCH 1/3] is_from_for_desugar: add match for `for _ in x` This will avoid `let_unit_value` in the examples in the ui-test. It might match too widely. --- clippy_lints/src/utils/higher.rs | 16 ++++++++++++++++ tests/ui/let_unit.rs | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/clippy_lints/src/utils/higher.rs b/clippy_lints/src/utils/higher.rs index d138892a092..3665bdf2360 100644 --- a/clippy_lints/src/utils/higher.rs +++ b/clippy_lints/src/utils/higher.rs @@ -121,6 +121,22 @@ pub fn is_from_for_desugar(decl: &hir::Decl) -> bool { ], { return true; }} + + // This detects a variable binding in for loop to avoid `let_unit_value` + // 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 } diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs index ecae967f781..0b0afccef45 100644 --- a/tests/ui/let_unit.rs +++ b/tests/ui/let_unit.rs @@ -18,8 +18,30 @@ 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<()>. + let (tx, rx) = ::std::sync::mpsc::channel(); + tx.send(()).unwrap(); + count = 0; + for _ in rx.iter() { + count += 1; + } + assert_eq!(count, 1); +} + #[derive(Copy, Clone)] pub struct ContainsUnit(()); // should be fine From a5147e8a0819dd1daab28a3ef1970069396a5bf1 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 18 Aug 2017 17:12:00 +0300 Subject: [PATCH 2/3] is_from_for_loop: document what first check matches Removing the first check will break a lot of for-loop UI tests and the dogfood test. --- clippy_lints/src/utils/higher.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/clippy_lints/src/utils/higher.rs b/clippy_lints/src/utils/higher.rs index 3665bdf2360..67319f0c355 100644 --- a/clippy_lints/src/utils/higher.rs +++ b/clippy_lints/src/utils/higher.rs @@ -114,6 +114,13 @@ pub fn range(expr: &hir::Expr) -> Option { /// 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, From 171f7b4eb7fed11f9282eb0c5af5eda1e77f9bc6 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 18 Aug 2017 17:29:05 +0300 Subject: [PATCH 3/3] tests/ui/let_unit: fix comment and example code 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. --- tests/ui/let_unit.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs index 0b0afccef45..d07cf8ede2f 100644 --- a/tests/ui/let_unit.rs +++ b/tests/ui/let_unit.rs @@ -33,9 +33,11 @@ fn consume_units_with_for_loop() { } assert_eq!(count, 3); - // Same for consuming from some other Iterator<()>. + // Same for consuming from some other Iterator. let (tx, rx) = ::std::sync::mpsc::channel(); tx.send(()).unwrap(); + drop(tx); + count = 0; for _ in rx.iter() { count += 1;