Skip to content

Commit

Permalink
Auto merge of #12823 - schvv31n:fix-iter-on-empty-collections, r=y21
Browse files Browse the repository at this point in the history
Suppress `iter_on_empty_collections` if the iterator's concrete type is relied upon

changelog: fixed #12807
  • Loading branch information
bors committed May 27, 2024
2 parents 7e4c1ae + 7439ecb commit 76eee82
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 2 deletions.
47 changes: 46 additions & 1 deletion clippy_lints/src/methods/iter_on_single_or_empty_collections.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -25,7 +29,29 @@ 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 IntoIterator<Item = &'tcx Expr<'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).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.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>) {
let item = match recv.kind {
ExprKind::Array([]) => None,
ExprKind::Array([e]) => Some(e),
Expand All @@ -43,6 +69,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,
) => 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, 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(),
child_id,
once(recv).chain(args.iter()),
),
ExprKind::If(_, _, _)
| ExprKind::Match(_, _, _)
| ExprKind::Closure(_)
Expand Down
22 changes: 22 additions & 0 deletions tests/ui/iter_on_empty_collections.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,28 @@ 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 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}");
}

// 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 {
Expand Down
22 changes: 22 additions & 0 deletions tests/ui/iter_on_empty_collections.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,28 @@ 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 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}");
}

// 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 {
Expand Down
8 changes: 7 additions & 1 deletion tests/ui/iter_on_empty_collections.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 76eee82

Please sign in to comment.