-
Notifications
You must be signed in to change notification settings - Fork 151
Mix scala 2 and 3 macros #395
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR but can you please add a description for this that explains why we want this? |
@lloydmeta ah, thanks for taking a look. I've updated the description to clarify what's going on. |
@lloydmeta thanks for your patience with this. I just updated things again to hopefully resolve the build failures. I can't remember, were you manually triggering builds? |
Yea... I think I set something up once that requires me to approve the CI runs 🤦♂️ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 85.71% 85.30% -0.41%
==========================================
Files 63 65 +2
Lines 511 599 +88
Branches 34 51 +17
==========================================
+ Hits 438 511 +73
- Misses 73 88 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks for doing this.
There are quite a lot of changes that do not feel necessary, some of which are IMO a step backwards. My request would be to remove any changes that are not absolutely necessary.
.scalafmt.conf
Outdated
@@ -1,4 +1,4 @@ | |||
version = 3.7.8 | |||
version = 3.7.17 |
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.
Was this needed?
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.
Seems like no, I'll revert.
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.
reverted
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 changed this to 3.7.10 because without that update, in sbt ++3.3.4 test:compile
fails with:
[error] (coreJS / Compile / scalafmt) org.scalafmt.sbt.ScalafmtSbtReporter$ScalafmtSbtError: scalafmt: /home/gregg/lucid/enumeratum/enumeratum-core/src/main/scala-3/enumeratum/EnumCompat.scala:14: error: [dialect scala3] pattern must be a value or have parens: EnumMacros.findValuesImpl[A]
[error] protected inline def findValues: IndexedSeq[A] = ${ EnumMacros.findValuesImpl[A] }
[error] ^ [/home/gregg/lucid/enumeratum/enumeratum-core/src/main/scala-3/enumeratum/EnumCompat.scala]
there and a few other places.
build.sbt
Outdated
lazy val scala_2_13Version = "2.13.11" | ||
lazy val scala_3Version = "3.3.0" | ||
lazy val scala_2_13Version = "2.13.13" | ||
lazy val scala_3Version = "3.4.0" |
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.
Please revert.
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.
reverted
"org.scala-lang" %% "scala3-compiler" % scalaVersion.value % Provided | ||
Seq( | ||
"org.scala-lang" %% "scala3-compiler" % scalaVersion.value % Provided, | ||
"org.scala-lang" % "scala-reflect" % scala_2_13Version, |
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.
Curious if there's a way to make this additional dependency
- Contingent on actually using the mix mode macro
- Only a compile time constraint?
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.
Unfortunately I think only the consumer of the library knows if the mixed mode macro is being used, from here, sbt only knows that it's a scala3 build.
I believe we could mark this as Provided
too, but then all consumers would need to explicitly provide it. Marking it provided here means I also need to add it as Provided in core
. It would only need to be added as a normal/runtime dep if the project was using the mixed mode because I think this is only needed for the scala 2 portion of the build.
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.
@lloydmeta let me know if you have a preference 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.
Still having a think about this - has there been any better solution than the ones presented here? Adding another runtime dependency is not great.
Personally I think it's fine if the experience is subpar for "mixed mode" but not for Scala 2 or 3 users.
val base = { | ||
if (scalaBinaryVersion.value == "3") { | ||
minimal :+ "-deprecation" | ||
minimal :+ "-Wconf:cat=deprecation:s" |
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.
Curious why this was needed?
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.
[error] -- Error: /home/gregg/lucid/enumeratum/macros/src/main/scala/enumeratum/compat/EnumMacros.scala:109:32
[error] 109 | val enclosingModule = c.enclosingClass match {
[error] | ^^^^^^^^^^^^^^^^
[error] |method enclosingClass in trait Enclosures is deprecated since 2.11.0: c.enclosingTree-style APIs are now deprecated; consult the scaladoc for more information
and a few more identical errors
* aren't using this method... why are you even bothering with this lib? | ||
*/ | ||
inline def findValues: IndexedSeq[A] = ${ EnumMacros.findValuesImpl[A] } | ||
protected inline def findValues: IndexedSeq[A] = ${ EnumMacros.findValuesImpl[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.
Curious why this is now 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.
I'm not sure. I reverted it.
def entryName: String = stableEntryName | ||
|
||
private[this] lazy val stableEntryName: String = toString | ||
private lazy val stableEntryName: String = toString |
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.
Was this (and below) removals of this
needed?
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.
the scala 3 build fails without removing these (deprecated feature)
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.
This change has been reverted. It was originally made because Scala v3.4.0 deprecates [this]
access modifiers, but, as we're now using Scala v3.3.5 on master
, it's no longer necessary.
object Forms extends FormsCompat { | ||
|
||
protected[this] def formatter[ValueType, EntryType <: ValueEnumEntry[ | ||
protected def formatter[ValueType, EntryType <: ValueEnumEntry[ |
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 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.
This change has been reverted.
/** Finds any [A] in the current scope and returns an expression for a list of them | ||
*/ | ||
def findValuesImpl[A: c.WeakTypeTag](c: Context): c.Expr[IndexedSeq[A]] = { | ||
def findValuesImpl[A: c.WeakTypeTag](c: Context): c.Tree = { |
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.
Was this needed? Feels like a step backwards to lose type info, so if it's not required, please revert.
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.
See my comment here:
#395 (comment)
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 question regarding type change for everything 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.
See my comment here:
#395 (comment)
78ea80e
to
18c59d8
Compare
Hi, @lloydmeta! I've obtained @gregghz's permission to take over ownership of this PR. I've made a couple changes to it:
Regarding the modified return types in the macro methods ( Would you mind reviewing the PR another time whenever you get the chance? We really appreciate your help 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.
Pull Request Overview
This PR implements mixed Scala 2 and 3 macro support following the pattern from the Scala 3 migration guide. This allows projects using mixed Scala versions to depend on the Scala 3 version of enumeratum while building correctly for both Scala 2 and 3 modules.
- Creates a compatibility layer with macro implementations in the
compat
package - Updates return types from
c.Expr[IndexedSeq[A]]
toc.Tree
for cross-version compatibility - Adds Scala 2 macro fallback definitions alongside existing Scala 3 inline macros
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
macros/src/main/scala/enumeratum/compat/ValueEnumMacros.scala | Moves ValueEnumMacros to compat package and updates return types to c.Tree |
macros/src/main/scala/enumeratum/compat/EnumMacros.scala | Moves EnumMacros to compat package and updates return types to c.Tree |
enumeratum-core/src/main/scala-3/enumeratum/values/ValueEnumCompat.scala | Adds Scala 2 macro fallback definitions alongside Scala 3 inline macros |
enumeratum-core/src/main/scala-3/enumeratum/EnumCompat.scala | Adds Scala 2 macro fallback definitions alongside Scala 3 inline macros |
enumeratum-core/src/main/scala-2/enumeratum/values/ValueEnumCompat.scala | Updates import to use compat package macros |
enumeratum-core/src/main/scala-2/enumeratum/EnumCompat.scala | Updates import to use compat package macros |
macros/src/test/scala-2/enumeratum/FindValEnums.scala | Adds import for compat package |
macros/compat/src/main/scala-3 | Creates symlink to scala-2.13 for macro compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
weakTypeOf[ValueEntryType].baseClasses.contains( | ||
c.mirror.staticClass(classOf[AllowAlias].getName) | ||
) |
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.
[nitpick] The refactored type check using baseClasses.contains
and c.mirror.staticClass(classOf[AllowAlias].getName)
is more complex than the original <:<
subtype check. Consider if this change is necessary for Scala 2/3 compatibility or if the original approach could be preserved.
weakTypeOf[ValueEntryType].baseClasses.contains( | |
c.mirror.staticClass(classOf[AllowAlias].getName) | |
) | |
weakTypeOf[ValueEntryType] <:< weakTypeOf[AllowAlias] |
Copilot uses AI. Check for mistakes.
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's a similar problem here: the Scala 3 compiler is unable to summon WeakTypeTag
s
[error] -- [E172] Type Error: /home/jpeterson/lucid/enumeratum/macros/src/main/scala/enumeratum/compat/ValueEnumMacros.scala:108:61
[error] 108 | if (weakTypeOf[ValueEntryType] <:< weakTypeOf[AllowAlias]) {
[error] | ^
[error] | No WeakTypeTag available for enumeratum.values.AllowAlias
): c.Tree = { | ||
import c.universe._ | ||
val resultType = weakTypeOf[A] | ||
val indexedSeq = Ident(c.mirror.staticModule(classOf[IndexedSeq[A]].getName)) |
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.
Using classOf[IndexedSeq[A]].getName
with c.mirror.staticModule
is more fragile than the original reify(IndexedSeq).tree
approach. The classOf[IndexedSeq[A]]
will fail at runtime because A
is a type parameter. Consider using "scala.collection.immutable.IndexedSeq"
as a string literal instead.
val indexedSeq = Ident(c.mirror.staticModule(classOf[IndexedSeq[A]].getName)) | |
val indexedSeq = Ident(c.mirror.staticModule("scala.collection.immutable.IndexedSeq")) |
Copilot uses AI. Check for mistakes.
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 believe this change was necessary because we no longer have a WeakTypeTag[IndexedSeq[A\]
available.
This follows the pattern shown here: https://docs.scala-lang.org/scala3/guides/migration/tutorial-macro-mixing.html#:~:text=Introduction,during%20the%20macro%20expansion%20phase
This would allow a mixed scala version project to depend on the scala 3 version of enumeratum and build correctly for both scala 2 modules and scala 3 modules. This is particularly helpful when building with bazel since you can't conditionally include a dependency based on the current scala version the same way you would in sbt.
This was brought up previously by @coreywoodfield here: #300 (comment) (and he is the author of this PR -- I just rebased the branch).