Fix #4404: In LambdaLift, modify flags properly #4804
Conversation
| @@ -338,11 +338,16 @@ object LambdaLift { | |||
| else (topClass, JavaStatic) | |||
| } | |||
| else (lOwner, EmptyFlags) | |||
| // drop Module because class is no longer a singleton in the lifted context. | |||
| val nFlags = local.flags &~ Module | Lifted | maybeStatic | |||
| // keep Public when it is a local class | |||
There was a problem hiding this comment.
Why do local classes need to be public and classes lifted into traits non-final?
There was a problem hiding this comment.
In the issue #4404, @mbloms said:
Anonymous classes are compiled without the
public final.
Therefore, I tried to keep anonymous classes as public after lifting. However, I did not consider non-anonymous local classes. I think non-anonymous local classes should not be public. I'll correct the code to distinguish anonymous and non-anonymous classes.
At the first, I changed the code simply to keep final always. Then, test t4565_1 failed. It was compiled but during the execution, there was an exception:
Exception in thread "main" java.lang.ClassFormatError: Method A$1 in class T has illegal modifiers: 0x1A
According to the JVM specification, a method declared inside interface cannot have the final flag, so that I thought that final should be dropped when a method is lifted into a trait.
There was a problem hiding this comment.
I think leaving the final flag for anonymous/local classes makes sense, but they shouldn't be public: they should never be called by external code.
According to the JVM specification, a method declared inside interface cannot have the final flag,
I see! Can you add this information as a comment in the code? (With a link to the relevant part of the spec)
|
Otherwise, this looks great but is missing tests. You could either add a file in |
|
I changed the code to add the |
|
@Medowhill Can you rebase the PR on master and remove the merge commits? |
|
I removed the merge commits. |
| // According to the JVM specification, a method declared inside interface cannot have the final flag. | ||
| // "Methods of interfaces may have any of the flags in Table 4.6-A set except ACC_PROTECTED, ACC_FINAL, ..." | ||
| // (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.6) | ||
| val initFlags = if (!local.isClass && (newOwner.flags is Trait)) preInitFlags &~ Final else preInitFlags |
There was a problem hiding this comment.
Why don't you test local.is(Method) instead of !local.isClass?
There was a problem hiding this comment.
Personal taste: I think it is clearer to use a var rather than coming up with new unique names. Ie.
var initFlags = ...
if (...) initFlags = initFlags &~ FinalThere was a problem hiding this comment.
I think your suggestion is better than my original code. I'll fix it. Thank you!
tests/run/i4404c.scala
Outdated
| } | ||
| } | ||
|
|
||
| object Test { |
There was a problem hiding this comment.
Can you explain what you are testing here
There was a problem hiding this comment.
trait T {
def f = {
object O
}
}
When we compile the above code, the compiler will generate final lazy <accessor> module def O(): O$ inside method f. I tried to check whether the final flag of method O is dropped correctly during the LambdaLift phase through the test.
There was a problem hiding this comment.
If you want to test that methods lose their final modifier, I would replace the object by a normal def. I think it is clearer than using an object relying on the fact that it desugars to a lazy val which itself desugars to a method
There was a problem hiding this comment.
That would be clearer. I'll change it. Thank you!
tests/run/i4404c.scala
Outdated
|
|
||
| trait T { | ||
| def f = { | ||
| final def f0 = () |
There was a problem hiding this comment.
Can you remove the final modifier since I believe being able to write it in the first place is a bug?
There was a problem hiding this comment.
Then, do I need to change the test to use object inside a method?
There was a problem hiding this comment.
No I think the test is fine as is. We want to make sure that we don't add the final modifier in this case
|
Thanks! 🎉 |
No description provided.