crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.26k stars 1.61k forks source link

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

Open straight-shoota opened 2 months ago

straight-shoota commented 2 months ago

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.

# 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 poisons all implementations of the same Enumerable(T) type.

# 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):

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.

crysbot commented 2 months ago

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/range-of-floats-and-enumerable-strange-behaviour/6833/5