-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Dotty syntax compat #28837
Dotty syntax compat #28837
Conversation
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! |
Very cool if we can have Akka release for dotty and add Akka to dotty's community builds. |
Test FAILed. |
@@ -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]) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With scalafix rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. dotty compile errors => I just manually change :)
e7049a4
to
ac316da
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with scalfix rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but new code will be added then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but new code will be added then.
what? sorry I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
It would be better to extract some changes related to the build definition to a dedicated plugin like |
OK. I will try that idea |
Test PASSed. |
fbab667
to
f7de200
Compare
Test FAILed. |
1 similar comment
Test FAILed. |
done |
@ohze Will need a scalafmtAll,I always forgot that too. |
Looks like this needs a copyright header in |
f7de200
to
101ef98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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
dc1f891
to
314f0ee
Compare
Test FAILed. |
For example: Reference to context is ambiguous it is both defined in object TypedActor and inherited subsequently in class TypedActor
314f0ee
to
ebb8527
Compare
Test FAILed. |
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f.setAccessible(true) | ||
f.setLong(this, time) | ||
} | ||
earlyInit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be good to add a comment here explaining why this. I personally can't immediately see the motivation.
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this class is being renamed?
I'm not against it, but if not strictly necessary better to do on another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If don't rename => dotty compile error in all case Event(..
in StressTest
because FSM
(base trait of StressTest
) also has Event
member.
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Subscriber, | ||
BusType <: EventBusSpec.EventBusAux[Event, Classifier, Subscriber]]( | ||
busName: String, | ||
conf: Config = ConfigFactory.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Like first submit the syntax changes and get that be merged and more following up. |
This is superseded by smaller PRs, as far as I understand |
@ohze friend ping with |
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:![Build Status](https://camo.githubusercontent.com/5b2a3388665a51a47e9b3758a2d7646c3818871ab418a58eed09e561212fcc2e/68747470733a2f2f7472617669732d63692e6f72672f6f687a652f616b6b612e7376673f6272616e63683d646f7474792d776970)