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.

Sign up for GitHub

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

Jump to bottom

More dotty support #2147

Merged
merged 10 commits into from May 30, 2022
Merged

More dotty support #2147

merged 10 commits into from May 30, 2022

Conversation

exoego
Copy link
Collaborator

@exoego exoego commented May 1, 2022 •
edited

Summary

module Before After
diJVM Passed 84 Passed 89 (+5) All ✅
surfaceJVM Passed 100 Passed 101 (+1) All ✅
codecJVM Passed 107 Passed 107 (-) 😑

Raw results

Before

$ DOTTY=1 sbt  projectDotty/test | grep '\[error\]' > error.txt
$ cat error.txt
[error] Error: Total 89, Failed 0, Errors 5, Passed 84, Pending 1
[error] Error during tests:
[error] 	wvlet.airframe.DesignSerializationTest
[error] 	wvlet.airframe.ProviderSerializationTest
[error] 	wvlet.airframe.DesignTest
[error] Error: Total 109, Failed 0, Errors 2, Passed 107, Pending 1
[error] Error during tests:
[error] 	wvlet.airframe.codec.PrimitiveCodecTest
[error] Failed: Total 101, Failed 1, Errors 0, Passed 100
[error] Failed tests:
[error] 	wvlet.airframe.surface.RecursiveHigherKindTypeTest
[error] (codecJVM / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] (diJVM / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] (surfaceJVM / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 19 s, completed 7 May 2022, 19:16:03

After

$ DOTTY=1 sbt  projectDotty/test | grep '\[error\]' > error.txt
$ cat error.txt
[error] Failed: Total 109, Failed 1, Errors 0, Passed 108, Pending 1
[error] Failed tests:
[error]         wvlet.airframe.codec.PrimitiveCodecTest
[error] (codecJVM / Test / test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 36 s, completed May 28, 2022, 10:32:27 PM

exoego
exoego commented May 1, 2022
View reviewed changes
airframe-di/.jvm/src/test/scala/wvlet/airframe/DesignSerializationTest.scala

import DesignTest._
import wvlet.airspec.AirSpec

class CustomClassLoader(in: InputStream) extends ObjectInputStream(in) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note) This custom class loader fixes java.lang.ClassCastException: cannot assign instance of scala.collection.generic.DefaultSerializationProxy to field wvlet.airframe.Design.binding of type scala.collection.immutable.Vector in instance of wvlet.airframe.Design

Copy link
Member

@xerial xerial May 1, 2022 •
edited

Choose a reason for hiding this comment

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

FYI: Serializing Design objects is necessary in td-spark for sending a design to remote Spark worker nodes https://www.slideshare.net/taroleo/tdspark-internals-extending-spark-with-airframe-spark-meetup-tokyo-3-2020

image

@codecov
Copy link

codecov bot commented May 1, 2022 •
edited

Codecov Report

Merging #2147 (373fcb7) into master (558e981) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2147   +/-   ##
=======================================
  Coverage   82.62%   82.62%           
=======================================
  Files         313      313           
  Lines       12357    12357           
  Branches      780      780           
=======================================
  Hits        10210    10210           
  Misses       2147     2147

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 558e981...373fcb7. Read the comment docs.

@exoego exoego force-pushed the fix-dotty branch 3 times, most recently from be5352c to 2df02bd Compare May 7, 2022 14:24
@xerial xerial added this to the Scala 3 milestone May 8, 2022
@xerial
Copy link
Member

xerial commented May 17, 2022

@exoego Let me know when this PR is ready for review :)

xerial
xerial reviewed May 28, 2022
View reviewed changes
airframe-di/src/test/scala/wvlet/airframe/DesignTest.scala
@@ -84,7 +84,7 @@ object DesignTest extends AirSpec {

test("bind providers") {
val d = newSilentDesign
.bind[Hello].toProvider { (m: ProductionString) => Hello(m) }
.bind[Hello].toProvider[ProductionString] { m => Hello(m) }
Copy link
Member

Choose a reason for hiding this comment

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

I guess there should be a workaround for this 😓

If we need to list all types D1, D2, ..., D5 everytime, it would be hard to use provider bindings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, added TODO here

exoego
exoego commented May 28, 2022
View reviewed changes
airframe-surface/src/main/scala-3/wvlet/airframe/surface/CompileTimeSurfaceFactory.scala
val tastyType = quotes.reflect.TypeRepr.typeConstructorOf(paramType)
surfaceOf(tastyType)
}.toSeq
// FIXME: Use TastyInspector as runtime-like reflection for Scala 3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try Tasty Inspector for here in subsequent PR. https://dotty.epfl.ch/docs/reference/metaprogramming/tasty-inspect.html
It will be relatively big.

@exoego exoego marked this pull request as ready for review May 28, 2022 13:38
@exoego
Copy link
Collaborator Author

exoego commented May 28, 2022

This stalls for a bit, since I think Tasty Inspector as runtime-like reflection is needed to construct Surface from Class for MessageCodec.of[Any] support.
I will split the part for later work.

xerial
xerial reviewed May 30, 2022
View reviewed changes
airframe-surface/src/main/scala-3/wvlet/airframe/surface/CompileTimeSurfaceFactory.scala
case Some(expr) =>
expr
case None =>
// FIXME: Is this right ?
Copy link
Member

Choose a reason for hiding this comment

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

ok for now.

xerial
xerial reviewed May 30, 2022
View reviewed changes
Copy link
Member

@xerial xerial left a comment

Choose a reason for hiding this comment

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

ok. Thanks. There is no major concern for now.

@xerial xerial merged commit 83bf966 into master May 30, 2022
@exoego exoego deleted the fix-dotty branch May 30, 2022 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xerial xerial xerial left review comments

Assignees
No one assigned
Labels
None yet
Projects
Airframe Roadmap
Status: Done
Milestone
Scala 3
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@exoego @xerial