Exhaustive integer matching

[varkor]

This adds a new feature flag exhaustive_integer_patterns that enables exhaustive matching of integer types by their values. For example, the following is now accepted:

#![feature(exhaustive_integer_patterns)]
#![feature(exclusive_range_pattern)]

fn matcher(x: u8) {
  match x { // ok
    0 .. 32 => { /* foo */ }
    32 => { /* bar */ }
    33 ..= 255 => { /* baz */ }
  }
}

This matching is permitted on all integer (signed/unsigned and char) types. Sensible error messages are also provided. For example:

fn matcher(x: u8) {
  match x { //~ ERROR
    0 .. 32 => { /* foo */ }
  }
}

results in:

error[E0004]: non-exhaustive patterns: `32u8...255u8` not covered
 --> matches.rs:3:9
  |
6 |   match x {
  |         ^ pattern `32u8...255u8` not covered

This implements rust-lang/rfcs#1550 for #50907. While there hasn't been a full RFC for this feature, it was suggested that this might be a feature that obviously complements the existing exhaustiveness checks (e.g. for bool) and so a feature gate would be sufficient for now.

[rust-highfive]

r? @michaelwoerister

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

[scottmcm]

I'm definitely fine with merging this without an RFC (feature-gated of course, as you did).

^ pattern 32u8...255u8 not covered

This is amazing even if all it did was give that error instead of just "pattern _ not covered".

[scottmcm]

I'm definitely fine with merging this without an RFC (feature-gated of course, as you did).

^ pattern 32u8...255u8 not covered

This is amazing even if all it did was give that error instead of just "pattern _ not covered".

[scottmcm]

Since char is a USV, it doesn't actually need to cover that entire range. If it's feasible, could this handle the example in rust-lang/rfcs#1550 (comment) too? (Feel free to punt if hard.)

[scottmcm]

Could adjacent ranges be merged, or at least not overlap? 121 is in two of the uncovered ranges, and it would be nice if the message was "patterns -128i8...-5i8 and 120i8...127i8 not covered" instead.

[scottmcm]

All the holes in the examples are non-singleton. Consider a UI test for something like -127i8..=127 or match 0 { i16::MIN..=-1 => {} 1..=i16::MAX => {} }.

[scottmcm]

Hopefully the char request is just to initialize this as a two-element vec instead...

[michaelwoerister]

r? @eddyb maybe?

[eddyb]

cc @oli-obk This sort of thing can be done from the bit width (which you can get from the layout of these types).

[eddyb]

You should use ty::layout::Integer here or similar.

[eddyb]

Maybe you can use RangeInclusive<u128> instead?

[eddyb]

Should ConstantRange use ty::Const? Seems expensive, and useless unless the ty::Const is Bits (cc @oli-obk).

[oli-obk]

We should definitely be using ConstValue instead of ty::Const. I put it on my TODO list

[eddyb]

ConstantRange can only hold integers, so ConstValue shouldn't be needed, right?

[oli-obk]

Probably. My TODO already says "ConstValue or smaller" ;). That should not block this PR though, since it's a preexisting issue.

[eddyb]

I think you missed -1 and +1 on the fields of pat_interval, here.

[eddyb]

You could construct the intersection by i = a.start.max(b.start)..=a.end.min(b.end), and the two halves of the subtraction a - b by sl = a.start..=i.start-1 and sh = i.end+1..=a.end (be careful with overflow).

AFAIK, the empty condition is range.start > range.end (for inclusive ranges).

Since I don't think you need the intersection by itself, the two halves can be:
sl = a.start..=a.start.max(b.start)-1 and
sh = a.end.min(b.end)+1..=a.end
(when a.start.max(b.start) <= a.end.min(b.end), otherwise sl = sh = a)

Therefore:

if a.start > b.end || b.start > a.end {
    // No intersection.
    remaining_ranges.push(a);
} else {
    // Overflow is now not a problem, because of the conditions.
    if b.start > a.start {
        remaining_ranges.push(a.start..=b.start-1);
    }
    if b.end < a.end {
        remaining_ranges.push(b.end+1..=a.end);
    }
}

---

I've just checked and your code corresponds to this, but you have the 4 possibilities of my two small ifs expanded out - my main suggestion from this comment would be to orthogonalize those.

[eddyb]

Could this iterate over &all_ctors instead?

[eddyb]

I left some nit-picky comments about implementation details, but r? @nikomatsakis

[eddyb]

Oh, this is broken, because it's using the host's usize / isize bitwidth instead of the target's.

[varkor]

I've addressed most of the comments, but there's still the issue with usize/isize for offset_sign_for_ty, which I'll try to get to tomorrow.

[varkor]

These offsets do not look right now. I'll look into that.

[eddyb]

You should use cx.param_env or something. Also, you don't need to match on uint_ty at all! Pass pcx.ty instead.

[varkor]

Oh, that's so much nicer, of course!

[eddyb]

remaining_ranges could be a Vec<RangeInclusive<u128>>, i.e. use ..= syntax.

[eddyb]

These, especially usize, are a bad sign - you shouldn't need them.

[eddyb]

This can be implemented with just !0 >> (128 - bits).

[eddyb]

Why do you need wrapping_neg here?

[eddyb]

Don't need wrapping_sub, can just - 1.

[varkor]

Everything seems to be working correctly now, as far as I can tell.

@eddyb had some suggestions for refactoring ConstantRange and PatternKind::Range to use u128 instead of ty::Const, but after looking into it, it looks like it's going to require touching a lot of unrelated areas, which I'd rather avoid doing for this PR. At the very least, any negative effects of creating extra Consts are going to be limited to #![feature(exhaustive_integer_patterns)], so following this PR up later shouldn't be an issue. (Though let me know if I've misunderstood anything, @eddyb.)

[arielb1]

comment: duplicate comment, should split by cases

[arielb1]

comment: duplicate comment, should split by cases

[arielb1]

Nice PR overall. Just need to take a look at the range-splitting thing and write a mega-comment there (you could try to write one yourself).

[varkor]

I've tried to improve the comments in split_grouped_constructors, but it's hard to know just how detailed it should be (as it's fresher in my head than it is to a reader), so let me know how it can be improved further!

[arielb1]

How does this handle integer overflow (i.e. a.0 = uint128::MAX)? worth a test case, at least.

[varkor]

(Edited) a.0 < b.0 strictly, so overflow shouldn't be possible. There are already test cases for covering the entire range of u128 and i128, so I think that covers the bases already. I'll add a comment to this effect.

[arielb1]

Other than that and the duplication, this looks good.

[arielb1]

A: integer overflow can't occur, because only the last point can be uint128::MAX, and only the first point can be 0.

[arielb1]

A potential simplification:
I just noticed that range boundaries are always "in between" numbers - an Endpoint::Start is a boundary between p-1 and p, and an Endpoint::End is between p and p+1 (that means that an Endpoint::End is equivalent to an Endpoint::Start that immediately follows it). You could do something based on that.

[arielb1]

e.g., I think this is a correct implementation:

/// Represents a border between 2 integers. Because of the "fencepost error",
/// there are be 2^128+1 such borders.
#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq)]
enum Border {
    JustBefore(u128),
    AfterU128Max
}

impl Border {
    fn before(n: u128) -> Self {
        Border::JustBefore(n)
    }

    fn after(n: u128) -> Self {
        match n.checked_add(1) {
            m => Border::JustBefore(m),
            None => Border::AfterU128Max
        }
    }

    fn range_borders(r: IntRange<'_>) -> [Self; 2] {
        let (r_lo, r_hi) = r.range.into_inner();
        vec![Border::before(r_lo), Border::after(r_hi)]
    }

    // return the integers between `self` and `to` if `self < to`, `None` otherwise.
    fn range_to(self, to: Self, ty: Ty<'tcx>) -> Option<IntRange<'tcx>> {
        match (self, to) {
            (Border::JustBefore(n), Border::JustBefore(m)) => {
                IntRange::from_interval(ty, n, m, RangeEnd::Excluded)
            }
            (Border::JustBefore(n), Border::AfterU128Max) => {
                IntRange::from_interval(ty, n, u128::MAX, RangeEnd::Included)
            }
            (Border::AfterU128Max, _) => None
        }
    }
}

// `borders` is the set of borders between equivalence classes - each equivalence
// class is between 2 borders.
let row_borders = m.iter()
    .flat_map(|row| IntRange::from_pat(tcx, row[0]))
    .flat_map(|range| ctor_range.intersection(&r))
    .flat_map(|range| Border::range_borders(range));
let range_borders = Border::range_borders(ctor_range);
let mut borders: Vec<Border> = row_borders.chain(range_borders).collect();
borders.sort();

let ranges = borders.windows(2).map(|window| {
    window[0].range_to(window[1]);
})

[varkor]

I do think this is cleaner :) The logic looks good to me too — I'm going to give it a go.

[arielb1]

LGTM. If you think borders are cleaner enough than endpoints, you can use that, otherwise r=me. (Also, add a test that 0..u128::MAX-1 is not exhaustive and write my "why integer overflow can't occur" note in a comment).

[arielb1]

@bors r+

[bors]

📌 Commit 6971c5d has been approved by arielb1

[varkor]

@arielb1: thank you for all your detailed comments and pointing me in the right direction!

[bors]

⌛️ Testing commit 6971c5d with merge a79cffb...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing a79cffb to master...