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

[pbor]

View all comments

Extend the lint to detect the case where the popped value is used, but in such cases just emit the lint with no suggestion.

changelog: [manual_pop_if]: in case the popped value is used, just emit the lint with no suggestion.
changelog: [manual_pop_if]: also cover .pop().unwrap_unchecked()

[rustbot]

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[pbor]

r? @ada4a

This is a follow up to #16582

I did not include the changelog since the new lint was just added and that will be in the changelog

[ada4a]

Some tentative review

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[ada4a]

I did not include the changelog since the new lint was just added and that will be in the changelog

In that case, changelog: none is still required^^

[pbor]

Thanks for the review and suggestions, PR updated.

[pbor]

@rustbot ready

[ada4a]

Hm, it didn't come out as nice as I'd imagined – it's a bit too verbose. Let me play around with it locally and get back to you when I find something nicer

[ada4a]

Hm, nevermind, I think the original version was the best after all. Could you please revert to that?^^

[pbor]

Fair enough :)

Updated (I kept the other suggestion)

[pbor]

@rustbot ready

[ada4a]

Hm, I experimented with this yet more, and thought we could lint on the spans that interest us, without needing to label them, which saves space. Here's what the patch ended up looking like:

Patch

Commit ID: 143ec05386db50ce049e149fb984931eb1201a58
Change ID: rvtzvwrmkwoomzrsuutxytmylqwqyutn
Author   : Ada Alakbarova <ada.alakbarova@proton.me> (2026-03-14 22:52:23)
Committer: Ada Alakbarova <ada.alakbarova@proton.me> (2026-03-14 22:52:23)

    (no description set)

diff --git a/clippy_lints/src/manual_pop_if.rs b/clippy_lints/src/manual_pop_if.rs
index 78176a9ad5..ee7fb80ac3 100644
--- a/clippy_lints/src/manual_pop_if.rs
+++ b/clippy_lints/src/manual_pop_if.rs
@@ -6,7 +6,7 @@
 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_errors::{Applicability, MultiSpan};
 use rustc_hir::{Expr, ExprKind, LangItem, PatKind, RustcVersion, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
@@ -143,12 +143,17 @@
     /// Span of the if expression (including the `if` keyword)
     if_span: Span,
 
+    /// Span of the:
+    /// - check call (e.g. `vec.last().is_some_and(|x| *x > 5)`)
+    /// - pop+unwrap call (e.g. `vec.pop().unwrap()`)
+    spans: MultiSpan,
+
     /// Whether we are able to provide a suggestion
     suggestable: bool,
 }
 
 impl ManualPopIfPattern<'_> {
-    fn emit_lint(&self, cx: &LateContext<'_>) {
+    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;
@@ -159,7 +164,7 @@
         span_lint_and_then(
             cx,
             MANUAL_POP_IF,
-            self.if_span,
+            self.spans,
             format!("manual implementation of {}", self.kind),
             |diag| {
                 let sugg = format!("{collection_snippet}.{pop_if_method}(|{param_name}| {predicate_snippet});");
@@ -196,7 +201,7 @@
         && 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, suggestable)) = check_pop_unwrap(then_block, pop_method)
+        && let Some((pop_collection, pop_span, 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()
@@ -207,6 +212,7 @@
             predicate: body.value,
             param_name: ident.name,
             if_span: if_expr_span,
+            spans: MultiSpan::from(vec![if_expr_span.with_hi(cond.span.hi()), pop_span]),
             suggestable,
         });
     }
@@ -257,7 +263,7 @@
 
             if let ExprKind::If(inner_cond, inner_then, None) = inner_if.kind
                 && is_local_used(cx, inner_cond, binding_id)
-                && let Some((pop_collection, suggestable)) = check_pop_unwrap(inner_then, pop_method)
+                && let Some((pop_collection, pop_span, suggestable)) = check_pop_unwrap(inner_then, pop_method)
                 && eq_expr_value(cx, collection_expr, pop_collection)
             {
                 return Some(ManualPopIfPattern {
@@ -266,6 +272,11 @@
                     predicate: inner_cond,
                     param_name: binding_name.name,
                     if_span: if_expr_span,
+                    spans: MultiSpan::from(vec![
+                        if_expr_span.with_hi(cond.span.hi()),
+                        inner_if.span.with_hi(inner_cond.span.hi()),
+                        pop_span,
+                    ]),
                     suggestable,
                 });
             }
@@ -305,7 +316,7 @@
             && path.ident.name == peek_method
             && kind.is_diag_item(cx, collection_expr)
             && is_local_used(cx, right, binding_id)
-            && let Some((pop_collection, suggestable)) = check_pop_unwrap(then_block, pop_method)
+            && let Some((pop_collection, pop_span, suggestable)) = check_pop_unwrap(then_block, pop_method)
             && eq_expr_value(cx, collection_expr, pop_collection)
         {
             return Some(ManualPopIfPattern {
@@ -314,6 +325,7 @@
                 predicate: right,
                 param_name: binding_name.name,
                 if_span: if_expr_span,
+                spans: MultiSpan::from(vec![if_expr_span.with_hi(cond.span.hi()), pop_span]),
                 suggestable,
             });
         }
