-
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
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.
✅ Project coverage is 85.30%. Comparing base (636ccdf) to head (18c59d8).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
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
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
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
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.
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
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.
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.
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.
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:
- I rebased it off of the latest version of
master - I reverted a couple commits that weren't necessary
- I reverted the changes to library/compiler versions, since those have been updated on
mastersince this PR was originally opened
Regarding the modified return types in the macro methods (c.Expr[???] to c.Tree), this change was made because the Scala 3 compiler is unable to summon WeakTypeTags, so it's incompatible with mixed macro compilation. However, I'm hopeful that with comprehensive testing, this won't be much of a problem. If you disagree, please let me know and we can explore other ways to maintain specific type information.
Would you mind reviewing the PR another time whenever you get the chance? We really appreciate your help here.
@Copilot
Copilot
AI
left a 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.
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
compatpackage - Updates return types from
c.Expr[IndexedSeq[A]]toc.Treefor 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.
Copilot
AI
Sep 20, 2025
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.
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 WeakTypeTags
[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
Copilot
AI
Sep 20, 2025
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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).