From be55a96d802e919ec91a3264bb5fa5b6d211da65 Mon Sep 17 00:00:00 2001 From: Ben Schofield Date: Sun, 27 Aug 2023 21:45:14 -0600 Subject: [PATCH 1/2] Add config flag for reborrows in explicit_iter_loop This commit adds a config flag for enforcing explicit into iter lint for reborrowed values. The config flag, enforce_iter_loop_reborrow, can be added to clippy.toml files to enable the linting behaviour. By default the lint is not enabled. --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/loops/explicit_iter_loop.rs | 17 +++-- clippy_lints/src/loops/mod.rs | 10 ++- clippy_lints/src/utils/conf.rs | 20 ++++++ .../toml_unknown_key/conf_unknown_key.stderr | 2 + tests/ui/explicit_iter_loop.fixed | 7 +- tests/ui/explicit_iter_loop.rs | 1 + tests/ui/explicit_iter_loop.stderr | 64 ++++++------------- 8 files changed, 68 insertions(+), 56 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ab71bfbdc58..f3559771a57 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -695,7 +695,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(unit_types::UnitTypes)); - store.register_late_pass(move |_| Box::new(loops::Loops::new(msrv()))); + let enforce_iter_loop_reborrow = conf.enforce_iter_loop_reborrow; + store.register_late_pass(move |_| Box::new(loops::Loops::new(msrv(), enforce_iter_loop_reborrow))); store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(lifetimes::Lifetimes)); store.register_late_pass(|_| Box::new(entry::HashMapPass)); diff --git a/clippy_lints/src/loops/explicit_iter_loop.rs b/clippy_lints/src/loops/explicit_iter_loop.rs index 7b8c88235a9..6ab256ef001 100644 --- a/clippy_lints/src/loops/explicit_iter_loop.rs +++ b/clippy_lints/src/loops/explicit_iter_loop.rs @@ -13,8 +13,14 @@ 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 { +pub(super) fn check( + cx: &LateContext<'_>, + self_arg: &Expr<'_>, + call_expr: &Expr<'_>, + msrv: &Msrv, + enforce_iter_loop_reborrow: bool, +) { + let Some((adjust, ty)) = is_ref_iterable(cx, self_arg, call_expr, enforce_iter_loop_reborrow) else { return; }; if let ty::Array(_, count) = *ty.peel_refs().kind() { @@ -102,6 +108,7 @@ fn is_ref_iterable<'tcx>( cx: &LateContext<'tcx>, self_arg: &Expr<'_>, call_expr: &Expr<'_>, + enforce_iter_loop_reborrow: bool, ) -> Option<(AdjustKind, Ty<'tcx>)> { let typeck = cx.typeck_results(); if let Some(trait_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) @@ -142,7 +149,8 @@ fn is_ref_iterable<'tcx>( { return Some((AdjustKind::None, self_ty)); } - } else if let ty::Ref(region, ty, Mutability::Mut) = *self_ty.kind() + } else if enforce_iter_loop_reborrow + && let ty::Ref(region, ty, Mutability::Mut) = *self_ty.kind() && let Some(mutbl) = mutbl { // Attempt to reborrow the mutable reference @@ -186,7 +194,8 @@ fn is_ref_iterable<'tcx>( }, .. ] => { - if target != self_ty + if enforce_iter_loop_reborrow + && 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]) diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index ffd29ab7630..1fb16adad7a 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -609,10 +609,14 @@ pub struct Loops { msrv: Msrv, + enforce_iter_loop_reborrow: bool, } impl Loops { - pub fn new(msrv: Msrv) -> Self { - Self { msrv } + pub fn new(msrv: Msrv, enforce_iter_loop_reborrow: bool) -> Self { + Self { + msrv, + enforce_iter_loop_reborrow, + } } } impl_lint_pass!(Loops => [ @@ -719,7 +723,7 @@ fn check_for_loop_arg(&self, cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) 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::check(cx, self_arg, arg, &self.msrv, self.enforce_iter_loop_reborrow); }, "into_iter" => { explicit_into_iter_loop::check(cx, self_arg, arg); diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 58ae0656db7..26889475522 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -561,6 +561,26 @@ pub fn get_configuration_metadata() -> Vec { /// Which crates to allow absolute paths from (absolute_paths_allowed_crates: rustc_data_structures::fx::FxHashSet = rustc_data_structures::fx::FxHashSet::default()), + /// Lint: EXPLICIT_ITER_LOOP + /// + /// Whether to recommend using implicit into iter for reborrowed values. + /// + /// #### Example + /// ``` + /// let mut vec = vec![1, 2, 3]; + /// let rmvec = &mut vec; + /// for _ in rmvec.iter() {} + /// for _ in rmvec.iter_mut() {} + /// ``` + /// + /// Use instead: + /// ``` + /// let mut vec = vec![1, 2, 3]; + /// let rmvec = &mut vec; + /// for _ in &*rmvec {} + /// for _ in &mut *rmvec {} + /// ``` + (enforce_iter_loop_reborrow: bool = false), } /// Search for the configuration file. diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index cdabe6460cd..b97bb144468 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -28,6 +28,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect disallowed-types doc-valid-idents enable-raw-pointer-heuristic-for-send + enforce-iter-loop-reborrow enforced-import-renames enum-variant-name-threshold enum-variant-size-threshold @@ -99,6 +100,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect disallowed-types doc-valid-idents enable-raw-pointer-heuristic-for-send + enforce-iter-loop-reborrow enforced-import-renames enum-variant-name-threshold enum-variant-size-threshold diff --git a/tests/ui/explicit_iter_loop.fixed b/tests/ui/explicit_iter_loop.fixed index 57292114e38..f08397defa5 100644 --- a/tests/ui/explicit_iter_loop.fixed +++ b/tests/ui/explicit_iter_loop.fixed @@ -4,6 +4,7 @@ clippy::similar_names, clippy::needless_borrow, clippy::deref_addrof, + clippy::unnecessary_mut_passed, dead_code )] @@ -20,15 +21,15 @@ fn main() { for _ in rvec {} let rmvec = &mut vec; - for _ in &*rmvec {} - for _ in &mut *rmvec {} + for _ in rmvec.iter() {} + for _ in rmvec.iter_mut() {} for _ in &vec {} // these are fine for _ in &mut vec {} // these are fine for _ in &[1, 2, 3] {} - for _ in &*(&mut [1, 2, 3]) {} + for _ in (&mut [1, 2, 3]).iter() {} for _ in &[0; 32] {} for _ in &[0; 33] {} diff --git a/tests/ui/explicit_iter_loop.rs b/tests/ui/explicit_iter_loop.rs index 66280c23843..2ee6825d445 100644 --- a/tests/ui/explicit_iter_loop.rs +++ b/tests/ui/explicit_iter_loop.rs @@ -4,6 +4,7 @@ clippy::similar_names, clippy::needless_borrow, clippy::deref_addrof, + clippy::unnecessary_mut_passed, dead_code )] diff --git a/tests/ui/explicit_iter_loop.stderr b/tests/ui/explicit_iter_loop.stderr index c311096117f..725d9b63cf8 100644 --- a/tests/ui/explicit_iter_loop.stderr +++ b/tests/ui/explicit_iter_loop.stderr @@ -1,5 +1,5 @@ error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:16:14 + --> $DIR/explicit_iter_loop.rs:17:14 | LL | for _ in vec.iter() {} | ^^^^^^^^^^ help: to write this more concisely, try: `&vec` @@ -11,132 +11,106 @@ LL | #![deny(clippy::explicit_iter_loop)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:17:14 + --> $DIR/explicit_iter_loop.rs:18:14 | LL | for _ in vec.iter_mut() {} | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut vec` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:20:14 + --> $DIR/explicit_iter_loop.rs:21:14 | LL | for _ in rvec.iter() {} | ^^^^^^^^^^^ help: to write this more concisely, try: `rvec` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:23:14 - | -LL | for _ in rmvec.iter() {} - | ^^^^^^^^^^^^ help: to write this more concisely, try: `&*rmvec` - -error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:24:14 - | -LL | for _ in rmvec.iter_mut() {} - | ^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut *rmvec` - -error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:29:14 + --> $DIR/explicit_iter_loop.rs:30:14 | LL | for _ in [1, 2, 3].iter() {} | ^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&[1, 2, 3]` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:31:14 - | -LL | for _ in (&mut [1, 2, 3]).iter() {} - | ^^^^^^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&*(&mut [1, 2, 3])` - -error: the method `iter` doesn't need a mutable reference - --> $DIR/explicit_iter_loop.rs:31:14 - | -LL | for _ in (&mut [1, 2, 3]).iter() {} - | ^^^^^^^^^^^^^^^^ - | - = note: `-D clippy::unnecessary-mut-passed` implied by `-D warnings` - -error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:33:14 + --> $DIR/explicit_iter_loop.rs:34:14 | LL | for _ in [0; 32].iter() {} | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&[0; 32]` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:34:14 + --> $DIR/explicit_iter_loop.rs:35:14 | LL | for _ in [0; 33].iter() {} | ^^^^^^^^^^^^^^ help: to write this more concisely, try: `&[0; 33]` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:37:14 + --> $DIR/explicit_iter_loop.rs:38:14 | LL | for _ in ll.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&ll` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:39:14 + --> $DIR/explicit_iter_loop.rs:40:14 | LL | for _ in rll.iter() {} | ^^^^^^^^^^ help: to write this more concisely, try: `rll` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:42:14 + --> $DIR/explicit_iter_loop.rs:43:14 | LL | for _ in vd.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&vd` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:44:14 + --> $DIR/explicit_iter_loop.rs:45:14 | LL | for _ in rvd.iter() {} | ^^^^^^^^^^ help: to write this more concisely, try: `rvd` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:47:14 + --> $DIR/explicit_iter_loop.rs:48:14 | LL | for _ in bh.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bh` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:50:14 + --> $DIR/explicit_iter_loop.rs:51:14 | LL | for _ in hm.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&hm` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:53:14 + --> $DIR/explicit_iter_loop.rs:54:14 | LL | for _ in bt.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bt` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:56:14 + --> $DIR/explicit_iter_loop.rs:57:14 | LL | for _ in hs.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&hs` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:59:14 + --> $DIR/explicit_iter_loop.rs:60:14 | LL | for _ in bs.iter() {} | ^^^^^^^^^ help: to write this more concisely, try: `&bs` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:148:14 + --> $DIR/explicit_iter_loop.rs:149:14 | LL | for _ in x.iter() {} | ^^^^^^^^ help: to write this more concisely, try: `&x` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:149:14 + --> $DIR/explicit_iter_loop.rs:150:14 | LL | for _ in x.iter_mut() {} | ^^^^^^^^^^^^ help: to write this more concisely, try: `&mut x` error: it is more concise to loop over references to containers instead of using explicit iteration methods - --> $DIR/explicit_iter_loop.rs:152:14 + --> $DIR/explicit_iter_loop.rs:153:14 | LL | for _ in r.iter() {} | ^^^^^^^^ help: to write this more concisely, try: `r` -error: aborting due to 22 previous errors +error: aborting due to 18 previous errors From 55bd0fe296d897ab45e16522f21a0eb847b9e741 Mon Sep 17 00:00:00 2001 From: Ben Schofield Date: Wed, 30 Aug 2023 16:02:37 -0700 Subject: [PATCH 2/2] Fix metadata collection --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92cc19edb05..939de7d4e1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5570,4 +5570,5 @@ Released 2018-09-13 [`allow-one-hash-in-raw-strings`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-one-hash-in-raw-strings [`absolute-paths-max-segments`]: https://doc.rust-lang.org/clippy/lint_configuration.html#absolute-paths-max-segments [`absolute-paths-allowed-crates`]: https://doc.rust-lang.org/clippy/lint_configuration.html#absolute-paths-allowed-crates +[`enforce-iter-loop-reborrow`]: https://doc.rust-lang.org/clippy/lint_configuration.html#enforce-iter-loop-reborrow diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index caaad6d1173..52c795e04fe 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -751,3 +751,27 @@ Which crates to allow absolute paths from * [`absolute_paths`](https://rust-lang.github.io/rust-clippy/master/index.html#absolute_paths) +## `enforce-iter-loop-reborrow` +#### Example +``` +let mut vec = vec![1, 2, 3]; +let rmvec = &mut vec; +for _ in rmvec.iter() {} +for _ in rmvec.iter_mut() {} +``` + +Use instead: +``` +let mut vec = vec![1, 2, 3]; +let rmvec = &mut vec; +for _ in &*rmvec {} +for _ in &mut *rmvec {} +``` + +**Default Value:** `false` (`bool`) + +--- +**Affected lints:** +* [`explicit_iter_loop`](https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop) + +