Dotty syntax compat #28837

ohze · 2020-03-28T15:45:08Z

References #28835

This changeset should be full (binary & source, backward & forward) compatible for scala 2.
Pls see change & message in each commit for more detail.

Due to some bugs in dotty, I add some hacking changes (beside the change in this PR) to a different branch & build that branch in dotty in travis:

akka-ci · 2020-03-28T15:46:22Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

He-Pin · 2020-03-28T15:57:53Z

Very cool if we can have Akka release for dotty and add Akka to dotty's community builds.

akka-ci · 2020-03-28T15:58:51Z

Test FAILed.

He-Pin · 2020-03-28T16:09:44Z

akka-actor-testkit-typed/src/main/scala/akka/actor/testkit/typed/CapturedLogEvent.scala

@@ -28,35 +28,35 @@ final case class CapturedLogEvent(level: Level, message: String, cause: Option[T
      message: String,
      errorCause: Optional[Throwable],
      marker: Optional[Marker],
-      mdc: java.util.Map[String, Any]) {
+      mdc: java.util.Map[String, Any]) = {


With scalafix rule?

No. dotty compile errors => I just manually change :)

He-Pin · 2020-03-28T16:20:10Z

akka-actor/src/main/scala/akka/io/Tcp.scala

@@ -106,7 +106,7 @@ object Tcp extends ExtensionId[TcpExt] with ExtensionIdProvider {
   * This is the common trait for all commands understood by TCP actors.
   */
  trait Command extends Message with SelectionHandler.HasFailureMessage {
-    def failureMessage = CommandFailed(this)
+    def failureMessage: CommandFailed = CommandFailed(this)


with scalfix rule?

I don't think there will be a scalafix rule for this case.
I don't add explicit type def for all public methods. This case cause dotty compile errors => I manually change.

but new code will be added then.

but new code will be added then.

what? sorry I don't understand.

How do you keep Akka building with Dotty, if new contributions are only tested with Scalac and not fixed with Scalafix? (Maybe that’s what @hepin1989 is asking, but it’s a good question in either case). Are you adding Dotty to CI? And would Scalafix help port new contributions to Dotty?

Are you adding Dotty to CI?

Yes. I have already described this in the PR description above. Note this image:

ok. I know we should use scalafix and create rules if one don't exist.

But for this very specific case def failureMessage: CommandFailed = CommandFailed(this), I don't think we can create a rule to fix this (maybe I am wrong :D). And we don't have many code like this to create a rule.

Rules are good for other cases though, ex "constructor procedure syntax", or "don't use auto application"

He-Pin · 2020-03-28T16:26:08Z

It would be better to extract some changes related to the build definition to a dedicated plugin like DottySupportPlugin.

ohze · 2020-03-28T17:03:24Z

OK. I will try that idea

akka-ci · 2020-03-28T17:35:01Z

Test PASSed.

akka-ci · 2020-03-29T15:03:11Z

Test FAILed.

akka-ci · 2020-03-29T15:12:58Z

Test FAILed.

ohze · 2020-03-29T16:28:09Z

It would be better to extract some changes related to the build definition to a dedicated plugin like DottySupportPlugin.

done

He-Pin · 2020-03-30T06:28:31Z

@ohze Will need a scalafmtAll，I always forgot that too.

raboof · 2020-03-30T08:05:44Z

Looks like this needs a copyright header in project/DottySupportPlugin.scala

raboof

Pretty cool! I haven't reviewed in depth, but most of the changes I saw seem to be improvements in any case. Could you explain/document somewhere how to run your dotty branch to test?

raboof · 2020-03-30T08:19:53Z

akka-actor/src/main/scala/akka/japi/JavaAPI.scala

@@ -83,8 +82,6 @@ object Pair {
 *
 * This class is kept for compatibility, but for future API's please prefer [[akka.japi.function.Creator]].
 */
-@silent("@SerialVersionUID has no effect")
-@SerialVersionUID(1L)


While indeed the compiler says this SerialVersionUID 'has no effect', removing it is not strictly speaking binary compatible: mimaReportBinaryIssues reports the static field does disappear. Can we be sure that doesn't cause any problems?

The static field public static final long serialVersionUID = 1L; is ONLY generated by scala 2.12.
scala 2.13 don't generate that field.
I think we be sure that doesn't cause any problems.
See also: scala/scala3#8429

raboof · 2020-03-30T08:22:53Z

akka-actor-typed/src/main/scala/akka/actor/typed/internal/receptionist/LocalReceptionist.scala

@@ -60,12 +96,12 @@ private[akka] object LocalReceptionist extends ReceptionistBehaviorProvider {
   * @param subscriptionsPerActor current subscriptions per subscriber (needed since a subscriber can subscribe to several keys) FIXME is it really needed?
   */
  private final case class State(
-      services: TypedMultiMap[AbstractServiceKey, Service],
+      services: ServiceMultiMap[ServiceF],


Since this is private this change is OK, but our tooling that checks binary compatibility (mima) does flag it for review. Could you add an exclusion (e.g. ProblemFilters.exclude[Problem]("akka.actor.typed.internal.receptionist.LocalReceptionist*? You can run sbt mimaReportBinaryIssues to find more warnings, and CONTRIBUTING.md has more information on how/where to add the filters.

when test AsyncDnsResolverIntegrationSpec creation.warnings() is null and the following check is incorrect: creation.warnings() should be(null).or(have(size(0))) Log on travis: [info] akka.io.dns.AsyncDnsResolverIntegrationSpec *** ABORTED *** (13 seconds, 276 milliseconds) [info] java.lang.NullPointerException: [info] at org.scalatest.enablers.Size$$anon$2.sizeOf(Size.scala:132) [info] at org.scalatest.enablers.Size$$anon$2.sizeOf(Size.scala:132) [info] at org.scalatest.matchers.dsl.HaveWord$$anon$6$$anon$1.apply(HaveWord.scala:85) [info] at org.scalatest.matchers.MatchersHelper$.orMatchersAndApply(MatchersHelper.scala:161) [info] at org.scalatest.matchers.Matcher$$anon$11$$anon$1.apply(Matcher.scala:596) [info] at org.scalatest.matchers.should.Matchers$ShouldMethodHelperClass.shouldMatcher(Matchers.scala:6770) [info] at org.scalatest.matchers.should.Matchers$AnyShouldWrapper.should(Matchers.scala:6834) [info] at akka.io.dns.DockerBindDnsService.atStartup(DockerBindDnsService.scala:80) // line number maybe incorrect

…havior refers to private class PublishSessionMessage

…var` (system : akka.actor.typed.ActorSystem[String]) is not a valid type prefix, since it is not an immutable path

It's seem dotty will think `x: AnyRef` so `x.f` don't existed

akka-ci · 2020-04-09T14:34:23Z

Test FAILed.

For example: Reference to context is ambiguous it is both defined in object TypedActor and inherited subsequently in class TypedActor

akka-ci · 2020-04-09T16:52:25Z

Test FAILed.

octonato · 2020-04-10T13:33:25Z

akka-actor-tests/src/test/scala/akka/actor/DeathWatchSpec.scala

@@ -210,19 +210,19 @@ trait DeathWatchSpec { this: AkkaSpec with ImplicitSender with DefaultTimeout =>
    }

    "only notify when watching" in {
-      val subject = system.actorOf(Props[EmptyActor]())
+      val subject = system.actorOf(Props[EmptyActor])


This change is related to ohze@34d57cd (not included here).

And commented here: #28837 (comment)

where you say:

although that commit is need for dotty

Do you really need to remove ()? I did a quick tests on a tiny dotty project and is does compile fine.

case class Props(cls: Class[_]) object Props { def apply[T: ClassTag](): Props = Props(implicitly[ClassTag[T]].runtimeClass) } val p = Props[String]() println(p.cls)

[info] running Main class java.lang.String

Hmm, I think I got why this is needed.

It's not needed by dotty, but it will be better to remove because then users can't call it as previously Props[EmptyActor].

The problem is that if we remove we make it source incompatible. We can be sure that there are many projects out there using Props[EmptyActor]().

Maybe we are tackling this from the wrong side. The goal of the PR here, if I understand it correctly, is to make the code source compatible with the Dotty compiler. That said, we should not remove () at all. And we should it in the Akka code base in all places that Dotty requires it.

The day that we provide the binaries for Dotty/Scala3, a user willing to move to Dotty/Scala3 will have to add () in all places they omit it before.

octonato · 2020-04-10T13:51:26Z

akka-actor-tests/src/test/scala/akka/actor/SchedulerSpec.scala

+          f.setAccessible(true)
+          f.setLong(this, time)
+        }
+        earlyInit()


It will be good to add a comment here explaining why this. I personally can't immediately see the motivation.

octonato · 2020-04-10T13:58:15Z

akka-actor-tests/src/test/scala/akka/actor/SupervisorHierarchySpec.scala

@@ -64,8 +64,8 @@ object SupervisorHierarchySpec {
  case object Abort
  case object PingOfDeath
  case object PongOfDeath
-  final case class Event(msg: Any, identity: Long) { val time: Long = System.nanoTime }
-  final case class ErrorLog(msg: String, log: Vector[Event])
+  final case class MyEvent(msg: Any, identity: Long) { val time: Long = System.nanoTime }


Why this class is being renamed?

I'm not against it, but if not strictly necessary better to do on another PR.

If don't rename => dotty compile error in all case Event(.. in StressTest because FSM (base trait of StressTest) also has Event member.

octonato · 2020-04-10T14:28:31Z

akka-actor-tests/src/test/scala/akka/actor/DeathWatchSpec.scala

@@ -210,19 +210,19 @@ trait DeathWatchSpec { this: AkkaSpec with ImplicitSender with DefaultTimeout =>
    }

    "only notify when watching" in {
-      val subject = system.actorOf(Props[EmptyActor]())
+      val subject = system.actorOf(Props[EmptyActor])


Hmm, I think I got why this is needed.

It's not needed by dotty, but it will be better to remove because then users can't call it as previously Props[EmptyActor].

The problem is that if we remove we make it source incompatible. We can be sure that there are many projects out there using Props[EmptyActor]().

octonato

Thanks @ohze for all that work.

I didn't finish all the files yet. I will continue next week.
I think we should cut this PR in a few smaller PRs. Making smaller PRs will also help review and push it forward.

There are a lot of changes that are binary and source compatible. Like, explicit type annotation, explicit usage of (). Those we can merge earlier and we should also have some lint rules to avoid introducing then again on new PRs.

Then we can have a deprecation cycle for the binary-compatible changes that impact users.

And then we also need to think about the most intrusive source-incompatible changes, like abstract type projection.

octonato · 2020-04-10T15:14:56Z

akka-actor-tests/src/test/scala/akka/event/EventBusSpec.scala

+    Subscriber,
+    BusType <: EventBusSpec.EventBusAux[Event, Classifier, Subscriber]](
+    busName: String,
+    conf: Config = ConfigFactory.empty())


yep, the refactoring for type projections will be intrusive. It would be nice to explore some alternatives to reduce the impact on users. However, that won't be zero-impact.

He-Pin · 2020-04-12T16:07:42Z

Like first submit the syntax changes and get that be merged and more following up.

patriknw · 2020-04-27T11:27:13Z

This is superseded by smaller PRs, as far as I understand

He-Pin · 2020-04-27T17:19:50Z

@ohze friend ping with
Patrik Nordwall @patriknw Apr 27 20:33 We will release Akka 2.6.5 this week. Probably Thursday.

akka-ci added the validating PR is currently being validated by Jenkins label Mar 28, 2020

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Mar 28, 2020

He-Pin reviewed Mar 28, 2020

View reviewed changes

giabao force-pushed the dotty-syntax-compat branch from e7049a4 to ac316da Compare March 28, 2020 16:17

He-Pin reviewed Mar 28, 2020

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 28, 2020

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Mar 28, 2020

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Mar 29, 2020

giabao force-pushed the dotty-syntax-compat branch from fbab667 to f7de200 Compare March 29, 2020 14:53

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Mar 29, 2020

giabao force-pushed the dotty-syntax-compat branch from f7de200 to 101ef98 Compare March 30, 2020 08:36

raboof reviewed Mar 30, 2020

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Mar 30, 2020

giabao added 7 commits April 9, 2020 19:13

fix dotty compile errors: Reference to SayHello is ambiguous

39aa1cc

fix dotty compile error: non-private method apply in object SessionBe…

242c4c9

…havior refers to private class PublishSessionMessage

fix a dotty compile error when using implicit class conversion on `…

174dfdf

…var` (system : akka.actor.typed.ActorSystem[String]) is not a valid type prefix, since it is not an immutable path

fix dotty compile error in construct: val x = new { def f = ??? }; x.f

aecd9f1

It's seem dotty will think `x: AnyRef` so `x.f` don't existed

scalafmtAll

57ed266

mima ProblemFilters

fbc29f6

giabao force-pushed the dotty-syntax-compat branch from dc1f891 to 314f0ee Compare April 9, 2020 14:23

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Apr 9, 2020

fix dotty compile errors ambiguous reference

ebb8527

For example: Reference to context is ambiguous it is both defined in object TypedActor and inherited subsequently in class TypedActor

giabao force-pushed the dotty-syntax-compat branch from 314f0ee to ebb8527 Compare April 9, 2020 15:26

octonato reviewed Apr 10, 2020

View reviewed changes

This was referenced Apr 20, 2020

dotty phase 1 #28947

Merged

dotty phase 2: scalafix ExplicitNonNullaryApply #28949

Merged

patriknw closed this Apr 27, 2020

ohze mentioned this pull request May 27, 2020

dotty phase 3: Some required syntax changes #29139

Merged

Dotty syntax compat #28837

Dotty syntax compat #28837

Conversation

ohze commented Mar 28, 2020

akka-ci commented Mar 28, 2020

He-Pin commented Mar 28, 2020 • edited

akka-ci commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin commented Mar 28, 2020 • edited

ohze commented Mar 28, 2020

akka-ci commented Mar 28, 2020

akka-ci commented Mar 29, 2020

akka-ci commented Mar 29, 2020

ohze commented Mar 29, 2020

He-Pin commented Mar 30, 2020

raboof commented Mar 30, 2020

raboof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Apr 9, 2020

akka-ci commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin commented Apr 12, 2020

patriknw commented Apr 27, 2020

He-Pin commented Apr 27, 2020

He-Pin commented Mar 28, 2020 •

edited

He-Pin commented Mar 28, 2020 •

edited