Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define equality between converter wrappers #10425

Merged
merged 3 commits into from
Jun 20, 2023

Conversation

RustedBones
Copy link
Contributor

When converter destination collection does not have equality defined, we should override equals/hashcode. 2 wrappers with same underlying collection should be equal.

Follow-up for scala/bug#12683

When converter destination collection does not have equality defined, we
should override equals/hashcode. 2 wrappers with same underlying
collection should be equal.
@scala-jenkins scala-jenkins added this to the 2.13.12 milestone Jun 8, 2023
@som-snytt
Copy link
Contributor

Thanks! It took me a while to realize this is a PR not a new ticket. I was looking at previous EqualsTest, but maybe it doesn't matter where the tests reside.

@som-snytt som-snytt self-requested a review June 8, 2023 20:25
@SethTisue SethTisue added the library:collections PRs involving changes to the standard collection library label Jun 9, 2023
@SethTisue SethTisue requested a review from a team June 9, 2023 00:39
@RustedBones RustedBones force-pushed the equals-converters branch 2 times, most recently from 9d765e0 to 660deb2 Compare June 9, 2023 08:45
@som-snytt
Copy link
Contributor

I confirmed (if it needed confirming) that the scio test passes with this change.

The following is a note to myself, so I don't have to tattoo it on my arm.

I'm not persuaded by delegating for the iterator wrappers, but it's also harmless, perhaps. (And it has the air of completeness.)

So the material change is for JIterableWrapper and JCollectionWrapper. I don't remember if there was a reason they were excluded from the previous fix; maybe it was just an oversight, or the result of zealous minimalism.

Refreshing my memory, I'm a bit surprised by:

scala> val xs = List(1,2,3).asJava
val xs: java.util.List[Int] = [1, 2, 3]

scala> .getClass
val res0: Class[_ <: java.util.List[Int]] = class scala.collection.convert.JavaCollectionWrappers$SeqWrapper

def set(x$1: Int, x$2: Int): Int

scala> xs.set(0, 42)
java.lang.UnsupportedOperationException
  at java.base/java.util.AbstractList.set(AbstractList.java:138)
  ... 30 elided

scala> xs.asScala
val res2: scala.collection.mutable.Buffer[Int] = Buffer(1, 2, 3)

scala> .getClass
val res3: Class[_ <: scala.collection.mutable.Buffer[Int]] = class scala.collection.convert.JavaCollectionWrappers$JListWrapper

scala> res2(0) = 42
java.lang.UnsupportedOperationException
  at java.base/java.util.AbstractList.set(AbstractList.java:138)
  at scala.collection.convert.JavaCollectionWrappers$JListWrapper.update(JavaCollectionWrappers.scala:126)
  ... 30 elided

That seems to me unhelpful. However, in the context of this PR, it endorses just delegating, come what may. The wrappers are the thinnest of veneers. (I almost wrote Venners.)

My other thought is that arbitrary wrappers with the same (eq) underlying should compare equal.

scala> val ns = List(1,2,3)
val ns: List[Int] = List(1, 2, 3)

scala> val xs = ns.asJava
val xs: java.util.List[Int] = [1, 2, 3]

scala> val ys = ns.asInstanceOf[Iterable[Int]].asJava
val ys: Iterable[Int] = [1, 2, 3]

scala> xs == ys
val res3: Boolean = false

A hypothetical rogue wrapper, RandomizedSeqWrapper, would break equality on java.util.List which requires "same elements in same order".

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Straightforward and complete. I had a couple of test tweaks but it can wait for another occasion.

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC, the strategy is to override equals unless the wrapper class already inherits equals (e.g., SeqWrapper, JSetWrapper), right?

If so, should we add equality to DictionaryWrapper?

scala> val m = collection.mutable.Map(1 -> 1)
val m: scala.collection.mutable.Map[Int,Int] = HashMap(1 -> 1)

scala> m.asJavaDictionary == m.asJavaDictionary
val res18: Boolean = false

@lrytz lrytz merged commit 7db02fe into scala:2.13.x Jun 20, 2023
4 checks passed
@RustedBones RustedBones deleted the equals-converters branch August 14, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:collections PRs involving changes to the standard collection library
Projects
None yet
5 participants