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

Port “The Architecture of Scala Collections” to the new architecture #960

Closed
wants to merge 2 commits into from

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Dec 22, 2017

Do not merge unless 2.13 is released!

I’ve started adapting the documentation page The architecture of Scala collections to the new design.

I’m not sure we should continue with the same examples. They were very interesting to show how to use CanBuildFrom to drive the return type of transformation operations, but this is not a feature which is supported by the new design (so… there is nothing really fancy to show here). On the other hand I don’t think the examples here demonstrate all the features of the new framework (like how to get the return type of map & cie work as expected without having to add overloads, which is only possible for polymorphic collection types).

def map(f: Base => Base): RNA = {
var b = newSpecificBuilder()
for (base <- this) {
b += f(base)
}
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 was also wondering if we should provide helpers to implement such operations in a strict way. For the view based version it is as easy as writing:

def map(f: Base => Base): RNA = fromSpecificIterable(View.Map(this, f))

We should probably have an equivalent strict version:

object Builder {

  def map[CC[_], A, B](source: IterableOnce[A], f: A => B, builder: Builder[B, CC[B]]): CC[B] = {
    for (a <- source) {
      b += f(base)
    }
    builder.result()
  }
}

So that users could just write:

def map(f: Base => Base): RNA = Builder.map(this, f, newSpecificBuilder())

And we could use it also in StrictOptimizedIterableOps.

The only part that I don’t like is that the builder is created in one place (user code) and used in a different place (framework code). This can be error prone because we really want the builder to be freshly created.

@julienrf julienrf changed the title WIP — Port “The Architecture of Scala Collections” to the new architecture Port “The Architecture of Scala Collections” to the new architecture Jan 4, 2018
@julienrf
Copy link
Contributor Author

julienrf commented Jan 4, 2018

The PR is ready to review!

In addition to explaining the new architecture, I’ve made the following changes to the examples:

  • I start with a new example of a collection with a capped size, which illustrates the basic case (how to get right the return type of transformation operations such as filter and map, which traits to inherit from in the case of a strict collection),
  • I’ve kept RNA and PrefixMap, which show how to use overloading to apply the “same-result-type” principle.

@julienrf
Copy link
Contributor Author

I’d like to get this PR reviewed so that we have time to improve things if needed. @SethTisue, would you have time to do that soon?

@SethTisue
Copy link
Member

SethTisue commented Jan 18, 2018

I think the document would be more helpful and easier to follow if you led with a summary of all of the challenges that the design seeks to solve, before proceeding to discuss each challenge in depth.

The challenges include:

  • Some collections have a single element type (e.g. List[A]), others have two (e.g. Map[K, V]).
  • Some transformations return the same concrete collection type, e.g. filter never changes the type.
  • Some transformations return the same type constructor with a different element type (e.g. map from List[A] to List[B]).
  • Operations on some collections require additional implicit parameters, e.g. map on a SortedSet takes an implicit Ordering
  • Some collections are strict (e.g. List), others are non-strict (e.g. LazyList)

I made that list by looking through the opening pages of the document, but as I read it over, it strikes me as curiously incomplete. Missing items that come to mind:

  • Some "collections" are actually existing types the JVM provides, e.g. Array and String, that must be transparently integrated somehow.
  • Some collections are mutable, others are immutable.
  • Some operations on collections return a different collection type constructors depending on the element type. For example String.map[A, B] returns a String if B is Char but must otherwise return some other kind of collection.
  • Views. One of the main goals of the redesign is to correctly support views.
  • Builders. The builder design is different than the old collections, right? What challenge prompted the change?

Having given a list of challenges, I think it would also be helpful to the reader to briefly say what the solution to each one is, in a sentence or two, before going into the detailed, multi-paragraph discussions of each one.

(Meta-question: is this the kind of feedback you were looking for...? I assume it's too soon for a detailed proofread/copyedit.)



kind | not sorted | sorted
===========|===============|===============
Copy link
Member

Choose a reason for hiding this comment

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

this table syntax does not render on GitHub

@julienrf
Copy link
Contributor Author

Thanks @SethTisue for the feedback! (yes that’s exactly what I was looking for)

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Very nice work, thanks! Great suggestions by Seth as well, fully agree.

I wonder if it's a good approach to hold this updated document back until 2.13 is released and then replace the old version. I think it would be valuable to have the 2.13 version available before the release (during milestones / RCs), and probably also valuable to keep the 2.12 one around for a while.

There should be instructions how to cross-build custom collections against 2.12 and 2.13 - could be in this or a separate document.

Does the "Collections" guide need a rework as well (http://docs.scala-lang.org/overviews/collections/introduction.html)? Is someone planning to work on this?

### Abstracting over collection types ###

The template trait `IterableOps` implements the operations available
on the `Iterable[A]` collection type.
Copy link
Member

Choose a reason for hiding this comment

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

explain why operatinos are not implemented in Iterable directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that wasn’t very clear but that’s what I’ve tried to explain before that section: to implement the operations you need to abstract over their return type, hence the need for the template traits.

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 wonder if it's worth saying that having the ops trait (with 3 type params) allows having a clean Iterable[A] type with a single type param

- `toIterable` turns the `IterableOps` instance into an `Iterable`. This operation is
a no-op when it is effectively called on an `Iterable` instance (it is possible to
create an `IterableOps` instance that does not inherit from `Iterable` but that’s out
of the scope of this document) ;
Copy link
Member

Choose a reason for hiding this comment

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

this makes readers curious without giving a link or hint where to look further :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We could add a link to StringOps for instance.

~~~ scala
trait IterableOps[+A, +CC[_], +C] {
def iterableFactory: IterableFactory[CC]
protected def fromSpecificIterable(coll: Iterable[A]): C
Copy link
Member

Choose a reason for hiding this comment

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

show fromIterable as well?

By symmetry with `fromSpecificIterable` and `fromIterable`, template traits provide
ways to get a builder resulting in a collection with the same type of elements, and
to get a builder resulting in a collection of the same type but with a different
type of elements. The following code shows the relevant parts of `IterableOps` and
Copy link
Member

Choose a reason for hiding this comment

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

give an example (groupMap)?

`Seq`, `Set`, `Map` or just `Iterable`? In our case, it is tempting
to choose `Seq` because our collection can contain duplicates and
iteration order is determined by insertion order. However, some
properties of `Seq` are not satisfied:
Copy link
Member

Choose a reason for hiding this comment

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

@julienrf
Copy link
Contributor Author

julienrf commented Jan 23, 2018

I wonder if it's a good approach to hold this updated document back until 2.13 is released and then replace the old version. I think it would be valuable to have the 2.13 version available before the release (during milestones / RCs), and probably also valuable to keep the 2.12 one around for a while.

You’re right, we shouldn’t replace that document yet but have both documents coexist.

There should be instructions how to cross-build custom collections against 2.12 and 2.13 - could be in this or a separate document.

I’d rather put that information in a separate document. I find this one already quite long.

Does the "Collections" guide need a rework as well (http://docs.scala-lang.org/overviews/collections/introduction.html)? Is someone planning to work on this?

Yes! We have that meta-ticket to keep track on what needs to be done: https://github.com/scala/collection-strawman/issues/311. And the part that you are referring to corresponds to #995. Nobody is working on it.

@SethTisue SethTisue added the wip label Feb 15, 2018
@julienrf
Copy link
Contributor Author

Superseded by #1078

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

Successfully merging this pull request may close these issues.

3 participants