-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversions: FromLossy
and TryFromLossy
traits
#2484
base: master
Are you sure you want to change the base?
Conversation
FromLossy
and TryFromLossy
traits
I think the word exact is problematic and that it defined imprecisely in this RFC. It suggests to me that for Since One possible definition that seems to fit the criteria described in the RFC is:
|
Other questions that this RFC provoked:
|
should From be allowed to allocate remote resources? (granted you should probably use TryFrom for things like that) |
I disagree with the need for an isomorphism; e.g. I see no problem with To be fair, that's not what you said. But surely an exact conversion
Meaning an intrinsic? I don't really see why this is important.
Since this wasn't specified before, I don't see it as a big issue, but I suppose it could be (if libs remove their implementations to comply).
It musn't fail (existing requirement, not from this RFC). Exactly what this means with regards to unlikely (and probably unrecoverable) panics is up for interpretation, I guess (and specifying something won't necessarily influence how people use it for anyway). |
@dhardy To be clear, I'm not saying that
That's exactly what the proposition says :) In other words:
or if we are inclined to be somewhat more pedantic:
No, "pure" means deterministic here. That is (formulated somewhat imprecisely), for I don't such a requirement is necessary tho? |
It's not just about this RFC, but can't we make |
@Centril someone likes their type theory 😄 WP has an article on pure functions. But it's not the same as what you wrote which is about Back on topic, I guess all @newpavlov Good point. I'm not sure, is it possible to do this without it being a big breaking change? |
I do! ;) I think we should specify formally (with logic -- probably not typing rules) in the RFC and the documentation of Yeah being imprecise about what
If that is a reasonable expectation the user has, we might as well specify it? |
@dhardy UPD: Hm, it looks automatic coercion is not planned, and it seems we'll get back to cyclic trait design from pub trait TryFrom<T>: Sized {
type Error;
fn from(value: T) -> Self {
Self::try_from(value).unwrap_or_else(|_| panic!("error"))
}
fn try_from(value: T) -> Result<Self, Self::Error> {
Ok(Self::from(value))
}
}
trait From = TryFrom<Error=!>; It will allow existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't formed an opinion on the actual proposal, so here's what I do best: nitpicking about floating point minutiae
text/0000-from-lossy.md
Outdated
This has several problems: | ||
|
||
- `as` can perform several different types of conversion (as noted) and is therefore error-prone | ||
- `as` can have [undefined behaviour](https://github.com/rust-lang/rust/issues/10184) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is "just" an implementation bug, thus "temporary" and not a problem with as
-the-language-feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, the undefined behaviour can be (and is being) fixed. I should say instead that there are certain casts which have no correct result and therefore should fail, which as
has no mechanism for (other than panics).
text/0000-from-lossy.md
Outdated
|
||
Where conversions are not exact, they should be equivalent to a conversion | ||
which first forces some number of low bits of the integer representation to | ||
zero, and then does an exact conversion to floating-point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this trying to avoid specifying a rounding mode? Or is it trying to specify a rounding mode in some convoluted manner? Either way, all these conversions should be specified to round to nearest, ties to even. That's the default rounding mode and we (as well as other languages) use it everywhere except for float -> int conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this is convoluted. And you're correct, since the output type here is floating-point, it would be expected to round to nearest with ties to even.
text/0000-from-lossy.md
Outdated
|
||
The implementations should fail on NaN, Inf, negative values (for unsigned | ||
integer result types), and values whose integer approximation is not | ||
representable. The integer approximation should be the value rounded towards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing if the integer approximation doesn't fit the target type is... debatable. The proposed semantics for as
are to saturate to int_type::MAX
or int_type::MIN
respectively, and that is in some ways an "approximately equivalent value".
BTW the same point applies to infinities (i.e., it's reasonable to define f32::INFINITY -> u8
to result in 255), but I can understand being more uneasy with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, infinities and out-of-range values fall into the same category here: not representable. IMO 255 is a very poor approximation of 1000f32 and an even worse approximation to 1e300f64.
Certainly I'm being opinionated here but I think if we have a fallible conversion available then we should fail on out-of-range values. (But if not, then TryFromLossy
also has no reason to exist.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have sympathy with that argument, but at the same time I'd like to reduce divergence from the semantics of as
(to avoid people sticking to as
because they prefer its behavior), and currently that seems more likely to be saturation than panicking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that as
must return some defined value simply in order to avoid undefined behaviour. We have no need to model this conversion trait on the limitations of as
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as
doesn't have to return a value, it can panic (there are some reasons to not want that, but it avoids UB and is reasonable in isolation). That would be the moral equivalent of this trait returning Err
.
text/0000-from-lossy.md
Outdated
- 100_000f32 → u16: error | ||
|
||
(Alternatively we could allow floats in the range (-1, 0] to convert to 0, also | ||
for unsigned integers.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems more consistent with the stated rounding mode (I view all these conversions as "round, then see if they fit into the target type") and it also matches the current behavior of as
.
text/0000-from-lossy.md
Outdated
negative, infinite or an NaN. So even though the output type has large enough | ||
range, this conversion trait is still applicable.) | ||
|
||
The implementations should fail on NaN, Inf, negative values (for unsigned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "negative values" include negative zero?
(This question disappears if values in (-1.0, 0.0] result in Ok(0)
as discussed below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not last time I checked 😄
But this question goes away if we use the alternative as you suggested anyway.
text/0000-from-lossy.md
Outdated
representable. The integer approximation should be the value rounded towards | ||
zero. E.g.: | ||
|
||
- 1.6f32 → u32: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't rounded towards zero.
Updated regarding several things mentioned. |
|
@repax The documentation of But yes, saying that The link to But regardless, it is nice to see a movement towards more exact definitions, kudos to @dhardy on that. |
@Centril of course all functions are homomorphisms w.r.t. equality (whoops). I guess we can drop the @SimonSapin thanks for the feedback.
I suppose there should be |
I’m having a hard time imagining a situation where generic code with these traits in a If I remember correctly the existence of both rust-lang/rust#42456 was another attempt that could be mentioned in Prior Art, even though it didn’t land. |
Adding traits sounds unnecessarily confusing and hierarchical. And maybe breaking. Could
If you needed this, then you'd write |
@SimonSapin this is what motivated me. Ideally we wouldn't need to implement this trait ourselves. But it may not have wide usage. @burdges interesting idea. But why use |
@dhardy This example is sort of making my point. As far as I can tell it is not generic, and Is there a use case for code like either of these? fn foo<N>(…) where N: CastFromInt<u32> {…}
fn bar<N>(…) where u32: CastFromInt<N> {…} |
I think |
Perhaps not. So I guess the alternative is simple but involves quite a few methods: add these to every integer type: fn to_f32(self) -> f32;
fn to_f64(self) -> f64; These types already have Or more akin to fn from_int<T: ToFloat<Self>>(x: T) -> Self; But @burdges |
We do nee a default regardless. I suppose you're saying If you want to avoid breaking changes then you can specify it like this:
|
@burdges adding constants like that seems way more confusing (and also breaking) than what is proposed in the RFC. IMO it was a mistake to add Having more traits is fine, but I think it would be useful to reduce the number of traits which must be implemented, so I think going forward all new infallible traits should have a generic implementation for types implementing the infallible trait with an error type of impl<T, U> FromLossy<T> for U where U: TryFromLossy<T, Error=!> { ... } Also, @dhardy, the trait definitions in the RFC are missing their generic parameter |
I imagined the coherence rules isolated any breakage when adding a default constant, but.. I suppose you're talking about how the trickier coherence rules around the existing type parameter, yes? I suggested constants here largely because they permit positive or indeterminate defaults, while traits mostly only support negative defaults. It's possible negative defaults are desirable here, but the RFC design looked kinda bent around negative defaults. I think folks have voiced roughly that opinion, hence my initial suggestion being a default As an aside, constants are less confusing because they're documented with the main trait, but rust doc could be modified to have subordinate traits documented on the same page as the main trait, so whatever. Oh. If one really wants unspecified defaults, one might also define |
@Diggsey what's your take on @SimonSapin's point that it may be better to find options which don't involve adding to the prelude? Auto-implementing |
It's a good point - I would at least like a solution which is generic over the numeric types (including user-defined numeric types) but I don't think it need be more general than that. Regarding the prelude - would it be possible to have the traits exist in both the prelude and a module, and only stabilise the version outside the prelude? (Effectively preventing the kinds of conflicts @SimonSapin described without committing to stabilisation of the trait's existence in the prelude). TBH, I'm not sure these need to be in the prelude when the |
So, lets try to revive some of the discussion here...
This is an interesting solution. I think the reason for the compile error is just because We could do something similar using wrapper types as above: /// pure marker wrapper
pub struct Round<T>(T);
impl TryFrom<Round<f32>> for u32 { ... }
assert_eq!(u32::try_from(Round(13.7f32))?, 14); One could achieve a similar effect with the existing rounding methods, but could we optimise it to similar code? let x = u32::try_from(13.7f32.round())?;
This is a numeric conversion supported by There's a related case: Making safe conversion as ergonomic as via the Alternatively, it seems @leonardo-m is hinting that we should have a language-level solution which does not have the drawbacks of
As mentioned, there seems to be no reason that one can't implement
This is just an optimiser problem IMO, and not common enough to worry about ergonomics.
Fundamentally the length is part of an array's type and a slice's value, so a library solution can never omit the error check from The only option for proving correctness at compile-time is language-level support via some new syntax (e.g. Both these examples (and many others) desire type-safe conversion which pub trait TryFrom {
type Error;
fn try_from(value: T) -> Result<Self, Self::Error>;
#[expect_success]
fn do_from(value: T) -> Self;
} where |
I definitely like (soft-)deprecating
Perhaps we could warn on |
With my lang hat on, that (a hard error on lossy The lint does exist in clippy, but even there it seems to be allow-by-default: https://rust-lang.github.io/rust-clippy/master/index.html#cast_lossless |
No. 2021 or not, I feel that a lint against |
A small subset of you might be interested in my new library, easy-cast. It attempts to replace most uses of |
…ssy conversions See: rust-lang/rfcs#2484 Note due to blanket-trait limitations, we can really only implement this for new types in the current crate
For anyone still interested on this topic: It just came up again in a thread about Rusts ergonomics around numerical computations. https://internals.rust-lang.org/t/rust-and-numeric-computation/20425/14 |
I pulled the wrapping part of this out to #3703, to try to decouple it from the seemingly-harder questions around what "lossy" should mean around floats and such. |
Add
FromLossy
,TryFromLossy
traits.Discuss the bigger picture of conversions and the
as
keyword.Specify that
From
implementations must not only be safe, but also exact.Rendered RFC