fix scala 2 macros in traits with type parameters

[johnduffell]

Fixes #16630

This PR fixes the above issue which affects cross scala2/3 projects that use a common macro library as per https://docs.scala-lang.org/scala3/guides/migration/tutorial-macro-mixing.html , e.g. scala-logging is blocked from fixing this issue lightbend-labs/scala-logging#317

The fix makes the scala 2 macro check read the Erased flag from the initial flags rather than completing the RHS first. This will work in the case of scala 2 macros because the erased flag is explicitly added rather than being in the source code. However it relies on using an "UNSAFE" value.

More background on my investigation:

1. The scala 2 macros are identified by being flagged as Macro and Erased
   https://github.com/lampepfl/dotty/blob/f2aa33c3643655fd9a48a1e448d7d79e6e52ea79/compiler/src/dotty/tools/dotc/ast/Desugar.scala#L275
2. Namer.completeConstructor indexes the constructor and then other statements (the macros here) and then completes the constructor symbol.
   https://github.com/lampepfl/dotty/blob/7c0a848d0ed6330873a39129e6d02da169150fa7/compiler/src/dotty/tools/dotc/typer/Namer.scala#L1458-L1461
3. When we index a statement, we check it's not a scala 2 macro so that we don't enter it into the symbol table for the scope.
   https://github.com/lampepfl/dotty/blob/7c0a848d0ed6330873a39129e6d02da169150fa7/compiler/src/dotty/tools/dotc/typer/Namer.scala#L307-L308
4. However, although the Erased flag is actually set in the SymDenotation at init time, Erased is not in the AfterLoadFlags that are in Flags.scala, meaning that ensureCompleted() is called on the SymDenotation in order to populate the final flag set.
   https://github.com/lampepfl/dotty/blob/e1a136a0076e260d1e12ca397f0a2816cbdc9015/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L128
5. Since we are still only part way through the process of completing the constructor, I don't think it's correct to try to complete the scala 2 macro yet. It actually fails because the type parameter A in the (trait?) definition does not have the actual type A as an attachment yet.
   https://github.com/lampepfl/dotty/blob/9464bf008694506250f3fd061d7de27f1dea5d38/compiler/src/dotty/tools/dotc/typer/Typer.scala#L2133

I think the fix without any restructuring must be to check the Erased flag without needing to call ensureCompleted on the symbol, but I'm not sure if there's a better way to identify scala 2 macros at this point in the code.

I managed to get it to compile by any of these:

• hacking Namer.completeConstructor to ensure the constructor symbol is completed before indexing the rest of the statements, but that seems backwards.
  i.e. add symbolOfTree(constr).ensureCompleted() between these two lines
  https://github.com/lampepfl/dotty/blob/7c0a848d0ed6330873a39129e6d02da169150fa7/compiler/src/dotty/tools/dotc/typer/Namer.scala#L1458-L1459
• adding Erased to Flags.FromStartFlags but I think this is missing for a good reason - things can become Erased later in the process.
  https://github.com/lampepfl/dotty/blob/6ff1774363208cfdd7e4d7f4eac60aad68c52cb1/compiler/src/dotty/tools/dotc/core/Flags.scala#L466
• This PR implementation.

I have added a minimal test to the macros suite, which now passes. I managed to make something that detects the bug and doesn't depend on scala-reflect library, however beyond compiling we don't know if it actually did something useful. Tips on making a better test case would be appreciated!

Any tips or corrections would be useful

[johnduffell]

I'm not sure why the CLA check is still failing, locally it looks to work

% curl -s "https://www.lightbend.com/contribute/cla/scala/check/johnduffell" | jq -r ".signed"
true

[smarter]

Tracking the CLA issue in scala/scala-dev#855

[smarter]

Tips on making a better test case would be appreciated!

You could add an sbt project as a scripted test in sbt-test/scala2-compat/ (format for the test file described in https://www.scala-sbt.org/1.x/docs/Testing-sbt-plugins.html#step+4%3A+write+a+script, to be run from sbt with sbt-test/scripted scala2-compat/directoryName)

[johnduffell]

I have added a "scripted" test for the bug, so it will actually try to use the macro once it compiles, however I hit a problem where scala 2.13.12 release refuses to read tasty files output by the scala 3 compiler from this repo. This TASTy file was produced by an unstable release.
I was unable to find a workaround, so the scripted test only checks that it compiles (like the pos-macro test already does) and that the scala 3 macro works.
I have added TODOs for if/when this problem is resolved.

[bishabosha]

however I hit a problem where scala 2.13.12 release refuses to read tasty files output by the scala 3 compiler from this repo. This TASTy file was produced by an unstable release.
I was unable to find a workaround

This is the same as dotty in that a 3.3.0 compiler can't read 3.4.0-RC1-SNAPSHOT produced tasty files. We could bake some escape hatch into the scala 2 compiler, but seeing as we don't have an escape hatch in Scala 3 it would be unprecedented.

[johnduffell]

Thanks for the update @bishabosha and I suppose that means we have a decision to make

Conservative:

1. keep the "scripted" test as it is, although it's not really addming much value (on top of the compilation-only test, this only tests that we can call the scala 3 macro from scala 3, not the more crucial case of scala 2 -> scala 3 -> scala 2 macro)
2. delete the test and rely on the simpler compilation-only one that I added

Proactive:

1. add the escape hatch necessary into scala 2.13 (this would open up the possiblity of covering this whole area of compat, which is currently not tested AFAICT)
2. put the escape hatch the other way round - let scala 3.4 generate files not flagged as experimental
3. make the test scrub the "experimental" flag from the tasty files

In order to get this fix in (assuming the fix itself is approved) I would personally be happy with one of the conservative options, but as an outside observer I would be happy to look at getting the test to scrub the flag if it were deemed a reasonable approach, either in this PR or a separate one.

[johnduffell]

update is I have looked into scrubbing the experimental flag from the tasty files as part of the test, and although it works well for the actual ones in the test, the scala 2 compiler still needs to use tasty files from scala-library_3 which could be scrubbed but it may make the test too expensive for its value. Scrubbing those would probably involve identifying the jar(s) on the classpath, unzipping them into a temporary directory, and scrubbing them there.

So I'm leaning towards the escape hatch in scalac 2.13 as being the only workable option.

[bishabosha]

You can replicate the issue with the following code:

import scala.language.experimental.macros
import scala.quoted.{Quotes, Expr, Type}

class Foo[A]
object Foo {
  def apply[A] = new Foo[A]
}

trait TraitWithTypeParam[A] {
  protected inline def foo: Foo[A] = ${ MacrosImpl.fooImpl[A] }
  protected def foo: Foo[A] = macro MacrosImpl.compatFooImpl[A]
}

// replication of scala.reflect.macros.blackbox.Context
class Ctx {
  trait WeakTypeTag[T]

  type Tree >: Null <: AnyRef with TreeApi
  trait TreeApi extends Product { this: Tree => }
}

object MacrosImpl {
  def fooImpl[A: Type](using quotes: Quotes): Expr[Foo[A]] = '{new Foo[A]}

  def compatFooImpl[A: c.WeakTypeTag](c: Ctx): c.Tree = {
    ???
  }
}

this is all you need to test, I think its best to delete the sbt scripted test as it will be too fragile

[johnduffell]

@bishabosha I will delete the scripted test as you suggest

Regarding the simpler test - I can detect the bug with the test I've already committed in tests/pos-macros/i16630.scala

import scala.language.experimental.macros

trait TraitWithTypeParam[A] {
  protected inline def foo: Option[A] = ${ ??? }
  protected def foo: Option[A] = macro ???
}

Are you suggesting I replace my existing test (tests/pos-macros/i16630.scala) with the one you suggest, or is mine enough?

[bishabosha]

Right I should have checked your test before I made the comment, then it's fine to keep your minimisation (and remove the scripted test)

[nicolasstucki]

The fix looks good to me.

Could you also squash the commits into one?

[nicolasstucki]

I would rather minimize the test as

import scala.language.experimental.macros

trait TraitWithTypeParam[A] {
  protected inline def foo: Foo[A] = ${ MacrosImpl.fooImpl[A] }
  protected def foo: Foo[A] = macro MacrosImpl.compatFooImpl[A]
}

object MacrosImpl {
  def fooImpl[A: Type](using quotes: Quotes): Expr[Foo[A]] = ???

  def compatFooImpl[A: c.WeakTypeTag](c: scala.reflect.macros.blackbox.Context): c.Tree = ???

There are some extra checks on the macro MacrosImpl.compatFooImpl[A] that might trigger other issues.

[bishabosha]

scala.reflect.macros.blackbox.Context isn't on the classpath, which is why I made my minimisation above

[nicolasstucki]

Right. You can create a dummy interface to test this. Here is an example https://github.com/lampepfl/dotty/blob/main/tests/pos-macros/i8945.scala#L2-L11

[johnduffell]

Thanks for all the pointers both of you. I have updated the test roughly as per your suggestions. Next I will work out how to squash the commits.

[nicolasstucki]

LGTM. @johnduffell you should squash the 3 commits into 1 before we merge this PR.

[johnduffell]

thanks, I have managed squashing now - took a bit to work out how to do it. Apparently you could also "squash and merge" directly in github which may save time in future.

[SethTisue]

@Kordyjan I see this is one of PRs still marked as "needs assessment" for LTS backport. Offhand, it seems to me a like a good candidate, and affects a notable library? Is this PR one you've looked at at all yet?

[Kordyjan]

Is this PR one you've looked at at all yet?

Not yet. Everything that is merged to the main is automatically marked as "needs assessment". I'll look at this PR before 3.3.3-RC1, which probably will have all backportable fixes that have made into 3.4.

And by the way, our plan is still to get 3.4.0-RC1 out before the winter vacation season.

[johnduffell]

Just to note that this also affects play-json library. As expected it's working in 3.4.0 but not 3.3.1 or 3.3.2.

For more detail read on:

If you try to compile this trait with the scala 3 compiler

https://github.com/playframework/play-json/blob/9adadace35fe711b2ff5f32afdace9129d7ffe8d/play-json/shared/src/main/scala-2/play/api/libs/json/JsMacros.scala#L142-L146

[error] -- Error: /Users/john_duffell/code/play-json/play-json/shared/src/main/scala-3/play/api/libs/json/JsMacros.scala:291:31 
[error] 291 |trait JsMacrosWithOptions[Opts <: Json.MacroOptions] {
[error]     |                               ^^^^^^^^^^^^^^^^^^^^
[error]     |              Something's wrong: missing original symbol for type tree