From 90a915d2fb71eb8cd78b08f9e5b5e51bce69e30e Mon Sep 17 00:00:00 2001 From: Philippe Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Sat, 27 Jan 2024 14:38:04 +0000 Subject: [PATCH 1/9] New lint `missing_iterator_fold` `cargo dev new_lint --name=missing_iterator_fold --pass=late` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 ++ clippy_lints/src/missing_iterator_fold.rs | 26 +++++++++++++++++++++++ tests/ui/missing_iterator_fold.rs | 5 +++++ 5 files changed, 35 insertions(+) create mode 100644 clippy_lints/src/missing_iterator_fold.rs create mode 100644 tests/ui/missing_iterator_fold.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 471163499e52..55757117077b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5360,6 +5360,7 @@ Released 2018-09-13 [`missing_errors_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc [`missing_fields_in_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_fields_in_debug [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items +[`missing_iterator_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_iterator_fold [`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 23d0983151a9..e8107aaeb342 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -488,6 +488,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES_INFO, crate::missing_fields_in_debug::MISSING_FIELDS_IN_DEBUG_INFO, crate::missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS_INFO, + crate::missing_iterator_fold::MISSING_ITERATOR_FOLD_INFO, crate::missing_trait_methods::MISSING_TRAIT_METHODS_INFO, crate::mixed_read_write_in_expression::DIVERGING_SUB_EXPRESSION_INFO, crate::mixed_read_write_in_expression::MIXED_READ_WRITE_IN_EXPRESSION_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7da916ade2ab..ec74870b8b05 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -222,6 +222,7 @@ mod missing_doc; mod missing_enforced_import_rename; mod missing_fields_in_debug; mod missing_inline; +mod missing_iterator_fold; mod missing_trait_methods; mod mixed_read_write_in_expression; mod module_style; @@ -1105,6 +1106,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { }); store.register_late_pass(move |_| Box::new(incompatible_msrv::IncompatibleMsrv::new(msrv()))); store.register_late_pass(|_| Box::new(to_string_trait_impl::ToStringTraitImpl)); + store.register_late_pass(|_| Box::new(missing_iterator_fold::MissingIteratorFold)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs new file mode 100644 index 000000000000..d2ac0301dba2 --- /dev/null +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -0,0 +1,26 @@ +use rustc_hir::*; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.77.0"] + pub MISSING_ITERATOR_FOLD, + nursery, + "default lint description" +} + +declare_lint_pass!(MissingIteratorFold => [MISSING_ITERATOR_FOLD]); + +impl LateLintPass<'_> for MissingIteratorFold {} diff --git a/tests/ui/missing_iterator_fold.rs b/tests/ui/missing_iterator_fold.rs new file mode 100644 index 000000000000..2605cbe86553 --- /dev/null +++ b/tests/ui/missing_iterator_fold.rs @@ -0,0 +1,5 @@ +#![warn(clippy::missing_iterator_fold)] + +fn main() { + // test code goes here +} From 96d30a0ebf2e62f114f377e7790c36edc5fc1f98 Mon Sep 17 00:00:00 2001 From: Philippe Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Sat, 27 Jan 2024 15:42:24 +0000 Subject: [PATCH 2/9] `missing_iterator_fold`: describe the lint --- clippy_lints/src/missing_iterator_fold.rs | 32 ++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index d2ac0301dba2..171c46114bba 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -4,21 +4,47 @@ use rustc_session::declare_lint_pass; declare_clippy_lint! { /// ### What it does + /// Checks for `Iterator` implementations not specializing `fold`. /// /// ### Why is this bad? + /// Specialize `fold` might provide more performant internal iteration. + /// Methods relying on `fold` would then be faster as well. + /// By default, methods that consume the entire iterator rely on it such as `for_each`, `count`... /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning + /// struct MyIter(u32); + /// + /// impl Iterator for MyIter { + /// type Item = u32; + /// fn next(&mut self) -> Option { + /// # todo!() + /// // ... + /// } + /// } /// ``` /// Use instead: /// ```no_run - /// // example code which does not raise clippy warning + /// struct MyIter(u32); + /// + /// impl Iterator for MyIter { + /// type Item = u32; + /// fn next(&mut self) -> Option { + /// # todo!() + /// // ... + /// } + /// fn fold(self, init: B, f: F) -> B + /// where + /// F: FnMut(B, Self::Item) -> B, + /// { + /// todo!() + /// } + /// } /// ``` #[clippy::version = "1.77.0"] pub MISSING_ITERATOR_FOLD, nursery, - "default lint description" + "a missing `Iterator::fold` specialization" } declare_lint_pass!(MissingIteratorFold => [MISSING_ITERATOR_FOLD]); From 4e8fc93fd089c761ea1d4412a02cf9cc64c10ec5 Mon Sep 17 00:00:00 2001 From: Philippe Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Sat, 27 Jan 2024 17:43:10 +0000 Subject: [PATCH 3/9] `missing_iterator_fold`: test the lint --- tests/ui/missing_iterator_fold.rs | 11 +++++++++-- tests/ui/missing_iterator_fold.stderr | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 tests/ui/missing_iterator_fold.stderr diff --git a/tests/ui/missing_iterator_fold.rs b/tests/ui/missing_iterator_fold.rs index 2605cbe86553..6cf365992722 100644 --- a/tests/ui/missing_iterator_fold.rs +++ b/tests/ui/missing_iterator_fold.rs @@ -1,5 +1,12 @@ #![warn(clippy::missing_iterator_fold)] -fn main() { - // test code goes here +struct Countdown(u8); + +impl Iterator for Countdown { + type Item = u8; + + fn next(&mut self) -> Option { + self.0 = self.0.checked_sub(1)?; + Some(self.0) + } } diff --git a/tests/ui/missing_iterator_fold.stderr b/tests/ui/missing_iterator_fold.stderr new file mode 100644 index 000000000000..88c264801e5a --- /dev/null +++ b/tests/ui/missing_iterator_fold.stderr @@ -0,0 +1,17 @@ +error: you are implementing `Iterator` without specializing its `fold` method + --> $DIR/missing_iterator_fold.rs:5:1 + | +LL | / impl Iterator for Countdown { +LL | | type Item = u8; +LL | | +LL | | fn next(&mut self) -> Option { +... | +LL | | } +LL | | } + | |_^ + | + = note: `-D clippy::missing-iterator-fold` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::missing_iterator_fold)]` + +error: aborting due to 1 previous error + From 3e1142148397413f8f65a43c945f661950bc2773 Mon Sep 17 00:00:00 2001 From: Philippe Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Sat, 27 Jan 2024 15:22:03 +0000 Subject: [PATCH 4/9] `missing_iterator_fold`: code the lint logic --- clippy_lints/src/missing_iterator_fold.rs | 32 +++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index 171c46114bba..becb2457e1c0 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -1,6 +1,8 @@ -use rustc_hir::*; +use clippy_utils::diagnostics::span_lint; +use rustc_hir::{AssocItemKind, Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; +use rustc_span::sym; declare_clippy_lint! { /// ### What it does @@ -49,4 +51,30 @@ declare_clippy_lint! { declare_lint_pass!(MissingIteratorFold => [MISSING_ITERATOR_FOLD]); -impl LateLintPass<'_> for MissingIteratorFold {} +impl LateLintPass<'_> for MissingIteratorFold { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + if let ItemKind::Impl(Impl { + of_trait: Some(trait_ref), + .. + }) = &item.kind + && let Some(trait_id) = trait_ref.trait_def_id() + && cx.tcx.is_diagnostic_item(sym::Iterator, trait_id) + { + let has_fold = item + .expect_impl() + .items + .iter() + .filter(|assoc| matches!(assoc.kind, AssocItemKind::Fn { .. })) + .map(|assoc| assoc.ident.name.as_str()) + .any(|name| name == "fold"); + if !has_fold { + span_lint( + cx, + MISSING_ITERATOR_FOLD, + item.span, + "you are implementing `Iterator` without specializing its `fold` method", + ); + } + } + } +} From 84b52f80885279a57b076089cf387f118381c212 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Mon, 29 Jan 2024 12:46:08 +0100 Subject: [PATCH 5/9] Ignore if `in_external_macro` --- clippy_lints/src/missing_iterator_fold.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index becb2457e1c0..3494de1c53ad 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint; use rustc_hir::{AssocItemKind, Impl, Item, ItemKind}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -53,6 +53,9 @@ declare_lint_pass!(MissingIteratorFold => [MISSING_ITERATOR_FOLD]); impl LateLintPass<'_> for MissingIteratorFold { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + if rustc_middle::lint::in_external_macro(cx.sess(), item.span) { + return; + } if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. From 80079ba47ccdedd7d9d0b9e8d9f650f3e076f8ee Mon Sep 17 00:00:00 2001 From: Philippe-Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Tue, 30 Jan 2024 18:44:31 +0100 Subject: [PATCH 6/9] import `in_external_macro` --- clippy_lints/src/missing_iterator_fold.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index 3494de1c53ad..c4c8b35af8fa 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -1,6 +1,7 @@ use clippy_utils::diagnostics::span_lint; use rustc_hir::{AssocItemKind, Impl, Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -53,7 +54,7 @@ declare_lint_pass!(MissingIteratorFold => [MISSING_ITERATOR_FOLD]); impl LateLintPass<'_> for MissingIteratorFold { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - if rustc_middle::lint::in_external_macro(cx.sess(), item.span) { + if in_external_macro(cx.sess(), item.span) { return; } if let ItemKind::Impl(Impl { From f057f1ae7e2bdb6f44bbbbeaf3e7adeebb5e4603 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Tue, 30 Jan 2024 19:07:25 +0100 Subject: [PATCH 7/9] Collapse map-any --- clippy_lints/src/missing_iterator_fold.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index c4c8b35af8fa..d7fbacf07c93 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -69,8 +69,7 @@ impl LateLintPass<'_> for MissingIteratorFold { .items .iter() .filter(|assoc| matches!(assoc.kind, AssocItemKind::Fn { .. })) - .map(|assoc| assoc.ident.name.as_str()) - .any(|name| name == "fold"); + .any(|assoc| assoc.ident.name.as_str() == "fold"); if !has_fold { span_lint( cx, From 496139e0ff20e981c4419db23c5cc4064ef59722 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Tue, 30 Jan 2024 19:17:55 +0100 Subject: [PATCH 8/9] Test it does not lint when `fold` is specialized --- tests/ui/missing_iterator_fold.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/ui/missing_iterator_fold.rs b/tests/ui/missing_iterator_fold.rs index 6cf365992722..a4136012f0f8 100644 --- a/tests/ui/missing_iterator_fold.rs +++ b/tests/ui/missing_iterator_fold.rs @@ -10,3 +10,21 @@ impl Iterator for Countdown { Some(self.0) } } + +struct Countdown2(u8); + +impl Iterator for Countdown2 { + type Item = u8; + + fn next(&mut self) -> Option { + self.0 = self.0.checked_sub(1)?; + Some(self.0) + } + + fn fold(self, init: B, f: F) -> B + where + F: FnMut(B, Self::Item) -> B, + { + (0..self.0).rfold(init, f) + } +} From 2008967a48053664dd7c3b6b92d28c398e644d4e Mon Sep 17 00:00:00 2001 From: Philippe-Cholet <44676486+Philippe-Cholet@users.noreply.github.com> Date: Tue, 30 Jan 2024 19:40:14 +0100 Subject: [PATCH 9/9] Select `restriction` category --- clippy_lints/src/missing_iterator_fold.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/missing_iterator_fold.rs b/clippy_lints/src/missing_iterator_fold.rs index d7fbacf07c93..a2624e13cdd1 100644 --- a/clippy_lints/src/missing_iterator_fold.rs +++ b/clippy_lints/src/missing_iterator_fold.rs @@ -46,7 +46,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.77.0"] pub MISSING_ITERATOR_FOLD, - nursery, + restriction, "a missing `Iterator::fold` specialization" }