From 330ebbb9f9ee3c4bab37e330726ccc3ac1c26e4a Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 18 Sep 2023 00:19:34 +0200 Subject: [PATCH] new lint: `iter_without_into_iter` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/iter_without_into_iter.rs | 135 +++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/iter_without_into_iter.rs | 120 +++++++++++++++++ tests/ui/iter_without_into_iter.stderr | 150 +++++++++++++++++++++ 6 files changed, 409 insertions(+) create mode 100644 clippy_lints/src/iter_without_into_iter.rs create mode 100644 tests/ui/iter_without_into_iter.rs create mode 100644 tests/ui/iter_without_into_iter.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index db54bfbf0b3..40510d78800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5036,6 +5036,7 @@ Released 2018-09-13 [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next [`iter_skip_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_zero [`iter_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain +[`iter_without_into_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_without_into_iter [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits [`large_const_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index b4b84b36044..06809038927 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -229,6 +229,7 @@ crate::items_after_statements::ITEMS_AFTER_STATEMENTS_INFO, crate::items_after_test_module::ITEMS_AFTER_TEST_MODULE_INFO, crate::iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR_INFO, + crate::iter_without_into_iter::ITER_WITHOUT_INTO_ITER_INFO, crate::large_const_arrays::LARGE_CONST_ARRAYS_INFO, crate::large_enum_variant::LARGE_ENUM_VARIANT_INFO, crate::large_futures::LARGE_FUTURES_INFO, diff --git a/clippy_lints/src/iter_without_into_iter.rs b/clippy_lints/src/iter_without_into_iter.rs new file mode 100644 index 00000000000..08a8e6dbf18 --- /dev/null +++ b/clippy_lints/src/iter_without_into_iter.rs @@ -0,0 +1,135 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::get_parent_as_impl; +use clippy_utils::source::snippet; +use clippy_utils::ty::{implements_trait, make_normalized_projection}; +use rustc_errors::Applicability; +use rustc_hir::{FnRetTy, ImplItemKind, ImplicitSelfKind, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Looks for `iter` and `iter_mut` methods without an associated `IntoIterator for (&|&mut) Type` implementation. + /// + /// ### Why is this bad? + /// It's not bad, but having them is idiomatic and allows the type to be used in for loops directly + /// (`for val in &iter {}`), without having to first call `iter()` or `iter_mut()`. + /// + /// ### Example + /// ```rust + /// struct MySlice<'a>(&'a [u8]); + /// impl<'a> MySlice<'a> { + /// pub fn iter(&self) -> std::slice::Iter<'a, u8> { + /// self.0.iter() + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// struct MySlice<'a>(&'a [u8]); + /// impl<'a> MySlice<'a> { + /// pub fn iter(&self) -> std::slice::Iter<'a, u8> { + /// self.0.iter() + /// } + /// } + /// impl<'a> IntoIterator for &MySlice<'a> { + /// type Item = &'a u8; + /// type IntoIter = std::slice::Iter<'a, u8>; + /// fn into_iter(self) -> Self::IntoIter { + /// self.iter() + /// } + /// } + /// ``` + #[clippy::version = "1.74.0"] + pub ITER_WITHOUT_INTO_ITER, + pedantic, + "implementing `iter(_mut)` without an associated `IntoIterator for (&|&mut) Type` impl" +} +declare_lint_pass!(IterWithoutIntoIter => [ITER_WITHOUT_INTO_ITER]); + +/// Checks if a given type is nameable in a trait (impl). +/// RPIT is stable, but impl Trait in traits is not (yet), so when we have +/// a function such as `fn iter(&self) -> impl IntoIterator`, we can't +/// suggest `type IntoIter = impl IntoIterator`. +fn is_nameable_in_impl_trait(ty: &rustc_hir::Ty<'_>) -> bool { + !matches!(ty.kind, TyKind::OpaqueDef(..)) +} + +impl LateLintPass<'_> for IterWithoutIntoIter { + fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &rustc_hir::ImplItem<'_>) { + let item_did = item.owner_id.to_def_id(); + let (borrow_prefix, expected_implicit_self) = match item.ident.name { + sym::iter => ("&", ImplicitSelfKind::ImmRef), + sym::iter_mut => ("&mut ", ImplicitSelfKind::MutRef), + _ => return, + }; + + if let ImplItemKind::Fn(sig, _) = item.kind + && let FnRetTy::Return(ret) = sig.decl.output + && is_nameable_in_impl_trait(ret) + && cx.tcx.generics_of(item_did).params.is_empty() + && sig.decl.implicit_self == expected_implicit_self + && sig.decl.inputs.len() == 1 + && let Some(imp) = get_parent_as_impl(cx.tcx, item.hir_id()) + && imp.of_trait.is_none() + && let sig = cx.tcx.liberate_late_bound_regions( + item_did, + cx.tcx.fn_sig(item_did).instantiate_identity() + ) + && let ref_ty = sig.inputs()[0] + && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) + && let Some(iterator_did) = cx.tcx.get_diagnostic_item(sym::Iterator) + && let ret_ty = sig.output() + // Order is important here, we need to check that the `fn iter` return type actually implements `IntoIterator` + // *before* normalizing `<_ as IntoIterator>::Item` (otherwise make_normalized_projection ICEs) + && implements_trait(cx, ret_ty, iterator_did, &[]) + && let Some(iter_ty) = make_normalized_projection( + cx.tcx, + cx.param_env, + iterator_did, + sym!(Item), + [ret_ty], + ) + // Only lint if the `IntoIterator` impl doesn't actually exist + && !implements_trait(cx, ref_ty, into_iter_did, &[]) + { + let self_ty_snippet = format!("{borrow_prefix}{}", snippet(cx, imp.self_ty.span, "..")); + + span_lint_and_then( + cx, + ITER_WITHOUT_INTO_ITER, + item.span, + &format!("`{}` method without an `IntoIterator` impl for `{self_ty_snippet}`", item.ident), + |diag| { + // Get the lower span of the `impl` block, and insert the suggestion right before it: + // impl X { + // ^ fn iter(&self) -> impl IntoIterator { ... } + // } + let span_behind_impl = cx.tcx + .def_span(cx.tcx.hir().parent_id(item.hir_id()).owner.def_id) + .shrink_to_lo(); + + let sugg = format!( +" +impl IntoIterator for {self_ty_snippet} {{ + type IntoIter = {ret_ty}; + type Iter = {iter_ty}; + fn into_iter() -> Self::IntoIter {{ + self.iter() + }} +}} +" + ); + diag.span_suggestion_verbose( + span_behind_impl, + format!("consider implementing `IntoIterator` for `{self_ty_snippet}`"), + sugg, + // Suggestion is on a best effort basis, might need some adjustments by the user + // such as adding some lifetimes in the associated types, or importing types. + Applicability::Unspecified, + ); + }); + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 63a3fbcb897..0f35ec27665 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -169,6 +169,7 @@ mod items_after_statements; mod items_after_test_module; mod iter_not_returning_iterator; +mod iter_without_into_iter; mod large_const_arrays; mod large_enum_variant; mod large_futures; @@ -1121,6 +1122,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv()))); + store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/iter_without_into_iter.rs b/tests/ui/iter_without_into_iter.rs new file mode 100644 index 00000000000..cedb756c79d --- /dev/null +++ b/tests/ui/iter_without_into_iter.rs @@ -0,0 +1,120 @@ +//@no-rustfix +#![warn(clippy::iter_without_into_iter)] + +fn main() { + { + struct S; + impl S { + pub fn iter(&self) -> std::slice::Iter<'_, u8> { + //~^ ERROR: `iter` method without an `IntoIterator` impl + [].iter() + } + pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, u8> { + //~^ ERROR: `iter_mut` method without an `IntoIterator` impl + [].iter_mut() + } + } + } + { + struct S; + impl S { + pub fn iter(&self) -> impl Iterator { + // RPITIT is not stable, so we can't generally suggest it here yet + [].iter() + } + } + } + { + struct S<'a>(&'a mut [u8]); + impl<'a> S<'a> { + pub fn iter(&self) -> std::slice::Iter<'_, u8> { + //~^ ERROR: `iter` method without an `IntoIterator` impl + self.0.iter() + } + pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, u8> { + //~^ ERROR: `iter_mut` method without an `IntoIterator` impl + self.0.iter_mut() + } + } + } + { + // Incompatible signatures + struct S; + impl S { + pub fn iter(self) -> std::slice::Iter<'static, u8> { + todo!() + } + } + struct S2; + impl S2 { + pub async fn iter(&self) -> std::slice::Iter<'static, u8> { + todo!() + } + } + struct S3; + impl S3 { + pub fn iter(&self, _additional_param: ()) -> std::slice::Iter<'static, u8> { + todo!() + } + } + struct S4(T); + impl S4 { + pub fn iter(&self) -> std::slice::Iter<'static, (T, U)> { + todo!() + } + } + struct S5(T); + impl S5 { + pub fn iter(&self) -> std::slice::Iter<'static, T> { + todo!() + } + } + } + { + struct S(T); + impl S { + pub fn iter(&self) -> std::slice::Iter<'_, T> { + //~^ ERROR: `iter` method without an `IntoIterator` impl + todo!() + } + pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, T> { + //~^ ERROR: `iter_mut` method without an `IntoIterator` impl + todo!() + } + } + } + { + struct S(T); + impl S { + pub fn iter(&self) -> std::slice::Iter<'_, T> { + // Don't lint, there's an existing (wrong) IntoIterator impl + todo!() + } + } + + impl<'a, T> IntoIterator for &'a S { + type Item = &'a String; + type IntoIter = std::slice::Iter<'a, String>; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } + { + struct S(T); + impl S { + pub fn iter_mut(&self) -> std::slice::IterMut<'_, T> { + // Don't lint, there's an existing (wrong) IntoIterator impl + todo!() + } + } + + impl<'a, T> IntoIterator for &'a mut S { + type Item = &'a mut String; + type IntoIter = std::slice::IterMut<'a, String>; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } +} diff --git a/tests/ui/iter_without_into_iter.stderr b/tests/ui/iter_without_into_iter.stderr new file mode 100644 index 00000000000..9d0b99415a5 --- /dev/null +++ b/tests/ui/iter_without_into_iter.stderr @@ -0,0 +1,150 @@ +error: `iter` method without an `IntoIterator` impl for `&S` + --> $DIR/iter_without_into_iter.rs:8:13 + | +LL | / pub fn iter(&self) -> std::slice::Iter<'_, u8> { +LL | | +LL | | [].iter() +LL | | } + | |_____________^ + | + = note: `-D clippy::iter-without-into-iter` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::iter_without_into_iter)]` +help: consider implementing `IntoIterator` for `&S` + | +LL ~ +LL + impl IntoIterator for &S { +LL + type IntoIter = std::slice::Iter<'_, u8>; +LL + type Iter = &u8; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter_mut` method without an `IntoIterator` impl for `&mut S` + --> $DIR/iter_without_into_iter.rs:12:13 + | +LL | / pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, u8> { +LL | | +LL | | [].iter_mut() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&mut S` + | +LL ~ +LL + impl IntoIterator for &mut S { +LL + type IntoIter = std::slice::IterMut<'_, u8>; +LL + type Iter = &mut u8; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter` method without an `IntoIterator` impl for `&S<'a>` + --> $DIR/iter_without_into_iter.rs:30:13 + | +LL | / pub fn iter(&self) -> std::slice::Iter<'_, u8> { +LL | | +LL | | self.0.iter() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&S<'a>` + | +LL ~ +LL + impl IntoIterator for &S<'a> { +LL + type IntoIter = std::slice::Iter<'_, u8>; +LL + type Iter = &u8; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter_mut` method without an `IntoIterator` impl for `&mut S<'a>` + --> $DIR/iter_without_into_iter.rs:34:13 + | +LL | / pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, u8> { +LL | | +LL | | self.0.iter_mut() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&mut S<'a>` + | +LL ~ +LL + impl IntoIterator for &mut S<'a> { +LL + type IntoIter = std::slice::IterMut<'_, u8>; +LL + type Iter = &mut u8; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter` method without an `IntoIterator` impl for `&S5` + --> $DIR/iter_without_into_iter.rs:68:13 + | +LL | / pub fn iter(&self) -> std::slice::Iter<'static, T> { +LL | | todo!() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&S5` + | +LL ~ +LL + impl IntoIterator for &S5 { +LL + type IntoIter = std::slice::Iter<'static, T>; +LL + type Iter = &T; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter` method without an `IntoIterator` impl for `&S` + --> $DIR/iter_without_into_iter.rs:76:13 + | +LL | / pub fn iter(&self) -> std::slice::Iter<'_, T> { +LL | | +LL | | todo!() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&S` + | +LL ~ +LL + impl IntoIterator for &S { +LL + type IntoIter = std::slice::Iter<'_, T>; +LL + type Iter = &T; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: `iter_mut` method without an `IntoIterator` impl for `&mut S` + --> $DIR/iter_without_into_iter.rs:80:13 + | +LL | / pub fn iter_mut(&mut self) -> std::slice::IterMut<'_, T> { +LL | | +LL | | todo!() +LL | | } + | |_____________^ + | +help: consider implementing `IntoIterator` for `&mut S` + | +LL ~ +LL + impl IntoIterator for &mut S { +LL + type IntoIter = std::slice::IterMut<'_, T>; +LL + type Iter = &mut T; +LL + fn into_iter() -> Self::IntoIter { +LL + self.iter() +LL + } +LL + } + | + +error: aborting due to 7 previous errors +