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

Final methods in traits are not compiled as final #11485

Closed
bbonanno opened this Issue Apr 13, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@bbonanno
Copy link

commented Apr 13, 2019

In 2.13.0-RC1, if we write this simple code

import java.lang.reflect.Modifier.isFinal

trait HaveFinalMethod {
  def normalMethod: String = "normal"
  final def finalMethod: String = "final"
}

class Child extends HaveFinalMethod

object Main {
  def main(args: Array[String]): Unit = {
    println(isFinal(classOf[HaveFinalMethod].getMethod("normalMethod").getModifiers)) //false
    println(isFinal(classOf[HaveFinalMethod].getMethod("finalMethod").getModifiers)) //false but should be true!

    println(isFinal(classOf[Child].getMethod("normalMethod").getModifiers)) //false
    println(isFinal(classOf[Child].getMethod("finalMethod").getModifiers))  //true (as it should be)
  }
}

Then we'll see how finalMethod in HaveFinalMethod is not actually marked as final, which, on top of being inconsistent with what happens when we test the same on Child, causes issues with frameworks that use reflection.

if we make Child a trait instead of a class, then the problem is also present when inspecting finalMethod.

trait HaveFinalMethod {
  def normalMethod: String = "normal"
  final def finalMethod: String = "final"
}

trait Child extends HaveFinalMethod

object Main {
  def main(args: Array[String]): Unit = {
    println(isFinal(classOf[HaveFinalMethod].getMethod("normalMethod").getModifiers)) //false
    println(isFinal(classOf[HaveFinalMethod].getMethod("finalMethod").getModifiers)) //false but should be true!

    println(isFinal(classOf[Child].getMethod("normalMethod").getModifiers)) //false
    println(isFinal(classOf[Child].getMethod("finalMethod").getModifiers))  //false but should be true!
  }
}
@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

You can't legitimately have final interface methods, can you?

JVMS 4.6:

Methods of interfaces may have any of the flags in Table 4.6-A set except ACC_PROTECTED, ACC_FINAL, ACC_SYNCHRONIZED, and ACC_NATIVE (JLS §9.4).

Do we do this in 2.12? It seems like that would be a bug.

@bbonanno

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

@hrhino You're right, I thought my problem was related to this, but it must be a different one as 2.12 does the same.

So, what happens is that while trying to create a mock, in this example for the Child class, I get the following error

Caused by: java.lang.IllegalStateException: Error invoking java.lang.ClassLoader#defineClass
...
Caused by: java.lang.VerifyError: class org.mockito.Child$MockitoMock$488775342 overrides final method finalMethod.()Ljava/lang/String;
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
...

Now, the exact same code runs smoothly in 2.12.

Given that the exception was complaining about the dynamically generated class org.mockito.Child$MockitoMock$488775342 overriding the final method, I thought it was related to the lack of said flag (and thus bytebuddy not filtering out the final methods when creating said class), but now that you pointed that out and that I've verified that the code seems to use the flags in the same way in 2.12 I'm now out of ideas...

@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I'm on my phone, but: does 2.12 emit finalMethod in Child (the class version) as final? If not, we've probably changed in copying that flag when mixing in the trait method. I'll have a look when I get to a computer.

I'm not sure which behavior is arguably more correct, but IMO we should do what 2.12 does unless this was an intentional change.

@bbonanno

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

Yes, 2.12 emits finalMethod as final in Child, so the behaviour from the flags point of view seems to be the same. It was my bad not to compare them before raising the bug.

The thing is, for some reason, I can create a mock of Child in 2.12, but in 2.13 I get the error I posted above, so maybe something else in the bytecode is different?

@bbonanno

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

@hrhino I think I found the problem
After hours of debugging I first noticed that the modifiers for finalMethod in Child are different, i.e.

2.12 = 17
2.13 = 4177

Now, the ByteBuddy framework at some point checks if the method in the Child class is a bridge, for which it does something like

 ACC_BRIDGE = 64
 (method.getModifiers() & ACC_BRIDGE) == ACC_BRIDGE

That comparison yields false for 2.12 and true for 2.13, and so a complete different set of things happen that end up in my error.

Now, I'm not sure what the correct behaviour should be here, as this framework uses that comparison to decide if it should use the finalMethod defined in HaveFinalMethod or the one defined in Child for the newly created type in runtime, but that's the difference between 2.12 and 2.13 that breaks my code.

I've found a similar bug recently closed (although I'm not sure it's exactly the same) #7917

Many thanks for your time

@smarter

This comment has been minimized.

Copy link

commented Apr 14, 2019

That's an intentional change: scala/scala#7843.

Now, I'm not sure what the correct behaviour should be here, as this framework uses that comparison to decide if it should use the finalMethod defined in HaveFinalMethod or the one defined in Child for the newly created type in runtime

Since the method in Child just forwards to the one in HaveFinalMethod that should be perfectly fine.

@bbonanno

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Do you mean is correct to mark the method in Child as ‘bridge’?
Remember that what this framework is trying to decide is if it it should override the behaviour of finalMethod or not, and for that it uses the bridge flag, and hence, with the new set of flags the framework tries to override the final method which ends up in this verification error.

@smarter

This comment has been minimized.

Copy link

commented Apr 14, 2019

Yeah I guess we shouldn't emit bridges as final, I'll make a PR.

smarter added a commit to smarter/scala that referenced this issue Apr 14, 2019

Don't emit mixin forwarders as final
They're emitted as bridge since scala#7843 and a final bridge is a weird
concept that never comes up in Java (in particular, according to
scala/bug#11485 ByteBuddy assumes that it can always override bridges).
So let's just drop the "final" part which is not essential for interop.

Fixes scala/bug#11485.

smarter added a commit to smarter/scala that referenced this issue Apr 14, 2019

Don't emit mixin forwarders as final
They're emitted as bridge since scala#7843 and a final bridge is a weird
concept that never comes up in Java (in particular, according to
scala/bug#11485 ByteBuddy assumes that it can always override bridges).
So let's just drop the "final" part which is not essential for interop.

Fixes scala/bug#11485.

Note: the final flag cannot be dropped before the backend because the
inliner relies on this flag being present to know what can be inlined.

smarter added a commit to smarter/scala that referenced this issue Apr 15, 2019

Don't emit mixin forwarders as final
They're emitted as bridge since scala#7843 and a final bridge is a weird
concept that never comes up in Java (in particular, according to
scala/bug#11485 ByteBuddy assumes that it can always override bridges).
So let's just drop the "final" part which is not essential for interop.

Fixes scala/bug#11485.

Note: the final flag cannot be dropped before the backend because the
inliner relies on this flag being present to know what can be inlined.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.