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

WIP: Migrate quasiquote macros to Scala 3 #3347

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jchyb
Copy link

@jchyb jchyb commented Sep 29, 2023

This draft PR is meant to serve as a signal of progress made in migrating scalameta quasiquote macros to Scala3. Apologies for taking up the PR tracker with another draft PR. I will be unavailable for a few weeks, but I intend to come back to this PR after that time and finish it. It is very much not ready for review.

The aim here was to migrate only the macro methods used for defining quasiquotes, leaving all of the codegen-by-macro-annotations stuff in scala 2 for now. So far, I managed to get some quasiquote apply methods working, although sometimes the resolved type is incorrect (requiring asInstanceOf, as seen in the tests). The unapply methods do need some more work. Of course, things like the build structure etc. are not final.

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 2, 2023

I tried to come up with a way to publish just the parsers module, but that still has a bunch of errors when using fastparse and also there is a small macro that we would need to rewrite 🤔

https://github.com/scalameta/scalameta/compare/main...tgodzik:scalameta:cross-publish?expand=1

If successful it would be possible to just depend on parsers_3 and get basically everything we need. I will try and see if I can fix things there.

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 4, 2023

Ok it seems I managed to progress further with the above branch. I also merged two PRs the reduce the number of errors when cross compiling. I am a bit stuck at adding the AstInfo macro, if you could help, we could merge that first and then quasiquotes.

I am currently stuck at:

  • writing the AstInfo macro for Scala 3
  • a new error that only popped up when moved the Scala 2 macro to a separate package:
[error] java.lang.StackOverflowError
[error]         at scala.reflect.internal.Symbols$Symbol.isPrimaryConstructor(Symbols.scala:960)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.withOuter$1(Contexts.scala:1143)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.implicitssImpl(Contexts.scala:1166)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.withOuter$1(Contexts.scala:1145)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.implicitssImpl(Contexts.scala:1166)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.withOuter$1(Contexts.scala:1145)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.implicitssImpl(Contexts.scala:1166)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.withOuter$1(Contexts.scala:1145)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.implicitssImpl(Contexts.scala:1166)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.withOuter$1(Contexts.scala:1145)
[error]         at scala.tools.nsc.typechecker.Contexts$Context.implicitssImpl(Contexts.scala:1166)

If anyone knows why this would pop up I would be super grateful. Otherwise, we might need to keep the macro in trees and only add the Scala 3 macro to the new macro module (though not sure if that would work)

Let's talk after you are back from holidays

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 5, 2023

I decided to instead remove the problematic AstInfo Macros and we should be able to cross compile part of Scalameta soon https://github.com/scalameta/scalameta/compare/main...tgodzik:scalameta:cross-publish2?expand=1 <- works already with parsers module

@jchyb
Copy link
Author

jchyb commented Jan 3, 2024

Progress update:
All the apply methods should work now, and unapply methods for quasiquotes are also implemented. There are some issues, however:

  • unapply methods generated with trees involving Some crash the compiler for now. I've submitted the issue into the compiler (and will probably look more closely into it later on, Some in a macro reflect-based unapply method can crash the compiler scala/scala3#19362). I have a workaround in mind, as previously when I had an implicit dialect (that I had to remove for unrelated reasons) in unapply macro method entry point, the crash didn't trigger.
  • making transparent inline unapply methods work will crash the compiler in 3.3.1, but I see the fix for this is already backported to 3.3.2, also for some reason transparent inline methods get executed twice (I've still yet to minimize and submit this issue into the compiler)
  • since unapply is not transparent for now, the compiler is not able to type the returned values correctly (we always obtain Any and have to cast it ourselves). I've adjusted the tests to reflect that (pretty much every single test was changed because of that).
  • I've never managed to obtain type ascriptions from unapply methods in Scala 3. There was a feature here, where we could return the correct type depending on the type ascription (e.g. we could either return Term.ArgParam or List[Term]). The feature is ported, but unused since we are missing those ascriptions. I managed to make this work only for refutable extractors like val q"def method(args: Term.ArgClause)" = q"def method(a: Int, b: Double)", but it will only work for non-transparent inline unapply (which we want to make transparent, for reasons stated in the bullet points above). It also does not work for unapplies in pattern matches at all. Nevertheless, I've left this experiment in the code, commented out (with the "EXPERIMENT" label).
  • The above also means that we are not able to throw any compilation errors if users use the wrong ascription in a quasiquote, this may be fixed by making the unapply method transparent when that becomes available

@tgodzik
Copy link
Collaborator

tgodzik commented Jan 4, 2024

Thanks for the great work! I will have to go over it and ask some questions since it's something I haven't dealt with yet.

I almost finished with making scalameta release without quasiquotes for Scala 3, I need to tidy it up and figure out some smaller sbt issues.

unapply methods generated with trees involving Some crash the compiler for now.

So only Boolean ones were working previously? There were no tests for that?

since unapply is not transparent for now, the compiler is not able to type the returned values correctly (we always obtain Any and have to cast it ourselves). I've adjusted the tests to reflect that (pretty much every single test was changed because of that).

Looks like we should wait for 3.3.2 for that? It would be super useful to have the correct types here without a need for casts. I think we can wait for the next release since it should be soon.

I've never managed to obtain type ascriptions from unapply methods in Scala 3.

Which feature do you mean? Do we have it in a test suite somewhere?

}

// TODO should be generated by a custom codegen script
trait ExprLifts(using override val internalQuotes: Quotes)(isPatternMode: Boolean) extends ReificationMacros with InternalTrait {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this generated previously? Why do we need so much more in Scala 3?

Copy link
Author

@jchyb jchyb Jan 4, 2024

Choose a reason for hiding this comment

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

It was all generated by a macro triggered inside ReificationMacros.scala referencing scalameta/common/shared/src/main/scala/scala/meta/internal/trees/Liftables.scala (`materializeAst[]')

}

// unapply methods
extension (stringContext: scala.StringContext)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to repeat the methods here?

Copy link
Author

Choose a reason for hiding this comment

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

they have different names, and those names point to different parsers in ReificationMacros

}
object XTensionQuasiquoteSource {
private[meta] def parse(input: scala.meta.inputs.Input, dialect: scala.meta.Dialect) = {
val parse = implicitly[Parse[Source]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is all repeated could we not have a method:

def parseImpl[T](using parse: Parse[T] )(input: scala.meta.inputs.Input, dialect: scala.meta.Dialect) =
{ 
      parse(input, dialect)
    } match {
      case x: scala.meta.parsers.Parsed.Success[_] => x.tree
      case x: scala.meta.parsers.Parsed.Error => throw x.details
    }

and use it everwhere?



// parsers
object XTensionQuasiquoteTerm {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are those parsers used? I don't see any explicit usage.

Copy link
Author

Choose a reason for hiding this comment

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

In ReificationMacros in instantiateParser(), they are classloaded and invoked (just like in scala 2). In scala 2 this boilerplate was hidden inside classes with a macro annotation

}

// test("deconstruction ascriptions") {
// val q"foo(..${xs: List[Term]})" = q"foo(x, y)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ach, I see where we have the type ascriptions, I wasn't even sure it was possible. Would be good to support but if it's too hard we can leave it out.

Copy link
Author

Choose a reason for hiding this comment

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

without a new SIP it is impossible unfortunately

Copy link
Collaborator

@tgodzik tgodzik Jan 4, 2024

Choose a reason for hiding this comment

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

It's fine to drop that functionality for Scala 3 then

import meta.prettyprinters.XtensionSyntax

class Success extends TreeSuiteBase {
test("rank-0 liftables") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this the same test suite like for Scala 2? Could we reuse it? Or would it need my PR first I guess?

Copy link
Author

Choose a reason for hiding this comment

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

For now it has tests slightly changed to accommodate the Any types returned from the unapply. I hope to use the old suite once we can get transparent inline unapply working, outside of those 2 tests with non-standard type ascriptions of course

@jchyb
Copy link
Author

jchyb commented Jan 4, 2024

  1. It's unrelated to what we return from the unapply, but concerns how the unapply method is implemented. In our unapply macro method we create a custom pattern match using the reflection API (where we basically create the program tree ourselves). The crash seems to trigger when we use an UnApply node for Some in that program tree as part of a macro expansion of an unapply method (pretty specific case, and finding the cause here was sure something...).
    Curiously, the crash does not trigger when rewriting the Some pattern match into a quoted expression ('{...}) (by converting I mean hardcoding a single macro method output, in general it is impossible for us to redesign the macro to be able to use quoted expressions like this). When converting that quoted expression to a program tree, the compiler wraps UnApply nodes with additional Typed nodes, which I suspect are related why it is stopping crashing in that case. We cannot use the Typed nodes in the same way in the reflection API by design, but other cases of UnApply nodes without the Typed node (and even Some in other contexts) work.
    Even more curiously, this crash did not trigger before, when I had implicit dialect: Dialect in the unapplySeq method signature (I had to get rid of it on Tuesday for other reasons, which is when I noticed the crash), so I want to try having some other fake implicit in that signature as a workaround. But right now I'm looking directly into the compiler in case the fix for the underlying issue is trivial.
  2. Agreed
  3. Yes it is at the beginning of the SuccessSuite.scala. The other issue here is the fact when using incorrect ascriptions, the macro was able to error out in compiletime, now we get a match error on runtime. I believe this is something which will be able to be addressed by the compiler itself after making the method transparent inline unapply.

@kitbellew
Copy link
Contributor

@tgodzik is this logic close enough, to be included in 4.9.0?

@tgodzik
Copy link
Collaborator

tgodzik commented Jan 12, 2024

@tgodzik is this logic close enough, to be included in 4.9.0?

I think it needs more work still, especially on the cross publishing branch

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.

None yet

3 participants