SAM Functions from scala-java8-compat [ci: last-only] #4594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
locally i get this alarming outcome: |
bytecode: The mistake seems to be that the Scala compiler incorrectly selects Since we're doing mixed compilation, this is almost certainly a problem of the Java parser. I'll take a look. |
|
I think this is my bug in the backend. Try this: diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
index 40ba0c0..06a7027 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala
@@ -1299,7 +1299,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
val invokedType = asm.Type.getMethodDescriptor(asmType(functionalInterface), (receiver ::: capturedParams).map(sym => toTypeKind(sym.info).toASMType): _*)
val constrainedType = new MethodBType(lambdaParams.map(p => toTypeKind(p.tpe)), toTypeKind(lambdaTarget.tpe.resultType)).toASMType
- val sam = functionalInterface.info.decls.find(_.isDeferred).getOrElse(functionalInterface.info.member(nme.apply))
+ val sam = functionalInterface.info.decls.find(_.isDeferredNotDefault).getOrElse(functionalInterface.info.member(nme.apply))
val samName = sam.name.toString
val samMethodType = asmMethodType(sam).toASMType |
|
Thanks for submitting this PR, BTW. I tried something similar but wasn't able to convince |
|
ok, trying now. still, it's probably a bug in JavaParser: the symbol flags should be the same when reading from source or from classfile. |
|
thanks for the ScalacFork link, i have to check what it chooses to recompile if a subset of the sources is modified. |
|
My theory was that the order of the decls was different in joint vs separate compilation, and that the |
|
The java parser sets |
|
pushed a fix to the java parser - note that the commits show in the wrong order on GitHub, the correct order is |
|
I have a slight preference to hide |
|
|
yes, i'm looking into this - any intuitive ideas? |
|
The latest is 2.4.1. We're using 1.50.0. |
|
yeah, the issue you link mentions we need 2.4, i'll give it a try. |
|
This PR tentatively LGTM. I won't be around much to continue review for the next few days, but I think if you get through the Ant/SBT pain this ought to go in. |
|
|
ok, thanks for your help! |
|
i'm running out of ideas for the bnd issue (see above), maybe @adriaanm knows more? scala-library.jar doesn't have any classfiles in the empty package. can it be due to other files, e.g., |
lrytz
force-pushed
the
jfun
branch
2 times, most recently
from
530d65e
to
a01ea0c
Compare
lrytz
changed the title
|
From a clean (non-optimized) build, osgi.core succeeds for me. I did ant pack.done && ant osgi.done |
|
osgi.done fails, though, since Hacky fix: diff --git i/build-ant-macros.xml w/build-ant-macros.xml
index 795779193a..b090682fcc 100644
--- i/build-ant-macros.xml
+++ w/build-ant-macros.xml
@@ -488,7 +488,7 @@
<filter token="SCALA_COMPILER_INTERACTIVE_VERSION" value="${scala-compiler-interactive.version.number}"/>
<filter token="XML_VERSION" value="${scala-xml.version.number}" />
<filter token="PARSER_COMBINATORS_VERSION" value="${scala-parser-combinators.version.number}" />
- <filter token="SCALA_SWING_VERSION" value="${scala-swing.version.number}" />
+ <filter token="SCALA_SWING_VERSION" value="${scala-swing.version.osgi}" />
</filterset>
</copy>
<bnd classpath="${@{project}.jar}" eclipse="false" failok="false" exceptions="true" files="${build-osgi.dir}/${@{project}.name}.bnd" output="${build-osgi.dir}"/>
diff --git i/versions.properties w/versions.properties
index 5fd2ac4b71..ad3d659aee 100644
--- i/versions.properties
+++ w/versions.properties
@@ -25,6 +25,7 @@ scala.binary.version=2.12.0-M1
scala-xml.version.number=1.0.4
scala-parser-combinators.version.number=1.0.4
scala-swing.version.number=2.0.0-M2
+scala-swing.version.osgi=2.0.0.M2
jline.version=2.12.1
scala-asm.version=5.0.4-scala-2
|
This reverts commit e1895d6.
The JFunction classes depend on the FunctionN traits, so the java compiler needs the Scala library on the classpath. At the same time, while compiling the Scala library, the symbols for JFunction classes need to be available to emit indy-lambda closures. Therefore we pass the JFunctions as Java sources while compiling the Scala library.
Default methods (and also static methods) in interfaces should not get the `DEFERRED` flag (they don't have it in bytecode). Also tightens the Java parser for abstract methods to disallow a method body.
Used revision: scala/scala-java8-compat@9253ed9 moved files to package scala.runtime.java8
The original file is here: https://github.com/scala/scala-java8-compat/blob/master/src/main/java/scala/compat/java8/runtime/LambdaDeserializer.scala The only difference is the package name (changed to scala.runtime). This commit is a cherry-pick of retronym's c0732e6
Bnd 1.50 doesn't work with Java 8 classfiles, so upgrade to 2.4.1. Also upgrade all other tools to make tests pass. It seems that the new version of bnd only copies classfiles from the original into the resulting jar, while the old version also copied all other files. This is fixed by adding Include-Resource: @@SOURCE_JARNAME@ This makes bnd copy all resources from the source jar. I ran the following on the osgi artifacts of this branch, and on 2.11.x: for f in `find build/osgi -name '*.jar' -a -not -name '*src.jar'`; do unzip -l $f | grep -v '\.class' ; done Comparing the two file lists, things look OK: https://gist.github.com/lrytz/be08db051a53eded192d For `org.eclipse.osgi` we moved to the group ID `org.eclipse.tycho`, where there's a newer version available. The osgi tests would fail with the most recent version available in the `org.eclipse.osgi` group ID. Sets the required java version in bnd files (JavaSE-1.8). Introduces scala-swing.version.osgi - the version.number is not a valid osgi version.
It tests the serialVersionUID field on closure classes. The field doesn't exist for indyLambda closures. See https://issues.scala-lang.org/browse/SI-9373
They fail at runtime in GenBCode since scala is built with indyLambda enabled: java.lang.AssertionError: assertion failed: Bad superClass for trait JFunction1: class Any at scala.tools.nsc.Global.assert(Global.scala:261) at scala.tools.nsc.backend.jvm.BTypesFromSymbols.setClassInfo(BTypesFromSymbols.scala:228) Noted in https://issues.scala-lang.org/browse/SI-9374
The old inliner fails more often when the library is built with indylambda. Noted in https://issues.scala-lang.org/browse/SI-9374. Example: List.foreach ➜ sandbox git:(jfun) ✗ qs -Ybackend:GenASM -optimize -Yinline-warnings Welcome to Scala version 2.12.0-20150630-220939-1cb032d806 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_45). Type in expressions to have them evaluated. Type :help for more information. scala> List(1,2,3).foreach(x => x + 1) <console>:11: warning: Could not inline required method foreach because bytecode unavailable. List(1,2,3).foreach(x => x + 1) ^ <console>:11: warning: At the end of the day, could not inline @inline-marked method foreach List(1,2,3).foreach(x => x + 1) ^
The delambdafyLambdaClassNames tests was removed, there's nothing to tests with indyLambda.
|
it builds green. cleaned up the commits, so running the test again. i think we should keep this in [ci: last-only], because for the build to be green we'd have to squash everything, which would be a huge commit of many things. review by @adriaanm. |
|
btw i marked these two as blockers for M2, but we can change this: |
|
Sounds good to me. Let's give ourselves the rest of the week to polish M2. Does that sound like enough time? Stage on Monday/Tuesday? |
yes, but what i found gives you the full path to the jar, while we really need the filename only.
yes! |
|
I think we should squash this into, say, four commits:
|
|
Happy to do the rebasing this morning. |
Merged
This was referenced Apr 7, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.