Skip to content

Fix macro annotations spliceOwner #16513

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 19, 2022
Merged

Fix macro annotations spliceOwner #16513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions compiler/src/dotty/tools/dotc/quoted/PickledQuotes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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._
Expand Down Expand Up @@ -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
}

}
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/MacroAnnotations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
4 changes: 3 additions & 1 deletion compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
20 changes: 13 additions & 7 deletions library/src/scala/annotation/MacroAnnotation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all these macros here and in run-macros changed? Does the old implementation not work anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the old implementation still works but is suboptimal and has a bug. It had three issues:

  • With quotes getting unpickled in with the wrong owner each splice has the potential to trigger an owner change.
  • If users would assemble the expression using reflection they would need extra explicit changeOwner calls
  • To recover the type of the signature we should look at the signature and not the terms. In the previous version, we had an accidental term reference to the parameter of the annotated def that leaked into the new field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative way to construct the RHSs will be added in the documentation in #16454.

https://github.com/lampepfl/dotty/pull/16454/files#diff-0a9170b85c5b55edde099043317e7f1e48b3d8998b34707be1596b8161e40f89R85-R165

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the context changes simpler, I plan to add a method like inContext.

Something like

val rhs = inQuotes(owner){ '{...} }

* 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 _ =>
Expand Down
2 changes: 1 addition & 1 deletion tests/neg-macros/annot-accessIndirect/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion tests/neg-macros/annot-accessIndirect/Macro_2.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '{
Expand Down
4 changes: 3 additions & 1 deletion tests/pos-macros/annot-then-inline/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
}

Expand Down
1 change: 1 addition & 0 deletions tests/run-macros/annot-annot-order/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/run-macros/annot-bind/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions tests/run-macros/annot-gen2/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions tests/run-macros/annot-gen2/Macro_2.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ${
Expand Down
2 changes: 1 addition & 1 deletion tests/run-macros/annot-generate/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
1 change: 1 addition & 0 deletions tests/run-macros/annot-generate/Macro_2.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]}
Expand Down
20 changes: 13 additions & 7 deletions tests/run-macros/annot-memo/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 _ =>
Expand Down
7 changes: 7 additions & 0 deletions tests/run-macros/annot-memo/Test_2.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
6 changes: 4 additions & 2 deletions tests/run-macros/annot-result-order/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
28 changes: 16 additions & 12 deletions tests/run-macros/annot-simple-fib/Macro_1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}