New lints iter_filter_is_some and iter_filter_is_ok
#12004
Merged
Conversation
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
Adds a pair of lints that check for cases of an iterator over `Result` and `Option` followed by `filter` without being followed by `map` as that is covered already by a different, specialized lint. changelog: New Lint: [`iter_filter_is_some`] changelog: New Lint: [`iter_filter_is_ok`]
|
r? @xFrednet (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
PS, sorry for the double PR, I was doing Christmas cleaning and deleted my fork. Didn't know GH wouldn't allow me to reopen the previous one 😓 |
xFrednet
reviewed
Dec 24, 2023
xFrednet
approved these changes
Dec 26, 2023
|
Looks good to me, thank you for the new lints :D @bors r+ |
bors
added a commit
that referenced
this pull request
Dec 26, 2023
New lints `iter_filter_is_some` and `iter_filter_is_ok` Adds a pair of lints that check for cases of an iterator over `Result` and `Option` followed by `filter` without being followed by `map` as that is covered already by a different, specialized lint. Fixes #11843 PS, I also made some minor documentations fixes in a case where a double tick (`) was included. --- * changelog: New Lint: [`iter_filter_is_some`] [#12004](rust-lang/rust#12004) * changelog: New Lint: [`iter_filter_is_ok`] [#12004](rust-lang/rust#12004)
|
💔 Test failed - checks-action_test |
|
Woops, looks like I invalidated the changelog entries. Let's try it again: @bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
Woops, seems that I didn't squash and force push 😬😔 |
This was referenced Dec 26, 2023
This was referenced Dec 30, 2023
bors
added a commit
that referenced
this pull request
Jan 7, 2024
Fixed ICE introduced in #12004 Issue: in #12004, we emit a lint for `filter(Option::is_some)`. If the parent expression is a `.map` we don't emit that lint as there exists a more specialized lint for that. The ICE introduced in #12004 is a consequence of the assumption that a parent expression after a filter would be a method call with the filter call being the receiver. However, it is entirely possible to have a closure of the form ``` || { vec![Some(1), None].into_iter().filter(Option::is_some) } ``` The previous implementation looked at the parent expression; namely the closure, and tried to check the parameters by indexing [0] on an empty list. This commit is an overhaul of the lint with significantly more FP tests and checks. Impl details: 1. We verify that the filter method we are in is a proper trait method to avoid FPs. 2. We check that the parent expression is not a map by checking whether it exists; if is a trait method; and then a method call. 3. We check that we don't have comments in the span. 4. We verify that we are in an Iterator of Option and Result. 5. We check the contents of the filter. 1. For closures we peel it. If it is not a single expression, we don't lint. We then try again by checking the peeled expression. 2. For paths, we do a typecheck to avoid FPs for types that impl functions with the same names. 3. For calls, we verify the type, via the path, and that the param of the closure is the single argument to the call. 4. For method calls we verify that the receiver is the parameter of the closure. Since we handle single, non-block exprs, the parameter can't be shadowed, so no FP. This commit also adds additional FP tests. Fixes: #12058 Adding `@xFrednet` as you've the most context for this as you reviewed it last time. `@rustbot` r? `@xFrednet` --- changelog: none (Will be backported and therefore don't effect stable)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds a pair of lints that check for cases of an iterator over
ResultandOptionfollowed byfilterwithout being followed bymapas that is covered already by a different, specialized lint.Fixes #11843
PS, I also made some minor documentations fixes in a case where a double tick (`) was included.
changelog: New Lint: [
iter_filter_is_some]#12004
changelog: New Lint: [
iter_filter_is_ok]#12004