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

Crossbuild scala-3 #206

Merged
merged 2 commits into from Jul 20, 2021
Merged

Crossbuild scala-3 #206

merged 2 commits into from Jul 20, 2021

Conversation

Daenyth
Copy link
Contributor

@Daenyth Daenyth commented Jul 14, 2021

  • This includes the Traverse derivation, but not with the same api, and doesn't include the recursion scheme derivation
  • I've tried to keep api changes to a minimum
  • The various data types apply/un methods in scala3 use .isInstanceOf directly - this might be slower than the scala2 fastCast macro
  • reftree now builds on all versions, but has source code only in 2.12, due to s̵͎̿̾b̶͚̜͝t̸̮̑ ̷̡̀r̶̭̒̎e̵̢̮͝a̴̜͑s̸̤̈́̏ò̸̭n̸̨̨̋s̸̤̅

@Daenyth Daenyth marked this pull request as ready for review July 14, 2021 20:46
def traverse[G[_]: Applicative, A, B](
fa: Expr[V, A]
)(f: A => G[B]): G[Expr[V, B]] = fa match {
case v: Var[V, A] => (v.asInstanceOf[Expr[V, B]]).pure[G]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

scala3 reports this match as non-exhaustive with the @unchecked annotations in there.

@@ -45,7 +45,7 @@ object ExprParser {
case TSub => shunt(op, 1).flatMap(_.traverse(enqueue))
case TProd => shunt(op, 2).flatMap(_.traverse(enqueue))
case TDiv => shunt(op, 2).flatMap(_.traverse(enqueue))
}).as(tail.asLeft)
}).map(_ => tail.asLeft)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as doesn't resolve, for some reason, in scala3. No idea why.

Choose a reason for hiding this comment

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

I think it was proposed, at some point, as a replacement for the @ in pattern matching. I had some comments about that.

Comment on lines +188 to +196
// 'nowarn' doesn't work on scala3 yet, make warnings not fatal.
scalacOptions --= on(3)("-Xfatal-warnings").value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 This is pretty important

Comment on lines +13 to +14
import higherkindness.droste.data.prelude._
import higherkindness.droste.util.DefaultTraverse
Copy link
Contributor Author

@Daenyth Daenyth Jul 14, 2021

Choose a reason for hiding this comment

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

I ran into issues with things not resolving using relative imports; I'll double check whether this is still needed now that everything compiles.

Edit: it is, don't know why

Copy link
Member

Choose a reason for hiding this comment

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

Hm... I thought we had a scalafix rule to prevent relative imports to be used, and always use absolute ones...

Comment on lines +15 to +16
def apply[F[_], A](f: (A, F[Attr[F, A]])): Attr[F, A] = f.asInstanceOf
def un[F[_], A](f: Attr[F, A]): (A, F[Attr[F, A]]) = f.asInstanceOf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this and the other data types, the main difference is the apply/un things.

I might try to reduce some of the duplication, but we'll see if that proves worthwhile

@@ -160,16 +181,22 @@ private[droste] sealed trait FloatingBasisInstances0[H[F[_], A] >: Basis[F, A]]
private[droste] sealed trait FloatingBasisSolveInstances {
import Basis.Solve

implicit val drosteSolveFix: Solve.Aux[Fix, λ[(F[_], α) => F[α]]] = null
implicit def drosteSolveAttr[A]: Solve.Aux[Attr[*[_], A], AttrF[*[_], A, *]] =
implicit val drosteSolveFix: Solve.Aux[Fix, ({ type L[F[_], A] = F[A] })#L] =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to desugar the type lambdas here because scala3's kind-projector syntax support doesn't work for type lambdas including higher-kinded types

Comment on lines +20 to +22
new (Algebra[F, *] ~> Id) {
def apply[A](fa: Algebra[F, A]): Id[A] = Mu.this.apply(fa)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

λ values aren't supported by scala3's -Ykind-projector support, so I had to desugar those as well.

@@ -96,7 +98,7 @@ class SchemePartialBasisTests extends Properties("SchemePartialBasis") {
if (n > 0) Mu(Some(expected(n - 1)))
else Mu(None: Option[Mu[Option]])

forAll((n: Int Refined Less[W.`100`.T]) => f(n) ?= expected(n))
forAll((n: Int Refined Less[100]) => f(n) ?= expected(n))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In scala3, the W witness syntax isn't supported, so I moved these to src/test/scala-2.13+ and used the literal type syntax instead.

Thanks to Rob Norris (as always) for help on that;
https://github.com/tpolecat/skunk/blob/main/build.sbt#L64-L73

@@ -167,10 +154,10 @@ object MakeChange {

val makeChangeAlgebra: CVAlgebra[Nat, Set[List[Coin]]] = CVAlgebra {
case Next(attr) =>
val given = fromNat(attr.forget) + 1
val _given = fromNat(attr.forget) + 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't use given as an identifier name in scala3

@@ -29,18 +29,20 @@ final class SmallPost extends Properties("SmallPost") {
}
}

implicit def streamFEmbed[A] = new Embed[StreamF[A, *], LazyList[A]] {
implicit def streamFEmbed[A]: Embed[StreamF[A, *], LazyList[A]] = new Embed[StreamF[A, *], LazyList[A]] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit vals must have types annotated

pepegar
pepegar previously approved these changes Jul 19, 2021
Copy link
Member

@pepegar pepegar left a comment

Choose a reason for hiding this comment

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

Terrific job @Daenyth, thank you very much!

).map(_ % "test"))
).map(_ % "test"),
// 'nowarn' doesn't work on scala3 yet, make warnings not fatal.
scalacOptions --= on(3)("-Xfatal-warnings").value
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Comment on lines +88 to +96
Compile / unmanagedSourceDirectories ++= {
val sourceDir = (Compile / sourceDirectory).value
CrossVersion.partialVersion(scalaVersion.value) match {
case Some((3, _)) => Seq(sourceDir / "scala-2.13+")
case Some((2, 12)) => Seq()
case Some((2, _)) => Seq(sourceDir / "scala-2.13+")
case _ => Seq()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

build.sbt Outdated Show resolved Hide resolved
- This doesn't include derivation macros yet
- I've tried to keep api changes to a minimum
- The various `data` types `apply`/`un` methods in scala3 use `.isInstanceOf` directly - this might be slower than the scala2 `fastCast` macro
By shamelessly copying code from `kittens`
Comment on lines +63 to +67
case e: Neg[V, A] => f(e.x) map (Neg(_))
case e: Add[V, A] => (f(e.x), f(e.y)) mapN (Add(_, _))
case e: Sub[V, A] => (f(e.x), f(e.y)) mapN (Sub(_, _))
case e: Prod[V, A] => (f(e.x), f(e.y)) mapN (Prod(_, _))
case e: Div[V, A] => (f(e.x), f(e.y)) mapN (Div(_, _))

Choose a reason for hiding this comment

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

As a BTW, are we still too keen on infix syntax?

Suggested change
case e: Neg[V, A] => f(e.x) map (Neg(_))
case e: Add[V, A] => (f(e.x), f(e.y)) mapN (Add(_, _))
case e: Sub[V, A] => (f(e.x), f(e.y)) mapN (Sub(_, _))
case e: Prod[V, A] => (f(e.x), f(e.y)) mapN (Prod(_, _))
case e: Div[V, A] => (f(e.x), f(e.y)) mapN (Div(_, _))
case e: Neg[V, A] => f(e.x) map (Neg(_))
case e: Add[V, A] => (f(e.x), f(e.y)).mapN(Add(_, _))
case e: Sub[V, A] => (f(e.x), f(e.y)).mapN(Sub(_, _))
case e: Prod[V, A] => (f(e.x), f(e.y)).mapN(Prod(_, _))
case e: Div[V, A] => (f(e.x), f(e.y)).mapN(Div(_, _))

specially if it saves no parentheses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm personally not keen on it, I just left things as they were mainly and how scalafmt left them

@@ -45,7 +45,7 @@ object ExprParser {
case TSub => shunt(op, 1).flatMap(_.traverse(enqueue))
case TProd => shunt(op, 2).flatMap(_.traverse(enqueue))
case TDiv => shunt(op, 2).flatMap(_.traverse(enqueue))
}).as(tail.asLeft)
}).map(_ => tail.asLeft)

Choose a reason for hiding this comment

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

I think it was proposed, at some point, as a replacement for the @ in pattern matching. I had some comments about that.

@Daenyth Daenyth merged commit ccbd2f6 into main Jul 20, 2021
@Daenyth Daenyth deleted the gb-scala3 branch July 20, 2021 12:17
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

3 participants