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

Match usize/isize exhaustively with half-open ranges #116692

Merged
merged 9 commits into from
Nov 1, 2023

Conversation

Nadrieril
Copy link
Member

The long-awaited finale to the saga of exhaustiveness checking for integers!

match 0usize {
    0.. => {} // exhaustive!
}
match 0usize {
    0..usize::MAX => {} // helpful error message!
}

Features:

  • Half-open ranges behave as expected for usize/isize;
  • Trying to use 0..usize::MAX will tell you that usize::MAX.. is missing and explain why. No more unhelpful "_ is missing";
  • Everything else stays the same.

This should unblock #37854.

Review-wise:

  • I recommend looking commit-by-commit;
  • This regresses perf because of the added complexity in IntRange; hopefully not too much;
  • I measured each #[inline], they all help a bit with the perf regression (tho I don't get why);
  • I did not touch MIR building; I expect there's an easy PR there that would skip unnecessary comparisons when the range is half-open.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2023

r? @compiler-errors

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2023
@Nadrieril
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2023
@bors
Copy link
Contributor

bors commented Oct 13, 2023

⌛ Trying commit 6cd0d45 with merge 36d4397...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2023
Match usize/isize exhaustively with half-open ranges

The long-awaited finale to the saga of [exhaustiveness checking for integers](rust-lang#50912)!

```rust
match 0usize {
    0.. => {} // exhaustive!
}
match 0usize {
    0..usize::MAX => {} // helpful error message!
}
```

Features:
- Half-open ranges behave as expected for `usize`/`isize`;
- Trying to use `0..usize::MAX` will tell you that `usize::MAX..` is missing and explain why. No more unhelpful "`_` is missing";
- Everything else stays the same.

This should unblock rust-lang#37854.

Review-wise:
- I recommend looking commit-by-commit;
- This regresses perf because of the added complexity in `IntRange`; hopefully not too much;
- I measured each `#[inline]`, they all help a bit with the perf regression (tho I don't get why);
- I did not touch MIR building; I expect there's an easy PR there that would skip unnecessary comparisons when the range is half-open.
@rust-log-analyzer

This comment has been minimized.

@Nadrieril
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 13, 2023

⌛ Trying commit c0b2138 with merge a23767c...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2023
Match usize/isize exhaustively with half-open ranges

The long-awaited finale to the saga of [exhaustiveness checking for integers](rust-lang#50912)!

```rust
match 0usize {
    0.. => {} // exhaustive!
}
match 0usize {
    0..usize::MAX => {} // helpful error message!
}
```

Features:
- Half-open ranges behave as expected for `usize`/`isize`;
- Trying to use `0..usize::MAX` will tell you that `usize::MAX..` is missing and explain why. No more unhelpful "`_` is missing";
- Everything else stays the same.

This should unblock rust-lang#37854.

Review-wise:
- I recommend looking commit-by-commit;
- This regresses perf because of the added complexity in `IntRange`; hopefully not too much;
- I measured each `#[inline]`, they all help a bit with the perf regression (tho I don't get why);
- I did not touch MIR building; I expect there's an easy PR there that would skip unnecessary comparisons when the range is half-open.
@bors
Copy link
Contributor

bors commented Oct 13, 2023

☀️ Try build successful - checks-actions
Build commit: a23767c (a23767c7a2d14f82db2a6901fbc8e948cb39b0f5)

1 similar comment
@bors
Copy link
Contributor

bors commented Oct 13, 2023

☀️ Try build successful - checks-actions
Build commit: a23767c (a23767c7a2d14f82db2a6901fbc8e948cb39b0f5)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a23767c): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.2%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.5%, -1.3%] 6
All ❌✅ (primary) 1.1% [1.1%, 1.2%] 2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 626.271s -> 628.214s (0.31%)
Artifact size: 271.31 MiB -> 271.33 MiB (0.00%)

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

yo this is great!

@Nadrieril
Copy link
Member Author

Fixed the conflict with master

@Nadrieril Nadrieril removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 17, 2023
@cjgillot cjgillot self-assigned this Oct 19, 2023
@bors

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@Nadrieril
Copy link
Member Author

I'm thinking: diagnostic-wise, we currently tend to suggest things like "10..=u32::MAX missing". We could instead suggest 10.. to make half-open range patterns more discoverable

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 31, 2023

📌 Commit 35fe75d has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 31, 2023
@Nadrieril
Copy link
Member Author

Thanks for the review!

@bors
Copy link
Contributor

bors commented Nov 1, 2023

⌛ Testing commit 35fe75d with merge 7fc6365...

@bors
Copy link
Contributor

bors commented Nov 1, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 7fc6365 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 1, 2023
@bors bors merged commit 7fc6365 into rust-lang:master Nov 1, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 1, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7fc6365): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [1.1%, 1.2%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [1.1%, 1.2%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.5% [-4.2%, -2.8%] 2
All ❌✅ (primary) 1.1% [1.1%, 1.1%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.7%, -2.6%] 4
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 636.96s -> 639.311s (0.37%)
Artifact size: 304.45 MiB -> 304.48 MiB (0.01%)

@rustbot rustbot removed the perf-regression Performance regression. label Nov 1, 2023
@Nadrieril Nadrieril deleted the half-open-ranges branch November 1, 2023 10:14
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 2, 2023
80: Automated pull from upstream `master` r=tshepang a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117498
  * rust-lang/rust#117488
  * rust-lang/rust#117441
  * rust-lang/rust#117373
  * rust-lang/rust#117298
* rust-lang/rust#117029
* rust-lang/rust#117289
* rust-lang/rust#117307
* rust-lang/rust#114208
* rust-lang/rust#117482
  * rust-lang/rust#117475
  * rust-lang/rust#117401
  * rust-lang/rust#117397
  * rust-lang/rust#115626
* rust-lang/rust#117436
* rust-lang/rust#115356
* rust-lang/rust#117422
* rust-lang/rust#116692



Co-authored-by: David CARLIER <[email protected]>
Co-authored-by: Taiki Endo <[email protected]>
Co-authored-by: ltdk <[email protected]>
Co-authored-by: Ryan Mehri <[email protected]>
@Nadrieril Nadrieril removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 4, 2023
@Nadrieril
Copy link
Member Author

Is this worthy of relnotes? I think this is worthy of relnotes. I'm doing it, someone stop me if I shouldn't

@Nadrieril Nadrieril added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 4, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2023
…ize_matching, r=davidtwco

Remove the `precise_pointer_size_matching` feature gate

`usize` and `isize` are special for pattern matching because their range might depend on the platform. To make code portable across platforms, the following is never considered exhaustive:
```rust
let x: usize = ...;
match x {
    0..=18446744073709551615 => {}
}
```
Because of how rust handles constants, this also unfortunately counts `0..=usize::MAX` as non-exhaustive. The [`precise_pointer_size_matching`](rust-lang#56354) feature gate was introduced both for this convenience and for the possibility that the lang team could decide to allow the above.

Since then, [half-open range patterns](rust-lang#67264) have been implemented, and since rust-lang#116692 they correctly support `usize`/`isize`:
```rust
match 0usize { // exhaustive!
    0..5 => {}
    5.. => {}
}
```
I believe this subsumes all the use cases of the feature gate. Moreover no attempt has been made to stabilize it in the 5 years of its existence. I therefore propose we retire this feature gate.

Closes rust-lang#56354
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2023
Rollup merge of rust-lang#118598 - Nadrieril:remove_precise_pointer_size_matching, r=davidtwco

Remove the `precise_pointer_size_matching` feature gate

`usize` and `isize` are special for pattern matching because their range might depend on the platform. To make code portable across platforms, the following is never considered exhaustive:
```rust
let x: usize = ...;
match x {
    0..=18446744073709551615 => {}
}
```
Because of how rust handles constants, this also unfortunately counts `0..=usize::MAX` as non-exhaustive. The [`precise_pointer_size_matching`](rust-lang#56354) feature gate was introduced both for this convenience and for the possibility that the lang team could decide to allow the above.

Since then, [half-open range patterns](rust-lang#67264) have been implemented, and since rust-lang#116692 they correctly support `usize`/`isize`:
```rust
match 0usize { // exhaustive!
    0..5 => {}
    5.. => {}
}
```
I believe this subsumes all the use cases of the feature gate. Moreover no attempt has been made to stabilize it in the 5 years of its existence. I therefore propose we retire this feature gate.

Closes rust-lang#56354
@Nadrieril Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2024
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 16, in accordance
   with the upstream changes.
 * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported.
 * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded
   LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it,
   resulting in an illegal instruction fault during the build.
   Ref. rust-lang/rust#117231

Upstream changes:

Version 1.75.0 (2023-12-28)
==========================

- [Stabilize `async fn` and return-position `impl Trait` in traits.]
  (rust-lang/rust#115822)
- [Allow function pointer signatures containing `&mut T` in `const` contexts.]
  (rust-lang/rust#116015)
- [Match `usize`/`isize` exhaustively with half-open ranges.]
  (rust-lang/rust#116692)
- [Guarantee that `char` has the same size and alignment as `u32`.]
  (rust-lang/rust#116894)
- [Document that the null pointer has the 0 address.]
  (rust-lang/rust#116988)
- [Allow partially moved values in `match`.]
  (rust-lang/rust#103208)
- [Add notes about non-compliant FP behavior on 32bit x86 targets.]
  (rust-lang/rust#113053)
- [Stabilize ratified RISC-V target features.]
  (rust-lang/rust#116485)

Compiler
--------

- [Rework negative coherence to properly consider impls that only
  partly overlap.] (rust-lang/rust#112875)
- [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.]
  (rust-lang/rust#116493)
- [Consider alias bounds when computing liveness in NLL.]
  (rust-lang/rust#116733)
- [Add the V (vector) extension to the `riscv64-linux-android` target spec.]
  (rust-lang/rust#116618)
- [Automatically enable cross-crate inlining for small functions]
  (rust-lang/rust#116505)
- Add several new tier 3 targets:
    - [`csky-unknown-linux-gnuabiv2hf`]
      (rust-lang/rust#117049)
    - [`i586-unknown-netbsd`]
      (rust-lang/rust#117170)
    - [`mipsel-unknown-netbsd`]
      (rust-lang/rust#117356)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.]
  (rust-lang/rust#96979)
- [Implement `BufRead` for `VecDeque<u8>`.]
  (rust-lang/rust#110604)
- [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.]
  (rust-lang/rust#110729)
- [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.]
  (rust-lang/rust#113747)
- [Implement `Default` for `ExitCode`.]
  (rust-lang/rust#114589)
- [Guarantee representation of None in NPO]
  (rust-lang/rust#115333)
- [Document when atomic loads are guaranteed read-only.]
  (rust-lang/rust#115577)
- [Broaden the consequences of recursive TLS initialization.]
  (rust-lang/rust#116172)
- [Windows: Support sub-millisecond sleep.]
  (rust-lang/rust#116461)
- [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl]
  (rust-lang/rust#100806)
- [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.]
  (rust-lang/rust#115108)

Stabilized APIs
---------------

- [`Atomic*::from_ptr`]
  (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr)
- [`FileTimes`]
  (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html)
- [`FileTimesExt`]
  (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html)
- [`File::set_modified`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified)
- [`File::set_times`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times)
- [`IpAddr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical)
- [`Ipv6Addr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical)
- [`Option::as_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice)
- [`Option::as_mut_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice)
- [`pointer::byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add)
- [`pointer::byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset)
- [`pointer::byte_offset_from`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from)
- [`pointer::byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub)
- [`pointer::wrapping_byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add)
- [`pointer::wrapping_byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset)
- [`pointer::wrapping_byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub)

These APIs are now stable in const contexts:

- [`Ipv6Addr::to_ipv4_mapped`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped)
- [`MaybeUninit::assume_init_read`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read)
- [`MaybeUninit::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed)
- [`mem::discriminant`]
  (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html)
- [`mem::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html)

Cargo
-----

- [Add new packages to `[workspace.members]` automatically.]
  (rust-lang/cargo#12779)
- [Allow version-less `Cargo.toml` manifests.]
  (rust-lang/cargo#12786)
- [Make browser links out of HTML file paths.]
  (rust-lang/cargo#12889)

Rustdoc
-------

- [Accept less invalid Rust in rustdoc.]
  (rust-lang/rust#117450)
- [Document lack of object safety on affected traits.]
  (rust-lang/rust#113241)
- [Hide `#[repr(transparent)]` if it isn't part of the public ABI.]
  (rust-lang/rust#115439)
- [Show enum discriminant if it is a C-like variant.]
  (rust-lang/rust#116142)

Compatibility Notes
-------------------

- [FreeBSD targets now require at least version 12.]
  (rust-lang/rust#114521)
- [Formally demote tier 2 MIPS targets to tier 3.]
  (rust-lang/rust#115238)
- [Make misalignment a hard error in `const` contexts.]
  (rust-lang/rust#115524)
- [Fix detecting references to packed unsized fields.]
  (rust-lang/rust#115583)
- [Remove support for compiler plugins.]
  (rust-lang/rust#116412)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants