From f2eee9cab074db68289ee1db42c28b8fb8a9818c Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Fri, 4 Nov 2022 15:45:46 +1000 Subject: [PATCH] Reduce implicit search space for BuildFrom avoiding divergence BuildFrom (nee CanBuildFrom) is de-emphasised in 2.13 but remains for use by generic extension methods that abstract over different collection types. As part of the 2.13 redesign, implicit instances for `BuildFrom[SomeCollection]` where moved from the `SomeCollection` companions into the `BuildFrom` companion. This has an unfortunate side effect -- more implicits are in scope for implicit searches, which can incur longer search time and can also lead into divergent searches. Implicit search prunes candidates based on the "depoly"-ed type, which type params and value params with wildcards types in the implicit signature. The nested implicit search for implicit params occurs, which can guide inference of the type params. Only then is the true type known and compared against the expected type of the search. It seems that this "depoly" transform could be smarter by approximating type params to bounds. Perhaps this is harder than it sounds when you have things like F-bounded or mutually recursive type params. Instead of trying to fix that general problem, I've instead restated the bounds in the result type of relevant implicits in BuildFrom. This would be redundant, were it not for way that they are preseved through `depoly`. The new test case now compiles without divergence and the search considers only one candidate in detail. Previously, compile failed with a divergent error in `Ordering` implicits after about 80 steps. --- src/library/scala/collection/BuildFrom.scala | 9 +++-- .../scala/collection/BuildFromTest.scala | 37 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/library/scala/collection/BuildFrom.scala b/src/library/scala/collection/BuildFrom.scala index e0e92a7e66..bc9c49d949 100644 --- a/src/library/scala/collection/BuildFrom.scala +++ b/src/library/scala/collection/BuildFrom.scala @@ -45,14 +45,14 @@ trait BuildFrom[-From, -A, +C] extends Any { self => object BuildFrom extends BuildFromLowPriority1 { /** Build the source collection type from a MapOps */ - implicit def buildFromMapOps[CC[X, Y] <: Map[X, Y] with MapOps[X, Y, CC, _], K0, V0, K, V]: BuildFrom[CC[K0, V0], (K, V), CC[K, V]] = new BuildFrom[CC[K0, V0], (K, V), CC[K, V]] { + implicit def buildFromMapOps[CC[X, Y] <: Map[X, Y] with MapOps[X, Y, CC, _], K0, V0, K, V]: BuildFrom[CC[K0, V0] with Map[K0, V0], (K, V), CC[K, V] with Map[K, V]] = new BuildFrom[CC[K0, V0], (K, V), CC[K, V]] { //TODO: Reuse a prototype instance def newBuilder(from: CC[K0, V0]): Builder[(K, V), CC[K, V]] = (from: MapOps[K0, V0, CC, _]).mapFactory.newBuilder[K, V] def fromSpecific(from: CC[K0, V0])(it: IterableOnce[(K, V)]): CC[K, V] = (from: MapOps[K0, V0, CC, _]).mapFactory.from(it) } /** Build the source collection type from a SortedMapOps */ - implicit def buildFromSortedMapOps[CC[X, Y] <: SortedMap[X, Y] with SortedMapOps[X, Y, CC, _], K0, V0, K : Ordering, V]: BuildFrom[CC[K0, V0], (K, V), CC[K, V]] = new BuildFrom[CC[K0, V0], (K, V), CC[K, V]] { + implicit def buildFromSortedMapOps[CC[X, Y] <: SortedMap[X, Y] with SortedMapOps[X, Y, CC, _], K0, V0, K : Ordering, V]: BuildFrom[CC[K0, V0] with SortedMap[K0, V0], (K, V), CC[K, V] with SortedMap[K, V]] = new BuildFrom[CC[K0, V0], (K, V), CC[K, V]] { def newBuilder(from: CC[K0, V0]): Builder[(K, V), CC[K, V]] = (from: SortedMapOps[K0, V0, CC, _]).sortedMapFactory.newBuilder[K, V] def fromSpecific(from: CC[K0, V0])(it: IterableOnce[(K, V)]): CC[K, V] = (from: SortedMapOps[K0, V0, CC, _]).sortedMapFactory.from(it) } @@ -92,7 +92,10 @@ object BuildFrom extends BuildFromLowPriority1 { trait BuildFromLowPriority1 extends BuildFromLowPriority2 { /** Build the source collection type from an Iterable with SortedOps */ - implicit def buildFromSortedSetOps[CC[X] <: SortedSet[X] with SortedSetOps[X, CC, _], A0, A : Ordering]: BuildFrom[CC[A0], A, CC[A]] = new BuildFrom[CC[A0], A, CC[A]] { + // Restating the upper bound of CC in the result type seems redundant, but it serves to prune the + // implicit search space for faster compilation and reduced change of divergence. See the compilation + // test in test/junit/scala/collection/BuildFromTest.scala and discussion in https://github.com/scala/scala/pull/10209 + implicit def buildFromSortedSetOps[CC[X] <: SortedSet[X] with SortedSetOps[X, CC, _], A0, A : Ordering]: BuildFrom[CC[A0] with SortedSet[A0], A, CC[A] with SortedSet[A]] = new BuildFrom[CC[A0], A, CC[A]] { def newBuilder(from: CC[A0]): Builder[A, CC[A]] = (from: SortedSetOps[A0, CC, _]).sortedIterableFactory.newBuilder[A] def fromSpecific(from: CC[A0])(it: IterableOnce[A]): CC[A] = (from: SortedSetOps[A0, CC, _]).sortedIterableFactory.from(it) } diff --git a/test/junit/scala/collection/BuildFromTest.scala b/test/junit/scala/collection/BuildFromTest.scala index fedbe1b904..12fe6d9595 100644 --- a/test/junit/scala/collection/BuildFromTest.scala +++ b/test/junit/scala/collection/BuildFromTest.scala @@ -176,4 +176,41 @@ class BuildFromTest { immutable.LongMap: BuildFrom[_, (Long, String), immutable.LongMap[String]] mutable.LongMap: BuildFrom[_, (Long, String), mutable.LongMap[String]] mutable.AnyRefMap: BuildFrom[_, (String, String), mutable.AnyRefMap[String, String]] + + // Check that we don't get an implicit divergence in a futile part of the search tree: + { + sealed trait GPoint + sealed trait HNil extends GPoint + class HCons[H, +T <: GPoint] extends GPoint + abstract class ExtendsOrdered extends Ordered[ExtendsOrdered] + + + // In scala 2.13, this implicit search considers BuildFrom.buildFromSortedSetOps + // which looks for a dep. implicit of type Ordering[(Int, HCons[ExtendsOrdered, HNil])] + implicitly[collection.BuildFrom[Seq[Any], (Int, HCons[ExtendsOrdered, HNil]), Seq[(Int, HCons[ExtendsOrdered, HNil])]]] + + // + // In Scala 2.12, buildFromSortedSetOps is not a candidate because if it is in the companion object of + // the SortedSet heirarchy, which is not part of the implicit scope for this search. + // In 2.13, the implicit was moved to `object BuildFrom`, so _is_ considered + // + // The dependent implicit search: + // + // implicitly[(Int, HCons[ExtendsOrdered, HNil])] + // + // ... diverges on both Scala 2.12 and 2.13 + // + // error: diverging implicit expansion for type scala.math.Ordering.AsComparable[(Int, HCons[ExtendsOrdered,HNil])] + // starting with method orderingToOrdered in object Ordered + // + // Divergences in Ordering implicits are a long standing problem, but I always thought it too hard to + // fix while retaining source compatibility. + // + // Removing `extends Ordered[X]` avoids divergence, but I'm not sure why. I diffed the -Vtyper log but + // can't figure out why that is relevant. + // + // (In the original code, ExtendsOrdered was actually scala.Enumeration.Value, which does extends Ordered. + // + // + } }