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.

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

Getting started with Scala 3 #81

Merged
merged 41 commits into from Aug 5, 2021
Merged

Conversation

russellremple
Copy link
Collaborator

@russellremple russellremple commented Jul 12, 2021

Only have common code between Scala 2 and 3 when Scala 2 libs can maintain binary compatibility. Any non-passing tests are commented out in the scala-3 source paths (same as in uPickle, e.g., AdvancedTests). Macros are based on uPickle macros with WeePickle-specific features like custom discriminator names, default-dropping behavior, special handling for Option defaults, and "Nullable" To's.

There are still many TODOs, no benchmarks, etc. So this PR is really just the beginning.

@russellremple russellremple marked this pull request as ready for review July 15, 2021 00:55
@@ -37,7 +37,7 @@ object JsonPointerVisitor {
* Compared to something like a List[String] or List[Object], this does not require
* extra String allocation or boxing unless we actually ask for the path.
*/
private trait HasPath {
trait HasPath { // Scala 3 needs this to be public -- doesn't break bin compat
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does scala 3 need this?

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'm actually surprised that Scala 2 let the private class leak out in a public constructor, but it did. And this is one place where the weird rules of bin compat work in our favor: technically changing something from private to public should affect bin compat, but Scala generates these as public interfaces in the bytecode, so from Mima perspective, it was already public. (Which is why, I think, it is hard to lock Scala bin compat down even by making everything private, because the compiler often winds up exposing private things as public.)

...JsonPointerVisitor.scala:97:32 
[error] 97 |private class JsonPointerVisitor[T, J](
[error]    |                                ^
[error]    |non-private constructor JsonPointerVisitor in class JsonPointerVisitor refers to private trait HasPath
[error]    |in its type signature [T, J]
[error]    |  (delegate: com.rallyhealth.weepickle.v1.core.Visitor[T, J], parentPath: 
[error]    |    com.rallyhealth.weepickle.v1.core.JsonPointerVisitor.HasPath
[error]    |  ): com.rallyhealth.weepickle.v1.core.JsonPointerVisitor[T, J]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought... TIL that you can mark the default constructor as private, which is apparently not the default for a private class (weird that it isn't -- seems like a bug). Anyway, this allows HasPath to remain private and makes both Scala 2 and 3 happy. See latest commit.

@@ -215,7 +215,7 @@ object Obj {
implicit def from(items: TraversableOnce[(String, Value)]): Obj = {
val initialCapacity = items match {
case is: mutable.IndexedSeq[_] => is.size
case _ => 2
case _ => 2

Choose a reason for hiding this comment

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

not that it's going to affect anything, but this whitespace change is a bit nutty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@russellremple russellremple merged commit 271b927 into rallyhealth:v1 Aug 5, 2021
import deriving._, compiletime._

inline def getDefaultParams[T]: String => Option[() => AnyRef] = ${ getDefaultParmasImpl[T] }
def getDefaultParmasImpl[T](using Quotes, Type[T]): Expr[String => Option[() => AnyRef]] =
Copy link

Choose a reason for hiding this comment

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

Minor detail (currently catching up), but while I notice it: getDefaultParmasImpl -> getDefaultParamsImpl .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants