Skip to content

Support Scala 3 #226

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

Merged
merged 23 commits into from
Dec 2, 2021
Merged

Support Scala 3 #226

merged 23 commits into from
Dec 2, 2021

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Nov 10, 2021

This PR is built on top of the commits by @edwardcwang.

Changes:

  • Add Mima to check how much we break binary compatibility.
  • Update Mill to version 0.9.10 and fix build deprecations
  • Add scalaMajorVersion in buildInfoMembers since Scala 3 represents types wrapped inside () in error messages.
  • Explicitly apply implicit function ev: A => Frag in various implicit classes (like SeqFrag) since Scala 3 doesn't apply implicitly.
  • Remove protected[this] val RawFrag: Companion[RawFrag] and protected[this] val StringFrag: Companion[StringFrag] since they break variance and Scala 3 is much less permissive with variance. Overriding them with strictier types in subclasses result in compiler errors. Since this was the only use case of Companion I removed it as well. Also I failed to represent Companion in Scala 3 since it uses different signatures for unapply
  • Add explicit types to all implicit fields as required by Scala 3.
  • Replace def +(other: Any): String in class String with String interpolator since it is not supported in Scala 3
  • Make all abstract members in Tags.scala, Tags2.scala and SvgTags.scala defs instead of vals since Scala 3 doesn't support overriding an abstract val member with a lazy val as Scalatags does.
  • Move the SourceClasses class and its companion object to the version specific code. Since it is the only code using macros.
  • Updated tests to not use the * - utest syntax. Also not supported in Scala 3.
  • Move objects used in tests to be stable and not defined in the utest macro. Without this change the Scala 3 version doesn't compile.

edwardcwang and others added 5 commits November 10, 2021 21:49
There is a bug in the dotty compiler? scala/scala3#13518

scalatags.js[3.0.1,1.7.0].compile java.lang.AssertionError: assertion failed
    scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:11)
    dotty.tools.backend.sjs.JSCodeGen.genStringConcat(JSCodeGen.scala:2487)
    dotty.tools.backend.sjs.JSCodeGen.genPrimitiveOp(JSCodeGen.scala:2098)
@edwardcwang edwardcwang mentioned this pull request Nov 11, 2021
@@ -32,58 +32,58 @@ trait DataConverters{
*
* MDN
*/
def px = x + "px"
def px = x.toString + "px"

Choose a reason for hiding this comment

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

Imo this would look a little nicer as

def px = s"${x}px"

Just personal opinion though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, agree!

inline def apply[T <: StyleSheet]: SourceClasses[T] = $ { manglerImpl[T] }

def manglerImpl[T <: StyleSheet: Type](using Quotes): Expr[SourceClasses[T]] = {
' { new SourceClasses[T]((t: T) => ${ Expr.ofSeq(terms('t)) }) }

Choose a reason for hiding this comment

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

Github's code highlighting doesn't seem to be aware of the new quotes syntax 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be a good bug report over at https://github.com/scala/vscode-scala-syntax/issues (repo name says "vscode", but it also powers highlighting on GitHub)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestion @SethTisue 🙂
I opened an issue as you suggested and opened also a PR with a fix.
Issue: scala/vscode-scala-syntax#226, PR: scala/vscode-scala-syntax#227

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow even better!

Comment on lines 143 to 145
// Scala 3 behaviour prevents us from using the same name as the case
// class for some reason, thus also preventing us from using the
// auto-generated companion object.
Copy link
Contributor

Choose a reason for hiding this comment

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

It works fine in 3.0.2, not sure what was the problem here.

@sake92
Copy link
Contributor

sake92 commented Dec 1, 2021

Seems like trait Companion[V] extends (String => V) is redundant if we have the def apply already.
It could be just trait Companion[V] I guess.

Then we need to new RawFrag(target) etc in VirtualDom, because scala3 has creator-applications

/**
* A [[Modifier]] which contains a String which will not be escaped.
*/
protected[this] type RawFrag <: Modifier
protected[this] val RawFrag: Companion[RawFrag]
Copy link
Member Author

@lolgab lolgab Dec 1, 2021

Choose a reason for hiding this comment

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

I removed this and all the tests pass.
The way unapply works in Scala 3 is different from Scala 2 and it's hard to write it generically.
Does this have an usage @lihaoyi ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is generally used. People construct raw frags via raw(...), and generally we do not support or encourage de-constructing frags: they are to be constructed and rendered, with no other operations in between.

Thus we can probably do without this if it's too difficult. If removing it doesn't break the test suite, then we it probably won't be missed

@lolgab
Copy link
Member Author

lolgab commented Dec 1, 2021

Seems like trait Companion[V] extends (String => V) is redundant if we have the def apply already. It could be just trait Companion[V] I guess.

Then we need to new RawFrag(target) etc in VirtualDom, because scala3 has creator-applications

I tried removing Companion altogether and tests pass just fine. Maybe we can get rid of it.
The failure was because Scala 3 handles unapplies differently from Scala 2.

@lolgab lolgab marked this pull request as ready for review December 1, 2021 19:24
@lolgab
Copy link
Member Author

lolgab commented Dec 1, 2021

Tests pass now 🎉

@lolgab lolgab changed the title Support scala 3 second attempt Support Scala 3 Dec 1, 2021
@lihaoyi
Copy link
Member

lihaoyi commented Dec 2, 2021

@lolgab could you provide a verbose PR description of what the major changes are and why they are necessary? That would be a lot easier to review than trying to reverse engineer your intent from the raw code diff

@lolgab
Copy link
Member Author

lolgab commented Dec 2, 2021

@lihaoyi Reverted some changed that are not necessary for Scala 3 tests to pass.
Also added a list of changes in the description trying to explain the whys of the various changes.

@jodersky
Copy link
Member

jodersky commented Dec 2, 2021

Move objects used in tests to be stable and not defined in the utest macro. Without this change the Scala 3 version doesn't compile.

Do you know if this is an inherent limitation of the utest macro, or is it something that could be fixed in utest for scala 3? Not a blocker, but I'm just curious

@lolgab
Copy link
Member Author

lolgab commented Dec 2, 2021

@jodersky I don't know, sorry.

@sake92
Copy link
Contributor

sake92 commented Dec 2, 2021

@lolgab don't these scala specific folders have to be src-2.12 etc?
https://com-lihaoyi.github.io/mill/mill/Common_Project_Layouts.html#_cross_scala_version_modules

Co-authored-by: Sakib Hadžiavdić <sake92@users.noreply.github.com>
@lolgab
Copy link
Member Author

lolgab commented Dec 2, 2021

@lolgab don't these scala specific folders have to be src-2.12 etc? https://com-lihaoyi.github.io/mill/mill/Common_Project_Layouts.html#_cross_scala_version_modules

Mill splits the version by . and adds all prefixes as supported versions.
src-2, src-2.13 and src-2.13.4 are supported.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 2, 2021

I think this generally looks good to me. @sake92 I know you've made some efforts around Scala3, if you could give this pR your approval then I think we're good to merge

@sake92
Copy link
Contributor

sake92 commented Dec 2, 2021

@lihaoyi Yep, all looks good I think. :shipit:

@lihaoyi
Copy link
Member

lihaoyi commented Dec 2, 2021

Great. @lolgab go ahead and merge whenever you're ready :)

@lolgab lolgab merged commit 0cfff0c into com-lihaoyi:master Dec 2, 2021
@lolgab lolgab deleted the support-scala-3-second-attempt branch December 2, 2021 14:37
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.

7 participants