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

Symbol.asQuotes doesn't work as advertised #20471

Closed
joroKr21 opened this issue May 25, 2024 · 4 comments · Fixed by #21945
Closed

Symbol.asQuotes doesn't work as advertised #20471

joroKr21 opened this issue May 25, 2024 · 4 comments · Fixed by #21945
Assignees
Labels
area:metaprogramming:quotes Issues related to quotes and splices itype:bug
Milestone

Comments

@joroKr21
Copy link
Member

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

@joroKr21 joroKr21 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels May 25, 2024
@joroKr21 joroKr21 changed the title asQuotes doesn't work as advertised Symbol.asQuotes doesn't work as advertised May 25, 2024
@jchyb jchyb added the area:metaprogramming:quotes Issues related to quotes and splices label May 27, 2024
@jchyb
Copy link
Contributor

jchyb commented May 27, 2024

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

(...)
[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
Copy link
Member Author

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
Copy link
Contributor

jchyb commented May 27, 2024

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 jchyb self-assigned this May 27, 2024
@jchyb
Copy link
Contributor

jchyb commented May 28, 2024

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]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment