From 9d311b5c2b29936349acdc9dfddafc8ea919b51e Mon Sep 17 00:00:00 2001 From: schvv31n Date: Sun, 19 May 2024 21:41:13 +0100 Subject: [PATCH 1/5] initial fix --- .../iter_on_single_or_empty_collections.rs | 43 ++++++++++++++++++- tests/ui/iter_on_empty_collections.fixed | 13 ++++++ tests/ui/iter_on_empty_collections.rs | 13 ++++++ tests/ui/iter_on_empty_collections.stderr | 8 +++- 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index 6c9bdcff826..0e929cfe798 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -1,8 +1,12 @@ +use std::iter::once; + use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; use clippy_utils::{get_expr_use_or_unification_node, is_res_lang_ctor, path_res, std_or_core}; use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; +use rustc_hir::hir_id::HirId; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind, Node}; use rustc_lint::LateContext; @@ -25,7 +29,25 @@ impl IterType { } } -pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: &str, recv: &Expr<'_>) { +fn is_arg_ty_unified_in_fn<'tcx>( + cx: &LateContext<'tcx>, + fn_id: DefId, + arg_id: HirId, + args: impl Iterator> + Clone, +) -> bool { + let fn_sig = cx.tcx.fn_sig(fn_id).instantiate_identity(); + let arg_id_in_args = args.clone().position(|e| e.hir_id == arg_id).unwrap(); + let arg_ty_in_args = fn_sig.input(arg_id_in_args); + + cx.tcx.predicates_of(fn_id).predicates.iter().any(|(clause, _)| { + clause + .as_projection_clause() + .and_then(|p| p.map_bound(|p| p.term.ty()).transpose()) + .is_some_and(|ty| ty == arg_ty_in_args) + }) +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: &str, recv: &'tcx Expr<'tcx>) { let item = match recv.kind { ExprKind::Array([]) => None, ExprKind::Array([e]) => Some(e), @@ -43,6 +65,25 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: &str, re let is_unified = match get_expr_use_or_unification_node(cx.tcx, expr) { Some((Node::Expr(parent), child_id)) => match parent.kind { ExprKind::If(e, _, _) | ExprKind::Match(e, _, _) if e.hir_id == child_id => false, + ExprKind::Call( + Expr { + kind: ExprKind::Path(path), + hir_id, + .. + }, + args, + ) => is_arg_ty_unified_in_fn( + cx, + cx.typeck_results().qpath_res(path, *hir_id).def_id(), + expr.hir_id, + args.iter(), + ), + ExprKind::MethodCall(_name, recv, args, _span) => is_arg_ty_unified_in_fn( + cx, + cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(), + expr.hir_id, + once(recv).chain(args.iter()), + ), ExprKind::If(_, _, _) | ExprKind::Match(_, _, _) | ExprKind::Closure(_) diff --git a/tests/ui/iter_on_empty_collections.fixed b/tests/ui/iter_on_empty_collections.fixed index 794629f240e..871cc18d7c2 100644 --- a/tests/ui/iter_on_empty_collections.fixed +++ b/tests/ui/iter_on_empty_collections.fixed @@ -20,6 +20,19 @@ fn array() { }; let _ = if false { ["test"].iter() } else { [].iter() }; + + let smth = Some(vec![1, 2, 3]); + + // Don't trigger when the empty collection iter is relied upon for its concrete type + // But do trigger if it is just an iterator, despite being an argument to a method + for i in smth.as_ref().map_or([].iter(), |s| s.iter()).chain(std::iter::empty()) { + println!("{i}"); + } + + // Same as above, but for regular function calls + for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { + println!("{i}"); + } } macro_rules! in_macros { diff --git a/tests/ui/iter_on_empty_collections.rs b/tests/ui/iter_on_empty_collections.rs index a6461a702eb..7e3df49eefc 100644 --- a/tests/ui/iter_on_empty_collections.rs +++ b/tests/ui/iter_on_empty_collections.rs @@ -20,6 +20,19 @@ fn array() { }; let _ = if false { ["test"].iter() } else { [].iter() }; + + let smth = Some(vec![1, 2, 3]); + + // Don't trigger when the empty collection iter is relied upon for its concrete type + // But do trigger if it is just an iterator, despite being an argument to a method + for i in smth.as_ref().map_or([].iter(), |s| s.iter()).chain([].iter()) { + println!("{i}"); + } + + // Same as above, but for regular function calls + for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { + println!("{i}"); + } } macro_rules! in_macros { diff --git a/tests/ui/iter_on_empty_collections.stderr b/tests/ui/iter_on_empty_collections.stderr index ade20ff26a0..da9caa6925b 100644 --- a/tests/ui/iter_on_empty_collections.stderr +++ b/tests/ui/iter_on_empty_collections.stderr @@ -37,5 +37,11 @@ error: `iter` call on an empty collection LL | assert_eq!(None.iter().next(), Option::<&i32>::None); | ^^^^^^^^^^^ help: try: `std::iter::empty()` -error: aborting due to 6 previous errors +error: `iter` call on an empty collection + --> tests/ui/iter_on_empty_collections.rs:28:66 + | +LL | for i in smth.as_ref().map_or([].iter(), |s| s.iter()).chain([].iter()) { + | ^^^^^^^^^ help: try: `std::iter::empty()` + +error: aborting due to 7 previous errors From 36293727238d2559f594d7019a551853cb0c57b3 Mon Sep 17 00:00:00 2001 From: schvv31n Date: Mon, 20 May 2024 08:05:49 +0100 Subject: [PATCH 2/5] Lint on closure calls, suppress on callable constants calls --- .../iter_on_single_or_empty_collections.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index 0e929cfe798..f16385b5f95 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -33,10 +33,10 @@ fn is_arg_ty_unified_in_fn<'tcx>( cx: &LateContext<'tcx>, fn_id: DefId, arg_id: HirId, - args: impl Iterator> + Clone, + args: impl IntoIterator>, ) -> bool { let fn_sig = cx.tcx.fn_sig(fn_id).instantiate_identity(); - let arg_id_in_args = args.clone().position(|e| e.hir_id == arg_id).unwrap(); + let arg_id_in_args = args.into_iter().position(|e| e.hir_id == arg_id).unwrap(); let arg_ty_in_args = fn_sig.input(arg_id_in_args); cx.tcx.predicates_of(fn_id).predicates.iter().any(|(clause, _)| { @@ -72,12 +72,11 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method .. }, args, - ) => is_arg_ty_unified_in_fn( - cx, - cx.typeck_results().qpath_res(path, *hir_id).def_id(), - expr.hir_id, - args.iter(), - ), + ) => { + cx.typeck_results().qpath_res(path, *hir_id).opt_def_id() + .filter(|fn_id| cx.tcx.def_kind(fn_id).is_fn_like()) + .is_some_and(|fn_id| is_arg_ty_unified_in_fn(cx, fn_id, expr.hir_id, args)) + } ExprKind::MethodCall(_name, recv, args, _span) => is_arg_ty_unified_in_fn( cx, cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(), From 3955bd4c98a61ada7041c028a5d0c749c3c0ee11 Mon Sep 17 00:00:00 2001 From: schvv31n Date: Mon, 20 May 2024 08:07:48 +0100 Subject: [PATCH 3/5] fixed formatting --- .../methods/iter_on_single_or_empty_collections.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index f16385b5f95..e8bba0dfe74 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -72,11 +72,12 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method .. }, args, - ) => { - cx.typeck_results().qpath_res(path, *hir_id).opt_def_id() - .filter(|fn_id| cx.tcx.def_kind(fn_id).is_fn_like()) - .is_some_and(|fn_id| is_arg_ty_unified_in_fn(cx, fn_id, expr.hir_id, args)) - } + ) => cx + .typeck_results() + .qpath_res(path, *hir_id) + .opt_def_id() + .filter(|fn_id| cx.tcx.def_kind(fn_id).is_fn_like()) + .is_some_and(|fn_id| is_arg_ty_unified_in_fn(cx, fn_id, expr.hir_id, args)), ExprKind::MethodCall(_name, recv, args, _span) => is_arg_ty_unified_in_fn( cx, cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(), From b31625cdc7034df5fae32c4d6d6ff0e7d43496d8 Mon Sep 17 00:00:00 2001 From: schvv31n Date: Mon, 20 May 2024 21:27:30 +0100 Subject: [PATCH 4/5] Accounted for possible extra layers before the consuming parent expr --- .../src/methods/iter_on_single_or_empty_collections.rs | 4 ++-- tests/ui/iter_on_empty_collections.fixed | 5 +++++ tests/ui/iter_on_empty_collections.rs | 5 +++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index e8bba0dfe74..1bf323cf3c7 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -77,11 +77,11 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method .qpath_res(path, *hir_id) .opt_def_id() .filter(|fn_id| cx.tcx.def_kind(fn_id).is_fn_like()) - .is_some_and(|fn_id| is_arg_ty_unified_in_fn(cx, fn_id, expr.hir_id, args)), + .is_some_and(|fn_id| is_arg_ty_unified_in_fn(cx, fn_id, child_id, args)), ExprKind::MethodCall(_name, recv, args, _span) => is_arg_ty_unified_in_fn( cx, cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(), - expr.hir_id, + child_id, once(recv).chain(args.iter()), ), ExprKind::If(_, _, _) diff --git a/tests/ui/iter_on_empty_collections.fixed b/tests/ui/iter_on_empty_collections.fixed index 871cc18d7c2..4b5746c7b6f 100644 --- a/tests/ui/iter_on_empty_collections.fixed +++ b/tests/ui/iter_on_empty_collections.fixed @@ -29,6 +29,11 @@ fn array() { println!("{i}"); } + // Same as above, but for empty collection iters with extra layers + for i in smth.as_ref().map_or({ [].iter() }, |s| s.iter()) { + println!("{y}", y = i + 1); + } + // Same as above, but for regular function calls for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { println!("{i}"); diff --git a/tests/ui/iter_on_empty_collections.rs b/tests/ui/iter_on_empty_collections.rs index 7e3df49eefc..737192d556d 100644 --- a/tests/ui/iter_on_empty_collections.rs +++ b/tests/ui/iter_on_empty_collections.rs @@ -29,6 +29,11 @@ fn array() { println!("{i}"); } + // Same as above, but for empty collection iters with extra layers + for i in smth.as_ref().map_or({ [].iter() }, |s| s.iter()) { + println!("{y}", y = i + 1); + } + // Same as above, but for regular function calls for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { println!("{i}"); From 7439ecb07c85bcb148b3b62b2de0be746d5beac2 Mon Sep 17 00:00:00 2001 From: schvv31n Date: Tue, 21 May 2024 22:21:33 +0100 Subject: [PATCH 5/5] Added check for type unification with the iter --- .../src/methods/iter_on_single_or_empty_collections.rs | 10 +++++++--- tests/ui/iter_on_empty_collections.fixed | 4 ++++ tests/ui/iter_on_empty_collections.rs | 4 ++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs index 1bf323cf3c7..f4397212cf6 100644 --- a/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs +++ b/clippy_lints/src/methods/iter_on_single_or_empty_collections.rs @@ -37,14 +37,18 @@ fn is_arg_ty_unified_in_fn<'tcx>( ) -> bool { let fn_sig = cx.tcx.fn_sig(fn_id).instantiate_identity(); let arg_id_in_args = args.into_iter().position(|e| e.hir_id == arg_id).unwrap(); - let arg_ty_in_args = fn_sig.input(arg_id_in_args); + let arg_ty_in_args = fn_sig.input(arg_id_in_args).skip_binder(); cx.tcx.predicates_of(fn_id).predicates.iter().any(|(clause, _)| { clause .as_projection_clause() .and_then(|p| p.map_bound(|p| p.term.ty()).transpose()) - .is_some_and(|ty| ty == arg_ty_in_args) - }) + .is_some_and(|ty| ty.skip_binder() == arg_ty_in_args) + }) || fn_sig + .inputs() + .iter() + .enumerate() + .any(|(i, ty)| i != arg_id_in_args && ty.skip_binder().walk().any(|arg| arg.as_type() == Some(arg_ty_in_args))) } pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, method_name: &str, recv: &'tcx Expr<'tcx>) { diff --git a/tests/ui/iter_on_empty_collections.fixed b/tests/ui/iter_on_empty_collections.fixed index 4b5746c7b6f..0f28b48d9ab 100644 --- a/tests/ui/iter_on_empty_collections.fixed +++ b/tests/ui/iter_on_empty_collections.fixed @@ -38,6 +38,10 @@ fn array() { for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { println!("{i}"); } + + // Same as above, but when there are no predicates that mention the collection iter type. + let mut iter = [34, 228, 35].iter(); + let _ = std::mem::replace(&mut iter, [].iter()); } macro_rules! in_macros { diff --git a/tests/ui/iter_on_empty_collections.rs b/tests/ui/iter_on_empty_collections.rs index 737192d556d..702da514df7 100644 --- a/tests/ui/iter_on_empty_collections.rs +++ b/tests/ui/iter_on_empty_collections.rs @@ -38,6 +38,10 @@ fn array() { for i in Option::map_or(smth.as_ref(), [].iter(), |s| s.iter()) { println!("{i}"); } + + // Same as above, but when there are no predicates that mention the collection iter type. + let mut iter = [34, 228, 35].iter(); + let _ = std::mem::replace(&mut iter, [].iter()); } macro_rules! in_macros {