Skip to content

Commit

Permalink
Split explicit_iter_loop into std and deref
Browse files Browse the repository at this point in the history
Referencing this raised issue: rust-lang#11074

The lint adds too much visual noise when deref lints are enforced. This commit
splits the explicit_iter_loop into two versions, the first, `std`, where
non-deref types are linted, and the second `deref` where deref types are
linted.
  • Loading branch information
Benjscho committed Aug 25, 2023
1 parent 706c48b commit 45ec139
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 77 deletions.
3 changes: 2 additions & 1 deletion clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::loops::EMPTY_LOOP_INFO,
crate::loops::EXPLICIT_COUNTER_LOOP_INFO,
crate::loops::EXPLICIT_INTO_ITER_LOOP_INFO,
crate::loops::EXPLICIT_ITER_LOOP_INFO,
crate::loops::EXPLICIT_ITER_LOOP_DEREF_INFO,
crate::loops::EXPLICIT_ITER_LOOP_STD_INFO,
crate::loops::FOR_KV_MAP_INFO,
crate::loops::ITER_NEXT_LOOP_INFO,
crate::loops::MANUAL_FIND_INFO,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::EXPLICIT_ITER_LOOP;
use super::EXPLICIT_ITER_LOOP_DEREF;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_applicability;
Expand Down Expand Up @@ -35,7 +35,7 @@ pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
span_lint_and_sugg(
cx,
EXPLICIT_ITER_LOOP,
EXPLICIT_ITER_LOOP_DEREF,
call_expr.span,
"it is more concise to loop over references to containers instead of using explicit \
iteration methods",
Expand All @@ -47,28 +47,11 @@ pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<

#[derive(Clone, Copy)]
enum AdjustKind {
None,
Borrow,
BorrowMut,
Deref,
Reborrow,
ReborrowMut,
}
impl AdjustKind {
fn borrow(mutbl: Mutability) -> Self {
match mutbl {
Mutability::Not => Self::Borrow,
Mutability::Mut => Self::BorrowMut,
}
}

fn auto_borrow(mutbl: AutoBorrowMutability) -> Self {
match mutbl {
AutoBorrowMutability::Not => Self::Borrow,
AutoBorrowMutability::Mut { .. } => Self::BorrowMut,
}
}

fn reborrow(mutbl: Mutability) -> Self {
match mutbl {
Mutability::Not => Self::Reborrow,
Expand All @@ -85,9 +68,6 @@ impl AdjustKind {

fn display(self) -> &'static str {
match self {
Self::None => "",
Self::Borrow => "&",
Self::BorrowMut => "&mut ",
Self::Deref => "*",
Self::Reborrow => "&*",
Self::ReborrowMut => "&mut *",
Expand Down Expand Up @@ -117,12 +97,6 @@ fn is_ref_iterable<'tcx>(
{
let adjustments = typeck.expr_adjustments(self_arg);
let self_ty = typeck.expr_ty(self_arg);
let self_is_copy = is_copy(cx, self_ty);

if adjustments.is_empty() && self_is_copy {
// Exact type match, already checked earlier
return Some((AdjustKind::None, self_ty));
}

let res_ty = cx.tcx.erase_regions(EarlyBinder::bind(req_res_ty)
.instantiate(cx.tcx, typeck.node_args(call_expr.hir_id)));
Expand All @@ -133,16 +107,7 @@ fn is_ref_iterable<'tcx>(
};

if !adjustments.is_empty() {
if self_is_copy {
// Using by value won't consume anything
if implements_trait(cx, self_ty, trait_id, &[])
&& let Some(ty) =
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [self_ty])
&& ty == res_ty
{
return Some((AdjustKind::None, self_ty));
}
} else if let ty::Ref(region, ty, Mutability::Mut) = *self_ty.kind()
if let ty::Ref(region, ty, Mutability::Mut) = *self_ty.kind()
&& let Some(mutbl) = mutbl
{
// Attempt to reborrow the mutable reference
Expand All @@ -160,24 +125,8 @@ fn is_ref_iterable<'tcx>(
}
}
}
if let Some(mutbl) = mutbl
&& !self_ty.is_ref()
{
// Attempt to borrow
let self_ty = Ty::new_ref(cx.tcx,cx.tcx.lifetimes.re_erased, TypeAndMut {
ty: self_ty,
mutbl,
});
if implements_trait(cx, self_ty, trait_id, &[])
&& let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [self_ty])
&& ty == res_ty
{
return Some((AdjustKind::borrow(mutbl), self_ty));
}
}

match adjustments {
[] => Some((AdjustKind::None, self_ty)),
&[
Adjustment { kind: Adjust::Deref(_), ..},
Adjustment {
Expand Down Expand Up @@ -209,24 +158,6 @@ fn is_ref_iterable<'tcx>(
None
}
}
&[
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])
&& ty == res_ty
{
Some((AdjustKind::auto_borrow(mutbl), target))
} else {
None
}
}
_ => None,
}
} else {
Expand Down
167 changes: 167 additions & 0 deletions clippy_lints/src/loops/explicit_iter_loop_std.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
use super::EXPLICIT_ITER_LOOP_STD;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::snippet_with_applicability;
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, Ty, TypeAndMut};
use rustc_span::sym;

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;
}
} 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);
span_lint_and_sugg(
cx,
EXPLICIT_ITER_LOOP_STD,
call_expr.span,
"it is more concise to loop over references to containers instead of using explicit \
iteration methods",
"to write this more concisely, try",
format!("{}{object}", adjust.display()),
applicability,
);
}

#[derive(Clone, Copy)]
enum AdjustKind {
None,
Borrow,
BorrowMut,
}
impl AdjustKind {
fn borrow(mutbl: Mutability) -> Self {
match mutbl {
Mutability::Not => Self::Borrow,
Mutability::Mut => Self::BorrowMut,
}
}

fn auto_borrow(mutbl: AutoBorrowMutability) -> Self {
match mutbl {
AutoBorrowMutability::Not => Self::Borrow,
AutoBorrowMutability::Mut { .. } => Self::BorrowMut,
}
}

fn display(self) -> &'static str {
match self {
Self::None => "",
Self::Borrow => "&",
Self::BorrowMut => "&mut ",
}
}
}

/// Checks if an `iter` or `iter_mut` call returns `IntoIterator::IntoIter`. Returns how the
/// argument needs to be adjusted.
#[expect(clippy::too_many_lines)]
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)
&& let sig = cx.tcx.liberate_late_bound_regions(fn_id, cx.tcx.fn_sig(fn_id).skip_binder())
&& let &[req_self_ty, req_res_ty] = &**sig.inputs_and_output
&& let param_env = cx.tcx.param_env(fn_id)
&& implements_trait_with_env(cx.tcx, param_env, req_self_ty, trait_id, &[])
&& let Some(into_iter_ty) =
make_normalized_projection_with_regions(cx.tcx, param_env, trait_id, sym!(IntoIter), [req_self_ty])
&& let req_res_ty = normalize_with_regions(cx.tcx, param_env, req_res_ty)
&& into_iter_ty == req_res_ty
{
let adjustments = typeck.expr_adjustments(self_arg);
let self_ty = typeck.expr_ty(self_arg);
let self_is_copy = is_copy(cx, self_ty);

if adjustments.is_empty() && self_is_copy {
// Exact type match, already checked earlier
return Some((AdjustKind::None, self_ty));
}

let res_ty = cx.tcx.erase_regions(EarlyBinder::bind(req_res_ty)
.instantiate(cx.tcx, typeck.node_args(call_expr.hir_id)));
let mutbl = if let ty::Ref(_, _, mutbl) = *req_self_ty.kind() {
Some(mutbl)
} else {
None
};

if !adjustments.is_empty() {
if self_is_copy {
// Using by value won't consume anything
if implements_trait(cx, self_ty, trait_id, &[])
&& let Some(ty) =
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [self_ty])
&& ty == res_ty
{
return Some((AdjustKind::None, self_ty));
}
} }
if let Some(mutbl) = mutbl
&& !self_ty.is_ref()
{
// Attempt to borrow
let self_ty = Ty::new_ref(cx.tcx,cx.tcx.lifetimes.re_erased, TypeAndMut {
ty: self_ty,
mutbl,
});
if implements_trait(cx, self_ty, trait_id, &[])
&& let Some(ty) = make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [self_ty])
&& ty == res_ty
{
return Some((AdjustKind::borrow(mutbl), self_ty));
}
}

match adjustments {
[] => Some((AdjustKind::None, self_ty)),
&[
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])
&& ty == res_ty
{
Some((AdjustKind::auto_borrow(mutbl), target))
} else {
None
}
}
_ => None,
}
} else {
None
}
}
43 changes: 39 additions & 4 deletions clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
mod empty_loop;
mod explicit_counter_loop;
mod explicit_into_iter_loop;
mod explicit_iter_loop;
mod explicit_iter_loop_deref;
mod explicit_iter_loop_std;
mod for_kv_map;
mod iter_next_loop;
mod manual_find;
Expand Down Expand Up @@ -116,11 +117,45 @@ declare_clippy_lint! {
/// }
/// ```
#[clippy::version = "pre 1.29.0"]
pub EXPLICIT_ITER_LOOP,
pub EXPLICIT_ITER_LOOP_STD,
pedantic,
"for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for loops on `x.iter()` where `&x` will do, and
/// suggests the latter.
///
/// ### Why is this bad?
/// Readability.
///
/// ### Known problems
/// False negatives. We currently only warn on some known
/// types.
///
/// ### Example
/// ```rust
/// // with `y` a borrowed mut vec or slice:
/// # let y = &mut vec;
/// for x in y.iter() {
/// // ..
/// }
/// ```
///
/// Use instead:
/// ```rust
/// # let y = &mut vec;
/// for x in &*y {
/// // ..
/// }
/// ```
#[clippy::version = "pre 1.29.0"]
pub EXPLICIT_ITER_LOOP_DEREF,
pedantic,
"for-looping over `_.iter()` or `_.iter_mut()` when `&*_` or `&mut *_` would do"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for loops on `y.into_iter()` where `y` will do, and
Expand Down Expand Up @@ -619,7 +654,6 @@ impl_lint_pass!(Loops => [
MANUAL_MEMCPY,
MANUAL_FLATTEN,
NEEDLESS_RANGE_LOOP,
EXPLICIT_ITER_LOOP,
EXPLICIT_INTO_ITER_LOOP,
ITER_NEXT_LOOP,
WHILE_LET_LOOP,
Expand Down Expand Up @@ -719,7 +753,8 @@ impl Loops {
if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind {
match method.ident.as_str() {
"iter" | "iter_mut" => {
explicit_iter_loop::check(cx, self_arg, arg, &self.msrv);
explicit_iter_loop_std::check(cx, self_arg, arg, &self.msrv);
explicit_iter_loop_deref::check(cx, self_arg, arg, &self.msrv);
},
"into_iter" => {
explicit_into_iter_loop::check(cx, self_arg, arg);
Expand Down

0 comments on commit 45ec139

Please sign in to comment.