search

scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.75k stars 1.04k forks source link

Symbol.asQuotes doesn't work as advertised #20471

Open joroKr21 opened 2 months ago

joroKr21 commented 2 months ago

Compiler version

3.4.2

Minimized code

Diff: https://github.com/typelevel/cats-tagless/pull/547/files?w=1#diff-19baba9f9fc14acd2200e21ad491f2871a69edbae3de81d0d3e7eec2c88899c5

CI: https://github.com/typelevel/cats-tagless/actions/runs/9236004309/job/25411405213?pr=547

Here is my change:

       body = {
         case (method, tpe, body) if tpe =:= B =>
+          given Quotes = method.asQuotes
           '{
             @tailrec def step(x: A): B =
               ${ body.replace(a, '{ x }).asExprOf[Either[A, B]] } match
                 case Left(a) => step(a)
                 case Right(b) => b
             step($a)
-          }.asTerm.changeOwner(method)
+          }.asTerm

The test which fails to compile:

  trait TestAlgebra[T] derives FlatMap:
    def abstractEffect(a: String): T
    def concreteEffect(a: String): T = abstractEffect(a + " concreteEffect")
    def abstractOther(a: String): String
    def concreteOther(a: String): String = a + " concreteOther"
    def withoutParams: T

Output

[error] -- Error: /home/runner/work/cats-tagless/cats-tagless/tests/src/test/scala-3/cats/tagless/tests/autoFlatMapTests.scala:39:31 
[error] 39 |  trait TestAlgebra[T] derives FlatMap:
[error]    |                               ^
[error]    |Exception occurred while executing macro expansion.
[error]    |java.lang.AssertionError: assertion failed: Tree had an unexpected owner for method step
[error]    |Expected: method withoutParams (cats.tagless.tests.autoFlatMapTests$.TestAlgebra$._$_$$anon._$$anon$macro$3.withoutParams)
[error]    |But was: method concreteEffect (cats.tagless.tests.autoFlatMapTests$.TestAlgebra$._$_$$anon._$$anon$macro$3.concreteEffect)
[error]    |
[error]    |The code of the definition of method step is
[error]    |@scala.annotation.tailrec def step(x: A): B = f.apply(x).withoutParams match {
[error]    |  case scala.Left(a) =>
[error]    |    step(a)
[error]    |  case scala.Right(b) =>
[error]    |    (b: B)
[error]    |}
[error]    |
[error]    |which was found in the code
[error]    |{
[error]    |  @scala.annotation.tailrec def step(x: A): B = f.apply(x).withoutParams match {
[error]    |    case scala.Left(a) =>
[error]    |      step(a)
[error]    |    case scala.Right(b) =>
[error]    |      (b: B)
[error]    |  }
[error]    |  step(`a₂`)
[error]    |}
[error]    |
[error]    |which has the AST representation
[error]    |Inlined(Some(TypeIdent("MacroFlatMap$")), Nil, Block(List(DefDef("step", List(TermParamClause(List(ValDef("x", Inferred(), None)))), Inferred(), Some(Match(Inlined(None, Nil, Select(Inlined(Some(TypeIdent("MacroFlatMap$")), Nil, Apply(Select(Inlined(None, Nil, Ident("f")), "apply"), List(Inlined(None, Nil, Ident("x"))))), "withoutParams")), List(CaseDef(TypedOrTest(Unapply(TypeApply(Select(Ident("Left"), "unapply"), List(Inferred(), Inferred())), Nil, List(Bind("a", Wildcard()))), Inferred()), None, Block(Nil, Apply(Ident("step"), List(Ident("a"))))), CaseDef(TypedOrTest(Unapply(TypeApply(Select(Ident("Right"), "unapply"), List(Inferred(), Inferred())), Nil, List(Bind("b", Wildcard()))), Inferred()), None, Block(Nil, Typed(Ident("b"), Inferred())))))))), Apply(Ident("step"), List(Inlined(None, Nil, Ident("a"))))))
[error]    |
[error]    |
[error]    |
[error]    |Tip: The owner of a tree can be changed using method `Tree.changeOwner`.
[error]    |Tip: The default owner of definitions created in quotes can be changed using method `Symbol.asQuotes`.

Expectation

I expect it to compile the same way it does if I use changeOwner. It's even one of the tips:

Tip: The default owner of definitions created in quotes can be changed using method Symbol.asQuotes

jchyb commented 2 months ago

Hi @joroKr21! After printing the trees in cats-tagless I see everything behaving correctly:

```scala (...) [info] final lazy given val q2: scala.quoted.Quotes = [info] given_DeriveMacros_q_type.q.reflect.SymbolMethods. [info] asQuotes(method) [info] q.reflect.asTerm( [info] '{ [info] { [info] @tailrec def step(x: A): B = [info] ${ [info] { [info] def $anonfun(using [info] evidence$8: scala.quoted.Quotes): [info] scala.quoted.Expr[Either[A, B]] = [info] given_DeriveMacros_q_type.q.reflect. [info] TreeMethods.asExprOf( [info] given_DeriveMacros_q_type.replace(body)( [info] a, '{x}.apply(evidence$8)) [info] )[Either[A, B]]( [info] scala.quoted.Type.of[Either[A, B]]( [info] evidence$8) [info] ) [info] closure($anonfun) [info] } [info] } match [info] { [info] case Left.unapply[A, B](a @ _):Left[A, B] => [info] step(a) [info] case Right.unapply[A, B](b @ _):Right[A, B] [info] => [info] b:B [info] } [info] step( [info] ${ [info] { [info] def $anonfun(using [info] evidence$9: scala.quoted.Quotes): [info] scala.quoted.Expr[A] = a [info] closure($anonfun) [info] } [info] } [info] ) [info] } [info] }.apply(q2) (...) ``` (q2 is the Quotes object applied to the splice, which is what we want)

The issue you are having stems from changeOwner and given Quotes behaving slightly differently by design. Supplying correct given Quotes guarantees that every symbol defined as part of the quoted code block has its owner set to a pointed symbol. Meanwhile changeOwner(symbol) goes through the whole tree and replaces every nonlocal owner with symbol.

With the first method we still have to set the correct owners inside splices, with the second those will likely be corrected.

So in our case, while the def step does have a correct owner set, the stuff returned by DeriveMacros.replace does not. The (correct) Quotes object supplied by the splice is ignored, and instead the previous quotes object passed to DeriveMacros is used:

def replace(from: Expr[?], to: Expr[?]): Term =
  ReplaceTerm(from.asTerm, to.asTerm).transformTerm(term)(Symbol.spliceOwner)

You could rewrite the replace method to something like

def replace(from: Expr[?], to: Expr[?])(using quotes: Quotes): quotes.reflect.Term =
  import quotes.reflect._
  ReplaceTerm(from.asTerm, to.asTerm).transformTerm(term)(Symbol.spliceOwner)

to use the correct quotes object supplied by the splice (it will not compile yet, but the point is to use a correct symbol).

joroKr21 commented 2 months ago

Hmm I will try it but that sounds strange, because the error is explicitly about the owner of step:

[error]    |java.lang.AssertionError: assertion failed: Tree had an unexpected owner for method step
[error]    |Expected: method withoutParams (cats.tagless.tests.autoFlatMapTests$.TestAlgebra$._$_$$anon._$$anon$macro$3.withoutParams)
[error]    |But was: method concreteEffect (cats.tagless.tests.autoFlatMapTests$.TestAlgebra$._$_$$anon._$$anon$macro$3.concreteEffect)

For context, body.replace(a, '{ x }) doesn't introduce any definitions.

jchyb commented 2 months ago

Ah, you are right, I now replaced step implementation with ??? and those errors are still thrown. I will have to investigate this further. Thank you for reporting!

jchyb commented 2 months ago

Self-contained minimisation: Macro_1.scala


import scala.annotation.experimental
import scala.quoted.*
import scala.annotation.tailrec

@experimental
object MacroFlatMap:

  inline def derive[F[_]]: FlatMap[F] = ${ flatMap }

  def flatMap[F[_]: Type](using Quotes): Expr[FlatMap[F]] = '{
    new FlatMap[F]:
      def tailRecM[A, B](a: A)(f: A => F[Either[A, B]]): F[B] =
        ${ deriveTailRecM('{ a }, '{ f }) }
  }

  def deriveTailRecM[F[_]: Type, A: Type, B: Type](
      a: Expr[A],
      f: Expr[A => F[Either[A, B]]]
  )(using q: Quotes): Expr[F[B]] =
    import quotes.reflect.*

    val body: PartialFunction[(Symbol, TypeRepr), Term] = {
        case (method, tpe) => {
          given q2: Quotes = method.asQuotes
          '{
            def step(x: A): B = ???
            ???
          }.asTerm//.changeOwner(method)
        }
      }

    val term = '{ $f($a) }.asTerm
    val name = Symbol.freshName("$anon")
    val parents = List(TypeTree.of[Object], TypeTree.of[F[B]])

    extension (sym: Symbol) def overridableMembers: List[Symbol] = 
      val member1 = sym.methodMember("abstractEffect")(0)
      val member2 = sym.methodMember("concreteEffect")(0)
      def meth(member: Symbol) = Symbol.newMethod(sym, member.name, This(sym).tpe.memberType(member), Flags.Override, Symbol.noSymbol)
      List(meth(member1), meth(member2))

    val cls = Symbol.newClass(Symbol.spliceOwner, name, parents.map(_.tpe), _.overridableMembers, None)

    def transformDef(method: DefDef)(argss: List[List[Tree]]): Option[Term] =
      val sym = method.symbol
      Some(body.apply((sym, method.returnTpt.tpe)))

    val members = cls.declarations
      .filterNot(_.isClassConstructor)
      .map: sym =>
        sym.tree match
          case method: DefDef => DefDef(sym, transformDef(method))
          case _ => report.errorAndAbort(s"Not supported: $sym in ${sym.owner}")

    val newCls = New(TypeIdent(cls)).select(cls.primaryConstructor).appliedToNone
    Block(ClassDef(cls, parents, members) :: Nil, newCls).asExprOf[F[B]]

Test_2.scala

import scala.annotation.experimental

@experimental
object autoFlatMapTests:
  trait TestAlgebra[T] derives FlatMap:
    def abstractEffect(a: String): T
    def concreteEffect(a: String): T = abstractEffect(a + " concreteEffect")

object FlatMap {
  @experimental inline def derived[F[_]]: FlatMap[F] = MacroFlatMap.derive
}
trait FlatMap[F[_]]{
  def tailRecM[A, B](a: A)(f: A => F[Either[A, B]]): F[B]
}