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

Macro annotation seems to not change the parent #18677

Open
eed3si9n opened this issue Oct 12, 2023 · 11 comments
Open

Macro annotation seems to not change the parent #18677

eed3si9n opened this issue Oct 12, 2023 · 11 comments

Comments

@eed3si9n
Copy link
Member

Compiler version

3.4.0-RC1-bin-20231010-7dc9798-NIGHTLY

Minimized code

https://github.com/eed3si9n/ifdef

import scala.annotation.{ experimental, MacroAnnotation }
import scala.quoted.*

@experimental
class ifdef(key: String) extends MacroAnnotation:
  private final val macroSetting = "com.eed3si9n.ifdef.declare:"

  def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
    import quotes.reflect.*
    tree match
      case cls @ ClassDef(className, ctr, parents, self, body) =>
        val keys = (CompilationInfo.XmacroSettings.collect:
          case x if x.startsWith(macroSetting) => x.drop(macroSetting.size)
        ).toSet
        if keys(key) then List(tree)
        else
          val trees = List(ClassDef.copy(tree)(
            className,
            DefDef(cls.symbol.primaryConstructor, _ => None),
            parents = Nil,
            None,
            Nil))
          println(trees.map(_.show))
          trees
      case _ =>
        report.error("annotation only supports `class`")
        List(tree)

then:

package example

import scala.annotation.experimental

class A:
  def foo: Int = 42

@experimental
@ifdef("test")
class ATest extends munit.FunSuite:
  test("hello"):
    val actual = new A().foo
    val expected = 42
    assertEquals(actual, expected)

Output

$ javap -cp app/target/scala-3.4.0-RC1-bin-20231010-7dc9798-NIGHTLY/classes/ example.ATest
Compiled from "app.scala"
public class example.ATest extends munit.FunSuite {
  public example.ATest();
}

Expectation

Given parents = Nil, I expect that example.ATest to not extend munit.FunSuite.

@eed3si9n eed3si9n added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 12, 2023
@nicolasstucki nicolasstucki added area:metaprogramming:macro-annotations and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 12, 2023
@bishabosha
Copy link
Member

I think in this case the tree doesn't matter because the class symbol never changed

@nicolasstucki
Copy link
Contributor

@bishabosha is correct.

Nevertheless, in this case, there might be an argument to be made to allow the addition of parents in the same way we can add new methods to that class. Such an addition to the spec would need to be examined in detail. This is probably not something that would be available in the first iteration of macro annotation but added later as an extension to the spec.

@nicolasstucki
Copy link
Contributor

We must make sure that -Ycheck:all (and maybe -Xcheck-macros) flag these classes with misaligned parents.

@eed3si9n
Copy link
Member Author

Thanks for the response. So at this point, there's no way to remove parents (or switch to List(TypeTree.of[Object])) from macro annotation? I tried giving a fresh symbol, but that failed the macro with "was not return by":

val cls = Symbol.newClass(Symbol.spliceOwner, className, parents = List(TypeRepr.of[Object]), _ => Nil, selfType = None)
val clsDef = ClassDef(cls, parents = List(TypeTree.of[Object]), body = Nil)
val trees = List(clsDef)

usage:

[error] -- Error:ifdef/app/app.scala:10:1 ------------------------
[error] 10 |@ifdef("test")
[error]    |^^^^^^^^^^^^^^
[error]    |Transformed tree for @SourceFile("app/app.scala") @ifdef("test") @experimental class ATest() extends
[error]    |  munit.FunSuite() {
[error]    |  this.test("hello")(
[error]    |    {
[error]    |      val actual: Int = new example.A().foo
[error]    |      val expected: Int = 43
[error]    |      this.assertEquals[Int, Int](actual, expected,
[error]    |        this.assertEquals$default$3[Int, Int])(munit.Location.generate,
[error]    |        <:<.refl[Int])
[error]    |    }
[error]    |  )(munit.Location.generate)
[error]    |} was not return by `(new com.eed3si9n.ifdef.ifdef("test")).transform(..)` during macro expansion
[error] one error found
[error] (app / Compile / compileIncremental) Compilation failed

@sjrd
Copy link
Member

sjrd commented Oct 12, 2023

Removing parents could effectively remove members from your API. That would mean that external code that typechecks when the macro is not expanded does not compile anymore after it is expanded. This goes against some of the design goals of the Scala 3 macro system.

@nicolasstucki

This comment was marked as duplicate.

@eed3si9n
Copy link
Member Author

Removing parents could effectively remove members from your API. That would mean that external code that typechecks when the macro is not expanded does not compile anymore after it is expanded. This goes against some of the design goals of the Scala 3 macro system.

For all other parts of the macro system, I really like the safety aspect of it, but for macro annotations specifically, I think what I'd want is basically a string generator post-parser, pre-typer, such that I can remove classes, add classes, add methods, and the rest of the code can reference new members as well.

@sjrd
Copy link
Member

sjrd commented Oct 12, 2023

Those abilities are what made Scala 2 macro annotations so fragile, notably in the presence of IDEs or incremental compilation. They won't be provided in Scala 3.

@bishabosha
Copy link
Member

bishabosha commented Oct 13, 2023

notably in the presence of IDEs

if we had pre-typer macro annotations and we removed some trees, what should the IDE even do when you hover over that piece of code? (grey-it-out?)

@nicolasstucki
Copy link
Contributor

if we had pre-typer macro annotations and we removed some trees, what should the IDE even do when you hover over that piece of code? (grey-it-out?)

We cannot transform a tree before it is typed.

@eed3si9n
Copy link
Member Author

or incremental compilation

Not sure about IDEs, but given that sbt-injected phases all run after the typer, if we could do everything pre-typer, it would be no different than a generated code, I think, from Zinc's perspective.

We cannot transform a tree before it is typed.

We can't transform a typed tree, but we can transform the untyped tree post parser:

  override def runOn(units: List[CompilationUnit])(using ctx: Context): List[CompilationUnit] =
    val unitContexts =
      for unit <- units
      yield ctx.fresh.setPhase(this.start).setCompilationUnit(unit)
    keys = (ctx.settings.XmacroSettings.value.collect:
      case x if x.startsWith(macroSetting) => x.drop(macroSetting.size)
    ).toSet
    unitContexts.foreach(preprocess(using _))
    unitContexts.map(_.compilationUnit)

  def preprocess(using ctx: Context): Unit =
    val unit = ctx.compilationUnit
    try
      if !unit.suspended then
        unit.untpdTree = (new M).transform(unit.untpdTree)
    catch case _: CompilationUnit.SuspendException => ()

@hamzaremmal hamzaremmal self-assigned this Nov 10, 2023
hamzaremmal added a commit that referenced this issue Feb 12, 2024
This PR adds a check for the parents in the TreeChecker. 

In the context of macro annotations, this PR doesn't allow the
modification of the parents. This restriction will probably be partially
lifted as we come up with the final specification.

This PR is related to #18677 put does not close it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants