Implicit factories #168
Implicit factories #168
Conversation
| def newBuilder(from: CC[A]): Builder[E, CC[E]] = from.sortedIterableFactory.newBuilder[E]() | ||
| def fromSpecificIterable(from: CC[A])(it: Iterable[E]): CC[E] = from.sortedIterableFactory.fromSpecificIterable(it) | ||
| } | ||
| } |
There was a problem hiding this comment.
These definitions are not anymore necessary: they have been superseded by the implicit definitions in factories.
| // TODO wrap with assert when HashMap’s equality is correctly implemented | ||
| Parse.parseCollection[(Int, Int), immutable.HashMap[Int, Int]](immutable.HashMap).parse("1-2|3-4").contains(immutable.HashMap((1, 2), (3, 4))) | ||
| assert(Parse.parseCollection[Int, immutable.List[Int]].parse("1|2|3").contains(1 :: 2 :: 3 :: immutable.Nil)) | ||
| assert(Parse.parseCollection[(Int, Int), immutable.HashMap[Int, Int]].parse("1-2|3-4").contains(immutable.HashMap((1, 2), (3, 4)))) |
There was a problem hiding this comment.
The factory is now provided implicitly, which was the initial goal of this PR.
| def apply[A](xs: A*): CC[A] = fromIterable(View.Elems(xs: _*)) | ||
| def fill[A](n: Int)(elem: => A): CC[A] = fromIterable(View.Fill(n)(elem)) | ||
| def newBuilder[A](): Builder[A, CC[A]] | ||
| implicit def canBuildIterable[A]: FromSpecificIterable[A, CC[A]] = IterableFactory.toSpecific(this) |
There was a problem hiding this comment.
BTW, I’d like to rename FromSpecificIterable[A, C] to CanBuild[A, C]. If users write code that takes parameters of that type, I think CanBuild conveys a clearer meaning (“ability to build a collection C from elements of type A”) than FromSpecificIterable. WDYT?
|
|
||
| val xs4 = immutable.TreeMap((1, "1"), (2, "2")) | ||
| val xs5 = flatCollect(xs4) { case (2, v) => immutable.List((v, v)) } | ||
| val xs5 = flatCollect(xs4) { case (2, v) => immutable.List((v, v)) }(immutable.TreeMap) |
There was a problem hiding this comment.
So, this is the issue. Here the TreeMap factory hasn’t been inferred because of an ambiguity with SortedSet.
Here is what happens, if I understand correctly. We are looking for an implicit BuildFrom[TreeMap[Int, String], (String, String), C]. The two candidates we find are:
SortedSet.canBuildSortedIterable, of type[A] => BuildFrom[Any, A, SortedSet[A]]TreeMap.canBuildSortedMap, of type[K, V] => BuildFrom[Any, (K, V), TreeMap[K, V]]
Why are these candidates ambiguous? Because none is more specific than the other: TreeMap[?, ?] and SortedSet[?] are not subtypes of each other.
Why are they both available in the implicit scope? The one from the companion object TreeMap is available because we are looking for an implicit BuildFrom[TreeMap[Int, String], ?, ?]. For the other one, I guess this is because somewhere in the hierarchy of SortedMap we have a SortedSet that shows up (SortedMap[K, V] extends SortedOps[K, SortedSet, SortedMap[K, V]]). Consequently, the companion object of SortedSet contributes to the implicit scope.
How to fix the problem? Unfortunately, I don’t think we can make the type of one of the two implicits more specific than the other (e.g. we can not make SortedMap[K, V] extend SortedSet[K] because they extend Iterable[(K, V)] and Iterable[K], respectively). One solution could be to change the priority of implicits in the language: when exploring companion objects, the further we go in the hierarchy (ie when we explore companions of ancestors of TreeMap), the lower the priority of their implicit definitions should be. Does that make sense (/cc @adriaanm!)? With this change, I don’t think we would break existing code. However, code that doesn’t compile now (because of an ambiguity) may compile after this change.
There was a problem hiding this comment.
I created scala/bug#10427 and scala/scala3#2911
|
I just pushed a workaround for the ambiguity: I no longer mention |
|
The PR is ready to merge. |
1 participant
Fixes #139.
The approach is to provide not just implicit
BuildFrominstances but implicitFromSpecificIterableinstances (which have anewBuilder()method that takes no initial collection).However this solution raises a ambiguous implicits error when used with sorted Map collections (e.g.
TreeMap). But I believe this is a limitation of the current way we prioritize implicits. See my comments in the diff for more details.