From 2666c38acb8352b7fd5903f50ee5fd76e2441cff Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Thu, 28 Jul 2022 20:50:43 -0400 Subject: [PATCH 1/2] Add [`unused_peekable`] lint --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_suspicious.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unused_peekable.rs | 208 ++++++++++++++++++++ clippy_utils/src/paths.rs | 1 + clippy_utils/src/ty.rs | 2 +- tests/ui/unused_peekable.rs | 119 +++++++++++ tests/ui/unused_peekable.stderr | 51 +++++ 10 files changed, 386 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/unused_peekable.rs create mode 100644 tests/ui/unused_peekable.rs create mode 100644 tests/ui/unused_peekable.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index edd7bc250a759..c42a03c04a00f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4152,6 +4152,7 @@ Released 2018-09-13 [`unused_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_collect [`unused_io_amount`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount [`unused_label`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_label +[`unused_peekable`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_peekable [`unused_rounding`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_rounding [`unused_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_self [`unused_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index a0a4b07a77e5a..de0514c1b6650 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -337,6 +337,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(unnecessary_sort_by::UNNECESSARY_SORT_BY), LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), LintId::of(unused_io_amount::UNUSED_IO_AMOUNT), + LintId::of(unused_peekable::UNUSED_PEEKABLE), LintId::of(unused_unit::UNUSED_UNIT), LintId::of(unwrap::PANICKING_UNWRAP), LintId::of(unwrap::UNNECESSARY_UNWRAP), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index b7dbd30aa0c85..eb23bb0c7dbe6 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -573,6 +573,7 @@ store.register_lints(&[ unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, unused_async::UNUSED_ASYNC, unused_io_amount::UNUSED_IO_AMOUNT, + unused_peekable::UNUSED_PEEKABLE, unused_rounding::UNUSED_ROUNDING, unused_self::UNUSED_SELF, unused_unit::UNUSED_UNIT, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 6e185f8d31ee1..5793d59bcad11 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -33,4 +33,5 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS), + LintId::of(unused_peekable::UNUSED_PEEKABLE), ]) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a041fbc7fd97a..d4c6c4918cb30 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -398,6 +398,7 @@ mod unnested_or_patterns; mod unsafe_removed_from_name; mod unused_async; mod unused_io_amount; +mod unused_peekable; mod unused_rounding; mod unused_self; mod unused_unit; @@ -935,6 +936,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed)); store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone)); store.register_late_pass(|| Box::new(manual_empty_string_creations::ManualEmptyStringCreations)); + store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable::default())); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unused_peekable.rs b/clippy_lints/src/unused_peekable.rs new file mode 100644 index 0000000000000..4988ec0eca12c --- /dev/null +++ b/clippy_lints/src/unused_peekable.rs @@ -0,0 +1,208 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::ty::{match_type, peel_mid_ty_refs_is_mutable}; +use clippy_utils::{fn_def_id, path_to_local_id, paths, peel_ref_operators}; +use rustc_ast::Mutability; +use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::lang_items::LangItem; +use rustc_hir::{Block, Expr, ExprKind, HirId, Local, Node, PatKind, PathSegment, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for the creation of a `peekable` iterator that is never `.peek()`ed + /// + /// ### Why is this bad? + /// Creating a peekable iterator without using any of its methods is likely a mistake, + /// or just a leftover after a refactor. + /// + /// ### Example + /// ```rust + /// let collection = vec![1, 2, 3]; + /// let iter = collection.iter().peekable(); + /// + /// for item in iter { + /// // ... + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// let collection = vec![1, 2, 3]; + /// let iter = collection.iter(); + /// + /// for item in iter { + /// // ... + /// } + /// ``` + #[clippy::version = "1.64.0"] + pub UNUSED_PEEKABLE, + suspicious, + "creating a peekable iterator without using any of its methods" +} + +#[derive(Default)] +pub struct UnusedPeekable { + visited: Vec, +} + +impl_lint_pass!(UnusedPeekable => [UNUSED_PEEKABLE]); + +impl<'tcx> LateLintPass<'tcx> for UnusedPeekable { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) { + // Don't lint `Peekable`s returned from a block + if let Some(expr) = block.expr + && let Some(ty) = cx.typeck_results().expr_ty_opt(peel_ref_operators(cx, expr)) + && match_type(cx, ty, &paths::PEEKABLE) + { + return; + } + + for (idx, stmt) in block.stmts.iter().enumerate() { + if !stmt.span.from_expansion() + && let StmtKind::Local(local) = stmt.kind + && !self.visited.contains(&local.pat.hir_id) + && let PatKind::Binding(_, _, ident, _) = local.pat.kind + && let Some(init) = local.init + && !init.span.from_expansion() + && let Some(ty) = cx.typeck_results().expr_ty_opt(init) + && let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty) + && match_type(cx, ty, &paths::PEEKABLE) + { + let mut vis = PeekableVisitor::new(cx, local.pat.hir_id); + + if idx + 1 == block.stmts.len() && block.expr.is_none() { + return; + } + + for stmt in &block.stmts[idx..] { + vis.visit_stmt(stmt); + } + + if let Some(expr) = block.expr { + vis.visit_expr(expr); + } + + if !vis.found_peek_call { + span_lint_and_help( + cx, + UNUSED_PEEKABLE, + ident.span, + "`peek` never called on `Peekable` iterator", + None, + "consider removing the call to `peekable`" + ); + } + } + } + } +} + +struct PeekableVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + expected_hir_id: HirId, + found_peek_call: bool, +} + +impl<'a, 'tcx> PeekableVisitor<'a, 'tcx> { + fn new(cx: &'a LateContext<'tcx>, expected_hir_id: HirId) -> Self { + Self { + cx, + expected_hir_id, + found_peek_call: false, + } + } +} + +impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> { + fn visit_expr(&mut self, ex: &'_ Expr<'_>) { + if path_to_local_id(ex, self.expected_hir_id) { + for (_, node) in self.cx.tcx.hir().parent_iter(ex.hir_id) { + match node { + Node::Expr(expr) => { + match expr.kind { + // some_function(peekable) + // + // If the Peekable is passed to a function, stop + ExprKind::Call(_, args) => { + if let Some(func_did) = fn_def_id(self.cx, expr) + && let Ok(into_iter_did) = self + .cx + .tcx + .lang_items() + .require(LangItem::IntoIterIntoIter) + && func_did == into_iter_did + { + // Probably a for loop desugar, stop searching + return; + } + + for arg in args.iter().map(|arg| peel_ref_operators(self.cx, arg)) { + if let ExprKind::Path(_) = arg.kind + && let Some(ty) = self + .cx + .typeck_results() + .expr_ty_opt(arg) + .map(Ty::peel_refs) + && match_type(self.cx, ty, &paths::PEEKABLE) + { + self.found_peek_call = true; + return; + } + } + }, + // Peekable::peek() + ExprKind::MethodCall(PathSegment { ident: method_name, .. }, [arg, ..], _) => { + let arg = peel_ref_operators(self.cx, arg); + let method_name = method_name.name.as_str(); + + if (method_name == "peek" + || method_name == "peek_mut" + || method_name == "next_if" + || method_name == "next_if_eq") + && let ExprKind::Path(_) = arg.kind + && let Some(ty) = self.cx.typeck_results().expr_ty_opt(arg).map(Ty::peel_refs) + && match_type(self.cx, ty, &paths::PEEKABLE) + { + self.found_peek_call = true; + return; + } + }, + // Don't bother if moved into a struct + ExprKind::Struct(..) => { + self.found_peek_call = true; + return; + }, + _ => {}, + } + }, + Node::Local(Local { init: Some(init), .. }) => { + if let Some(ty) = self.cx.typeck_results().expr_ty_opt(init) + && let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty) + && match_type(self.cx, ty, &paths::PEEKABLE) + { + self.found_peek_call = true; + return; + } + + break; + }, + Node::Stmt(stmt) => match stmt.kind { + StmtKind::Expr(_) | StmtKind::Semi(_) => {}, + _ => { + self.found_peek_call = true; + return; + }, + }, + Node::Block(_) => {}, + _ => { + break; + }, + } + } + } + + walk_expr(self, ex); + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 8d697a301c444..d3ae537a890de 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -96,6 +96,7 @@ pub const PARKING_LOT_RWLOCK_READ_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwL pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 3] = ["lock_api", "rwlock", "RwLockWriteGuard"]; pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"]; pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; +pub const PEEKABLE: [&str; 5] = ["core", "iter", "adapters", "peekable", "Peekable"]; pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"]; #[cfg_attr(not(unix), allow(clippy::invalid_paths))] pub const PERMISSIONS_FROM_MODE: [&str; 6] = ["std", "os", "unix", "fs", "PermissionsExt", "from_mode"]; diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index e7d670766a050..edbe9c5a8970e 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -410,7 +410,7 @@ pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) { peel(ty, 0) } -/// Peels off all references on the type.Returns the underlying type, the number of references +/// Peels off all references on the type. Returns the underlying type, the number of references /// removed, and whether the pointer is ultimately mutable or not. pub fn peel_mid_ty_refs_is_mutable(ty: Ty<'_>) -> (Ty<'_>, usize, Mutability) { fn f(ty: Ty<'_>, count: usize, mutability: Mutability) -> (Ty<'_>, usize, Mutability) { diff --git a/tests/ui/unused_peekable.rs b/tests/ui/unused_peekable.rs new file mode 100644 index 0000000000000..12cab9956219c --- /dev/null +++ b/tests/ui/unused_peekable.rs @@ -0,0 +1,119 @@ +#![warn(clippy::unused_peekable)] +#![allow(clippy::no_effect)] + +use std::iter::Empty; +use std::iter::Peekable; + +fn main() { + invalid(); + valid(); +} + +#[allow(clippy::unused_unit)] +fn invalid() { + let peekable = std::iter::empty::().peekable(); + + // Only lint `new_local` + let old_local = std::iter::empty::().peekable(); + let new_local = old_local; + + // Behind mut ref + let mut by_mut_ref_test = std::iter::empty::().peekable(); + let by_mut_ref = &mut by_mut_ref_test; + + // Explicitly returns `Peekable` + fn returns_peekable() -> Peekable> { + std::iter::empty().peekable() + } + + let peekable_from_fn = returns_peekable(); + + // Using a method not exclusive to `Peekable` + let mut peekable_using_iterator_method = std::iter::empty::().peekable(); + peekable_using_iterator_method.next(); + + let mut peekable_in_for_loop = std::iter::empty::().peekable(); + for x in peekable_in_for_loop {} +} + +fn valid() { + fn takes_peekable(_peek: Peekable>) {} + + // Passed to another function + let passed_along = std::iter::empty::().peekable(); + takes_peekable(passed_along); + + // `peek` called in another block + let mut peekable_in_block = std::iter::empty::().peekable(); + { + peekable_in_block.peek(); + } + + // Check the other `Peekable` methods :) + { + let mut peekable_with_peek_mut = std::iter::empty::().peekable(); + peekable_with_peek_mut.peek_mut(); + + let mut peekable_with_next_if = std::iter::empty::().peekable(); + peekable_with_next_if.next_if(|_| true); + + let mut peekable_with_next_if_eq = std::iter::empty::().peekable(); + peekable_with_next_if_eq.next_if_eq(&3); + } + + let mut peekable_in_closure = std::iter::empty::().peekable(); + let call_peek = |p: &mut Peekable>| { + p.peek(); + }; + call_peek(&mut peekable_in_closure); + + // From a macro + macro_rules! make_me_a_peekable_please { + () => { + std::iter::empty::().peekable() + }; + } + + let _unsuspecting_macro_user = make_me_a_peekable_please!(); + + // Generic Iterator returned + fn return_an_iter() -> impl Iterator { + std::iter::empty::().peekable() + } + + let _unsuspecting_user = return_an_iter(); + + // Call `peek` in a macro + macro_rules! peek_iter { + ($iter:ident) => { + $iter.peek(); + }; + } + + let mut peek_in_macro = std::iter::empty::().peekable(); + peek_iter!(peek_in_macro); + + // Behind mut ref + let mut by_mut_ref_test = std::iter::empty::().peekable(); + let by_mut_ref = &mut by_mut_ref_test; + by_mut_ref.peek(); + + // Behind ref + let mut by_ref_test = std::iter::empty::().peekable(); + let by_ref = &by_ref_test; + by_ref_test.peek(); + + // In struct + struct PeekableWrapper { + f: Peekable>, + } + + let struct_test = std::iter::empty::().peekable(); + PeekableWrapper { f: struct_test }; + + // `peek` called in another block as the last expression + let mut peekable_last_expr = std::iter::empty::().peekable(); + { + peekable_last_expr.peek(); + } +} diff --git a/tests/ui/unused_peekable.stderr b/tests/ui/unused_peekable.stderr new file mode 100644 index 0000000000000..bd087f56e4ceb --- /dev/null +++ b/tests/ui/unused_peekable.stderr @@ -0,0 +1,51 @@ +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:14:9 + | +LL | let peekable = std::iter::empty::().peekable(); + | ^^^^^^^^ + | + = note: `-D clippy::unused-peekable` implied by `-D warnings` + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:18:9 + | +LL | let new_local = old_local; + | ^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:22:9 + | +LL | let by_mut_ref = &mut by_mut_ref_test; + | ^^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:29:9 + | +LL | let peekable_from_fn = returns_peekable(); + | ^^^^^^^^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:32:13 + | +LL | let mut peekable_using_iterator_method = std::iter::empty::().peekable(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:35:13 + | +LL | let mut peekable_in_for_loop = std::iter::empty::().peekable(); + | ^^^^^^^^^^^^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: aborting due to 6 previous errors + From 0efafa4a6e2d96f55584fe77782c289199a560b1 Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Fri, 5 Aug 2022 13:49:43 -0400 Subject: [PATCH 2/2] Better handle method/function calls --- clippy_lints/src/lib.register_suspicious.rs | 2 +- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/unused_peekable.rs | 103 ++++++++++++-------- tests/ui/unused_peekable.rs | 25 +++++ tests/ui/unused_peekable.stderr | 20 +++- 5 files changed, 105 insertions(+), 47 deletions(-) diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 5793d59bcad11..369d4b4eed697 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -32,6 +32,6 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), - LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS), LintId::of(unused_peekable::UNUSED_PEEKABLE), + LintId::of(write::POSITIONAL_NAMED_FORMAT_PARAMETERS), ]) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4c6c4918cb30..2a0fb30131f7d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -936,7 +936,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed)); store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone)); store.register_late_pass(|| Box::new(manual_empty_string_creations::ManualEmptyStringCreations)); - store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable::default())); + store.register_late_pass(|| Box::new(unused_peekable::UnusedPeekable)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unused_peekable.rs b/clippy_lints/src/unused_peekable.rs index 4988ec0eca12c..c060d767e2340 100644 --- a/clippy_lints/src/unused_peekable.rs +++ b/clippy_lints/src/unused_peekable.rs @@ -1,13 +1,13 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::ty::{match_type, peel_mid_ty_refs_is_mutable}; -use clippy_utils::{fn_def_id, path_to_local_id, paths, peel_ref_operators}; +use clippy_utils::{fn_def_id, is_trait_method, path_to_local_id, paths, peel_ref_operators}; use rustc_ast::Mutability; use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_hir::lang_items::LangItem; use rustc_hir::{Block, Expr, ExprKind, HirId, Local, Node, PatKind, PathSegment, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::Ty; -use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; declare_clippy_lint! { /// ### What it does @@ -42,12 +42,7 @@ declare_clippy_lint! { "creating a peekable iterator without using any of its methods" } -#[derive(Default)] -pub struct UnusedPeekable { - visited: Vec, -} - -impl_lint_pass!(UnusedPeekable => [UNUSED_PEEKABLE]); +declare_lint_pass!(UnusedPeekable => [UNUSED_PEEKABLE]); impl<'tcx> LateLintPass<'tcx> for UnusedPeekable { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) { @@ -62,15 +57,14 @@ impl<'tcx> LateLintPass<'tcx> for UnusedPeekable { for (idx, stmt) in block.stmts.iter().enumerate() { if !stmt.span.from_expansion() && let StmtKind::Local(local) = stmt.kind - && !self.visited.contains(&local.pat.hir_id) - && let PatKind::Binding(_, _, ident, _) = local.pat.kind + && let PatKind::Binding(_, binding, ident, _) = local.pat.kind && let Some(init) = local.init && !init.span.from_expansion() && let Some(ty) = cx.typeck_results().expr_ty_opt(init) && let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty) && match_type(cx, ty, &paths::PEEKABLE) { - let mut vis = PeekableVisitor::new(cx, local.pat.hir_id); + let mut vis = PeekableVisitor::new(cx, binding); if idx + 1 == block.stmts.len() && block.expr.is_none() { return; @@ -117,6 +111,10 @@ impl<'a, 'tcx> PeekableVisitor<'a, 'tcx> { impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> { fn visit_expr(&mut self, ex: &'_ Expr<'_>) { + if self.found_peek_call { + return; + } + if path_to_local_id(ex, self.expected_hir_id) { for (_, node) in self.cx.tcx.hir().parent_iter(ex.hir_id) { match node { @@ -138,50 +136,58 @@ impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> { return; } - for arg in args.iter().map(|arg| peel_ref_operators(self.cx, arg)) { - if let ExprKind::Path(_) = arg.kind - && let Some(ty) = self - .cx - .typeck_results() - .expr_ty_opt(arg) - .map(Ty::peel_refs) - && match_type(self.cx, ty, &paths::PEEKABLE) - { - self.found_peek_call = true; - return; - } + if args.iter().any(|arg| { + matches!(arg.kind, ExprKind::Path(_)) && arg_is_mut_peekable(self.cx, arg) + }) { + self.found_peek_call = true; + return; } }, - // Peekable::peek() - ExprKind::MethodCall(PathSegment { ident: method_name, .. }, [arg, ..], _) => { - let arg = peel_ref_operators(self.cx, arg); - let method_name = method_name.name.as_str(); - - if (method_name == "peek" - || method_name == "peek_mut" - || method_name == "next_if" - || method_name == "next_if_eq") - && let ExprKind::Path(_) = arg.kind - && let Some(ty) = self.cx.typeck_results().expr_ty_opt(arg).map(Ty::peel_refs) - && match_type(self.cx, ty, &paths::PEEKABLE) + // Catch anything taking a Peekable mutably + ExprKind::MethodCall( + PathSegment { + ident: method_name_ident, + .. + }, + [self_arg, remaining_args @ ..], + _, + ) => { + let method_name = method_name_ident.name.as_str(); + + // `Peekable` methods + if matches!(method_name, "peek" | "peek_mut" | "next_if" | "next_if_eq") + && arg_is_mut_peekable(self.cx, self_arg) + { + self.found_peek_call = true; + return; + } + + // foo.some_method() excluding Iterator methods + if remaining_args.iter().any(|arg| arg_is_mut_peekable(self.cx, arg)) + && !is_trait_method(self.cx, expr, sym::Iterator) { self.found_peek_call = true; return; } + + // foo.by_ref(), keep checking for `peek` + if method_name == "by_ref" { + continue; + } + + return; + }, + ExprKind::AddrOf(_, Mutability::Mut, _) | ExprKind::Unary(..) | ExprKind::DropTemps(_) => { }, - // Don't bother if moved into a struct - ExprKind::Struct(..) => { + ExprKind::AddrOf(_, Mutability::Not, _) => return, + _ => { self.found_peek_call = true; return; }, - _ => {}, } }, Node::Local(Local { init: Some(init), .. }) => { - if let Some(ty) = self.cx.typeck_results().expr_ty_opt(init) - && let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty) - && match_type(self.cx, ty, &paths::PEEKABLE) - { + if arg_is_mut_peekable(self.cx, init) { self.found_peek_call = true; return; } @@ -206,3 +212,14 @@ impl<'tcx> Visitor<'_> for PeekableVisitor<'_, 'tcx> { walk_expr(self, ex); } } + +fn arg_is_mut_peekable(cx: &LateContext<'_>, arg: &Expr<'_>) -> bool { + if let Some(ty) = cx.typeck_results().expr_ty_opt(arg) + && let (ty, _, Mutability::Mut) = peel_mid_ty_refs_is_mutable(ty) + && match_type(cx, ty, &paths::PEEKABLE) + { + true + } else { + false + } +} diff --git a/tests/ui/unused_peekable.rs b/tests/ui/unused_peekable.rs index 12cab9956219c..153457e367165 100644 --- a/tests/ui/unused_peekable.rs +++ b/tests/ui/unused_peekable.rs @@ -32,6 +32,15 @@ fn invalid() { let mut peekable_using_iterator_method = std::iter::empty::().peekable(); peekable_using_iterator_method.next(); + // Passed by ref to another function + fn takes_ref(_peek: &Peekable>) {} + let passed_along_ref = std::iter::empty::().peekable(); + takes_ref(&passed_along_ref); + + // `by_ref` without `peek` + let mut by_ref_test = std::iter::empty::().peekable(); + let _by_ref = by_ref_test.by_ref(); + let mut peekable_in_for_loop = std::iter::empty::().peekable(); for x in peekable_in_for_loop {} } @@ -43,6 +52,18 @@ fn valid() { let passed_along = std::iter::empty::().peekable(); takes_peekable(passed_along); + // Passed to another method + struct PeekableConsumer; + impl PeekableConsumer { + fn consume(&self, _: Peekable>) {} + fn consume_mut_ref(&self, _: &mut Peekable>) {} + } + + let peekable_consumer = PeekableConsumer; + let mut passed_along_to_method = std::iter::empty::().peekable(); + peekable_consumer.consume_mut_ref(&mut passed_along_to_method); + peekable_consumer.consume(passed_along_to_method); + // `peek` called in another block let mut peekable_in_block = std::iter::empty::().peekable(); { @@ -111,6 +132,10 @@ fn valid() { let struct_test = std::iter::empty::().peekable(); PeekableWrapper { f: struct_test }; + // `by_ref` before `peek` + let mut by_ref_test = std::iter::empty::().peekable(); + let peeked_val = by_ref_test.by_ref().peek(); + // `peek` called in another block as the last expression let mut peekable_last_expr = std::iter::empty::().peekable(); { diff --git a/tests/ui/unused_peekable.stderr b/tests/ui/unused_peekable.stderr index bd087f56e4ceb..d557f54179dba 100644 --- a/tests/ui/unused_peekable.stderr +++ b/tests/ui/unused_peekable.stderr @@ -40,12 +40,28 @@ LL | let mut peekable_using_iterator_method = std::iter::empty::().peek = help: consider removing the call to `peekable` error: `peek` never called on `Peekable` iterator - --> $DIR/unused_peekable.rs:35:13 + --> $DIR/unused_peekable.rs:37:9 + | +LL | let passed_along_ref = std::iter::empty::().peekable(); + | ^^^^^^^^^^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:42:9 + | +LL | let _by_ref = by_ref_test.by_ref(); + | ^^^^^^^ + | + = help: consider removing the call to `peekable` + +error: `peek` never called on `Peekable` iterator + --> $DIR/unused_peekable.rs:44:13 | LL | let mut peekable_in_for_loop = std::iter::empty::().peekable(); | ^^^^^^^^^^^^^^^^^^^^ | = help: consider removing the call to `peekable` -error: aborting due to 6 previous errors +error: aborting due to 8 previous errors