Fix #1770: Fix __scala_== with ScalaNumbers.

[LeeTibbert]

• This PR was motivated by Issue __scala_.== is borked with BigInt & BigInteger, almost certainly both BigDecimal #1770:
  "_scala== is borked with BigInt & BigInteger, both BigDecimal".

  That issue is now resolved.

• This PR needs review from experts in Scala & Java equality. It affects a key and widely used
  piece of Scala Native.

• The presenting problem was that some tests used to test Zio compiled
  with Scala native. reported failures not seen in JVM or Scala.js
  variants. Examination showed that the reported failures were because
  two Scala BigInts compared false using '==' while comparing true
  using .equals(). They should have compared true in the former case
  and did not.

  Later studies for issue __scala_.== is borked with BigInt & BigInteger, almost certainly both BigDecimal #1770 showed that comparison of two
  Scala BigDecimal instances which should have compared '=='
  as true compared as false.

  The edit to javalib/...java/lang/Number.scala addresses this problem.
  ScalaNumber extends Number.

• I scanned the entire code base for similar problems. The family
  of Atomic*Number objects were examined more closely and found not
  to bear this flaw.

• Unit tests were created to exercise the proposed changes. They
  failed before this PR and pass as of this PR.

Documentation:

• This PR deserves a prominent change note under a "Bugs" fixed
  section.

Testing:

Safety

• Built ("rebuild")and tested ("test-all") in debug mode using sbt 1.3.10
  & Java 8 on X86_64 only . All tests pass.

Efficacy

• Specially crafted unit-tests provided in this PR fail before the PR and pass after it.

[sjrd]

This should not be here. The codegen is supposed to automatically add this in the vtable because BigDecimal overrides equals, as explained by the comment at

scala-native/nativelib/src/main/scala/java/lang/Object.scala

Lines 39 to 44 in b49e74e

	@inline def __scala_==(that: _Object): scala.Boolean = {
	// This implementation is only called for classes that don't override
	// equals. Otherwise, whenever equals is overriden, we also update the
	// vtable entry for scala_== to point to the override directly.
	this eq that
	}

If that's not the case, that's a problem in the codegen; not in the library.

In general, user space classes should never implement __scala_==, since that is an implementation detail of Scala Native.

[LeeTibbert]

Removed

[sjrd]

Same as the comment in BigDecimal above.

[LeeTibbert]

removed

[sjrd]

I don't think this is the appropriate fix. The previous code should have been correct, if java.lang.Object.__scala_== had a reasonable implementation. The reasonable implementation should be:

  @inline def __scala_==(that: _Object): scala.Boolean =
    this.equals(that)

Instead, it's

scala-native/nativelib/src/main/scala/java/lang/Object.scala

Lines 39 to 44 in b49e74e

	@inline def __scala_==(that: _Object): scala.Boolean = {
	// This implementation is only called for classes that don't override
	// equals. Otherwise, whenever equals is overriden, we also update the
	// vtable entry for scala_== to point to the override directly.
	this eq that
	}

as an optimization based on some vtable hackery.

Unfortunately the vtable hackery doesn't patch up super.__scala_== calls. That means we can never do super.__scala_==, and instead we should inline the correct, reasonable implementation that is written nowhere.

So the correct implementation of Number.__scala_== should be:

  @inline override def __scala_==(other: _Object): scala.Boolean = {
    if (other.isInstanceOf[ScalaNumber] && !this.isInstanceOf[ScalaNumber]) {
      other.equals(this)
    } else {
      this.equals(other)
    }
  }

[LeeTibbert]

I committed with the recommended fix and Travis CI is all green. Thank you for the
patient explanation.

[LeeTibbert]

After the successful commit, I sat down and drew out the truth tables, mentally thanking my Discrete Math professor.

I believe the following code is equivalent and correct. It passes the same tests as the
code in the current commit. I find this code to be easier to read and understand.
I think future developers may also find it easier.

I can do another commit if my proposed code is acceptable.

  @inline override def __scala_==(other: _Object): scala.Boolean = {           
    if ((this.isInstanceOf[ScalaNumber]) ||                                    
        (!other.isInstanceOf[ScalaNumber])) {                                  
      this.equals(other)                                                       
    } else {                                                                   
      other.equals(this)                                                       
    }       

It may also have performance benefits. I believe that the this left comparand will more
likely be a ScalaNumber, by a large factor. The code above would do one test in that case,
leading to a lower amortized cost for some/most workloads.

Since the code is inlined, and the type of this is known at compile time, there is
a sporting chance that the inliner will reduce the call to this.equals(other) when
this is known to be a ScalaNumber.

Normally, I would go with the commited code and be done with it. However, this is a
hot path, where likely instruction path length matters, as long as correctness is maintained.

Insert the classical refrain of "Show me the data, across a series of workloads'.

Note to future self:

The test for other being a scala number is necessary to pass the 'j.l.Integer == BigInt case and similar in the test suite. The code above can not be correctly reduced tothis.equals(other)`.

[sjrd]

I believe the following code is equivalent and correct. It passes the same tests as the
code in the current commit. I find this code to be easier to read and understand.
I think future developers may also find it easier.

I can do another commit if my proposed code is acceptable.

  @inline override def __scala_==(other: _Object): scala.Boolean = {           
    if ((this.isInstanceOf[ScalaNumber]) ||                                    
        (!other.isInstanceOf[ScalaNumber])) {                                  
      this.equals(other)                                                       
    } else {                                                                   
      other.equals(this)                                                       
    }       

This is indeed correct. I'm not sure about the performance impact.

It may also have performance benefits. I believe that the this left comparand will more
likely be a ScalaNumber, by a large factor. The code above would do one test in that case,
leading to a lower amortized cost for some/most workloads.

The cases where this is a ScalaNumber are basically only for scala.math.BigInt and scala.math.BigDecimal. For those, we know that equality is expensive (at least it's more than one or two tests) so their equality will dwarf whatever optimization we try to do for them in __scala_==. Therefore, this does not seem to be a scenario worth optimizing for.

Since the code is inlined, and the type of this is known at compile time, there is
a sporting chance that the inliner will reduce the call to this.equals(other) when
this is known to be a ScalaNumber.

That is true, but the same reasoning applies to the existing code as well. If this is known to be a ScalaNumber at compile time, then the condition

other.isInstanceOf[ScalaNumber] && !this.isInstanceOf[ScalaNumber]

reduces to

other.isInstanceOf[ScalaNumber] && false

which, because isInstanceOf is always pure, can also be optimized down to

false

allowing the whole method to reduce to this.equals(that).

[sjrd]

Also, could you rebase on the latest master? It seems some changes are scalafmt applied to otherwise untouched files, which shouldn't happen if the PR is up-to-date with master.