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

Fix #15199: Exclude JavaDefined Modules from bridge generation. #15499

Merged

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Jun 22, 2022

JavaDefined modules are imaginary symbols that dotc uses to refer to static methods of Java classes. They have no reification, and therefore it makes no sense for them to participate in bridge generation.

How I fixed it

First, I happen to know that bridges are generated in Erasure, so that's one thing to look at.

I looked at the JavaDoc of javax.swing.plaf.metal.MetalTabbedPaneUI to find out what TabbedPaneLayout. I discovered that it is an inner class, and that it shadows an inner class of the same name in basic.BasicTabbedPaneUI. Java inner classes should never have bridges, as they are types. But they also implicitly declare companion objects, which dotc uses to refer to static members.

Apparently, dotc gets confused about those imaginary objects, and tries to create bridges for them. It shouldn't even consider them.

I reproduced the issue with my own Java-defined classes, to get rid of the dependency on the JDK Swing APIs, which is a beast.

I search for "Bridge" in Erasure.scala, which leads me to the class Bridges. I did not know that class, but it's not very big. A quick look at BridgesCursor and its relationship to OverridingPairs.Cursor (the latter is the thing that enumerates pairs of members where one overrides the other) tells me that I should exclude JavaDefined modules from BridgesCursor. Lucky me: there is a def exclude in it, so I amend it to also exclude sym.isAllOf(Module | JavaDefined).

sjrd added 2 commits June 22, 2022 13:40
JavaDefined modules are imaginary symbols that dotc uses to refer
to static methods of Java classes. They have no reification, and
therefore it makes no sense for them to participate in bridge
generation.
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@odersky odersky merged commit c37175b into scala:main Jul 5, 2022
@odersky odersky deleted the spurious-bridge-for-java-inner-class branch July 5, 2022 14:41
@odersky
Copy link
Contributor

odersky commented Jul 5, 2022

@sjrd Merging this PR failed in https://github.com/lampepfl/dotty/runs/7198532616?check_suite_focus=true
with

[info] Test run dotty.tools.dotc.ScalaJSCompilationTests started
[info] Test dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS started
Referring to non-existent method java.lang.Class.getDeclaredMethods()[java.lang.reflect.Method
called from Test_2$package$.Test()void
called from static Test.main([java.lang.String)void
called from core module module initializers
involving instantiated classes:
Test_2$package$
Referring to non-existent class java.lang.reflect.Method
called from Test_2$package$.Test()void
called from static Test.main([java.lang.String)void
called from core module module initializers
involving instantiated classes:
Test_2$package$
Referring to non-existent class Child_1
called from ScalaChild
called from Test_2$package$.Test()void
called from static Test.main([java.lang.String)void
called from core module module initializers
involving instantiated classes:
Test_2$package$
Test 'tests/run/i15199' failed with output:
org.scalajs.linker.interface.LinkingException: There were linking errors
at org.scalajs.linker.frontend.BaseLinker.reportErrors$1(BaseLinker.scala:91)
at org.scalajs.linker.frontend.BaseLinker.$anonfun$analyze$5(BaseLinker.scala:100)
at scala.concurrent.impl.Promise$Transformation.run(Promise.scala:467)
at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1434)
at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:295)
at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)
Error: Test dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS failed: java.lang.AssertionError: Run test failed, but should not, reasons:
Error: encountered 1 test failure(s):
Error: - generic failure (see test output), took 236.373 sec
Error: at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkRuns(ParallelTesting.scala:1056)
Error: at dotty.tools.dotc.ScalaJSCompilationTests.runScalaJS(ScalaJSCompilationTests.scala:61)
Error: at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error: at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
Error: at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Error: at java.lang.reflect.Method.invoke(Method.java:567)
Error: ...

Can you take a look?

@sjrd
Copy link
Member Author

sjrd commented Jul 5, 2022

Ah yes of course: #15590

@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants