Retrofit `BuildFrom` into the new design #97

szeiger · 2017-06-02T13:43:59Z

Extension of #85, work in progress

`BuildFrom` is like `FromSpecificIterable` but with an extra `From` argument, like in the final version of scala#45. `FromSpecificIterable` existed conceptually in that version as `BuildFrom[Any, …]` but didn’t have a separate type. This new version has separate abstractions for buildable (strict) collection types in the form of `StrictBuildFrom` and `FromSpecificIterableWithBuilder`. Since we can get a surrogate builder (through one of the new `Builder.from` methods) for any lazy collection and we can restrict code to work only with strict collections via the `Buildable` trait, this is not strictly necessary. The only reason for separating the `Builder` abstractions is to avoid exposing them in `FromSpecificIterable`. Even though everything can be built in a strict way, these abstractions sit on top of the lazy ones, not below them.

szeiger · 2017-06-12T14:19:54Z

BuildFrom is like FromSpecificIterable but with an extra From
argument, like in the final version of #45. FromSpecificIterable
existed conceptually in that version as BuildFrom[Any, …] but didn’t
have a separate type.

This new version has separate abstractions for buildable (strict)
collection types in the form of StrictBuildFrom and
FromSpecificIterableWithBuilder. Since we can get a surrogate builder
(through one of the new Builder.from methods) for any lazy collection
and we can restrict code to work only with strict collections via the
Buildable trait, this is not strictly necessary. The only reason for
separating the Builder abstractions is to avoid exposing them in
FromSpecificIterable. Even though everything can be built in a strict
way, these abstractions sit on top of the lazy ones, not below them.

szeiger · 2017-06-12T14:20:30Z

@odersky @julienrf I just updated this with the finished version of the design we discussed at ScalaDays.

szeiger · 2017-06-12T14:22:00Z

src/test/scala/strawman/collection/test/TraverseTest.scala

+    }.map(_.result)
+
+  // ...or use BuildFrom to abstract over both and also allow building arbitrary collection types
+  def optionSequence2[CC[X] <: Iterable[X], A, To](xs: CC[Option[A]])(implicit bf: BuildFrom[CC[Option[A]], A, To]): Option[To] =


This should work for the tests below if you ask for a StrictBuildFrom instead of a BuildFrom but it doesn't because Set is not Buildable at the moment. I'm currently working on changing that but I ran into some complications, so I wanted to get this PR out first. Making Sets buildable is a separate concern.

szeiger · 2017-06-12T14:25:10Z

src/main/scala/strawman/collection/SortedOps.scala


  implicit def ordering: Ordering[A]

+  def sortedIterableFactory: SortedIterableFactory[CC]


This PR also adds the another factor kinds. So far every collection exposes an IterableFactory. Now in addition to that every sorted collection has a SortedIterableFactory. We'll probably want to add MapFactory and SortedMapFactory types to the appropriate collection types as well.

julienrf

Thanks for working on this!

I left a few comments. Could you check that you are able to implement all the tests that I implemented in #91?

Also, I really feel like this PR introduces a lot of complexity in factories. I wonder if we could find a better compromise between #91 (which only introduces strict factories and implicit builder factories) and this PR.

julienrf · 2017-06-13T07:44:30Z

src/main/scala/strawman/collection/Factories.scala

+
+object BuildFrom {
+  /** Build the source collection type from an IterableOps */
+  implicit def buildFromIterableOps[C[X] <: IterableOps[X, C, _], A, E]: BuildFrom[C[A], E, C[E]] = new BuildFrom[C[A], E, C[E]] {


The naming convention is to use CC for Collection Constructors

julienrf · 2017-06-13T08:02:44Z

src/main/scala/strawman/collection/Factories.scala

+/** A more specific `FromSpecificIterable` for strict collection types which can provide a `Builder`. */
+trait FromSpecificIterableWithBuilder[-A, +C] extends Any with FromSpecificIterable[A, C] with StrictBuildFrom[Any, A, C] {
+  def newBuilder(from: Any): Builder[A, C] = newBuilder
+  def newBuilder: Builder[A, C]


I prefer that we add parens to emphasize the fact that each created builder has its own identity: def newBuilder(): Builder[A, C].

We have many methods without a parameter list that can return objects with different identities, for example Iterable.tail or IterableFactory.empty. The name newBuilder in particular makes it clear that you get a new object. I would prefer to omit empty parameter lists wherever possible.

julienrf · 2017-06-13T08:08:32Z

src/main/scala/strawman/collection/Iterable.scala

+  *
+  * All factories provided by such a collection must be of the `*WithBuilder` variant
+  * even though this is not formally enforced through types.
+  *


This is unfortunate that we can not enforce this constraint :(

We can relax it from must to should though. Some strict collections may not be able to provide a more efficient builder anyway so it would be OK to fall back to a an ArrayBuffer. Implementing a WithBuilder factory would be an optimization, just like overriding a default implementation of some collection operation.

julienrf · 2017-06-13T08:09:30Z

src/main/scala/strawman/collection/SortedOps.scala

@@ -3,10 +3,12 @@ package strawman.collection
 import scala.{Ordering, Option, Some}

 /** Base trait for sorted collections */
-trait SortedOps[A, +C] {
+trait SortedOps[A, +C, +CC[_]] {


So far we usually order the type parameters such that CC is before C.

julienrf · 2017-06-13T08:11:42Z

src/main/scala/strawman/collection/SortedMap.scala

-     with SortedOps[K, C] {
+     with SortedOps[K, C, SortedSet] {
+
+  def sortedIterableFactory = SortedSet


Does that mean that when I navigate through the sortedIterableFactory of a SortedMap then I get back a SortedSet?

Yes. This is a side-effect of SortedOps being shared by sorted sets and maps. I was also baffled by this at first but it does make sense. A sorted map is sorted by key. If you assume a standard Ordering on tuples, it degrades nicely to a sorted set. The ordering for (K,V) with unique K values is the same and with K already being unique, so is (K,V).

julienrf · 2017-06-13T08:14:08Z

src/main/scala/strawman/collection/immutable/TreeSet.scala

@@ -113,4 +115,5 @@ object TreeSet extends SortedIterableFactory[TreeSet] {
      case _ => empty[E] ++ it
    }

+  def newBuilder[A : Ordering](): Builder[A, TreeSet[A]] = new ArrayBuffer[A].mapResult(sortedFromIterable[A] _)


What’s the advantage of using an intermediate ArrayBuffer over directly building a TreeSet?

Hm, right, the current collections append directly to the immutable TreeSet. We should do the same.

I'll address those issues in the Set PR. Doing it the old way requires a SetBuilder, which in turn requires a + operation on Sets that we do not have yet...

julienrf · 2017-06-13T08:15:38Z

src/main/scala/strawman/collection/immutable/BitSet.scala

@@ -27,6 +27,7 @@ sealed abstract class BitSet
  def empty: BitSet = BitSet.empty

  def iterableFactory = Set
+  def sortedIterableFactory = SortedSet


Why can’t we tell that the factory of a BitSet collection is the BitSet object?

Because the SortedIterableFactory can build a collection for every element type with an Ordering.

julienrf · 2017-06-13T08:17:25Z

src/main/scala/strawman/collection/mutable/TreeSet.scala


  def empty[A : Ordering]: TreeSet[A] = new TreeSet[A]()

  def sortedFromIterable[E : Ordering](it: collection.Iterable[E]): TreeSet[E] = Growable.fromIterable(empty[E], it)

+  def newBuilder[A : Ordering](): Builder[A, TreeSet[A]] = new ArrayBuffer[A].mapResult(sortedFromIterable[A] _)


Why not directly grow a TreeSet?

odersky · 2017-06-13T14:30:37Z

Hi Stefan, can you push this the strawman repo? Makes it easier for me to check it out locally and play with it.

szeiger · 2017-06-13T14:41:05Z

@odersky Done. I can confirm that this PR is the cause of the problem. Master compiles fine on the current Dotty nightly, this PR fails.

There is no way to get a BuildFrom purely by type in this design, so we need to call parseCollection explicitly with a companion object.

szeiger · 2017-06-14T17:22:52Z

I've added the missing map factory support and ported some of the additional tests from #91 (which use Map and SortedMap types). Next up: Collapsing strict building and lazy building again (like in #45), which should remove a lot of the boilerplate.

julienrf · 2017-06-15T08:29:34Z

src/main/scala/strawman/collection/Factories.scala

  def fromSpecificIterable(it: Iterable[A]): C
 }

+/** A more specific `FromSpecificIterable` for strict collection types which can provide a `Builder`. */
+trait FromSpecificIterableWithBuilder[-A, +C] extends Any with FromSpecificIterable[A, C] with StrictBuildFrom[Any, A, C] {


Do we need this type? It does not seem to be consumed anywhere.

Not per so but we do need FromSpecificIterable with StrictBuildFrom

julienrf · 2017-06-15T08:35:41Z

src/test/scala/strawman/collection/test/GenericTest.scala

+  def parseCollection[A, C](bf: BuildFrom[Any, A, C])
+                                    (implicit parseA: Parse[A]): Parse[C] = { (s: String) =>
+    val parts = s.split("\\|")
+    parts.foldLeft[Option[Builder[A, C]]](Some(Builder.from(bf, null))) { (maybeBuilder, s) =>


The fact that the first parameter of BuildFrom is Any and the second parameter of Build.from is null gives me the feeling that BuildFrom is not really the appropriate abstraction.

FromSpecificIterable should be sufficient. AFAIR I ran into some problem with the implicits with that.

This is an extension of scala#97 which provides Builders in BuildFrom and FromSpecificIterable for all collection types because we need them for always-strict operations like groupBy and we can always get them anyway. This greatly simplifies the design of BuildFrom by getting rid of half of the factory types.

julienrf changed the base branch from master to equals June 4, 2017 06:14

julienrf changed the base branch from equals to master June 4, 2017 06:14

julienrf mentioned this pull request Jun 12, 2017

Add groupBy #108

Closed

szeiger force-pushed the expose-factories-cb branch from 4efa362 to 720b52d Compare June 12, 2017 14:18

szeiger changed the title ~~New CanBuildFrom abstraction~~ Retrofit BuildFrom into the new design Jun 12, 2017

szeiger commented Jun 12, 2017

View reviewed changes

julienrf reviewed Jun 13, 2017

View reviewed changes

julienrf mentioned this pull request Jun 14, 2017

Pull up the newBuilder method from Buildable to IterableOps. #110

Merged

szeiger added 2 commits June 14, 2017 17:29

Add map factories and more tests from scala#91

7afd191

Add GenericTest from scala#91

ec368a0

There is no way to get a BuildFrom purely by type in this design, so we need to call parseCollection explicitly with a companion object.

julienrf reviewed Jun 15, 2017

View reviewed changes

julienrf mentioned this pull request Jun 15, 2017

Implicit builders on top of #110 #112

Closed

szeiger mentioned this pull request Jun 16, 2017

Unify strict and lazy building #114

Closed

julienrf closed this Jun 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrofit `BuildFrom` into the new design #97

Retrofit `BuildFrom` into the new design #97

szeiger commented Jun 2, 2017

szeiger commented Jun 12, 2017

szeiger commented Jun 12, 2017

szeiger Jun 12, 2017

szeiger Jun 12, 2017

julienrf left a comment

julienrf Jun 13, 2017

julienrf Jun 13, 2017

szeiger Jun 13, 2017

julienrf Jun 13, 2017

szeiger Jun 13, 2017

julienrf Jun 13, 2017

julienrf Jun 13, 2017

szeiger Jun 13, 2017

julienrf Jun 13, 2017

szeiger Jun 13, 2017

szeiger Jun 13, 2017

julienrf Jun 13, 2017

szeiger Jun 13, 2017

julienrf Jun 13, 2017

odersky commented Jun 13, 2017

szeiger commented Jun 13, 2017

szeiger commented Jun 14, 2017

julienrf Jun 15, 2017

szeiger Jun 15, 2017

julienrf Jun 15, 2017

szeiger Jun 15, 2017


		implicit def ordering: Ordering[A]

		def sortedIterableFactory: SortedIterableFactory[CC]

Retrofit BuildFrom into the new design #97

Retrofit BuildFrom into the new design #97

Conversation

szeiger commented Jun 2, 2017

szeiger commented Jun 12, 2017

szeiger commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Jun 13, 2017

szeiger commented Jun 13, 2017

szeiger commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Retrofit `BuildFrom` into the new design #97

Retrofit `BuildFrom` into the new design #97