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.

Sign up for GitHub

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

Jump to bottom

Fix compilation of the collections with Dotty #7696

Merged
merged 5 commits into from Jan 30, 2019
Merged

Fix compilation of the collections with Dotty #7696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8aa67d7
Avoid Dotty warning "final modifier is redundant for objects"
smarter Jan 29, 2019
7a6c836
ProcessImpl: add missing () when calling take()
smarter Jan 29, 2019
fd8a658
Make Set#++ take an IterableOnce like Set#concat
smarter Jan 29, 2019
4366332
Set#concat: avoid clashing overloads by adding a DummyImplicit
smarter Jan 29, 2019
376f4c2
Add an immutable.StrictOptimizedSortedSetOps
smarter Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/library/scala/collection/BitSet.scala
View file
Open in desktop
Expand Up @@ -233,7 +233,7 @@ trait BitSetOps[+C <: BitSet with BitSetOps[C]]
coll.fromBitMaskNoCopy(a)
}

override def concat(other: collection.IterableOnce[Int]): C = other match {
override def concat(other: collection.IterableOnce[Int])(implicit dummy: DummyImplicit): C = other match {
case otherBitset: BitSet =>
val len = coll.nwords max otherBitset.nwords
val words = new Array[Long](len)
Expand Down
8 changes: 6 additions & 2 deletions src/library/scala/collection/Set.scala
View file
Open in desktop
Expand Up @@ -176,6 +176,10 @@ trait SetOps[A, +CC[_], +C <: SetOps[A, CC, C]]
@deprecated("Use &- with an explicit collection argument instead of - with varargs", "2.13.0")
def - (elem1: A, elem2: A, elems: A*): C = diff(elems.toSet + elem1 + elem2)

// The implicit dummy parameter is necessary to avoid erased signature clashes
// between this `concat` and the polymorphic one defined in `IterableOps`.
// Note that these clashes only happen in Dotty because it adds mixin
// forwarders before erasure unlike Scala 2.
Copy link
Member

Choose a reason for hiding this comment

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

Would this implicit param be removed at some point in the future, i.e. in 3.X, or is it permanent? If permanent, then maybe it's better to just name this method something else?

Copy link
Member

Choose a reason for hiding this comment

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

seems pretty gross to have these dummy implicits turn up in Scaladoc and so forth..

Copy link
Member Author

Choose a reason for hiding this comment

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

Would this implicit param be removed at some point in the future, i.e. in 3.X, or is it permanent?

Permanent.

If permanent, then maybe it's better to just name this method something else?

There are alternative names for concat/++ in Set already: union/|. But then that means we need to tell everyone to not use concat/++ if they don't want to get worse performance.

seems pretty gross to have these dummy implicits turn up in Scaladoc and so forth...

Agreed. Note that this isn't without precedent: AnyRefMap does the same thing for map, flatMap and collect. One cosmetic improvement would be to hide implicit parameter lists consisting only of DummyImplicit in scaladoc and the IDE hover-on-type.

I also considered various alternatives to this and discussed them with Julien and others but I wasn't able to come up with anything better: I can't think of a way to twist the various definitions of concat to not cause these signature clashes without either breaking soundness or worsening performance. And I don't want to change Dotty to add mixin forwarders after erasure like Scala 2 does, because this creates other problems (see e.g. scala/scala-dev#178).

Copy link
Member Author

@smarter smarter Jan 29, 2019 •
edited

Choose a reason for hiding this comment

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

To add some context: the root cause of the issue here is that we both have a def concat[B >: A](that: IterableOnce[B]): CC[B] in Iterable and a def concat(that: IterableOnce[A]): C in Set. The latter can't be in IterableOnce because of variance. It has two advantages: since it knows that it takes a collection with elements of the same type, it can be implemented more efficiently, it also returns a more precise result type (which only matters when CC[A] != C, which is only the case for BitSet and ValueSet I think).

Copy link
Member

Choose a reason for hiding this comment

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

(Accidentally clicked "Resolve")

If permanent, then maybe it's better to just name this method something else?

There are alternative names for concat/++ in Set already: union/|. But then that means we need to tell everyone to not use concat/++ if they don't want to get worse performance.

What do we think about widening the argument of union to collection.IterableOnce[A] then, and pattern matching for case of Set[A]?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, on its own ? Or combined with removing the monomorphic concat ? The former is fine, the latter doesn't solve the problem of making performance worse for everyone using concat/++.

Copy link
Member

Choose a reason for hiding this comment

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

Well I did mean the latter, but yeah I see what you mean 😞

Copy link
Member Author

@smarter smarter Jan 30, 2019 •
edited

Choose a reason for hiding this comment

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

We could also imagine a language feature to "unclash" your method, e.g. writing @unclash1 def foo(...) would make the compiler generate code equivalent to manually adding one DummyImplicit. Library authors might appreciate something like that.

Copy link
Member

@joshlemer joshlemer Jan 30, 2019 •
edited

Choose a reason for hiding this comment

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

If we're throwing out ideas, maybe something like implicit argument of type B <:< A but which always is filled in by compiler and at runtime can be inspected for whether or not it is true. Like

class Foo[A](a: A) {
  def bar[B](b: B)(implicit ev: B <:<? A): A = {
    if (ev.isTrue) ev(b) else a
  }
}

/** Creates a new $coll by adding all elements contained in another collection to this $coll, omitting duplicates.
*
* This method takes a collection of elements and adds all elements, omitting duplicates, into $coll.
Expand All @@ -189,7 +193,7 @@ trait SetOps[A, +CC[_], +C <: SetOps[A, CC, C]]
* @param that the collection containing the elements to add.
* @return a new $coll with the given elements added, omitting duplicates.
*/
def concat(that: collection.IterableOnce[A]): C = fromSpecific(that match {
def concat(that: collection.IterableOnce[A])(implicit dummy: DummyImplicit): C = fromSpecific(that match {
case that: collection.Iterable[A] => new View.Concat(toIterable, that)
case _ => iterator.concat(that.iterator)
})
Expand All @@ -201,7 +205,7 @@ trait SetOps[A, +CC[_], +C <: SetOps[A, CC, C]]
def + (elem1: A, elem2: A, elems: A*): C = fromSpecific(new View.Concat(new View.Appended(new View.Appended(toIterable, elem1), elem2), elems))

/** Alias for `concat` */
@`inline` final def ++ (that: collection.Iterable[A]): C = concat(that)
@`inline` final def ++ (that: collection.IterableOnce[A])(implicit dummy: DummyImplicit): C = concat(that)

/** Computes the union between of set and another set.
*
Expand Down
2 changes: 1 addition & 1 deletion src/library/scala/collection/StrictOptimizedSetOps.scala
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ trait StrictOptimizedSetOps[A, +CC[_], +C <: SetOps[A, CC, C]]
extends SetOps[A, CC, C]
with StrictOptimizedIterableOps[A, CC, C] {

override def concat(that: IterableOnce[A]): C =
override def concat(that: IterableOnce[A])(implicit dummy: DummyImplicit): C =
strictOptimizedConcat(that, newSpecificBuilder)

}
2 changes: 1 addition & 1 deletion src/library/scala/collection/immutable/HashSet.scala
View file
Open in desktop
Expand Up @@ -73,7 +73,7 @@ final class HashSet[A] private[immutable] (val rootNode: SetNode[A])
if (rootNode ne newRootNode) new HashSet(newRootNode) else this
}

override def concat(that: IterableOnce[A]): HashSet[A] = {
override def concat(that: IterableOnce[A])(implicit dummy: DummyImplicit): HashSet[A] = {
val builder = iterableFactory.newBuilder[A]
builder ++= this
builder ++= that
Expand Down
2 changes: 1 addition & 1 deletion src/library/scala/collection/immutable/Set.scala
View file
Open in desktop
Expand Up @@ -79,7 +79,7 @@ trait StrictOptimizedSetOps[A, +CC[X], +C <: SetOps[A, CC, C]]
with collection.StrictOptimizedSetOps[A, CC, C]
with StrictOptimizedIterableOps[A, CC, C] {

override def concat(that: collection.IterableOnce[A]): C = {
override def concat(that: collection.IterableOnce[A])(implicit dummy: DummyImplicit): C = {
var result: C = coll
val it = that.iterator
while (it.hasNext) result = result + it.next()
Expand Down
5 changes: 5 additions & 0 deletions src/library/scala/collection/immutable/SortedSet.scala
View file
Open in desktop
Expand Up @@ -38,6 +38,11 @@ trait SortedSetOps[A, +CC[X] <: SortedSet[X], +C <: SortedSetOps[A, CC, C]]
def unsorted: Set[A]
}

trait StrictOptimizedSortedSetOps[A, +CC[X] <: SortedSet[X], +C <: SortedSetOps[A, CC, C]]
extends SortedSetOps[A, CC, C]
with collection.StrictOptimizedSortedSetOps[A, CC, C]
with StrictOptimizedSetOps[A, Set, C]

/**
* $factoryInfo
* @define coll immutable sorted set
Expand Down
12 changes: 6 additions & 6 deletions src/library/scala/collection/immutable/TreeSeqMap.scala
View file
Open in desktop
Expand Up @@ -284,9 +284,9 @@ final class TreeSeqMap[K, +V] private (
}
object TreeSeqMap extends MapFactory[TreeSeqMap] {
sealed trait OrderBy
final object OrderBy {
final case object Insertion extends OrderBy
final case object Modification extends OrderBy
object OrderBy {
case object Insertion extends OrderBy
case object Modification extends OrderBy
}

val Empty = new TreeSeqMap[Nothing, Nothing](Ordering.empty, HashMap.empty, 0, OrderBy.Insertion)
Expand Down Expand Up @@ -351,7 +351,7 @@ object TreeSeqMap extends MapFactory[TreeSeqMap] {
private val Mapping = Map

/* The ordering implementation below is an adapted version of immutable.IntMap. */
private[immutable] final object Ordering {
private[immutable] object Ordering {
import scala.collection.generic.BitOperations.Int._

@inline private[immutable] def toBinaryString(i: Int): String = s"$i/${i.toBinaryString}"
Expand Down Expand Up @@ -404,7 +404,7 @@ object TreeSeqMap extends MapFactory[TreeSeqMap] {
def empty[V]: Iterator[V] = Empty.asInstanceOf[Iterator[V]]
}

final case object Zero extends Ordering[Nothing] {
case object Zero extends Ordering[Nothing] {
// Important! Without this equals method in place, an infinite
// loop from Map.equals => size => pattern-match-on-Nil => equals
// develops. Case objects and custom equality don't mix without
Expand Down Expand Up @@ -635,4 +635,4 @@ object TreeSeqMap extends MapFactory[TreeSeqMap] {
else bin(prefix, mask, l, r)
}
}
}
}
2 changes: 1 addition & 1 deletion src/library/scala/collection/immutable/TreeSet.scala
View file
Open in desktop
Expand Up @@ -139,7 +139,7 @@ final class TreeSet[A] private[immutable] (private[immutable] val tree: RB.Tree[
def excl(elem: A): TreeSet[A] =
newSetOrSelf(RB.delete(tree, elem))

override def concat(that: collection.IterableOnce[A]): TreeSet[A] = {
override def concat(that: collection.IterableOnce[A])(implicit dummy: DummyImplicit): TreeSet[A] = {
val t = that match {
case ts: TreeSet[A] if ordering == ts.ordering =>
RB.union(tree, ts.tree)
Expand Down
4 changes: 2 additions & 2 deletions src/library/scala/sys/process/ProcessImpl.scala
View file
Open in desktop
Expand Up @@ -193,7 +193,7 @@ private[process] trait ProcessImpl {
protected[this] val source = new SyncVar[Option[InputStream]]
override final def run(): Unit = {
@tailrec def go(): Unit =
source.take match {
source.take() match {
case Some(in) => runloop(in, pipe) ; go()
case None =>
}
Expand All @@ -216,7 +216,7 @@ private[process] trait ProcessImpl {
protected[this] val sink = new SyncVar[Option[OutputStream]]
override def run(): Unit = {
@tailrec def go(): Unit =
sink.take match {
sink.take() match {
case Some(out) => runloop(pipe, out) ; go()
case None =>
}
Expand Down