scala / scala3

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

Defining a closure in a macro causes a compiler exception #12309

Closed adamw closed 3 years ago

adamw commented 3 years ago

Compiler version

3.0.0-RC3

Minimized example

import scala.quoted.*

object TestMacro {
  def use(f: () => String): Unit = ()

  inline def test: Unit = ${testImpl}

  def testImpl(using Quotes): Expr[Unit] = {
    import quotes.reflect.*

    def resultDefBody(): Term = '{
      val result: String = "xxx"
      result
    }.asTerm
    val resultDefSymbol = Symbol.newMethod(Symbol.spliceOwner, "getResult", MethodType(Nil)(_ => Nil, _ => TypeRepr.of[String]))
    val resultDef = DefDef(resultDefSymbol, { case _ => Some(resultDefBody()) })
    val resultExpr = Block(List(resultDef), Closure(Ref(resultDefSymbol), None)).asExprOf[() => String]

    //

    val r = '{ TestMacro.use($resultExpr) }
    println(r.asTerm.show(using Printer.TreeShortCode))
    r
  }
}

and then calling the macro:

object Test extends App {
  TestMacro.test
}

Output

I'm trying to generate code which passes a function as a parameter to a method. From what I've seen by printing the AST of quoted code, I need a Block with a DefDef as the statement, and a Closure as the expression. In the body of the function, I define a local variable - and this seems to be a problem. The original exception from the compiler I get is:

java.util.NoSuchElementException: val result while compiling Test.scala
[error] ## Exception when compiling 25 sources to /core/target/jvm-3.0.0-RC3/test-classes
[error] java.util.NoSuchElementException: val result
[error] scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:508)
[error] scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:507)
[error] scala.collection.mutable.AnyRefMap.apply(AnyRefMap.scala:207)
[error] dotty.tools.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:507)

while trying to minify the example, I started getting another exception, so I'm not sure if my code reproduces the problem, but maybe it reproduces some problem :)

Here's the exception I get now:

java.lang.IllegalArgumentException: Could not find proxy for val result: String in List(val result, val <local Test$>, module class Test$, module class softwaremill, module class com, module class <root>), encl = (...)
[error] ## Exception when compiling 6 sources to /target/jvm-3.0.0-RC3/classes
[error] java.lang.IllegalArgumentException: Could not find proxy for val result: String in List(val result, val <local Test$>, module class Test$, module class softwaremill, module class com, module class <root>), encl = (...)
[error] dotty.tools.dotc.transform.LambdaLift$Lifter.searchIn$1(LambdaLift.scala:396)
[error] dotty.tools.dotc.transform.LambdaLift$Lifter.proxy(LambdaLift.scala:409)
[error] dotty.tools.dotc.transform.LambdaLift$Lifter.proxyRef(LambdaLift.scala:427)
[error] dotty.tools.dotc.transform.LambdaLift$Lifter.addFreeArgs$$anonfun$1(LambdaLift.scala:433)
[error] scala.collection.immutable.List.map(List.scala:246)

When printing the generated code, it looks fine:

TestMacro.use((() => {
  val result: String = "xxx"

  (result: String)
}))

Expectation

A way to generate a closure and pass it to a method, in a macro.

adamw commented 3 years ago

I also tried separating the def & using the closure:

val resultDefSymbol = Symbol.newMethod(Symbol.spliceOwner, "getResult", MethodType(Nil)(_ => Nil, _ => TypeRepr.of[String]))
val resultDef = DefDef(resultDefSymbol, { case _ => Some(resultDefBody()) })
val resultExpr = Closure(Ref(resultDefSymbol), None).asExprOf[() => String]

Block(List(resultDef), '{ TestMacro.use($resultExpr) }.asTerm).asExprOf[Unit]

but this causes the same exception (Could not find proxy for val result)

adamw commented 3 years ago

As the signature of the method/closure that I'm trying to create is static, but the body is dynamically generated, a work-around might be to quote the whole definition of the method:

val getResultDef: Term = '{
  def getResult(): String = {
    val result: String = "xxx"
    result
  }
}.asTerm

but then, how to get hold of the Symbol corresponding to getResult, so that I can generate the closure? I've got: '{ TestMacro.use(???) }, and in place of ??? I need to somehow reference the getResult method.

nicolasstucki commented 3 years ago

Compiling this code using the -Xcheck-macros produces the following error

2 |  TestMacro.test
  |  ^^^^^^^^^^^^^^
  |Exception occurred while executing macro expansion.
  |java.lang.AssertionError: assertion failed: Tree had an unexpected owner for val result
  |Expected: method getResult (Test$._$_$getResult)
  |But was: val macro (Test$._$macro)
  |
  |
  |The code of the definition of val result is
  |val result: scala.Predef.String = "xxx"
  |
  |which was found in the code
  |{
  |  val result: scala.Predef.String = "xxx"
  |
  |  (result: java.lang.String)
  |}
  |
  |which has the AST representation
  |Inlined(Some(TypeIdent("TestMacro$")), Nil, Block(List(ValDef("result", TypeIdent("String"), Some(Literal(StringConstant("xxx"))))), Typed(Ident("result"), Inferred())))
  |
  |
  |     at scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:8)
  |     at scala.quoted.runtime.impl.QuotesImpl$$anon$9.traverse(QuotesImpl.scala:2825)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.apply(Trees.scala:1584)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.apply(Trees.scala:1584)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeAccumulator.fold$1(Trees.scala:1459)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeAccumulator.apply(Trees.scala:1461)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeAccumulator.foldOver(Trees.scala:1492)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.traverseChildren(Trees.scala:1585)
  |     at scala.quoted.runtime.impl.QuotesImpl$$anon$9.traverse(QuotesImpl.scala:2828)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.apply(Trees.scala:1584)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.apply(Trees.scala:1584)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeAccumulator.foldOver(Trees.scala:1512)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeAccumulator.foldOver(Trees.scala:1465)
  |     at dotty.tools.dotc.ast.Trees$Instance$TreeTraverser.traverseChildren(Trees.scala:1585)
  |     at scala.quoted.runtime.impl.QuotesImpl$$anon$9.traverse(QuotesImpl.scala:2828)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$.xCheckMacroOwners(QuotesImpl.scala:2829)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$.scala$quoted$runtime$impl$QuotesImpl$reflect$$$xCheckMacroedOwners(QuotesImpl.scala:2791)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$DefDef$.apply$$anonfun$2$$anonfun$1(QuotesImpl.scala:255)
  |     at dotty.tools.dotc.ast.tpd$.DefDef(tpd.scala:283)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$DefDef$.apply$$anonfun$1(QuotesImpl.scala:256)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$.scala$quoted$runtime$impl$QuotesImpl$reflect$$$withDefaultPos(QuotesImpl.scala:2782)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$DefDef$.apply(QuotesImpl.scala:256)
  |     at scala.quoted.runtime.impl.QuotesImpl$reflect$DefDef$.apply(QuotesImpl.scala:253)
  |     at TestMacro$.testImpl(Macro_1.scala:16)
  |
  | This location contains code that was inlined from Test_2.scala:2

A simple way to fix the code is to change the owner of the content of getResult

-    val resultDef = DefDef(resultDefSymbol, { case _ => Some(resultDefBody()) })
+    val resultDef = DefDef(resultDefSymbol, { case _ => Some(resultDefBody().changeOwner(resultDefSymbol)) })
nicolasstucki commented 3 years ago

To avoid the explicit change of owner we could introduce a way to create a Quotes instance that has the correct owner. This would be passed to resultDefBody.

adamw commented 3 years ago

Thanks! This solved both problems (in the minimised & original macros).

Since DefDef is a "smart constructor", couldn't it change the owner? Alternatively, maybe it would be an option to mention in DefDefModule.apply scaladoc, that the rhs term should have the symbol set as the owner, and that this can be done using .changeOwner? Unless this is already documented somewhere, and I just missed that place :)

nicolasstucki commented 3 years ago

Since DefDef is a "smart constructor", couldn't it change the owner?

The issue is that if we make DefDef a smart constructor that fixes the owners, then every call to it would need to check the owner which might add some overhead.

Alternatively, maybe it would be an option to mention in DefDefModule.apply scaladoc, that the rhs term should have the symbol set as the owner, and that this can be done using .changeOwner?

Yes, that should be mentioned in the documentation of DefDef

odersky commented 3 years ago

I think checking owners is probably not going to add much overhead. You just need to traverse down to all nested definition but not beyond them. It's changing owners which is costly. So I would be in favor of making DefDef and related constructors maintain owners automatically.

nicolasstucki commented 3 years ago

The issue is that if we do it automatically, then users will probably not care about creating the trees with the correct owner. Then the norm would be to have to do an owner change.

LPTK commented 3 years ago

@nicolasstucki is that really a problem? If the owner is changed only once when constructing the thing, as opposed to on every transformation of the tree. It's only a constant cost to pay.

Should macro users who just want to quickly put some trees together have to care about owners, something the compiler should in principle be able to figure out on its own?

Symbol owners and owner chain corruptions were probably the number one cause for unreliability and crashes with Scala 2 macros. The entire thing was so complicated that it was basically impossible to get it right, from my experience (though at the time I tried to do this I was less experimented). Look at this old tutorial for example. Any deviation from the compiler's internal notion of how things must be done led to horrible crashes. This is one of the main reason people ended up simply wiping types off the trees they received in macros, and just using untyped tree manipulations throughout their macros, which has its own set of problems.