Java wrappers need not be case classes

[som-snytt]

Inspired by scala/scala3#15764 (comment) to consider whether the wrapper classes ought to be case classes, as they have been since early days.

The hierarchy is complex (because collections).

ConcurrentMapWrapper overrode the underlying case class field in MutableMapWrapper, to narrow the result type. Now it provides underlyingConcurrentMap as a convenience.

Arguably, the pattern matches are easier to read.

It would be nice to backport universal apply and compile library with -Xsource:3.

Mima may disagree and require persuading.

[som-snytt]

Mima may disagree and require persuading.

[error]   - library/mimaReportBinaryIssues failed: java.lang.RuntimeException: Failed binary compatibility check against org.scala-lang:scala-library:2.13.8! Found 186 potential problems (filtered 7)

Only 186 issues. That includes all the junk case class synthetic methods. Maybe it's possible to quarantine the internal package.

[lrytz]

👍 (for the record, the changed classes are private[collection])

[kasparito]

This change causes lots of problems in a large code base that I am working on. When scala.collection.convert.JavaCollectionWrappers.IterableWrapper was a case class, equals was defined as comparing the wrapped iterables. Now it is not:

scala> import scala.jdk.CollectionConverters.IterableHasAsJava
import scala.jdk.CollectionConverters.IterableHasAsJava

scala> Nil.asJavaCollection == Nil.asJavaCollection
val res0: Boolean = false

scala> Set("foo").asJavaCollection == Set("foo").asJavaCollection
val res1: Boolean = false

[SethTisue]

@scala/collections wdyt? is this something we should consider reverting?

on the one hand, as a behavior change it seems like fair game for a minor version. (recall that Scala 2 versions are epoch.major.minor, not major.minor.patch

on the other hand, if the change isn't that high-value, then maybe the disruption isn't worth it

on the other other hand, it seems unlikely we'll be doing 2.13.11 in less than 3-4 months, at which point I'm not sure the possible disruption of changing it back is worth it — we might have felt differently if the report had come in during the brief 2.13.10 window

would anyone like to make a specific, evidence-supported argument that this wasn't a fair-game change...?

[SethTisue]

note that https://docs.oracle.com/javase/8/docs/api/java/util/Collection.html#equals-java.lang.Object- says:

the simplest course of action is to rely on Object's implementation, but the implementor may wish to implement a "value comparison" in place of the default "reference comparison." (The List and Set interfaces mandate such value comparisons.)

[som-snytt]

@kasparito Sorry for the trouble. Universal methods FTW. "Nothing is internal API."

The motivation was unexpected member overrides in the internal classes; case classiness was taken as a convenience for construction. Reliance on equals and hashCode didn't come up.

Maybe hashCode not so much because of mutability.

I think the expectation that it just works is justified not unreasonable.

Also compare scala/bug#10663 where the asJava side has a contract that is not lax.

[som-snytt]

I took a look, and it mostly just works. However, AbstractCollection doesn't do structural equality:

scala> def f = new java.util.AbstractCollection[Int] {
     | def size = 3; def iterator = Iterator(1,2,3).asJava }
def f: java.util.AbstractCollection[Int]

scala> f == f
val res0: Boolean = false

The other adapters extend the specific base classes which do implement the expected equals.

Probably it's harmless to remain compatible with the previous behavior for IterableWrapper.

I'll open a ticket to track the change.

[SethTisue]

resolved by #10205