Implement always-fallible TryFrom for usize/isize conversions that are infallible on some platforms

[SimonSapin]

This reverts commit 837d6c7 "Remove TryFrom impls that might become conditionally-infallible with a portability lint".

This fixes #49415 by adding (restoring) missing TryFrom impls for integer conversions to or from usize or isize, by making them always fallible at the type system level (that is, with Error=TryFromIntError) even though they happen to be infallible on some platforms (for some values of size_of::<usize>()).

They had been removed to allow the possibility to conditionally having some of them be infallible From impls instead, depending on the platforms, and have the portability lint warn when they are used in code that is not already opting into non-portability. For example #[allow(some_lint)] usize::from(x: u64) would be valid on code that only targets 64-bit platforms.

This PR gives up on this possiblity for two reasons:

• Based on discussion with @aturon, it seems that the portability lint is not happening any time soon. It’s better to have the conversions be available at all than keep blocking them for so long. Portability-lint-gated platform-specific APIs can always be added separately later.

• For code that is fine with fallibility, the alternative would force it to opt into "non-portability" even though there would be no real portability issue.

[rust-highfive]

r? @withoutboats

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

[SimonSapin]

@rfcbot fcp merge

[rfcbot]

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

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

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]

So if a trait impl can't be implemented for all targets it shouldn't be implemented for any even with a portability lint? Is that going to be the new policy or is there something special about From and/or isize/usize?

The way I see it is that having different error types for impls like impl TryFrom<isize> for i32 is just a consequence of isize and usize being different sizes on different targets. It's no different to the differing error types for impl TryFrom<c_long> for i32 which apparently isn't an issue. It's already easy to write non-portable code involving usize or isize so differing associated types on impls for them isn't a new issue. For example the following code compiles (with no warnings) and runs on 64 bit platforms but fails on 32 bit:

fn main() {
    assert_eq!(4294967296usize, 1usize << 32);
}

isize and usize aren't portable types.

[SimonSapin]

So if a trait impl can't be implemented for all targets it shouldn't be implemented for any even with a portability lint?

This presumes the existence of a portability lint, which most likely won’t be the case for many months yet.

By the way this is why non-pointer-sized Atomic* types are not stable yet.

[ltratt]

Thanks for this @SimonSapin -- I know quite a few of us will be grateful for this one being merged!

[rfcbot]

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

[Kimundi]

While I agree with @ollie27 that isize and usize are non-portable types, and as such having different trait impls for them for them would not be unexpected, I also think that just providing them with a unified error type wouldn't be a problem, and actually make the conversions somewhat more portable (because the code compiles the same on any platform).

Performance-wise it also shouldn't make much of a difference, since presumably the optimizer will be able to figure out that a Err value will never be produced by the conversion.

[SimonSapin]

Indeed, those conversions are marked #[inline] and the optimizer does eliminate .unwrap() when it never panics:
http://play.rust-lang.org/?gist=3167bddf125915403b64eb98fcf16fe5&version=stable&mode=release&edition=2015

[alexcrichton]

@bors: r+

[bors]

📌 Commit e7c122c has been approved by alexcrichton

[ollie27]

This issue was never about performance. I don't know why people keep mentioning that. This issue is about whether the From impls proposed in #37423 can be added in the future. It was my understanding that the portability lint was accepted to allow such impls but this PR will make adding them a breaking change. Using ! as the error type for the infallible conversions will allow those impls to be added without breakage.

The question now is, if those impls won't be allowed, will the portability lint be usable for any trait impls?

[SimonSapin]

Right, this isn’t about performance, I was only responding that it isn’t a concern.

This issue is about whether the From impls proposed in #37423 can be added in the future.

Right, and this PR is proposing that they can not because we’re adding different, overlapping impls.

Using ! as the error type for the infallible conversions will allow those impls to be added without breakage.

This proposal is almost the same as adding From impls: public APIs (in this case associated types and return types) vary by platform, so code that compiles for a target of a given pointer size might have a type error for a different target.

Which I think we should’t do without a portability lint. And even then, as mentioned in this PR’s description marking some TryFrom integer conversions as non-portable is a downside, for callers that don’t care about the error type or whether it is uninhabited.

The question now is, if those impls won't be allowed, will the portability lint be usable for any trait impls?

That is a question that is unresolved as far as I know: will the lint be powerful enough to detect portability issues in enough cases. (For example: using a blanket portable impl which depends on a non-portable impl.)

[bors]

⌛ Testing commit e7c122c with merge 0fb6e39...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0fb6e39 to master...