Fix explicit_into_iter_loop with mutable references

This commit is contained in:
Jason Newcomb 2023-02-27 13:28:27 -05:00
parent 974900b50e
commit 482baf2bcc
6 changed files with 188 additions and 77 deletions

View file

@ -5,15 +5,76 @@ use clippy_utils::source::snippet_with_applicability;
use rustc_errors::Applicability; use rustc_errors::Applicability;
use rustc_hir::Expr; use rustc_hir::Expr;
use rustc_lint::LateContext; use rustc_lint::LateContext;
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
use rustc_span::symbol::sym; 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<'_>) { pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>) {
let self_ty = cx.typeck_results().expr_ty(self_arg); if !is_trait_method(cx, call_expr, sym::IntoIterator) {
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)) {
return; 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 mut applicability = Applicability::MachineApplicable;
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability); let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
span_lint_and_sugg( 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 \ "it is more concise to loop over containers instead of using explicit \
iteration methods", iteration methods",
"to write this more concisely, try", "to write this more concisely, try",
object.to_string(), format!("{}{}", adjust.display(), object.to_string()),
applicability, applicability,
); );
} }

View file

@ -1,58 +1,39 @@
use super::EXPLICIT_ITER_LOOP; use super::EXPLICIT_ITER_LOOP;
use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_trait_method;
use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_applicability; use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::{implements_trait_with_env, make_normalized_projection_with_regions, normalize_with_regions, use clippy_utils::ty::{
make_normalized_projection, implements_trait, is_copy}; 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_errors::Applicability;
use rustc_hir::{Expr, Mutability}; use rustc_hir::{Expr, Mutability};
use rustc_lint::LateContext; use rustc_lint::LateContext;
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; 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; use rustc_span::sym;
pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, method_name: &str, msrv: &Msrv) { pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, msrv: &Msrv) {
let borrow_kind = match method_name { let Some((adjust, ty)) = is_ref_iterable(cx, self_arg, call_expr) else {
"iter" | "iter_mut" => match is_ref_iterable(cx, self_arg, call_expr) { return;
Some((kind, ty)) => { };
if let ty::Array(_, count) = *ty.peel_refs().kind() { if let ty::Array(_, count) = *ty.peel_refs().kind() {
if !ty.is_ref() { if !ty.is_ref() {
if !msrv.meets(msrvs::ARRAY_INTO_ITERATOR) { 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 {
return; return;
} }
}, } else if count
_ => return, .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 mut applicability = Applicability::MachineApplicable;
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability); let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
let prefix = match borrow_kind { let prefix = match adjust {
AdjustKind::None => "", AdjustKind::None => "",
AdjustKind::Borrow => "&", AdjustKind::Borrow => "&",
AdjustKind::BorrowMut => "&mut ", AdjustKind::BorrowMut => "&mut ",
@ -105,7 +86,11 @@ impl AdjustKind {
/// Checks if an `iter` or `iter_mut` call returns `IntoIterator::IntoIter`. Returns how the /// Checks if an `iter` or `iter_mut` call returns `IntoIterator::IntoIter`. Returns how the
/// argument needs to be adjusted. /// 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(); let typeck = cx.typeck_results();
if let Some(trait_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) 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) && 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 { match adjustments {
[] => Some((AdjustKind::None, self_ty)), [] => 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 if target != self_ty
&& implements_trait(cx, target, trait_id, &[]) && 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 && ty == res_ty
{ {
Some((AdjustKind::reborrow(mutbl), target)) 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 }, ..] => { &[Adjustment { kind: Adjust::Deref(_), target }, ..] => {
if is_copy(cx, target) if is_copy(cx, target)
&& implements_trait(cx, target, trait_id, &[]) && 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 && ty == res_ty
{ {
Some((AdjustKind::Deref, target)) Some((AdjustKind::Deref, target))
@ -180,10 +174,17 @@ fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr:
None None
} }
} }
&[Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), target }, ..] => { &[
Adjustment {
kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)),
target,
},
..
] => {
if self_ty.is_ref() if self_ty.is_ref()
&& implements_trait(cx, target, trait_id, &[]) && 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 && ty == res_ty
{ {
Some((AdjustKind::auto_borrow(mutbl), target)) Some((AdjustKind::auto_borrow(mutbl), target))

View file

@ -715,14 +715,11 @@ impl Loops {
fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) { fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) {
if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind { if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind {
let method_name = method.ident.as_str(); match method.ident.as_str() {
// check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
match method_name {
"iter" | "iter_mut" => { "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" => { "into_iter" => {
explicit_iter_loop::check(cx, self_arg, arg, method_name, &self.msrv);
explicit_into_iter_loop::check(cx, self_arg, arg); explicit_into_iter_loop::check(cx, self_arg, arg);
}, },
"next" => { "next" => {

View file

@ -7,9 +7,7 @@ fn main() {
where where
for<'a> &'a T: IntoIterator<Item = &'a String>, for<'a> &'a T: IntoIterator<Item = &'a String>,
{ {
for i in iterator { for _ in iterator {}
println!("{}", i);
}
} }
struct T; struct T;
@ -17,23 +15,39 @@ fn main() {
type Item = (); type Item = ();
type IntoIter = std::vec::IntoIter<Self::Item>; type IntoIter = std::vec::IntoIter<Self::Item>;
fn into_iter(self) -> Self::IntoIter { fn into_iter(self) -> Self::IntoIter {
vec![].into_iter() unimplemented!()
} }
} }
let t = T; let mut t = T;
for _ in &t {}
let r = &t; let r = &t;
let rr = &&t;
// This case is handled by `explicit_iter_loop`. No idea why.
for _ in t.into_iter() {}
for _ in r {} for _ in r {}
// No suggestion for this. // No suggestion for this.
// We'd have to suggest `for _ in *rr {}` which is less clear. // We'd have to suggest `for _ in *rr {}` which is less clear.
let rr = &&t;
for _ in rr.into_iter() {} 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<Self::Item>;
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 // Issue #6900
struct S; struct S;
impl S { impl S {

View file

@ -7,9 +7,7 @@ fn main() {
where where
for<'a> &'a T: IntoIterator<Item = &'a String>, for<'a> &'a T: IntoIterator<Item = &'a String>,
{ {
for i in iterator.into_iter() { for _ in iterator.into_iter() {}
println!("{}", i);
}
} }
struct T; struct T;
@ -17,23 +15,39 @@ fn main() {
type Item = (); type Item = ();
type IntoIter = std::vec::IntoIter<Self::Item>; type IntoIter = std::vec::IntoIter<Self::Item>;
fn into_iter(self) -> Self::IntoIter { fn into_iter(self) -> Self::IntoIter {
vec![].into_iter() unimplemented!()
} }
} }
let t = T; let mut t = T;
let r = &t;
let rr = &&t;
// This case is handled by `explicit_iter_loop`. No idea why.
for _ in t.into_iter() {} for _ in t.into_iter() {}
let r = &t;
for _ in r.into_iter() {} for _ in r.into_iter() {}
// No suggestion for this. // No suggestion for this.
// We'd have to suggest `for _ in *rr {}` which is less clear. // We'd have to suggest `for _ in *rr {}` which is less clear.
let rr = &&t;
for _ in rr.into_iter() {} 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<Self::Item>;
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 // Issue #6900
struct S; struct S;
impl S { impl S {

View file

@ -1,16 +1,40 @@
error: it is more concise to loop over containers instead of using explicit iteration methods error: it is more concise to loop over containers instead of using explicit iteration methods
--> $DIR/explicit_into_iter_loop.rs:10:18 --> $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` | ^^^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `iterator`
| |
= note: `-D clippy::explicit-into-iter-loop` implied by `-D warnings` = 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 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() {} LL | for _ in r.into_iter() {}
| ^^^^^^^^^^^^^ help: to write this more concisely, try: `r` | ^^^^^^^^^^^^^ 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