playframework / play-json

The Play JSON library
Apache License 2.0
361 stars 134 forks source link

Please can the scala 3 lib include the scala 2 macros? #954

Closed johnduffell closed 3 months ago

johnduffell commented 10 months ago

Play JSON Version (2.5.x / etc)

3.0.1

API (Scala / Java / Neither / Both)

Scala

Overview

The project doesn't seem to have been set up for macro mixing as per https://docs.scala-lang.org/scala3/guides/migration/tutorial-macro-mixing.html

I had a look at https://github.com/playframework/play-json/blob/main/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacros.scala and I noticed it doesn't include the scala 2 macro declarations that are in https://github.com/playframework/play-json/blob/main/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacros.scala and the build.sbt

A similar PR (not yet merged) on another project to implement macro mixing is here: https://github.com/lightbend-labs/scala-logging/pull/415

Expected Behaviour

Setup: I have a mixed module Scala 2.13/3 project using play-json. I imported play-json_3 using cross(CrossVersion.for2_13Use3) My scala 2.13 code includes implicit val reads1: Reads[Price] = Json.reads[Price]

Result: I expect it to compile and work the same as if I import the play-json_2.13 library.

Actual Behaviour

Result: That line fails to compile with illegal cyclic reference involving object Json

Reproducible Test Case

I haven't provided a test case because there's likely to be a simple answer (either it's not implemented for a good reason, or because no one has time yet) rather than it being something subtle that needs clarity.

mkurz commented 10 months ago

I think there was no reason. Feel free to provide a pull request.

johnduffell commented 10 months ago

thanks, thought that might be the case! I'll work around for now, might get onto a PR in my 10% time the new year...

cchantep commented 10 months ago

I don't think it would make sense. Macro are compile time. That's clearly started in the Scala 3 migration guide that when using the compatibility mode, the macro won't be ok.

johnduffell commented 8 months ago

Hi @cchantep thanks for your comment, unfortunately I'm not clear whether you misunderstand or I do! Macros are indeed compile time. For non-macro libraries, the scala 2.13 and scala 3 compilers can both compile against the scala 3 version, meaning we can depend on the scala 3 version of non macro libraries in a multiproject 2.13/3 build. However, as you point out, for macro libraries we don't have that flexibility - the scala 2.13 compiler requires the scala 2.13 macros and the same for 3. The idea of the macro mixing functionality mentioned in the scala docs is to include both the scala 2.13 macros and the scala 3 macros in the scala 3 library, meaning we can just depend on the scala 3 version of play-json, even if we are compiling scala 2.13. The scala 3 and scala 2.13 compilers both know to pick the right version at compile time. It will increase the size of the artifact, but it makes it possible to upgrade larger projects far more easily.

cchantep commented 8 months ago

Having migrated "some" projects with macros, I doubt that really possible, if it is, it will increase drastically the complexity for play-json, but also for the project.

Moreover, I don't see why a Scala 3 project would include play-json_2.13 whereas play-json_3 is available (with macros), neither I would get the point for a Scala 2.13 to include play-json_3.

johnduffell commented 8 months ago

@cchantep thanks for replying but we are just looking for the benefits outlined in the official docs here: https://docs.scala-lang.org/scala3/guides/migration/tutorial-macro-mixing.html

One of the major selling points for the 2.13->3 migration is that if you have multi project builds, or transitive library dependencies, you neednt upgrade them all at once, you can do the dependencies and then the application code. We would love to gradually upgrade to scala 3, but it would be infeasible to do it in one go, and we have reached the limits of what we can do without macro mixing as detailed above.

cchantep commented 8 months ago

Transitive dependency is a runtime point, not a compile time one. Even in a multi-module project, as play-json is available for both version, I don't see the point. A sub-module A can depends on play-json 2.13 (as compat or because it is itself in 2.13), and same for 3.

johnduffell commented 8 months ago

I'm not sure what's gone wrong in communication here but at the rate this conversation is going it might be quicker for me just to produce the PR!

Transitive dependency is a runtime point, not a compile time one

In the general case it's both - but this is macros so we are concerned about compile time dependencies.

A sub-module A can depends on play-json 2.13 (as compat or because it is itself in 2.13), and same for 3.

I'm not sure what you mean here, but since play-json contains macros (and macro mixing is not implemented) then compat mode does not work. If macro mixing is implemted, then play-json_3 can be used in compat mode with scala 2.13 source code.

For an example, see this PR, which passes all the relevant tests. https://github.com/lightbend-labs/scala-logging/pull/415

cchantep commented 8 months ago

Rather than a PR, a show case of why it would be useful. For now, I don't see any legit case.

It can be used to create a new Scala 3 macro library and make it available for Scala 2.13 users. It can also be used to port an existing Scala 2.13 macro library to Scala 3, although it is probably easier to cross-build.

PlayJSON is not is such case, as macro dedidated to each version are already available.

johnduffell commented 8 months ago

Hi cchatep - ok the particular case I needed it for is that we have a repo https://github.com/guardian/support-service-lambdas with a load of production lambdas in which rely on some common modules and thereafter libraries. We are not using a lot of scala 3 yet but there are a few of us pushing things in different teams (I am in supporter revenue team)

I have started writing new things and updating some tiny things in scala 3 and had small successes, but here I quickly got blocked, because as soon as I updated one thing to scala 3, it failed at the sbt update stage about a version conflict play-json_213 and play-json_3. I overrode the dependency to be the scala 3 version, but then the modules that were still on 2.13 failed at the sbt compile stage. https://github.com/search?q=repo%3Aguardian%2Fsupport-service-lambdas%20playJson&type=code

There are a few options for us here:

  1. leave everything on scala 2.13 (the default option, but it would speed up our move away from scala entirely)
  2. implement macro mixing for required macro libraries (this seemed the most promising based on the official scala docs)
  3. upgrade all lambdas in that mono repo in one go (not feasible due to the time commitment + risk)
  4. create a new scala 3 dependency tree in that build and cross compile all modules that depend on play-json directly or indirectly (too much work and residual complexity, the build is already very complex, our scala 3 migration progress is already too slow)
  5. use a shading library to allow both play-json 213 and 3 on the classpath (not sure how the tooling is there, but it adds complexity regardless, and tooling such as snyk doesn't handle shaded dependencies)
  6. migrate away from play-json to circe or upickle (assuming they either have support or are open to it)

If you are happy to take a look at our project and provide a recommendation then I would be very pleased to take it on board.

johnduffell commented 8 months ago

on reflection the scala docs that you quote are misleading and seem to discourage macro mixing as an experimental option, and don't explain the benefits fully. So I can understand your reluctance to deviate from the documentation's recommendation.

I don't believe it's experimental any more so I will submit a docs PR there. Depending on the outcome of my PR there, that might inform your thinking on this matter.

cchantep commented 8 months ago

There is no "leave everything on scala 2.13".

Latest Play-JSON version is avoid with same compile and run time feature for 2.13 and 3.

For me, the link to the project is a clear reproducer to understand what would be a blocker.

johnduffell commented 8 months ago

For me, the link to the project is a clear reproducer to understand what would be a blocker.

Hello @cchantep , I have made a PR on our production codebase so you can see what doesn't work without macro mixing. Of course it's not quite a minimised test case due to it being a real code base, but it should compile fine if you clone it locally. Please let me know your advice to proceed with the migration either with or without macro mixing in play-json.

https://github.com/guardian/support-service-lambdas/pull/2169

cchantep commented 8 months ago

I would say that issue is related to the way it tried to mix submodules while migrating some of them.

Even not considering macro that raise library management issue such as:

[error] Modules were resolved with conflicting cross-version suffixes in ProjectRef(uri("file:/private/tmp/support-service-lambdas/"), "salesforce-client"):
[error]    com.typesafe.play:play-functional _2.13, _3
[error]    com.typesafe.play:play-json _2.13, _3

Updating the build can fix that.

cross-build.patch

johnduffell commented 8 months ago

thanks @cchantep I think that will help us a lot - I was not aware of the ability to define the module twice in the build with different name and settings, this will be easier than trying to patch upstream libraries! (Since the docs used to erroneously discourage macro mixing, perhaps that means many libraries don't implement it)

All things considered, it's not ideal in our build because it will mean code has to be compiled against both scala 2 and 3 unnecessarily, and adds a more cumbersome technique for macro libraries compared with standard, but overall I think it's a good technique for us!

Thanks again.

Finally - does that mean it's an official "no" for implementing it in play-json, i.e. should I close this issue, or is it still a nice idea to include it for convenience of downstream consumers?

cchantep commented 8 months ago

I don't know what could be considered as official, but as a maintainer of macros on PlayJSON and other libraries, I won't go for it. For me mixing in library doesn't help much, and introduce medium/long term complexity for a temporary issue.