Range of Floats and Enumerable strange behaviour

straight-shoota · May 10, 2024, 4:14pm

I’m proposing to change this:

`Range` includes `Enumerable` but doesn't always implement it

opened 04:14PM - 10 May 24 UTC

kind:bug status:discussion topic:stdlib:collection

`Range` is quite a special type because parts of its behaviour depends a lot on …which generic arguments are chosen. This is a continuation of #13121 specifically for `#each`. `Range(B, E)` includes `Enumerable(B)` (and `Iterable(B)`, but I'll focus on the former). The only interface that `Enumerable` requires is the method `#each`. `Range` defines `#each`, but it's not guaranteed to compile. The implementation depends on the generic argument type `B`. That type must respond to `#succ` or `Range#each` doesn't compile. ```cr # Int32#succ is defined (1..2).each {} # fine # Float64#succ is not defined (1.0..2.0).each {} # Error: undefined method 'succ' for Float64 ``` It think this is okay when you directly call `#each` on a range of floats. This method doesn't work there so it's fine to get an error. But there's a big problem: the module inclusion poisens *all* implementations of the same `Enumerable(T)` type. ```cr # This instantiates `Range(Float64, Float64)` as an implementation of `Enumerable(Float64)` # Comment this line and the program compiles 1.0..2.0 [1.0, 2.0].as(Enumerable(Float64)).each {} # Error: undefined method 'succ' for Float64 ``` The actual type here is `Array(Float64)` and calling `#each` should work for that and all other, actual implementations of `Enumerable(Float64)`. `Range` cannot possibly implement `Enumerable(Float64)`[^1], but it still includes that module. This is bad. Unfortunately, it’s currently not possible to express such a conditional implementation in the type system. This would be ideal if we could eventually do something like this (pseudo-like code that probably will never work that way): ```cr struct Range(B, E) {% if B.instance_responds_to?(:succ) %} include Enumerable(B) def each(&) # ... end {% end %} end ``` For the time being, it might be best to follow https://github.com/crystal-lang/crystal/pull/13278 and turn the compile time error into a runtime error. This can lead to situations where a runtime exception could've been prevented at compile time, which would be a better solution. But I think it's necessary in order to avoid unintended breakage for valid code. This is hard to debug and can be quite a surprise (see https://forum.crystal-lang.org/t/range-of-floats-and-enumerable-strange-behaviour/6833). [^1]: At least not in a meaningful way. It could just add `Float64::EPSILON`. Or raise.