@@ -349,7 +361,7 @@
         && 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, suggestable)) = check_pop_unwrap(then_block, pop_method)
+        && let Some((pop_collection, pop_span, 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()
@@ -360,6 +372,7 @@
             predicate: body.value,
             param_name: ident.name,
             if_span: if_expr_span,
+            spans: MultiSpan::from(vec![if_expr_span.with_hi(cond.span.hi()), pop_span]),
             suggestable,
         });
     }
@@ -370,7 +383,7 @@
 /// Checks for `collection.<pop_method>().unwrap()` or `collection.<pop_method>().expect(..)`
 /// 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)> {
+fn check_pop_unwrap<'tcx>(expr: &'tcx Expr<'_>, pop_method: Symbol) -> Option<(&'tcx Expr<'tcx>, Span, bool)> {
     let ExprKind::Block(block, _) = expr.kind else {
         return None;
     };
@@ -385,7 +398,7 @@
         && pop_path.ident.name == pop_method
         && !stmt_expr.span.from_expansion()
     {
-        return Some((collection_expr, true));
+        return Some((collection_expr, stmt_expr.span, true));
     }
 
     // Check if the pop unwrap is present at all
@@ -395,7 +408,7 @@
             && let ExprKind::MethodCall(pop_path, collection_expr, [], _) = receiver.kind
             && pop_path.ident.name == pop_method
         {
-            ControlFlow::Break((collection_expr, false))
+            ControlFlow::Break((collection_expr, expr.span, false))
         } else {
             ControlFlow::Continue(())
         }
diff --git a/tests/ui/manual_pop_if.rs b/tests/ui/manual_pop_if.rs
index 2f403dcf89..9c1bf26910 100644
--- a/tests/ui/manual_pop_if.rs
+++ b/tests/ui/manual_pop_if.rs
@@ -4,6 +4,8 @@
 use std::collections::VecDeque;
 use std::marker::PhantomData;
 
+fn main() {}
+
 // FakeVec has the same methods as Vec but isn't actually a Vec
 struct FakeVec<T>(PhantomData<T>);
 
diff --git a/tests/ui/manual_pop_if_unfixable.rs b/tests/ui/manual_pop_if_unfixable.rs
index 1150a68c21..c263a2a71a 100644
--- a/tests/ui/manual_pop_if_unfixable.rs
+++ b/tests/ui/manual_pop_if_unfixable.rs
@@ -2,6 +2,8 @@
 #![allow(clippy::collapsible_if, clippy::redundant_closure)]
 //@no-rustfix
 
+fn main() {}
+
 fn is_some_and_pattern(mut vec: Vec<i32>) {
     if false {
         // something

I think this strikes a good balance between brevity and helpfulness. In particular, it really benefits from the colored output -- you can see it by running

cargo dev lint tests/ui/manual_pop_if.rs

(which will fail without a fn main present, hence the addition of those in the patch)

Do let me know what you think.

[pbor]

Thanks! Looks indeed nicer.

I have squashed your change into the commit and regenerated the test output. If you prefer I can keep your changes as a their own commit

[pbor]

@rustbot ready

[ada4a]

If you prefer I can keep your changes as a their own commit

Oh don't worry about that:)

So, I think this should be mostly done, but I'm afraid I'll be too busy until the end of this week to take a final look at it, so expect some delay. My apologies.

[ada4a]

Well okay, I did end up finding a bit of time after all, so here are a couple of things; a more thorough review coming later:)

View changes since this review

[pbor]

Thanks for the review and suggestions and do not worry if you cannot get back to this right away.

I have updated the PR with the suggested changes and rebased.

@rustbot ready

[pbor]

I added one more change to also handle unsafe { pop().unwrap_unchecked() }

[ada4a]

Good news: I'm back with some time
Bad news: I've got a couple comments that might be tedious to address, sorry about that.
Good news again: Everything looks very nice otherwise, so this should be good to merge right after. Thank you for all your work so far:)

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[pbor]

I should have fixed all the comments... I did a little bit of a mess rebasing, so it is a single commit. Hopefully that is ok

[pbor]

@rustbot ready

[ada4a]

I did a little bit of a mess rebasing, so it is a single commit.

Now that I've looked at the code again, the changes to account for comments would be somewhat annoying to untangle from the rest, so let's leave this as is for now and hope no one will need to revert this^^

One last thing: I'm pretty sure these changes are worth mentioning in the changelog, given that they are user-visible. I've added two entries (an additional one for .unwrap_unchecked), trying to keep the messages somewhat short -- feel free to reword if/as desired, and let me know when I can merge.

[pbor]

Sure! Changelog looks good, I had omitted since I thought this would end up bundled together with the initial release of the lint

Let's merge!

Thanks again for your patience with the many reviews

[ada4a]

I had omitted since I thought this would end up bundled together with the initial release of the lint

Ah, that's a good point. Well, let's see what the changelog writers will decide

Thanks again for your patience with the many reviews

Thank you! Working with you has been a pleasure:)