scala / collection-strawman

Implementation of the new Scala 2.13 Collections
Apache License 2.0
200 stars 72 forks source link

Don't extend collection types in ArrayOps #479

Closed szeiger closed 6 years ago

szeiger commented 6 years ago

See commit comments for details.

lrytz commented 6 years ago

Nice! What are your thoughts for keeping the "interface" (available methods) in synch? https://github.com/scala/collection-strawman/issues/465

julienrf commented 6 years ago

This PR makes type inference fail on arrays usage. Also, what do you think of implementing the optimized versions of reduction operations on WrappedArray instead (which inherits from IterableOps)?

szeiger commented 6 years ago

There's one type inference failure in the tests. I have not been able to figure out why this change makes it fail. The example we have is something like Array("") ++ Array("", null). ArrayOps should have exactly the same methods as before (i.e. a single ++ method that takes a ClassTag; the old implementation had another without the ClassTag which was defined in a superclass and therefore shadowed) but the failure is new. Note that the general problem already existed before this change, we just didn't have any test for it: Iterable("") ++ Array("", null) fails in the same way even though it does not involve ArrayOps.

szeiger commented 6 years ago

We could additionally make WrappedArray use more optimized implementations but the primary place for them should be ArrayOps because it is a pure value class. Because of the specialization WrappedArray always needs to allocate an instance (unless you implement a method in all of the specialized subclasses, which was probably the reason for the duplicate apply and update implementations in the old ArrayOps).

szeiger commented 6 years ago

Something weird is going on with the Array.apply inference problem. The good news is that it works when we're using Predef and the scala namespace:

[info] Running scala.tools.nsc.MainGenericRunner -usejavacp
Welcome to Scala 2.13.0-20180220-202434-37b204e (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_102).
Type in expressions for evaluation. Or try :help.

scala> Array("") ++ Array("", null)
res0: Array[String] = Array("", "", null)

scala> Iterable("") ++ Array("", null)
res1: Iterable[String] = List("", "", null)

The problem only occurs when testing in the strawman namespace. My guess would be that some special case handling in the compiler is getting in the way. For example, you can see in -Ytyper-debug output that arrayToWrappedArray is always considered as an implicit conversion even when it is not in scope and not imported.

szeiger commented 6 years ago

Ticket for the overload problem: https://github.com/scala/bug/issues/10746

The reason why it works from Predef is that we still have the separate implicits for converting primitive and reference arrays to WrappedArray, whereas in strawman there is only a single conversion method.