Unify strict and lazy building #114

szeiger · 2017-06-16T12:08:30Z

This is an extension of #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.

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

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.

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

On one hand, introducing the newBuilder method in all factories is tempting because it makes factories a lot simpler! But on the other hand I’d rather not have them in View and LazyList factories.

In summary, we will have to decide on

whether we want to keep or not the public builders in all the factories (including View and LazyList)
in comparison with Implicit builders on top of #110 #112, do you think that would be doable to have generic tests use implicit instances?
also in comparison with Implicit builders on top of #110 #112: in this PR we have one type of implicit builder: BuildFrom, which takes a From parameter and which has a fromSpecificIterable method. Whereas in Implicit builders on top of #110 #112 we have CanBuild which has no From type parameter and no fromSpecificIterable method. Then, the idea in Implicit builders on top of #110 #112 is to derive BuildFrom and BuildTo from this CanBuild. But if in most cases we find it useful to have the From parameter maybe it makes sense to just have BuildFrom, as in this PR.

Do other people have thoughts?

julienrf · 2017-06-16T13:16:33Z

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

+
+  def sortedIterableFactory = SortedSet
+
+  def sortedMapFactory: SortedMapFactory[CC]

  protected[this] def sortedMapFromIterable[K2, V2](it: collection.Iterable[(K2, V2)])(implicit ordering: Ordering[K2]): CC[K2, V2]


Shouldn’t this be implemented?

protected[this] def sortedMapFromIterable[K2, V2](it: collection.Iterable[(K2, V2)])(implicit ordering: Ordering[K2]): CC[K2, V2] = sortedMapFactory.sortedFromIterable(it)

Yes, I think we can delegate this methods now the same way it is already done for fromSpecificIterable.

julienrf · 2017-06-16T13:20:01Z

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

-
-trait IterableFactoryWithBuilder[+CC[_]] extends IterableFactory[CC] {
-  def newBuilder[A](): Builder[A, CC[A]]
+  def newBuilder[A](): Builder[A, CC[A]] = new ArrayBuffer[A]().mapResult(fromIterable _)


I’m not sure this default implementation will be useful. Will it be useful elsewhere than for ArrayBuffer and ImmutableArray?

This default is used for all collection types that cannot provide a better builder. This includes all lazy collections.

In the case of lazy collections, what about using an ImmutableArray instead of an ArrayBuffer to avoid the extra space?

You don't know the size ahead of time so you can either copy everything again or live with some wasted space.

I mean that even after an ArrayBuffer has been completely built there might still be some unused extra space. If you build an ImmutableArray (or just call toArray on the ArrayBuffer, which is almost exactly the same thing), then you trim the end of the buffer. (But yes, that means one additional copy)

Is there any precedent for this in other collection methods? AFAIK trimming to the current size is always an explicit operation. The default is to avoid copying at the expense of some unused space.

julienrf · 2017-06-16T13:22:37Z

src/main/scala/strawman/collection/SortedSet.scala

@@ -9,7 +9,7 @@ trait SortedSet[A] extends Set[A] with SortedSetOps[A, SortedSet, SortedSet[A]]

 trait SortedSetOps[A, +CC[X], +C <: SortedSet[A]]
  extends SetOps[A, Set, C]
-     with SortedOps[A, C] {
+     with SortedOps[A, C, CC] {

  protected[this] def sortedFromIterable[B: Ordering](it: Iterable[B]): CC[B]


It should be possible to implement this by delegating to sortedIterableFactory.

julienrf · 2017-06-16T13:33:59Z

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

+
+  @Test
+  def genericTest: Unit = {
+    assert(Parse.parseCollection[Int, immutable.List[Int]](immutable.List).parse("1|2|3").contains(1 :: 2 :: 3 :: immutable.Nil))


It’s unfortunate that the FromSpecificIterable can not be available implicitly. That’s a requirement to do generic typeclass derivation.

julienrf · 2017-06-16T13:37:53Z

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

 /**
  * Builds a collection of type `C` from elements of type `A`
  * @tparam A Type of elements (e.g. `Int`, `Boolean`, etc.)
  * @tparam C Type of collection (e.g. `List[Int]`, `TreeMap[Int, String]`, etc.)
  */
-trait FromSpecificIterable[-A, +C] extends Any {
+trait FromSpecificIterable[-A, +C] extends Any with BuildFrom[Any, A, C] {


Where do we rely on the fact that FromSpecificIterable is a BuildFrom?

Whenever you pass a factory object instead of an implicit BuildFrom. All factories provide an implicit conversion to FromSpecificIterable.

julienrf · 2017-06-16T14:51:56Z

Also, we have to figure out the issue with the dotty compilation in this PR.

szeiger · 2017-06-16T15:17:49Z

@julienrf As discussed previously we want strict methods like groupBy on Iterable, so providing builders for everything is unavoidable. Building without a source collection is also supported here via FromSpecificIterable. This is unchanged from #85. The only difference is that FromSpecificIterable also provides the right Builder. This is now used in GenericTest instead of BuildFrom.

julienrf · 2017-06-16T15:19:23Z

As discussed previously we want strict methods like groupBy on Iterable, so providing builders for everything is unavoidable.

Well, this builder can be protected and used by operation implementations only (as in #110 and #111)

odersky · 2017-06-16T16:06:20Z

I'd like to review this but can only do it after PLDI. So it will be probably a week until you have the review. Thanks for holding that long...

Ichoran · 2017-06-17T22:18:42Z

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

+  * @tparam A Type of elements (e.g. `Int`, `Boolean`, etc.)
+  * @tparam C Type of collection (e.g. `List[Int]`, `TreeMap[Int, String]`, etc.)
+  */
+trait BuildFrom[-From, -A, +C] extends Any {


What does making the C a type parameter instead of a type member do for us?

I would prefer a type member (and I did use one in my original implementation in #45) but FromSpecificIterable in Martin's simplified version uses a type parameter so I used the same here. I think we should use type members for both. Unlike CanBuildFrom, lookup for BuildFrom and FromSpecificIterable is never determined by this type. It is strictly an "out" parameter.

Ichoran · 2017-06-17T22:22:26Z

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

+  }
+
+  /** Build the source collection type from a SortedMapOps */
+  implicit def buildFromSortedMapOps[CC[K, V] <: SortedMap[K, V] with SortedMapOps[K, V, CC, _], A, B, E : Ordering, F]: BuildFrom[CC[A, B], (E, F), CC[E, F]] = new BuildFrom[CC[A, B], (E, F), CC[E, F]] {


Most people won't have to look at this, but it's still pretty imposing with seven (?!) separate type parameters (five "real", two parameterizing CC).

Ichoran · 2017-06-17T22:38:51Z

I'm also quite comfortable with this approach. It feels a little more flexible but a little more imposing than Julien's #112. I can't easily tell what one could accomplish here that one couldn't there, but overall the type member strategy, despite requiring a little more magic, seems overall a bit friendlier.

Both are pretty good, though, for what the tests cover. Do either of you have a clear idea of what the most important differences are for user-level code? I wasn't able to easily distinguish them.

Also, I think there's a principle underlying builders that should simplify matters: the only reason you care about the From collection type is when you want it to be the same as the To type (or as close as possible). Otherwise, you just want to get the elements and you don't really care what From is (beyond any constraints you put on the type of the collection itself, but the method etc parameters will see to that). So you might care about From, A or A, To, but not about From, A, To.

This suggests to me that we probably don't need an explicit From, A, To set of types like in CBF, but can do with type members as in #112. But that's a hunch based on the logical structure; are there counterexamples?

julienrf · 2017-06-20T09:37:46Z

Do either of you have a clear idea of what the most important differences are for user-level code?

Both solutions seem to be equivalent. The only difference I noted is that in #112, we can get an implicit Parse[List[…]] instance (see here), whereas in this PR we have to explicitly provide the factory (see here). But I think it might just be a choice from @szeiger and that it should be possible to get implicit instances (wdyt, @szeiger?).

Also, in #112 I keep differentiating factories that have builders from those that don’t. I’d rather not expose a LazyList.newBuilder() method nor a View.newBuilder() method.

szeiger · 2017-06-26T14:04:03Z

It should be possible to add an implicit method to each factory type that provides a BuildFrom[Any, ...]. This would give you type-driven lookups. This may require also moving the implicits from the BuildFrom companion object to get the priorities right when the source type is known.

szeiger · 2017-06-26T14:37:12Z

One important difference is that in #112 AFAICT all Builders are constructed by expected target type, not driven by the source collection, whereas here the default implicits fall back to the source collection. This allows you to do transformations independent of the static type: The static type of the source determines the level of detail of see of the static type of the target but the implementation class is always the same. This fixes one of the consistency problem of the current collections' CanBuildFrom (where the type-driven lookup is similar to #112).

szeiger · 2017-06-26T14:41:44Z

Also note that the source-driven transformation is consistent with the new way of performing operations without CanBuildFrom. In the current collections it works in some cases but not in others because there are no advanced builder types for maps and sorted collections.

julienrf · 2017-06-26T15:33:08Z

in #112 AFAICT all Builders are constructed by expected target type, not driven by the source collection

They are if you ask for an implicit BuildFrom.

But if you ask for an implicit BuildTo then that’s the opposite: you directly ask for a builder for your target collection.

The static type of the source determines the level of detail of see of the static type of the target but the implementation class is always the same.

Indeed this feature is not supported by my PR: if the static type of the source is Seq then you’ll get a default Seq implementation as a result of the transformation, whatever the runtime type of the source collection was.

szeiger · 2017-06-27T12:57:12Z

I've been playing around with type-driven building. I run into the problem that I already encountered previously that BuildFrom for sorted maps and sorted sets are ambiguous. This is the reason why this PR has a BuildFromLowPriority but this solution doesn't work for implicits in the factories. I believe the solution is a SortedIterableOps in parallel to SortedMapOps so we get a clear split between unary and binary sorted collections.

This still leaves the problem of distinguishing between the static and dynamic BuildFrom instances. With From being contravariant, a BuildFrom[Any, ...] is preferred but we want it the other way around.

Ichoran · 2017-06-28T02:49:21Z

With regard to the actual type, not the statically visible type, being the collection: I think that's important behavior, probably important enough to favor this approach. But if it can be made to work with the simplified type parameters (pushing To to a type member, and using Aux under a friendly name if needed), that would be even better.

Why do the implicits in the factories not obey priority? Or is it just that you need to create a low priority thing for them each time which is awkward?

szeiger · 2017-06-28T12:12:40Z

For sorted maps you get a BuildFrom[..., ..., TreeSet[...]] and a BuildFrom[..., ..., TreeMap[...]], neither of which is more specific than the other. I'll squash and rebase this PR and I will also try to fix this issue by restricting buildFromSortedOps to a new SortedIterableOps type so it doesn't apply to SortedMapOps anymore.

szeiger · 2017-06-28T13:41:40Z

OK, restricting to SortedSetOps instead of SortedOps doesn't solve the problem. There are still ambiguities between either one of buildFromMapOps / buildFromSortedMapOps and buildFromIterableOps. This used to work in #45. It is not clear to me why it doesn't work anymore with the simplified design. Both Scala and Dotty give the same results and switching back to a type To type member doesn't make a difference, either. I'll keep the BuildFromLowPriority for now.

Fixes for #114

szeiger added 4 commits June 12, 2017 16:18

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.

szeiger requested review from julienrf and odersky June 16, 2017 12:08

julienrf reviewed Jun 16, 2017

View reviewed changes

Ichoran reviewed Jun 17, 2017

View reviewed changes

szeiger mentioned this pull request Jun 23, 2017

Collections meeting 06/27 #118

Closed

julienrf mentioned this pull request Jun 29, 2017

Fixes for #114 #136

Merged

julienrf added a commit that referenced this pull request Jun 29, 2017

Merge pull request #136 from scala/new-strict-building-fixes

6ca2383

Fixes for #114

julienrf closed this Jun 29, 2017

Unify strict and lazy building #114

Unify strict and lazy building #114

Uh oh!

Conversation

szeiger commented Jun 16, 2017

Uh oh!

julienrf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrf Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrf commented Jun 16, 2017

Uh oh!

szeiger commented Jun 16, 2017

Uh oh!

julienrf commented Jun 16, 2017

Uh oh!

odersky commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ichoran commented Jun 17, 2017

Uh oh!

julienrf commented Jun 20, 2017

Uh oh!

szeiger commented Jun 26, 2017

Uh oh!

szeiger commented Jun 26, 2017

Uh oh!

szeiger commented Jun 26, 2017

Uh oh!

julienrf commented Jun 26, 2017

Uh oh!

szeiger commented Jun 27, 2017

Uh oh!

Ichoran commented Jun 28, 2017

Uh oh!

szeiger commented Jun 28, 2017

Uh oh!

szeiger commented Jun 28, 2017

Uh oh!

Uh oh!

julienrf left a comment •

edited

Loading

szeiger Jun 16, 2017 •

edited

Loading

julienrf Jun 19, 2017 •

edited

Loading

szeiger Jun 19, 2017 •

edited

Loading