diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c798a3c2e5a..2e7312227f91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6705,6 +6705,7 @@ Released 2018-09-13 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or [`manual_option_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_as_slice [`manual_pattern_char_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison +[`manual_pop_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_pop_if [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e16b194c0cad..441b907eaf2f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -312,6 +312,7 @@ crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, crate::manual_option_as_slice::MANUAL_OPTION_AS_SLICE_INFO, + crate::manual_pop_if::MANUAL_POP_IF_INFO, crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4dca8dfe94d0..950fcf0db0a4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -211,6 +211,7 @@ mod manual_main_separator_str; mod manual_non_exhaustive; mod manual_option_as_slice; +mod manual_pop_if; mod manual_range_patterns; mod manual_rem_euclid; mod manual_retain; @@ -864,6 +865,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(move |tcx| Box::new(duration_suboptimal_units::DurationSuboptimalUnits::new(tcx, conf))), Box::new(move |_| Box::new(manual_take::ManualTake::new(conf))), Box::new(|_| Box::new(manual_checked_ops::ManualCheckedOps)), + Box::new(move |_| Box::new(manual_pop_if::ManualPopIf::new(conf))), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/manual_pop_if.rs b/clippy_lints/src/manual_pop_if.rs new file mode 100644 index 000000000000..6662a34bc332 --- /dev/null +++ b/clippy_lints/src/manual_pop_if.rs @@ -0,0 +1,434 @@ +use clippy_config::Conf; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::res::MaybeDef; +use clippy_utils::source::snippet_with_context; +use clippy_utils::visitors::is_local_used; +use clippy_utils::{eq_expr_value, is_else_clause, is_lang_item_or_ctor, peel_blocks_with_stmt, sym}; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, LangItem, PatKind, RustcVersion, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::{Span, Symbol}; +use std::fmt; + +declare_clippy_lint! { + /// ### What it does + /// Checks for code to be replaced by `pop_if` methods. + /// + /// ### Why is this bad? + /// Using `pop_if` is more concise and idiomatic. + /// + /// ### Known issues + /// Currently, the lint does not handle the case where the + /// `if` condition is part of an `else if` branch. + /// + /// The lint also does not handle the case where + /// the popped value is assigned and used. + /// + /// ### Examples + /// ```no_run + /// # use std::collections::VecDeque; + /// # let mut vec = vec![1, 2, 3, 4, 5]; + /// # let mut deque: VecDeque = VecDeque::from([1, 2, 3, 4, 5]); + /// if vec.last().is_some_and(|x| *x > 5) { + /// vec.pop().unwrap(); + /// } + /// if deque.back().is_some_and(|x| *x > 5) { + /// deque.pop_back().unwrap(); + /// } + /// if deque.front().is_some_and(|x| *x > 5) { + /// deque.pop_front().unwrap(); + /// } + /// ``` + /// Use instead: + /// ```no_run + /// # use std::collections::VecDeque; + /// # let mut vec = vec![1, 2, 3, 4, 5]; + /// # let mut deque: VecDeque = VecDeque::from([1, 2, 3, 4, 5]); + /// vec.pop_if(|x| *x > 5); + /// deque.pop_back_if(|x| *x > 5); + /// deque.pop_front_if(|x| *x > 5); + /// ``` + #[clippy::version = "1.95.0"] + pub MANUAL_POP_IF, + complexity, + "manual implementation of `pop_if` methods" +} + +impl_lint_pass!(ManualPopIf => [MANUAL_POP_IF]); + +pub struct ManualPopIf { + msrv: Msrv, +} + +impl ManualPopIf { + pub fn new(conf: &'static Conf) -> Self { + Self { msrv: conf.msrv } + } +} + +#[allow(clippy::enum_variant_names)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ManualPopIfKind { + Vec, + VecDequeBack, + VecDequeFront, +} + +impl ManualPopIfKind { + fn check_method(self) -> Symbol { + match self { + ManualPopIfKind::Vec => sym::last, + ManualPopIfKind::VecDequeBack => sym::back, + ManualPopIfKind::VecDequeFront => sym::front, + } + } + + fn pop_method(self) -> Symbol { + match self { + ManualPopIfKind::Vec => sym::pop, + ManualPopIfKind::VecDequeBack => sym::pop_back, + ManualPopIfKind::VecDequeFront => sym::pop_front, + } + } + + fn pop_if_method(self) -> Symbol { + match self { + ManualPopIfKind::Vec => sym::pop_if, + ManualPopIfKind::VecDequeBack => sym::pop_back_if, + ManualPopIfKind::VecDequeFront => sym::pop_front_if, + } + } + + fn is_diag_item(self, cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let ty = cx.typeck_results().expr_ty(expr).peel_refs(); + match self { + ManualPopIfKind::Vec => ty.is_diag_item(cx, sym::Vec), + ManualPopIfKind::VecDequeBack | ManualPopIfKind::VecDequeFront => ty.is_diag_item(cx, sym::VecDeque), + } + } + + fn msrv(self) -> RustcVersion { + match self { + ManualPopIfKind::Vec => msrvs::VEC_POP_IF, + ManualPopIfKind::VecDequeBack => msrvs::VEC_DEQUE_POP_BACK_IF, + ManualPopIfKind::VecDequeFront => msrvs::VEC_DEQUE_POP_FRONT_IF, + } + } +} + +impl fmt::Display for ManualPopIfKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ManualPopIfKind::Vec => write!(f, "`Vec::pop_if`"), + ManualPopIfKind::VecDequeBack => write!(f, "`VecDeque::pop_back_if`"), + ManualPopIfKind::VecDequeFront => write!(f, "`VecDeque::pop_front_if`"), + } + } +} + +struct ManualPopIfPattern<'tcx> { + kind: ManualPopIfKind, + + /// The collection (`vec` in `vec.last()`) + collection_expr: &'tcx Expr<'tcx>, + + /// The closure (`*x > 5` in `|x| *x > 5`) + predicate: &'tcx Expr<'tcx>, + + /// Parameter name for the closure (`x` in `|x| *x > 5`) + param_name: Symbol, + + /// Span of the if expression (including the `if` keyword) + if_span: Span, +} + +impl ManualPopIfPattern<'_> { + fn emit_lint(&self, cx: &LateContext<'_>) { + let mut app = Applicability::MachineApplicable; + let ctxt = self.if_span.ctxt(); + let collection_snippet = snippet_with_context(cx, self.collection_expr.span, ctxt, "..", &mut app).0; + let predicate_snippet = snippet_with_context(cx, self.predicate.span, ctxt, "..", &mut app).0; + let param_name = self.param_name; + let pop_if_method = self.kind.pop_if_method(); + + let suggestion = format!("{collection_snippet}.{pop_if_method}(|{param_name}| {predicate_snippet});"); + + span_lint_and_sugg( + cx, + MANUAL_POP_IF, + self.if_span, + format!("manual implementation of {}", self.kind), + "try", + suggestion, + app, + ); + } +} + +fn pop_value_is_used(then_block: &Expr<'_>) -> bool { + let ExprKind::Block(block, _) = then_block.kind else { + return true; + }; + + if block.expr.is_some() { + return true; + } + + match block.stmts { + [stmt] => !matches!(stmt.kind, StmtKind::Semi(_) | StmtKind::Item(_)), + [.., last] => matches!(last.kind, StmtKind::Expr(_)), + [] => false, + } +} + +/// Checks for the pattern: +/// ```ignore +/// if vec.last().is_some_and(|x| *x > 5) { +/// vec.pop().unwrap(); +/// } +/// ``` +fn check_is_some_and_pattern<'tcx>( + cx: &LateContext<'tcx>, + cond: &'tcx Expr<'_>, + then_block: &'tcx Expr<'_>, + if_expr_span: Span, + kind: ManualPopIfKind, +) -> Option> { + if pop_value_is_used(then_block) { + return None; + } + + let check_method = kind.check_method(); + let pop_method = kind.pop_method(); + + if let ExprKind::MethodCall(path, receiver, [closure_arg], _) = cond.kind + && path.ident.name == sym::is_some_and + && let ExprKind::MethodCall(check_path, collection_expr, [], _) = receiver.kind + && check_path.ident.name == check_method + && kind.is_diag_item(cx, collection_expr) + && let ExprKind::Closure(closure) = closure_arg.kind + && let body = cx.tcx.hir_body(closure.body) + && let Some((pop_collection, _pop_span)) = check_pop_unwrap(then_block, pop_method) + && eq_expr_value(cx, collection_expr, pop_collection) + && let Some(param) = body.params.first() + && let Some(ident) = param.pat.simple_ident() + { + return Some(ManualPopIfPattern { + kind, + collection_expr, + predicate: body.value, + param_name: ident.name, + if_span: if_expr_span, + }); + } + + None +} + +/// Checks for the pattern: +/// ```ignore +/// if let Some(x) = vec.last() { +/// if *x > 5 { +/// vec.pop().unwrap(); +/// } +/// } +/// ``` +fn check_if_let_pattern<'tcx>( + cx: &LateContext<'tcx>, + cond: &'tcx Expr<'_>, + then_block: &'tcx Expr<'_>, + if_expr_span: Span, + kind: ManualPopIfKind, +) -> Option> { + let check_method = kind.check_method(); + let pop_method = kind.pop_method(); + + if let ExprKind::Let(let_expr) = cond.kind + && let PatKind::TupleStruct(qpath, [binding_pat], _) = let_expr.pat.kind + { + let res = cx.qpath_res(&qpath, let_expr.pat.hir_id); + + if let Some(def_id) = res.opt_def_id() + && is_lang_item_or_ctor(cx, def_id, LangItem::OptionSome) + && let PatKind::Binding(_, binding_id, binding_name, _) = binding_pat.kind + && let ExprKind::MethodCall(path, collection_expr, [], _) = let_expr.init.kind + && path.ident.name == check_method + && kind.is_diag_item(cx, collection_expr) + && let ExprKind::Block(block, _) = then_block.kind + { + // The inner if can be either a statement or a block expression + let inner_if = match (block.stmts, block.expr) { + ([stmt], _) => match stmt.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => expr, + _ => return None, + }, + ([], Some(expr)) => expr, + _ => return None, + }; + + if let ExprKind::If(inner_cond, inner_then, None) = inner_if.kind + && is_local_used(cx, inner_cond, binding_id) + && !pop_value_is_used(inner_then) + && let Some((pop_collection, _pop_span)) = check_pop_unwrap(inner_then, pop_method) + && eq_expr_value(cx, collection_expr, pop_collection) + { + return Some(ManualPopIfPattern { + kind, + collection_expr, + predicate: inner_cond, + param_name: binding_name.name, + if_span: if_expr_span, + }); + } + } + } + + None +} + +/// Checks for the pattern: +/// ```ignore +/// if let Some(x) = vec.last() && *x > 5 { +/// vec.pop().unwrap(); +/// } +/// ``` +fn check_let_chain_pattern<'tcx>( + cx: &LateContext<'tcx>, + cond: &'tcx Expr<'_>, + then_block: &'tcx Expr<'_>, + if_expr_span: Span, + kind: ManualPopIfKind, +) -> Option> { + if pop_value_is_used(then_block) { + return None; + } + + let check_method = kind.check_method(); + let pop_method = kind.pop_method(); + + if let ExprKind::Binary(op, left, right) = cond.kind + && op.node == rustc_ast::BinOpKind::And + && let ExprKind::Let(let_expr) = left.kind + && let PatKind::TupleStruct(qpath, [binding_pat], _) = let_expr.pat.kind + { + let res = cx.qpath_res(&qpath, let_expr.pat.hir_id); + + if let Some(def_id) = res.opt_def_id() + && is_lang_item_or_ctor(cx, def_id, LangItem::OptionSome) + && let PatKind::Binding(_, binding_id, binding_name, _) = binding_pat.kind + && let ExprKind::MethodCall(path, collection_expr, [], _) = let_expr.init.kind + && path.ident.name == check_method + && kind.is_diag_item(cx, collection_expr) + && is_local_used(cx, right, binding_id) + && !pop_value_is_used(then_block) + && let Some((pop_collection, _pop_span)) = check_pop_unwrap(then_block, pop_method) + && eq_expr_value(cx, collection_expr, pop_collection) + { + return Some(ManualPopIfPattern { + kind, + collection_expr, + predicate: right, + param_name: binding_name.name, + if_span: if_expr_span, + }); + } + } + + None +} + +/// Checks for the pattern: +/// ```ignore +/// if vec.last().map(|x| *x > 5).unwrap_or(false) { +/// vec.pop().unwrap(); +/// } +/// ``` +fn check_map_unwrap_or_pattern<'tcx>( + cx: &LateContext<'tcx>, + cond: &'tcx Expr<'_>, + then_block: &'tcx Expr<'_>, + if_expr_span: Span, + kind: ManualPopIfKind, +) -> Option> { + if pop_value_is_used(then_block) { + return None; + } + + let check_method = kind.check_method(); + let pop_method = kind.pop_method(); + + if let ExprKind::MethodCall(unwrap_path, receiver, [default_arg], _) = cond.kind + && unwrap_path.ident.name == sym::unwrap_or + && matches!(default_arg.kind, ExprKind::Lit(lit) if matches!(lit.node, LitKind::Bool(false))) + && let ExprKind::MethodCall(map_path, map_receiver, [closure_arg], _) = receiver.kind + && map_path.ident.name == sym::map + && let ExprKind::MethodCall(check_path, collection_expr, [], _) = map_receiver.kind + && check_path.ident.name == check_method + && kind.is_diag_item(cx, collection_expr) + && let ExprKind::Closure(closure) = closure_arg.kind + && let body = cx.tcx.hir_body(closure.body) + && cx.typeck_results().expr_ty(body.value).is_bool() + && let Some((pop_collection, _pop_span)) = check_pop_unwrap(then_block, pop_method) + && eq_expr_value(cx, collection_expr, pop_collection) + && let Some(param) = body.params.first() + && let Some(ident) = param.pat.simple_ident() + { + return Some(ManualPopIfPattern { + kind, + collection_expr, + predicate: body.value, + param_name: ident.name, + if_span: if_expr_span, + }); + } + + None +} + +/// Checks for `collection.().unwrap()` or `collection.().expect(..)` +/// and returns the collection and the span of the peeled expr +fn check_pop_unwrap<'tcx>(expr: &'tcx Expr<'_>, pop_method: Symbol) -> Option<(&'tcx Expr<'tcx>, Span)> { + let inner_expr = peel_blocks_with_stmt(expr); + + if let ExprKind::MethodCall(unwrap_path, receiver, _, _) = inner_expr.kind + && matches!(unwrap_path.ident.name, sym::unwrap | sym::expect) + && let ExprKind::MethodCall(pop_path, collection_expr, [], _) = receiver.kind + && pop_path.ident.name == pop_method + { + return Some((collection_expr, inner_expr.span)); + } + + None +} + +impl<'tcx> LateLintPass<'tcx> for ManualPopIf { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + let ExprKind::If(cond, then_block, None) = expr.kind else { + return; + }; + + // Do not lint if we are in an else-if branch. + if is_else_clause(cx.tcx, expr) { + return; + } + + for kind in [ + ManualPopIfKind::Vec, + ManualPopIfKind::VecDequeBack, + ManualPopIfKind::VecDequeFront, + ] { + if let Some(pattern) = check_is_some_and_pattern(cx, cond, then_block, expr.span, kind) + .or_else(|| check_if_let_pattern(cx, cond, then_block, expr.span, kind)) + .or_else(|| check_let_chain_pattern(cx, cond, then_block, expr.span, kind)) + .or_else(|| check_map_unwrap_or_pattern(cx, cond, then_block, expr.span, kind)) + && self.msrv.meets(cx, kind.msrv()) + { + pattern.emit_lint(cx); + return; + } + } + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 5aa457b9d19c..ecadc3322e14 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -23,9 +23,11 @@ macro_rules! msrv_aliases { // names may refer to stabilized feature flags or library items msrv_aliases! { + 1,93,0 { VEC_DEQUE_POP_BACK_IF, VEC_DEQUE_POP_FRONT_IF } 1,91,0 { DURATION_FROM_MINUTES_HOURS } 1,88,0 { LET_CHAINS } 1,87,0 { OS_STR_DISPLAY, INT_MIDPOINT, CONST_CHAR_IS_DIGIT, UNSIGNED_IS_MULTIPLE_OF, INTEGER_SIGN_CAST } + 1,86,0 { VEC_POP_IF } 1,85,0 { UINT_FLOAT_MIDPOINT, CONST_SIZE_OF_VAL } 1,84,0 { CONST_OPTION_AS_SLICE, MANUAL_DANGLING_PTR } 1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY, CONST_MUT_REFS, CONST_UNWRAP } diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index d6ba678434bc..bb67e4350d2b 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -140,6 +140,7 @@ macro_rules! generate { as_str, assert_failed, author, + back, binaryheap_iter, bool_then, borrow, @@ -295,6 +296,7 @@ macro_rules! generate { from_str_method, from_str_radix, from_weeks, + front, fs, fs_create_dir, fuse, @@ -464,6 +466,12 @@ macro_rules! generate { peekable, permissions_from_mode, pin_macro, + pop, + pop_back, + pop_back_if, + pop_front, + pop_front_if, + pop_if, position, pow, powf, diff --git a/tests/ui/manual_pop_if.fixed b/tests/ui/manual_pop_if.fixed new file mode 100644 index 000000000000..6e30786c8097 --- /dev/null +++ b/tests/ui/manual_pop_if.fixed @@ -0,0 +1,247 @@ +#![warn(clippy::manual_pop_if)] +#![allow(clippy::collapsible_if, clippy::redundant_closure)] + +use std::collections::VecDeque; +use std::marker::PhantomData; + +// FakeVec has the same methods as Vec but isn't actually a Vec +struct FakeVec(PhantomData); + +impl FakeVec { + fn last(&self) -> Option<&T> { + None + } + + fn pop(&mut self) -> Option { + None + } +} + +fn is_some_and_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + vec.pop_if(|x| *x > 2); + + vec.pop_if(|x| *x > 2); + + deque.pop_back_if(|x| *x > 2); + + deque.pop_front_if(|x| *x > 2); +} + +fn is_some_and_pattern_negative(mut vec: Vec, mut deque: VecDeque) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if vec.last().is_some_and(|x| *x > 2) { + vec2.pop().unwrap(); + } + + // Do not lint, non-Vec type + let mut fake_vec: FakeVec = FakeVec(PhantomData); + if fake_vec.last().is_some_and(|x| *x > 2) { + fake_vec.pop().unwrap(); + } + + // Do not lint, else-if branch + if false { + // something + } else if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if vec.last().is_some_and(|x| *x > 2) { + let _value = vec.pop().unwrap(); + println!("Popped: {}", _value); + } + + // Do not lint, value used in expression + if vec.last().is_some_and(|x| *x > 2) { + println!("Popped: {}", vec.pop().unwrap()); + } + + // Do not lint, else block + let _result = if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap() + } else { + 0 + }; +} + +fn if_let_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + vec.pop_if(|x| *x > 2); + + vec.pop_if(|x| *x > 2); + + deque.pop_back_if(|x| *x > 2); + + deque.pop_front_if(|x| *x > 2); +} + +fn if_let_pattern_negative(mut vec: Vec) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if let Some(x) = vec.last() { + if *x > 2 { + vec2.pop().unwrap(); + } + } + + // Do not lint, intervening statements + if let Some(x) = vec.last() { + println!("Checking {}", x); + if *x > 2 { + vec.pop().unwrap(); + } + } + + // Do not lint, bound variable not used in condition + if let Some(_x) = vec.last() { + if vec.len() > 2 { + vec.pop().unwrap(); + } + } + + // Do not lint, value used in let binding + if let Some(x) = vec.last() { + if *x > 2 { + let _val = vec.pop().unwrap(); + } + } + + // Do not lint, else block + let _result = if let Some(x) = vec.last() { + if *x > 2 { vec.pop().unwrap() } else { 0 } + } else { + 0 + }; +} + +fn let_chain_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + vec.pop_if(|x| *x > 2); + + vec.pop_if(|x| *x > 2); + + deque.pop_back_if(|x| *x > 2); + + deque.pop_front_if(|x| *x > 2); +} + +fn let_chain_pattern_negative(mut vec: Vec) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if let Some(x) = vec.last() + && *x > 2 + { + vec2.pop().unwrap(); + } + + // Do not lint, bound variable not used in condition + if let Some(_x) = vec.last() + && vec.len() > 2 + { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if let Some(x) = vec.last() + && *x > 2 + { + let _val = vec.pop().unwrap(); + } + + // Do not lint, value used in expression + if let Some(x) = vec.last() + && *x > 2 + { + println!("Popped: {}", vec.pop().unwrap()); + } + + // Do not lint, else block + let _result = if let Some(x) = vec.last() + && *x > 2 + { + vec.pop().unwrap() + } else { + 0 + }; +} + +fn map_unwrap_or_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + vec.pop_if(|x| *x > 2); + + vec.pop_if(|x| *x > 2); + + deque.pop_back_if(|x| *x > 2); + + deque.pop_front_if(|x| *x > 2); +} + +fn map_unwrap_or_pattern_negative(mut vec: Vec) { + // Do not lint, unwrap_or(true) instead of false + if vec.last().map(|x| *x > 2).unwrap_or(true) { + vec.pop().unwrap(); + } + + // Do not lint, different vectors + let mut vec2 = vec![0]; + if vec.last().map(|x| *x > 2).unwrap_or(false) { + vec2.pop().unwrap(); + } + + // Do not lint, non-Vec type + let mut fake_vec: FakeVec = FakeVec(PhantomData); + if fake_vec.last().map(|x| *x > 2).unwrap_or(false) { + fake_vec.pop().unwrap(); + } + + // Do not lint, map returns non-boolean + if vec.last().map(|x| x + 1).unwrap_or(0) > 2 { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if vec.last().map(|x| *x > 2).unwrap_or(false) { + let _val = vec.pop().unwrap(); + } + + // Do not lint, else block + let _result = if vec.last().map(|x| *x > 2).unwrap_or(false) { + vec.pop().unwrap() + } else { + 0 + }; +} + +// this makes sure we do not expand vec![] in the suggestion +fn handle_macro_in_closure(mut vec: Vec>) { + vec.pop_if(|e| *e == vec![1]); +} + +#[clippy::msrv = "1.85.0"] +fn msrv_too_low_vec(mut vec: Vec) { + if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap(); + } +} + +#[clippy::msrv = "1.92.0"] +fn msrv_too_low_vecdeque(mut deque: VecDeque) { + if deque.back().is_some_and(|x| *x > 2) { + deque.pop_back().unwrap(); + } + + if deque.front().is_some_and(|x| *x > 2) { + deque.pop_front().unwrap(); + } +} + +#[clippy::msrv = "1.86.0"] +fn msrv_high_enough_vec(mut vec: Vec) { + vec.pop_if(|x| *x > 2); +} + +#[clippy::msrv = "1.93.0"] +fn msrv_high_enough_vecdeque(mut deque: VecDeque) { + deque.pop_back_if(|x| *x > 2); + + deque.pop_front_if(|x| *x > 2); +} diff --git a/tests/ui/manual_pop_if.rs b/tests/ui/manual_pop_if.rs new file mode 100644 index 000000000000..a2f679dfc6b3 --- /dev/null +++ b/tests/ui/manual_pop_if.rs @@ -0,0 +1,319 @@ +#![warn(clippy::manual_pop_if)] +#![allow(clippy::collapsible_if, clippy::redundant_closure)] + +use std::collections::VecDeque; +use std::marker::PhantomData; + +// FakeVec has the same methods as Vec but isn't actually a Vec +struct FakeVec(PhantomData); + +impl FakeVec { + fn last(&self) -> Option<&T> { + None + } + + fn pop(&mut self) -> Option { + None + } +} + +fn is_some_and_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + if vec.last().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + vec.pop().unwrap(); + } + + if vec.last().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + vec.pop().expect("element"); + } + + if deque.back().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + deque.pop_back().unwrap(); + } + + if deque.front().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + deque.pop_front().unwrap(); + } +} + +fn is_some_and_pattern_negative(mut vec: Vec, mut deque: VecDeque) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if vec.last().is_some_and(|x| *x > 2) { + vec2.pop().unwrap(); + } + + // Do not lint, non-Vec type + let mut fake_vec: FakeVec = FakeVec(PhantomData); + if fake_vec.last().is_some_and(|x| *x > 2) { + fake_vec.pop().unwrap(); + } + + // Do not lint, else-if branch + if false { + // something + } else if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if vec.last().is_some_and(|x| *x > 2) { + let _value = vec.pop().unwrap(); + println!("Popped: {}", _value); + } + + // Do not lint, value used in expression + if vec.last().is_some_and(|x| *x > 2) { + println!("Popped: {}", vec.pop().unwrap()); + } + + // Do not lint, else block + let _result = if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap() + } else { + 0 + }; +} + +fn if_let_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + if let Some(x) = vec.last() { + //~^ manual_pop_if + if *x > 2 { + vec.pop().unwrap(); + } + } + + if let Some(x) = vec.last() { + //~^ manual_pop_if + if *x > 2 { + vec.pop().expect("element"); + } + } + + if let Some(x) = deque.back() { + //~^ manual_pop_if + if *x > 2 { + deque.pop_back().unwrap(); + } + } + + if let Some(x) = deque.front() { + //~^ manual_pop_if + if *x > 2 { + deque.pop_front().unwrap(); + } + } +} + +fn if_let_pattern_negative(mut vec: Vec) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if let Some(x) = vec.last() { + if *x > 2 { + vec2.pop().unwrap(); + } + } + + // Do not lint, intervening statements + if let Some(x) = vec.last() { + println!("Checking {}", x); + if *x > 2 { + vec.pop().unwrap(); + } + } + + // Do not lint, bound variable not used in condition + if let Some(_x) = vec.last() { + if vec.len() > 2 { + vec.pop().unwrap(); + } + } + + // Do not lint, value used in let binding + if let Some(x) = vec.last() { + if *x > 2 { + let _val = vec.pop().unwrap(); + } + } + + // Do not lint, else block + let _result = if let Some(x) = vec.last() { + if *x > 2 { vec.pop().unwrap() } else { 0 } + } else { + 0 + }; +} + +fn let_chain_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + if let Some(x) = vec.last() //~ manual_pop_if + && *x > 2 + { + vec.pop().unwrap(); + } + + if let Some(x) = vec.last() //~ manual_pop_if + && *x > 2 + { + vec.pop().expect("element"); + } + + if let Some(x) = deque.back() //~ manual_pop_if + && *x > 2 + { + deque.pop_back().unwrap(); + } + + if let Some(x) = deque.front() //~ manual_pop_if + && *x > 2 + { + deque.pop_front().unwrap(); + } +} + +fn let_chain_pattern_negative(mut vec: Vec) { + // Do not lint, different vectors + let mut vec2 = vec![0]; + if let Some(x) = vec.last() + && *x > 2 + { + vec2.pop().unwrap(); + } + + // Do not lint, bound variable not used in condition + if let Some(_x) = vec.last() + && vec.len() > 2 + { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if let Some(x) = vec.last() + && *x > 2 + { + let _val = vec.pop().unwrap(); + } + + // Do not lint, value used in expression + if let Some(x) = vec.last() + && *x > 2 + { + println!("Popped: {}", vec.pop().unwrap()); + } + + // Do not lint, else block + let _result = if let Some(x) = vec.last() + && *x > 2 + { + vec.pop().unwrap() + } else { + 0 + }; +} + +fn map_unwrap_or_pattern_positive(mut vec: Vec, mut deque: VecDeque) { + if vec.last().map(|x| *x > 2).unwrap_or(false) { + //~^ manual_pop_if + vec.pop().unwrap(); + } + + if vec.last().map(|x| *x > 2).unwrap_or(false) { + //~^ manual_pop_if + vec.pop().expect("element"); + } + + if deque.back().map(|x| *x > 2).unwrap_or(false) { + //~^ manual_pop_if + deque.pop_back().unwrap(); + } + + if deque.front().map(|x| *x > 2).unwrap_or(false) { + //~^ manual_pop_if + deque.pop_front().unwrap(); + } +} + +fn map_unwrap_or_pattern_negative(mut vec: Vec) { + // Do not lint, unwrap_or(true) instead of false + if vec.last().map(|x| *x > 2).unwrap_or(true) { + vec.pop().unwrap(); + } + + // Do not lint, different vectors + let mut vec2 = vec![0]; + if vec.last().map(|x| *x > 2).unwrap_or(false) { + vec2.pop().unwrap(); + } + + // Do not lint, non-Vec type + let mut fake_vec: FakeVec = FakeVec(PhantomData); + if fake_vec.last().map(|x| *x > 2).unwrap_or(false) { + fake_vec.pop().unwrap(); + } + + // Do not lint, map returns non-boolean + if vec.last().map(|x| x + 1).unwrap_or(0) > 2 { + vec.pop().unwrap(); + } + + // Do not lint, value used in let binding + if vec.last().map(|x| *x > 2).unwrap_or(false) { + let _val = vec.pop().unwrap(); + } + + // Do not lint, else block + let _result = if vec.last().map(|x| *x > 2).unwrap_or(false) { + vec.pop().unwrap() + } else { + 0 + }; +} + +// this makes sure we do not expand vec![] in the suggestion +fn handle_macro_in_closure(mut vec: Vec>) { + if vec.last().is_some_and(|e| *e == vec![1]) { + //~^ manual_pop_if + vec.pop().unwrap(); + } +} + +#[clippy::msrv = "1.85.0"] +fn msrv_too_low_vec(mut vec: Vec) { + if vec.last().is_some_and(|x| *x > 2) { + vec.pop().unwrap(); + } +} + +#[clippy::msrv = "1.92.0"] +fn msrv_too_low_vecdeque(mut deque: VecDeque) { + if deque.back().is_some_and(|x| *x > 2) { + deque.pop_back().unwrap(); + } + + if deque.front().is_some_and(|x| *x > 2) { + deque.pop_front().unwrap(); + } +} + +#[clippy::msrv = "1.86.0"] +fn msrv_high_enough_vec(mut vec: Vec) { + if vec.last().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + vec.pop().unwrap(); + } +} + +#[clippy::msrv = "1.93.0"] +fn msrv_high_enough_vecdeque(mut deque: VecDeque) { + if deque.back().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + deque.pop_back().unwrap(); + } + + if deque.front().is_some_and(|x| *x > 2) { + //~^ manual_pop_if + deque.pop_front().unwrap(); + } +} diff --git a/tests/ui/manual_pop_if.stderr b/tests/ui/manual_pop_if.stderr new file mode 100644 index 000000000000..d57e20dc6205 --- /dev/null +++ b/tests/ui/manual_pop_if.stderr @@ -0,0 +1,197 @@ +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:21:5 + | +LL | / if vec.last().is_some_and(|x| *x > 2) { +LL | | +LL | | vec.pop().unwrap(); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + | + = note: `-D clippy::manual-pop-if` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_pop_if)]` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:26:5 + | +LL | / if vec.last().is_some_and(|x| *x > 2) { +LL | | +LL | | vec.pop().expect("element"); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_back_if` + --> tests/ui/manual_pop_if.rs:31:5 + | +LL | / if deque.back().is_some_and(|x| *x > 2) { +LL | | +LL | | deque.pop_back().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_back_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_front_if` + --> tests/ui/manual_pop_if.rs:36:5 + | +LL | / if deque.front().is_some_and(|x| *x > 2) { +LL | | +LL | | deque.pop_front().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_front_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:82:5 + | +LL | / if let Some(x) = vec.last() { +LL | | +LL | | if *x > 2 { +LL | | vec.pop().unwrap(); +LL | | } +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:89:5 + | +LL | / if let Some(x) = vec.last() { +LL | | +LL | | if *x > 2 { +LL | | vec.pop().expect("element"); +LL | | } +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_back_if` + --> tests/ui/manual_pop_if.rs:96:5 + | +LL | / if let Some(x) = deque.back() { +LL | | +LL | | if *x > 2 { +LL | | deque.pop_back().unwrap(); +LL | | } +LL | | } + | |_____^ help: try: `deque.pop_back_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_front_if` + --> tests/ui/manual_pop_if.rs:103:5 + | +LL | / if let Some(x) = deque.front() { +LL | | +LL | | if *x > 2 { +LL | | deque.pop_front().unwrap(); +LL | | } +LL | | } + | |_____^ help: try: `deque.pop_front_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:151:5 + | +LL | / if let Some(x) = vec.last() +LL | | && *x > 2 +LL | | { +LL | | vec.pop().unwrap(); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:157:5 + | +LL | / if let Some(x) = vec.last() +LL | | && *x > 2 +LL | | { +LL | | vec.pop().expect("element"); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_back_if` + --> tests/ui/manual_pop_if.rs:163:5 + | +LL | / if let Some(x) = deque.back() +LL | | && *x > 2 +LL | | { +LL | | deque.pop_back().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_back_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_front_if` + --> tests/ui/manual_pop_if.rs:169:5 + | +LL | / if let Some(x) = deque.front() +LL | | && *x > 2 +LL | | { +LL | | deque.pop_front().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_front_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:217:5 + | +LL | / if vec.last().map(|x| *x > 2).unwrap_or(false) { +LL | | +LL | | vec.pop().unwrap(); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:222:5 + | +LL | / if vec.last().map(|x| *x > 2).unwrap_or(false) { +LL | | +LL | | vec.pop().expect("element"); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_back_if` + --> tests/ui/manual_pop_if.rs:227:5 + | +LL | / if deque.back().map(|x| *x > 2).unwrap_or(false) { +LL | | +LL | | deque.pop_back().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_back_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_front_if` + --> tests/ui/manual_pop_if.rs:232:5 + | +LL | / if deque.front().map(|x| *x > 2).unwrap_or(false) { +LL | | +LL | | deque.pop_front().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_front_if(|x| *x > 2);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:276:5 + | +LL | / if vec.last().is_some_and(|e| *e == vec![1]) { +LL | | +LL | | vec.pop().unwrap(); +LL | | } + | |_____^ help: try: `vec.pop_if(|e| *e == vec![1]);` + +error: manual implementation of `Vec::pop_if` + --> tests/ui/manual_pop_if.rs:302:5 + | +LL | / if vec.last().is_some_and(|x| *x > 2) { +LL | | +LL | | vec.pop().unwrap(); +LL | | } + | |_____^ help: try: `vec.pop_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_back_if` + --> tests/ui/manual_pop_if.rs:310:5 + | +LL | / if deque.back().is_some_and(|x| *x > 2) { +LL | | +LL | | deque.pop_back().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_back_if(|x| *x > 2);` + +error: manual implementation of `VecDeque::pop_front_if` + --> tests/ui/manual_pop_if.rs:315:5 + | +LL | / if deque.front().is_some_and(|x| *x > 2) { +LL | | +LL | | deque.pop_front().unwrap(); +LL | | } + | |_____^ help: try: `deque.pop_front_if(|x| *x > 2);` + +error: aborting due to 20 previous errors +