Skip to content

New Lint: result_filter_map / Mirror of option_filter_map #11869

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.

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

Merged
merged 1 commit into from
Dec 16, 2023
Merged

New Lint: result_filter_map / Mirror of option_filter_map #11869

merged 1 commit into from
Dec 16, 2023

Conversation

PartiallyUntyped
Copy link
Contributor

@PartiallyUntyped PartiallyUntyped commented Nov 25, 2023

Added the Result mirror of option_filter_map.

changelog: New Lint: [result_filter_map]

I had to move around some code because the function def was too long 🙃.

I have also added some pattern checks on option_filter_map

@rustbot
Copy link
Collaborator

rustbot commented Nov 25, 2023

r? @Alexendoo

(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 Nov 25, 2023
@PartiallyUntyped
Copy link
Contributor Author

PartiallyUntyped commented Nov 25, 2023

This partially fixes #11843, maybe @flip1995 would be interested in reviewing this, and given this, I take it, the goal for #11843 is lint all other cases not caught here and suggest directly flattening?

@flip1995
Copy link
Member

As I suggested this, I'd like to get a second opinion from a different reviewer on this :) thanks for the ping though!

@PartiallyUntyped
Copy link
Contributor Author

PartiallyUntyped commented Nov 25, 2023

@flip1995 is this sufficiently close to what you had in mind for #11843? For this I just mirrored the Option::is_some case, but I have the hunch that's not exactly what you had in mind, and were looking for a more general lint instead.

@PartiallyUntyped PartiallyUntyped marked this pull request as ready for review November 25, 2023 18:41
@PartiallyUntyped
Copy link
Contributor Author

@rustbot ready

@PartiallyUntyped PartiallyUntyped changed the title New Lint: result_filter_map / Mirror of ok_filter_map New Lint: result_filter_map / Mirror of option_filter_map Nov 25, 2023
@flip1995
Copy link
Member

Yes, that's what I had in mind.

@@ -3752,6 +3752,28 @@ declare_clippy_lint! {
"using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for indirect collection of populated `Result`
Copy link
Member

Choose a reason for hiding this comment

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

I see this wording is copied from option_filter_map but it would be good to change it in both places since it sounds like it's talking about .collect(), maybe go very literal along the lines of

Checks for iterators of Results using .filter(Result::is_ok).map(Result::unwrap) that can be replaced with .flatten()

Comment on lines 3760 to 3761
/// `Result` is like a collection of 0-1 things, so `flatten`
/// automatically does this without suspicious-looking `unwrap` calls.
Copy link
Member

Choose a reason for hiding this comment

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

The "0-1 things" doesn't apply that well to Result, you could instead mention directly that this works because Result<T, E> implements IntoIterator<Item = T>

@PartiallyUntyped
Copy link
Contributor Author

PartiallyUntyped commented Dec 4, 2023

Sorry for the delay, life's gotten pretty busy lately. I have made the appropriate changes.

@PartiallyUntyped
Copy link
Contributor Author

@Alexendoo is everything okay with this?

@Alexendoo
Copy link
Member

Just that and I think it's good to go

Added the `Result` mirror of `option_filter_map` to catch

```
   .into_iter().filter(Result::is_ok).map(Result::unwrap)
```

changelog: New Lint: [`result_filter_map`]
Co-authored-by: Alex Macleod <[email protected]>
@Alexendoo
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 16, 2023

📌 Commit 8892420 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 16, 2023

⌛ Testing commit 8892420 with merge f9b5def...

@bors
Copy link
Contributor

bors commented Dec 16, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing f9b5def to master...

@bors bors merged commit f9b5def into rust-lang:master Dec 16, 2023
@PartiallyUntyped PartiallyUntyped deleted the result-filter-map branch December 19, 2023 18:41
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.

5 participants