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

Projects
None yet
@SimonSapin
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 23, 2018

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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 23, 2018

@SimonSapin SimonSapin referenced this pull request Mar 23, 2018

Merged

Stabilize impl Trait #49255

2 of 2 tasks complete

@SimonSapin SimonSapin force-pushed the SimonSapin:fallible branch from a3e631e to 8331925 Mar 26, 2018

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 26, 2018

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

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

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.

@ollie27

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

rfcbot commented Mar 26, 2018

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

@SimonSapin

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

sfackler commented Mar 26, 2018

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

SimonSapin added a commit to SimonSapin/rust that referenced this pull request Mar 26, 2018

@SimonSapin SimonSapin force-pushed the SimonSapin:fallible branch from 9942101 to f3d46e7 Mar 26, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this pull request Mar 26, 2018

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Contributor

bors commented Mar 26, 2018

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

@SimonSapin SimonSapin force-pushed the SimonSapin:fallible branch from f3d46e7 to adf53de Mar 26, 2018

@carllerche

This comment has been minimized.

Copy link
Member

carllerche commented Mar 29, 2018

@SimonSapin

The policy you refer includes:

All that said, it is incumbent on library authors to ensure that such "minor" changes are in fact minor in practice: if a conflict like t.foo() is likely to arise at all often in downstream code, it would be advisable to explore a different choice of names. More guidelines for the standard library are given later on.

There was some discussion in hyperium/http#192.

I will repeat my main point here:

The Rust team has advertised stability as a feature in such a way that it implied to me that pinning the stable rust version was not needed. To downstream users, it doesn't actually matter if a change is “minor” or not, it matters if the build will break. If the Rust team is not reasonably able to provide a guarantee of stability, that is fine but should be advertised and downstream users can adjust best practices by, for example, pinning the stable version of Rust.

Again if this change lands on Rust stable, it will break the existing frozen Conduit builds due to the fact that we did not pin the version of Rust but dependencies are pinned. We cannot fix these past version releases as they have been cut.

@carllerche

This comment has been minimized.

Copy link
Member

carllerche commented Mar 29, 2018

To be explicit, I understand that almost literally any change to std could be a breaking change. My understanding of the policy, especially given how the snippet I quoted above reads, is that if a change turns out to be breaking in practice then it is a non starter ("minor" or not), and that a best effort would be made to changes that break existing code.

If this interpretation is not correct, I would appreciate it if the Rust team goes on the record to clarify the policy, because at least I and probably others are under the assumption that we can expect stability across Rust releases. If this assumption is not correct, we need to change our practices to start pinning the compiler version in applications.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 29, 2018

With the new 2018 edition coming soon, it seems reasonable to include these in the prelude only in the newer editions. It prevents any breakage.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 29, 2018

@carllerche Completely agree. We have roughly 6 weeks before this change reaches the stable channel. 1.26 goes to beta this week (today?) and the plan was to wait for crater results for that beta to get an idea of how bad the breakage is in the ecosystem. I’ll talk with other libs team member today to see what they think of reverting the prelude addition sooner than that based on the reports on http and other crates in this thread.

@seanmonstar Indeed we’ve discussed having a larger prelude based on a crate’s edition. That could be an opportunity to add TryFrom and TryInto without breaking existing code.

@tirkarthi

This comment has been minimized.

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

This comment has been minimized.

Copy link

tirkarthi commented Mar 30, 2018

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

This comment has been minimized.

Copy link
Member

frewsxcv commented Mar 30, 2018

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

bors added a commit that referenced this pull request Mar 30, 2018

Auto merge of #49518 - SimonSapin:prelude, r=alexcrichton
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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 30, 2018

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

Use a specific version of rustc nightly.
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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 14, 2018

🔔 🔔 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

This comment has been minimized.

Copy link

SoniEx2 commented Aug 8, 2018

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

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Aug 8, 2018

@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Aug 8, 2018

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