Skip to content

Conversation

y21
Copy link
Member

@y21 y21 commented Apr 15, 2023

This PR implements the lint I suggested on zulip.
It looks for while loops like these:

let mut numbers = vec![0, 1, 2];
while !numbers.is_empty() {
  let number = numbers.pop().unwrap();
  // use `number`
}

and suggests replacing it with a while-let loop, like this:

let mut numbers = vec![0, 1, 2];
while let Some(number) = numbers.pop() {
  // use `number`
}

... which is more concise and idiomatic.

It only looks for Vec::pop() calls in the first statement of the loop body in an attempt to not trigger FPs (as pop might only be called conditionally).

changelog: new lint [manual_while_let_some]

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 15, 2023
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good starting point! 👍

For a style lint, I'd like to have a real working suggestion. Otherwise I'd be fine with putting the lint in nursery for the time being, and work on it in follow-up PRs. Otherwise a few more tests would be great.

@llogiq
Copy link
Contributor

llogiq commented Apr 19, 2023

I like it! 👍 The only thing I think we might improve is the name. while_pop_unwrap looks a bit out of place as a lint name. How about manual_while_let_some (we have a good number of manual_* lints already)?

@y21
Copy link
Member Author

y21 commented Apr 19, 2023

I agree, manual_while_let_some sounds better. Changing.

@y21 y21 changed the title new lint: while_pop_unwrap new lint: manual_while_let_some Apr 19, 2023
@y21 y21 force-pushed the while_pop_unwrap branch from 82a8626 to 8d8178f Compare April 29, 2023 17:00
@y21 y21 requested a review from llogiq April 29, 2023 17:19
@llogiq
Copy link
Contributor

llogiq commented Apr 29, 2023

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 29, 2023

📌 Commit 9613ea8 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 29, 2023

⌛ Testing commit 9613ea8 with merge 7bc3da9...

@bors
Copy link
Contributor

bors commented Apr 29, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 7bc3da9 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants