Skip to content

Conversation

retronym
Copy link
Contributor

@retronym retronym commented Nov 17, 2016

We can reliably/simply determine whether a class if from scalac
by the presence/absense of Scala/ScalaSig attributes. With this
knowledge, we can include static methods from non Scala classfiles
into MiMa's purview.

I've added specific code to warn about static <-> non-static changes,
which aren't binary compatible. In addition, the existing rules will
now run for these members.

In order to make this work, we also need to skip static members in
Scala classes after parsing attributes

Two other problems discovered and fixed while implementing this:

  • Members.members is only a TraversableOnce which is traversed in
    the constructor. It should not be exposed as a field.

  • The now inlined methods parseFields and parseMethods in
    ClassfileParser did not always skip the members in the class file
    even though attributes have to be parsed in all cases afterwards.

Co-Authored-By: Stefan Zeiger [email protected]

@retronym
Copy link
Contributor Author

Review by @2m @ktoso @SethTisue

hat tip to @paplorinc over in scala/scala#5528 for trying to sneak (😜) binary incompatible changes to BoxesRuntime that MiMa didn't report (even after #138.)

@retronym
Copy link
Contributor Author

In the 2.11 and 2.12 test runs:

[info] Test 'test-case-class-becomes-final-nok' succeeded.
java.lang.RuntimeException: com.typesafe.tools.mima.lib.TestFailed: test-case-class-concrete-becomes-abstract-nok' failed.
	The following problems were not expected
	- method apply()A in class A does not have a correspondent in new version
	at scala.sys.package$.error(package.scala:27)
	at MimaBuild$$anonfun$runTest$1.apply(Build.scala:229)
	at MimaBuild$$anonfun$runTest$1.apply(Build.scala:201)
	at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
	at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
	at sbt.std.Transform$$anon$4.work(System.scala:63)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
	at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
	at sbt.Execute.work(Execute.scala:237)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
	at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
	at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
[error] (test-case-class-concrete-becomes-abstract-nok/*:testFunctional) com.typesafe.tools.mima.lib.TestFailed: test-case-class-concrete-becomes-abstract-nok' failed.
[error] 	The following problems were not expected
[error] 	- method apply()A in class A does not have a correspondent in new version
[error] Total time: 52 s, completed Nov 17, 2016 5:53:54 AM

Sound familiar to anyone?

@szeiger
Copy link
Contributor

szeiger commented Nov 22, 2016

I believe this is due to scala/scala#2951. It is a genuine new binary incompatibility, so we could just leave the reporting as is and change the expected results.

Or you could argue that it should be subsumed under either one of the already reported errors:

class A was concrete; is declared abstract in new version
method apply()A in object A does not have a correspondent in new version

But this would require a way to recognize that it is in fact an auto-generated apply of a case class.

@szeiger
Copy link
Contributor

szeiger commented Nov 28, 2016

On second glance, it's supposed to ignore Scala files. Here's the problem with the current approach:

---- checking _isScala for class A$
---- checking _isScala for class A$
---- setting _isScala for class A$
---- checking _isScala for class A
---- checking _isScala for class A
---- setting _isScala for class A
---- checking _isScala for class A
---- setting _isScala for class A
---- checking _isScala for class Object
---- setting _isScala for class Product
---- setting _isScala for class Equals
---- setting _isScala for class Serializable
---- setting _isScala for class Product
---- setting _isScala for class Equals
---- setting _isScala for class Serializable
---- checking _isScala for class A$
---- checking _isScala for class A$
---- setting _isScala for class A$
---- setting _isScala for class AbstractFunction0
---- setting _isScala for class Function0

There's a comment claiming "safe because attributes already parsed for clazz" but it appears to be false. The log shows that the check is performed before the SourceFile is parsed (and thus _isScala set for the class).

@szeiger
Copy link
Contributor

szeiger commented Nov 28, 2016

Here's a fix for the problem: master...szeiger:retronym-topic/java-statics

We can reliably/simply determine whether a class if from scalac
by the presence/absense of Scala/ScalaSig attributes. With this
knowledge, we can include static methods from non Scala classfiles
into MiMa's purview.

I've added specific code to warn about static <-> non-static changes,
which aren't binary compatible. In addition, the existing rules will
now run for these members.

In order to make this work, we also need to skip static members in
Scala classes after parsing attributes

Two other problems discovered and fixed while implementing this:

- `Members.members` is only a `TraversableOnce` which is traversed in
  the constructor. It should not be exposed as a field.

- The now inlined methods `parseFields` and `parseMethods` in
  `ClassfileParser` did not always skip the members in the class file
  even though attributes have to be parsed in all cases afterwards.

Co-Authored-By: Stefan Zeiger <[email protected]>
@retronym
Copy link
Contributor Author

@szeiger I've squashed your fix into this PR.

@szeiger szeiger merged commit f9cd0c1 into lightbend-labs:master Nov 29, 2016
@ktoso
Copy link
Contributor

ktoso commented Dec 1, 2016

I'm seeing something very weird in Akka on 0.1.12:

[error]  * static method shutdownActorSystem(akka.actor.ActorSystem,java.lang.Boolean)Unit in class akka.testkit.JavaTestKit is non-static in current version
[error]    filter with: ProblemFilters.exclude[VirtualStaticMemberProblem]("akka.testkit.JavaTestKit.shutdownActorSystem")

for:

class JavaTestKit {
  public static void shutdownActorSystem(ActorSystem actorSystem, Boolean verifySystemShutdown) {
    shutdownActorSystem(actorSystem, null, verifySystemShutdown);
  }

@ktoso
Copy link
Contributor

ktoso commented Dec 1, 2016

I'll try to reproduce as a test in MiMa

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

Successfully merging this pull request may close these issues.

4 participants