IterableView#filterKeys and #mapValues cannot be chained

[NthPortal]

With the current extension methods, (as of 2badce8), you cannot chain filterKeys and mapValues on an IterableView[_, _ <: Map]. I believe the reason for this is that both take a CanBuildFrom and return a That, unlike their corresponding 2.12 methods. The filterKeys extension is even particularly egregious, as filter methods always return the same type and do not ever take a CanBuildFrom.

The methods are currently defined as:

class MapViewExtensionMethods[K, V, C <: scala.collection.Map[K, V]](
    private val self: IterableView[(K, V), C])
    extends AnyVal {

  def mapValues[W, That](f: V => W)(
      implicit bf: CanBuildFrom[IterableView[(K, V), C], (K, W), That]): That =
    self.map[(K, W), That] { case (k, v) => (k, f(v)) }

  def filterKeys[That](p: K => Boolean)(
      implicit bf: CanBuildFrom[IterableView[(K, V), C], (K, V), That]): That =
    self.collect[(K, V), That] { case (k, v) if p(k) => (k, v) }
}

The following definitions do not use CanBuildFrom and can chain with each other:

class MapViewExtensionMethods[K, V, C <: scala.collection.Map[K, V]](
    private val self: IterableView[(K, V), C])
    extends AnyVal {

  def mapValues[W](f: V => W): IterableView[(K, W), C] =
    // the implementation of `self.map` also casts the result
    self.map({ case (k, v) => (k, f(v)) }).asInstanceOf[IterableView[(K, W), C]]

  def filterKeys(p: K => Boolean): IterableView[(K, V), C] =
    self.filter { case (k, _) => p(k) }
}

Unfortunately, mapValues has already been in previous releases, so its signature cannot be changed without a new release. However, we can still change the implementation of filterKeys (at least as of the aforementioned commit), as it has not been in any release yet. This allows the (I believe more common) use of .view.filterKeys(...).mapValues(...), though not .view.mapValues(...).filterKeys(...).

[julienrf]

Edit: just ignore the comment, I misread the signature…

I’m not sure the proposed signature is correct: in 2.13, mapValues and filterKeys are defined on MapView only, not on Map (they do exist on Map but are deprecated).

[julienrf]

Do you have an idea what the CanBuildFrom parameter is resolved to when you can mapValues or filterKeys? And what the type That is resolved to as well?

Would it be possible to fix both operations by providing CanBuildFrom instances?

[NthPortal]

Do you have an idea what the CanBuildFrom parameter is resolved to when you can mapValues or filterKeys? And what the type That is resolved to as well?

According to IntelliJ, the CanBuildFrom resolves to s.c.IterableView.canBuildFrom, and the type of That is s.c.IterableView[(K, V), s.c.Iterable[_]].

For reference, the type of s.c.IterableView.canBuildFrom[A] is CanBuildFrom[Coll, A, IterableView[A, s.c.Iterable[_]]], where Coll is TraversableView[_, C] forSome {type C <: Traversable[_]}.

Would it be possible to fix both operations by providing CanBuildFrom instances?

I'm not super great at finding the right CanBuildFrom, but I wasn't able to summon one using a breakOut such as collection.breakOut[IterableView[(String, Int), Map[String, Int]], (String, Int), IterableView[(String, Int), Map[String, Int]]]

[julienrf]

By adding the following CanBuildFrom instance to the compat object I was able to get both of your tests (in #295) compile.

// CanBuildFrom instances for `IterableView[(K, V), Map[K, V]]` that preserve
// the strict type of the view to be `Map` instead of `Iterable`
implicit def canBuildFromIterableViewMapLike[K, V, L, W, CC[X, Y] <: c.Map[X, Y]]: CanBuildFrom[c.IterableView[(K, V), CC[K, V]], (L, W), c.IterableView[(L, W), CC[L, W]]] =
  new CanBuildFrom[c.IterableView[(K, V), CC[K, V]], (L, W), c.IterableView[(L, W), CC[L, W]]] {
    // `CanBuildFrom` parameters are used as type constraints, they are not used
    // at run-time, hence the dummy builder implementations
    def apply(from: c.IterableView[(K, V), CC[K, V]]) = new c.TraversableView.NoBuilder
    def apply() = new c.TraversableView.NoBuilder
  }

I’m not saying that this is a simpler approach, though… What do you think?

[NthPortal]

@julienrf sorry I haven't gotten around to replying until now.

I think this approach is a good stopgap to allow compatibility until we can change the underlying implementation of mapValues, but fundamentally, I don't think either filterKeys or mapValues should have a CanBuildFrom. They don't in 2.12, so they shouldn't here. Also, a filtering method should never need one, as it ought to return the same collection type.

IMO, we should change filterKeys now (since we can), add that implicit CanBuildFrom so that mapValues chains (although you should probably be calling filterKeys first anyway), and leave a note to change the implementation of mapValues in an eventual 3.0.0 release. Does that sound reasonable to you?

[julienrf]

IMO, we should change filterKeys now (since we can), add that implicit CanBuildFrom so that mapValues chains (although you should probably be calling filterKeys first anyway), and leave a note to change the implementation of mapValues in an eventual 3.0.0 release. Does that sound reasonable to you?

Yes, let’s do that!

[NthPortal]

I'll update #295