Fix compilation of the collections with Dotty #7696
Conversation
This seems to have been an oversight.
Because Dotty adds mixing forwarders before erasure, classes can end up with forwarders for both the non-polymorphic and polymorphic concats that will clash after erasure.
smarter
added
the
library:collections
|
Ah indeed, |
|
What was the reason for having this unsound IterableCC rather than making the relevant methods return |
This will be mixed into TreeSet and BitSet and has two purposes: - Make TreeSet and BitSet extend `collection.StrictOptimizedIterableOps` with `CC = immutable.Set` instead of `CC = collection.Set` as before. This is important since the latter is incoherent and potentially unsound. Scala 2 does not detect this (scala/bug#11042), but Dotty does and emit an error. - Make TreeSet and BitSet use more optimized versions of some methods.
smarter
force-pushed
the
fix/dotty-compilation
branch
from
1c293d5
to
376f4c2
Compare
| // 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
seems pretty gross to have these dummy implicits turn up in Scaladoc and so forth..
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
(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]?
There was a problem hiding this comment.
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/++.
There was a problem hiding this comment.
Well I did mean the latter, but yeah I see what you mean 😞
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
}
They were introduced to reduce some boilerplate for collection implementers: scala/collection-strawman#468 |
But is is worth it if collection implementers can now accidentally and very easily make their collections unsound by using the wrong type argument ? |
Can confirm this is a razor-sharp prickly thorn in the api, been bitten by it when trying to modify |
|
Actually, I share your concerns. I think we already discussed about the pros and cons but I couldn’t find this discussion… @szeiger do you remember? |
There was a problem hiding this comment.
The changes look good to me. However, I must say that I would prefer a solution that doesn’t add the DummyImplicit parameter (at the cost of having a slightly less efficient concat implementation on Set, but union would stay as efficient as it is currently so I think this is okay).
|
Thanks, merging now because I really want the dotty community build to go back to green but we can discuss this further. |
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
Just like what we did for concat in scala#7696, we add a dummy parameter list to avoid signature clashes between mixin forwarders, but this change should be less controversial since it only affects a deprecated method. Note that this isn't actually needed to compile with Dotty master yet because we only emit mixin forwarders when required to, but we will soon switch to emitting them all the time like scalac does (scala/scala3#2563), therefore we need to get the fix in first (and most importantly in time for 2.13).
This reverts commit 17647af. the upstream change was reverted, so we revert this too also use latest Scala nightly
No description provided.