Enumerable(T)#to_a regression with interface type

[Blacksmoke16]

It seems #12653 introduced a regression when calling #to_a on an enumerable of a module type. For example,

private struct SomeStruct(T, S)
  include Indexable(T)

  @values = [One.new, Two.new]

  def size : Int32
    S
  end

  # # :nodoc:
  def unsafe_fetch(index : Int) : T
    case index
    when 0 then @values[0]
    when 1 then @values[1]
    else
      raise ""
    end
  end
end

module SomeInterface; end

record One do
  include SomeInterface
end

record Two do
  include SomeInterface
end

pp SomeStruct(SomeInterface, 2).new.to_a

Works fine on 1.10.0

$ docker run --rm -it -w /app -v $PWD:/app crystallang/crystal:1.10.0-alpine crystal test.cr
[One(), Two()]

Fails on 1.11.0

$ docker run --rm -it -w /app -v $PWD:/app crystallang/crystal:1.11.0-alpine crystal test.cr
Showing last frame. Use --error-trace for full trace.

In /usr/share/crystal/src/enumerable.cr:2000:14

 2000 | def to_a : Array(T)
                   ^
Error: method SomeStruct(SomeInterface, 2)#to_a must return Array(SomeInterface) but it is returning Array(One | Two)

[straight-shoota]

That's an interesting problem.
The underlying cause is a discrepancy in the return types throughought the interface API. SomeStruct#unsafe_fetch actually returns One | Two, which matches T (=SomeInterface) but is not techincally the same.

I suppose the proper way to fix this is to revert the changes to the non-yielding #to_a in #12653 so that it keepy returning Array(T). Other types than Enumerable might be affected as well.
Indexable should probably get an optimized override with the default capacity.

That would still leave a bit of a weird situation that to_a and to_a(&.itself) are typed differently. But I suppose that's a limitation of the type system that we need to live with. (And usually it shouldn't be much of an issue assuming everything is working correctly).

[straight-shoota]

Actually, if you put the correct type on SomeStruct#unsafe_fetch by casting the return value to T explicitly, the same error appears

So it turns out the actual issue is Object#itself, called from Enumerable#to_a. It resolves the interface type SomeInterface to a union of all its implementors (One | Two).
The fix stays the same though.

[straight-shoota]

We might want to think about this behaviour of Object#itself, which seems missaligned with its purpos as a no-op. It appears to not be a no-op for the type system.

It should be worth to investigat other uses of this method whether it could lead to similar issues in edge cases like this.

[HertzDevil]

That sounds exactly like #12846

[straight-shoota]

I looked for other potential issues in relation to .itself. Only found Enumerable#tally:

SomeStruct(SomeInterface, 2).new.tally # Error: method SomeStruct(SomeInterface, 2)#tally must return Hash(SomeInterface, Int32) but it is returning Hash(One | Two, Int32)

@Blacksmoke16 If you want to add that to #14447, the fix is

--- i/src/enumerable.cr
+++ w/src/enumerable.cr
@@ -1970,7 +1970,7 @@ module Enumerable(T)
   # ["a", "b", "c", "b"].tally # => {"a"=>1, "b"=>2, "c"=>1}
   # ```
   def tally : Hash(T, Int32)
-    tally_by(&.itself)
+    tally_by(Hash(T, Int32).new, &.itself)
   end