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

Can't detect binary incompatibility when trait extends abstract class #225

Open
xuwei-k opened this issue Jun 23, 2018 · 19 comments
Open

Can't detect binary incompatibility when trait extends abstract class #225

xuwei-k opened this issue Jun 23, 2018 · 19 comments
Labels

Comments

@xuwei-k
Copy link
Contributor

xuwei-k commented Jun 23, 2018

library x v1

package com.example

trait A

library x v2

package com.example

abstract class B {
  def foo: Int = 42
}

trait A extends B

mima said library x v1 => v2 binary compatible.

another library y

  • depends on library x v1
package com.example

object C {
  val a = new com.example.A {}
}

main

  • depends on library y
  • also depends on library x v2 (override x v1 transitive dependency from y)
package com.example

object Main {
  def main(args: Array[String]): Unit = println(C.a.foo)
}

run main

[error] (run-main-0) java.lang.ClassCastException: com.example.C$$anon$1 cannot be cast to com.example.B
[error] java.lang.ClassCastException: com.example.C$$anon$1 cannot be cast to com.example.B
[error] 	at com.example.Main$.main(Main.scala:6)
[error] 	at com.example.Main.main(Main.scala)
[error] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
@xuwei-k
Copy link
Contributor Author

xuwei-k commented Jun 23, 2018

FYI, similar (but maybe different) issue #118

xuwei-k referenced this issue in scalikejdbc/scalikejdbc Jun 23, 2018
@dwijnand dwijnand added the bug label Jul 18, 2018
@dwijnand
Copy link
Collaborator

Just re-reading this, I wonder if the fact that class B is abstract is relevant.

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

The fact that class B is abstract is not relevant: the reproduction fails the same and MiMa doesn't detect it the same.

The fact that it's a class and not a trait is relevant, when it's a trait MiMa doesn't report it for Scala 2.12+, which is correct because Scala 2.12+ implements it as default methods. That's what #118 was about.

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

Well it looks like the problem might be that in

class B
trait A extends B

A has no superclass?!

@szeiger
Copy link
Contributor

szeiger commented Feb 7, 2019

scala> trait A1
defined trait A1

scala> :javap -c A1
Compiled from "<console>"
public interface $line8.$read$$iw$$iw$A1 {
}

scala> trait A2 extends B
defined trait A2

scala> :javap -c A2
Compiled from "<console>"
public interface $line5.$read$$iw$$iw$A2 {
}

This is as expected. An interface cannot extend a class. Similar to a self-type, the fact that A2 extends B is not visible to Java but unlike a self-type it is externally visible in Scala so the compiler may make use of it.

The incompatibility in the example is really the trait instantiation in C. main is a convenient way to expose it but C is already broken.

I can't think of a way of detecting this short of examining the Scala signatures, which means MiMa needs to be able to read those for all supported Scala versions.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 7, 2019

(one of the motivations for scala/scala-dev#601)

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

I see. Makes sense, but it's also tragic. So public traits that don't extend Object are binary hazards. How do we help users like Yoshida-san from running into these problems?

@sjrd, as a fellow binary compatibility advocate, does that change your stance on scala/scala-dev#601?

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

Forgot to say: thanks Stefan for the explanation.

Tbh this has kind of blown my mind. If declare a class extends the trait does scalac gives it the traits (non) superclasses? I guess it makes type A not a subtype of B even though it extends class B. Confusion.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 7, 2019

i don't follow entirely, but think of a trait extending a class as a downsteam obligation for any classes extending that trait (same as with a self type) -- the earliest opportunity, we'll add that trait's declared superclass as a parent for a class that mixes in the trait

@adriaanm
Copy link
Contributor

adriaanm commented Feb 7, 2019

so, any instance of the trait will also be an instance of its declared superclass

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

Yeah I get the mechanics now, but it breaks my mental model (invariant) that extends means parent and supertype. Still learning fundamentals in Scala, it's incredible/scary.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 7, 2019

Agreed -- again, the surprise element is why I think we should get rid of it, but I guess I should argue that over at the other ticket :-)

@szeiger
Copy link
Contributor

szeiger commented Feb 7, 2019

It's similar to a trait that can't be compiled to an interface (which was most traits before 2.12, fewer now). When you create a class that extends the trait the compiler has to add all the stuff that was not allowed in the interface.

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

Yeah, I'm with Adriaan, we should remove from trait that which is not allowed in an interface.

@sjrd
Copy link
Contributor

sjrd commented Feb 7, 2019

@sjrd, as a fellow binary compatibility advocate, does that change your stance on scala/scala-dev#601?

No, because you can reproduce the exact same issue with a self-type, and no one is advocating getting rid of self types.

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

How so?

Welcome to Scala 2.12.7 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_202-ea).
Type in expressions for evaluation. Or try :help.

scala> abstract class B {
     |   def foo: Int = 42
     | }
defined class B

scala> trait A { self: B => }
defined trait A

scala> val a = new A {}
<console>:12: error: illegal inheritance;
 self-type A does not conform to A's selftype A with B
       val a = new A {}
                   ^

@sjrd
Copy link
Contributor

sjrd commented Feb 7, 2019

Lib.scala version 1:

package test

trait B {
  def foo(): Int = 42
}

App.scala:

package test

object App {
  def main(args: Array[String]): Unit = {
    val b = new B {}
    println(b.foo())
  }
}
$ ~/opt/scala-2.12.6/bin/scalac Lib.scala 
$ ~/opt/scala-2.12.6/bin/scalac -cp . App.scala 
$ ~/opt/scala-2.12.6/bin/scala -cp . test.App
42

Lib.scala version 2:

package test

abstract class A {
  def bar(): Int = 42
}

trait B { self: A =>
  def foo(): Int = bar()
}
$ ~/opt/scala-2.12.6/bin/scalac Lib.scala 
$ ~/opt/scala-2.12.6/bin/scala -cp . test.App
java.lang.ClassCastException: test.App$$anon$1 cannot be cast to test.A
        at test.B.foo(Lib.scala:8)
        at test.B.foo$(Lib.scala:8)
        at test.App$$anon$1.foo(App.scala:5)
        at test.App$.main(App.scala:6)
        at test.App.main(App.scala)
        at ...

@dwijnand
Copy link
Collaborator

dwijnand commented Feb 7, 2019

Thanks, it's slightly different, but fundamentally the same problem.

Note to self's brain-MiMa:

  • beware what your open traits extend (prefer extending Object)
  • beware what your open traits self subtype

In addition to the other binary compatibility hazardous problems, like super calls, fields, and etc...

Maybe I'll just avoid traits altogether...

@adriaanm
Copy link
Contributor

adriaanm commented Feb 7, 2019

That's why a @PureInterface annotation would be useful: the compiler could check that your trait doesn't have any of this baggage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants