Fix == for value classes

[odersky]

We should not optimize

new V(x) == new V(y)   to    x == y

if value class V redefines equals.

This was found when further refining the Lst type, so that == is now supported.

[smarter]

In Scala 2, overriding equals was simply disallowed. If we decide to allow it we need to make sure the semantics are correct and the performance is OK. E.g., it would make sense to rewrite new VC(a) == new VC(b) to VC.equals$extension(a, b).

[odersky]

But we currently allow it, and I think it would be good to keep doing that!

The semantics looks good, as far as the test cases tell us. It does not rewrite to the extension method currently, but even if we do that we'd still have one boxing left. Here's the output of the test case for

c.equals(C(...))

vs

c == C(...)

      Test.C.equals$extension(c, 
        new Test.C(Array.apply(1, Predef.wrapIntArray([2,3 : Int])))
      )

      new Test.C(c).==(
        new Test.C(Array.apply(1, Predef.wrapIntArray([2,3 : Int])))
      )

It's probably easy to make the == case as efficient as equals. Make a separate PR for this?

[Blaisorblade]

I think this deserves further thought.

Beyond the semantic changes discussed below, the supported use case adds one more case that triggers boxing for value classes, which also seems contentious. That boxing is only needed because calling equals requires upcasting to Any (and boxing) so that the body can use a pattern match to unbox the argument and actually compare contents.

All this is avoided by sticking with extension methods.

[Blaisorblade]

The semantics looks good, as far as the test cases tell us.

Unfortunately overriding equals was only forbidden in value classes but not in universal traits, so this PR appears to silently change runtime behavior for code that works right now with Scalac.
It seems necessary to consider and discuss implications for migration. Not changing behavior for now seems safer.

assert(b1 != b2) also succeeds on Scalac, but just because the involved classes are different.
Instead, adding a test like trait Foo extends Any { override def equals(x: Any) = true } would confirm the semantic change with Scalac. To wit:

scala> trait Foo extends Any { override def equals(x: Any) = false }
defined trait Foo

scala>  trait Ding extends Any { override def hashCode = -1 }
defined trait Ding

scala> class Bippy1(val x: Int) extends AnyVal with Foo { }  // warn
defined class Bippy1

scala> class Bippy2(val x: Int) extends AnyVal with Ding { } // warn
defined class Bippy2

scala> val b1 = new Bippy1(71)
b1: Bippy1 = Bippy1@47

scala>      val b2 = new Bippy2(71)
b2: Bippy2 = Bippy2@47

scala> b1 == b2
<console>:14: warning: Bippy1 and Bippy2 are unrelated: they will never compare equal
       b1 == b2
          ^
res0: Boolean = false

scala> trait Foo extends Any { override def equals(x: Any) = true }
defined trait Foo

scala> class Bippy1(val x: Int) extends AnyVal with Foo { }  // warn
defined class Bippy1

scala> val b1 = new Bippy1(71)
b1: Bippy1 = Bippy1@47

scala> b1 == b2
<console>:14: warning: Bippy1 and Bippy2 are unrelated: they will never compare equal
       b1 == b2
          ^
res1: Boolean = false

[odersky]

I think the previous equals behavior was unintentional and non-sensical. So I don't think we need to worry about migration, that should count as a bugfix. As far as I can tell, the boxing behavior is also the same as it was before. So I don't see a downside for doing this.

Boxing could be improved for value classes that define equals, but that's a separate issue for which @smarter would be the best person.

[Blaisorblade]

I think the previous equals behavior was unintentional and non-sensical. So I don't think we need to worry about migration, that should count as a bugfix.

Only making more programs compile is a safe bugfix, changing behavior of existing programs isn't safe. As you pointed out other times, they might depend on it without even knowing. We'd need to forbid the scenario in Scala 2 to make this safe.

And this behavior wasn't even accidental — you, Paul and Adriaan discussed it in scala/bug#6534 and scala/scala#1526, so it can't be that nonsensical, and it sure isn't a bugfix.

That -Xlint warning for class Bippy1(val x: Int) extends AnyVal with Foo { } was even removed in
scala/scala@1da48a4#diff-90c2f5c65d04e4b756b9989ff2e1b97dR339, but apparently only because there was no way to suppress it.

[senia-psm]

In scala2 head +: tail has the same behaviour as head :: tail. This implementation looks like init :+ last.
I suspect this method should be named :+.

[odersky]

@allanrenucci @smarter There are more fixes to inline accessors in this commit which should go into the next release. The fixes are here because the test case is the latest Lst.scala in this PR.

I believe this PR is ready to go in. The only objection is that == is not optimized for value classes, which is true. If we can do this (I guess @smarter would be the person to do it), let's add it now. If not, let's merge and make a separate issue for the optimization part.

If not we can also pull out the inline fixes and add another test that exercises them. But I won't do that, too much other things to do. In any case we need to act on this before the next release is cut.

[smarter]

I believe this PR is ready to go in. The only objection is that == is not optimized for value classes

I'm OK with a non-optimized implementation but @Blaisorblade objection that this silently changes semantics compared to Scala 2 is more serious. For now I'm in favour of just pulling the inline fixes in another PR.

[odersky]

I don't believe the semantics change is worrisome. The previous behavior was that an == in a value class was ignored even if you write it! So anybody writing this either expected the == to be called and did not check, or was just sloppy. So we are talking about bug-for-bug compatibility here, really.

[smarter]

OK. Let's go with this. Meanwhile on the Scala 2 side it'd be great if the warning was enabled by default: scala/scala@1da48a4#diff-90c2f5c65d04e4b756b9989ff2e1b97dR339