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

Add Scala 2.12 support #1877

Merged
merged 66 commits into from Oct 7, 2020
Merged

Conversation

errikos
Copy link
Member

@errikos errikos commented Aug 17, 2020

Summary

Add Scala 2.12 support for Scala Native. You can consult the Scala 2.12 changelog here. This PR takes into account all the changes and adapts them for Scala Native.

Lambdas

In order to keep the NIR changes to a minimum, we continue generating a FunctionN class with an apply method for each lambda. All the lambda captures are stored inside the class instance and initialised via its constructor. The apply method calls the generated $anonfun method, also extracting and forwarding the captures.

NIR compatibility

Scala 2.12 now compiles trait methods as default methods in Java interfaces. This means that the NIR compiled with 2.12 is not compatible with that compiled with 2.11.

Scala 2.11 cross-compilation

In order to keep the project compilable with Scala 2.11, a scala.scalanative.nscplugin.NirCompat component is introduced in nscplugin. This allows the compiler plugin to function with both Scala 2.11 and 2.12.

In addition, some tests need to know under which Scala version they are running. We use sbt-buildinfo to provide Scala version information to the projects that need them (mainly for unit-tests and tools tests).

@sjrd
Copy link
Collaborator

sjrd commented Aug 17, 2020

For the issue with Arrays, I think the underlying issue is that you're not handling value classes correctly in genFunction. The problem is that you need to recover value classes in parameters and result types, and box/unbox them appropriately. You'll have to replicate the uses of enteringPhase(currentRun.posterasurePhase) { ... } and handling of ErasedValueTypes that we do in Scala.js. Some pointers:

sandbox/Test.scala Outdated Show resolved Hide resolved
@@ -0,0 +1,77 @@
/*
Copy link
Member

@ekrich ekrich Aug 17, 2020

Choose a reason for hiding this comment

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

Are the overrides just standard for 2.12 even in the Scala repo?

Copy link
Member Author

@errikos errikos Aug 18, 2020

Choose a reason for hiding this comment

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

Hmm, what do you mean?

Copy link
Member

@ekrich ekrich Aug 18, 2020

Choose a reason for hiding this comment

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

I am just asking a general question about why these are here. They are not changed for Scala Native are they?

Copy link
Member Author

@errikos errikos Aug 18, 2020

Choose a reason for hiding this comment

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

Oh yeah, we leave them as is. Basically, where applicable, I applied the changelog from the overrides-2.11 to the 2.12 files.

@errikos errikos force-pushed the topic/2.12.x-support branch 2 times, most recently from 3d1e5ce to 237e427 Compare Aug 19, 2020
Copy link
Collaborator

@sjrd sjrd left a comment

This is a first review. I still need to really understand what's happening now in the linker for default methods.

.travis.yml Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
unit-tests/src/test/scala/java/util/ArrayDequeSuite.scala Outdated Show resolved Hide resolved
unit-tests/src/test/scala/scala/AsInstanceOfSuite.scala Outdated Show resolved Hide resolved
unit-tests/src/test/scala/tests/SuiteSuite.scala Outdated Show resolved Hide resolved
util/src/main/scala/scala/scalanative/util/Platform.scala Outdated Show resolved Hide resolved
Copy link
Member Author

@errikos errikos left a comment

The changes in commit 9f7d2fc were introduced because IntelliJ IDEA treats inline as a keyword, thus the whole files are red and impossible to work with.

final val compat: Int = 4 // a.k.a. MAJOR version
final val revision: Int = 7 // a.k.a. MINOR version
final val compat: Int = 5 // a.k.a. MAJOR version
final val revision: Int = 8 // a.k.a. MINOR version
Copy link
Member Author

@errikos errikos Aug 22, 2020

Choose a reason for hiding this comment

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

We had increased the revision when we introduced reflection. We did not have a release in between, but I figured I would increase it again with compat, since this is a separate PR (i.e. commit hash).

Copy link
Contributor

@lolgab lolgab Aug 22, 2020

Choose a reason for hiding this comment

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

Shouldn't be 5.0 instead of 5.8 ?
How does semantic versioning work here?

Copy link
Collaborator

@sjrd sjrd Aug 22, 2020

Choose a reason for hiding this comment

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

It should definitely be 5.0 if we are actually breaking backward binary compat. But if we are doing so, we must make that into a separate commit (hence PR) that also removes all the deserialization hacks that were introduced so far.

Copy link
Member Author

@errikos errikos Aug 22, 2020

Choose a reason for hiding this comment

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

Hmm, okay. So, are we changing the increment behaviour described in the docstring above?

Copy link
Collaborator

@sjrd sjrd Oct 1, 2020

Choose a reason for hiding this comment

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

Let's keep the system in place for now, hence leave 5.8.

@errikos errikos requested a review from sjrd Sep 14, 2020
@ishubham-k
Copy link

ishubham-k commented Sep 22, 2020

Hi. I was wondering if this pr is been under active review. Is there anything pending that community can take a stab at ?

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Sep 22, 2020

@ikamthania Hey, it's still under review. I hope we can merge it (as well as other remaining 0.4.0 blockers) and make a release by end of this month

@ishubham-k
Copy link

ishubham-k commented Sep 22, 2020

@ikamthania Hey, it's still under review. I hope we can merge it (as well as other remaining 0.4.0 blockers) and make a release by end of this month

Thanks for update. No issues. These things take time understandably.

Copy link
Collaborator

@sjrd sjrd left a comment

I finally dedicated enough continuous time to properly review this.

This looks very good. I only minor comments here and there. I'm looking forward to merging it :)

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@@ -86,7 +86,7 @@ trait Opt { self: Interflow =>
val retty = rets match {
case Seq() => Type.Nothing
case Seq(ty) => ty
case tys => Sub.lub(tys)
case tys => Sub.lub(tys, Type.Ref(Global.Top("java.lang.Object")))
Copy link
Collaborator

@sjrd sjrd Oct 1, 2020

Choose a reason for hiding this comment

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

This is not resolve. We need to pass what used to be origRetty as the second argument to Sub.lub on this line (moving the computation of origRetty above val retty = ....

Suggested change
case tys => Sub.lub(tys, Type.Ref(Global.Top("java.lang.Object")))
case tys => Sub.lub(tys, origRetty)

Then we can remove resRetty and use retty on the last line of the method instead.


The current changes are sound, but they removed the type specialization entirely, instead of taking advantage of what Sub.lub can do for us.

tools/src/main/scala/scala/scalanative/linker/Reach.scala Outdated Show resolved Hide resolved
@sjrd
Copy link
Collaborator

sjrd commented Oct 2, 2020

This will also need to be rebased on master to sit on top of #1912.

@errikos
Copy link
Member Author

errikos commented Oct 2, 2020

There seems to be a test failing in tools due to recent changes, I will look into it.

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Oct 2, 2020

There seems to be a test failing in tools due to recent changes, I will look into it.

From what I can see there is problem with mangled name of closure, exactly it's order of it's arguments:
expected: D6loop$1iL16java.lang.StringzL16java.lang.StringEPT6xyz.B$
actual: D6loop$1zL16java.lang.StringiL16java.lang.StringEPT6xyz.B$

And here's actual method:

package xyz
object B extends A {
def encodeLoop(arg1: Int, arg2: String): String = {
  println("init_logic")
  val bool: Boolean = false
  def loop(): String = {
    if (bool) "asd" else arg2 * arg1
  }
  loop()
}
}

I'm sure that's not a problem with our mangling logic (cause it passes other tests)
I think that order in which Scala compiler creates mangled names for closures have changed in 2.12. Since our signatures/mangled names are based on Scala compiler output we won't be able to change it.

I would suggest to change test definition to expect method signature in both orders on arguments. I can provide patch resolving this issue tomorrow.

@WojciechMazur WojciechMazur mentioned this pull request Oct 5, 2020
2 tasks
@errikos errikos requested a review from sjrd Oct 6, 2020
@errikos
Copy link
Member Author

errikos commented Oct 6, 2020

This should be ready for another pass @sjrd. I think that all the comments are resolved and the CI seems to be green!

import scala.tools.nsc.settings._
ScalaVersion.current match {
case SpecificScalaVersion(2, 11, _, _) =>
HashMethods.foreach(addPrimitive(_, HASH))
Copy link
Member Author

@errikos errikos Oct 6, 2020

Choose a reason for hiding this comment

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

I was not able to use the sbt-buildinfo plugin here to condition for the Scala version, because it resulted to an infinite recursion in sbt when resolving the projects that depend on nscplugin (tried to enable sbt-buildinfo for nscplugin).

I went with nsc.settings.ScalaVersion instead. I don't know if the latter has any version guarantees.

sjrd
sjrd approved these changes Oct 7, 2020
Copy link
Collaborator

@sjrd sjrd left a comment

Ship it!

Thank you @errikos for this huge work!

@sjrd sjrd merged commit 12a305c into scala-native:master Oct 7, 2020
1 check passed
@errikos errikos deleted the topic/2.12.x-support branch Oct 8, 2020
vicopem pushed a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
The Scala 2.12 changelog can be found at
https://www.scala-lang.org/news/2.12.0/
This PR takes into account all the changes and adapts them for
Scala Native.

---

Lambdas

In order to keep the NIR changes to a minimum, we continue
generating a `FunctionN` class with an `apply` method for each
lambda. All the lambda captures are stored inside the class
instance and initialised via its constructor. The `apply` method
calls the generated `$anonfun` method, also extracting and
forwarding the captures.

---

NIR changes

Scala 2.12 now compiles trait methods as default methods in Java
interfaces. This means that the NIR was enhanced to support default
methods. The biggest impact of that is found in the reachability
analysis (`Reach.scala`).

---

Scala 2.11 cross-compilation

In order to keep the project compilable with Scala 2.11, a
`scala.scalanative.nscplugin.NirCompat` component is introduced in
`nscplugin`. This allows the compiler plugin to function with both
Scala 2.11 and 2.12.

In addition, some tests need to know under which Scala version they
are running. We use `sbt-buildinfo` to provide Scala version
information to the projects that need them (mainly for `unit-tests`
and `tools` tests).

---

Co-authored-by: Denys Shabalin <denys.shabalin@epfl.ch>
vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
vicopem added a commit to vicopem/scala-native that referenced this pull request Nov 19, 2020
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
The Scala 2.12 changelog can be found at
https://www.scala-lang.org/news/2.12.0/
This PR takes into account all the changes and adapts them for
Scala Native.

---

Lambdas

In order to keep the NIR changes to a minimum, we continue
generating a `FunctionN` class with an `apply` method for each
lambda. All the lambda captures are stored inside the class
instance and initialised via its constructor. The `apply` method
calls the generated `$anonfun` method, also extracting and
forwarding the captures.

---

NIR changes

Scala 2.12 now compiles trait methods as default methods in Java
interfaces. This means that the NIR was enhanced to support default
methods. The biggest impact of that is found in the reachability
analysis (`Reach.scala`).

---

Scala 2.11 cross-compilation

In order to keep the project compilable with Scala 2.11, a
`scala.scalanative.nscplugin.NirCompat` component is introduced in
`nscplugin`. This allows the compiler plugin to function with both
Scala 2.11 and 2.12.

In addition, some tests need to know under which Scala version they
are running. We use `sbt-buildinfo` to provide Scala version
information to the projects that need them (mainly for `unit-tests`
and `tools` tests).

---

Co-authored-by: Denys Shabalin <denys.shabalin@epfl.ch>
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Aug 25, 2021
The Scala 2.12 changelog can be found at
https://www.scala-lang.org/news/2.12.0/
This PR takes into account all the changes and adapts them for
Scala Native.

---

Lambdas

In order to keep the NIR changes to a minimum, we continue
generating a `FunctionN` class with an `apply` method for each
lambda. All the lambda captures are stored inside the class
instance and initialised via its constructor. The `apply` method
calls the generated `$anonfun` method, also extracting and
forwarding the captures.

---

NIR changes

Scala 2.12 now compiles trait methods as default methods in Java
interfaces. This means that the NIR was enhanced to support default
methods. The biggest impact of that is found in the reachability
analysis (`Reach.scala`).

---

Scala 2.11 cross-compilation

In order to keep the project compilable with Scala 2.11, a
`scala.scalanative.nscplugin.NirCompat` component is introduced in
`nscplugin`. This allows the compiler plugin to function with both
Scala 2.11 and 2.12.

In addition, some tests need to know under which Scala version they
are running. We use `sbt-buildinfo` to provide Scala version
information to the projects that need them (mainly for `unit-tests`
and `tools` tests).

---

Co-authored-by: Denys Shabalin <denys.shabalin@epfl.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants