Auto merge of #11587 - y21:into_iter_without_iter, r=Jarcho

new lint: `into_iter_without_iter`

Closes #9736 (part 2)

This implements the other lint that my earlier PR missed: given an `IntoIterator for &Type` impl, check that there exists an inherent `fn iter(&self)` method.

changelog: new lint: `into_iter_without_iter`

r? `@Jarcho` since you reviewed #11527 I figured it makes sense for you to review this as well?

Author: bors

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

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,

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 {

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!()
+            }
+        }
+    }
+}