From edd8360db74c91e87dfb7867ecccdfca6d400499 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 27 Feb 2023 13:28:27 -0500 Subject: [PATCH] Fix `explicit_into_iter_loop` with mutable references --- .../src/loops/explicit_into_iter_loop.rs | 69 +++++++++++++- clippy_lints/src/loops/explicit_iter_loop.rs | 93 ++++++++++--------- clippy_lints/src/loops/mod.rs | 7 +- tests/ui/explicit_into_iter_loop.fixed | 34 +++++-- tests/ui/explicit_into_iter_loop.rs | 32 +++++-- tests/ui/explicit_into_iter_loop.stderr | 30 +++++- 6 files changed, 188 insertions(+), 77 deletions(-) diff --git a/clippy_lints/src/loops/explicit_into_iter_loop.rs b/clippy_lints/src/loops/explicit_into_iter_loop.rs index 175e2b382e3f..bf5e018ce922 100644 --- a/clippy_lints/src/loops/explicit_into_iter_loop.rs +++ b/clippy_lints/src/loops/explicit_into_iter_loop.rs @@ -5,15 +5,76 @@ use clippy_utils::source::snippet_with_applicability; use rustc_errors::Applicability; use rustc_hir::Expr; use rustc_lint::LateContext; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_span::symbol::sym; +#[derive(Clone, Copy)] +enum AdjustKind { + None, + Borrow, + BorrowMut, + Reborrow, + ReborrowMut, +} +impl AdjustKind { + fn borrow(mutbl: AutoBorrowMutability) -> Self { + match mutbl { + AutoBorrowMutability::Not => Self::Borrow, + AutoBorrowMutability::Mut { .. } => Self::BorrowMut, + } + } + + fn reborrow(mutbl: AutoBorrowMutability) -> Self { + match mutbl { + AutoBorrowMutability::Not => Self::Reborrow, + AutoBorrowMutability::Mut { .. } => Self::ReborrowMut, + } + } + + fn display(self) -> &'static str { + match self { + Self::None => "", + Self::Borrow => "&", + Self::BorrowMut => "&mut ", + Self::Reborrow => "&*", + Self::ReborrowMut => "&mut *", + } + } +} + pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>) { - let self_ty = cx.typeck_results().expr_ty(self_arg); - let self_ty_adjusted = cx.typeck_results().expr_ty_adjusted(self_arg); - if !(self_ty == self_ty_adjusted && is_trait_method(cx, call_expr, sym::IntoIterator)) { + if !is_trait_method(cx, call_expr, sym::IntoIterator) { return; } + let typeck = cx.typeck_results(); + let self_ty = typeck.expr_ty(self_arg); + let adjust = match typeck.expr_adjustments(self_arg) { + [] => AdjustKind::None, + &[ + Adjustment { + kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), + .. + }, + ] => AdjustKind::borrow(mutbl), + &[ + Adjustment { + kind: Adjust::Deref(_), .. + }, + Adjustment { + kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), + target, + }, + ] => { + if self_ty == target && matches!(mutbl, AutoBorrowMutability::Not) { + AdjustKind::None + } else { + AdjustKind::reborrow(mutbl) + } + }, + _ => return, + }; + let mut applicability = Applicability::MachineApplicable; let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability); span_lint_and_sugg( @@ -23,7 +84,7 @@ pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr< "it is more concise to loop over containers instead of using explicit \ iteration methods", "to write this more concisely, try", - object.to_string(), + format!("{}{}", adjust.display(), object.to_string()), applicability, ); } diff --git a/clippy_lints/src/loops/explicit_iter_loop.rs b/clippy_lints/src/loops/explicit_iter_loop.rs index 3b5c1f2fa262..f9b26f4a55e4 100644 --- a/clippy_lints/src/loops/explicit_iter_loop.rs +++ b/clippy_lints/src/loops/explicit_iter_loop.rs @@ -1,58 +1,39 @@ use super::EXPLICIT_ITER_LOOP; use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_trait_method; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::{implements_trait_with_env, make_normalized_projection_with_regions, normalize_with_regions, - make_normalized_projection, implements_trait, is_copy}; +use clippy_utils::ty::{ + implements_trait, implements_trait_with_env, is_copy, make_normalized_projection, + make_normalized_projection_with_regions, normalize_with_regions, +}; use rustc_errors::Applicability; use rustc_hir::{Expr, Mutability}; use rustc_lint::LateContext; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; -use rustc_middle::ty::{self, EarlyBinder, TypeAndMut, Ty}; +use rustc_middle::ty::{self, EarlyBinder, Ty, TypeAndMut}; use rustc_span::sym; -pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, method_name: &str, msrv: &Msrv) { - let borrow_kind = match method_name { - "iter" | "iter_mut" => match is_ref_iterable(cx, self_arg, call_expr) { - Some((kind, ty)) => { - if let ty::Array(_, count) = *ty.peel_refs().kind() { - if !ty.is_ref() { - if !msrv.meets(msrvs::ARRAY_INTO_ITERATOR) { - return; - } - } else if count.try_eval_target_usize(cx.tcx, cx.param_env).map_or(true, |x| x > 32) - && !msrv.meets(msrvs::ARRAY_IMPL_ANY_LEN) - { - return - } - } - kind - }, - None => return, - }, - "into_iter" if is_trait_method(cx, call_expr, sym::IntoIterator) => { - let receiver_ty = cx.typeck_results().expr_ty(self_arg); - let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(self_arg); - let ref_receiver_ty = cx.tcx.mk_ref( - cx.tcx.lifetimes.re_erased, - ty::TypeAndMut { - ty: receiver_ty, - mutbl: Mutability::Not, - }, - ); - if receiver_ty_adjusted == ref_receiver_ty { - AdjustKind::None - } else { +pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, msrv: &Msrv) { + let Some((adjust, ty)) = is_ref_iterable(cx, self_arg, call_expr) else { + return; + }; + if let ty::Array(_, count) = *ty.peel_refs().kind() { + if !ty.is_ref() { + if !msrv.meets(msrvs::ARRAY_INTO_ITERATOR) { return; } - }, - _ => return, - }; + } else if count + .try_eval_target_usize(cx.tcx, cx.param_env) + .map_or(true, |x| x > 32) + && !msrv.meets(msrvs::ARRAY_IMPL_ANY_LEN) + { + return; + } + } let mut applicability = Applicability::MachineApplicable; let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability); - let prefix = match borrow_kind { + let prefix = match adjust { AdjustKind::None => "", AdjustKind::Borrow => "&", AdjustKind::BorrowMut => "&mut ", @@ -105,7 +86,11 @@ impl AdjustKind { /// Checks if an `iter` or `iter_mut` call returns `IntoIterator::IntoIter`. Returns how the /// argument needs to be adjusted. -fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: &Expr<'_>) -> Option<(AdjustKind, Ty<'tcx>)> { +fn is_ref_iterable<'tcx>( + cx: &LateContext<'tcx>, + self_arg: &Expr<'_>, + call_expr: &Expr<'_>, +) -> Option<(AdjustKind, Ty<'tcx>)> { let typeck = cx.typeck_results(); if let Some(trait_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) && let Some(fn_id) = typeck.type_dependent_def_id(call_expr.hir_id) @@ -158,10 +143,18 @@ fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: match adjustments { [] => Some((AdjustKind::None, self_ty)), - &[Adjustment { kind: Adjust::Deref(_), ..}, Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), target }, ..] => { + &[ + Adjustment { kind: Adjust::Deref(_), ..}, + Adjustment { + kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), + target, + }, + .. + ] => { if target != self_ty && implements_trait(cx, target, trait_id, &[]) - && let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) + && let Some(ty) = + make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) && ty == res_ty { Some((AdjustKind::reborrow(mutbl), target)) @@ -172,7 +165,8 @@ fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: &[Adjustment { kind: Adjust::Deref(_), target }, ..] => { if is_copy(cx, target) && implements_trait(cx, target, trait_id, &[]) - && let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) + && let Some(ty) = + make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) && ty == res_ty { Some((AdjustKind::Deref, target)) @@ -180,10 +174,17 @@ fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: None } } - &[Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), target }, ..] => { + &[ + Adjustment { + kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), + target, + }, + .. + ] => { if self_ty.is_ref() && implements_trait(cx, target, trait_id, &[]) - && let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) + && let Some(ty) = + make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target]) && ty == res_ty { Some((AdjustKind::auto_borrow(mutbl), target)) diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 6eb9a9830e77..7d46b0be365b 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -682,14 +682,11 @@ impl Loops { fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) { if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind { - let method_name = method.ident.as_str(); - // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x - match method_name { + match method.ident.as_str() { "iter" | "iter_mut" => { - explicit_iter_loop::check(cx, self_arg, arg, method_name, &self.msrv); + explicit_iter_loop::check(cx, self_arg, arg, &self.msrv); }, "into_iter" => { - explicit_iter_loop::check(cx, self_arg, arg, method_name, &self.msrv); explicit_into_iter_loop::check(cx, self_arg, arg); }, "next" => { diff --git a/tests/ui/explicit_into_iter_loop.fixed b/tests/ui/explicit_into_iter_loop.fixed index 703c68593374..0640c9c6288b 100644 --- a/tests/ui/explicit_into_iter_loop.fixed +++ b/tests/ui/explicit_into_iter_loop.fixed @@ -7,9 +7,7 @@ fn main() { where for<'a> &'a T: IntoIterator, { - for i in iterator { - println!("{}", i); - } + for _ in iterator {} } struct T; @@ -17,23 +15,39 @@ fn main() { type Item = (); type IntoIter = std::vec::IntoIter; fn into_iter(self) -> Self::IntoIter { - vec![].into_iter() + unimplemented!() } } - let t = T; - let r = &t; - let rr = &&t; - - // This case is handled by `explicit_iter_loop`. No idea why. - for _ in t.into_iter() {} + let mut t = T; + for _ in &t {} + let r = &t; for _ in r {} // No suggestion for this. // We'd have to suggest `for _ in *rr {}` which is less clear. + let rr = &&t; for _ in rr.into_iter() {} + let mr = &mut t; + for _ in &*mr {} + + struct U; + impl IntoIterator for &mut U { + type Item = (); + type IntoIter = std::vec::IntoIter; + fn into_iter(self) -> Self::IntoIter { + unimplemented!() + } + } + + let mut u = U; + for _ in &mut u {} + + let mr = &mut u; + for _ in &mut *mr {} + // Issue #6900 struct S; impl S { diff --git a/tests/ui/explicit_into_iter_loop.rs b/tests/ui/explicit_into_iter_loop.rs index 95a1c7426542..dd0d70952133 100644 --- a/tests/ui/explicit_into_iter_loop.rs +++ b/tests/ui/explicit_into_iter_loop.rs @@ -7,9 +7,7 @@ fn main() { where for<'a> &'a T: IntoIterator, { - for i in iterator.into_iter() { - println!("{}", i); - } + for _ in iterator.into_iter() {} } struct T; @@ -17,23 +15,39 @@ fn main() { type Item = (); type IntoIter = std::vec::IntoIter; fn into_iter(self) -> Self::IntoIter { - vec![].into_iter() + unimplemented!() } } - let t = T; - let r = &t; - let rr = &&t; - - // This case is handled by `explicit_iter_loop`. No idea why. + let mut t = T; for _ in t.into_iter() {} + let r = &t; for _ in r.into_iter() {} // No suggestion for this. // We'd have to suggest `for _ in *rr {}` which is less clear. + let rr = &&t; for _ in rr.into_iter() {} + let mr = &mut t; + for _ in mr.into_iter() {} + + struct U; + impl IntoIterator for &mut U { + type Item = (); + type IntoIter = std::vec::IntoIter; + fn into_iter(self) -> Self::IntoIter { + unimplemented!() + } + } + + let mut u = U; + for _ in u.into_iter() {} + + let mr = &mut u; + for _ in mr.into_iter() {} + // Issue #6900 struct S; impl S { diff --git a/tests/ui/explicit_into_iter_loop.stderr b/tests/ui/explicit_into_iter_loop.stderr index 1bd2b38a0e78..fa89b884fa0f 100644 --- a/tests/ui/explicit_into_iter_loop.stderr +++ b/tests/ui/explicit_into_iter_loop.stderr @@ -1,16 +1,40 @@ error: it is more concise to loop over containers instead of using explicit iteration methods --> $DIR/explicit_into_iter_loop.rs:10:18 | -LL | for i in iterator.into_iter() { +LL | for _ in iterator.into_iter() {} | ^^^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `iterator` | = note: `-D clippy::explicit-into-iter-loop` implied by `-D warnings` error: it is more concise to loop over containers instead of using explicit iteration methods - --> $DIR/explicit_into_iter_loop.rs:31:14 + --> $DIR/explicit_into_iter_loop.rs:23:14 + | +LL | for _ in t.into_iter() {} + | ^^^^^^^^^^^^^ help: to write this more concisely, try: `&t` + +error: it is more concise to loop over containers instead of using explicit iteration methods + --> $DIR/explicit_into_iter_loop.rs:26:14 | LL | for _ in r.into_iter() {} | ^^^^^^^^^^^^^ help: to write this more concisely, try: `r` -error: aborting due to 2 previous errors +error: it is more concise to loop over containers instead of using explicit iteration methods + --> $DIR/explicit_into_iter_loop.rs:34:14 + | +LL | for _ in mr.into_iter() {} + | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&*mr` + +error: it is more concise to loop over containers instead of using explicit iteration methods + --> $DIR/explicit_into_iter_loop.rs:46:14 + | +LL | for _ in u.into_iter() {} + | ^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut u` + +error: it is more concise to loop over containers instead of using explicit iteration methods + --> $DIR/explicit_into_iter_loop.rs:49:14 + | +LL | for _ in mr.into_iter() {} + | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut *mr` + +error: aborting due to 6 previous errors