diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 5793d59bcad1..369d4b4eed69 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 d4c6c4918cb3..2a0fb30131f7 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 4988ec0eca12..c060d767e234 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 12cab9956219..153457e36716 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 bd087f56e4ce..d557f54179db 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