Add check for overlapping ranges to unreachable patterns lint by estebank · Pull Request #64007 · rust-lang/rust

estebank · 2019-08-29T23:11:23Z

rust-highfive · 2019-08-29T23:11:26Z

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

estebank · 2019-08-29T23:15:01Z

Should this be separated to new lint instead of overloading unreachable_patterns?

I'm explicitly not handling the "multiple holes caught by _" case because it has the potential of being extremely annoying. It likely belongs in clippy complaining about _ being bad style.

CC @varkor @oli-obk @Centril

varkor · 2019-08-29T23:24:13Z

I'll look at the code more closely later, but I think this should be pulled out into its own lint, as I think there are legitimate reasons to want overlapping ranges (though overlapping by a single element seems less intentional).

joshtriplett · 2019-09-05T19:40:19Z

Conclusions from the lang team meeting:

We think there are legitimate use cases for some kinds of overlaps. For instance, matching a couple of specific letters and then matching a whole range that covers those letters as a fallback.
Thus, we think this would be prone to false positives.
We think some heuristics might help with that. For instance, overlapping ranges that don't completely overlap (0..10 and 5..15) seem more reasonable to warn about, rather than 5..10 and 0..20.

Because of that, we think this should be in clippy.

scottmcm · 2019-09-05T19:47:43Z

I feel strongly that this shouldn't be unreachable_patterns because the arm is reachable.

My interpretation here is that this needs to be heuristic-heavy, so it being in clippy sounds fine. But if there's some obvious simple heuristic that's essentially always correct I have no objection to having it in the compiler. (Maybe something like "the fix is just to change a literal to a different literal".)

estebank · 2019-09-05T20:27:39Z

I could restrict this new lint to single element overlap (0..=10/10..=20) and disjointed partial overlaps (0..10/5..15) and ignore the common desired case (100..102/0..=255). That would cover the cases I was worried about to begin with. Is this reasonable? I don't mind dropping this and relying on clippy instead :)

Centril · 2019-09-05T20:31:47Z

I think going for the single element overlap (just for ranges) is something I'd be OK with in rustc.

joshtriplett · 2019-09-12T15:01:34Z

On September 5, 2019 1:32:55 PM PDT, Mazdak Farrokhzad ***@***.***> wrote: I think going for the single element overlap (just for ranges) is something I'd be OK with in rustc.

Likewise. Good idea.

hdhoang · 2019-09-20T01:50:00Z

ping from triage @estebank, any update after the reviewers' comment?

JohnCSimon · 2019-09-27T23:57:07Z

Pinging again from triage.
@estebank Can you please address the merge conflicts.
Thanks!

JohnCSimon · 2019-10-05T18:47:32Z

Hello from triage:
@estebank -
This PR has sat idle for awhile, so I'm closing it as inactive,
Please reopen this when you have time to resolve the merge conflicts.

Thank you!

matthewjasper

@Centril should this be changed to only lint on single element overlap?

Centril · 2019-10-16T10:06:42Z

@Centril should this be changed to only lint on single element overlap?

Yes, let's start conservative for now with the consensus that is expressed in the PR discussion and we can consider expanding the scope later on. Possibly also file an issue for considering the scope-expansion and tag T-lang (maybe also dump the existing code there for keep-sakes).

…atterns

estebank · 2019-10-16T19:23:12Z

@Centril done and done.

matthewjasper · 2019-10-19T10:43:38Z

@bors r+

bors · 2019-10-19T10:43:39Z

📌 Commit 593cdcc has been approved by matthewjasper

…tthewjasper Add check for overlapping ranges to unreachable patterns lint Fixrust-lang#63987.

@ghost

Rollup of 5 pull requests Successful merges: - #64007 (Add check for overlapping ranges to unreachable patterns lint) - #65192 (Use structured suggestion for restricting bounds) - #65226 (BTreeSet symmetric_difference & union optimized) - #65448 (rustc_codegen_ssa: remove some unnecessary Box special-casing.) - #65505 (Rc: value -> allocation) Failed merges: r? @ghost

Refactor integer range handling in the usefulness algorithm Integer range handling had accumulated a lot of debt. This cleans up a lot of it. In particular this: - removes unnecessary conversions between `Const` and `u128`, and between `Constructor` and `IntRange` - clearly distinguishes between on the one hand ranges of integers that may or may not be matched exhaustively, and on the other hand ranges of non-integers that are never matched exhaustively and are compared using Const-based shenanigans - cleans up some overly complicated code paths - generally tries to be more idiomatic. As a nice side-effect, I measured a 10% perf increase on `unicode_normalization`. There's one thing that I feel remains to clean up: the [overlapping range check](#64007), which is currently quite ad-hoc. But that is intricate enough that I'm leaving it out of this PR. There's also one little thing I'm not sure I understand: can `try_eval_bits` fail for an integer constant value in that code ? What would that mean, and how do I construct a test case for this possibility ?

rust-highfive assigned matthewjasper Aug 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2019

estebank force-pushed the overlapping-patterns branch from 6bb131c to cd6d7baCompare August 29, 2019 23:12

estebank force-pushed the overlapping-patterns branch from cd6d7ba to 2731c02Compare August 29, 2019 23:24

Centril reviewed Aug 29, 2019
View reviewed changes

Comment threadsrc/librustc_mir/hair/pattern/_match.rs Outdated
Comment threadsrc/librustc_mir/hair/pattern/_match.rs Outdated
Comment threadsrc/librustc_mir/hair/pattern/check_match.rs Outdated

Centril added I-nominated T-lang Relevant to the language team labels Aug 29, 2019

This comment has been minimized.
Sign in to view

estebank force-pushed the overlapping-patterns branch from ea4ab95 to 4a8d83eCompare August 30, 2019 00:57

Centril reviewed Aug 30, 2019
View reviewed changes

Comment threadsrc/librustc_mir/hair/pattern/_match.rs Outdated

Centril removed the I-nominated label Sep 5, 2019

matthewjasper added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2019

This comment has been minimized.
Sign in to view

JohnCSimon added S-blocked-closed S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-blocked-closed labels Oct 5, 2019

JohnCSimon closed this Oct 5, 2019

estebank reopened this Oct 7, 2019

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2019

matthewjasper reviewed Oct 13, 2019
View reviewed changes

Comment threadsrc/test/ui/issues/issue-13867.rs Outdated

estebank added 10 commits October 16, 2019 11:57

Add check for overlapping ranges to unreachable patterns lint
91a3db9

Move overlapping patterns to its own lint
220b9b2

Silence lint in fit_signed and fit_unsigned
916936c

Move lint emitter to its own method
d2cb5a8

review comments: use if let
be9e6af

Deduplicate code for formatting RangeEnd
1ec6073

rename method
6832da8

Continue to emit unreachable pattern on cases caught by overlapping p…
89b19cc
…atterns

Only emit overlapping patterns lint if the overlap is partial
73d6efc

Lint only on single element overlap
593cdcc

estebank force-pushed the overlapping-patterns branch from c19d1a2 to 593cdccCompare October 16, 2019 19:22

estebank mentioned this pull request Oct 16, 2019
Extend overlapping ranges lint to cover cases with more than a single element overlapping #65477
Open

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2019

Centril added a commit to Centril/rust that referenced this pull request Oct 19, 2019
Rollup merge ofrust-lang#64007- estebank:overlapping-patterns, r=ma…
53a3bfc
…tthewjasper Add check for overlapping ranges to unreachable patterns lint Fixrust-lang#63987.

Centril mentioned this pull request Oct 19, 2019
Rollup of 5 pull requests #65598
Merged

bors merged commit 593cdcc into rust-lang:masterOct 19, 2019

Nadrieril mentioned this pull request Nov 12, 2019
Refactor integer range handling in the usefulness algorithm #66326
Merged

estebank deleted the overlapping-patterns branch November 9, 2023 05:18

Uh oh!

Uh oh!

Conversation

estebank commented Aug 29, 2019

Uh oh!

rust-highfive commented Aug 29, 2019

Uh oh!

estebank commented Aug 29, 2019

Uh oh!

varkor commented Aug 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

joshtriplett commented Sep 5, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmcm commented Sep 5, 2019

Uh oh!

estebank commented Sep 5, 2019

Uh oh!

Centril commented Sep 5, 2019

Uh oh!

This comment has been minimized.

joshtriplett commented Sep 12, 2019 via email

Uh oh!

hdhoang commented Sep 20, 2019

Uh oh!

JohnCSimon commented Sep 27, 2019

Uh oh!

JohnCSimon commented Oct 5, 2019

Uh oh!

matthewjasper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Centril commented Oct 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Oct 16, 2019

Uh oh!

matthewjasper commented Oct 19, 2019

Uh oh!

bors commented Oct 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

joshtriplett commented Sep 5, 2019•
edited
Loading

Centril commented Oct 16, 2019•
edited
Loading