manual_pop_if: lint when the popped value is discarded - #17630
Open
Iurig wants to merge 3 commits into
Open
Conversation
`if vec.last().is_some_and(|x| *x > 5) { vec.pop(); }` is exactly
`vec.pop_if(|x| *x > 5);`, but the lint only fired when the popped value
was unwrapped. Accept a pop whose value is thrown away as well, both as a
plain `collection.pop();` statement and as `let _ = collection.pop();`.
A discarded pop is only recognised in statement position. Unlike
`.unwrap()`, a `collection.pop()` in value position carries no assertion
that the collection is non-empty, so walking every expression for one
would be a large false-positive surface for no benefit.
An annotated `let _: Option<i32> = collection.pop();` is left alone too:
the suggestion replaces the whole statement, and the annotation may be
the only thing pinning down the element type of the collection.
rustbot
added
the
S-waiting-on-community-reviews
Collaborator
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. |
rustbot
added
the
S-waiting-on-review
CommanderStorm
suggested changes
Aug 25, 2026
Comment on lines
+449
to
+455
| // A statement that pops and throws the value away: `collection.pop();` or | ||
| // `let _ = collection.pop();`. Unlike a bare `collection.pop()` in value position, | ||
| // this is exactly what `pop_if` does, so it is worth linting on its own. | ||
| // | ||
| // An annotated `let _: Option<i32> = collection.pop();` is left alone: the whole | ||
| // statement is replaced by the suggestion, and the annotation may be what pins down | ||
| // the element type of the collection. |
Contributor
There was a problem hiding this comment.
please write comments a bit more human.
no weird semicolons, one sentence per line, short sentnces.
this goes for all the code comments you added.
Author
There was a problem hiding this comment.
let me know if this is succint enough, I can rework if not.
| // Returns the pop call of a statement, along with the position the replaced code | ||
| // starts at (which is the start of the statement itself, as e.g. the `let _ =` of a | ||
| // discarded pop is replaced as well). | ||
| let as_pop_stmt = |stmt: &Stmt<'tcx>| -> Option<(&'tcx Expr<'tcx>, Span, BytePos)> { |
Contributor
There was a problem hiding this comment.
only used once, please inline where you got the code from.
you can extract code, but this does not seem intentional.
Author
There was a problem hiding this comment.
yeah, that was leftover from an earlier iteration, not intentional.
inlined it back into the single statement check.
Inline `as_pop_stmt` into its single call site. Reword the added comments into short sentences, one per line.
Let sentences wrap across lines instead of forcing one per line, in line with the surrounding comment style.
CommanderStorm
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
changelog: [
manual_pop_if]: also lint when the popped value is discarded, e.g.vec.pop();if vec.last().is_some_and(|x| *x > 5) { vec.pop(); }is exactlyvec.pop_if(|x| *x > 5);, but the lint only fired when the popped value was unwrapped. Accept a pop whose value is thrown away as well, both as a plaincollection.pop();statement and aslet _ = collection.pop();.A discarded pop is only recognised in statement position. Unlike
.unwrap(), acollection.pop()in value position carries no assertion that the collection is non-empty, so walking every expression for one would be a large false-positive surface for no benefit.An annotated
let _: Option<i32> = collection.pop();is left alone too: the suggestion replaces the whole statement, and the annotation may be the only thing pinning down the element type of the collection.