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

Begin stripping down core::num #18827

Closed
wants to merge 16 commits into from

Conversation

brendanzab
Copy link
Member

This implements a considerable portion of rust-lang/rfcs#369 (tracked in #18640). Some interpretations had to be made in order to get this to work. The breaking changes are listed below:

[breaking-change]

  • core::num::{Num, Unsigned, Primitive} have been deprecated and their re-exports removed from the {std, core}::prelude.
  • core::num::{Zero, One, Bounded} have been deprecated. Use the static methods on core::num::{Float, Int} instead. There is no equivalent to Zero::is_zero. Use (==) with {Float, Int}::zero instead.
  • Signed::abs_sub has been moved to std::num::FloatMath, and is no longer implemented for signed integers.
  • core::num::Signed has been removed, and its methods have been moved to core::num::Float and a new trait, core::num::SignedInt. The methods now take the self parameter by value.
  • core::num::{Saturating, CheckedAdd, CheckedSub, CheckedMul, CheckedDiv} have been removed, and their methods moved to core::num::Int. Their parameters are now taken by value. This means that
  • std::time::Duration no longer implements core::num::{Zero, CheckedAdd, CheckedSub} instead defining the required methods non-polymorphically.
  • core::num::{zero, one, abs, signum} have been deprecated. Use their respective methods instead.
  • The core::num::{next_power_of_two, is_power_of_two, checked_next_power_of_two} functions have been deprecated in favor of methods defined a new trait, core::num::UnsignedInt
  • core::iter::{AdditiveIterator, MultiplicativeIterator} are now only implemented for the built-in numeric types.
  • core::iter::{range, range_inclusive, range_step, range_step_inclusive} now require core::num::Int to be implemented for the type they a re parametrized over.

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@brendanzab
Copy link
Member Author

Note, I am still running some tests locally, there might be some breakages - I will rebase in a bit. Apologies for the size of this.

cc. @aturon

@thestinger
Copy link
Contributor

The standard library can't just reserve all of these useful method names in the prelude without a plan for support of big integers, decimals and arbitrary precision floats. It would no longer be possible to write a generic number library with sane method names.

@brendanzab
Copy link
Member Author

@thestinger We have removed the traits from the prelude in the interests of future extensibility.

@thestinger
Copy link
Contributor

The Signed trait is still there, and it's reserving a definition of abs that's not sensible for types like big integers. There is a use case for a method performing the operation in-place (&mut self) and a method producing a new value, but it forces a split into 3 methods for all time.

@thestinger
Copy link
Contributor

@bjz: It shouldn't be done via traits if it's not meant to be used for generic programming.

@brendanzab
Copy link
Member Author

Re. Signed, this is true. I think it should probably be removed from the prelude.

@thestinger
Copy link
Contributor

I can't see how intentionally poor library design makes sense regardless of what's in the prelude. If doing it properly is considered too verbose, then that's a language problem. In C++, it's possible to write code using references and values based on the semantics required for all types without adding any verbosity for primitive types. I think fixing a serious regression from C++ should have been considered as an alternative to crippling support for non-primitive number types.

@pcwalton
Copy link
Contributor

@thestinger Objecting to the design of the language is off-topic for this PR. Please file an RFC if you have any concrete changes to the trait system that you would like to make.

@alexcrichton
Copy link
Member

r? @aturon

@aturon
Copy link
Member

aturon commented Nov 10, 2014

@bjz Thanks so much for doing this (and so quickly)! I'll review it shortly.

@aturon
Copy link
Member

aturon commented Nov 10, 2014

@bjz This is really nice work! I left a few minor comments, but overall this is looking good to me.

I would be fine with dropping Signed from the prelude as well; we can file an amendment RFC with implementation tweaks like that.

One other thing I'm uneasy about is the change to AdditiveIterator and MultiplicativeIterator. To be honest, I'm not sure that it's worth keeping these traits around given how trivially they wrap fold. (And including the seed explicitly means they're even less sugary.) An alternative would be to keep them in their original form, but with impls for each of the primitive numeric types. (Blanket impls for both Int and Float will fail coherence.) Or we could just drop them outright. What do you think?

@Gankra
Copy link
Contributor

Gankra commented Nov 10, 2014

I would vote for dropping them. With UFCS and your proposed operator cleanup you should be able to do something like iter.fold(0, Add::add), right? That's pretty nice imo.

@aturon
Copy link
Member

aturon commented Nov 10, 2014

@gankro good point!

@Gankra
Copy link
Contributor

Gankra commented Nov 10, 2014

Also if signed is leaving the prelude, do we even need it? I thought its whole justification in the RFC was to provide some prelude functionality?

@thestinger
Copy link
Contributor

@pcwalton: I don't see how it's off-topic. I'm pointing out that this is a major regression for types like big integers and that alternatives were not considered. I haven't proposed any changes to the trait system.

@aturon
Copy link
Member

aturon commented Nov 10, 2014

@gankro

Also if signed is leaving the prelude, do we even need it? I thought its whole justification in the RFC was to provide some prelude functionality?

It also serves to cut down a bit of duplication between the int and float traits. You could instead have the methods separately on Float and a new SignedInt trait, but you still wind up with the same number of traits over all (but more methods) so I'm not sure it's worth it.

@Gankra
Copy link
Contributor

Gankra commented Nov 11, 2014

I still favour either the denormalization of

  • Int
    • Signed Int
    • Unsigned Int
  • Float

or

  • Unsigned Int
  • Signed Int
  • Float

As this seems like a more natural grouping. The method duplication argument doesn't seem very convincing for a bunch of extension traits that will never be simultaneously impl'd by one type. It's fairly minor maintenance to have the same methods on two traits.

@thestinger
Copy link
Contributor

These traits include many methods that are not useful outside of generic code. For example, there are a large number of methods returning constants without a use case outside of generics. The ability to write code that's generic over number types is clearly useful, and there are even cases in the standard library. It's currently possible to use types like big integers with range, range_inclusive, range_step, range_step_inclusive and count among others.

The existing good practice in traits and generics is to take &T when ownership of the value is not required. It is not a performance problem because argument promotion and inlining takes care of it. If the function is too long to be inlined then the passing convention is not going to matter. This allows generic code to work with both primitives and types like big integers. The rest of the standard library sticks with this design, even though auto-referencing sugar isn't pervasive.

For example, searching for an integer key in a map type looks like map.find(&5) because auto-referencing only exists for self and various operators rather than any parameters. I don't think it makes sense to intentionally regress functionality with big integers in order to work around that lack of sugar. If the lack of sugar is a problem, then that should be addressed. The same warts that apply to numeric methods with parameters other than self also apply throughout the standard library.

Especially with traits like Add, providing an implementation for &T with big integers rather than the trait using by-reference would destroy the ability to write generic code. Types like big integers are already far more second-class in Rust than in C++ and it's quite sad to see things regressing even further. These issues and many more were not brought up in the proposal.

@pcwalton
Copy link
Contributor

@thestinger A meta-point: Your objections were specific to the prelude and were raised 5-6 days after the RFC was accepted. Procedural objections aren't going to work this time.

These issues and many more were not brought up in the proposal.

Yes, they were. Big integers were specifically called out as being out of scope of the Rust standard library's trait hierarchy. Your objections were specific to the prelude and covered by the fact that these traits are not in it.

Especially with traits like Add, providing an implementation for &T with big integers rather than the trait using by-reference would destroy the ability to write generic code.

Nobody said anything about Add. Big integers can certainly continue to implement it. They just won't implement the standard library's Num.

@thestinger
Copy link
Contributor

A meta-point: Your objections were specific to the prelude and were raised 5-6 days after the RFC was accepted. Procedural objections aren't going to work this time.

There are often unforeseen consequences to a proposal, as pointed out in the pull request itself: "Some interpretations had to be made in order to get this to work.". The fact that I'm raising concerns here doesn't make those concerns any more or less valid.

As the author of rust-gmp and one of the people who was involved in previous number design discussions, it should not be a surprise that I have an opinion about it. I need watch the pull request queue, because many major changes do not go through the RFC process. I don't have the time to read through every proposal too, especially since only a fraction are accepted.

Yes, they were. Big integers were specifically called out as being out of scope of the Rust standard library's trait hierarchy. Your objections were specific to the prelude and covered by the fact that these traits are not in it.

The Signed trait was in the prelude when I made that comment. You sure love cherry-picking my statements and misrepresenting them.

Nobody said anything about Add. Big integers can certainly continue to implement it. They just won't implement the standard library's Num.

I'm stating that a different implementation of Add for big integers and primitives would mean no mixing them in generic code. It's an argument against the suggestion that they should take the parameters by-value instead of by-reference.

@thestinger
Copy link
Contributor

Yes, they were. Big integers were specifically called out as being out of scope of the Rust standard library's trait hierarchy. Your objections were specific to the prelude and covered by the fact that these traits are not in it.

There are plenty of issues in my comment that were not brought up in the RFC and there is no mention of the prelude here:

These traits include many methods that are not useful outside of generic code. For example, there are a large number of methods returning constants without a use case outside of generics. The ability to write code that's generic over number types is clearly useful, and there are even cases in the standard library. It's currently possible to use types like big integers with range, range_inclusive, range_step, range_step_inclusive and count among others.

The existing good practice in traits and generics is to take &T when ownership of the value is not required. It is not a performance problem because argument promotion and inlining takes care of it. If the function is too long to be inlined then the passing convention is not going to matter. This allows generic code to work with both primitives and types like big integers. The rest of the standard library sticks with this design, even though auto-referencing sugar isn't pervasive.

For example, searching for an integer key in a map type looks like map.find(&5) because auto-referencing only exists for self and various operators rather than any parameters. I don't think it makes sense to intentionally regress functionality with big integers in order to work around that lack of sugar. If the lack of sugar is a problem, then that should be addressed. The same warts that apply to numeric methods with parameters other than self also apply throughout the standard library.

Especially with traits like Add, providing an implementation for &T with big integers rather than the trait using by-reference would destroy the ability to write generic code. Types like big integers are already far more second-class in Rust than in C++ and it's quite sad to see things regressing even further. These issues and many more were not brought up in the proposal.

No one brought up these issues on the RFC or mentioned various alternatives at both the language and library level without the same negative consequences. That's what happens when the discussion only involves the people with one perspective on the issue.

@pcwalton
Copy link
Contributor

The fact that I'm raising concerns here doesn't make those concerns any more or less valid.

That's true. You can't point to procedural issues this time, however; the RFC process took place, you did not take part in it, and now you're raising objections on the PR while simultaneously unfairly implying that there is a problem with the RFC process.

There are plenty of issues in my comment that were not brought up in the RFC and there is no mention of the prelude here:

I was referring to your comments in the RFC.

The Signed trait was in the prelude when I made that comment.

To me that was clearly just a mistake with the RFC, otherwise @aturon's reply would have not made sense.

That's what happens when the discussion only involves the people with one perspective on the issue.

This is what I mean by bringing up procedural issues. By your own admission, you didn't "have time to" to take part in the RFC. So you can't bring up procedural issues if you didn't bother to get involved. We followed the procedure to the letter.

I'm stating that a different implementation of Add for big integers and primitives would mean no mixing them in generic code. It's an argument against the suggestion that they should take the parameters by-value instead of by-reference.

I don't understand this. Big integers and primitives do have to have different implementations of Add. They're different types, after all…

@brendanzab brendanzab deleted the rfc369-numerics branch November 14, 2014 07:58
japaric pushed a commit to japaric-archived/stats.rs that referenced this pull request Nov 14, 2014
mtsr added a commit to mtsr/noise-rs that referenced this pull request Nov 15, 2014
brendanzab added a commit to Razaekel/noise-rs that referenced this pull request Nov 15, 2014
mtsr added a commit to mtsr/gfx-rs that referenced this pull request Nov 15, 2014
brendanzab added a commit to gfx-rs/gfx that referenced this pull request Nov 15, 2014
@ghost ghost mentioned this pull request Nov 15, 2014
@yuriks
Copy link
Contributor

yuriks commented Nov 16, 2014

What is the solution here for code that needs to abstract over both integers and floats? Even defining your own Num trait doesn't work well, since the following isn't possible in Rust at the moment:

impl<T: Int> Num for T { /* ... */ }
impl<T: Float> Num for T { /* ... */ }

I don't see any way of working around this problem except for copious use of macros to stamp out implementations, which kind of defeats the purpose of having compile-time polymorphism.

@brendanzab
Copy link
Member Author

One solution might be to resurrect https://github.com/bjz/num-rs/

@aturon
Copy link
Member

aturon commented Nov 16, 2014

@yuriks The intersection of Int and Float is basically just a list of the operator traits; it's straightforward to write your own Num as outlined in the RFC. You can always provide a blanket impl for Int and then f32 and f64; each impl should be about 6 lines of code.

We did consider having a Num trait still, as the "alternatives" link above mentions, but in the end it was determined that we should only provide generic programming over primitive numeric types of the same kind but differing size. Part of the idea is to leave numeric hierarchies to external libraries, since there's not a canonical way to do this.

@brendanzab
Copy link
Member Author

I'm currently working on getting num-rs back up to scratch :)

@aturon
Copy link
Member

aturon commented Nov 16, 2014

@bjz Delighted to hear it!

japaric pushed a commit to japaric-archived/simplot.rs that referenced this pull request Nov 16, 2014
japaric pushed a commit to bheisler/criterion.rs that referenced this pull request Nov 16, 2014
japaric pushed a commit to japaric-archived/parallel.rs that referenced this pull request Nov 16, 2014
Ms2ger added a commit to servo/euclid that referenced this pull request Nov 24, 2014
It was removed from the prelude as part of <rust-lang/rust#18827>.
Ms2ger added a commit to servo/euclid that referenced this pull request Nov 24, 2014
It was removed from the prelude as part of <rust-lang/rust#18827>.
Ms2ger added a commit to servo/euclid that referenced this pull request Nov 24, 2014
It was removed from the prelude as part of <rust-lang/rust#18827>.
GauravBholaris pushed a commit to GauravBholaris/criterion.rs that referenced this pull request Jul 20, 2022
GauravBholaris pushed a commit to GauravBholaris/criterion.rs that referenced this pull request Jul 20, 2022
GauravBholaris pushed a commit to GauravBholaris/criterion.rs that referenced this pull request Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants