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

Add support for Polytypes #79

Merged
merged 4 commits into from
Mar 27, 2022
Merged

Add support for Polytypes #79

merged 4 commits into from
Mar 27, 2022

Conversation

valencik
Copy link
Contributor

@valencik valencik commented Feb 21, 2022

This PR adds a new case for PolyType in TPrintImpl.

Hopefully this resolves:

I've used @mrdziuban's scastie reproduction as a test case.

Disclaimer: I don't actually know what these PolyType's should look like.

We can test with the console via mill -i "pprint.jvm[2.13.4].console" and see what the compiler gives us:

scala> import scala.language.implicitConversions
     |
     | type Foo[+A] = Unit
     |
     | trait Bar[F[_], A]
     |
     | object Bar {
     |   implicit def anyToBar[A](a: A): Bar[Foo, A] = new Bar[Foo, A] {}
     |
     |   def apply[F[_], A](b: Bar[F, A]): Bar[F, A] = b
     | }
     |
     | def tprint[A](a: A)(implicit t: pprint.TPrint[A]) = t.render
import scala.language.implicitConversions
type Foo
trait Bar
object Bar
def tprint[A](a: A)(implicit t: pprint.TPrint[A]): fansi.Str

scala> tprint(Bar(1))
val res0: fansi.Str = Bar[Foo[A], Int]

scala> Bar(1)
val res1: Bar[[+A]Foo[A],Int] = Bar$$anon$1@794cb26b

As shown, the compiler prints Bar[[+A]Foo[A],Int] instead of our Bar[Foo[A], Int].
I feel like the [+A]Foo[A] is not usually how we'd write this, but happy to be corrected.

Update Mar 27th:

We now render Polytypes differently in Scala 2.12 versus 2.13:

Scala 2.13: Bar[[A]Foo[A], Int]
Scala 2.12: Bar[Foo, Int]

Disclaimer: I do not know if this is reasonable. It just so happens to work with minimal changes to PPrint.

@joroKr21
Copy link

joroKr21 commented Mar 26, 2022

I feel like the [+A]Foo[A] is not usually how we'd write this, but happy to be corrected.

Although this is not a type a user of Scala 2 can write, PolyTypes behave more or less like type lambdas and they can arise as result of compiler transformations (such as dealiasing) and they can also be created by macros.

def tprint[A](a: A)(implicit t: pprint.TPrint[A]) = t.render
}
val rendered = Print.tprint(Bar(1))
assert(rendered.toString() == "Bar[Foo[A], Int]")

Choose a reason for hiding this comment

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

Shouldn't it be

Suggested change
assert(rendered.toString() == "Bar[Foo[A], Int]")
assert(rendered.toString() == "Bar[[+A]Foo[A], Int]")

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've switched to Bar[[A]Foo[A], Int] for 2.13+, ditching the variance annotation as I don't think anything in PPrint uses the variance.

@@ -221,6 +221,12 @@ object TPrintLowPri{
.map(typePrintImplRec(c)(_, true))
.reduceLeft[fansi.Str]((l, r) => l ++ " with " ++ r)
(pre + (if (defs.isEmpty) "" else "{" ++ defs.mkString(";") ++ "}"), WrapType.NoWrap)
case PolyType(typeParams, resultType) =>
val params = typeParams.map(t => printArgSyms(t.asInstanceOf[TypeSymbol].typeParams)).mkString("")

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
val params = typeParams.map(t => printArgSyms(t.asInstanceOf[TypeSymbol].typeParams)).mkString("")
val params = printArgSyms(typeParams)

@valencik
Copy link
Contributor Author

In scalajs 2.12.13 only, we are now failing with:

   utest.AssertionError: assert(rendered.toString() == "Bar[[A]Foo[A], Int]")
  rendered: fansi.Str = Bar[Foo, Int]

I unfortunately can't run the failing tests locally. When I try mill "pprint.js[2.12.13,0.6.33].test" I get:

...
[96/96] pprint.js[2.12.13,0.6.33].test.test 
Starting process: node
1 targets failed
pprint.js[2.12.13,0.6.33].test.test java.lang.RuntimeException: Failed to get framework
    mill.scalajslib.worker.ScalaJSWorkerImpl.$anonfun$getFramework$2(ScalaJSWorkerImpl.scala:141)
    scala.Option.getOrElse(Option.scala:201)
    mill.scalajslib.worker.ScalaJSWorkerImpl.getFramework(ScalaJSWorkerImpl.scala:141)
    mill.scalajslib.ScalaJSWorker.getFramework(ScalaJSWorkerApi.scala:68)
    mill.scalajslib.TestScalaJSModule.$anonfun$testTask$1(ScalaJSModule.scala:254)
    mill.define.Task$TraverseCtx.evaluate(Task.scala:379)

I do have node installed. And mill "pprint.js[2.13.4,1.5.1].test" works fine.
I'm not familiar with mill and don't know what else to try here.

@valencik
Copy link
Contributor Author

Update: I was able to run the 2.12 tests locally with just mill "pprint.js[2.12.13,1.5.1].test" (not using 0.6.33)

@valencik
Copy link
Contributor Author

We're now passing CI! Thanks for all the help @joroKr21 and @lolgab 🎉

I've updated the PR description to call out the different rendering in 2.13 vs 2.12.
This PR is now ready for review.

Copy link
Member

@lolgab lolgab left a comment

Choose a reason for hiding this comment

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

LGTM! 👍
Thank you for the contribution!

@lolgab lolgab merged commit 2626e05 into com-lihaoyi:master Mar 27, 2022
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.

3 participants