-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
@@ -33,7 +33,7 @@ trait IterableLike[+A, +C[X] <: Iterable[X]] | |||
/** Create a collection of type `C[A]` from the elements of `coll`, which has | |||
* the same element type as this collection. Overridden in StringOps and ArrayOps. | |||
*/ | |||
protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): C[A] = fromIterable(coll) | |||
protected[this] def fromIterableWithSameElemType(coll: Iterable[A]): C[A] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this default implementation because in the case of Map[K, V]
, fromIterable
does not return a Map[K, V]
but an Iterable[(K, V)]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it? I see no particular reason why we'd want the binary type constructors in fromIterableWithSameElemType
. This compiles: szeiger@0de8480
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we want filter
to still return a Map[K, V]
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I just ran into two different compiler bugs (one probably being https://issues.scala-lang.org/browse/SI-10081) while trying this out. We're pushing the limits here...
// val xs1 = xs.map { case (k, v) => (v, k) } | ||
// val xs2: strawman.collection.Map[String, Int] = xs1 | ||
val xs1 = xs.map(kv => (kv._2, kv._1)) | ||
val xs2: strawman.collection.Iterable[(String, Int)] = xs1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see in the above lines:
- Using the
.map { case (k, v) => …
syntax breaks type inference ; - The inferred type of
xs1
isIterable[(String, Int)]
instead ofMap[String, Int]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You haven't given it any way to get Map[String, Int]
, have you? The standard trick is something like
def map[C, U](f: ((K, V)) => ((C, U)))(implicit ev: ((C, U)) <:< Tuple2[_, _]): Map[C, U] = ???
or somehow get implicit resolution in there. That still gives broken type inference, but type inference and implicit resolution could be tweaked so that even without inferring all the types, the compiler would know to try this one first (and then be able to get through the case without an issue). I unfortunately don't know whether the internals can support this with a reasonable amount of work, but conceptually it seems like it would solve the issue.
Note--I chose an implicit that does nothing at all, just to get another parameter to disambiguate the two calls. One could imagine a type Priority[I <: Int]
where the compiler would automatically try first things that look like (implicit pri: Priority[1.type])
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just realized that I defined the map
overload that returns a Map
with the following signature:
def map[K2, V2](f: (K, V) => (K2, V2)): Map[K2, V2]
So, it takes as parameter a function with two parameters, not a function with one (tuple) parameter. But at call site I wrote code that was supplying a function taking one tuple parameter. That’s why the type inference didn’t result to what I expected.
I changed the call site and now the overloading trick works.
I did a few fixes since the first push and now I have something that works as follows. If you write: xs.map((k, v) => …) Then it selects the If you write: xs.map(kv => …) Then it selects the However, the following does not compile: xs.map { case (k, v) => … } // missing parameter type for expanded function So, this solution works but is not compatible with existing usage of |
As I mentioned before, you wouldn't expect the |
@Ichoran I was not clear but I tried different type signatures and no ones worked excepted the one that takes a These ones didn’t work, for instance: def map[U, K2, V2](f: ((K, V)) => U)(implicit ev: U <:< (K2, V2)): C[K2, V2]
def map[KV, K2, V2](f: KV => (K2, V2))(implicit ev: KV <:< (K, V)): C[K2, V2] |
Note that in dotty we have an automatic conversion from a binary lambda to a lambda over pairs. We could think of adding this to Scala 1.13 as well, the change is pretty simple. But it seems that @julienrf found how to make it work without that conversion, which is better. |
@odersky It doesn't work for PartialFunction lambdas. I'm not familiar with that part of scalac but it's not clear to me why it couldn't (even without full unification of tuples and parameter lists). The problem only occurs if you overload the existing |
Here's a fix for the PartialFunction case: scala/scala#5698 |
|
||
/** Base Map type */ | ||
trait Map[K, +V] | ||
extends Iterable[(K, V)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to use Product2
instead of Tuple2
here (and throughout)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I’m going to learn something here: I thought (K, V)
was a syntactic shortcut for Tuple2[K, V]
, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. I mean why are you using Tuple2 instead of Product2? I think you could and should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwijnand Why do you prefer Product2
over Tuple2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a design decision on specificity / principle of least power.
The subject here is Map, a top-level trait. Must we constrain its Iterable element type to be Tuple2 or should we allow it instead to return any type of Product2?
Does the correctness of the collections in this rewrite depend on the correctness of the implementation of equals and hashCode of the Iterable element type? If so then there's more safety in using Tuple2 (although not complete safety as Tuple2 isn't final so one could still technically override and subvert its synthetic equals/hashCode).
An example of the benefits in using Product2 instead is that in one's own implementation of Map one could avoid a lot of unboxing and boxing into Tuple2 by using an element type that implements Product2.
One's hand is slightly swayed (possibly unconsciously) by the fact that in Scala, at both the value and type level, the syntax (..) is tuple syntax, instead of product syntax. Therefore requiring the use of Product1/Product2/.. to be used for product syntax, which is more verbose.
Thank you for the question, it allowed me to think out my thoughts and present them in full.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjrd Well spotted. So I think we stick to tuples, after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it's hard to see how maps could profit from Product2
. To gain any efficiency through that, they'd need to already have a Product2 of a key and a value stored somewhere, so they could return that. It's a possibility for mutable hashmaps. Under open hashing, entries could presumably be made into the right Product2 types. But we still don't want to hand out an entry directly, because that could cause a memory leak... So, no.
Regarding "principle of least power": It actually goes the other way! Tuple2 is less powerful than Product2 because it constrains the implementation more. So "least power" and "ease of use" are aligned here, after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjrd You're right. You would need to use a Product2 extractor instead of the language Tuple2 extractor. Another example of the (..) tuple syntax swaying one's hand, yours in this case.
@odersky I think what is "least power" depends on whose side you're looking, the producer or the consumer of the type. Lots of times you're the only implementor, so you only have consumers. But in this case it's a collections library trait, so you have both. So it's a design decision/judgement call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwijnand I agree, it's an interesting observation that the principle works both ways, and means opposite things depending on how you look at it. I think it would be good to find different names for each direction:
-
Principle of least power: Given some requirements, choose the most constraining ("obvious") API that fulfills the requirements, in order to maximize understanding what the implementation will do.
-
Principle of maximal abstraction: Given some requirements, choose the API that admits the largest set of possible implementations.
Neither principle is inherently better than the other, but I believe that currently many libraries go too much towards maximal abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There we go. Nice, I like it.
Superseeded by #24. |
I did a quick experiment to see if overloading
map
methods forMap
collections would work.