Fix macro annotation splice owner

The splice owner of the macro annotation should be the owner of the annotated definition. The issue was that if that owner was a class quotes unpickled with this context accidentally entered definitions in the quotes into the class. Now we unpickle these definitions using a dummy val owner (similar to the LocalDummy).
scala · Dec 15, 2022 · 9431066 · 9431066
1 parent 01a5dd4
commit 9431066
Show file tree

Hide file tree

Showing 17 changed files with 99 additions and 47 deletions.
diff --git a/compiler/src/dotty/tools/dotc/quoted/PickledQuotes.scala b/compiler/src/dotty/tools/dotc/quoted/PickledQuotes.scala
@@ -5,6 +5,7 @@ import dotty.tools.dotc.ast.{TreeTypeMap, tpd}
 import dotty.tools.dotc.config.Printers._
 import dotty.tools.dotc.core.Contexts._
 import dotty.tools.dotc.core.Decorators._
+import dotty.tools.dotc.core.Flags._
 import dotty.tools.dotc.core.Mode
 import dotty.tools.dotc.core.Symbols._
 import dotty.tools.dotc.core.Types._
@@ -248,23 +249,41 @@ object PickledQuotes {
           case pickled: String => TastyString.unpickle(pickled)
           case pickled: List[String] => TastyString.unpickle(pickled)
 
-        quotePickling.println(s"**** unpickling quote from TASTY\n${TastyPrinter.showContents(bytes, ctx.settings.color.value == "never")}")
+        val unpicklingContext =
+          if ctx.owner.isClass then
+            // When a quote is unpickled with a Quotes context that that has a class `spliceOwner`
+            // we need to use a dummy owner to unpickle it. Otherwise any definitions defined
+            // in the quoted block would be accidentally entered in the class.
+            // When splicing this expression, this owner is replaced with the correct owner (see `quotedExprToTree` and `quotedTypeToTree` above).
+            // On the other hand, if the expression is used as a reflect term, the user must call `changeOwner` (same as with other expressions used within a nested owner).
+            // `-Xcheck-macros` will check for inconsistent owners and provide the users hints on how to improve them.
+            //
+            // Quotes context that that has a class `spliceOwner` can come from a macro annotation
+            // or a user setting it explicitly using `Symbol.asQuotes`.
+            ctx.withOwner(newSymbol(ctx.owner, "$quoteOwnedByClass$".toTermName, Private, defn.AnyType, NoSymbol))
+            else ctx
 
-        val mode = if (isType) UnpickleMode.TypeTree else UnpickleMode.Term
-        val unpickler = new DottyUnpickler(bytes, mode)
-        unpickler.enter(Set.empty)
+        inContext(unpicklingContext) {
 
-        val tree = unpickler.tree
-        QuotesCache(pickled) = tree
+          quotePickling.println(s"**** unpickling quote from TASTY\n${TastyPrinter.showContents(bytes, ctx.settings.color.value == "never")}")
 
-        // Make sure trees and positions are fully loaded
-        new TreeTraverser {
-          def traverse(tree: Tree)(using Context): Unit = traverseChildren(tree)
-        }.traverse(tree)
+          val mode = if (isType) UnpickleMode.TypeTree else UnpickleMode.Term
+          val unpickler = new DottyUnpickler(bytes, mode)
+          unpickler.enter(Set.empty)
 
-        quotePickling.println(i"**** unpickled quote\n$tree")
+          val tree = unpickler.tree
+          QuotesCache(pickled) = tree
+
+          // Make sure trees and positions are fully loaded
+          new TreeTraverser {
+            def traverse(tree: Tree)(using Context): Unit = traverseChildren(tree)
+          }.traverse(tree)
+
+          quotePickling.println(i"**** unpickled quote\n$tree")
+
+          tree
+        }
 
-        tree
   }
 
 }
diff --git a/compiler/src/dotty/tools/dotc/transform/MacroAnnotations.scala b/compiler/src/dotty/tools/dotc/transform/MacroAnnotations.scala
@@ -91,7 +91,7 @@ class MacroAnnotations(thisPhase: DenotTransformer):
     // TODO: Remove when scala.annaotaion.MacroAnnotation is no longer experimental
     assert(annotInstance.getClass.getClassLoader.loadClass("scala.annotation.MacroAnnotation").isInstance(annotInstance))
 
-    val quotes = QuotesImpl()(using SpliceScope.contextWithNewSpliceScope(tree.symbol.sourcePos)(using MacroExpansion.context(tree)).withOwner(tree.symbol))
+    val quotes = QuotesImpl()(using SpliceScope.contextWithNewSpliceScope(tree.symbol.sourcePos)(using MacroExpansion.context(tree)).withOwner(tree.symbol.owner))
     annotInstance.transform(using quotes)(tree.asInstanceOf[quotes.reflect.Definition])
 
   /** Check that this tree can be added by the macro annotation and enter it if needed */

diff --git a/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala b/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala
@@ -2662,7 +2662,9 @@ class QuotesImpl private (using val ctx: Context) extends Quotes, QuoteUnpickler
 
         def show(using printer: Printer[Symbol]): String = printer.show(self)
 
-        def asQuotes: Nested = new QuotesImpl(using ctx.withOwner(self))
+        def asQuotes: Nested =
+          assert(self.ownersIterator.contains(ctx.owner), s"$self is not owned by ${ctx.owner}")
+          new QuotesImpl(using ctx.withOwner(self))
 
       end extension
 

diff --git a/library/src/scala/annotation/MacroAnnotation.scala b/library/src/scala/annotation/MacroAnnotation.scala
@@ -31,14 +31,20 @@ trait MacroAnnotation extends StaticAnnotation:
    *    def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
    *      import quotes.reflect._
    *      tree match
-   *        case DefDef(name, TermParamClause(param :: Nil) :: Nil, tpt, Some(rhsTree)) =>
-   *          (Ref(param.symbol).asExpr, rhsTree.asExpr) match
-   *            case ('{ $paramRefExpr: t }, '{ $rhsExpr: u }) =>
-   *              val cacheSymbol = Symbol.newUniqueVal(tree.symbol.owner, name + "Cache", TypeRepr.of[mutable.Map[t, u]], Flags.Private, Symbol.noSymbol)
-   *              val cacheRhs = '{ mutable.Map.empty[t, u] }.asTerm
+   *        case DefDef(name, TermParamClause(param  :: Nil) :: Nil, tpt, Some(rhsTree)) =>
+   *          (param.tpt.tpe.asType, tpt.tpe.asType) match
+   *            case ('[t], '[u]) =>
+   *              val cacheSymbol = Symbol.newUniqueVal(Symbol.spliceOwner, name + "Cache", TypeRepr.of[mutable.Map[t, u]], Flags.Private, Symbol.noSymbol)
+   *              val cacheRhs =
+   *                given Quotes = cacheSymbol.asQuotes
+   *                '{ mutable.Map.empty[t, u] }.asTerm
    *              val cacheVal = ValDef(cacheSymbol, Some(cacheRhs))
-   *              val cacheRefExpr = Ref(cacheSymbol).asExprOf[mutable.Map[t, u]]
-   *              val newRhs = '{ $cacheRefExpr.getOrElseUpdate($paramRefExpr, $rhsExpr) }.asTerm
+   *              val newRhs =
+   *                given Quotes = tree.symbol.asQuotes
+   *                val cacheRefExpr = Ref(cacheSymbol).asExprOf[mutable.Map[t, u]]
+   *                val paramRefExpr = Ref(param.symbol).asExprOf[t]
+   *                val rhsExpr = rhsTree.asExprOf[u]
+   *                '{ $cacheRefExpr.getOrElseUpdate($paramRefExpr, $rhsExpr) }.asTerm
    *              val newTree = DefDef.copy(tree)(name, TermParamClause(param :: Nil) :: Nil, tpt, Some(newRhs))
    *              List(cacheVal, newTree)
    *        case _ =>

diff --git a/tests/neg-macros/annot-accessIndirect/Macro_1.scala b/tests/neg-macros/annot-accessIndirect/Macro_1.scala
@@ -5,7 +5,7 @@ import scala.quoted._
 class hello extends MacroAnnotation {
   def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
     import quotes.reflect._
-    val helloSymbol = Symbol.newUniqueVal(tree.symbol.owner, "hello", TypeRepr.of[String], Flags.EmptyFlags, Symbol.noSymbol)
+    val helloSymbol = Symbol.newUniqueVal(Symbol.spliceOwner, "hello", TypeRepr.of[String], Flags.EmptyFlags, Symbol.noSymbol)
     val helloVal = ValDef(helloSymbol, Some(Literal(StringConstant("Hello, World!"))))
     List(helloVal, tree)
 }
diff --git a/tests/neg-macros/annot-accessIndirect/Macro_2.scala b/tests/neg-macros/annot-accessIndirect/Macro_2.scala
@@ -7,7 +7,7 @@ class foo extends MacroAnnotation {
     import quotes.reflect._
     val s = '{@hello def foo1(x: Int): Int = x + 1;()}.asTerm
     val fooDef = s.asInstanceOf[Inlined].body.asInstanceOf[Block].statements.head.asInstanceOf[DefDef]
-    val hello = Ref(tree.symbol.owner.declaredFields("hello").head).asExprOf[String] // error
+    val hello = Ref(Symbol.spliceOwner.declaredFields("hello").head).asExprOf[String] // error
     tree match
       case DefDef(name, params, tpt, Some(t)) =>
         val rhs = '{

diff --git a/tests/pos-macros/annot-then-inline/Macro_1.scala b/tests/pos-macros/annot-then-inline/Macro_1.scala
@@ -7,7 +7,9 @@ class useInlinedIdentity extends MacroAnnotation {
     import quotes.reflect.*
     tree match
       case DefDef(name, params, tpt, Some(rhs)) =>
-        val newRhs = '{ inlinedIdentity(${rhs.asExpr}) }.asTerm
+        val newRhs =
+          given Quotes = tree.symbol.asQuotes
+          '{ inlinedIdentity(${rhs.asExpr}) }.asTerm
         List(DefDef.copy(tree)(name, params, tpt, Some(newRhs)))
 }
 

diff --git a/tests/run-macros/annot-annot-order/Macro_1.scala b/tests/run-macros/annot-annot-order/Macro_1.scala
@@ -7,6 +7,7 @@ class print(msg: String) extends MacroAnnotation:
     import quotes.reflect._
     tree match
       case DefDef(name, params, tpt, Some(rhsTree)) =>
+        given Quotes = tree.symbol.asQuotes
         rhsTree.asExpr match
           case '{ $rhsExpr: t } =>
             val newRhs = '{ println(${Expr(msg)}); $rhsExpr }.asTerm

diff --git a/tests/run-macros/annot-bind/Macro_1.scala b/tests/run-macros/annot-bind/Macro_1.scala
@@ -7,7 +7,7 @@ class bind(str: String) extends MacroAnnotation:
     import quotes.reflect._
     tree match
       case ValDef(name, tpt, Some(rhsTree)) =>
-        val valSym = Symbol.newUniqueVal(tree.symbol.owner, str, tpt.tpe, Flags.Private, Symbol.noSymbol)
+        val valSym = Symbol.newUniqueVal(Symbol.spliceOwner, str, tpt.tpe, Flags.Private, Symbol.noSymbol)
         val valDef = ValDef(valSym, Some(rhsTree))
         val newRhs = Ref(valSym)
         val newTree = ValDef.copy(tree)(name, tpt, Some(newRhs))

diff --git a/tests/run-macros/annot-gen2/Macro_1.scala b/tests/run-macros/annot-gen2/Macro_1.scala
@@ -7,6 +7,7 @@ class hello extends MacroAnnotation {
     import quotes.reflect._
     tree match
       case DefDef(name, params, tpt, Some(t)) =>
+        given Quotes = tree.symbol.asQuotes
         val rhs = '{
           ${t.asExprOf[String]} + "hello"
         }.asTerm

diff --git a/tests/run-macros/annot-gen2/Macro_2.scala b/tests/run-macros/annot-gen2/Macro_2.scala
@@ -7,6 +7,7 @@ class foo extends MacroAnnotation {
     import quotes.reflect._
     tree match
       case DefDef(name, params, tpt, Some(t)) =>
+        given Quotes = tree.symbol.asQuotes
         val s = Ref(params.head.params.head.symbol).asExprOf[String]
         val rhs = '{
           @hello def foo1(s: String): String = ${

diff --git a/tests/run-macros/annot-generate/Macro_1.scala b/tests/run-macros/annot-generate/Macro_1.scala
@@ -5,7 +5,7 @@ import scala.quoted._
 class hello extends MacroAnnotation {
   def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
     import quotes.reflect._
-    val helloSymbol = Symbol.newUniqueVal(tree.symbol.owner, "hello", TypeRepr.of[String], Flags.EmptyFlags, Symbol.noSymbol)
+    val helloSymbol = Symbol.newUniqueVal(Symbol.spliceOwner, "hello", TypeRepr.of[String], Flags.EmptyFlags, Symbol.noSymbol)
     val helloVal = ValDef(helloSymbol, Some(Literal(StringConstant("Hello, World!"))))
     List(helloVal, tree)
 }
diff --git a/tests/run-macros/annot-generate/Macro_2.scala b/tests/run-macros/annot-generate/Macro_2.scala
@@ -7,6 +7,7 @@ class foo extends MacroAnnotation {
     import quotes.reflect._
     tree match
       case DefDef(name, params, tpt, Some(t)) =>
+        given Quotes = tree.symbol.asQuotes
         val rhs = '{
           @hello def foo(x: Int): Int = x + 1
           ${t.asExprOf[Int]}

diff --git a/tests/run-macros/annot-memo/Macro_1.scala b/tests/run-macros/annot-memo/Macro_1.scala
@@ -7,14 +7,20 @@ class memoize extends MacroAnnotation:
   def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
     import quotes.reflect._
     tree match
-      case DefDef(name, TermParamClause(param :: Nil) :: Nil, tpt, Some(rhsTree)) =>
-        (Ref(param.symbol).asExpr, rhsTree.asExpr) match
-          case ('{ $paramRefExpr: t }, '{ $rhsExpr: u }) =>
-            val cacheSymbol = Symbol.newUniqueVal(tree.symbol.owner, name + "Cache", TypeRepr.of[mutable.Map[t, u]], Flags.Private, Symbol.noSymbol)
-            val cacheRhs = '{ mutable.Map.empty[t, u] }.asTerm
+      case DefDef(name, TermParamClause(param  :: Nil) :: Nil, tpt, Some(rhsTree)) =>
+        (param.tpt.tpe.asType, tpt.tpe.asType) match
+          case ('[t], '[u]) =>
+            val cacheSymbol = Symbol.newUniqueVal(Symbol.spliceOwner, name + "Cache", TypeRepr.of[mutable.Map[t, u]], Flags.Private, Symbol.noSymbol)
+            val cacheRhs =
+              given Quotes = cacheSymbol.asQuotes
+              '{ mutable.Map.empty[t, u] }.asTerm
             val cacheVal = ValDef(cacheSymbol, Some(cacheRhs))
-            val cacheRefExpr = Ref(cacheSymbol).asExprOf[mutable.Map[t, u]]
-            val newRhs = '{ $cacheRefExpr.getOrElseUpdate($paramRefExpr, $rhsExpr) }.asTerm
+            val newRhs =
+              given Quotes = tree.symbol.asQuotes
+              val cacheRefExpr = Ref(cacheSymbol).asExprOf[mutable.Map[t, u]]
+              val paramRefExpr = Ref(param.symbol).asExprOf[t]
+              val rhsExpr = rhsTree.asExprOf[u]
+              '{ $cacheRefExpr.getOrElseUpdate($paramRefExpr, $rhsExpr) }.asTerm
             val newTree = DefDef.copy(tree)(name, TermParamClause(param :: Nil) :: Nil, tpt, Some(newRhs))
             List(cacheVal, newTree)
       case _ =>

diff --git a/tests/run-macros/annot-memo/Test_2.scala b/tests/run-macros/annot-memo/Test_2.scala
@@ -4,6 +4,13 @@ class Bar:
     println(s"compute fib of $n")
     if n <= 1 then n
     else fib(n - 1) + fib(n - 2)
+  //> private val fibCache$macro$1: mutable.Map[Int, Int] = mutable.Map.empty[Int, Int]
+  //> @memoize def fib(n: Int): Int =
+  //>  fibCache$macro$1.getOrElseUpdate(n, {
+  //>     println(s"compute fib of $n")
+  //>     if n <= 1 then n
+  //>     else fib(n - 1) + fib(n - 2)
+  //>   })
 
   @memoize
   def fib(n: Long): Long =

diff --git a/tests/run-macros/annot-result-order/Macro_1.scala b/tests/run-macros/annot-result-order/Macro_1.scala
@@ -6,7 +6,9 @@ class print(msg: String) extends MacroAnnotation:
   def transform(using Quotes)(tree: quotes.reflect.Definition): List[quotes.reflect.Definition] =
     import quotes.reflect._
     def printMsg(msg: String) =
-      val valSym = Symbol.newUniqueVal(tree.symbol.owner, tree.symbol.name + "$print$" + msg, TypeRepr.of[Unit], Flags.Private, Symbol.noSymbol)
-      val valRhs = '{ println(${Expr(msg)}) }.asTerm
+      val valSym = Symbol.newUniqueVal(Symbol.spliceOwner, tree.symbol.name + "$print$" + msg, TypeRepr.of[Unit], Flags.Private, Symbol.noSymbol)
+      val valRhs =
+        given Quotes = valSym.asQuotes
+        '{ println(${Expr(msg)}) }.asTerm
       ValDef(valSym, Some(valRhs))
     List(printMsg(s"before: $msg"), tree, printMsg(s"after: $msg"))
diff --git a/tests/run-macros/annot-simple-fib/Macro_1.scala b/tests/run-macros/annot-simple-fib/Macro_1.scala
@@ -8,19 +8,23 @@ class memoize extends MacroAnnotation {
     import quotes.reflect._
     tree match
       case DefDef(name, params, tpt, Some(fibTree)) =>
-        val cacheRhs = '{Map.empty[Int, Int]}.asTerm
-        val cacheSymbol = Symbol.newUniqueVal(tree.symbol.owner, name + "Cache", TypeRepr.of[Map[Int, Int]], Flags.EmptyFlags, Symbol.noSymbol)
+        val cacheSymbol = Symbol.newUniqueVal(Symbol.spliceOwner, name + "Cache", TypeRepr.of[Map[Int, Int]], Flags.EmptyFlags, Symbol.noSymbol)
+        val cacheRhs =
+          given Quotes = cacheSymbol.asQuotes
+          '{Map.empty[Int, Int]}.asTerm
         val cacheVal = ValDef(cacheSymbol, Some(cacheRhs))
-        val fibCache = Ref(cacheSymbol).asExprOf[Map[Int, Int]]
-        val n = Ref(params.head.params.head.symbol).asExprOf[Int]
-        val rhs = '{
-          if $fibCache.contains($n) then
-              $fibCache($n)
-          else
-            val res = ${fibTree.asExprOf[Int]}
-            $fibCache($n) = res
-            res
-        }.asTerm
+        val rhs =
+          given Quotes = tree.symbol.asQuotes
+          val fibCache = Ref(cacheSymbol).asExprOf[Map[Int, Int]]
+          val n = Ref(params.head.params.head.symbol).asExprOf[Int]
+          '{
+            if $fibCache.contains($n) then
+                $fibCache($n)
+            else
+              val res = ${fibTree.asExprOf[Int]}
+              $fibCache($n) = res
+              res
+          }.asTerm
         val newFib = DefDef.copy(tree)(name, params, tpt, Some(rhs))
         List(cacheVal, newFib)
 }