diff --git a/CHANGELOG.md b/CHANGELOG.md index dc308285319f..4e41a5adca93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7039,6 +7039,7 @@ Released 2018-09-13 [`iter_filter_is_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_ok [`iter_filter_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_filter_is_some [`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map +[`iter_missing_exact_size`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_missing_exact_size [`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop [`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice [`iter_not_returning_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_not_returning_iterator diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index b14ee7f3de90..2aee0492ac5b 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -244,6 +244,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::item_name_repetitions::STRUCT_FIELD_NAMES_INFO, crate::items_after_statements::ITEMS_AFTER_STATEMENTS_INFO, crate::items_after_test_module::ITEMS_AFTER_TEST_MODULE_INFO, + crate::iter_missing_exact_size::ITER_MISSING_EXACT_SIZE_INFO, crate::iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR_INFO, crate::iter_over_hash_type::ITER_OVER_HASH_TYPE_INFO, crate::iter_without_into_iter::INTO_ITER_WITHOUT_ITER_INFO, diff --git a/clippy_lints/src/iter_missing_exact_size.rs b/clippy_lints/src/iter_missing_exact_size.rs new file mode 100644 index 000000000000..6c18675244d0 --- /dev/null +++ b/clippy_lints/src/iter_missing_exact_size.rs @@ -0,0 +1,190 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::paths::EXACT_SIZE_ITERATOR; +use clippy_utils::sym; +use clippy_utils::ty::{get_field_by_name, implements_trait, ty_from_hir_ty}; +use rustc_hir::{Body, Expr, ExprKind, Impl, ImplItemKind, Item, ItemKind, OwnerId, OwnerNode, QPath, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::symbol::kw; + +declare_clippy_lint! { + /// ### What it does + /// + /// Suggest that iterators be marked as `ExactSizeIterator` when they wrap + /// around another iterator that *does* implement `ExactSizeIterator`. + /// + /// ### Why is this bad? + /// + /// When the size of an iterator is based on some other iterator that is + /// known to have an exact size, the wrapping iterator may also have an + /// exact size and should be marked as such. + /// + /// ### Example + /// ```no_run + /// struct StringRepeater { + /// original: String, + /// range: std::ops::Range, + /// } + /// + /// impl Iterator for StringRepeater { + /// type Item = String; + /// fn next(&mut self) -> Option { + /// self.range.next().map(|i| self.original.repeat(i) ) + /// } + /// fn size_hint(&self) -> (usize, Option) { + /// self.range.size_hint() + /// } + /// } + /// + /// let repeater = StringRepeater { original: "Foo".to_string(), range: 1..5 }; + /// for value in repeater { + /// println!("{value}"); + /// } + /// + /// ``` + /// Use instead: + /// + /// ```no_run + /// struct StringRepeater { + /// original: String, + /// range: std::ops::Range, + /// } + /// + /// impl Iterator for StringRepeater { + /// type Item = String; + /// fn next(&mut self) -> Option { + /// self.range.next().map(|i| self.original.repeat(i) ) + /// } + /// fn size_hint(&self) -> (usize, Option) { + /// self.range.size_hint() + /// } + /// } + /// + /// impl ExactSizeIterator for StringRepeater {} + /// + /// let repeater = StringRepeater { original: "Foo".to_string(), range: 1..5 }; + /// for value in repeater { + /// println!("{value}"); + /// } + /// ``` + #[clippy::version = "1.99.0"] + pub ITER_MISSING_EXACT_SIZE, + pedantic, + "iterator delegates to an ExactSizeIterator for its size hint but does not itself implement ExactSizeIterator" +} + +declare_lint_pass!(IterMissingExactSize => [ITER_MISSING_EXACT_SIZE]); + +/// Given an `OwnerId` for an item that is `impl Iterator for {type}`, try to +/// locate the `size_hint()` function being defined in that block. +fn size_hint_body<'tcx>(cx: &LateContext<'tcx>, owner_id: OwnerId) -> Option<&'tcx Body<'tcx>> { + let size_hint_fn = cx + .tcx + .associated_items(owner_id) + .filter_by_name_unhygienic(sym::size_hint) + .find(|assoc_item| assoc_item.expect_trait_impl().is_ok() && assoc_item.is_method())?; + + let node = cx.tcx.expect_hir_owner_node(size_hint_fn.def_id.expect_local()); + let OwnerNode::ImplItem(impl_item) = node else { + return None; + }; + let ImplItemKind::Fn(_, body_id) = impl_item.kind else { + return None; + }; + Some(cx.tcx.hir_body(body_id)) +} + +/// Given a `Body` for the `size_hint()` function, try to get the return +/// expression, either +/// - a trailing expression when the block has no statements +/// - the singular `return` statement in a block with exactly one statement (the return) (and +/// optionally a dead-code trailing expression that we can ignore) +fn size_hint_return<'tcx>(body: &'tcx Body<'tcx>) -> Option<&'tcx Expr<'tcx>> { + let ExprKind::Block(block, None) = body.value.kind else { + // Function body isn't a block? + return None; + }; + // Block without statements - either it has a trailing expression (and we + // return that) or it doesn't (and we return none) + if block.stmts.is_empty() { + return block.expr; + } + if let [only_statement] = block.stmts + && let StmtKind::Semi(statement_semi) = only_statement.kind + && let ExprKind::Ret(returned_value) = statement_semi.kind + { + returned_value + } else { + None + } +} + +impl<'tcx> LateLintPass<'tcx> for IterMissingExactSize { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { + // Check for this item being an implementation of the iterator trait: + // 1) is it implementing a trait? + let ItemKind::Impl(Impl { + of_trait: Some(of_trait), + self_ty: current_type, + .. + }) = item.kind + else { + return; + }; + // 2) can we find the trait definition id? + let Some(trait_id) = of_trait.trait_ref.trait_def_id() else { + return; + }; + // 3) is it the iterator trait? + if !cx.tcx.is_diagnostic_item(sym::Iterator, trait_id) { + return; + } + + // We know that this item is an `impl Iterator for _` block; find the + // size_hint() function (if present) + let Some(size_hint_body) = size_hint_body(cx, item.owner_id) else { + return; + }; + // We found the function body for size_hint()! + let Some(size_hint_return) = size_hint_return(size_hint_body) else { + return; + }; + if let ExprKind::MethodCall(method_name, receiver, args, _) = size_hint_return.kind + && method_name.ident.name == sym::size_hint + && let ExprKind::Field(object, field_name) = receiver.kind + && let ExprKind::Path(QPath::Resolved(_, object_path)) = object.kind + && let [path_segment] = object_path.segments + && path_segment.ident.name == kw::SelfLower + && args.is_empty() + { + // The function body is just `self.{field}.size_hint()`, check + // for the type of the field + let current_middle_ty = ty_from_hir_ty(cx, current_type); + let field = get_field_by_name(cx.tcx, current_middle_ty, field_name.name); + let Some(field) = field else { + return; + }; + // Does that type implement ExactSizeIterator ? + let Some(trait_def) = EXACT_SIZE_ITERATOR.only(cx) else { + // Type isn't know, no_core environment + return; + }; + if !implements_trait(cx, field, trait_def, &[]) { + // Field type does not implement ExactSizeIterator + return; + } + if implements_trait(cx, current_middle_ty, trait_def, &[]) { + // Overall type already implements ExactSizeIterator + return; + } + span_lint_and_help( + cx, + ITER_MISSING_EXACT_SIZE, + item.span, + "iterator can implement `ExactSizeIterator`", + Some(size_hint_return.span), + "this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size", + ); + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7a233e09e800..f087ac2c741b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -179,6 +179,7 @@ mod int_plus_one; mod item_name_repetitions; mod items_after_statements; mod items_after_test_module; +mod iter_missing_exact_size; mod iter_not_returning_iterator; mod iter_over_hash_type; mod iter_without_into_iter; @@ -868,6 +869,7 @@ rustc_lint::late_lint_methods!( RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse, RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct, BlockScrutinee: block_scrutinee::BlockScrutinee = block_scrutinee::BlockScrutinee, + IterMissingExactSize: iter_missing_exact_size::IterMissingExactSize = iter_missing_exact_size::IterMissingExactSize, // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index f27c92f0d492..36e392cc7a3b 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -134,6 +134,7 @@ path_macros! { } // Paths in the standard library missing a diagnostic item +pub static EXACT_SIZE_ITERATOR: PathLookup = type_path!(core::iter::ExactSizeIterator); // Paths in external crates pub static FUTURES_IO_ASYNCREADEXT: PathLookup = type_path!(futures_util::AsyncReadExt); diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 5da94bfda5e6..fc296296b5ab 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -62,6 +62,7 @@ generate! { EarlyLintPass, Enumerate, Error, + ExactSizeIterator, File, FileType, FsOpenOptions, @@ -545,6 +546,7 @@ generate! { set_readonly, signum, single_component_path_imports, + size_hint, skip, skip_while, slice_from_ref, diff --git a/tests/ui/iter_missing_exact_size.rs b/tests/ui/iter_missing_exact_size.rs new file mode 100644 index 000000000000..06b21c0db880 --- /dev/null +++ b/tests/ui/iter_missing_exact_size.rs @@ -0,0 +1,163 @@ +#![warn(clippy::iter_missing_exact_size)] +#![allow(clippy::needless_return)] +#![allow(unreachable_code)] + +use std::ops::Range; + +// Struct field +struct StringRepeater1 { + original: String, + range: Range, +} + +impl Iterator for StringRepeater1 { + //~^ iter_missing_exact_size + type Item = String; + fn next(&mut self) -> Option { + self.range.next().map(|i| self.original.repeat(i)) + } + fn size_hint(&self) -> (usize, Option) { + self.range.size_hint() + } +} + +// Tuple-like struct +struct StringRepeater2(String, Range); + +impl Iterator for StringRepeater2 { + //~^ iter_missing_exact_size + type Item = String; + fn next(&mut self) -> Option { + self.1.next().map(|i| self.0.repeat(i)) + } + fn size_hint(&self) -> (usize, Option) { + self.1.size_hint() + } +} + +// Uses `return _;` rather than a trailing expression +struct StringRepeater3(String, Range); + +impl Iterator for StringRepeater3 { + //~^ iter_missing_exact_size + type Item = String; + fn next(&mut self) -> Option { + self.1.next().map(|i| self.0.repeat(i)) + } + fn size_hint(&self) -> (usize, Option) { + return self.1.size_hint(); + } +} + +// Uses `return _;` followed by a dead-code trailing expression +struct StringRepeater4(String, Range); + +impl Iterator for StringRepeater4 { + //~^ iter_missing_exact_size + type Item = String; + fn next(&mut self) -> Option { + self.1.next().map(|i| self.0.repeat(i)) + } + fn size_hint(&self) -> (usize, Option) { + return self.1.size_hint(); + (5, Some(5)) + } +} + +// Already marked as an ExactSizeIterator +struct StringRepeater5(String, Range); + +impl Iterator for StringRepeater5 { + type Item = String; + fn next(&mut self) -> Option { + self.1.next().map(|i| self.0.repeat(i)) + } + fn size_hint(&self) -> (usize, Option) { + self.1.size_hint() + } +} +impl ExactSizeIterator for StringRepeater5 {} + +// Delegates but to a non-ExactSizeIterator iterator +struct MyCollection { + elements: Vec, +} +impl MyCollection { + fn size_hint(&self) -> (usize, Option) { + (self.elements.len(), Some(self.elements.len())) + } +} + +struct MyCollectionIter { + inner: MyCollection, +} + +impl Iterator for MyCollectionIter { + type Item = u8; + + fn next(&mut self) -> Option { + self.inner.elements.pop() + } + fn size_hint(&self) -> (usize, Option) { + self.inner.size_hint() + } +} + +// Delegates to an ExactSizeIterator but not an object field +struct EmptyWithHint {} + +fn range_provider() -> (Range,) { + (0..5,) +} + +impl Iterator for EmptyWithHint { + type Item = String; + fn next(&mut self) -> Option { + None + } + fn size_hint(&self) -> (usize, Option) { + range_provider().0.size_hint() + } +} + +fn main() { + let repeater = StringRepeater1 { + original: "Foo".to_string(), + range: 1..5, + }; + for value in repeater { + println!("{value}"); + } + + let repeater = StringRepeater2("Bar".to_string(), 1..5); + for value in repeater { + println!("{value}"); + } + + let repeater = StringRepeater3("Bar".to_string(), 1..5); + for value in repeater { + println!("{value}"); + } + + let repeater = StringRepeater4("Bar".to_string(), 1..5); + for value in repeater { + println!("{value}"); + } + + let repeater = StringRepeater5("Bar".to_string(), 1..5); + for value in repeater { + println!("{value}"); + } + + let collection = MyCollectionIter { + inner: MyCollection { + elements: vec![3, 2, 1], + }, + }; + for value in collection { + println!("{value}"); + } + + let mut empty_provider = EmptyWithHint {}; + assert_eq!(None, empty_provider.next()); +} diff --git a/tests/ui/iter_missing_exact_size.stderr b/tests/ui/iter_missing_exact_size.stderr new file mode 100644 index 000000000000..5b5ba8c7e562 --- /dev/null +++ b/tests/ui/iter_missing_exact_size.stderr @@ -0,0 +1,72 @@ +error: iterator can implement `ExactSizeIterator` + --> tests/ui/iter_missing_exact_size.rs:13:1 + | +LL | / impl Iterator for StringRepeater1 { +LL | | +LL | | type Item = String; +LL | | fn next(&mut self) -> Option { +... | +LL | | } + | |_^ + | +help: this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size + --> tests/ui/iter_missing_exact_size.rs:20:9 + | +LL | self.range.size_hint() + | ^^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::iter-missing-exact-size` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::iter_missing_exact_size)]` + +error: iterator can implement `ExactSizeIterator` + --> tests/ui/iter_missing_exact_size.rs:27:1 + | +LL | / impl Iterator for StringRepeater2 { +LL | | +LL | | type Item = String; +LL | | fn next(&mut self) -> Option { +... | +LL | | } + | |_^ + | +help: this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size + --> tests/ui/iter_missing_exact_size.rs:34:9 + | +LL | self.1.size_hint() + | ^^^^^^^^^^^^^^^^^^ + +error: iterator can implement `ExactSizeIterator` + --> tests/ui/iter_missing_exact_size.rs:41:1 + | +LL | / impl Iterator for StringRepeater3 { +LL | | +LL | | type Item = String; +LL | | fn next(&mut self) -> Option { +... | +LL | | } + | |_^ + | +help: this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size + --> tests/ui/iter_missing_exact_size.rs:48:16 + | +LL | return self.1.size_hint(); + | ^^^^^^^^^^^^^^^^^^ + +error: iterator can implement `ExactSizeIterator` + --> tests/ui/iter_missing_exact_size.rs:55:1 + | +LL | / impl Iterator for StringRepeater4 { +LL | | +LL | | type Item = String; +LL | | fn next(&mut self) -> Option { +... | +LL | | } + | |_^ + | +help: this `size_hint()` implementation delegates to to the `size_hint()` of an `ExactSizeIterator`, so the overall iterator is likely to have an exact size + --> tests/ui/iter_missing_exact_size.rs:62:16 + | +LL | return self.1.size_hint(); + | ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors +