Tracking issue for {f32,f64}::to_int_unchecked methods

[SimonSapin]

First discussed in issue #10184.

As of Rust 1.39, casting a floating point number to an integer with as is Undefined Behavior if the value is out of range. -Z saturating-float-casts fixes this soundness hole by making as “saturate” to the maximum or minimum value of the integer type (or zero for NaN), but has measurable negative performance impact in some benchmarks. There is some consensus in that thread for enabling saturation by default anyway, but provide an unsafe fn alternative for users who know through some other mean that their values are in range.

PR #66841 adds that method to each of f32 and f64.

    /// Rounds toward zero and converts to any primitive integer type,
    /// assuming that the value is finite and fits in that type.
    ///
    /// ```
    /// #![feature(float_approx_unchecked_to)]
    ///
    /// let value = 4.6_f32;
    /// let rounded = unsafe { value.approx_unchecked_to::<u16>() };
    /// assert_eq!(rounded, 4);
    ///
    /// let value = -128.9_f32;
    /// let rounded = unsafe { value.approx_unchecked_to::<i8>() };
    /// assert_eq!(rounded, std::i8::MIN);
    /// ```
    ///
    /// # Safety
    ///
    /// The value must:
    ///
    /// * Not be `NaN`
    /// * Not be infinite
    /// * Be representable in the return type `Int`, after truncating off its fractional part
    #[cfg(not(bootstrap))]
    #[unstable(feature = "float_approx_unchecked_to", issue = "0")]
    #[inline]
    pub unsafe fn approx_unchecked_to<Int>(self) -> Int where Self: FloatToInt<Int> {
        FloatToInt::<Int>::approx_unchecked(self)
    }

The FloatToInt trait (tracking issue: #67057) has macro-generated impls for all combinations of primitive integer and primitive floating point types:

impl FloatToInt<$Int> for $Float {…}

[Mark-Simulacrum]

We briefly discussed this in the lang team meeting today, and felt that stabilizing this would help unblock progress on making f{32,64} as uN not unsound in the edge cases (i.e., where this function is UB).

@Amanieu, would you be up for proposing FCP merge here to stabilize the methods (but not the backing trait)?

[Amanieu]

I have some concerns regarding the naming of these methods.

First of all, the general convention in all existing methods is to put unchecked at the end of the name. The only exception to this rule is get_unchecked_mut.

Secondly, I feel that approx is a poor description of what is happening. This is particularly concerning since this is an unsafe function.

I propose that these methods be renamed to {f32,f64}::to_int_unchecked, which makes it absolutely clear what operation is being performed. Since the current lang team consensus is to make as perform a saturating conversion by default, the name also serves as a contrast to the "default" way of casting a float to an integer.

[Mark-Simulacrum]

I plan to post a PR soon with the renaming to to_int_unchecked, possibly including a stabilization with that (since we often rename and stabilize in one go I think and it seems unlikely that waiting would help).

[Amanieu]

Proposing to merge this with the name to_int_unchecked.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @KodrAus
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[SimonSapin]

Renaming sounds fine. I’d just like to point out that we may want multiple float-to-int conversion methods (with different semantics) in the future: https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395

[rfcbot]

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

[Mark-Simulacrum]

I've opened up a PR with the rename and stabilization: #70487

[leonardo-m]

In Rust we have arithmetic operations that are checked in debug builds and unchecked in release builds. I think I'd like a FP->int conversion that does the same. Is this a good idea?

[SimonSapin]

@leonardo-m This has been discussed at length in #10184. An important difference is that unchecked arithmetic can let overflow happen which could could logic bugs, but the uncheked float to int conversion can causes Undefined Behavior which should never happen in Rust without unsafe.

[hanna-kruppe]

Even if we take "unchecked in release mode" to mean "some defined behavior" rather than UB, the trade-offs are very different:

• With integer overflow, the "unchecked" option is defined as two's complement wrap-around, which is practically free to implement (just omit the overflow checks). For float->int casts, there is no behavior that's consistent across platforms and requires no extra code to be executed.
• Integer arithmetic is pervasive in most/all applications, so overflow checks apply in a ton of places and their performance impact can be spread across the whole application, requiring a "bulk" switch. In contrast, conversions from floats to integers are much rarer and (as far as we know) if saturation does impact performance then it's due to individual conversions in hot loops. This makes it much more practical and appealing to decide checked vs unchecked on a case-by-case basis after profiling and manual inspection.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[e00E]

I realize this is an old issue but I still want to leave this comment.

The way to_int_unchecked works today is that it is unsafe. You need to verify the invariants (float is in range of the integer) before calling it. This is the same as in C++.

There is an argument that this function should not be unsafe. On some (most?) hardware there is no undefined behavior for out of bounds. This could be turned into unspecified behavior where the resulting integer value is unspecified but safe. For example on x86 the cvttss2si instruction performs the conversion and this does not have undefined behavior. This means on x86, this function is safe for free (no extra overhead).

If it works like this on most platforms, then little is lost by making it safe. We only lose flexibility/performance on (potentially future) platforms this doesn't work. On other platforms we gain something by making this function easier to use. And we gain performance because you can use the function without checking for bounds first.

We could also keep to_int_unchecked the way it is and add a new function to_int_unspecified that does what I described. The reason I found this issue is that I implemented that in a crate here.

[CodesInChaos]

I agree that these should return an unspecified value instead of exhibiting undefined behaviour in release mode. However the function should also be allowed to diverge (panic or abort). In practice it should behave like overflowing integer operations, i.e. panic if integer overflow checks are enabled.