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

Collection strawman2 #819

Closed
wants to merge 6 commits into from
Closed

Conversation

DarkDimius
Copy link
Member

To support discussion in #818.

It's not going to pass tests before #805 is merged.

It has 4 changes that are applied to Martin’s version,
and I believe they could be discussed independently:

dotty-staging@48afa75
Gets rid of view class. View is a simple type alias to () => Iterator[A].
This points the major difference between Iterators and views: iterators are consumed by
any usage, while views are reusable.

dotty-staging@5281688
Makes .view method be a decorator. This is done in order to decouple views from collection higherarhy
and make implementation of custom collections easier. It would also allow to modularise collections library to a higher degree. I believe that the very same should be done to .par.

dotty-staging@b77ab7c
Makes Iterators have only next and hasNext methods. The motivation could be found in this code sample:

def intersectionIsEmpty[A](it1: Iterator[A], it2: Iterator[A]) =
       !(it1 exists (it2 contains _))

Which was taken from old bug in Dotty codebase dotty-staging@03ec379

I believe that this code simply should not compile: both contains and exists have no side effects on
mutable and immutable collections, but it has a silent side-effect on Iterators. It does not follow principle
of least surprise, I would actually say that this is one of most surprising methods for new-comments that
felt the joy of ”you can use the same methods on parallel, mutable and immutable collections”
when they discover that there is ”but be very careful if you use those methods on iterators”.

Ok, so what the change does: it moves all consuming methods from Iterator[A], to a decorator over View[A], that is, to () => Iterator[A]. This follows the patch that makes difference in use-cases of iterator[A] and View[A] more obvious, that I started in previous commit.

dotty-staging@5244c5c
Extracts a LengthType from all the signatures for documentation purposes & makes it be Long for big-data needs.
This change has a problem with compatibility with ScalaJs, as they simulate Longs.

@DarkDimius DarkDimius changed the title Callection strawman2 Collection strawman2 Oct 5, 2015
I've seen many projects, including Dotty itself where there is code similar to

```scala

def average(xs: Iterator[Int]) = xs.sum/xs.size
```

It simply should not compile.
@DarkDimius
Copy link
Member Author

Major change in 48afa75: there is no class view. It's a type alias, and from this type alias there are decorators that add convenient operations for views.
Note that all existing ViewOps, ViewMonoTransforms and ViewPolyTransforms continue to work. This is a source-compatible change :-D

@DarkDimius
Copy link
Member Author

As of 5281688 views are not part of collection operations. I believe that view method is fundamentally different from foldRight\head\isEmpty and should belong to a different implicit conversion.

@DarkDimius
Copy link
Member Author

As of b77ab7c, iterator class does not have operations.
Instead, operations are on views.

The motivating example is

def average(xs: Iterator[Int]) = xs.sum/xs.size

I believe it simply should not compile. User should instead write

def average(xs: View[Int]) = xs.sum/xs.size

Note that as of 48afa75 this View[T] is an alias to () => Iterable[T], making this code actually do what it is expected to.

@DarkDimius
Copy link
Member Author

As an argument that Iterator should not have operations that consume it I'll remind about a bug in dotty that was in code for long time and was tricky to find:
dotty-staging@03ec379

@DarkDimius
Copy link
Member Author

As of 5244c5c, there is a type alias type LengthType = Long which is used for all indexes. This means that size is Long.

@odersky
Copy link
Contributor

odersky commented Oct 7, 2015

/rebuild

@DarkDimius
Copy link
Member Author

As of 22ca08d LengthType is a value class, that defines common arithmetic operations, and has implicit convertions to Long and from Int.
The underlying type could be different on JVM and JavaScript platform.

Given provided methods and implicit conversions, those code samples work:

val s: List = ...
s(1) // 1 is converted to LengthType through implicit convertion
s.drop(s.length - 1) // arithmetic operations on LengthType
assert(s.length < 100) // comparisons

@odersky
Copy link
Contributor

odersky commented Jul 27, 2016

I don't think we need to keep this in the PR queue any longer.

@odersky odersky closed this Jul 27, 2016
OlivierBlanvillain pushed a commit to OlivierBlanvillain/dotty that referenced this pull request Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants