Refactor integer range handling in the usefulness algorithm by Nadrieril · Pull Request #66326 · rust-lang/rust

Nadrieril · 2019-11-12T09:37:46Z

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, 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 · 2019-11-12T09:37:49Z

r? @varkor

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

Nadrieril · 2019-11-12T09:44:17Z

Actually I figured out a test case where try_eval_bits fails: 1084eff . However I still don't understand what's causing this and whether this is a feature or a bug.

src/test/ui/pattern/usefulness/exhaustive_integer_patterns.rs

src/librustc_mir/hair/pattern/_match.rs

Centril · 2019-11-12T10:42:51Z

src/librustc_mir/hair/pattern/_match.rs

        }
    }

+    fn is_subrange(&self, other: &Self) -> bool {


This feels like a useful operation that could be added to the standard library on ranges; cc @dtolnay

src/librustc_mir/hair/pattern/_match.rs

Centril · 2019-11-12T11:17:34Z

@bors try @rust-timer queue

rust-timer · 2019-11-12T11:17:36Z

Awaiting bors try build completion

bors · 2019-11-12T11:17:45Z

⌛ Trying commit 1084eff with merge f1da198...

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 ?

Nadrieril · 2019-11-12T13:19:55Z

Hmm, it turns out I unknowingly fixed a bug:

const BAR: &i32 = &42;
match &0 {
    BAR => {}
    _ => {}
}

used to flag the first branch as unreachable. This looks similar to #65413.

varkor · 2019-11-12T13:30:04Z

Does this fix #53708 as well?

Nadrieril · 2019-11-12T13:39:53Z

@varkor No, this was a bug specific to integer ranges and values, in the case where try_eval_bits fails. There is a link with #53708 however: in both cases the problem comes from consts behind references.

varkor

I like what you've done here. Just have a couple of very minor comments :)

src/librustc_mir/hair/pattern/_match.rs

Centril · 2019-11-13T07:48:58Z

@bors try @rust-timer queue

rust-timer · 2019-11-13T07:49:01Z

Awaiting bors try build completion

bors · 2019-11-13T07:49:09Z

⌛ Trying commit 0e3ec65 with merge 9fabb30...

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 ?

bors · 2019-11-13T10:31:20Z

☀️ Try build successful - checks-azure
Build commit: 9fabb30 (9fabb30244907581fb04a64c9cb27f3ff9db04fc)

rust-timer · 2019-11-13T10:31:22Z

Queued 9fabb30 with parent 374ad1b, future comparison URL.

rust-timer · 2019-11-13T20:06:38Z

Finished benchmarking try commit 9fabb30, comparison URL.

Centril · 2019-11-14T05:20:10Z

@bors rollup=never

…stive

Only IntRange should need to worry about range exhaustiveness really.

That way no `ConstantRange` or `ConstantValue` ever needs to be converted to `IntRange`.

That condition was leftover from a refactor, and was probably not intended. In fact it can't trigger: it would require a ConstantValue of an integral type for which `try_eval_bits` fails. But since we only apply `subtract_ctors` to the output of `all_ctors`, this won't happen.

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Nadrieril · 2019-11-15T17:26:05Z

Rebased onto master and applied latest reviews :)

varkor · 2019-11-15T21:29:45Z

@bors r+

bors · 2019-11-15T21:29:47Z

📌 Commit 694a511 has been approved by varkor

bors · 2019-11-15T23:28:57Z

⌛ Testing commit 694a511 with merge 82161cd...

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 ?

bors · 2019-11-16T02:40:46Z

☀️ Test successful - checks-azure
Approved by: varkor
Pushing 82161cd to master...

rust-highfive assigned varkor Nov 12, 2019

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

Centril reviewed Nov 12, 2019

View reviewed changes

varkor reviewed Nov 12, 2019

View reviewed changes

Nadrieril added 9 commits November 15, 2019 15:38

Note link between apply/specialize/arity functions

c75685b

Clarify conditions for exhaustive integer range matching

8f7ba7a

Factor out range construction in all_constructors

3531c52

pat_constructor does not need pcx anymore

34ad52e

IntRange::from_pat is redundant with pat_constructors

8c1835d

Cleanup constructor_covered_by_range

f3752ee

Special-case range inclusion when the range is integral but non-exhau…

81db2ee

…stive

Move range exhaustiveness check to IntRange::intersection

0a18610

Only IntRange should need to worry about range exhaustiveness really.

Prefer IntRange::into_ctor to range_to_ctor

6de7afd

Nadrieril and others added 17 commits November 15, 2019 16:39

Introduce IntRange constructor

f93a700

formatting

4232816

Add IntRange::to_pat and use it instead of custom display()

6b8bfef

Eagerly convert ranges to IntRange

84784dd

That way no `ConstantRange` or `ConstantValue` ever needs to be converted to `IntRange`.

Factor out getting the boundaries of an IntRange

aadd5e5

Inline now-trivial IntRange::from_ctor

d1642f1

Make should_treat_range_exhaustively a method

0192124

Store Const directly in ConstantRange

f674f89

Remove unnecessary data from ConstantValue/ConstantRange

3e5aadc

Malformed range patterns can't happen thanks to E0030

e47d631

Add test for failing try_eval_bits

c38aad4

Apply suggestions from code review

9165dd0

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

Factor out IntRange::is_subrange

b679e77

Apply suggestions from code review

addd8a9

Add regression test

cde9808

Apply suggestions from code review

694a511

Nadrieril force-pushed the refactor-intrange branch from 0e3ec65 to 694a511 Compare November 15, 2019 17:25

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 Nov 15, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 16, 2019

bors merged commit 694a511 into rust-lang:master Nov 16, 2019

Nadrieril deleted the refactor-intrange branch November 16, 2019 13:24

SimonSapin mentioned this pull request Nov 17, 2019

ICE 'no entry found for key' in Servo’s style crate #66487

Closed

Uh oh!

Conversation

Nadrieril commented Nov 12, 2019

Uh oh!

rust-highfive commented Nov 12, 2019

Uh oh!

Nadrieril commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril commented Nov 12, 2019

Uh oh!

rust-timer commented Nov 12, 2019

Uh oh!

bors commented Nov 12, 2019

Uh oh!

Nadrieril commented Nov 12, 2019

Uh oh!

varkor commented Nov 12, 2019

Uh oh!

Nadrieril commented Nov 12, 2019

Uh oh!

varkor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Centril commented Nov 13, 2019

Uh oh!

rust-timer commented Nov 13, 2019

Uh oh!

bors commented Nov 13, 2019

Uh oh!

bors commented Nov 13, 2019

Uh oh!

rust-timer commented Nov 13, 2019

Uh oh!

rust-timer commented Nov 13, 2019

Uh oh!

Centril commented Nov 14, 2019

Uh oh!

Nadrieril commented Nov 15, 2019

Uh oh!

varkor commented Nov 15, 2019

Uh oh!

bors commented Nov 15, 2019

Uh oh!

bors commented Nov 15, 2019

Uh oh!

bors commented Nov 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants