Reject near-boundary and NaN values for Float-to-Int conversions

[HertzDevil]

Fixes #10421. Fixes #11105.

2147483648_f32.to_i32     # raises OverflowError
Float32::NAN.to_i32       # raises OverflowError
Float32::INFINITY.to_u128 # raises OverflowError

None of the above expressions currently raise.

[HertzDevil]

It seems more of compiler-rt's methods need to be ported manually if we want 128-bit integer support on Windows...

[ysbaddaden]

I'm having a hard time to understand why trying to convert a NaN to an int would be an overflow. It's not out of bounds like an overflow, a NaN just can't be represented as an integer (makes no sense), and reporting an overflow feels confusing.

Ruby raises a FloatDomainError (NaN) exception for example.

In plain Crystal I'd merely raise ArgumentError.new("NaN") if nan?.

[straight-shoota]

Maybe OverflowError could be interpreted more generically to also include conversion errors where the value can't be represented in the target type.

But that's probably not a good idea because it would apply to many other conversion errors as well. So yeah, ArgumentError seems like a good idea.

[HertzDevil]

There is no OverflowError in Ruby; instead they call it RangeError, and then make FloatDomainError a subclass of it. That means Ruby code that catches overflow errors will also catch domain errors:

begin
  (0.0 / 0.0).to_i
rescue RangeError
  puts 1 # => 1
end

I think we should copy this or simply rename OverflowError to RangeError.

[HertzDevil]

Still thinking about the best way to approach this. For performance reasons (for the compiler) it seems we should mimic OverflowError:

# :nodoc:
fun __crystal_raise_nan : NoReturn
  raise ArgumentError.new("not-a-number")
end

including making it a "well-known function".

[straight-shoota]

IMO overflow and conversion failure are the same class of error. I would use the same error class for both. Currently, we have OverflowError. Maybe that name doesn't fit very well with conversion failure. But I'd still suggest to use that. We can see to maybe rename OverflowError to a better name that also describes conversion failures (MathError? ArithmeticError?). It seems easier to go with the error type we already have instead of using a different one and then later introducing a new concept to reunite them. Alternatively, we could introduce a new error type as a parent for OverflowError (and maybe merger OverflowError into that later).

ArgumentError doesn't seem to fit for this.

[oprypin]

This spec's definition is kinda circular.
Also would be nice to test that it doesn't overflow right below the cutoff.
I'm not sure if my suggestion is better, let me know what you think.

Suggested change

	it "raises overflow if equal to Int::MAX (#11105)" do
	# these examples hold because the integer would be rounded _up_ to the
	# nearest representable float
	expect_raises(OverflowError) { Float32.new!(Int32::MAX).to_i32 }
	expect_raises(OverflowError) { Float32.new!(UInt32::MAX).to_u32 }
	expect_raises(OverflowError) { Float32.new!(Int64::MAX).to_i64 }
	expect_raises(OverflowError) { Float32.new!(UInt64::MAX).to_u64 }
	expect_raises(OverflowError) { Float32.new!(Int128::MAX).to_i128 }
	expect_raises(OverflowError) { Float64.new!(Int64::MAX).to_i64 }
	expect_raises(OverflowError) { Float64.new!(UInt64::MAX).to_u64 }
	expect_raises(OverflowError) { Float64.new!(Int128::MAX).to_i128 }
	expect_raises(OverflowError) { Float64.new!(UInt128::MAX).to_u128 }
	end
	describe "handles overflow near Int::MAX (#11105)" do
	describe "for Float32" do
	it "doesn't overflow below" do
	(n = 2.1474835e+9_f32).to_i32.should eq(n)
	(n = 4.294967e+9_f32).to_u32.should eq(n)
	(n = 9.2233715e+18_f32).to_i64.should eq(n)
	(n = 1.8446743e+19_f32).to_u64.should eq(n)
	(n = 1.7014117e+38_f32).to_i128.should eq(n)
	(n = Float32::MAX).to_u128.should eq(n)
	end
	it "overflows above" do
	expect_raises(OverflowError) { 2.1474836e+9_f32.to_i32 }
	expect_raises(OverflowError) { 4.2949673e+9_f32.to_u32 }
	expect_raises(OverflowError) { 9.223372e+18_f32.to_i64 }
	expect_raises(OverflowError) { 1.8446744e+19_f32.to_u64 }
	expect_raises(OverflowError) { 1.7014118e+38_f32.to_i128 }
	end
	end
	describe "for Float64" do
	it "doesn't overflow below" do
	(n = 9.223372036854775e+18).to_i64.should eq(n)
	(n = 1.844674407370955e+19).to_u64.should eq(n)
	(n = 1.7014118346046921e+38).to_i128.should eq(n)
	(n = 3.4028236692093843e+38).to_u128.should eq(n)
	end
	it "overflows above" do
	expect_raises(OverflowError) { 9.223372036854776e+18.to_i64 }
	expect_raises(OverflowError) { 1.8446744073709552e+19.to_u64 }
	expect_raises(OverflowError) { 1.7014118346046923e+38.to_i128 }
	expect_raises(OverflowError) { 3.402823669209385e+38.to_u128 }
	end
	end
	end

[oprypin]

I think you resolved one of my two concerns. But, I do think the idea of writing as exact number literals should be seriously considered. Because, if both the Float32.new! part and the .to_u64 part break at the same time, it's still possible for this spec to pass. I will not push very hard on this, but curious to see some feedback.