-
Notifications
You must be signed in to change notification settings - Fork 595
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
scala3: compile akka-http-core and pass all its tests #4097
Conversation
…ing values" and "If-Match dispatching"
Hi @hughsimpson, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
: TailSwitch[L, T, R] { type Out = TailSwitch0[L, L, T, T, R, HNil] } = `n/a` | ||
implicit def tailSwitch[L <: _ :: _, T <: _ :: _, R <: HList] | ||
: TailSwitch[L, T, R] { type Out = TailSwitch0[L, L, T, T, R, HNil] } = `n/a` | ||
/// Optimisations to reduce compilation times to something tolerable |
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 couldn't figure out how to solve the general issue, but having some special-case implicits to short-circuit logic in certain common patterns seems to be enough for the usages in akka-http to compile
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've been trying to solve this in a more general way but it looks like this would require some fixes/optimizations in the compiler itself.
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.
Yeah, I had the same feeling... I poked around a little but it looks like the main issue is that the compiler is 'surprisingly slow' at evaluating the compile-time type reduction. Since it blows up so quickly, I guess some resolution stage must be the wrong O...
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.
Anyway apologies for stepping on your toes with this one. I got rather carried away testing that the workaround held good for all the cases we encounter in the parsers 😅
Have signed CLA |
@@ -100,14 +100,14 @@ private[parser] trait SimpleHeaders { this: Parser with CommonRules with CommonA | |||
|
|||
// http://tools.ietf.org/html/rfc7233#section-4.2 | |||
def `content-range` = rule { | |||
(`byte-content-range` | `other-content-range`) ~ EOI ~> (`Content-Range`(_, _)) | |||
(`byte-content-range` ~ EOI ~> (`Content-Range`(_, _)) | `other-content-range` ~ EOI ~> (`Content-Range`(_, _))) |
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 original form is thrown off by the different subtypes, but this doesn't feel too bad an expansion...
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, agreed.
akka-http-core/src/main/scala/akka/http/impl/model/parser/UriParser.scala
Show resolved
Hide resolved
@@ -72,7 +75,7 @@ abstract class AkkaSpec(_system: ActorSystem) | |||
|
|||
def this() = this(ActorSystem(AkkaSpec.getCallerName(getClass), AkkaSpec.testConf)) | |||
|
|||
val log: LoggingAdapter = Logging(system, this.getClass) | |||
val log: LoggingAdapter = Logging(system, this.getClass.asInstanceOf[Class[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.
I don't really understand this one TBQH...
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 tried variations but also failed to come up something better. It seems some automatic conversions wrt to Java wildcard types don't work as well (or at least different) in Scala 3 than before.
Wow, great that you tackled this, @hughsimpson! I'm going to have a thorough look later. |
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.
Looking great. The only thing that probably needs fixing is the error message assertions in tests to make sure they work for both Scala 2 and Scala 3. I'll have a look what's going on there.
@@ -100,14 +100,14 @@ private[parser] trait SimpleHeaders { this: Parser with CommonRules with CommonA | |||
|
|||
// http://tools.ietf.org/html/rfc7233#section-4.2 | |||
def `content-range` = rule { | |||
(`byte-content-range` | `other-content-range`) ~ EOI ~> (`Content-Range`(_, _)) | |||
(`byte-content-range` ~ EOI ~> (`Content-Range`(_, _)) | `other-content-range` ~ EOI ~> (`Content-Range`(_, _))) |
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, agreed.
akka-http-core/src/main/scala/akka/http/impl/model/parser/UriParser.scala
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/impl/model/parser/HttpHeaderSpec.scala
Show resolved
Hide resolved
@@ -72,7 +75,7 @@ abstract class AkkaSpec(_system: ActorSystem) | |||
|
|||
def this() = this(ActorSystem(AkkaSpec.getCallerName(getClass), AkkaSpec.testConf)) | |||
|
|||
val log: LoggingAdapter = Logging(system, this.getClass) | |||
val log: LoggingAdapter = Logging(system, this.getClass.asInstanceOf[Class[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.
I tried variations but also failed to come up something better. It seems some automatic conversions wrt to Java wildcard types don't work as well (or at least different) in Scala 3 than before.
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.
LGTM, no blockers, if anything, it's the sorry shape of testing in the Scala 3 branch...
Thanks a lot, @hughsimpson, of getting this important milestone in place!
@jrudolph @hughsimpson it looks I've found the core reason of the problem with |
Great work. While the current solution works for akka-http, there are probably other parboiled2 parsers where the workaround still wouldn't be enough, so good to make progress on a more general solution! |
References #4079, task "akka-http-core"