.to_uint() and .to_int() should return None on NaN

[opp11]

Calling .to_uint() or .to_int() on an f64 or f32 will always return a Some value, even if they are called with NaN (playpen link):

   let f: f64 = Float::nan();
   let res = f.to_uint(); // <-- will be Some(0)

As far as I can tell this this behavior comes from the fact that .to_int() and .to_uint() is just implemented as casts as seen here:

fn to_int(&self) -> Option<int> { Some(*self as int) }
...
fn to_uint(&self) -> Option<uint> { Some(*self as uint) }

This seems (to me at least) a bit unintuitive since you would expect the method to return None when called with invalid input - such as NaN which has no obvious conversion to a u/int - since the method returns an Option.

Alternatively it would nice if it was mentioned in the docs, that the methods are the same as a cast for primitive types.

EDIT:
It seems like something similar happens for infinity.

EDIT 2:
Updated the playpen links - thank you frewsxcv!

[Gankro]

So the most obvious fix to me for these specific concerns is to have:

fn to_int(&self) -> Option<int> { 
    if self.is_finite() {
        Some(*self as int) 
    } else {
        None
    }
}

But this raises some intent ambiguities for me. Is this API intended to provide a generic interface for casting, or meant to provide a more finicky alternative, for when you really care if the cast is sane (similar to providing checked versions of the math operators)? Right now we snap to the "nearest" valid value. So for instance -300 maps to 0 on a uint. Do we want to yield None if the float is out of the valid upper/lower bound of the integral primitive? What if the value is like, 3.4? Are we happy with rounding, or do we want to yield None there, to?

[opp11]

Well to me at least, when a function returns an Option then that means that: "if no suitable value can be returned, then return None". So it seems strange to change this pattern for these methods.

Furthermore I think it is just as sane to have .to_int() and .to_uint() refer to the - as you said - more finicky way of casting, and then if all you want is a straight cast you use exactly that (thereby also showing what you mean to do). That would also make it easier to check if the conversion happened succesfully, without having to check all the edge cases beforehand (unless there is some other way to acheive this, which I don't know about).

But I see your point in that where to draw the line between valid and invalid input is not all that obvious. However if the current implementation sticks around, I would still love it if a short notice was added to the docs.

FWIW though I would be fine with your current implementation for .to_int(). I am just as unsure about what to do with .to_uint() as you are right now, though.

[frewsxcv]

This is still an issue. I updated the playpen to compile with the latest rust

[aturon]

@bjz @nikomatsakis any thoughts here?

[brendanzab]

I don't know, and it depends on the use case. I would tend towards saying that we should let the user handle the NaN. Adding in a conditional may prevent llvm from optimizing out those annoying unwraps we always have to do when working with generic numbers.

In more general terms, I would love some way of generically exposing the raw as behavior - the Option return value gets really annoying some times, to the point that we wrap it in noise-rs. But that's kind of separate to this issue.

[Gankro]

Should we have a suite of casts for different usecases?

[aturon]

Nominating for 1.0-beta P-backcompat-libs.

[pnkfelix]

1.0-beta, P-backcompat-libs.

[brson]

Do as the title says. Easy.

[brendanzab]

@brson Is this an endorsement of the change?

[aturon]

Note: we do not plan to stabilize the ToPrimitive/FromPrimitive traits (we plan to deprecate them), so this issue is likely moot for those existing methods.

triage: P-low ()

[huonw]

we plan to deprecate them

This has happened, and the traits have now been entirely removed too. Closing.