diff --git a/clippy_lints/src/loops/needless_collect.rs b/clippy_lints/src/loops/needless_collect.rs index ddaffc751880d..6d987f393fa5c 100644 --- a/clippy_lints/src/loops/needless_collect.rs +++ b/clippy_lints/src/loops/needless_collect.rs @@ -1,5 +1,6 @@ use super::NEEDLESS_COLLECT; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; +use clippy_utils::higher; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; @@ -184,10 +185,19 @@ struct IterFunctionVisitor<'a, 'tcx> { impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { fn visit_block(&mut self, block: &'tcx Block<'tcx>) { for (expr, hir_id) in block.stmts.iter().filter_map(get_expr_and_hir_id_from_stmt) { + if check_loop_kind(expr).is_some() { + continue; + } self.visit_block_expr(expr, hir_id); } if let Some(expr) = block.expr { - self.visit_block_expr(expr, None); + if let Some(loop_kind) = check_loop_kind(expr) { + if let LoopKind::Conditional(block_expr) = loop_kind { + self.visit_block_expr(block_expr, None); + } + } else { + self.visit_block_expr(expr, None); + } } } @@ -264,6 +274,28 @@ impl<'tcx> Visitor<'tcx> for IterFunctionVisitor<'_, 'tcx> { } } +enum LoopKind<'tcx> { + Conditional(&'tcx Expr<'tcx>), + Loop, +} + +fn check_loop_kind<'tcx>(expr: &Expr<'tcx>) -> Option> { + if let Some(higher::WhileLet { let_expr, .. }) = higher::WhileLet::hir(expr) { + return Some(LoopKind::Conditional(let_expr)); + } + if let Some(higher::While { condition, .. }) = higher::While::hir(expr) { + return Some(LoopKind::Conditional(condition)); + } + if let Some(higher::ForLoop { arg, .. }) = higher::ForLoop::hir(expr) { + return Some(LoopKind::Conditional(arg)); + } + if let ExprKind::Loop { .. } = expr.kind { + return Some(LoopKind::Loop); + } + + None +} + impl<'tcx> IterFunctionVisitor<'_, 'tcx> { fn visit_block_expr(&mut self, expr: &'tcx Expr<'tcx>, hir_id: Option) { self.current_statement_hir_id = hir_id; diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 9d0320bf065e9..5431f2ebc0a3d 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -413,11 +413,13 @@ fn check_rustfix_coverage() { if let Ok(missing_coverage_contents) = std::fs::read_to_string(missing_coverage_path) { assert!(RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS.iter().is_sorted_by_key(Path::new)); - for rs_path in missing_coverage_contents.lines() { - if Path::new(rs_path).starts_with("tests/ui/crashes") { + for rs_file in missing_coverage_contents.lines() { + let rs_path = Path::new(rs_file); + if rs_path.starts_with("tests/ui/crashes") { continue; } - let filename = Path::new(rs_path).strip_prefix("tests/ui/").unwrap(); + assert!(rs_path.starts_with("tests/ui/"), "{:?}", rs_file); + let filename = rs_path.strip_prefix("tests/ui/").unwrap(); assert!( RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS .binary_search_by_key(&filename, Path::new) @@ -425,7 +427,7 @@ fn check_rustfix_coverage() { "`{}` runs `MachineApplicable` diagnostics but is missing a `run-rustfix` annotation. \ Please either add `// run-rustfix` at the top of the file or add the file to \ `RUSTFIX_COVERAGE_KNOWN_EXCEPTIONS` in `tests/compile-test.rs`.", - rs_path, + rs_file, ); } } diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index 1f11d1f8d563c..12a9ace1ee688 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -112,3 +112,192 @@ fn allow_test() { let v = [1].iter().collect::>(); v.into_iter().collect::>(); } + +mod issue_8553 { + fn test_for() { + let vec = vec![1, 2]; + let w: Vec = vec.iter().map(|i| i * i).collect(); + + for i in 0..2 { + // Do not lint, because this method call is in the loop + w.contains(&i); + } + + for i in 0..2 { + let y: Vec = vec.iter().map(|k| k * k).collect(); + let z: Vec = vec.iter().map(|k| k * k).collect(); + // Do lint + y.contains(&i); + for j in 0..2 { + // Do not lint, because this method call is in the loop + z.contains(&j); + } + } + + // Do not lint, because this variable is used. + w.contains(&0); + } + + fn test_while() { + let vec = vec![1, 2]; + let x: Vec = vec.iter().map(|i| i * i).collect(); + let mut n = 0; + while n > 1 { + // Do not lint, because this method call is in the loop + x.contains(&n); + n += 1; + } + + while n > 2 { + let y: Vec = vec.iter().map(|k| k * k).collect(); + let z: Vec = vec.iter().map(|k| k * k).collect(); + // Do lint + y.contains(&n); + n += 1; + while n > 4 { + // Do not lint, because this method call is in the loop + z.contains(&n); + n += 1; + } + } + } + + fn test_loop() { + let vec = vec![1, 2]; + let x: Vec = vec.iter().map(|i| i * i).collect(); + let mut n = 0; + loop { + if n < 1 { + // Do not lint, because this method call is in the loop + x.contains(&n); + n += 1; + } else { + break; + } + } + + loop { + if n < 2 { + let y: Vec = vec.iter().map(|k| k * k).collect(); + let z: Vec = vec.iter().map(|k| k * k).collect(); + // Do lint + y.contains(&n); + n += 1; + loop { + if n < 4 { + // Do not lint, because this method call is in the loop + z.contains(&n); + n += 1; + } else { + break; + } + } + } else { + break; + } + } + } + + fn test_while_let() { + let vec = vec![1, 2]; + let x: Vec = vec.iter().map(|i| i * i).collect(); + let optional = Some(0); + let mut n = 0; + while let Some(value) = optional { + if n < 1 { + // Do not lint, because this method call is in the loop + x.contains(&n); + n += 1; + } else { + break; + } + } + + while let Some(value) = optional { + let y: Vec = vec.iter().map(|k| k * k).collect(); + let z: Vec = vec.iter().map(|k| k * k).collect(); + if n < 2 { + // Do lint + y.contains(&n); + n += 1; + } else { + break; + } + + while let Some(value) = optional { + if n < 4 { + // Do not lint, because this method call is in the loop + z.contains(&n); + n += 1; + } else { + break; + } + } + } + } + + fn test_if_cond() { + let vec = vec![1, 2]; + let v: Vec = vec.iter().map(|i| i * i).collect(); + let w = v.iter().collect::>(); + // Do lint + for _ in 0..w.len() { + todo!(); + } + } + + fn test_if_cond_false_case() { + let vec = vec![1, 2]; + let v: Vec = vec.iter().map(|i| i * i).collect(); + let w = v.iter().collect::>(); + // Do not lint, because w is used. + for _ in 0..w.len() { + todo!(); + } + + w.len(); + } + + fn test_while_cond() { + let mut vec = vec![1, 2]; + let mut v: Vec = vec.iter().map(|i| i * i).collect(); + let mut w = v.iter().collect::>(); + // Do lint + while 1 == w.len() { + todo!(); + } + } + + fn test_while_cond_false_case() { + let mut vec = vec![1, 2]; + let mut v: Vec = vec.iter().map(|i| i * i).collect(); + let mut w = v.iter().collect::>(); + // Do not lint, because w is used. + while 1 == w.len() { + todo!(); + } + + w.len(); + } + + fn test_while_let_cond() { + let mut vec = vec![1, 2]; + let mut v: Vec = vec.iter().map(|i| i * i).collect(); + let mut w = v.iter().collect::>(); + // Do lint + while let Some(i) = Some(w.len()) { + todo!(); + } + } + + fn test_while_let_cond_false_case() { + let mut vec = vec![1, 2]; + let mut v: Vec = vec.iter().map(|i| i * i).collect(); + let mut w = v.iter().collect::>(); + // Do not lint, because w is used. + while let Some(i) = Some(w.len()) { + todo!(); + } + w.len(); + } +} diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr index 0f5e78f91198c..9f0880cc6069d 100644 --- a/tests/ui/needless_collect_indirect.stderr +++ b/tests/ui/needless_collect_indirect.stderr @@ -125,5 +125,122 @@ LL ~ LL ~ sample.iter().count() | -error: aborting due to 9 previous errors +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:127:59 + | +LL | let y: Vec = vec.iter().map(|k| k * k).collect(); + | ^^^^^^^ +... +LL | y.contains(&i); + | -------------- the iterator could be used here instead + | +help: check if the original Iterator contains an element instead of collecting then checking + | +LL ~ +LL | let z: Vec = vec.iter().map(|k| k * k).collect(); +LL | // Do lint +LL ~ vec.iter().map(|k| k * k).any(|x| x == i); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:152:59 + | +LL | let y: Vec = vec.iter().map(|k| k * k).collect(); + | ^^^^^^^ +... +LL | y.contains(&n); + | -------------- the iterator could be used here instead + | +help: check if the original Iterator contains an element instead of collecting then checking + | +LL ~ +LL | let z: Vec = vec.iter().map(|k| k * k).collect(); +LL | // Do lint +LL ~ vec.iter().map(|k| k * k).any(|x| x == n); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:181:63 + | +LL | let y: Vec = vec.iter().map(|k| k * k).collect(); + | ^^^^^^^ +... +LL | y.contains(&n); + | -------------- the iterator could be used here instead + | +help: check if the original Iterator contains an element instead of collecting then checking + | +LL ~ +LL | let z: Vec = vec.iter().map(|k| k * k).collect(); +LL | // Do lint +LL ~ vec.iter().map(|k| k * k).any(|x| x == n); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:217:59 + | +LL | let y: Vec = vec.iter().map(|k| k * k).collect(); + | ^^^^^^^ +... +LL | y.contains(&n); + | -------------- the iterator could be used here instead + | +help: check if the original Iterator contains an element instead of collecting then checking + | +LL ~ +LL | let z: Vec = vec.iter().map(|k| k * k).collect(); +LL | if n < 2 { +LL | // Do lint +LL ~ vec.iter().map(|k| k * k).any(|x| x == n); + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:242:26 + | +LL | let w = v.iter().collect::>(); + | ^^^^^^^ +LL | // Do lint +LL | for _ in 0..w.len() { + | ------- the iterator could be used here instead + | +help: take the original Iterator's count instead of collecting it and finding the length + | +LL ~ +LL | // Do lint +LL ~ for _ in 0..v.iter().count() { + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:264:30 + | +LL | let mut w = v.iter().collect::>(); + | ^^^^^^^ +LL | // Do lint +LL | while 1 == w.len() { + | ------- the iterator could be used here instead + | +help: take the original Iterator's count instead of collecting it and finding the length + | +LL ~ +LL | // Do lint +LL ~ while 1 == v.iter().count() { + | + +error: avoid using `collect()` when not needed + --> $DIR/needless_collect_indirect.rs:286:30 + | +LL | let mut w = v.iter().collect::>(); + | ^^^^^^^ +LL | // Do lint +LL | while let Some(i) = Some(w.len()) { + | ------- the iterator could be used here instead + | +help: take the original Iterator's count instead of collecting it and finding the length + | +LL ~ +LL | // Do lint +LL ~ while let Some(i) = Some(v.iter().count()) { + | + +error: aborting due to 16 previous errors