Auto merge of rust-lang#11527 - y21:iter_without_into_iter, r=Jarcho

bors · bors · commit d38fa1a32dd2 · 2023-09-29T01:58:42.000Z
new lint: `iter_without_into_iter` Closes rust-lang#9736 A new lint that looks for `iter` (and `iter_mut`) method implementations without the type implementing `IntoIterator` for `&Type`. Imo this seems rather pedantic, so I went with that, but I can be convinced to change it to `style` like the linked issue asked for. Writing a machine applicable suggestion seems a bit tricky and tedious, so for now this relies on the user adding remaining lifetimes. changelog: new lint: `iter_without_into_iter`
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
@@ -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::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,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
@@ -169,6 +169,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;
@@ -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
@@ -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<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 S5<T>(T);
+        impl<T> S5<T> {
+            pub fn iter(&self) -> std::slice::Iter<'static, T> {
+                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
