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

Signed↔Unsigned integer methods #453

Open
scottmcm opened this issue Oct 1, 2024 · 7 comments
Open

Signed↔Unsigned integer methods #453

scottmcm opened this issue Oct 1, 2024 · 7 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@scottmcm
Copy link
Member

scottmcm commented Oct 1, 2024

Proposal

Problem statement

One common pattern of numeric conversions is having a signed value and needing it in the unsigned type, or vice versa. Things like isize::MAX as usize, for example, or before we added .add(i) needing .offset(i as isize).

Motivating examples or use cases

Unfortunately, doing it like that isn't necessarily great. In particular, if the source value changes to something wider, it'll silently start truncating. Not to mention that it's a big length jump from i as usize to usize::try_from(i).unwrap() to get a checked version, so people will generally do the short thing where we can't give them any help in debug mode because as is defined to truncate, so for all we know the 300_i32 as u8 might be intentional. Or maybe it turns out that i as usize was actually converting from a u16, not doing a signedness change at all.

It would be nice to have a way to express just that you want it as the other-signedness type, nothing more.

Solution sketch

impl uNNNN {
    /// In debug, `self.try_into().unwrap()`.
    /// In release, `self as _`.
    pub const fn to_signed(self) -> iNNNN;

    /// Always `self as _`.
    pub const fn reinterpret_signed(self) -> iNNNN;
}
impl iNNNN {
    /// In debug, `self.try_into().unwrap()`.
    /// In release, `self as _`.
    pub const fn to_unsigned(self) -> uNNNN;

    /// Always `self as _`.
    pub const fn reinterpret_unsigned(self) -> uNNNN;
}

Alternatives

  • Do nothing, since it's not strictly wrong as-is
  • Solve the as problems by telling people to use a trait like num::WrappingFrom trait for conversions between integers rfcs#3703 instead of as
  • Just have the as-equivalent one, saying that try_into is fine for the checking
  • Have a full set of wrapping_to_signed, checked_to_signed, saturating_to_signed instead of just reinterpret_signed.
  • Add constants for more common cases, like u32::SIGNED_MAX to stop needing i32::MAX as u32.

Links and related work

I vaguely remember some recent conversations about this, but haven't found it :/

Here's an old thread discussing something similar: https://internals.rust-lang.org/t/pre-rfc-add-methods-like-42u32-to-signed-to-the-standard-library/12173?u=scottmcm

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@scottmcm scottmcm added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Oct 1, 2024
@clarfonthey
Copy link

I like this, especially since I often use unsigned_abs to fulfill this purpose for the signed -> unsigned direction.

@joshtriplett
Copy link
Member

joshtriplett commented Oct 1, 2024

We discussed this in today's @rust-lang/libs-api meeting. We would definitely like to ship at least the always-bit-cast version as to_signed/to_unsigned, and the version returning a Result or Option as to_signed_unchecked/to_unsigned_unchecked.

Note that the former exist on nightly today as cast_signed/cast_unsigned; we'd like to rename those to to_* for convenience and simplicity, given that they're supposed to be good alternatives to as.

(We didn't have a clear consensus in the meeting about whether we want the checked variants to return Option or return Result with TryFromIntError.)

We'd like to know if those two variants would be sufficient, or if people also strongly want the versions that bit-cast in release and panic in debug. Do people have use cases that really want that variant? (And if so, do people have a good name for that variant?)

@okaneco
Copy link

okaneco commented Oct 1, 2024

Wasn't the reinterpret case already accepted? Edit: yes, this was addressed above.
ACP - #359
Tracking issue - rust-lang/rust#125882

Have a full set of wrapping_to_signed, checked_to_signed, saturating_to_signed instead of just reinterpret_signed.

Of these, I find the saturating case most useful in my own code but also with narrowing casts of the same signedness.

Just to make sure, is wrapping_to_signed the same as reinterpret_signed?

Add constants for more common cases, like u32::SIGNED_MAX to stop needing i32::MAX as u32.

Tangential, but reminds me of an ACP I never got around to writing for float equivalents of min/max-representable integer for f32/f64 to make it easier to tell casting between the two if you've lost precision.

@scottmcm
Copy link
Member Author

scottmcm commented Oct 1, 2024

Oh wow, I totally missed the cast_(un)signed methods in nightly 🤦

I don't have a great survey of whether people want the debug-vs-release version. I'm just thinking that ever time I can think of where I wanted something like .to_signed(), I was expecting the unsigned number to fit in the signed type as the same conceptual value, and that I wouldn't get a negative. And while checking that in debug does seem like something that people would complain about, I'd like the easiest version to check that for me in debug.

It makes me think of places like rust-lang/rust#130229, where if the "default, easy-to-write thing" checked, we wouldn't have had the question because of course my_unsigned.to_signed() isn't supposed to be giving me a negative number, just like how of course + isn't supposed to be wrapping, because I'd write it differently if that was intentional.

TL/DR: I think the intent difference of "look, I expect this to be positive, the thing I'm calling just wants a signed type" vs "yes, I'm smuggling intentionally-negative numbers in my unsigned type" is important enough to express in the functions.

@scottmcm
Copy link
Member Author

scottmcm commented Oct 1, 2024

@joshtriplett I'm having trouble following

and the version returning a Result or Option as to_signed_unchecked/to_unsigned_unchecked.

I would intuitively expect that something named *_unchecked wouldn't return a Result or an Option.

If anything, I'd expect to_signed_unchecked to be something like

impl u32 {
    pub const unsafe fn to_signed_unchecked(self) -> i32 {
        let x = self as i32;
        unsafe { hint::assume(x >= 0) };
        x
    }
}

@RalfJung
Copy link
Member

RalfJung commented Oct 1, 2024

and the version returning a Result or Option as to_signed_unchecked/to_unsigned_unchecked.

Surely you meant ..._checked, not ..._unchecked?

We'd like to know if those two variants would be sufficient, or if people also strongly want the versions that bit-cast in release and panic in debug. Do people have use cases that really want that variant? (And if so, do people have a good name for that variant?)

Personally I wished as casts would panic in debug mode if the mathematical value changes. The current behavior feels weird inconsistent where + is checked but as is not. So I'm fully in favor of some way to express "please change the type but the value should stay the same". We can then have a strict_ variant of this that always panics if the value changes, even in release builds.

@scottmcm
Copy link
Member Author

scottmcm commented Oct 1, 2024

Oh, I see my problem -- I looked for to_signed since apparently my brain had half-remembered #183, then mistakenly assumed I'd remembered wrong when I didn't find it. Oops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants