-
Notifications
You must be signed in to change notification settings - Fork 149
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
Support Scala3 #349
Support Scala3 #349
Conversation
@@ -3,39 +3,5 @@ package enumeratum | |||
import org.scalatest.funspec.AnyFunSpec | |||
import org.scalatest.matchers.should.Matchers | |||
|
|||
class EnumJVMSpec extends AnyFunSpec with Matchers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor tests based call to compiler (not portable accross Scala version) with SBT code generation (portable and somewhat safer)
@@ -1,25 +0,0 @@ | |||
package enumeratum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced by SBT code generation
/** Created by Lloyd on 8/30/16. | ||
* | ||
* Copyright 2016 | ||
*/ | ||
class ValueEnumJVMSpec extends AnyFunSpec with Matchers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor call to compiler (not portable) using SBT code generation
fc58621
to
e539a9f
Compare
e539a9f
to
4ebb5cf
Compare
4ebb5cf
to
a3f9386
Compare
a3f9386
to
d7d07ac
Compare
sbt -v ++${{ matrix.scala }} test:compile test:doc | ||
sbt -v ++${{ matrix.scala }} test | ||
;; | ||
*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to fail there, SBT will anyway check whether the version selected by the CI is supported (and it avoid to hard code more cases)
|
||
import scala.language.experimental.macros | ||
|
||
private[enumeratum] trait EnumCompat[A <: EnumEntry] { _: Enum[A] => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-version compatibility trait for Enum[A]
, mainly due to macro migration
@@ -62,23 +61,16 @@ trait Enum[A <: EnumEntry] { | |||
*/ | |||
lazy final val valuesToIndex: Map[A, Int] = values.zipWithIndex.toMap | |||
|
|||
/** The sequence of values for your [[Enum]]. You will typically want to implement this in your |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the EnumCompat
cross-version compatibility trait
@@ -80,79 +75,25 @@ sealed trait ValueEnum[ValueType, EntryType <: ValueEnumEntry[ValueType]] { | |||
* and macro invocations cannot provide implementations for a super class's abstract method | |||
*/ | |||
|
|||
object IntEnum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the cross-version compatibility trait IntEnumCompanion
|
||
/** Method that returns a Seq of [[A]] objects that the macro was able to find. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the cross-version compatibility trait LongEnumCompat
build.sbt
Outdated
case _ => | ||
throw new IllegalArgumentException(s"Unsupported Scala version $scalaVersion") | ||
throw new IllegalArgumentException(s"Unsupported Scala version $scalaVersion for Doobie") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better error message
build.sbt
Outdated
lazy val coreJS = core.js | ||
lazy val coreJVM = core.jvm | ||
|
||
def configureWithLocal(m: Project): Project => Project = { p => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utility to use local project/dependency rather than stable/previous version dependency
IO.writer[Seq[File]](ef, "", IO.defaultCharset, false) { w1 => | ||
// Generate enum file | ||
|
||
w1.append(s"""package generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generate an Enum
per each iteration
|
||
w2.append(s"""package generated | ||
|
||
trait ${enumName}Test { _spec: EnumBaseSpec with enumeratum.EnumJVMSpec => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generate associated test trait
|
||
// --- | ||
|
||
private def generateValueEnumTest[A]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for ValueEnum
defd7f6
to
b857103
Compare
@@ -1,18 +0,0 @@ | |||
package enumeratum.values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to duplicate those fixtures, rather provide them from coreX % "compile->compile;test->test"
as they are already used in tests there
) | ||
|
||
lazy val macrosJS = macros.js | ||
lazy val macrosJVM = macros.jvm | ||
|
||
lazy val useLocalVersion = sys.props.get("enumeratum.useLocalVersion").nonEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag not to try to use a previous stable version
val dep = "org.scalatest" %%% "scalatest" % scalaTestVersion % Test | ||
|
||
if (scalaBinaryVersion.value == "3") { | ||
dep.exclude("org.scala-lang.modules", "*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid conflict when mixing Scala2.13/3 for compatibility
b857103
to
e91a56b
Compare
e91a56b
to
94a2589
Compare
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
- Coverage 90.37% 85.76% -4.61%
==========================================
Files 63 63
Lines 530 527 -3
Branches 8 24 +16
==========================================
- Hits 479 452 -27
- Misses 51 75 +24
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -13,15 +13,18 @@ jobs: | |||
- java: 11 | |||
scala: 2.11.12 | |||
- java: 11 | |||
scala: 2.12.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update covered scala versions
build.sbt
Outdated
lazy val scala_2_12Version = "2.12.14" | ||
lazy val scala_2_13Version = "2.13.6" | ||
lazy val scalaVersionsAll = Seq(scala_2_11Version, scala_2_12Version, scala_2_13Version) | ||
lazy val scala_2_12Version = "2.12.16" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update covered scala versions
aggregate in publish := false, | ||
aggregate in PgpKeys.publishSigned := false | ||
// doctestWithDependencies := false, // sbt-doctest is not yet compatible with this 2.13 | ||
publish / aggregate := false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix deprecated syntax
.settings( | ||
version := "1.7.1-SNAPSHOT", | ||
version := Versions.Core.head, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align versions
@@ -5,7 +5,7 @@ import org.scalacheck.{Arbitrary, Gen} | |||
trait ArbitraryInstances { | |||
|
|||
implicit def arbEnumEntry[EnumType <: EnumEntry](implicit | |||
enum: Enum[EnumType] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid enum
naming
@@ -15,65 +15,73 @@ object EnumHandler { | |||
|
|||
/** Returns a BSONReader for a given enum [[Enum]] | |||
* | |||
* @param enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid reversed keyword enum
as naming
@@ -12,29 +12,29 @@ object EnumHandler { | |||
* Enum's value type. | |||
*/ | |||
def reader[ValueType, EntryType <: ValueEnumEntry[ValueType]]( | |||
enum: ValueEnum[ValueType, EntryType] | |||
@deprecatedName(Symbol("enum")) e: ValueEnum[ValueType, EntryType] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid reversed keyword enum
as naming
|
||
/** @author | ||
* Alessandro Lacava (@lambdista) | ||
* @since 2016-04-23 | ||
*/ | ||
sealed trait ReactiveMongoBsonValueEnum[ValueType, EntryType <: ValueEnumEntry[ValueType]] { | ||
enum: ValueEnum[ValueType, EntryType] => | ||
selfEnum: ValueEnum[ValueType, EntryType] => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid reversed keyword enum
as naming
|
||
/** @author | ||
* Alessandro Lacava (@lambdista) | ||
* @since 2016-04-23 | ||
*/ | ||
trait EnumBsonHandlerHelpers { this: AnyFunSpec with Matchers => | ||
trait ValueEnumBsonHandlerHelpers { this: AnyFunSpec with Matchers => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid EnumBsonHandlerHelpers
in several packages (misleading error messages)
|
||
def testWriter[EntryType <: ValueEnumEntry[ValueType], ValueType]( | ||
enumKind: String, | ||
enum: ValueEnum[ValueType, EntryType], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid reversed keyword enum
as naming
| import ctx._ | ||
| ctx.run(query[QuillShirt].insert(_.size -> lift(QuillShirtSize.Small: QuillShirtSize))) | ||
""".stripMargin should compile | ||
import io.getquill._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only string literal is supported by should compile
addSbtPlugin("com.github.tkawachi" % "sbt-doctest" % "0.9.9") | ||
addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.1.2") | ||
addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.9.0") | ||
resolvers += Classpaths.sbtPluginReleases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update plugins
|
||
def mappedColumnTypeForEnum[E <: EnumEntry]( | ||
enum: Enum[E] | ||
@deprecatedName(Symbol("enum")) e: Enum[E] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slick not yet ready, but prepare this code for a further migration
85ff7bd
to
bb5e42e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just finished reviewing the Quill, Reactive,and Slick-related changes; those all LGTM. Next round will be looking at the Scala3 macro-code, which at a glance looks quite good.
Again, apologies for the slow/intermittent process here 🙇♂️
This is one of the most important PR that unblocks our projects for scala-3 migration Thanks @cchantep and @lloydmeta for all the work 👍 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally finished making my way through. Overall, LGTM
@cchantep I had a few questions about the TODOs still in the Scala 3 code that I would love your input on. My main concern there would be whether or not they are really todos and what the consequences are for leaving them as TODOs.
)(using tpe: Type[T]): List[q.reflect.TypeRepr] = { | ||
import q.reflect.* | ||
|
||
// TODO: Use SumOf? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchantep can you please shed some light on this todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
child.asType match { | ||
case '[IsEntry[t]] => { | ||
val tpeSym = child.typeSymbol | ||
// TODO: Check is subtype (same in Scala2?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchantep :c
Curious if this is already verified by the type match + compiler because of the type alias above of IsEntry[E <: T]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
tpr.classSymbol | ||
.flatMap { cls => | ||
// TODO: cls.typeMembers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchantep is this todo still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
import q.reflect.* | ||
|
||
val ctx = q.asInstanceOf[scala.quoted.runtime.impl.QuotesImpl].ctx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a pattern match on the type and report.errorAndAbort
if not the expected class be nicer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see what you mean.
As indicated thereafter (L125), the settings must be check to be sure the tree can be inspected.
val moduleField = cls.getFields.find(_.getName == f"MODULE$$") | ||
|
||
moduleField.map { field => | ||
new ValueOf(field.get(null).asInstanceOf[T]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field.get(null).asInstanceOf[T]
is interesting; @cchantep can you please shed some light on this?
Ultimately this is could just be my ignorance of the Scala3 macro world, but it's odd to see two "Scala taboos" consecutively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not metaprogramming, it's low level reflection, as current metaprogramming doesn't cover such case.
I was testing this PR to see if I can migrate my existing usage of enumeratum to the scala-3 version and I found a few issues. I will try to post them in separate comments below. @cchantep @lloydmeta |
Issue-1//> using scala "3.2.1"
//> using repository "jitpack"
//> using lib "com.github.mushtaq.enumeratum::enumeratum:be1c8d6"
import enumeratum.{Enum, EnumEntry}
import scala.collection.immutable
sealed trait Base extends EnumEntry
class Derived extends Base
object Base extends Enum[Base] {
override def values: immutable.IndexedSeq[Base] = findValues // error in compilation
case object E1 extends Base
case object E2 extends Base
}
|
Issue-2//> using scala "3.2.1"
//> using repository "jitpack"
//> using lib "com.github.mushtaq.enumeratum::enumeratum:be1c8d6"
import enumeratum.{Enum, EnumEntry}
import scala.collection.immutable
sealed trait Base extends EnumEntry
object Base extends Enum[Base] {
class Derived extends Base
override def values: immutable.IndexedSeq[Base] = findValues
case object E1 extends Derived
case object E2 extends Derived
}
object Demo {
def main(args: Array[String]): Unit = {
println(Base.values) // prints empty vector
}
}
|
These are self contained scala-cli scripts which work with scala 2.13 and current published version of enumeratum. The artifact referred in the script is jitpack published from a fork of this PR |
@mushtaq Cannot reproduce Issue 1, check your env. Issue 1: sbt:enumeratum> console
Welcome to Scala 3.2.1-RC1 (9.0.1, Java Java HotSpot(TM) 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.
scala> :paste
Unknown command: ":paste", run ":help" for a list of commands
scala> import enumeratum.{Enum, EnumEntry}
| import scala.collection.immutable
|
| sealed trait Base extends EnumEntry
|
| class Derived extends Base
|
| object Base extends Enum[Base] {
| override def values: immutable.IndexedSeq[Base] = findValues // error in compilation
|
| case object E1 extends Base
| case object E2 extends Base
| }
// defined trait Base
// defined class Derived
// defined object Base
scala>
scala> Base.values
val res0: IndexedSeq[Base] = Vector(E1, E2) |
@cchantep Have you tried putting the test in .scala file? Using either REPL or a .sc file will not reproduce the issue. To make it easy, I put the issues in gists. You can run them like so: $ scala-cli https://gist.github.com/mushtaq/16705977494d73a3d921f53ffe5c7a32
Compiling project (Scala 3.2.1, JVM)
[error] 16705977494d73a3d921f53ffe5c7a32-main/issue1.scala:13:53:
The enum (i.e. the class containing the case objects and the call to `findValues`) must be an object
Error compiling project (Scala 3.2.1, JVM)
Compilation failed
$ scala-cli https://gist.github.com/mushtaq/d643b306e22464ab13e637d3971da3aa
Vector() |
@mushtaq In REPL or in a |
Interesting! I am Scala 3.2.1 with Java-17 on MacOS. Maybe something to do with that combination. Can you please share what is the output of this command on your machine:
|
@mushtaq Rather test it SBT/Scala REPL. |
219a8ca
to
bf8f4fe
Compare
Thanks! |
Sure, here is the github repo with sbt to reproduce the issue-1.
[error] -- Error: /Users/mushtaqahmed/projects/scala3/enumeraturm-compilation-issue/src/main/scala/Main.scala:9:52
[error] 9 | override def values: immutable.IndexedSeq[Base] = findValues // error in compilation
[error] | ^^^^^^^^^^
[error] |The enum (i.e. the class containing the case objects and the call to `findValues`) must be an object |
bf8f4fe
to
ee55e85
Compare
Issue-1 is fine |
@mushtaq Issue is fixed and covered by the indicated test |
sealed trait Word extends EnumEntry | ||
|
||
object Word extends Enum[Word] { | ||
sealed class Greeting extends Word |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove sealed
from class Greeting
, the test will fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchantep Notice that class Derived
is not sealed in my example.
To reproduce the issue-1, you should remove sealed
from class Greeting
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having sealed
cannot (and should not) be supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works perfectly well with scala-2.13 and enumeraturm 1.7.0 as shown in this gist:
$ scala-cli https://gist.github.com/mushtaq/5055834b968bf17c84bfa28f36980924
Compiling project (Scala 2.13.10, JVM)
Compiled project (Scala 2.13.10, JVM)
Vector(E1, E2)
Note that Base
is still sealed
.
Derived
does not require to be sealed, because we are creating an Enum[Base]
and not an Enum[Derived]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mushtaq That it works with 2.13 doesn't give it more sense.
- You may be lucky that Scala 2.13 find subobjects, but it has always add limitation for the macros to find subtypes.
- Such modeling looks like an edge one for me that I would not encourage, and even prevent and detect as erroneous.
- I won't take more time on such case, so feel free to block this PR and close it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchantep I would love this PR to be merged ASAP because it is so useful to have enumeratum on scala-3.
The issues pointed out here are not blockers because I already have found workarounds.
I am sharing them just so that you or @lloydmeta may want to make use of them!
Thanks a lot once again for your responsiveness 👍
Signed-off-by: lloydmeta <[email protected]>
Signed-off-by: Lloyd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
EnumMacros
andValueEnumMacros