Revive Iterator#flatten from type inference hell

[makenowjust]

In version 0.16.0, it said "I'LL BE BACK" and went down into a forge.

Today... "I'M BACK"

[makenowjust]

I believe this tail recursive method is optimized by LLVM, isn't it? If not, I'll rewrite it to loop version.

[asterite]

I don't know, you can try to check if LLVM generates tailcc (or something like that, can't remember now) when doing --emit llvm-ir (though finding the function definition for this method might be tricky, it will be named something like Iterator#next)

[asterite]

I just checked this and LLVM turns this into a loop... amazing :-)

[makenowjust]

Thanks! LLVM is awesome!

[RX14]

I'd love a https://godbolt.org/ for crystal. Seems like it wouldn't be too hard to add, although I should probably concentrate on finishing some of my other projects...

[asterite]

An easy way to combine two types (get a union of two expressions) is to use ||, I think (didn't try it) that this can work too:

iter || iterator_type(iter.next)

[asterite]

Wow, amazing!! I didn't know it could be done with the current type system. And the code is so simpler, much simpler than the previous version.

I know this is how the flatten iterator worked previous, but there are some differences with Array#flatten. For example:

a = [[1, 2], [[3, 4], [5, {6, 7}, {8 => 9}]]]
p a.flatten
p a.each.flatten.to_a

b = [[1, 2], [[3, 4], [5, {6, 7}, {8 => 9}.each]]]
p b.each.flatten.to_a

Output:

[1, 2, 3, 4, 5, {6, 7}, {8 => 9}]
[1, 2, 3, 4, 5, 6, 7, {8 => 9}]
[1, 2, 3, 4, 5, 6, 7, 8, 9]

So it seems tuples are flattened too, and also iterators of tuples. Maybe tuples shouldn't be flattened, but this is something we need to discuss and define (in Ruby there's only Array so the rule is to flatten Array, in Crystal we have more types, like Tuple and Slice, maybe these shouldn't be flattened, only Array). The problem is how to distinguish this in the case of Iterable and Iterator types.

[makenowjust]

Perhaps we cannot find the way to distinguish this (it is hard problem), however I can proposal two methods:

• Iterator#flatten flattens only Iterator, no Iterable flatten, just like Array#flatten flattens only Array.
• Iterator#flatten_iterable flattens Iterator and Iterable, same as current Iterator#flatten.

How is it?

[makenowjust]

@asterite Is the order a rabbit?

It works fine:

a = [[1, 2], [[3, 4], [5, {6, 7}, {8 => 9}]]]
p a.each.flatten.to_a                #=> [1, 2, 3, 4, 5, 6, 7, {8 => 9}]
p a.each.flatten(except: Tuple).to_a #=> [1, 2, 3, 4, 5, {6, 7}, {8 => 9}]

a = [[1, 2], [[3, 4], [5, {6, 7}, {8 => 9}.each]]]
p a.each.flatten(except: Tuple).to_a #=> [1, 2, 3, 4, 5, {6, 7}, {8, 9}]

But, it cannot work:

p a.each.flatten(except: Array | Tuple)

# can't declare variable of generic non-instantiated type Array(T)

[asterite]

I'd leave flatten without options, and never flatten Tuple. A tuple is usually some data that you don't want to break apart, for example it can represent a point with x and y, or a pair of key and value. I think if we can add a fixed rule to never flatten Tuple it will be OK.

[makenowjust]

It is simplest solution that Tuple doesn't include Iterable. For example, Hash has each method to return an iterator, so it can be Iterable however is not. Why?

Iterable is a set of methods for short-hand. If Tuple is no Iterable, we can use each to call Iterator method explicitly. You thought this is good on Hash, but isn't on Tuple?

[asterite]

@makenowjust Oh, some time ago Hash wasn't Enumerable so it couldn't be Iterable. When we made it Enumerable I forgot it can also be Iterable. Now it is :-)

I still think Tuple shouldn't be flattened.

[makenowjust]

@asterite Implemented the version not to flatten tuples.

[asterite]

Oooh... now I understand what you said about Tuple, Hash and Iterable. For example this:

[1, {2 => 3}].each.flatten.to_a # => [1, {2, 3}]

The above should probably be [1, {2 => 3}]. Maybe the problem is iterating every Iterable. Maybe only Array and Iterator should be flattened, which is the same behaviour as Array#flatten.

If I change Iterable to Array inside Flatten, and remove Tuple, it seems to work:

diff --git a/spec/std/iterator_spec.cr b/spec/std/iterator_spec.cr
index 6d6a7bd..548e6ea 100644
--- a/spec/std/iterator_spec.cr
+++ b/spec/std/iterator_spec.cr
@@ -588,7 +588,7 @@ describe Iterator do
       iter.next.should eq(1)
       iter.next.should eq({2, 3})
       iter.next.should eq(4)
-      iter.next.should eq({5, 6})
+      iter.next.should eq({5 => 6})
       iter.next.should eq(7)
       iter.next.should be_a(Iterator::Stop)
     end
diff --git a/src/iterator.cr b/src/iterator.cr
index 93e996f..f830a52 100644
--- a/src/iterator.cr
+++ b/src/iterator.cr
@@ -388,7 +388,7 @@ module Iterator(T)
     Flatten(typeof(Flatten.iterator_type(self)), typeof(Flatten.element_type(self))).new(self)
   end
 
-  private class Flatten(I, T)
+  private struct Flatten(I, T)
     include Iterator(T)
 
     @iterator : I
@@ -402,12 +402,10 @@ module Iterator(T)
 
     def next
       case value = @generators.last.next
-      when Tuple
-        value
       when Iterator
         @generators.push value
         self.next
-      when Iterable
+      when Array
         @generators.push value.each
         self.next
       when Stop
@@ -434,11 +432,9 @@ module Iterator(T)
       case element
       when Stop
         raise ""
-      when Tuple
-        element
       when Iterator
         element_type(element.next)
-      when Iterable
+      when Array
         element_type(element.each)
       else
         element
@@ -447,11 +443,9 @@ module Iterator(T)
 
     def self.iterator_type(iter)
       case iter
-      when Tuple
-        raise ""
       when Iterator
         iter || iterator_type iter.next
-      when Iterable
+      when Array
         iterator_type iter.each
       else
         raise ""

(I also changed Flatten to be a struct because it looks its instance vars are never re-assigned)

What do you think?

Sorry this discussion turned out longer than what I expected 😊

[makenowjust]

@asterite I think it is good too. I applied your patch.

[asterite]

@makenowjust Awesome!! Thank you so much for this.

Forgot to say this, best issue title ever! 🤘