new lint: iter_without_into_iter

y21 · y21 · commit 68b1e8b32234 · 2023-09-18T00:25:44.000+02:00
diff --git 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
@@ -228,6 +228,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::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
@@ -0,0 +1,149 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::get_parent_as_impl;
+use clippy_utils::ty::{implements_trait, make_normalized_projection};
+use rustc_errors::Applicability;
+use rustc_hir::{FnRetTy, ImplItemKind, ImplicitSelfKind, Mutability, QPath, TyKind};
+use rustc_hir_analysis::hir_ty_to_ty;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::print::with_forced_trimmed_paths;
+use rustc_middle::ty::{self};
+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(..))
+}
+
+fn check_for_mutability(cx: &LateContext<'_>, item: &rustc_hir::ImplItem<'_>, expected_mtbl: Mutability) {
+    let (borrow_prefix, name, expected_implicit_self) = match expected_mtbl {
+        Mutability::Not => ("&", "iter", ImplicitSelfKind::ImmRef),
+        Mutability::Mut => ("&mut ", "iter_mut", ImplicitSelfKind::MutRef),
+    };
+
+    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.owner_id.def_id).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 TyKind::Path(QPath::Resolved(_, path)) = imp.self_ty.kind
+        && let Some(ty_did) = path.res.opt_def_id()
+        && let middle_ty = cx.tcx.type_of(ty_did).skip_binder()
+        && let ref_ty = cx.tcx.mk_ty_from_kind(ty::Ref(cx.tcx.lifetimes.re_erased, middle_ty, expected_mtbl))
+        && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
+        && let bound_vars = cx.tcx.late_bound_vars(item.hir_id())
+        // We need to erase late bounds regions so that `-> Iter<'_, T>` works
+        && let ret_middle_ty = cx.tcx.erase_late_bound_regions(
+            ty::Binder::bind_with_vars(hir_ty_to_ty(cx.tcx, ret), bound_vars)
+        )
+        // 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_middle_ty, into_iter_did, &[])
+        && let Some(ret_iter_ty) = make_normalized_projection(
+            cx.tcx,
+            cx.param_env,
+            into_iter_did,
+            sym!(Item),
+            [ret_middle_ty]
+        )
+        // Only lint if the `IntoIterator` impl doesn't actually exist
+        && !implements_trait(cx, ref_ty, into_iter_did, &[])
+    {
+        let (self_ty_snippet, into_iter_snippet) = with_forced_trimmed_paths!((
+            format!("{borrow_prefix}{}", cx.tcx.def_path_str(ty_did)),
+            cx.tcx.def_path_str(into_iter_did)
+        ));
+
+        span_lint_and_then(
+            cx,
+            ITER_WITHOUT_INTO_ITER,
+            item.span,
+            &format!("`{name}` method without an `IntoIterator` impl for `{self_ty_snippet}`"),
+            |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 {into_iter_snippet} for {self_ty_snippet} {{
+    type IntoIter = {ret_middle_ty};
+    type Iter = {ret_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,
+                );
+        });
+    }
+}
+
+impl LateLintPass<'_> for IterWithoutIntoIter {
+    fn check_impl_item(&mut self, cx: &LateContext<'_>, item: &rustc_hir::ImplItem<'_>) {
+        match item.ident.name {
+            sym::iter => check_for_mutability(cx, item, Mutability::Not),
+            sym::iter_mut => check_for_mutability(cx, item, Mutability::Mut),
+            _ => {},
+        };
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -167,6 +167,7 @@ mod invalid_upcast_comparisons;
 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;
@@ -1117,6 +1118,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
             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
@@ -0,0 +1,114 @@
+//@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<Item = &u8> {
+                // 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>(T);
+        impl<T> S4<T> {
+            pub fn iter<U>(&self) -> std::slice::Iter<'static, (T, U)> {
+                todo!()
+            }
+        }
+    }
+    {
+        struct S<T>(T);
+        impl<T> S<T> {
+            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>(T);
+        impl<T> S<T> {
+            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<T> {
+            type Item = &'a String;
+            type IntoIter = std::slice::Iter<'a, String>;
+            fn into_iter(self) -> Self::IntoIter {
+                todo!()
+            }
+        }
+    }
+    {
+        struct S<T>(T);
+        impl<T> S<T> {
+            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<T> {
+            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
