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

Add impl Div<NonZeroU{0}> for u{0} which cannot panic #79134

Merged
merged 3 commits into from
Dec 27, 2020

Conversation

ohadravid
Copy link
Contributor

@ohadravid ohadravid commented Nov 17, 2020

Dividing an unsigned int by a NonZeroUxx requires a user to write (for example, in this SO question):

pub fn safe_div(x: u32, y: std::num::NonZeroU32) -> u32 {
    x / y.get()
}

which generates a panicking-checked-div assembly.
Avoiding the panic currently requires unsafe code.

This PR adds an impl Div<NonZeroU{0}> for u{0} (and impl Rem<NonZeroU{0}> for u{0}) which calls the unchecked_div (and unchecked_rem) intrinsic without any additional checks,
making the following code compile:

pub fn safe_div(x: u32, y: std::num::NonZeroU32) -> u32 {
    x / y
}

pub fn safe_rem(x: u32, y: std::num::NonZeroU32) -> u32 {
    x % y
}

The doc is set to match the regular div impl docs.

I've marked these as stable because (as I understand it) trait impls are automatically stable. I'm happy to change it to unstable if needed.

Following @dtolnay template from a similar issue:
this adds the following stable impls, which rely on dividing unsigned integers by nonzero integers being well defined and previously would have involved unsafe code to encode that knowledge:

impl Div<NonZeroU8> for u8 {
    type Output = u8;
}

impl Rem<NonZeroU8> for u8 {
    type Output = u8;
}

and equivalent for u16, u32, u64, u128, usize, but not for i8, i16, i32, i64, i128, isize (since -1/MIN is undefined).

r? @dtolnay

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2020
@ohadravid ohadravid force-pushed the nzint-div branch 3 times, most recently from 9416a01 to 41da9ba Compare November 17, 2020 13:01
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 17, 2020
@leonardo-m
Copy link

I could use % (Rem) too...

@leonardo-m
Copy link

See also: #72762

@bors
Copy link
Contributor

bors commented Nov 18, 2020

☔ The latest upstream changes (presumably #79167) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 20, 2020
library/core/src/num/nonzero.rs Outdated Show resolved Hide resolved
@dtolnay
Copy link
Member

dtolnay commented Nov 21, 2020

@rust-lang/libs:
@rfcbot fcp merge

This PR adds the following stable impls as a zero-overhead safe API around std::intrinsics::unchecked_div and std::intrinsics::unchecked_rem. They allow dividing unsigned integers by nonzero integers in a way that is statically known to be non-panicking. Previously it would have taken unsafe code to encode that knowledge.

impl Div<NonZeroU8> for u8 {
    type Output = u8;
}

impl Rem<NonZeroU8> for u8 {
    type Output = u8;
}

// and equivalent for u16, u32, u64, u128, usize

but not for i8, i16, i32, i64, i128, isize (since MIN/-1 is fallible).

@rfcbot
Copy link

rfcbot commented Nov 21, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 21, 2020
@ohadravid
Copy link
Contributor Author

ping @Amanieu / @BurntSushi ? 🙏

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 16, 2020
@rfcbot
Copy link

rfcbot commented Dec 16, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mqudsi
Copy link
Contributor

mqudsi commented Dec 16, 2020

Just because I got the notification for the final comment period: I opened an RFC in advance of a pull request to do exactly the same just a week ago - this issue had flown under the radar and slipped through my searches.

This is basically identical to the same findings and implementation I had come up with and is a pretty obvious win.

(Nitpick: There are a few comments that are hard-wrapped after just a few words, and "checked by because" -> "checked because")

👍

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 26, 2020
@rfcbot
Copy link

rfcbot commented Dec 26, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@ohadravid ohadravid requested a review from dtolnay December 27, 2020 11:00
@dtolnay
Copy link
Member

dtolnay commented Dec 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 27, 2020

📌 Commit 9586643 has been approved by dtolnay

@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 Dec 27, 2020
@bors
Copy link
Contributor

bors commented Dec 27, 2020

⌛ Testing commit 9586643 with merge 2fab321...

@bors
Copy link
Contributor

bors commented Dec 27, 2020

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 2fab321 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 27, 2020
@bors bors merged commit 2fab321 into rust-lang:master Dec 27, 2020
@rustbot rustbot added this to the 1.51.0 milestone Dec 27, 2020
@leonardo-m
Copy link

It doesn't work in const functions:

const fn safe_div(x: u32, y: std::num::NonZeroU32) -> u32 { x / y }

@jplatte
Copy link
Contributor

jplatte commented Dec 28, 2020

@leonardo-m This is not at all specific to these new trait implementations, you can't use trait methods at all in const fn right now. There's an RFC for allowing this here: rust-lang/rfcs#2632

The only reason any arithmetic works in const fn is that the desugaring of arithmetic operators to trait methods is somewhat of a lie: The compiler does a different translation for most (all?) operators applied to only primitive types. Since the NonZero* types are not primitives, that doesn't apply in this case.

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 31, 2020
@leonardo-m
Copy link

leonardo-m commented Jan 1, 2021

You may add a TODO, so once that RFC is usable in the std lib, you can improve this code.

By the way, this more efficient Mod has improved the performance of some functions in my code almost by 10% (those division by zero tests take some time).

@leonardo-m
Copy link

Also DivAssign<NonZeroUxx> is missing.

@CDirkx CDirkx mentioned this pull request Mar 24, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 24, 2021
Update RELEASES.md

A couple of things that were missing in the release notes:

- `Div` and `Rem` by their `NonZero` variant is now implemented for all unsigned integers (rust-lang#79134)
- Stabilization of `VecDeque::range` and `VecDeque::range_mut` (rust-lang#79022, stabilization version corrected to 1.51.0 rust-lang#80448)
- Deprecation of `spin_loop_hint` (rust-lang#80966)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 12, 2021
Update RELEASES.md

A couple of things that were missing in the release notes:

- `Div` and `Rem` by their `NonZero` variant is now implemented for all unsigned integers (rust-lang#79134)
- Stabilization of `VecDeque::range` and `VecDeque::range_mut` (rust-lang#79022, stabilization version corrected to 1.51.0 rust-lang#80448)
- Deprecation of `spin_loop_hint` (rust-lang#80966)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 13, 2021
Update RELEASES.md

A couple of things that were missing in the release notes:

- `Div` and `Rem` by their `NonZero` variant is now implemented for all unsigned integers (rust-lang#79134)
- Stabilization of `VecDeque::range` and `VecDeque::range_mut` (rust-lang#79022, stabilization version corrected to 1.51.0 rust-lang#80448)
- Deprecation of `spin_loop_hint` (rust-lang#80966)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 26, 2021
Pkgsrc changes:
 * Add support for the big-endian arm64 NetBSD target (aarch64_be).
 * On NetBSD/i386, use the i586 (pentium) bootstrap kit variant in
   preference to i686.
 * Adjust patches, re-compute line offsets, re-compute crate checksums.
 * Remove a patch which was either integrated upstream and/or no longer
   applies.
 * Bump bootstraps to 1.50.0.
 * Move conditionals until after bsd.prefs.mk so that they work...
 * Default to "dist" build target if cross-compiling, but allow
   also to override via rust.BUILD_TARGET.
 * Allow overriding MAKE_JOBS_SAFE via rust.MAKE_JOBS_SAFE if you
   want a different trade-off between occasional breakage and performance.
 * Adjust platform.mk according to work already done in wip/rust/
 * Add a patch to optimize the install.sh script used to install binary
   bootstraps to not do so many forks; use case/esac and parameter expansion
   instead of grep, sed and cut.
 * Drop building documentation for the binary bootstrap kits.  This will
   also impact the lang/rust-bin package.  For full documentation, build
   or install lang/rust as a package.

Upstream changes:

Version 1.51.0 (2021-03-25)
============================

Language
--------
- [You can now parameterize items such as functions, traits, and
  `struct`s by constant values in addition to by types and
  lifetimes.][79135] Also known as "const generics" E.g. you can
  now write the following. Note:  Only values of primitive integers,
  `bool`, or `char` types are currently permitted.
  ```rust
  struct GenericArray<T, const LENGTH: usize> {
      inner: [T; LENGTH]
  }

  impl<T, const LENGTH: usize> GenericArray<T, LENGTH> {
      const fn last(&self) -> Option<&T> {
          if LENGTH == 0 {
              None
          } else {
              Some(&self.inner[LENGTH - 1])
          }
      }
  }
  ```

Compiler
--------

- [Added the `-Csplit-debuginfo` codegen option for macOS platforms.][79570]
  This option controls whether debug information is split across
  multiple files or packed into a single file. **Note** This option
  is unstable on other platforms.
- [Added tier 3\* support for `aarch64_be-unknown-linux-gnu`,
  `aarch64-unknown-linux-gnu_ilp32`, and
  `aarch64_be-unknown-linux-gnu_ilp32` targets.][81455]

- [Added tier 3 support for `i386-unknown-linux-gnu` and
  `i486-unknown-linux-gnu` targets.][80662]

- [The `target-cpu=native` option will now detect individual features
  of CPUs.][80749]

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

Libraries
---------

- [`Box::downcast` is now also implemented for any `dyn Any + Send
  + Sync` object.][80945]
- [`str` now implements `AsMut<str>`.][80279]
- [`u64` and `u128` now implement `From<char>`.][79502]
- [`Error` is now implemented for `&T` where `T` implements `Error`.][75180]
- [`Poll::{map_ok, map_err}` are now implemented for
  `Poll<Option<Result<T,E>>>`.][80968]
- [`unsigned_abs` is now implemented for all signed integer types.][80959]
- [`io::Empty` now implements `io::Seek`.][78044]
- [`rc::Weak<T>` and `sync::Weak<T>`'s methods such as `as_ptr`
  are now implemented for `T: ?Sized` types.][80764]
- [`Div` and `Rem` by their `NonZero` variant is now implemented
  for all unsigned integers.][79134]

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

- [`Arc::decrement_strong_count`]
- [`Arc::increment_strong_count`]
- [`Once::call_once_force`]
- [`Peekable::next_if_eq`]
- [`Peekable::next_if`]
- [`Seek::stream_position`]
- [`array::IntoIter`]
- [`panic::panic_any`]
- [`ptr::addr_of!`]
- [`ptr::addr_of_mut!`]
- [`slice::fill_with`]
- [`slice::split_inclusive_mut`]
- [`slice::split_inclusive`]
- [`slice::strip_prefix`]
- [`slice::strip_suffix`]
- [`str::split_inclusive`]
- [`sync::OnceState`]
- [`task::Wake`]
- [`VecDeque::range`]
- [`VecDeque::range_mut`]

Cargo
-----
- [Added the `split-debuginfo` profile option to control the -Csplit-debuginfo
  codegen option.][cargo/9112]
- [Added the `resolver` field to `Cargo.toml` to enable the new
  feature resolver and CLI option behavior.][cargo/8997] Version
  2 of the feature resolver will try to avoid unifying features of
  dependencies where that unification could be unwanted.  Such as
  using the same dependency with a `std` feature in a build scripts
  and proc-macros, while using the `no-std` feature in the final
  binary. See the [Cargo book documentation][feature-resolver@2.0]
  for more information on the feature.

Rustdoc
-------

- [Rustdoc will now include documentation for methods available
  from _nested_ `Deref` traits.][80653]
- [You can now provide a `--default-theme` flag which sets the
  default theme to use for documentation.][79642]

Various improvements to intra-doc links:

- [You can link to non-path primitives such as `slice`.][80181]
- [You can link to associated items.][74489]
- [You can now include generic parameters when linking to items,
  like `Vec<T>`.][76934]

Misc
----
- [You can now pass `--include-ignored` to tests (e.g. with
  `cargo test -- --include-ignored`) to include testing tests marked
  `#[ignore]`.][80053]

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

- [WASI platforms no longer use the `wasm-bindgen` ABI, and instead
  use the wasm32 ABI.][79998]
- [`rustc` no longer promotes division, modulo and indexing operations
  to `const` that could fail.][80579]
- [The minimum version of glibc for the following platforms has
  been bumped to version 2.31 for the distributed artifacts.][81521]
    - `armv5te-unknown-linux-gnueabi`
    - `sparc64-unknown-linux-gnu`
    - `thumbv7neon-unknown-linux-gnueabihf`
    - `armv7-unknown-linux-gnueabi`
    - `x86_64-unknown-linux-gnux32`
- [`atomic::spin_loop_hint` has been deprecated.][80966] It's
  recommended to use `hint::spin_loop` instead.

Internal Only
-------------

- [Consistently avoid constructing optimized MIR when not doing codegen][80718]

[79135]: rust-lang/rust#79135
[74489]: rust-lang/rust#74489
[76934]: rust-lang/rust#76934
[79570]: rust-lang/rust#79570
[80181]: rust-lang/rust#80181
[79642]: rust-lang/rust#79642
[80945]: rust-lang/rust#80945
[80279]: rust-lang/rust#80279
[80053]: rust-lang/rust#80053
[79502]: rust-lang/rust#79502
[75180]: rust-lang/rust#75180
[79135]: rust-lang/rust#79135
[81521]: rust-lang/rust#81521
[80968]: rust-lang/rust#80968
[80959]: rust-lang/rust#80959
[80718]: rust-lang/rust#80718
[80653]: rust-lang/rust#80653
[80579]: rust-lang/rust#80579
[79998]: rust-lang/rust#79998
[78044]: rust-lang/rust#78044
[81455]: rust-lang/rust#81455
[80764]: rust-lang/rust#80764
[80749]: rust-lang/rust#80749
[80662]: rust-lang/rust#80662
[79134]: rust-lang/rust#79134
[80966]: rust-lang/rust#80966
[cargo/8997]: rust-lang/cargo#8997
[cargo/9112]: rust-lang/cargo#9112
[feature-resolver@2.0]: https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-resolver-version-2
[`Once::call_once_force`]: https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.call_once_force
[`sync::OnceState`]: https://doc.rust-lang.org/stable/std/sync/struct.OnceState.html
[`panic::panic_any`]: https://doc.rust-lang.org/stable/std/panic/fn.panic_any.html
[`slice::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.strip_prefix
[`slice::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.strip_prefix
[`Arc::increment_strong_count`]: https://doc.rust-lang.org/nightly/std/sync/struct.Arc.html#method.increment_strong_count
[`Arc::decrement_strong_count`]: https://doc.rust-lang.org/nightly/std/sync/struct.Arc.html#method.decrement_strong_count
[`slice::fill_with`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.fill_with
[`ptr::addr_of!`]: https://doc.rust-lang.org/nightly/std/ptr/macro.addr_of.html
[`ptr::addr_of_mut!`]: https://doc.rust-lang.org/nightly/std/ptr/macro.addr_of_mut.html
[`array::IntoIter`]: https://doc.rust-lang.org/nightly/std/array/struct.IntoIter.html
[`slice::split_inclusive`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.split_inclusive
[`slice::split_inclusive_mut`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.split_inclusive_mut
[`str::split_inclusive`]: https://doc.rust-lang.org/nightly/std/primitive.str.html#method.split_inclusive
[`task::Wake`]: https://doc.rust-lang.org/nightly/std/task/trait.Wake.html
[`Seek::stream_position`]: https://doc.rust-lang.org/nightly/std/io/trait.Seek.html#method.stream_position
[`Peekable::next_if`]: https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if
[`Peekable::next_if_eq`]: https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if_eq
[`VecDeque::range`]: https://doc.rust-lang.org/nightly/std/collections/struct.VecDeque.html#method.range
[`VecDeque::range_mut`]: https://doc.rust-lang.org/nightly/std/collections/struct.VecDeque.html#method.range_mut
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.