Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Merge ops, mappings, and like traits #76

Merged
merged 15 commits into from
May 8, 2017
Merged

Merge ops, mappings, and like traits #76

merged 15 commits into from
May 8, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 5, 2017

Merge ops, mappings, and like traits into an Ops trait with 3 parameters.

Also, drop ImmutableBuilders, we don't seem to need them.

odersky and others added 10 commits April 22, 2017 15:09
It breaks Scala implicit inference, maybe because of the covariance/contravariance issue.
Not sure what it is, but in any case we only have a single instance, so abstraction is
premature (my rule of thumb is you need 3 plausible instances to make it worthwhile to
abstract).

So in this commit we replace constrained by ordered throughout.
After the specialization to Ordered, everything compiles now under dotty.
Two FIXMEs in Test.scala remain, where the correct type
is not yet inferred under dotty.
Always define in parallel with

    fromIterable
    mapFromIterable
    orderedFromIterable
They now are all protected[this]
... into an Ops trait with 3 parameters.

Also, drop ImmutableBuilders, we don't seem to need them.
@DarkDimius
Copy link
Member

Glad this idea worked!

@julienrf
Copy link
Contributor

julienrf commented May 5, 2017

There is an issue because ArrayOps and StringOps can not implement the self-type they pretend to implement.

@julienrf
Copy link
Contributor

julienrf commented May 5, 2017

To give you more information, the heart of the issue is here:

[error] /home/travis/build/scala/collection-strawman/src/main/scala/strawman/collection/View.scala:136: illegal inheritance;
[error]  trait IndexedView inherits different type instances of trait IterableOps:
[error] strawman.collection.IterableOps[A,strawman.collection.Seq,strawman.collection.Seq[A]] and strawman.collection.IterableOps[A,strawman.collection.View,strawman.collection.View[A]]
[error] trait IndexedView[+A] extends View[A] with Seq[A] { self =>
[error]       ^

The problem is that Seq[A] inherits IterableOps[A, Seq, Seq[A]], whereas View[A] inherits IterableOps[A, View, View[A]] but Seq and View are unrelated types. The fix should be to make IndexedView extend SeqLike[A, IndexedView, IndexedView[A]] but then we would have to re-implement fromIterable and fromSpecificIterable because we wouldn’t anymore be able to reuse implementations inherited from View (because they return a View instead of an IndexedView. But then even if we re-implement them that means implementing the following:

def fromIterable[B](c: Iterable[B]): IndexedView[B]

Which is not possible when the given Iterable[B] does not itself extend Seq[B].

@odersky
Copy link
Contributor Author

odersky commented May 5, 2017

@julienrf Good analysis, thanks! I fixed it by not having IndexedView inherit from Seq.

@julienrf
Copy link
Contributor

julienrf commented May 8, 2017

I added one commit that essentially fixes some C and CC types in the hierarchy, moves the fromIterable method implementation to the leaves, adds explicit type annotations to the xs.map { case (k, v) => … } tests and comments out the traverse tests.

@julienrf julienrf merged commit 63e6c17 into master May 8, 2017
@julienrf julienrf deleted the merge-ops-mappings branch May 8, 2017 12:15
@szeiger
Copy link
Member

szeiger commented May 11, 2017

So what about TraverseTest? This is hardly a problem we can ignore. We need these methods in the standard library and I'm pretty sure they will also be common in third-party code.

@julienrf
Copy link
Contributor

@szeiger I agree with you that we should support these operations. And also, I think we should even support the use case described in #65. On the other hand, the previous solution was maybe too complex. We should try to make it simpler.

@szeiger
Copy link
Member

szeiger commented May 12, 2017

The essence of this simplification here seems to be the removal of all the work I did on BuildFrom (which went through several iterations to come up with a simple design). This includes single-method extensions (like Future.traverse, a breakOut-like pattern, and first-class support for strict building. We're taking a giant step back by removing it instead of trying to simplify it further.

@julienrf
Copy link
Contributor

I think we can re-introduce a simplified BuildFrom thing on top of which things like Traverse, generic programming things (like in #65) and unfolds (like in #34) could be implemented externally.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants