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

Mix Scala 2 macros in MUnit Scala 3 module #283

Merged
merged 8 commits into from
Jan 6, 2021

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Jan 5, 2021

With this change, Scala 2.13 users will be able to depend on munit_3.0
artifacts instead of munit_2.13.

I wasn't able to manually test this change because MUnit doesn't
cross-build for 3.0.0-M1, which is the latest version supported by the
-Ytasty-reader flag in 2.13.4. Once 2.13.5 is out, it will be easier
ot make use of this feature.

@olafurpg olafurpg requested a review from tgodzik January 5, 2021 17:26
authorImageURL: https://github.com/olafurpg.png
---

The next release of MUnit makes use of a new compiler feature that allows you to
Copy link
Member Author

Choose a reason for hiding this comment

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

cc/ @bishabosha @sjrd could you please take a look at this blog post? Please correct me if I got the details wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Live preview at https://olafurpg.github.io/munit/blog/2021/01/05/macromix.html so you can see the rendered SVG files.

With this change, Scala 2.13 users will be able to depend on munit_3.0
artifacts instead of munit_2.13.

I wasn't able to manually test this change because MUnit doesn't
cross-build for 3.0.0-M1, which is the latest version supported by the
`-Ytasty-reader` flag in 2.13.4. Once 2.13.5 is out, it will be easier
ot make use of this feature.
Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

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

Really cool!

What's the longer plan though? Do we still publish MUnit also for 2.13? If not, I assume Scala 2 users will need to use the full artifact name in sbt to depend on the 3.0 artifact? Either way it may be good to add some notes about this in the user documentation, what do you think?

build.sbt Outdated Show resolved Hide resolved
* Avoid copy-pasted code for Scala 2 macros.
Makes it easier to understand the console output while compiling with
Scala 3.
@olafurpg
Copy link
Member Author

olafurpg commented Jan 6, 2021

@gabro thank you for the review!

What's the longer plan though? Do we still publish MUnit also for 2.13? If not, I assume Scala 2 users will need to use the full artifact name in sbt to depend on the 3.0 artifact? Either way it may be good to add some notes about this in the user documentation, what do you think?

I was thinking of keeping things unchanged. I updated the post to explain that MUnit 2.13 will continue to be published.

The primary purpose of this post is to advocate for this route since I don't everybody are fully aware of the risks of cross-building 2.13 and 3.

upcoming Scala 2.13.5 release will be able to understand newer Scala 3.x
releases.
- Some common features in Scala 2 macros don't work in Scala 3. Most notably,
you can't use quasiquotes or macro bundles. In the case of MUnit, we had to
Copy link

@bishabosha bishabosha Jan 6, 2021

Choose a reason for hiding this comment

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

I was confused to find that I could compile a macro bundle using Scala 3.0.0-M1 but the macro expansion failed from Scala 2.13.4 (and not when compiled by Scala 2.13.4), despite the macro bundle having the same bytecode when compiled by either 2.13.4 or 3.0.0-M1, and the tasty to call the macro was the same either way. Something to do with reflective invocation not having the required number of arguments.

It's probably worth opening an issue with the trouble you found, because I don't see at this moment why it should fail.

my example was the following:

class TpeTagImpl(val c: Context) {
  import c.universe._

  // factory for `final case class TpeTag[T](repr: String)`
  // invoked by `implicit def mkTpeTag[T]: TpeTag[T] = macro TpeTagImpl.mkTag[T]`
  def mkTag[T](implicit T: c.WeakTypeTag[T]): Tree = {
    val tpeString = Literal(Constant(T.tpe.toString))
    // new testframework.TpeTag[T](T.tpe.toString)
    New(appliedType(c.mirror.staticClass(classOf[testframework.TpeTag[_]].getName()).toType, T.tpe), tpeString)
  }

}

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the macro bundle mention. I didn't try using bundles, I just assumed they wouldn't work because I thought they were implemented with macroparadise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I confused bundles with https://github.com/milessabin/macro-compat, which is implemented as a macro annotation to add bundle support in Scala 2.11

Choose a reason for hiding this comment

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

This is very nice work, I will open the issue then for my example.

@olafurpg olafurpg merged commit 566431d into scalameta:master Jan 6, 2021
@olafurpg olafurpg deleted the scala-3 branch January 6, 2021 10:48
Copy link

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

A bit late, but FWIW the blog post LGTM.

@olafurpg
Copy link
Member Author

olafurpg commented Jan 6, 2021

@sjrd whoops 😅 I should have waited for your LGTM before merging.

@sjrd
Copy link

sjrd commented Jan 6, 2021

No worries. I could have looked at this earlier this morning. ;)

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.

4 participants