foldRight override on indexedSeqOps is forward incompatible

[martijnhoekstra]

reproduction steps

as reported by @diesalbla on gitter: https://gitter.im/scala/scala?at=5f4e88c49566774dfe49860c

using scodec-bits 1.1.19 and scala 2.13.2

import scodec.bits._

scodec.bits.ByteVector(1, 2, 3, 4, 5, 6, 7).toIndexedSeq.foldRight(0)(_ + _)

problem

java.lang.NoSuchMethodError: scala.collection.IndexedSeqOps.foldRight$(Lscala/collection/IndexedSeqOps;Ljava/lang/Object;Lscala/Function2;)Ljava/lang/Object;
	at scodec.bits.ByteVector$$anon$6.foldRight(ByteVector.scala:706)

scodec-bits 1.1.19 is compiled against scala 2.13.3 which introduced an override on IndexedSeqOps for foldRight in scala/scala#8929

toIndexedSeq is defined as

final def toIndexedSeq: IndexedSeq[Byte] =
  new IndexedSeq[Byte] {
    val length = toIntSize(ByteVector.this.size)
    def apply(i: Int) = ByteVector.this.apply(i.toLong)
  }

foldRight apparently links to the overload which doesn't exist on 2.13.2

I don't see any mima warnings on that PR.

[diesalbla]

I would like to ask Michael Pilquist @mpilquist, as maintaner/developer of scodec and scodec-bits, for attention.

[lrytz]

To lay out what's going on: every concrete trait method gets a static method on the side

trait T {
  def f = 1
  // static def f$ = invokespecial f
}

This allows invoking the specific implementation T.f from anywhere (bypassing dynamic dispatch) by calling T.f$. These static accessor methods are used in compiler-generated "mixin forwarders". For every concrete trait method that a class inherits, a mixin forwarder is generated

class C extends T {
  override def f = T.f$
}

This means that adding a new override to a trait is not forwards binary compatible: new subclasses will refer to the static forwarder of the new override.

Even though this situation is the same since 2.12.0, I think we were not aware of how it impacts binary compatibility.

[martijnhoekstra]

Does overriding the overridden method again sound like a plausible workaround for scodec-bits?

[lrytz]

Yes, but a simple override def foldRight = super.foldRight would not help, super calls are implemented using these static accessor methods.

[lrytz]

Echoing a comment I made elsewhere: "fixing" the incompatibility by reverting 2.13.4 back to the binary format of 2.13.0 is most likely a worse choice than doing nothing.

A library compiled against 2.13.3 may include a reference to the new static super accessor, and therefore not work with a 2.13.2 standard library. However, if we remove the override in 2.13.4, that library build will also not work on 2.13.4. The latter, a library compiled with 2.13.x used with 2.13.(x+n), is quite certainly much more common.

[smarter]

This means that adding a new override to a trait is not forwards binary compatible: new subclasses will refer to the static forwarder of the new override.

Dotty used to have a different encoding for traits which didn't require static forwarders and so might not have the same compat issue (not sure, I haven't checked): scala/scala3#5928, but we recently switched to the 2.13 encoding to stay binary-compatible with it.

[lrytz]

Yeah, with -Xmixin-force-forwarders:false the problem would almost not exist, there are only some corner cases where the mixin forwarder method generated in subclasses is actually required for correctness. But given the default is :true, that doesn't help us much.

[smarter]

Could 2.13.4 keep the static method for bc with 2.13.3 but make sure calls to it never get emitted for bc with 2.13.2- ?

[lrytz]

Yes, we can internalize the known binary incompatibilities (https://github.com/scala/scala/pull/9197/files).

We could then also add an annotation like @inheritSignature to allow new overrides in traits. The compiler would avoid generating static accessors, mixin forwarders and/or use invokespecial for the super calls.

[lrytz]

Could 2.13.4 keep the static method for bc with 2.13.3 but make sure calls to it never get emitted for bc with 2.13.2- ?

The downside of this is that it introduces serialization incompatibilities, see scala/scala#9263 (comment).

[dwijnand]

Current:

• ✅ faster implementation (method override with a faster implementation)
• ❌ binary incompatible (the mixin forwarder points to a method that doesn't exist in 2.13.2-)
• ✅ serialisation compatible (there was a mixin forwarder before and after)

Option 1 - Excluding the forwarders:

• ✅ faster implementation (invokevirtual to the faster impl if available)
• ✅ binary compatible (not pointing to anything)
• ⚠️ serialisation incompatible unless you pin the @SerialVersionUID

Option 2 - Including the forwarders but point to the old impl:

• ❌ slower implementation (as well as confusing: the method override is never invoked)
• ✅ binary compatible (pointing to the old impl)
• ✅ serialisation compatible

So Option 1 looks better than Option 2 and the status quo.

[dwijnand]

scala/scala#9263 (comment)