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 cast function to primitive integers #42456

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

This commit is a result of the libs team's discussion of #33417 and how it
affects integral types. The conclusion was that the motivation for converting
integral types, working in a cross-platform code that uses platform-specific
integer types, was different enough from the intent of TryFrom that it doesn't
make sense to unify the paths. As a result this is a proposal for the
alternative version of the API which purely works with integral types.

An unstable Cast trait is added as the implementation detail of this API, and
otherwise with this you should be able to call i32::cast(0u8) at will. The
intention is then to call this in platform-specific contexts like:

// Convert from C to Rust
let a: c_int = ...;
let b: u32 = u32::cast(a)?;

// Convert from Rust to C
let a: u32 = ...;
let b: c_int = <c_int>::cast(a)?;

Everything here is unstable for now, but the intention is that this will
stabilize sooner than TryFrom.

@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@alexcrichton
Copy link
Member Author

cc @rust-lang/libs

Note I haven't allocated a tracking issue for this yet, wanted to discuss a bit to make sure we're all on board and then I'll open a tracking issue!

/// not be performed losslessly.
#[unstable(feature = "num_cast", issue = "0")]
#[derive(Debug)]
pub struct CastError(());
Copy link
Member

Choose a reason for hiding this comment

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

Is this separate error really needed? Why not an option based API? The ? operator is about to work with options as well, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention is that you can define From<CastError> for MyError and have ? work in result-based functions, whereas in option base dfunctions you can just use cast(..).ok()?

Copy link
Member

Choose a reason for hiding this comment

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

IDK, for me the operation feels similar to checked_add and that returns an option as well. Or is the policy to return Result everywhere for new APIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pretty difficult to create a blanket policy that works everywhere, but this is intended for use in APIs that are already dealing with calls to libc and such and are almost guaranteed to already be returning Result.

#[unstable(feature = "num_cast", issue = "0")]
impl Cast<$unsigned> for $signed {
fn cast(u: $unsigned) -> Result<$signed, CastError> {
let max = <$signed>::max_value() as u128;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using u128 here I think we should rather implement a (non public) trait Signed for each unsigned type that has an assoc type with the corresponding unsigned type, and then perform conversion via <$signed>::Unsigned. At least as long as we can't prove that the u128 conversion is optimized away.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is currently just copying the implementation for TryFrom which has been in use for quite some time.

@nagisa
Copy link
Member

nagisa commented Jun 5, 2017

IMO cast as a name sounds fairly weird when it is not a method that casts Self into something else. It is doubly weird that although we have as (which is a language construct for casting) which can convert from floats, this function, being named cast, cannot.

@petrochenkov
Copy link
Contributor

The conclusion was that the motivation for converting
integral types, working in a cross-platform code that uses platform-specific
integer types, was different enough from the intent of TryFrom that it doesn't
make sense to unify the paths.

My personal take on the a fallible conversion trait is that I think it'd almost replace the need for these methods altogether.
TryFrom and TryInto should help tie us over with fallible conversions in the meantime

An unstable Cast trait is added as the implementation detail of this API

I don't think I've frequently really wanted a trait here so much as just some methods. Sure a trait is pretty nice for generic programming, but the standard library is exceptionally conservative in generic programming over numerics today already, so we may want to continue that trend.

Are you sure you are the same @alexcrichton and library team as year ago?


@nagisa

IMO cast as a name sounds fairly weird when it is not a method that casts Self into something else. It is doubly weird that although we have as (which is a language construct for casting) which can convert from floats, this function, being named cast, cannot.

rust-lang/rfcs#1218 contains some detailed discussion about this stuff (see the commit history as well), and methods that cast Self. Maybe another year later the design will finally converge to what that RFC proposed.

@alexcrichton
Copy link
Member Author

@nagisa I'm not in love with the name, I'd be down for other suggestions!

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2017
@steveklabnik
Copy link
Member

Are you sure you are the same @alexcrichton and library team as year ago?

A year is a long time, and people can certainly change opinions in that amount of time.

@sfackler
Copy link
Member

sfackler commented Jun 6, 2017

@petrochenkov What is the purpose of that comment? What action would you like to see taken?

@nagisa
Copy link
Member

nagisa commented Jun 7, 2017

@alexcrichton how about new? That name would make me a lot of sense :)

@aturon
Copy link
Member

aturon commented Jun 7, 2017

@petrochenkov Indeed, it's seeming like your RFC had it right. It's not the first time we've ended up long delaying one proposal on the basis of thinking that something more general would suffice, only to have it not really work out in the end. And I'm sure it won't be the last!

To be honest, by this point I had totally forgotten about your RFC, and am glad you've re-raised it! We should definitely revisit the API you proposed as an alternative before landing this one. I'm particularly curious about the choice of having cast panic on debug builds, as you proposed, versus only providing the checked variant, as this PR does.

cc @rust-lang/libs

($storage:ty, $target:ty, $($source:ty),*) => {$(
#[unstable(feature = "num_cast", issue = "0")]
impl Cast<$source> for $target {
fn cast(u: $source) -> Result<$target, CastError> {
Copy link
Member

Choose a reason for hiding this comment

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

These functions want an #[inline]. I’ve observed in my Range specialisation PR that the casts won’t get inlined and inlining is able to get rid of the unnecessary code resulting in one or two instructions per cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, forgot this originally, added now.

@scottmcm
Copy link
Member

Since this is number-specific, I rather expected the checked/overflowing/wrapping/saturating methods, like @petrochenkov linked, for consistency with the other mathy methods.

For example, Range::size_hint wants (usize::saturating_cast(delta), usize::checked_cast(delta)). (In stable today it just gives up and always returns (0, None) for large types, which is rather suboptimal.)

@bors
Copy link
Contributor

bors commented Jun 14, 2017

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

@alexcrichton
Copy link
Member Author

@scottmcm my personal hope is that we can make progress here as this feature has been stalled for years at this point now. Along those lines while I think such future additions may affect naming today I'd prefer to focus solely on one use case for now, FFI conversions. Those want the checked versions 99% of the time and to be usable also want them to be ergonomic.

I'm just wary of continuing to bog down progress in the endless bikeshed of what methods, what names, and what signatures we want. My hope is that this strikes a right balance for ergonomics (usable with ? and short name like cast) and also covers the use case at hand (all integral types can be casted to all other integral types)

@alexcrichton
Copy link
Member Author

@nagisa I personally feel new is not the right name here as a fallible conversion is happening, whereas that's sort of operation is typically not dubbed "new"

@scottmcm
Copy link
Member

@alexcrichton I'm totally onboard with not having the saturating_ and other variants immediately.

Let's see if I can better phrase as motivational statements:

@bors
Copy link
Contributor

bors commented Jun 16, 2017

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

@carols10cents carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2017
@carols10cents
Copy link
Member

Merge conflicts!

@alexcrichton
Copy link
Member Author

Rebased

@alexcrichton alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2017
@bors
Copy link
Contributor

bors commented Jun 21, 2017

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

This commit is a result of the libs team's discussion of rust-lang#33417 and how it
affects integral types. The conclusion was that the motivation for converting
integral types, working in a cross-platform code that uses platform-specific
integer types, was different enough from the intent of `TryFrom` that it doesn't
make sense to unify the paths. As a result this is a proposal for the
alternative version of the API which purely works with integral types.

An unstable `Cast` trait is added as the implementation detail of this API, and
otherwise with this you should be able to call `i32::cast(0u8)` at will. The
intention is then to call this in platform-specific contexts like:

    // Convert from C to Rust
    let a: c_int = ...;
    let b: u32 = u32::cast(a)?;

    // Convert from Rust to C
    let a: u32 = ...;
    let b: c_int = <c_int>::cast(a)?;

Everything here is unstable for now, but the intention is that this will
stabilize sooner than `TryFrom`.
@Mark-Simulacrum
Copy link
Member

@alexcrichton Not sure if this is legitimate. May just need to delete something? Either way, I believe this potentially need to be rebased -- not super clear about the current status.

[00:45:18] An error occured:
[00:45:18] '/checkout/obj/build/x86_64-unknown-linux-gnu/md-doc/unstable-book/src/library-features/num-cast.md' referenced from SUMMARY.md does not exist.
[00:45:18] 
[00:45:18] 
[00:45:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/rustbook" "build" "/checkout/obj/build/x86_64-unknown-linux-gnu/md-doc/unstable-book" "-d" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc/unstable-book"
[00:45:18] expected success, got: exit code: 101

@est31
Copy link
Member

est31 commented Jun 25, 2017

@Mark-Simulacrum SUMMARY.MD should be autogenerated now, and create links on its own. One will probably have to rebase in order to get this autogeneration though.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jun 27, 2017

Are you still working on this @alexcrichton? Friendly ping to keep this on your radar.

@briansmith
Copy link
Contributor

Which RFC is this based on? Which specific conversions are to be supported: signed -> unsigned, unsigned -> signed, potentially-lossy narrowing conversions? As was discussed in the RFC I'm aware of (rust-lang/rfcs#1218), each type of conversion should have a different name. In particular, potentially-lossy narrowing conversions should be called something like narrow() and signed/unsigned conversions should be called something like signed()/unsigned(). This way, the writer of the code can indicate exactly what kind of conversion they want and the readers/reviewers of the code can more quickly verify that that kind of conversion makes sense.

I also don't think there's anything special about FFI types. usize/isize vs fixed-width types have the same issues.

@est31
Copy link
Member

est31 commented Jun 30, 2017

Which RFC is this based on?

I'm not sure about the procedure, but afaik smaller lib apis like this one can be added without an RFC.

@SimonSapin
Copy link
Contributor

The closest in https://github.com/rust-lang/rfcs/blob/master/libs_changes.md#is-an-rfc-required is:

Obvious API hole patch, such as adding an API from one type to a symmetric type. e.g. Vec<T> -> Box<[T]> clearly motivates adding String -> Box<str>

I think a new inherent method to primitive types is not a "hole patch", and @briansmith listed more than enough questions to show that details of this API are not obvious.

I’ve seen and had RFCs be asked for much less than this.

@CryZe
Copy link
Contributor

CryZe commented Jun 30, 2017

Yeah I agree. This at least needs more discussions as to why there's no methods for all the different kinds of possible casts you would want. This exists for overflow on all kinds of different operations, but here there's just the "checked" version available.

@alexcrichton
Copy link
Member Author

Ok, I don't believe I have the time or energy to push this over the hump, so closing.

@alexcrichton alexcrichton deleted the cast branch June 30, 2017 22:25
@aturon
Copy link
Member

aturon commented Jul 3, 2017

@petrochenkov, are you interested in revisiting your RFC on this topic, now that the TryFrom story has shaken out? This is clearly still important functionality.

@petrochenkov
Copy link
Contributor

@aturon
No, sorry.
If someone is interested, feel free to reuse the old RFC's text.

@briansmith
Copy link
Contributor

If nobody volunteers for this beforehand, I will pick up redoing the RFC in mid-August.

@kennytm
Copy link
Member

kennytm commented Aug 30, 2017

@briansmith @petrochenkov Hi. Any update on the RFC?

@petrochenkov
Copy link
Contributor

@kennytm
Nothing new from me.
If you want to resurrect it yourself, please do!

@briansmith
Copy link
Contributor

@kennytm Sure, I will write a new RFC for this. Am I correct in understanding that it won't even be considered by the libs team until after January, though? Not sure how the "impl period" thing is working.

@kennytm
Copy link
Member

kennytm commented Aug 31, 2017

@briansmith I think that depends on whether stabilizing TryFrom is considered part of the 2017 roadmap or not. (cc @aturon)

Recently #44174 is submitted, so the FFI issue #33417 (comment) is becoming a real problem, which is the original reason why this PR exists. But it seems not definite that TryFrom will be stabilized before the end of the impl period (2017 Dec 17th).

@aturon
Copy link
Member

aturon commented Aug 31, 2017

@briansmith The plan is to put our primary focus on implementation work during the impl period, so the question probably comes down to how much shepherding work would be required on the ensuing discussion. I'd suggest trying to get one open in the next ~week, which would leave some time for shepherding discussion prior to the impl period, and thus put us in a place where FCP shouldn't be too hard to reach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.