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

Revive Iterator#flatten from type inference hell #3703

Merged
merged 3 commits into from Dec 18, 2016

Conversation

makenowjust
Copy link
Contributor

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

Today... "I'M BACK"

@makenowjust makenowjust force-pushed the feature/iterator/flatten branch 2 times, most recently from fac4b67 to a3e2796 Compare December 15, 2016 15:55
end

def next
case value = @generators.last.next
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LLVM is awesome!

Copy link
Contributor

@RX14 RX14 Dec 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

def self.iterator_type(iter)
case iter
when Iterator
[iter, iterator_type(iter.next)].sample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

@asterite Implemented the version not to flatten tuples.

@asterite
Copy link
Member

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
Copy link
Contributor Author

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

@asterite
Copy link
Member

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

Forgot to say this, best issue title ever! 🤘

@asterite asterite merged commit b8334ae into crystal-lang:master Dec 18, 2016
@asterite asterite added this to the 0.20.2 milestone Dec 18, 2016
@makenowjust makenowjust deleted the feature/iterator/flatten branch December 18, 2016 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants