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

Scala 2.12 support #78

Closed
SethTisue opened this issue Aug 21, 2015 · 21 comments
Closed

Scala 2.12 support #78

SethTisue opened this issue Aug 21, 2015 · 21 comments
Assignees

Comments

@SethTisue
Copy link
Collaborator

@sjrd reports that MiMa is just about useless with 2.12.0-M2.

for now, 2.12 is very much a moving target and the way lambdas and other things are encoded in bytecode is still changing. so it may or may not make sense for someone to attempt to tackle this right now, but obviously it will need tackling eventually.

@dotta
Copy link
Contributor

dotta commented Aug 22, 2015

Hi Seth, could you describe what the issue is? I had a quick look at the conversation you linked, but it's not apparent to me what is the problem, and how you plan on fixing it. I'd appreciate if this ticket could contain these information.

@sjrd
Copy link
Contributor

sjrd commented Aug 22, 2015

The issue is that MiMa reports many more spurious incompatibility errors related to anonymous functions. scalac 2.12 generates one (private but public-ized) method in the enclosing class for the body of every anonymous function. If you add, move, or remove anonymous functions within a method, scalac will now produce a different set of such methods, and those produce binary incompatibilities as identified by MiMa. Although, in fact, we know they cannot be called from anywhere but the method body, since they have been generated by scalac.

Unlike other sources of synthetic-method-causes-bin-incompat issues, this one produces so many false positives that humanly sorting through those (to whitelist them) is impractical. See the gist with the reported errors.

As of now, I am not 100% sure where the responsibilities to fix this lie. It might be scalac's fault, or MiMa's fault, or they might have shared responsibilities in the problem.

An obvious (but not well thought-about; might be too permissive) fix with the way 2.12.0-M2 emits lambdas would be to blindly ignore any method containing $anonfun$ in its name, assuming that comes from an anonymous function.

@dotta
Copy link
Contributor

dotta commented Aug 22, 2015

What I don't get is why these synthetic methods aren't private in the bytecode. If they are non-public, then they might be accessed in other places, which could lead to binary incompatibilities. Do you guys really feel confident this eventuality isn't possible (now and in the future)?

@dotta
Copy link
Contributor

dotta commented Aug 22, 2015

I'd still appreciate if someone could provide a small class snippet with an example of the generated synthetic method so that I can get up to speed on what has changed in Scala 2.12 :-)

@sjrd
Copy link
Contributor

sjrd commented Aug 22, 2015

What I don't get is why these synthetic methods aren't private in the bytecode.

I think because it is actually called by the apply method of the anonymous class generated for the lambda. It's like when you call a private method from an inner class: it's valid Scala, but not valid bytecode, so scalac "public-izes" the method with a horrible mangled name. But at the language level it's not actually visible from outside, so it can't be called.

@dotta
Copy link
Contributor

dotta commented Aug 23, 2015

I see. I'm wondering if having Scala generating these methods as private package, and then adding in MiMa an option for ignoring private package binary incompatibilities might be a better solution. The advantages I see are:

  • it would substantially reduce the number of filtering rules people are using (since usually private package things are often considered internal API), and
  • no ad-hoc logic in MiMa for ignoring members based on the mangled name.

This should be relatively straightforward to implement in MiMa. When parsing the classfiles, package private classes/members could be simply ignored.
EDIT: It is better to keep parsing everything and avoid reporting incompatibilities from package private members. I don't think it will be hard to implement.

@SethTisue
Copy link
Collaborator Author

@dotta you asked for an example.

if you compile e.g.:

class C {
  var a = 5
  def foo() {
    val f1 = () => a.toString
    val f2 = (x: Int) => x * a
    val f3 = (z: String) => s"$z$a$z"
  }
}

it's crucial here that the lambdas close over something from the enclosing class, otherwise the generated anonfun methods would be static and be ignored by MiMa.

compiled with 2.12.0-M2, you get (javap):

public class C {
  ...
  public final java.lang.String C$$$anonfun$1();
  public final int C$$$anonfun$2(int);
  public final java.lang.String C$$$anonfun$3(java.lang.String);
  ...
}

and the anonfun methods get picked up by MiMa. the naming isn't stable (the $1, $2 numbering), but that's not the real problem, the real problem is that MiMa doesn't ignore the methods entirely.

@retronym @adriaanm can you weigh in on whether you advocate fixing it on the compiler side with Mirco's private or package-private suggestion, or whether you think it would be better for MiMa to just ignore anything with $anonfun in the name, as suggested by Sébastien?

@adriaanm
Copy link
Contributor

Can we make them private without hurting the inliner? This would mean you cannot inline code that calls an anonfun unless you have access to that anonfun method. I'm not sure either we can ignore anonfun methods unless we are sure they are never referenced from outside the compilation unit that defines them, but that seems to contradict the assumption of my previous concern.

@SethTisue
Copy link
Collaborator Author

hmm, inliner, so maybe we need @lrytz

@retronym
Copy link
Contributor

The areas to consider are deserialization and the inliner.

For deserialization, I had originally thought that we would need to make them public to support our strategy of using generic code (LambdaDeserializer) to deserialize all lambdas. Java does it differently, and embeds custom deserialization code into each class that hosts lambdas for serializable functional interfaces.

However, after working out the the details of the implementation, I found that we needed to pass a MethodHandles.Lookup from the lambda host's $deserializeLambda$ method to LambdaDeserializer, and that this would give us permission to form a method handle over the private implementation method.

However, I didn't revert the decision to make them uniformly public. Interaction with inlining was part of the reason, but I recall that the change to make the public simplified other parts of the indylamnda implementation, too. I can't recall the details right now, but can dig it up if you're interested.

Would it be possible for MiMa to turn a blind eye to ACC_SYNTHETIC members?

scala> class C { () => () }
defined class C

scala> classOf[C].getDeclaredMethods.last
res10: java.lang.reflect.Method = public static final void C.$line15$$read$C$$$anonfun$1()

scala> .isSynthetic
res11: Boolean = true

Note that wouldn't exclude Flags.SYNTHETIC members, like case class boilerplate.

scala> case class CC()
defined class CC

scala> classOf[CC].getDeclaredMethod("toString").isSynthetic
res12: Boolean = false

scala> typeOf[CC].member(TermName("toString"))
res15: $r.intp.global.Symbol = method toString

scala> (res15.isSynthetic, res15.isArtifact)
res16: (Boolean, Boolean) = (true,false)

Bytecode ACC_SYNTHETIC, meaning: "invisible to Java source", corresponds to Scala's Flag.ARTIFACT.

@retronym
Copy link
Contributor

the naming isn't stable (the $1, $2 numbering), but that's not the real problem [...]

That's actually an area that we ought to improve on a little. Tracking as scala/scala-dev#43

@dotta
Copy link
Contributor

dotta commented Aug 28, 2015

@retronym I can't find where it is said in the JVM spec that members with the ACC_SYNTHETIC flag are "invisible to Java sources". The only thing I found in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.5-200-A.1 is:

ACC_SYNTHETIC 0x1000 Declared synthetic; not present in the source code.

But that doesn't clarify its visibility.

If ACC_SYNTHETIC members are indeed guaranteed to be accessible only at compile time, then it would be OK to ignore them in MiMa.

@dotta
Copy link
Contributor

dotta commented Aug 28, 2015

@SethTisue Thanks for the example!

@retronym
Copy link
Contributor

ACC_SYNTHETIC is used for things like the static accessor methods that Java generates to expose private methods to in nested classes.

Java doesn't do any boilerplate method generation (like Scala case classes), so "not present in the source code" ends up meaning the same as "not callable from source code".

@dotta
Copy link
Contributor

dotta commented Aug 28, 2015

@retronym Great, then I think we are good. Ignoring ACC_SYNTHETIC members and classes in MiMa seems to be the right thing to do.

@SethTisue
Copy link
Collaborator Author

sounds good to me, but maybe do a test run over some sample jars, a line of debugging output every time it ignores an ACC_SYNTHETIC, just so you can see whether unwanted fish are getting caught in the net

@dotta
Copy link
Contributor

dotta commented Aug 28, 2015

@SethTisue I was under the impression one of you would do the work ;-)

@SethTisue
Copy link
Collaborator Author

heh. OK, yes, I will do it.

@SethTisue
Copy link
Collaborator Author

@dotta I reviewed and merged #92. what's the best way to get the fix to @sjrd for testing in a Scala.js context? roll a release? publish a SNAPSHOT?

@dotta
Copy link
Contributor

dotta commented Nov 12, 2015

@SethTisue Feel free to roll out a release (version numbers are cheap).

@sjrd
Copy link
Contributor

sjrd commented Nov 12, 2015

I publishLocal'ed master and tested locally. I can confirm that it solves the issue we were experiencing in Scala.js :-) Thanks a lot!

Looking forward to a release so we can re-enable MiMa checking in our build for the 2.12 artifacts.

sjrd added a commit to sjrd/scala-js that referenced this issue Nov 15, 2015
MiMa 0.1.8 fixes the issue
lightbend-labs/mima#78
which was responsible for the high level of noise in 2.12
artifacts.
sjrd added a commit to sjrd/scala-js that referenced this issue Jan 25, 2016
MiMa 0.1.8 fixes the issue
lightbend-labs/mima#78
which was responsible for the high level of noise in 2.12
artifacts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants