-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 generic NonZero
type.
#100428
Add generic NonZero
type.
#100428
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Version numbers on
|
This comment has been minimized.
This comment has been minimized.
b772c7c
to
567ea1a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This simplifies my code. |
This becomes useless then? |
I am really excited about this! One quick question - why is |
@MomoLangenstein, |
@reitermarkus Thanks for the explanation! Would it be useful to also allow wrapping |
I don't think so, a non-zero |
0992019
to
9ee03cd
Compare
This comment has been minimized.
This comment has been minimized.
2796971
to
0d88d1b
Compare
src/test/ui/binop/issue-28837.rs
Outdated
@@ -15,7 +15,9 @@ fn main() { | |||
|
|||
a & a; //~ ERROR no implementation for `A & A` | |||
|
|||
a | a; //~ ERROR no implementation for `A | A` | |||
a | a; | |||
//~^ ERROR no implementation for `A | NonZero<A>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted this test for now so it passes, but this seems unrelated. Especially
must implement `core::num::nonzero::ZeroablePrimitive`
in the output should not be suggested.
let _ = |A | B: E| (); //~ ERROR no implementation for `E | ()` | ||
let _ = |A | B: E| (); | ||
//~^ ERROR mismatched types | ||
//~^^ ERROR no implementation for `E | NonZero<E>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for this, but this seems to be caused by the same problem related to BitOr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
All the parts outside of nonzero.rs look great to me.
Nonzero.rs is challenging to review in this PR because all of the following are being done simultaneously in that single commit:
- expanding derived impls into handwritten impls
- modifying non-generic associated functions into generic ones
- refactoring macro-generated trait impls
- introducing brand new functionality that wasn't available in the standard library before, namely something to do with nonzero raw pointers, along with new From impls
- changing code inside function bodies, such as the
cfg(debug_assertions)
which appeared insidenew_unchecked
- adding the new struct definition and aliases
- reordering some big blocks of code
- adapting documentation of stable methods to use unstable
NonZero<T>
instead of the original stable types
Overall I feel that I couldn't review this as a diff with sufficient confidence, only by reviewing the end result from scratch. And reviewing the end result, it's unlikely that I would be able to pick up on anything that has been inadvertently added or gone missing, especially as this is the result of some gnarly rebases, it sounds like.
Regarding the raw pointer stuff, I am not interested in merging that in this PR. FWIW that is what is causing that doctest on core::mem::discriminant to fail. There has been a From impl added which didn't exist before this PR, and is causing an inference breakage.
The doc changes on methods like checked_add
also shouldn't merge in this PR, I think. NonZero<T>
is going to be nightly-only for a period of at least a few months. During that time, it's extra important that the documentation shows how to continue using the existing stable functionality using only the stable aliases.
Separately, I noticed from building the documentation with x.py doc library/core
, this ends up being an unexpectedly significant regression in usability of the docs for NonZero, since there end up being 67 separate impl blocks on NonZero<T>
. See the screenshots in #118665.
I think a sequence of reviewable changes to get NonZero<T>
merged would be:
-
Consolidate all associated items on the NonZero integer types into a single impl block per type #118665 so that we don't get the 67 impl blocks in the docs
-
A self-contained change which introduces private type aliases by doing
pub(crate) type NonZero<T> = <T as ZeroablePrimitive>::NonZero
. Note that the type alias here is in the opposite direction as the desired end state, however all the rest of the changes are in the correct direction (replacing code that mentionsNonZeroI8
withNonZero<i8>
, such as incore::convert
, and various macros that need to change$:ident
to$:ty
) so overall this ends up being good forward progress that can land independently (or even in multiple pieces). -
Expand derived impls into handwritten impls in all the cases where they would otherwise hit "borrow of layout constrained field with interior mutability" in step 4. This can land independently.
-
Add the new generic NonZero struct definition, delete the alias from step 2, and add non-generic aliases in the opposite direction. Because of steps 2 and 3, very little standard library code should need to change here. The clippy change would need to go here I think. Also this commit would insert some
unsafe
blocks into the handwritten impls from step 3, but no other changes. Maybe this would be about 1/10 the size of this PR. -
Later during stabilization process: update the example code in docs to use
NonZero<T>
.
I have intentionally left out some of the changes I noted seeing in this PR. In particular:
-
Converting non-generic associated functions into generic ones is not something that needs to happen in order to merge or stabilize NonZero. If a function is in
impl NonZeroI8
today, it should be inimpl NonZero<i8>
after this work, not inimpl<T> NonZero<T>
. That can happen afterward as an independent change, probably after stabilization of NonZero, or in the run-up to stabilization if there is a reason to think there would be a backward compatibility risk in doing it afterward. -
Same thing for trait impls. All the same trait impls should exist before and after the refactor. There shouldn't be new generic
impl<T> ... for NonZero<T>
. As with associated functions, the non-generic trait impls can be converted over to generic ones separately afterward.
…=dtolnay Add private `NonZero<T>` type alias. According to step 2 suggested in rust-lang#100428 (review). This adds a private type alias for `NonZero<T>` so that some parts of the code can already start using `NonZero<T>` syntax. Using `NonZero<T>` for `convert` and other parts which implement `From` doesn't work while it is a type alias, since this results in conflicting implementations.
Rollup merge of rust-lang#119990 - reitermarkus:nonzero-type-alias, r=dtolnay Add private `NonZero<T>` type alias. According to step 2 suggested in rust-lang#100428 (review). This adds a private type alias for `NonZero<T>` so that some parts of the code can already start using `NonZero<T>` syntax. Using `NonZero<T>` for `convert` and other parts which implement `From` doesn't work while it is a type alias, since this results in conflicting implementations.
Consolidate all associated items on the NonZero integer types into a single impl block per type **Before:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... } impl NonZeroI8 { pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... } impl NonZeroI8 { pub const fn abs(self) -> NonZeroI8 ... } ... ``` **After:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... pub const fn abs(self) -> NonZeroI8 ... ... } ``` Having 6-7 different impl blocks per type is not such a problem in today's implementation, but becomes awful upon the switch to a generic `NonZero<T>` type (context: rust-lang#82363 (comment)). In the implementation from rust-lang#100428, there end up being **67** impl blocks on that type. <img src="https://github.com/rust-lang/rust/assets/1940490/5b68bd6f-8a36-4922-baa3-348e30dbfcc1" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2cfec71e-c2cd-4361-a542-487f13f435d9" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2fe00337-7307-405d-9036-6fe1e58b2627" width="200"> Without the refactor to a single impl block first, introducing `NonZero<T>` would be a usability regression compared to today's separate pages per type. With all those blocks expanded, Ctrl+F is obnoxious because you need to skip 12× past every match you don't care about. With all the blocks collapsed, Ctrl+F is useless. Getting to a state in which exactly one type's (e.g. `NonZero<u32>`) impl blocks are expanded while the rest are collapsed is annoying. After this refactor to a single impl block, we can move forward with making `NonZero<T>` a generic struct whose docs all go on the same rustdoc page. The rustdoc will have 12 impl blocks, one per choice of `T` supported by the standard library. The reader can expand a single one of those impl blocks e.g. `NonZero<u32>` to understand the entire API of that type. Note that moving the API into a generic `impl<T> NonZero<T> { ... }` is not going to be an option until after `NonZero<T>` has been stabilized, which may be months or years after its introduction. During the period while generic `NonZero` is unstable, it will be extra important to offer good documentation on all methods demonstrating the API being used through the stable aliases such as `NonZeroI8`. This PR follows a `key = $value` syntax for the macros which is similar to the macros we already use for producing a single large impl block on the integer primitives. https://github.com/rust-lang/rust/blob/1dd4db50620fb38a6382c22456a96ed7cddeff83/library/core/src/num/mod.rs#L288-L309 Best reviewed one commit at a time.
Rollup merge of rust-lang#118665 - dtolnay:signedness, r=Nilstrieb Consolidate all associated items on the NonZero integer types into a single impl block per type **Before:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... } impl NonZeroI8 { pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... } impl NonZeroI8 { pub const fn abs(self) -> NonZeroI8 ... } ... ``` **After:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... pub const fn abs(self) -> NonZeroI8 ... ... } ``` Having 6-7 different impl blocks per type is not such a problem in today's implementation, but becomes awful upon the switch to a generic `NonZero<T>` type (context: rust-lang#82363 (comment)). In the implementation from rust-lang#100428, there end up being **67** impl blocks on that type. <img src="https://github.com/rust-lang/rust/assets/1940490/5b68bd6f-8a36-4922-baa3-348e30dbfcc1" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2cfec71e-c2cd-4361-a542-487f13f435d9" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2fe00337-7307-405d-9036-6fe1e58b2627" width="200"> Without the refactor to a single impl block first, introducing `NonZero<T>` would be a usability regression compared to today's separate pages per type. With all those blocks expanded, Ctrl+F is obnoxious because you need to skip 12× past every match you don't care about. With all the blocks collapsed, Ctrl+F is useless. Getting to a state in which exactly one type's (e.g. `NonZero<u32>`) impl blocks are expanded while the rest are collapsed is annoying. After this refactor to a single impl block, we can move forward with making `NonZero<T>` a generic struct whose docs all go on the same rustdoc page. The rustdoc will have 12 impl blocks, one per choice of `T` supported by the standard library. The reader can expand a single one of those impl blocks e.g. `NonZero<u32>` to understand the entire API of that type. Note that moving the API into a generic `impl<T> NonZero<T> { ... }` is not going to be an option until after `NonZero<T>` has been stabilized, which may be months or years after its introduction. During the period while generic `NonZero` is unstable, it will be extra important to offer good documentation on all methods demonstrating the API being used through the stable aliases such as `NonZeroI8`. This PR follows a `key = $value` syntax for the macros which is similar to the macros we already use for producing a single large impl block on the integer primitives. https://github.com/rust-lang/rust/blob/1dd4db50620fb38a6382c22456a96ed7cddeff83/library/core/src/num/mod.rs#L288-L309 Best reviewed one commit at a time.
Consolidate all associated items on the NonZero integer types into a single impl block per type **Before:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... } impl NonZeroI8 { pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... } impl NonZeroI8 { pub const fn abs(self) -> NonZeroI8 ... } ... ``` **After:** ```rust #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] pub struct NonZeroI8(i8); impl NonZeroI8 { pub const fn new(n: i8) -> Option<Self> ... pub const fn get(self) -> i8 ... pub const fn leading_zeros(self) -> u32 ... pub const fn trailing_zeros(self) -> u32 ... pub const fn abs(self) -> NonZeroI8 ... ... } ``` Having 6-7 different impl blocks per type is not such a problem in today's implementation, but becomes awful upon the switch to a generic `NonZero<T>` type (context: rust-lang/rust#82363 (comment)). In the implementation from rust-lang/rust#100428, there end up being **67** impl blocks on that type. <img src="https://github.com/rust-lang/rust/assets/1940490/5b68bd6f-8a36-4922-baa3-348e30dbfcc1" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2cfec71e-c2cd-4361-a542-487f13f435d9" width="200"><img src="https://github.com/rust-lang/rust/assets/1940490/2fe00337-7307-405d-9036-6fe1e58b2627" width="200"> Without the refactor to a single impl block first, introducing `NonZero<T>` would be a usability regression compared to today's separate pages per type. With all those blocks expanded, Ctrl+F is obnoxious because you need to skip 12× past every match you don't care about. With all the blocks collapsed, Ctrl+F is useless. Getting to a state in which exactly one type's (e.g. `NonZero<u32>`) impl blocks are expanded while the rest are collapsed is annoying. After this refactor to a single impl block, we can move forward with making `NonZero<T>` a generic struct whose docs all go on the same rustdoc page. The rustdoc will have 12 impl blocks, one per choice of `T` supported by the standard library. The reader can expand a single one of those impl blocks e.g. `NonZero<u32>` to understand the entire API of that type. Note that moving the API into a generic `impl<T> NonZero<T> { ... }` is not going to be an option until after `NonZero<T>` has been stabilized, which may be months or years after its introduction. During the period while generic `NonZero` is unstable, it will be extra important to offer good documentation on all methods demonstrating the API being used through the stable aliases such as `NonZeroI8`. This PR follows a `key = $value` syntax for the macros which is similar to the macros we already use for producing a single large impl block on the integer primitives. https://github.com/rust-lang/rust/blob/1dd4db50620fb38a6382c22456a96ed7cddeff83/library/core/src/num/mod.rs#L288-L309 Best reviewed one commit at a time.
…lnay Manually implement derived `NonZero` traits. Step 3 as mentioned in rust-lang#100428 (review). Manually implement the traits that would cause “borrow of layout constrained field with interior mutability” errors when switching to `NonZero<T>`. r? `@dtolnay`
…lnay Manually implement derived `NonZero` traits. Step 3 as mentioned in rust-lang#100428 (review). Manually implement the traits that would cause “borrow of layout constrained field with interior mutability” errors when switching to `NonZero<T>`. r? `@dtolnay`
…lnay Manually implement derived `NonZero` traits. Step 3 as mentioned in rust-lang#100428 (review). Manually implement the traits that would cause “borrow of layout constrained field with interior mutability” errors when switching to `NonZero<T>`. r? ``@dtolnay``
…lnay Manually implement derived `NonZero` traits. Step 3 as mentioned in rust-lang#100428 (review). Manually implement the traits that would cause “borrow of layout constrained field with interior mutability” errors when switching to `NonZero<T>`. r? ```@dtolnay```
Rollup merge of rust-lang#120160 - reitermarkus:nonzero-traits, r=dtolnay Manually implement derived `NonZero` traits. Step 3 as mentioned in rust-lang#100428 (review). Manually implement the traits that would cause “borrow of layout constrained field with interior mutability” errors when switching to `NonZero<T>`. r? ```@dtolnay```
…direction, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…direction, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…rection, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…rection, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…rection, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…rection, r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang#100428 (review). Depends on rust-lang#120160. r? `@dtolnay`
…r=dtolnay Switch `NonZero` alias direction. Step 4 mentioned in rust-lang/rust#100428 (review). Depends on rust-lang/rust#120160. r? `@dtolnay`
Add generic
NonZero
type as suggested in #82363.Currently, only the type itself is generic, the implementation is still done individually for each type with the existing macros.
It should be possible to implement most methods generically, but this would be incompatible with the stability attributes since unsigned and signed non-zero types were stabilized in different versions. Not sure what the best way to deal with this is.
r? @joshtriplett