Skip to content
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

overlapping_range_endpoints false positive #117648

Closed
benluelo opened this issue Nov 7, 2023 · 6 comments
Closed

overlapping_range_endpoints false positive #117648

benluelo opened this issue Nov 7, 2023 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-patterns Relating to patterns and pattern matching C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@benluelo
Copy link
Contributor

benluelo commented Nov 7, 2023

Code

match (0i8, 0i8) {
    (0, _) => {}
    // change either of these to not overlap and it will error
    (..=-1, ..=0) => {}
    (1.., 0..) => {}
    (1.., ..=-1) | (..=-1, 1..) => {}
}

Current output

warning: multiple patterns overlap on their endpoints
 --> src/main.rs:6:15
  |
5 |         (..=-1, ..=0) => {}
  |                 ---- this range overlaps on `0_i8`...
6 |         (1.., 0..) => {}
  |               ^^^ ... with this range
  |
  = note: you likely meant to write mutually exclusive ranges
  = note: `#[warn(overlapping_range_endpoints)]` on by default

Desired output

no output, pattern is exhaustive. addressing the warning causes an error as it is no longer exhaustive.

Rationale and extra context

No response

Other cases

No response

Anything else?

this is only occurring on nightly (at least as of 2023-11-01, i haven't tested anything sooner)

@benluelo benluelo added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 7, 2023
@saethlin saethlin added C-bug Category: This is a bug. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 9, 2023
@Nadrieril
Copy link
Member

Yep, that's somewhat intentional. The true reason is that detecting this properly would require more work from exhaustiveness checking, which is a performance cost I predicted we wouldn't want to pay. That said I haven't actually measured anything so I could be wrong.

The rationalization is that forcing the user to clarify intent could be worth it for future readers. One way to clarify would be to write ..0 | 0 instead of ..=0. I won't argue that this is particularly clearer in your case 😅.

I won't close this as wontfix until we can decide based on perf measurements, but I also don't have the intention to do this work myself so this may not be fixed for a long while sorry.

@Nadrieril Nadrieril added the A-patterns Relating to patterns and pattern matching label Dec 19, 2023
@scooter-dangle
Copy link

scooter-dangle commented Dec 28, 2023

This is showing up in a way that will cause confusion for devs in our codebase. Would it be possible to move this warning in its current form to a clippy lint given that it has easily confirmed false positives?

EDIT: I posted too quickly. Hadn't realized that this was a modification of an existing warning rather than a totally new warning. Same resolution would be preferred (move FP-prone portion to a Clippy lint if eliminating FPs isn't feasible at the moment), although maybe splitting out the behavior of the warning is more labor-intensive than just moving it in its entirety?

The addition of false positives to a previously reliable warning is unfortunate from a workaround perspective since an allow for matches of compound structures will mean a reduction in correctness checks vs 1.74.1.

One way to clarify would be to write ..0 | 0 instead of ..=0. I won't argue that this is particularly clearer in your case 😅.

Note that that workaround isn't available on stable.


Other note: I wish I had the time + knowledge to help with this. Sorry to only raise problems and not offer help on an actual fix 😞

@Nadrieril
Copy link
Member

It's unfortunately not possible to split off the FP-prone portion. The whole lint is FP-prone from the way it is currently computed :/

@Nadrieril
Copy link
Member

This turned out to be pretty easy to implement! (#119396) Let's see what the perf report says.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 28, 2023
…try>

Experiment: track overlapping ranges precisely

The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 29, 2023
…try>

Experiment: track overlapping ranges precisely

The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 30, 2023
…try>

Experiment: track overlapping ranges precisely

The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable.

r? `@ghost`
@Nadrieril
Copy link
Member

Nadrieril commented Jan 1, 2024

The perf report reports a perf improvement x) I'm glad I tried, the PR will likely get merged soon.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 12, 2024
…affleLapkin

Exhaustiveness: track overlapping ranges precisely

The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable.

r? `@ghost`
@Nadrieril
Copy link
Member

Fixed by #119396 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-patterns Relating to patterns and pattern matching C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants