implement float_to_int_unchecked

[RalfJung]

@hanna-kruppe would be great if you could have a look at this.

float.rs tests legal casts. test_cast checks that both as casts and unchecked casts work (i.e., these are not saturating). The compile-fail tests should ensure that illegal casts via the intrinsic are detected as such.

Fixes #1264

[hanna-kruppe]

I can't give this as much time and thought as I'd need to be more confident, but it seems at least plausibly correct to me.

[RalfJung]

@SimonSapin could you have a look at this, maybe?

[SimonSapin]

Hmm there’s a fair amount going on here and I’m not at all familiar with the code base. Do you have a more specific question?

Outside of const eval, float_to_int_unchecked behaves exactly the same as conversion with as (for relevant types) as of Rust 1.42 (before rust-lang/rust#10184) is fixed. What does Miri do for as?

[RalfJung]

Do you have a more specific question?

Hm, if you are not familiar with apfloat then probably it would make most sense if you could look at the testcases. The run-pass test_cast tests are all tests that should be allowed for the unsafe intrinsics, and the tests in the compile-fail folder are all tests that demonstrate various examples of UB with the intrinsic.

Outside of const eval, float_to_int_unchecked behaves exactly the same as conversion with as (for relevant types) as of Rust 1.42 (before rust-lang/rust#10184) is fixed. What does Miri do for as?

For as, miri simply uses apfloat to_u128/to_i128, which performs a saturating cast.
The goal here is less to implement what rustc does and more to implement the spec -- so for all legal uses of the intrinsic, it should be the same as rustc, but for all illegal uses (causing UB), this must raise an error so Miri can inform the user that there was UB.

[SimonSapin]

I think this test is correct, but it looks misleading or confusing. 340282366920938463463374607431768211455 (the integer) is u128::MAX, so at first glance the conversion should succeed. But the 340282366920938463463374607431768211455.0f64 float is in fact not equal to that integer. That integer cannot be represented exactly in f64, so the Rust parser presumably rounds to the nearest representable float which is one more, 2^128, and does indeed overflow conversion to u128.

(I’m using Python to figure this out since it has infinite-capacity integers and f64 floats. a = 340282366920938463463374607431768211455; (hex(a), int(float(a)) - a, a.bit_length()) prints ('0xffffffffffffffffffffffffffffffff', 1, 128).)

But the point of these tests is not to test the Rust parser. I feel it’d be better to pick a decimal representation of the float that would, if precision and range were infinite, have the same numerical value as the actual f64 value does.

In fact it’d be even better if the source code of test cases could contain something closer to the memory representation of f64. Hex float syntax exists, but not in Rust literals. I don’t know if this would be worth using a library for parsing it from strings. I see some other tests use f32::from_bits with hex integers, but that’s much harder for humans to read.

[RalfJung]

FWIW, that number here and these from_bits constants are all derived from https://github.com/WebAssembly/testsuite/blob/master/conversions.wast.

I could change this to u128::MAX as f64, if you think that's better.

[Diggsey]

Is there a reason the upper and lower bounds for each integer type could not be determined ahead of time, reducing this code to just:

match {integer_type} {
    I32 => ({min}..={max}).contains(f),
    ...
}

?

[RalfJung]

I don't see how that would handle edge cases correctly. For example, unchecked_cast(u8::MAX as f64): u8 is legal but unchecked_cast(u64::MAX as f32): u64 is UB (because the int-to-float cast rounds up). Where are the casts in your example?

Also that match alone would probably be longer than the code I got here.

[RalfJung]

I changed the test @SimonSapin commented on. Otherwise I think this has pretty solid test coverage and the implementation matches what @hanna-kruppe wrote at #1264 (comment). So, I'll land this.

@bors r+

[bors]

📌 Commit bb38ab4 has been approved by RalfJung

[bors]

⌛ Testing commit bb38ab4 with merge 45113eb...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 45113eb to master...