Skip to content

Commit f916c29

Browse files
JarchoJarcho
committed
Fix explicit_into_iter_loop with mutable references
1 parent c78be03 commit f916c29

6 files changed

+188
-77
lines changed

Diff for: ‎clippy_lints/src/loops/explicit_into_iter_loop.rs

+65-4
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,76 @@ use clippy_utils::source::snippet_with_applicability;
55
use rustc_errors::Applicability;
66
use rustc_hir::Expr;
77
use rustc_lint::LateContext;
8+
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
89
use rustc_span::symbol::sym;
910

11+
#[derive(Clone, Copy)]
12+
enum AdjustKind {
13+
None,
14+
Borrow,
15+
BorrowMut,
16+
Reborrow,
17+
ReborrowMut,
18+
}
19+
impl AdjustKind {
20+
fn borrow(mutbl: AutoBorrowMutability) -> Self {
21+
match mutbl {
22+
AutoBorrowMutability::Not => Self::Borrow,
23+
AutoBorrowMutability::Mut { .. } => Self::BorrowMut,
24+
}
25+
}
26+
27+
fn reborrow(mutbl: AutoBorrowMutability) -> Self {
28+
match mutbl {
29+
AutoBorrowMutability::Not => Self::Reborrow,
30+
AutoBorrowMutability::Mut { .. } => Self::ReborrowMut,
31+
}
32+
}
33+
34+
fn display(self) -> &'static str {
35+
match self {
36+
Self::None => "",
37+
Self::Borrow => "&",
38+
Self::BorrowMut => "&mut ",
39+
Self::Reborrow => "&*",
40+
Self::ReborrowMut => "&mut *",
41+
}
42+
}
43+
}
44+
1045
pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>) {
11-
let self_ty = cx.typeck_results().expr_ty(self_arg);
12-
let self_ty_adjusted = cx.typeck_results().expr_ty_adjusted(self_arg);
13-
if !(self_ty == self_ty_adjusted && is_trait_method(cx, call_expr, sym::IntoIterator)) {
46+
if !is_trait_method(cx, call_expr, sym::IntoIterator) {
1447
return;
1548
}
1649

50+
let typeck = cx.typeck_results();
51+
let self_ty = typeck.expr_ty(self_arg);
52+
let adjust = match typeck.expr_adjustments(self_arg) {
53+
[] => AdjustKind::None,
54+
&[
55+
Adjustment {
56+
kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)),
57+
..
58+
},
59+
] => AdjustKind::borrow(mutbl),
60+
&[
61+
Adjustment {
62+
kind: Adjust::Deref(_), ..
63+
},
64+
Adjustment {
65+
kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)),
66+
target,
67+
},
68+
] => {
69+
if self_ty == target && matches!(mutbl, AutoBorrowMutability::Not) {
70+
AdjustKind::None
71+
} else {
72+
AdjustKind::reborrow(mutbl)
73+
}
74+
},
75+
_ => return,
76+
};
77+
1778
let mut applicability = Applicability::MachineApplicable;
1879
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
1980
span_lint_and_sugg(
@@ -23,7 +84,7 @@ pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<
2384
"it is more concise to loop over containers instead of using explicit \
2485
iteration methods",
2586
"to write this more concisely, try",
26-
object.to_string(),
87+
format!("{}{}", adjust.display(), object.to_string()),
2788
applicability,
2889
);
2990
}

Diff for: ‎clippy_lints/src/loops/explicit_iter_loop.rs

+47-46
Original file line numberDiff line numberDiff line change
@@ -1,58 +1,39 @@
11
use super::EXPLICIT_ITER_LOOP;
22
use clippy_utils::diagnostics::span_lint_and_sugg;
3-
use clippy_utils::is_trait_method;
43
use clippy_utils::msrvs::{self, Msrv};
54
use clippy_utils::source::snippet_with_applicability;
6-
use clippy_utils::ty::{implements_trait_with_env, make_normalized_projection_with_regions, normalize_with_regions,
7-
make_normalized_projection, implements_trait, is_copy};
5+
use clippy_utils::ty::{
6+
implements_trait, implements_trait_with_env, is_copy, make_normalized_projection,
7+
make_normalized_projection_with_regions, normalize_with_regions,
8+
};
89
use rustc_errors::Applicability;
910
use rustc_hir::{Expr, Mutability};
1011
use rustc_lint::LateContext;
1112
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
12-
use rustc_middle::ty::{self, EarlyBinder, TypeAndMut, Ty};
13+
use rustc_middle::ty::{self, EarlyBinder, Ty, TypeAndMut};
1314
use rustc_span::sym;
1415

15-
pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, method_name: &str, msrv: &Msrv) {
16-
let borrow_kind = match method_name {
17-
"iter" | "iter_mut" => match is_ref_iterable(cx, self_arg, call_expr) {
18-
Some((kind, ty)) => {
19-
if let ty::Array(_, count) = *ty.peel_refs().kind() {
20-
if !ty.is_ref() {
21-
if !msrv.meets(msrvs::ARRAY_INTO_ITERATOR) {
22-
return;
23-
}
24-
} else if count.try_eval_target_usize(cx.tcx, cx.param_env).map_or(true, |x| x > 32)
25-
&& !msrv.meets(msrvs::ARRAY_IMPL_ANY_LEN)
26-
{
27-
return
28-
}
29-
}
30-
kind
31-
},
32-
None => return,
33-
},
34-
"into_iter" if is_trait_method(cx, call_expr, sym::IntoIterator) => {
35-
let receiver_ty = cx.typeck_results().expr_ty(self_arg);
36-
let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(self_arg);
37-
let ref_receiver_ty = cx.tcx.mk_ref(
38-
cx.tcx.lifetimes.re_erased,
39-
ty::TypeAndMut {
40-
ty: receiver_ty,
41-
mutbl: Mutability::Not,
42-
},
43-
);
44-
if receiver_ty_adjusted == ref_receiver_ty {
45-
AdjustKind::None
46-
} else {
16+
pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, msrv: &Msrv) {
17+
let Some((adjust, ty)) = is_ref_iterable(cx, self_arg, call_expr) else {
18+
return;
19+
};
20+
if let ty::Array(_, count) = *ty.peel_refs().kind() {
21+
if !ty.is_ref() {
22+
if !msrv.meets(msrvs::ARRAY_INTO_ITERATOR) {
4723
return;
4824
}
49-
},
50-
_ => return,
51-
};
25+
} else if count
26+
.try_eval_target_usize(cx.tcx, cx.param_env)
27+
.map_or(true, |x| x > 32)
28+
&& !msrv.meets(msrvs::ARRAY_IMPL_ANY_LEN)
29+
{
30+
return;
31+
}
32+
}
5233

5334
let mut applicability = Applicability::MachineApplicable;
5435
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
55-
let prefix = match borrow_kind {
36+
let prefix = match adjust {
5637
AdjustKind::None => "",
5738
AdjustKind::Borrow => "&",
5839
AdjustKind::BorrowMut => "&mut ",
@@ -105,7 +86,11 @@ impl AdjustKind {
10586

10687
/// Checks if an `iter` or `iter_mut` call returns `IntoIterator::IntoIter`. Returns how the
10788
/// argument needs to be adjusted.
108-
fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: &Expr<'_>) -> Option<(AdjustKind, Ty<'tcx>)> {
89+
fn is_ref_iterable<'tcx>(
90+
cx: &LateContext<'tcx>,
91+
self_arg: &Expr<'_>,
92+
call_expr: &Expr<'_>,
93+
) -> Option<(AdjustKind, Ty<'tcx>)> {
10994
let typeck = cx.typeck_results();
11095
if let Some(trait_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
11196
&& 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:
158143

159144
match adjustments {
160145
[] => Some((AdjustKind::None, self_ty)),
161-
&[Adjustment { kind: Adjust::Deref(_), ..}, Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), target }, ..] => {
146+
&[
147+
Adjustment { kind: Adjust::Deref(_), ..},
148+
Adjustment {
149+
kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)),
150+
target,
151+
},
152+
..
153+
] => {
162154
if target != self_ty
163155
&& implements_trait(cx, target, trait_id, &[])
164-
&& let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
156+
&& let Some(ty) =
157+
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
165158
&& ty == res_ty
166159
{
167160
Some((AdjustKind::reborrow(mutbl), target))
@@ -172,18 +165,26 @@ fn is_ref_iterable<'tcx>(cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr:
172165
&[Adjustment { kind: Adjust::Deref(_), target }, ..] => {
173166
if is_copy(cx, target)
174167
&& implements_trait(cx, target, trait_id, &[])
175-
&& let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
168+
&& let Some(ty) =
169+
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
176170
&& ty == res_ty
177171
{
178172
Some((AdjustKind::Deref, target))
179173
} else {
180174
None
181175
}
182176
}
183-
&[Adjustment { kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)), target }, ..] => {
177+
&[
178+
Adjustment {
179+
kind: Adjust::Borrow(AutoBorrow::Ref(_, mutbl)),
180+
target,
181+
},
182+
..
183+
] => {
184184
if self_ty.is_ref()
185185
&& implements_trait(cx, target, trait_id, &[])
186-
&& let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
186+
&& let Some(ty) =
187+
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
187188
&& ty == res_ty
188189
{
189190
Some((AdjustKind::auto_borrow(mutbl), target))

Diff for: ‎clippy_lints/src/loops/mod.rs

+2-5
Original file line numberDiff line numberDiff line change
@@ -715,14 +715,11 @@ impl Loops {
715715

716716
fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) {
717717
if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind {
718-
let method_name = method.ident.as_str();
719-
// check for looping over x.iter() or x.iter_mut(), could use &x or &mut x
720-
match method_name {
718+
match method.ident.as_str() {
721719
"iter" | "iter_mut" => {
722-
explicit_iter_loop::check(cx, self_arg, arg, method_name, &self.msrv);
720+
explicit_iter_loop::check(cx, self_arg, arg, &self.msrv);
723721
},
724722
"into_iter" => {
725-
explicit_iter_loop::check(cx, self_arg, arg, method_name, &self.msrv);
726723
explicit_into_iter_loop::check(cx, self_arg, arg);
727724
},
728725
"next" => {

Diff for: ‎tests/ui/explicit_into_iter_loop.fixed

+24-10
Original file line numberDiff line numberDiff line change
@@ -7,33 +7,47 @@ fn main() {
77
where
88
for<'a> &'a T: IntoIterator<Item = &'a String>,
99
{
10-
for i in iterator {
11-
println!("{}", i);
12-
}
10+
for _ in iterator {}
1311
}
1412

1513
struct T;
1614
impl IntoIterator for &T {
1715
type Item = ();
1816
type IntoIter = std::vec::IntoIter<Self::Item>;
1917
fn into_iter(self) -> Self::IntoIter {
20-
vec![].into_iter()
18+
unimplemented!()
2119
}
2220
}
2321

24-
let t = T;
25-
let r = &t;
26-
let rr = &&t;
27-
28-
// This case is handled by `explicit_iter_loop`. No idea why.
29-
for _ in t.into_iter() {}
22+
let mut t = T;
23+
for _ in &t {}
3024

25+
let r = &t;
3126
for _ in r {}
3227

3328
// No suggestion for this.
3429
// We'd have to suggest `for _ in *rr {}` which is less clear.
30+
let rr = &&t;
3531
for _ in rr.into_iter() {}
3632

33+
let mr = &mut t;
34+
for _ in &*mr {}
35+
36+
struct U;
37+
impl IntoIterator for &mut U {
38+
type Item = ();
39+
type IntoIter = std::vec::IntoIter<Self::Item>;
40+
fn into_iter(self) -> Self::IntoIter {
41+
unimplemented!()
42+
}
43+
}
44+
45+
let mut u = U;
46+
for _ in &mut u {}
47+
48+
let mr = &mut u;
49+
for _ in &mut *mr {}
50+
3751
// Issue #6900
3852
struct S;
3953
impl S {

Diff for: ‎tests/ui/explicit_into_iter_loop.rs

+23-9
Original file line numberDiff line numberDiff line change
@@ -7,33 +7,47 @@ fn main() {
77
where
88
for<'a> &'a T: IntoIterator<Item = &'a String>,
99
{
10-
for i in iterator.into_iter() {
11-
println!("{}", i);
12-
}
10+
for _ in iterator.into_iter() {}
1311
}
1412

1513
struct T;
1614
impl IntoIterator for &T {
1715
type Item = ();
1816
type IntoIter = std::vec::IntoIter<Self::Item>;
1917
fn into_iter(self) -> Self::IntoIter {
20-
vec![].into_iter()
18+
unimplemented!()
2119
}
2220
}
2321

24-
let t = T;
25-
let r = &t;
26-
let rr = &&t;
27-
28-
// This case is handled by `explicit_iter_loop`. No idea why.
22+
let mut t = T;
2923
for _ in t.into_iter() {}
3024

25+
let r = &t;
3126
for _ in r.into_iter() {}
3227

3328
// No suggestion for this.
3429
// We'd have to suggest `for _ in *rr {}` which is less clear.
30+
let rr = &&t;
3531
for _ in rr.into_iter() {}
3632

33+
let mr = &mut t;
34+
for _ in mr.into_iter() {}
35+
36+
struct U;
37+
impl IntoIterator for &mut U {
38+
type Item = ();
39+
type IntoIter = std::vec::IntoIter<Self::Item>;
40+
fn into_iter(self) -> Self::IntoIter {
41+
unimplemented!()
42+
}
43+
}
44+
45+
let mut u = U;
46+
for _ in u.into_iter() {}
47+
48+
let mr = &mut u;
49+
for _ in mr.into_iter() {}
50+
3751
// Issue #6900
3852
struct S;
3953
impl S {

0 commit comments

Comments
 (0)