Scala3 derives under -Yexplicit-nulls compiler flag
#2083
Conversation
|
@rossabaker Could you help to give a review here. Thx :) |
|
I am not yet a Scala 3 user other than compiling several libraries for it, but this seems reasonable to me, and addresses my concern with the prior PR. |
|
It's strange that I passed all tests with similar conditions (temurin@8, rootJVM). My local dev environment Tested for two JVM versions How could we rerun actions? |
|
I can rerun it, but compiler errors are usually not flakes. In case it suddenly passes, here's the 3.2.1, temurin@8, rootJVM failure: |
|
Thanks! But seems more failures 😢, I will try to test them on Linux or exact Ubuntu distribution. |
|
I suspect the problem here is that the build is failing on a merge ref. That commit isn't the HEAD of your PR branch, but a merge commit that would result from merging this PR into main. While your branch is healthy, and main is healthy, the merge that would result is not. GitHub tests this merge commit to guard against these "semantic merge conflicts". It's complaining about an amibiguous implicit instance. I didn't dig too deep, but my guess is that Cats added it since the original author started this PR, and now it's conflicting with one Circe had in If you merge the latest main into your branch, I expect you'll see this error locally and be able to fix it. |
modules/tests/shared/src/main/scala-3/io/circe/tests/CirceMunitSuite.scala
Outdated
Show resolved
Hide resolved
|
@zarthross @rossabaker Could you review again? I have fixed problems and addressed change requests. Thank you for your helps! |
modules/tests/shared/src/test/scala-3/io/circe/DerivesSuite.scala
Outdated
Show resolved
Hide resolved
|
We need to get this branch updated and conflicts resolved, there was a big PR that refactored Derivation that just went through. |
|
@zarthross Ok. Thank you for the reminder. I would rebase it later (probably weekend) and reorg the tests (I have passed native tests, is trying to reduce boilerplate codes for now). |
|
@zarthross It's ready for review. And actually, after my tests, using the latest release 0.14.4, Circe can be successfully compiled by user's library with Honestly, it's also fine for me to reject PR. Both |
There was a problem hiding this comment.
I'm really not sure what these tests are actually testing right now? I tried adding this test and it still passes.
test("Java boxed class would not compile without `.nn` postfix with `-Yexplicit-nulls`") {
def code(using Quotes) = '{
given Encoder[Instant] = Encoder.encodeString.contramap[Instant](t =>
ISO_OFFSET_DATE_TIME
.format(
t.truncatedTo(MILLIS).atOffset(ZoneOffset.UTC)
)
)
given Codec[Event] = Codec.AsObject.derived[Event]
}
staging.run(code)
}
I realize the assertion is not correct, but i wanted to see this fail before i wrote the assertion, and when the test passed it told me the other tests passing could be a false positive.
Fixes #1786. Since this requires a new compiler flag, I had to come up with some way to test it. I looked into setting this flag for the tests in the tests module, but this would have made a lot of code that is shared with Scala 2 stop compiling, including where JDK APIs return values that are never null but not annotated as such. I opted instead to add a dependency on scala3-staging and use the "Runtime Multi-Stage Programming" describe at https://dotty.epfl.ch/docs/reference/metaprogramming/staging.html
|
@zarthross Thanks for your reply. It comes from a real case. Yeah, as I said:
Here is my minimal reproduction: import java.time.Instant
import java.time.format.DateTimeFormatter.ISO_OFFSET_DATE_TIME
import java.time.temporal.ChronoUnit.MILLIS
import io.circe.{Codec, Encoder, Json}
import io.circe.syntax.*
import io.circe.parser.{decode, parse}
given Encoder[Instant] =
Encoder.encodeString.contramap[Instant](t =>
ISO_OFFSET_DATE_TIME.nn.format(t.truncatedTo(MILLIS).nn).nn
)
case class InEvent(id: Int, name: String, eventTime: Instant)
derives Codec.AsObject
@main def hello: Unit =
val rawJson: String = """
{
"id": 13,
"name": "test-event",
"eventTime": "2023-01-05T08:04:55.697Z"
}
"""
val inEvent = parse(rawJson)
.flatMap(_.as[InEvent])
.getOrElse(InEvent(0, "error", Instant.now().nn))
println(s"${inEvent}")
println(msg)
def msg = "I was compiled by Scala 3 with Circe :)"And Above sample codes will successfully be compiled and run with You could download this to setup project minimal-reproductions.zip.
Honestly, I have tried rebase to history to find which commits fix this problem, but I don't locate it yet. I will be appreciated if someone could teach me how to use |
|
There is a type mismatching error if we do not use |
|
@lqhuang I completely understand what you are trying to do, you are trying to make a test to prevent the regression of support for I think something is wrong with your use of |
|
@zarthross You're right! It's my faults. And I did some experiments and found both test cases are not actually compiled then run. Even the simplest demo not works ether. That's probably why I didn't find which commit fix issue. |
|
Here is my experiment. I create a Scala 3 project and using staging API to write codes directly. And I upload these codes to a repo: https://github.com/lqhuang/staging-with-case-class-and-circe or download it from attachments staging-with-case-class-and-circe-master.zip. All 3 cases cannot normally run. There are some limits in current staging API. Case 1import io.circe.{Codec, Encoder, Json, Decoder}
import io.circe.syntax.*
import io.circe.parser.{decode, parse}
import scala.quoted.staging
import scala.quoted.{Quotes, Type}
@main def Case1: Unit = {
val settings =
staging.Compiler.Settings.make(compilerArgs = List("-Yexplicit-nulls"))
given explicitNullsCompiler: staging.Compiler =
staging.Compiler.make(getClass.getClassLoader)(settings)
def code(using Quotes) = '{
case class Foo(val i: Int, val s: String)
given Codec[Foo] = Codec.AsObject.derived[Foo]
given Decoder[Foo] = Decoder.derived[Foo]
given Encoder[Foo] = Encoder.AsObject.derived[Foo]
println(
"Case class definitions are not allowed in inline methods or quoted code. Use a normal class instead."
)
}
staging.run(code)
}Compiler tells me that Case 2I moved the import io.circe.{Codec, Encoder, Json, Decoder}
import io.circe.syntax.*
import io.circe.parser.{decode, parse}
import scala.quoted.staging
import scala.quoted.{Quotes, Type}
@main def Case2: Unit = {
val settings =
staging.Compiler.Settings.make(compilerArgs = List("-Yexplicit-nulls"))
given explicitNullsCompiler: staging.Compiler =
staging.Compiler.make(getClass.getClassLoader)(settings)
case class Foo(val i: Int, val s: String) derives Codec.AsObject
def expr(using Quotes, Type[Foo]) = '{
given Codec[Foo] = Codec.AsObject.derived[Foo]
given Decoder[Foo] = Decoder.derived[Foo]
given Encoder[Foo] = Encoder.AsObject.derived[Foo]
println(
"""access to object Foo from wrong staging level:
| - the definition is at level 0,
| - but the access is at level 1.
""".stripMargin
)
}
staging.run(expr)
}Compiler error: Seems because the Case 3In case 3, just define original import scala.quoted.{Quotes, staging, Type}
@main def Case3: Unit = {
val settings =
staging.Compiler.Settings.make(compilerArgs = List("-Yexplicit-nulls"))
given explicitNullsCompiler: staging.Compiler =
staging.Compiler.make(getClass.getClassLoader)(settings)
def code(using Quotes) = '{
import io.circe.{Codec, Encoder, Json, Decoder}
import io.circe.syntax.*
import io.circe.parser.{decode, parse}
import io.circe.JsonObject
class Foo(val i: Int, val s: String) {
override def toString(): String = s"Foo($i, $s)"
}
object Foo {
def apply(i: Int, s: String): Foo = new Foo(i, s)
given Codec.AsObject[Foo] = {
Codec.AsObject.from(
Decoder.instance(c =>
for {
i <- c.downField("i").as[Int]
s <- c.downField("s").as[String]
} yield Foo(i, s)
),
Encoder.AsObject.instance[Foo](x =>
JsonObject(
"i" -> Json.fromInt(x.i),
"s" -> Json.fromString(x.s)
)
)
)
}
}
parse("{'i': 1, 's': 'string'}").flatMap(_.as[Foo]) match {
case Left(error) => println(error)
case Right(foo) => println(foo)
}
}
staging.run(code)
}Error info: |
|
I have no idea about how to solve it. If you have other approaches to test these behaviors, I'm glad to try it and help. Anyway, @zarthross, really appreciate to your patience! Sorry to waste your time. I will close the PR now or reopen it until we get something new progress in the future. Thanks for your time! |
|
No problem! I'm sorry we weren't able to get a test working. Thanks for trying! Maybe we can retry with a future Scala 3 version 😅 |
Fixes #1786. Projects added
-Yexplicit-nullsflag cannot compile successfully with Circe.Previous PR #1788 seems staled, this PR takes same changes but has rebased to the latest head of current master.
The left one problem is I'm not sure whether it's correct and enough to test different compiler flags with or without
-Yexplicit-nulls(see changes inbuild.sbtandmodules/tests/shared/src/test/scala-3/io/circe/DerivesSuite.scala).Your help and feedback are really appreciated.
Thanks!
extra info (without ScalaJS):