diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/iter_without_into_iter.rs | 120 | ||||
| -rw-r--r-- | tests/ui/into_iter_without_iter.rs | 124 | ||||
| -rw-r--r-- | tests/ui/into_iter_without_iter.stderr | 114 |
5 files changed, 357 insertions, 3 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 40510d78800..25230e46e88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5011,6 +5011,7 @@ Released 2018-09-13 [`integer_division`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_division [`into_iter_on_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array [`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref +[`into_iter_without_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_without_iter [`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering [`invalid_null_ptr_usage`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_null_ptr_usage [`invalid_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_ref diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 06809038927..481c44031cf 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -229,6 +229,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ 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::INTO_ITER_WITHOUT_ITER_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, diff --git a/clippy_lints/src/iter_without_into_iter.rs b/clippy_lints/src/iter_without_into_iter.rs index 08a8e6dbf18..43e1b92c9b9 100644 --- a/clippy_lints/src/iter_without_into_iter.rs +++ b/clippy_lints/src/iter_without_into_iter.rs @@ -2,11 +2,13 @@ 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_ast::Mutability; use rustc_errors::Applicability; -use rustc_hir::{FnRetTy, ImplItemKind, ImplicitSelfKind, TyKind}; +use rustc_hir::{FnRetTy, ImplItemKind, ImplicitSelfKind, ItemKind, TyKind}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::sym; +use rustc_span::{sym, Symbol}; declare_clippy_lint! { /// ### What it does @@ -46,7 +48,51 @@ declare_clippy_lint! { pedantic, "implementing `iter(_mut)` without an associated `IntoIterator for (&|&mut) Type` impl" } -declare_lint_pass!(IterWithoutIntoIter => [ITER_WITHOUT_INTO_ITER]); + +declare_clippy_lint! { + /// ### What it does + /// This is the opposite of the `iter_without_into_iter` lint. + /// It looks for `IntoIterator for (&|&mut) Type` implementations without an inherent `iter` or `iter_mut` method. + /// + /// ### Why is this bad? + /// It's not bad, but having them is idiomatic and allows the type to be used in iterator chains + /// by just calling `.iter()`, instead of the more awkward `<&Type>::into_iter` or `(&val).iter()` syntax + /// in case of ambiguity with another `Intoiterator` impl. + /// + /// ### Example + /// ```rust + /// struct MySlice<'a>(&'a [u8]); + /// impl<'a> IntoIterator for &MySlice<'a> { + /// type Item = &'a u8; + /// type IntoIter = std::slice::Iter<'a, u8>; + /// fn into_iter(self) -> Self::IntoIter { + /// 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.into_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.0.iter() + /// } + /// } + /// ``` + #[clippy::version = "1.74.0"] + pub INTO_ITER_WITHOUT_ITER, + pedantic, + "implementing `IntoIterator for (&|&mut) Type` without an inherent `iter(_mut)` method" +} + +declare_lint_pass!(IterWithoutIntoIter => [ITER_WITHOUT_INTO_ITER, INTO_ITER_WITHOUT_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 @@ -56,7 +102,75 @@ fn is_nameable_in_impl_trait(ty: &rustc_hir::Ty<'_>) -> bool { !matches!(ty.kind, TyKind::OpaqueDef(..)) } +fn type_has_inherent_method(cx: &LateContext<'_>, ty: Ty<'_>, method_name: Symbol) -> bool { + if let Some(ty_did) = ty.ty_adt_def().map(ty::AdtDef::did) { + cx.tcx.inherent_impls(ty_did).iter().any(|&did| { + cx.tcx + .associated_items(did) + .filter_by_name_unhygienic(method_name) + .next() + .is_some_and(|item| item.kind == ty::AssocKind::Fn) + }) + } else { + false + } +} + impl LateLintPass<'_> for IterWithoutIntoIter { + fn check_item(&mut self, cx: &LateContext<'_>, item: &rustc_hir::Item<'_>) { + if let ItemKind::Impl(imp) = item.kind + && let TyKind::Ref(_, self_ty_without_ref) = &imp.self_ty.kind + && let Some(trait_ref) = imp.of_trait + && trait_ref.trait_def_id().is_some_and(|did| cx.tcx.is_diagnostic_item(sym::IntoIterator, did)) + && let &ty::Ref(_, ty, mtbl) = cx.tcx.type_of(item.owner_id).instantiate_identity().kind() + && let expected_method_name = match mtbl { + Mutability::Mut => sym::iter_mut, + Mutability::Not => sym::iter, + } + && !type_has_inherent_method(cx, ty, expected_method_name) + && let Some(iter_assoc_span) = imp.items.iter().find_map(|item| { + if item.ident.name == sym!(IntoIter) { + Some(cx.tcx.hir().impl_item(item.id).expect_type().span) + } else { + None + } + }) + { + span_lint_and_then( + cx, + INTO_ITER_WITHOUT_ITER, + item.span, + &format!("`IntoIterator` implemented for a reference type without an `{expected_method_name}` method"), + |diag| { + // The suggestion forwards to the `IntoIterator` impl and uses a form of UFCS + // to avoid name ambiguities, as there might be an inherent into_iter method + // that we don't want to call. + let sugg = format!( +" +impl {self_ty_without_ref} {{ + fn {expected_method_name}({ref_self}self) -> {iter_ty} {{ + <{ref_self}Self as IntoIterator>::into_iter(self) + }} +}} +", + self_ty_without_ref = snippet(cx, self_ty_without_ref.ty.span, ".."), + ref_self = mtbl.ref_prefix_str(), + iter_ty = snippet(cx, iter_assoc_span, ".."), + ); + + diag.span_suggestion_verbose( + item.span.shrink_to_lo(), + format!("consider implementing `{expected_method_name}`"), + sugg, + // Just like iter_without_into_iter, this suggestion is on a best effort basis + // and requires potentially adding lifetimes or moving them around. + Applicability::Unspecified + ); + } + ); + } + } + 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 { diff --git a/tests/ui/into_iter_without_iter.rs b/tests/ui/into_iter_without_iter.rs new file mode 100644 index 00000000000..6be3bb8abdd --- /dev/null +++ b/tests/ui/into_iter_without_iter.rs @@ -0,0 +1,124 @@ +//@no-rustfix +#![warn(clippy::into_iter_without_iter)] + +use std::iter::IntoIterator; + +fn main() { + { + struct S; + + impl<'a> IntoIterator for &'a S { + //~^ ERROR: `IntoIterator` implemented for a reference type without an `iter` method + type IntoIter = std::slice::Iter<'a, u8>; + type Item = &'a u8; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + impl<'a> IntoIterator for &'a mut S { + //~^ ERROR: `IntoIterator` implemented for a reference type without an `iter_mut` method + type IntoIter = std::slice::IterMut<'a, u8>; + type Item = &'a mut u8; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } + { + struct S<T>(T); + impl<'a, T> IntoIterator for &'a S<T> { + //~^ ERROR: `IntoIterator` implemented for a reference type without an `iter` method + type IntoIter = std::slice::Iter<'a, T>; + type Item = &'a T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + impl<'a, T> IntoIterator for &'a mut S<T> { + //~^ ERROR: `IntoIterator` implemented for a reference type without an `iter_mut` method + type IntoIter = std::slice::IterMut<'a, T>; + type Item = &'a mut T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } + { + // Both iter and iter_mut methods exist, don't lint + struct S<'a, T>(&'a T); + + impl<'a, T> S<'a, T> { + fn iter(&self) -> std::slice::Iter<'a, T> { + todo!() + } + fn iter_mut(&mut self) -> std::slice::IterMut<'a, T> { + todo!() + } + } + + impl<'a, T> IntoIterator for &S<'a, T> { + type IntoIter = std::slice::Iter<'a, T>; + type Item = &'a T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + + impl<'a, T> IntoIterator for &mut S<'a, T> { + type IntoIter = std::slice::IterMut<'a, T>; + type Item = &'a mut T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } + { + // Only `iter` exists, no `iter_mut` + struct S<'a, T>(&'a T); + + impl<'a, T> S<'a, T> { + fn iter(&self) -> std::slice::Iter<'a, T> { + todo!() + } + } + + impl<'a, T> IntoIterator for &S<'a, T> { + type IntoIter = std::slice::Iter<'a, T>; + type Item = &'a T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + + impl<'a, T> IntoIterator for &mut S<'a, T> { + //~^ ERROR: `IntoIterator` implemented for a reference type without an `iter_mut` method + type IntoIter = std::slice::IterMut<'a, T>; + type Item = &'a mut T; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } + { + // `iter` exists, but `IntoIterator` is implemented for an alias. inherent_impls doesn't "normalize" + // aliases so that `inherent_impls(Alias)` where `type Alias = S` returns nothing, so this can lead + // to fun FPs. Make sure it doesn't happen here (we're using type_of, which should skip the alias). + struct S; + + impl S { + fn iter(&self) -> std::slice::Iter<'static, u8> { + todo!() + } + } + + type Alias = S; + + impl IntoIterator for &Alias { + type IntoIter = std::slice::Iter<'static, u8>; + type Item = &'static u8; + fn into_iter(self) -> Self::IntoIter { + todo!() + } + } + } +} diff --git a/tests/ui/into_iter_without_iter.stderr b/tests/ui/into_iter_without_iter.stderr new file mode 100644 index 00000000000..f543d1d8e86 --- /dev/null +++ b/tests/ui/into_iter_without_iter.stderr @@ -0,0 +1,114 @@ +error: `IntoIterator` implemented for a reference type without an `iter` method + --> $DIR/into_iter_without_iter.rs:10:9 + | +LL | / impl<'a> IntoIterator for &'a S { +LL | | +LL | | type IntoIter = std::slice::Iter<'a, u8>; +LL | | type Item = &'a u8; +... | +LL | | } +LL | | } + | |_________^ + | + = note: `-D clippy::into-iter-without-iter` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::into_iter_without_iter)]` +help: consider implementing `iter` + | +LL ~ +LL + impl S { +LL + fn iter(&self) -> std::slice::Iter<'a, u8> { +LL + <&Self as IntoIterator>::into_iter(self) +LL + } +LL + } + | + +error: `IntoIterator` implemented for a reference type without an `iter_mut` method + --> $DIR/into_iter_without_iter.rs:18:9 + | +LL | / impl<'a> IntoIterator for &'a mut S { +LL | | +LL | | type IntoIter = std::slice::IterMut<'a, u8>; +LL | | type Item = &'a mut u8; +... | +LL | | } +LL | | } + | |_________^ + | +help: consider implementing `iter_mut` + | +LL ~ +LL + impl S { +LL + fn iter_mut(&mut self) -> std::slice::IterMut<'a, u8> { +LL + <&mut Self as IntoIterator>::into_iter(self) +LL + } +LL + } + | + +error: `IntoIterator` implemented for a reference type without an `iter` method + --> $DIR/into_iter_without_iter.rs:29:9 + | +LL | / impl<'a, T> IntoIterator for &'a S<T> { +LL | | +LL | | type IntoIter = std::slice::Iter<'a, T>; +LL | | type Item = &'a T; +... | +LL | | } +LL | | } + | |_________^ + | +help: consider implementing `iter` + | +LL ~ +LL + impl S<T> { +LL + fn iter(&self) -> std::slice::Iter<'a, T> { +LL + <&Self as IntoIterator>::into_iter(self) +LL + } +LL + } + | + +error: `IntoIterator` implemented for a reference type without an `iter_mut` method + --> $DIR/into_iter_without_iter.rs:37:9 + | +LL | / impl<'a, T> IntoIterator for &'a mut S<T> { +LL | | +LL | | type IntoIter = std::slice::IterMut<'a, T>; +LL | | type Item = &'a mut T; +... | +LL | | } +LL | | } + | |_________^ + | +help: consider implementing `iter_mut` + | +LL ~ +LL + impl S<T> { +LL + fn iter_mut(&mut self) -> std::slice::IterMut<'a, T> { +LL + <&mut Self as IntoIterator>::into_iter(self) +LL + } +LL + } + | + +error: `IntoIterator` implemented for a reference type without an `iter_mut` method + --> $DIR/into_iter_without_iter.rs:93:9 + | +LL | / impl<'a, T> IntoIterator for &mut S<'a, T> { +LL | | +LL | | type IntoIter = std::slice::IterMut<'a, T>; +LL | | type Item = &'a mut T; +... | +LL | | } +LL | | } + | |_________^ + | +help: consider implementing `iter_mut` + | +LL ~ +LL + impl S<'a, T> { +LL + fn iter_mut(&mut self) -> std::slice::IterMut<'a, T> { +LL + <&mut Self as IntoIterator>::into_iter(self) +LL + } +LL + } + | + +error: aborting due to 5 previous errors + |