Skip to content

Fixed ICE introduced in #12004 #12080

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
Jan 7, 2024
Merged

Fixed ICE introduced in #12004 #12080

merged 1 commit into from
Jan 7, 2024

Conversation

PartiallyUntyped
Copy link
Contributor

@PartiallyUntyped PartiallyUntyped commented Jan 3, 2024

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)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2024
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for fixing this ICE! ❤️ I have three nits, but nothing major.

None
}

#[allow(clippy::too_many_arguments)]
Copy link
Member

Choose a reason for hiding this comment

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

NIT:

Suggested change
#[allow(clippy::too_many_arguments)]
#[expect(clippy::too_many_arguments)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't necessary for dogfood tests, so I removed it completely.

Copy link
Member

Choose a reason for hiding this comment

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

#[expect] doing some work 💪

I really hope we can stabilize it soon 🙏

@xFrednet xFrednet added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 7, 2024
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.
  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.
@xFrednet
Copy link
Member

xFrednet commented Jan 7, 2024

Looks good to me, thank you!

And now, sir bors, please merging this beautiful update!

@bors
Copy link
Contributor

bors commented Jan 7, 2024

📌 Commit bbadce9 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 7, 2024

⌛ Testing commit bbadce9 with merge 7fbaba8...

@bors
Copy link
Contributor

bors commented Jan 7, 2024

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

@bors bors merged commit 7fbaba8 into rust-lang:master Jan 7, 2024
@PartiallyUntyped PartiallyUntyped deleted the 12058 branch January 7, 2024 23:46
@flip1995
Copy link
Member

flip1995 commented Feb 1, 2024

The commit that introduced the ICE was merged after last beta was branched. So the ICE-introducing commit is not on beta, meaning that this PR doesn't need a backport as it will land together with the ICE-introducing commit in beta.

As always, thanks for labeling this PR anyway!

@flip1995 flip1995 removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 1, 2024
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.

index out of bounds: panicked at iter_filter.rs:26:26
5 participants