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

Stabilize TryFrom / TryInto, and tweak impls for integers #49305

Merged
merged 5 commits into from Mar 27, 2018

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Mar 23, 2018

Fixes #33417 (tracking issue)


This adds:

  • impl From<u16> for usize
  • impl From<i16> for isize
  • impl From<u8> for isize

… replacing corresponding TryFrom<Error=!> impls. (TryFrom still applies through the generic impl<T, U> TryFrom<U> for T where T: From<U>.) Their infallibility is supported by the C99 standard which (indirectly) requires pointers to be at least 16 bits.

The remaining TryFrom impls that define type Error = ! all involve usize or isize. This PR changes them to use TryFromIntError instead, since having a return type change based on the target is a portability hazard.

Note: if we make similar assumptions about the maximum bit size of pointers (for all targets Rust will ever run on in the future), we could have similar From impls converting pointer-sized integers to large fixed-size integers. RISC-V considers the possibility of a 128-bit address space (RV128), which would leave only impl From<usize> for u128 and impl From<isize> for u128. I found some things about 256-bit “capabilities”, but I don’t know how relevant that would be to Rust’s usize and isize types.

I chose conservatively to make no assumption about the future there. Users making their portability decisions and using something like .try_into().unwrap().


Since this feature already went through FCP in the tracking issue #33417, this PR also proposes stabilize the following items:

  • The convert::TryFrom trait
  • The convert::TryFrom trait
  • impl<T> TryFrom<&[T]> for &[T; $N] (for $N up to 32)
  • impl<T> TryFrom<&mut [T]> for &mut [T; $N] (for $N up to 32)
  • The array::TryFromSliceError struct, with impls of Debug, Copy, Clone, and Error
  • impl TryFrom<u32> for char
  • The char::CharTryFromError struct, with impls of Copy, Clone, Debug, PartialEq, Eq, Display, and Error
  • Impls of TryFrom for all (?) combinations of primitive integer types where From isn’t implemented.
  • The num::TryFromIntError struct, with impls of Debug, Copy, Clone, Display, From<!>, and Error

Some minor remaining questions that I hope can be resolved in this PR:

  • Should the impls for error types be unified?
  • Should TryFrom and TryInto be in the prelude? From and Into are. (Yes.)

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2018
@SimonSapin
Copy link
Contributor Author

SimonSapin commented Mar 23, 2018

CC @rust-lang/libs

r? @sfackler

@bors: p=1

@SimonSapin SimonSapin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Mar 23, 2018
@SimonSapin SimonSapin mentioned this pull request Mar 23, 2018
2 tasks
@SimonSapin
Copy link
Contributor Author

Should TryFrom and TryInto be in the prelude? From and Into are.

I think they should, so I added a commit that makes them so. Let’s see if anyone objects :)

@SimonSapin
Copy link
Contributor Author

We already did the FCP dance in the tracking issue, but just to get eyes on this again:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 26, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 26, 2018
@ollie27
Copy link
Member

ollie27 commented Mar 26, 2018

The remaining TryFrom impls that define type Error = ! all involve usize or isize. This PR changes them to use TryFromIntError instead, since having a return type change based on the target is a portability hazard.

The problem with this is that when (and I hope it's a when) the missing From impls for usize and isize are added then it will be a breaking change as the error type will have to change from TryFromIntError to !. It also doesn't actually fix the portability hazard when we consider the C int type defs which were one of the main motivating examples for these traits. For example the error type for TryFrom<c_long> for i32 depends on the platform. If that is too much of a portability hazard then these traits are blocked on the portability lint (#41619).

@SimonSapin
Copy link
Contributor Author

@ollie27 Which impls are you saying are missing? After this PR I believe that that every impl that can be implemented in on all platforms between primitive integers are already implemented. There shouldn’t be any we haven’t gotten around to yet.

As to type aliases like c_long, I think this problem is inherent to using a type alias whose definition changes across platform. However if you’re aware of this issue it is possible to write portable code with TryFrom<c_long>: in a function or method that returns Result<_, TryFromIntError>, use something like x.try_into()?. This works out because TryFromIntError implements both From<TryFromIntError> and From<!>.

@ollie27
Copy link
Member

ollie27 commented Mar 26, 2018

Which impls are you saying are missing?

All of the impls that aren't strictly speaking portable. They were proposed in #37423 but postponed for the portability lint.

However if you’re aware of this issue it is possible to write portable code

Exactly. I don't see why people can't do this for usize and isize.

@SimonSapin
Copy link
Contributor Author

As far as I can tell the portability lint wouldn’t help much here. We want TryFrom to always be available between any two integer types, the only thing that might change is the associated Error types. But the portability lint can only lint against using some items at all, it cannot be used to opt into changing an associated type.

I don't see why people can't do this for usize and isize.

Just because a work around exists doesn’t mean it’s not bad. This PR tries to avoid this situation where we can.

@ollie27
Copy link
Member

ollie27 commented Mar 26, 2018

Just to be clear, are you saying that the impls proposed in #37423 will never be added? If you are then I strongly oppose that. Having From impls to statically assert that conversions can never fail is far more important than the portability hazard of error types being TryFromIntError or ! depending on the platform.

@rfcbot
Copy link

rfcbot commented Mar 26, 2018

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

@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 Mar 26, 2018
@SimonSapin
Copy link
Contributor Author

SimonSapin commented Mar 26, 2018

With this PR as originally proposed, indeed those could never be added as they would conflict with impls being stabilized here.

When discussing this offline @sfackler suggested that with the portability lint we could have some From impls conditionally compiled based on #[cfg(target_pointer_width = …)] and linted as non-portable, and TryFrom<Error=TryFromIntError> impls with opposite cfg, also linted as non-portable. This could work, but with the downside that could that uses these impls but would in fact be portable (because it either doesn’t care about the error type or does something with it like ?’s conversion that works in both cases) is still required to "opt into non-portability" to silence the lint. Of course this also requires the portability lint to be implemented, and to work even when non-portable impls are used indirectly, "through" a generic impl like TryFrom<T> for U where U: From<T> or Into<U> for T where U: From<T>.

The set of impls that would be affected is not obvious so I tried to make an exhaustive table (with the same assumptions about pointer width than in this PR’s original message: at least 16 bits, but no assumed upper bound)

Source →
Dest ↓
u8 u16 u32 u64 u128 usize i8 i16 i32 i64 i128 isize
u8 ! E E E E E E E E E E E
u16 ! ! E E E NP E E E E E E
u32 ! ! ! E E NP E E E E E E
u64 ! ! ! ! E NP E E E E E E
u128 ! ! ! ! ! NP E E E E E E
usize ! ! NP NP NP ! E E E E E E
i8 E E E E E E ! E E E E E
i16 ! E E E E E ! ! E E E NP
i32 ! ! E E E NP ! ! ! E E NP
i64 ! ! ! E E NP ! ! ! ! E NP
i128 ! ! ! ! E NP ! ! ! ! ! NP
isize ! NP NP NP NP E ! ! NP NP NP !

Legend:

  • !: this conversion never fails can be implemented with From on any platform
  • E: this conversion can fail and must be implemented with TryFrom<Error=TryFromIntError> on every platform
  • NP assuming a functional portability lint, this conversion could on some platforms be a non-portable infallible From impl

Whatever else we do, the few From impls for ! entries that are not already stable can be added now.

I think there are four things we can do with the rest:

  1. This PR as originally proposed: stabilize with apparently-fallible TryFrom<Error=TryFromIntError> impls for all E and NP entries, on all platforms.
  2. Wait until the portability lint is implemented and use it as described above
  3. Stabilize the traits now, but remove impls corresponding to NP entries in the table. Ship without them. Later, decide to do either 1. or 2.
  4. Same as 3. but since it’s non-obvious what combinations of types are NP in the table also remove all TryFrom impls where usize or isize is involved, because that’s easier to understand. (But keep the ! entries since some of those From impls are already stable.)

I’m now leaning toward 3 or 4.

@sfackler
Copy link
Member

I think 4 may be the cleanest but 3 also seems fine.

@SimonSapin
Copy link
Contributor Author

I’ve implemented 3 since that seems more useful. Most users don’t need to figure out mentally which impls exist or not, they can just try to use it and see what happens.

I’ve had to modify range iterators and io::Cursor since they relied on some of the impls being removed.

r? @sfackler

@bors
Copy link
Contributor

bors commented Mar 26, 2018

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

@tirkarthi
Copy link

tirkarthi commented Mar 30, 2018

To add to this rand also fails on nightly since it depends on stdweb.

Build : https://travis-ci.org/rust-lang-nursery/rand/jobs/359738013

@tirkarthi
Copy link

I downloaded the modules metadata from https://github.com/rust-lang/crates.io-index and inserted them into MongoDB to analyze the number of modules that might break. I did a query for stdweb and http. I also did a query on the number of packages that depend on rand which uses stdweb. The approx results are as below :

stdweb - 15 dependants
http - 20 dependants
rand - 1070 dependants

Gist of dependencies : https://gist.github.com/tirkarthi/846f9d2fd87c90463c5ca9cf99f3d44d
File that can be used to do queries locally : https://gist.github.com/tirkarthi/846f9d2fd87c90463c5ca9cf99f3d44d#file-scraper-py

@frewsxcv
Copy link
Member

FYI, @SimonSapin opened a PR which reverts the prelude additions: #49518

bors added a commit that referenced this pull request Mar 30, 2018
Revert "Add TryFrom and TryInto to the prelude"

This reverts commit 09008cc.

This addition landed in #49305 and turned out to break crates that had their own copy of `TryFrom` in order to use it on the Stable channel :(

We’ll explore the possibility of the 2018 edition having a different prelude that includes this traits. However per the editions RFC this requires implementing a warning in the 2015 edition for code that *would* break.
@SimonSapin
Copy link
Contributor Author

Oops, I meant to comment about this here but forgot. Thanks @frewsxcv.

As mentioned in that PR I’d still like to figure something out later, but for now let’s fix immediate breakage.

ltratt added a commit to softdevteam/lrpar that referenced this pull request Apr 3, 2018
Because rust-lang/rust#49305 removed the TryFrom
conversions from usize (temporarily), we have to use a fixed version of rustc.
It's unclear when we'll be able to revert this :/
@SimonSapin
Copy link
Contributor Author

🔔 🔔 Proposed resolution for #49415:

#51564 restores TryFrom impls that are infallible on some platforms only as always fallible, on all platforms, at the type system level (with Error=TryFromIntError).

@SoniEx2
Copy link
Contributor

SoniEx2 commented Aug 8, 2018

How does one handle something like libc::time_t using the new TryFrom?

@SimonSapin
Copy link
Contributor Author

@SoniEx2 A merged or otherwise closed PR/issue is often not a good place to ask question. Consider asking on #33417 or on https://internals.rust-lang.org/ instead. But also: what do you mean by "handle"? Convert to/from what other type?

@SoniEx2
Copy link
Contributor

SoniEx2 commented Aug 8, 2018

say I need to turn a time_t to/from u64. since time_t can change types, this seems like I'd have to be putting either "as" or #[cfg] everywhere? none of them let me write code that "just works"...

I wonder why we couldn't have multiple otherwise identical TryFroms with different error types...

@SimonSapin
Copy link
Contributor Author

It looks like time_t is always either i32 or i64. So converting to u64 is fallible in both cases because of negative values. Converting from u64 is also fallible in both cases because of very large values. So you’d always get TryFrom impls with type Error = TryFromIntError;.

Note that infallible conversions still get a TryFrom impl through impl<T, U> TryFrom<U> for T where T: From<U> { type Error = !; }. So if you were to convert time_t to/from i64 instead of u64 for example, then the try_* methods would be available on all platforms but with a varying error type. However it’s still possible to write portable code without using #[cfg]. For example:

fn foo(x: u64) -> Result<time_t, TryFromIntError> {
    Ok(x.try_into()?)
}

This works because ? does error type conversion. Depending on the platform this ends up calling impl From<!> for TryFromIntError or the no-op impl<T> From<T> for T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. 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.

Tracking issue for TryFrom/TryInto traits