Outer check should use `equals` rather than `eq` in inner class

[alexarchambault]

reproduction steps

// class Wrapper, inner class Foo
class Wrapper {
  // all Wrapper instances are "equals"
  override def equals(obj: Any) =
    obj != null && obj.isInstanceOf[Wrapper]

  case class Foo()
}

// create two Foo instances via two Wrapper instances
val wrapper1 = new Wrapper
val foo1 = new wrapper1.Foo()
val wrapper2 = new Wrapper
val foo2 = new wrapper2.Foo()

// get a hand on the outer reference of Foo
val outerFld = classOf[Wrapper#Foo].getDeclaredField("$outer")
outerFld.setAccessible(true)

// equality checks
val outerReferencesEqual = outerFld.get(foo1) == outerFld.get(foo2)
val instancesEqual = foo1 == foo2

problem

instancesEqual is false, even though outerReferencesEqual is true.

expectation

instancesEqual is true.

This is related to but not exactly the same problem as #4440 (comment). Here, we don't have the final modifier, so we don't run in #4440. The issue here is about the use of eq, rather than equals, to compare outer references.

[sjrd]

This behavior is by-spec.

The equals method of case classes must be consistent with pattern matching on the case class. Now, pattern matching takes paths into account: pattern matching with case Foo(...) expands to case Wrapper.this.Foo(...), which only matches if the path (the outer reference) of the scrutinee is equal to the path Wrapper.this.

Since paths are always compared by identity (eq), it follows that the equals() method of case classes must compare the outer references with eq.

[alexarchambault]

FYI that corresponds to the spark issues SPARK-2620 and SPARK-9621, that have been biting people for years. (SPARK-2620 was bulk closed with resolution "incomplete" after being open for 5 years, and is still unfixed as of now…)

Overriding equals in their wrapper would offer a way to properly fix them.

[hrhino]

It's also unsound (using a local scala in which I implemented this change):

scala> :pa -raw
// Entering paste mode (ctrl-D to finish)

class Outer[T] {
  override def equals(that: Any) = that.isInstanceOf[Outer[T]]
  case class Foo(x: T)
}

// Exiting paste mode, now interpreting.


scala> val oi = new Outer[Int]; val os = new Outer[String]
oi: Outer[Int] = Outer@68d6f48e
os: Outer[String] = Outer@6c96403e

scala> (oi.Foo(1) : AnyRef) match { case os.Foo(x) => x.length }
java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
  ... 28 elided

[alexarchambault]

@hrhino It's unsound because the Outer.equals method that you added is unsound.

[smarter]

FYI that corresponds to the spark issues SPARK-2620 and SPARK-9621, that have been biting people for years.

Am I understanding correctly that this is happening because spark uses a class-based repl? If so, that sounds like a good reason for not using a class-based repl (I still think that all issues with the object-based repl are solvable: scala/scala3#5135 (comment)).

[hrhino]

What is unsound about my equals? It's the same as yours! (Ok, I elided the null check because I'm lazy.)

I don't even have to write an equals method.

case class Outer[T]() {
  case class Foo(t: T)
}

[sjrd]

x.isInstanceOf[C] always returns false when x is null. So even with the laziness, the two equals methods are strictly equivalent.

[lrytz]

For reference, https://www.scala-lang.org/files/archive/spec/2.13/08-pattern-matching.html#type-patterns

A singleton type 𝑝.type. This type pattern matches only the value denoted by the path 𝑝 (the eq method is used to compare the matched value to 𝑝).

[lrytz]

spark uses a class-based repl?

I think that's true, I don't know / remember why.

[joroKr21]

I think that's true, I don't know / remember why.

To be able to serialize functions and send them across the network I think.

[alexarchambault]

It would still be interesting to allow to replace the eq with an equals for outer references, when a specific compiler setting is passed say. That setting could be enabled along -Yrepl-class-based, and the right equals method added to the wrappers of the class-based repl. That would allow to fix SPARK-2620, which has been opened / unresolved for 6 years already.

That would only impact code entered by users in the repl, and only so when the right compiler options are passed.

As @smarter said, scala/scala3#5135 (comment) may address that if it gets implemented. But that's if there are no roadblocks down the line, and that's in a more distant future.