Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
straight-shoota opened this issue May 10, 2024 · 1 comment
Open

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

straight-shoota opened this issue May 10, 2024 · 1 comment

Comments

@straight-shoota
Copy link
Member

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 poisens 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 #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).

Footnotes

  1. At least not in a meaningful way. It could just add Float64::EPSILON. Or raise.

@crysbot
Copy link

crysbot commented May 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants