Skip to content

Commit

Permalink
Reborrow mutable references in explicit_iter_loop
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarcho committed Feb 27, 2023
1 parent edd8360 commit cd21cf7
Show file tree
Hide file tree
Showing 23 changed files with 154 additions and 54 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/default_numeric_fallback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ impl<'a, 'tcx> Visitor<'tcx> for NumericFallbackVisitor<'a, 'tcx> {
let fields_def = &variant.fields;

// Push field type then visit each field expr.
for field in fields.iter() {
for field in *fields {
let bound =
fields_def
.iter()
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ fn has_where_lifetimes<'tcx>(cx: &LateContext<'tcx>, generics: &'tcx Generics<'_
// if the bounds define new lifetimes, they are fine to occur
let allowed_lts = allowed_lts_from(pred.bound_generic_params);
// now walk the bounds
for bound in pred.bounds.iter() {
for bound in pred.bounds {
walk_param_bound(&mut visitor, bound);
}
// and check that all lifetimes are allowed
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/loops/explicit_into_iter_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,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",
format!("{}{}", adjust.display(), object.to_string()),
format!("{}{object}", adjust.display()),
applicability,
);
}
74 changes: 54 additions & 20 deletions clippy_lints/src/loops/explicit_iter_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,19 @@ pub(super) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>, call_expr: &Expr<

let mut applicability = Applicability::MachineApplicable;
let object = snippet_with_applicability(cx, self_arg.span, "_", &mut applicability);
let prefix = match adjust {
AdjustKind::None => "",
AdjustKind::Borrow => "&",
AdjustKind::BorrowMut => "&mut ",
AdjustKind::Deref => "*",
AdjustKind::Reborrow => "&*",
AdjustKind::ReborrowMut => "&mut *",
};
span_lint_and_sugg(
cx,
EXPLICIT_ITER_LOOP,
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!("{prefix}{object}"),
format!("{}{object}", adjust.display()),
applicability,
);
}

#[derive(Clone, Copy)]
enum AdjustKind {
None,
Borrow,
Expand All @@ -76,16 +69,35 @@ impl AdjustKind {
}
}

fn reborrow(mutbl: AutoBorrowMutability) -> Self {
fn reborrow(mutbl: Mutability) -> Self {
match mutbl {
Mutability::Not => Self::Reborrow,
Mutability::Mut => Self::ReborrowMut,
}
}

fn auto_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::Deref => "*",
Self::Reborrow => "&*",
Self::ReborrowMut => "&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<'_>,
Expand All @@ -108,27 +120,49 @@ fn is_ref_iterable<'tcx>(
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(req_res_ty).subst(cx.tcx, typeck.node_substs(call_expr.hir_id)));
if !adjustments.is_empty() && self_is_copy {
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));
}
}

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));
}
} else if let ty::Ref(region, ty, Mutability::Mut) = *self_ty.kind()
&& let Some(mutbl) = mutbl
{
// Attempt to reborrow the mutable reference
let self_ty = if mutbl.is_mut() {
self_ty
} else {
cx.tcx.mk_ref(region, TypeAndMut { 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::reborrow(mutbl), self_ty));
}
}
}
if let Some(mutbl) = mutbl
&& !self_ty.is_ref()
{
// Attempt to borrow
let self_ty = cx.tcx.mk_ref(cx.tcx.lifetimes.re_erased, TypeAndMut {
ty: self_ty,
mutbl,
Expand Down Expand Up @@ -157,7 +191,7 @@ fn is_ref_iterable<'tcx>(
make_normalized_projection(cx.tcx, cx.param_env, trait_id, sym!(IntoIter), [target])
&& ty == res_ty
{
Some((AdjustKind::reborrow(mutbl), target))
Some((AdjustKind::auto_reborrow(mutbl), target))
} else {
None
}
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,8 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
missing_spin_loop::check(cx, condition, body);
}
}

extract_msrv_attr!(LateContext);
}

impl Loops {
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/loops/same_item_push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
}

fn visit_block(&mut self, b: &'tcx Block<'_>) {
for stmt in b.stmts.iter() {
for stmt in b.stmts {
self.visit_stmt(stmt);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/macro_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl<'tcx> LateLintPass<'tcx> for MacroUseImports {
});
if !id.is_local();
then {
for kid in cx.tcx.module_children(id).iter() {
for kid in cx.tcx.module_children(id) {
if let Res::Def(DefKind::Macro(_mac_type), mac_id) = kid.res {
let span = mac_attr.span;
let def_path = cx.tcx.def_path_str(mac_id);
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/matches/match_wild_err_arm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm<'
let mut ident_bind_name = kw::Underscore;
if !matching_wild {
// Looking for unused bindings (i.e.: `_e`)
for pat in inner.iter() {
for pat in inner {
if let PatKind::Binding(_, id, ident, None) = pat.kind {
if ident.as_str().starts_with('_') && !is_local_used(cx, arm.body, id) {
ident_bind_name = ident.name;
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/matches/significant_drop_in_scrutinee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ impl<'a, 'tcx> SigDropChecker<'a, 'tcx> {
}
}

for generic_arg in b.iter() {
for generic_arg in *b {
if let GenericArgKind::Type(ty) = generic_arg.unpack() {
if self.has_sig_drop_attr(cx, ty) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/matches/try_err.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, scrutine
/// Finds function return type by examining return expressions in match arms.
fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option<Ty<'tcx>> {
if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr {
for arm in arms.iter() {
for arm in *arms {
if let ExprKind::Ret(Some(ret)) = arm.body.kind {
return Some(cx.typeck_results().expr_ty(ret));
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/misc_early/mixed_case_hex_literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub(super) fn check(cx: &EarlyContext<'_>, lit_span: Span, suffix: &str, lit_sni
return;
}
let mut seen = (false, false);
for ch in lit_snip.as_bytes()[2..=maybe_last_sep_idx].iter() {
for ch in &lit_snip.as_bytes()[2..=maybe_last_sep_idx] {
match ch {
b'a'..=b'f' => seen.0 = true,
b'A'..=b'F' => seen.1 = true,
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/mismatching_type_param_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for TypeParamMismatch {
then {
// get the name and span of the generic parameters in the Impl
let mut impl_params = Vec::new();
for p in generic_args.args.iter() {
for p in generic_args.args {
match p {
GenericArg::Type(Ty {kind: TyKind::Path(QPath::Resolved(_, path)), ..}) =>
impl_params.push((path.segments[0].ident.to_string(), path.span)),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/returns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ fn check_final_expr<'tcx>(
// note, if without else is going to be a type checking error anyways
// (except for unit type functions) so we don't match it
ExprKind::Match(_, arms, MatchSource::Normal) => {
for arm in arms.iter() {
for arm in *arms {
check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit);
}
},
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/significant_drop_tightening.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ impl<'cx, 'sdt, 'tcx> SigDropChecker<'cx, 'sdt, 'tcx> {
return true;
}
}
for generic_arg in b.iter() {
for generic_arg in *b {
if let GenericArgKind::Type(ty) = generic_arg.unpack() {
if self.has_sig_drop_attr(ty) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/trait_bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl<'tcx> LateLintPass<'tcx> for TraitBounds {
) = cx.tcx.hir().get_if_local(*def_id);
then {
if self_bounds_map.is_empty() {
for bound in self_bounds.iter() {
for bound in *self_bounds {
let Some((self_res, self_segments, _)) = get_trait_info_from_bound(bound) else { continue };
self_bounds_map.insert(self_res, self_segments);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol {

for &module in &[&paths::KW_MODULE, &paths::SYM_MODULE] {
for def_id in def_path_def_ids(cx, module) {
for item in cx.tcx.module_children(def_id).iter() {
for item in cx.tcx.module_children(def_id) {
if_chain! {
if let Res::Def(DefKind::Const, item_def_id) = item.res;
let ty = cx.tcx.type_of(item_def_id).subst_identity();
Expand Down
4 changes: 2 additions & 2 deletions clippy_utils/src/qualify_min_const_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub fn is_min_const_fn<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, msrv: &Msrv)
body.local_decls.iter().next().unwrap().source_info.span,
)?;

for bb in body.basic_blocks.iter() {
for bb in &*body.basic_blocks {
check_terminator(tcx, body, bb.terminator(), msrv)?;
for stmt in &bb.statements {
check_statement(tcx, body, def_id, stmt)?;
Expand Down Expand Up @@ -89,7 +89,7 @@ fn check_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, span: Span) -> McfResult {
return Err((span, "function pointers in const fn are unstable".into()));
},
ty::Dynamic(preds, _, _) => {
for pred in preds.iter() {
for pred in *preds {
match pred.skip_binder() {
ty::ExistentialPredicate::AutoTrait(_) | ty::ExistentialPredicate::Projection(_) => {
return Err((
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ pub fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
false
},
ty::Dynamic(binder, _, _) => {
for predicate in binder.iter() {
for predicate in *binder {
if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate.skip_binder() {
if cx.tcx.has_attr(trait_ref.def_id, sym::must_use) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/usage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ pub struct ParamBindingIdCollector {
impl<'tcx> ParamBindingIdCollector {
fn collect_binding_hir_ids(body: &'tcx hir::Body<'tcx>) -> Vec<hir::HirId> {
let mut hir_ids: Vec<hir::HirId> = Vec::new();
for param in body.params.iter() {
for param in body.params {
let mut finder = ParamBindingIdCollector {
binding_hir_ids: Vec::new(),
};
Expand Down
2 changes: 1 addition & 1 deletion lintcheck/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ fn read_crates(toml_path: &Path) -> (Vec<CrateSource>, RecursiveOptions) {
});
} else if let Some(ref versions) = tk.versions {
// if we have multiple versions, save each one
for ver in versions.iter() {
for ver in versions {
crate_sources.push(CrateSource::CratesIo {
name: tk.name.clone(),
version: ver.to_string(),
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/explicit_iter_loop.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ fn main() {
for _ in &vec {}
for _ in &mut vec {}

let rvec = &vec;
for _ in rvec {}

let rmvec = &mut vec;
for _ in &*rmvec {}
for _ in &mut *rmvec {}

for _ in &vec {} // these are fine
for _ in &mut vec {} // these are fine

Expand All @@ -29,9 +36,13 @@ fn main() {

let ll: LinkedList<()> = LinkedList::new();
for _ in &ll {}
let rll = &ll;
for _ in rll {}

let vd: VecDeque<()> = VecDeque::new();
for _ in &vd {}
let rvd = &vd;
for _ in rvd {}

let bh: BinaryHeap<()> = BinaryHeap::new();
for _ in &bh {}
Expand Down Expand Up @@ -137,4 +148,7 @@ fn main() {
let mut x = CustomType;
for _ in &x {}
for _ in &mut x {}

let r = &x;
for _ in r {}
}
14 changes: 14 additions & 0 deletions tests/ui/explicit_iter_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ fn main() {
for _ in vec.iter() {}
for _ in vec.iter_mut() {}

let rvec = &vec;
for _ in rvec.iter() {}

let rmvec = &mut vec;
for _ in rmvec.iter() {}
for _ in rmvec.iter_mut() {}

for _ in &vec {} // these are fine
for _ in &mut vec {} // these are fine

Expand All @@ -29,9 +36,13 @@ fn main() {

let ll: LinkedList<()> = LinkedList::new();
for _ in ll.iter() {}
let rll = &ll;
for _ in rll.iter() {}

let vd: VecDeque<()> = VecDeque::new();
for _ in vd.iter() {}
let rvd = &vd;
for _ in rvd.iter() {}

let bh: BinaryHeap<()> = BinaryHeap::new();
for _ in bh.iter() {}
Expand Down Expand Up @@ -137,4 +148,7 @@ fn main() {
let mut x = CustomType;
for _ in x.iter() {}
for _ in x.iter_mut() {}

let r = &x;
for _ in r.iter() {}
}
Loading

0 comments on commit cd21cf7

Please sign in to comment.