WIP: Add cross-build for Scala 3.0 #290
Conversation
|
I've thought a bit more about what we can do with all the code using
For cases where For cases where "deeper" type information is extracted, we have to find a different solution because My main goal is to unblock |
|
Thanks, this is great! Just as a heads up, we will probably want to merge this stuff in incrementally, rather than as a big-bang patch, so that we can review it carefully (especially where we have to break APIs w/ Manifest / TypeTag). However, I think keeping a bunch of ideas on a single big branch is also good. We'll just need to break it up later. One other thing is that we're considering having a couple of interns take a stab at some of this work too, so we may be able to put together a crew to work on this. Re: |
|
We have something similar to I could imagine a similar strategy here as it allows us to migrate modules one-by-one, split up work between people, and maybe even cross-publish the already migrated subset (not all modules might be needed for downstream projects). Once #288 is merged, I can try and open a PR for just the cross-build setup and we go from there. I'll leave this PR up for now if you don't mind, as the diff might be useful to others working on it just like I used the previous work by @martijnhoekstra .
Scala 3 will only have We will have to test how libraries like Jackson work with type-parameters. For example if something like |
|
Thank you so much for looking into this! I was imagining that if there wasn't a way to create binary compatible I did some work to tease apart what could be done in parallel, I'll add the table below. I suspected we'd have to have some 2.13 and 3 specific code for the cross-build until we can figure out blockers. The information you provided about ClassTags is really useful, thank you!
|
|
I've opened a Scala3 bug regarding Java-API not being able to call overridden trait-methods (e.g. |
To be clear: TypeTag and everything else from scala-reflect will never come back in Scala 3, Scala 3 has its own meta-programming mechanisms detailed in https://dotty.epfl.ch/docs/reference/metaprogramming/toc.html. But depending on what you're using TypeTag for you might not need any of that and could either use ClassTag (if all you need is a convenient way to get the erased class of a type) or the new TypeTest mechanism: http://dotty.epfl.ch/docs/reference/other-new-features/type-test.html |
|
@smarter Thanks for the clarification. It wasn't my intention to imply TypeTag could potentially come back, I worded that poorly. I didn't know about |
|
I just confirmed that in Scala 3.0.2 the compile errors for the Java-API will be fixed. 🙂 (I tried |
| @@ -467,26 +461,26 @@ class Promise[A] extends Future[A] with Promise.Responder[A] with Updatable[Try[ | |||
| // - Transforming | |||
| // - Try[A] (Done) | |||
| // - Promise[A] | |||
| @volatile private[this] var state: Any = WaitQueue.empty[A] | |||
| private def theState(): Any = state | |||
| private[this] val state: AtomicReference[Any] = new AtomicReference[Any](WaitQueue.empty[A]) | |||
There was a problem hiding this comment.
Hey @felixbr, was this an automated rewrite or did you change this yourself? Where you addressing a warning that looked like the type test for <blah> cannot be checked at runtime?
There was a problem hiding this comment.
I changed usage of com.sun.misc.Unsafe and @volatile to AtomicReference and other Atomic* types because the former no longer worked with Scala3 for me. I didn't try to fix or improve anything else because this code is very low-level and "dangerous". The tests all worked fine with Atomic* but I didn't do any performance checks.
There were some other changes in the file, simplifying the logic, which I've copied from earlier work by @martijnhoekstra. So maybe he was addressing that warning.
edit: After thinking some more about it, maybe it wasn't Scala3 why com.sun.misc.Unsafe was no longer working, could also be a JDK11+ thing. In any case, if performance is the same, I'd recommend using Atomic* types, as they are much simpler and safer to use.
There was a problem hiding this comment.
Noted! thank you so much for expanding!
felixbr
force-pushed
the
update/scala-3.0.x
branch
from
b3436cb
to
4d2ddcb
Compare
|
I rebased the changes onto the last commit before the (twitter-internal?) Scala3 migration by @hamdiallam as there's a bunch of conflicts now. The Scala 3.0.2-RC1 version works fine, so that's good. If I can help in any way by splitting things from this PR into a separate PR which can actually be merged into |
| /** | ||
| * Forwarder for Int, as Scala 3.0 seems to not like the implicit conversion to Long. | ||
| */ | ||
| implicit def numBytesFromInt(numBytes: Int): RichStorageUnit = new RichStorageUnit(numBytes.toLong) |
There was a problem hiding this comment.
Do you know what the Scala 3 compiler doesn't like the implicit conversion from int to long? Perhaps a bug?
There was a problem hiding this comment.
I don't see it as a bug. In Scala 2 you can already force a warning/error with -Wnumeric-widen in most cases where Int would implicitly be treated as Long. I'm not sure if that includes extension methods but if not that's an oversight imo.
I think not doing these conversions implicitly by default is a good thing.
There was a problem hiding this comment.
edit: I mixed up the types before.
Here you can see that with the compiler-flags turned on, it also produces a warning/error: https://scastie.scala-lang.org/vfZWBXEBQ3i0lXIlj7Q8cA
If you switch to Scala 3 it no longer works at all.
There was a problem hiding this comment.
Another option would be to write the extension methods in terms of Numeric but that has other problems like how to handle floating-point numbers correctly and losing the value class:
https://scastie.scala-lang.org/S9UEDrf4R02nklksX6Qfaw
I'd not do this for the Scala 3 migration at least, as the implicit conversion to Long is much simpler.
|
@felixbr Hey, I realized I dropped the ball on this. We decided to have our interns work on Scala 3.x directly to reduce their overhead so they could make as much progress during their internship as possible. I’m really sorry for not keeping you in the loop earlier! Your work greatly inspired their intern project, so I feel especially bad that I didn’t do a good job of communicating it with you. We’ll work with you to split out the rest of the patches you’ve made so you can finish the job after their internship ends next week. I owe you a beer (or the beverage of your choice), please look me up if you’re ever in NYC! |
|
Hi @felixbr,so sorry we dropped the ball on this! Let's talk about breaking up some of this work so we can start merging it! We like to merge subproject by subproject, so if you take a look at the build.sbt file, you will see that we have two settings: |
|
Sorry for not answering sooner, I was on vacation. @mosesn No need to apologize. I honestly don't care too much about attribution if omitting it speeds up anything. I just want to use Scala 3 with Finagle, any progress on that is good news to me. @dotordogh The two settings are great, that'll make things easier for sure. 🙂 |
This fixes lost source-compatibility for unit conversions from Int values in Scala3. As discussed in #295 I extracted/adapted this from #290 (previous discussion). Previously something like 1.toInt.second did compile in Scala2 (with default settings) but didn't in Scala3. I added forwarders for all units which use Long as base, as that seemed reasonable and safe. I didn't add an implicit conversion from Float to Double because floating-point types are dumb and I don't trust the conversion to be lossless in all cases. Signed-off-by: Yufan Gong <[email protected]> Differential Revision: https://phabricator.twitter.biz/D759923
IMPORTANT: This is WIP, I just want to put it up for discussion and to avoid duplication of efforts. There are lots of things that still need to be fixed. I've prefixed them with
// TODO Scala3so it's easy to find.I took the initial work by @martijnhoekstra and tried to push it as far as possible. Many things work with little or no modification but anything that touches reflection is likely a blocker at this point. There are also one or two things that might be compiler-bugs and we should probably open tickets for the Scala 3 team.
Blockers so far:
Manifest. In some cases replacing it withClassTagmight be fine but it can be unsafe when used with the wrong types. The alternative would be source-incompatible changes. Both options aren't great.scala-reflect. I haven't looked into it yet bututil-reflectneedsscala-reflectand is used by other modules (util-benchmark,util-validator)DurationandTimeis not compiling with Scala3. This could be a compiler-bug. In any case we have to solve it or provide the Java-API separately (which would be a breaking change).util-mockdepends onmockito-scalawhich uses macros and doesn't have a Scala3 version. I haven't checked yet if we can port it withoutmockito-scalamacros, but I doubt it will work without breaking changes.A lot of things are already working for both 2.13.6 and 3.0.0. Most importantly
util-core. I had to replacesun.misc.Unsafeusage because in Scala3 it caused weird crashes. But withAtomicIntegerandAtomicReferenceit works fine and if there are no performance issues, it also makes the code a bit cleaner.There's probably more to be said but I'm hungry and the most important part is discussing the blockers. If anyone wants to collaborate directly on this PR, I'll gladly give you commit-access to the fork.
Cheers
~ Felix