Skip to content

manual_pop_if: lint more cases, even if we do not provide a suggestion #16683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pbor wants to merge 1 commit into
base: master
Choose a base branch
from
+266 −172
Open

manual_pop_if: lint more cases, even if we do not provide a suggestion #16683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 67 additions & 68 deletions clippy_lints/src/manual_pop_if.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
use clippy_config::Conf;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_and_then;
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 clippy_utils::visitors::{for_each_expr_without_closures, is_local_used};
use clippy_utils::{eq_expr_value, is_else_clause, is_lang_item_or_ctor, 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;
use std::ops::ControlFlow;

declare_clippy_lint! {
/// ### What it does
Expand All @@ -21,11 +22,9 @@ declare_clippy_lint! {
/// 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.
/// When the popped value is assigned or used in an expression,
/// or when the `if` condition is part of an `else if` branch, the
/// lint will trigger but will not provide an automatic suggestion.
///
/// ### Examples
/// ```no_run
Expand Down Expand Up @@ -78,7 +77,7 @@ enum ManualPopIfKind {
}

impl ManualPopIfKind {
fn check_method(self) -> Symbol {
fn peek_method(self) -> Symbol {
match self {
ManualPopIfKind::Vec => sym::last,
ManualPopIfKind::VecDequeBack => sym::back,
Expand Down Expand Up @@ -143,6 +142,9 @@ struct ManualPopIfPattern<'tcx> {

/// Span of the if expression (including the `if` keyword)
if_span: Span,

/// Whether we are able to provide a suggestion
suggestable: bool,
}

impl ManualPopIfPattern<'_> {
Expand All @@ -154,36 +156,23 @@ impl ManualPopIfPattern<'_> {
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(
span_lint_and_then(
cx,
MANUAL_POP_IF,
self.if_span,
format!("manual implementation of {}", self.kind),
"try",
suggestion,
app,
|diag| {
let sugg = format!("{collection_snippet}.{pop_if_method}(|{param_name}| {predicate_snippet});");
if self.suggestable {
diag.span_suggestion(self.if_span, "try", sugg, app);
} else {
diag.help(format!("try using `{sugg}`"));
}
},
);
}
}

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) {
Expand All @@ -197,21 +186,17 @@ fn check_is_some_and_pattern<'tcx>(
if_expr_span: Span,
kind: ManualPopIfKind,
) -> Option<ManualPopIfPattern<'tcx>> {
if pop_value_is_used(then_block) {
return None;
}

let check_method = kind.check_method();
let peek_method = kind.peek_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
&& check_path.ident.name == peek_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)
&& let Some((pop_collection, suggestable)) = 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()
Expand All @@ -222,6 +207,7 @@ fn check_is_some_and_pattern<'tcx>(
predicate: body.value,
param_name: ident.name,
if_span: if_expr_span,
suggestable,
});
}

Expand All @@ -243,7 +229,7 @@ fn check_if_let_pattern<'tcx>(
if_expr_span: Span,
kind: ManualPopIfKind,
) -> Option<ManualPopIfPattern<'tcx>> {
let check_method = kind.check_method();
let peek_method = kind.peek_method();
let pop_method = kind.pop_method();

if let ExprKind::Let(let_expr) = cond.kind
Expand All @@ -255,7 +241,7 @@ fn check_if_let_pattern<'tcx>(
&& 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
&& path.ident.name == peek_method
&& kind.is_diag_item(cx, collection_expr)
&& let ExprKind::Block(block, _) = then_block.kind
{
Expand All @@ -271,8 +257,7 @@ fn check_if_let_pattern<'tcx>(

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)
&& let Some((pop_collection, suggestable)) = check_pop_unwrap(inner_then, pop_method)
&& eq_expr_value(cx, collection_expr, pop_collection)
{
return Some(ManualPopIfPattern {
Expand All @@ -281,6 +266,7 @@ fn check_if_let_pattern<'tcx>(
predicate: inner_cond,
param_name: binding_name.name,
if_span: if_expr_span,
suggestable,
});
}
}
Expand All @@ -302,11 +288,7 @@ fn check_let_chain_pattern<'tcx>(
if_expr_span: Span,
kind: ManualPopIfKind,
) -> Option<ManualPopIfPattern<'tcx>> {
if pop_value_is_used(then_block) {
return None;
}

let check_method = kind.check_method();
let peek_method = kind.peek_method();
let pop_method = kind.pop_method();

if let ExprKind::Binary(op, left, right) = cond.kind
Expand All @@ -320,11 +302,10 @@ fn check_let_chain_pattern<'tcx>(
&& 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
&& path.ident.name == peek_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)
&& let Some((pop_collection, suggestable)) = check_pop_unwrap(then_block, pop_method)
&& eq_expr_value(cx, collection_expr, pop_collection)
{
return Some(ManualPopIfPattern {
Expand All @@ -333,6 +314,7 @@ fn check_let_chain_pattern<'tcx>(
predicate: right,
param_name: binding_name.name,
if_span: if_expr_span,
suggestable,
});
}
}
Expand All @@ -353,11 +335,7 @@ fn check_map_unwrap_or_pattern<'tcx>(
if_expr_span: Span,
kind: ManualPopIfKind,
) -> Option<ManualPopIfPattern<'tcx>> {
if pop_value_is_used(then_block) {
return None;
}

let check_method = kind.check_method();
let peek_method = kind.peek_method();
let pop_method = kind.pop_method();

if let ExprKind::MethodCall(unwrap_path, receiver, [default_arg], _) = cond.kind
Expand All @@ -366,12 +344,12 @@ fn check_map_unwrap_or_pattern<'tcx>(
&& 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
&& check_path.ident.name == peek_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)
&& let Some((pop_collection, suggestable)) = 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()
Expand All @@ -382,26 +360,46 @@ fn check_map_unwrap_or_pattern<'tcx>(
predicate: body.value,
param_name: ident.name,
if_span: if_expr_span,
suggestable,
});
}

None
}

/// Checks for `collection.<pop_method>().unwrap()` or `collection.<pop_method>().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);
/// and returns the expression. Also returns a boolean to indicate whether we are able
// to provide an automatic suggestion on how to use `pop_if`.
fn check_pop_unwrap<'tcx>(expr: &'tcx Expr<'_>, pop_method: Symbol) -> Option<(&'tcx Expr<'tcx>, bool)> {
let ExprKind::Block(block, _) = expr.kind else {
return None;
};

if let ExprKind::MethodCall(unwrap_path, receiver, _, _) = inner_expr.kind
// Check for single statement with the pop unwrap (not in a macro or other expression)
if let [stmt] = block.stmts
&& block.expr.is_none()
&& let StmtKind::Semi(stmt_expr) | StmtKind::Expr(stmt_expr) = &stmt.kind
&& let ExprKind::MethodCall(unwrap_path, receiver, _, _) = stmt_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
&& !stmt_expr.span.from_expansion()
{
return Some((collection_expr, inner_expr.span));
return Some((collection_expr, true));
}

None
// Check if the pop unwrap is present at all
for_each_expr_without_closures(block, |expr| {
if let ExprKind::MethodCall(unwrap_path, receiver, _, _) = 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
{
ControlFlow::Break((collection_expr, false))
} else {
ControlFlow::Continue(())
}
})
}

impl<'tcx> LateLintPass<'tcx> for ManualPopIf {
Expand All @@ -410,22 +408,23 @@ impl<'tcx> LateLintPass<'tcx> for ManualPopIf {
return;
};

// Do not lint if we are in an else-if branch.
if is_else_clause(cx.tcx, expr) {
return;
}
let in_else_clause = is_else_clause(cx.tcx, expr);

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)
if let Some(mut 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())
{
if in_else_clause {
pattern.suggestable = false;
}

pattern.emit_lint(cx);
return;
}
Expand Down
44 changes: 0 additions & 44 deletions tests/ui/manual_pop_if.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,6 @@ fn is_some_and_pattern_negative(mut vec: Vec<i32>, mut deque: VecDeque<i32>) {
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()
Expand Down Expand Up @@ -100,13 +82,6 @@ fn if_let_pattern_negative(mut vec: Vec<i32>) {
}
}

// 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 }
Expand Down Expand Up @@ -141,20 +116,6 @@ fn let_chain_pattern_negative(mut vec: Vec<i32>) {
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
Expand Down Expand Up @@ -198,11 +159,6 @@ fn map_unwrap_or_pattern_negative(mut vec: Vec<i32>) {
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()
Expand Down
Loading