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 truncate_into() #79477

Open
camelid opened this issue Nov 27, 2020 · 10 comments
Open

Add truncate_into() #79477

camelid opened this issue Nov 27, 2020 · 10 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Nov 27, 2020

This is a proposal to add a truncate_into() method on the various small
primitive types (u*, i*, and f* primarily).

Currently, an as cast can be used to 'upcast' from e.g. u8 to u16, but it
can also be used to 'downcast' from e.g. u16 to u8. The downcast variant
will truncate any bits that can't fit into the smaller variant, which may be
unexpected behavior. Changing as's behavior so it does not support downcasting
would be a big task, and perhaps not worth it – that is out of the scope of this
proposal. Instead, I propose to add a new method, truncate_into() that
performs the downcasting behavior such that it is clear in the code what is
happening.

It would likely be implemented as two traits, TruncateFrom and TruncateInto,
akin to TryFrom and TryInto. TruncateInto would be implemented in terms of
TruncateFrom, and TruncateFrom could just use as internally. In the
future, if we change the behavior of as, we would probably have to convert
into an array first and then ignore some of the bytes.

This proposal stemmed from discussion on Zulip about the confusion arising
from as's truncating behavior.

@camelid camelid added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 27, 2020
@elichai
Copy link
Contributor

elichai commented Nov 27, 2020

Just want to point out that type inference deciding what type to truncate into can be somewhat foot gunny

@ehuss
Copy link
Contributor

ehuss commented Nov 27, 2020

You may be interested in WrappingFrom from RFC 2484 which is something similar (if not identical).

@camelid
Copy link
Member Author

camelid commented Nov 27, 2020

Re the potential type inference issues: We could also just start with TruncateFrom, used like u8::truncate_from(300_u16), and then always add TruncateInto later.

@leonardo-m
Copy link

I think "as" is a sharp tool that has to nearly become deprecated in normal Rust code. But I think a global vision about the whole casting topic is needed to avoid a patchwork solution.
See also: #72762

@scottmcm
Copy link
Member

I think this is a large enough change that it needs to be an RFC, not just an issue. Any trait in core tends to be more than just an issue, and this is implicitly a suggestion that people change how they write their code too, so is worth more eyes on it.

I'd definitely like to get num::WrappingFrom from the RFC that ehuss mentioned.

@camelid
Copy link
Member Author

camelid commented Nov 28, 2020

So should I write an RFC for this or is it covered by the WrappingFrom RFC? (Also, it seems like WrappingFrom should be renamed to TruncateFrom.)

@scottmcm
Copy link
Member

scottmcm commented Nov 29, 2020

I'm on lang, not libs, so as always take what I say with a grain of salt about library things.

But what I'd say is that you should take the existing RFC, branch it, and cut it down to just num::WrappingFrom and post that as a separate RFC. That way it can make progress separate from the more-complicated question of what to do with non-integers.

Personally I like WrappingFrom better than TruncateFrom for a couple reasons:

  • WrappingFrom naturally works for things like i8: WrappingFrom<u16>, which I wouldn't really call a truncation
    • It's not clear to me that it's worth separating the sign differences from the other wrapping things
  • u64: TruncateFrom<u16> feels a bit weird to me, but u64: WrappingFrom<u16> seems clearly fine (if an opportunity for a "consider using From instead" lint)
    • I'm assuming this should have the lossless impls too, for use with usize and c_int and such
  • it fits the existing verbiage of num::Wrapping and iN::wrapping_add and such
    • one could even imagine impl<T, F> From<Wrapping<T>> for Wrapping<F> where F: WrappingFrom<T>, though I guess that can't work until we have overlapping/specialized impls
  • it extends simply to a possible future num::SaturatingFrom so that one can have u8::saturating_from(300) => 255
    • the other of the normal trilogy, CheckedFrom, isn't really needed since it's equivalent to TryFrom+Result::ok

@thomcc
Copy link
Member

thomcc commented Nov 29, 2020

I agree with WrappingFrom being better than TruncateFrom for all the listed reasons, but one thing is the Wrapping terminology only works for integers and not for conversions involving floats.

@scottmcm
Copy link
Member

and not for conversions involving floats.

Completely agreed, but I'm ok with that, as I was putting questions of floating-point as explicitly out of scope for this. (I'd like to get a solution for that too, but it seems fundamentally harder -- should it have rounding mode control, for example? -- so would like to side-step it to be able to make progress on an operation that I think has fewer open questions.)

I would consider truncation of floats to be a subset of rounding -- materially different from truncation of integers (affecting low bits vs high bits, in the meanings I'm used to, for example) -- and thus using different traits is an advantage.

@thomcc
Copy link
Member

thomcc commented Nov 29, 2020

Hmm, I'm a lot less sold on the utility of this as just for integers cases, but I guess that can be born out later in the RFC discussion...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants