Skip to content

Contextualize owners in reflection API #10394

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

Closed
wants to merge 1 commit into from
Closed

Contextualize owners in reflection API #10394

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
6 changes: 3 additions & 3 deletions compiler/src/scala/quoted/internal/impl/Matcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ object Matcher {
def bodyFn(lambdaArgs: List[Tree]): Tree = {
val argsMap = args.map(_.symbol).zip(lambdaArgs.asInstanceOf[List[Term]]).toMap
new TreeMap {
override def transformTerm(tree: Term)(using ctx: Context): Term =
override def transformTerm(tree: Term)(using owner: Owner): Term =
tree match
case tree: Ident => summon[Env].get(tree.symbol).flatMap(argsMap.get).getOrElse(tree)
case tree => super.transformTerm(tree)
Expand All @@ -211,7 +211,7 @@ object Matcher {
}
val argTypes = args.map(x => x.tpe.widenTermRefExpr)
val resType = pattern.tpe
val res = Lambda(Symbol.currentOwner, MethodType(names)(_ => argTypes, _ => resType), (meth, x) => bodyFn(x).changeOwner(meth))
val res = Lambda(MethodType(names)(_ => argTypes, _ => resType), x => bodyFn(x).adaptOwner)
matched(res.asExpr)

//
Expand Down Expand Up @@ -354,7 +354,7 @@ object Matcher {
/** Return all free variables of the term defined in the pattern (i.e. defined in `Env`) */
def freePatternVars(term: Term)(using ctx: Context, env: Env): Set[Symbol] =
val accumulator = new TreeAccumulator[Set[Symbol]] {
def foldTree(x: Set[Symbol], tree: Tree)(using ctx: Context): Set[Symbol] =
def foldTree(x: Set[Symbol], tree: Tree)(using owner: Owner): Set[Symbol] =
tree match
case tree: Ident if env.contains(tree.symbol) => foldOverTree(x + tree.symbol, tree)
case _ => foldOverTree(x, tree)
Expand Down
45 changes: 30 additions & 15 deletions compiler/src/scala/quoted/internal/impl/QuoteContextImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
end extension

extension [ThisTree <: Tree](self: ThisTree):
def changeOwner(newOwner: Symbol): ThisTree =
def adaptOwner(using newOwner: Symbol): ThisTree =
tpd.TreeOps(self).changeNonLocalOwners(newOwner).asInstanceOf[ThisTree]
Copy link
Contributor

Choose a reason for hiding this comment

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

When a meta-programmer calls this method explicitly, it seems better to use explicit arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to handle them explicitly, but this would complicate quite a few abstractions. Mainly TreeTraverser, TreeMap, TreeAccumulator and ValDef.let

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it might be better to change using newOwner: Symbol to newOwner: Symbol. The reason is about consistency: meta-programmers are already calling the method explicitly, it does not make sense to make its only important argument implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one parameter that can be passed here which is the current owner. Which is itself looking for this implicit and returning it. This has the same implicitness but requires the user to write the same parameter on every use site.

end extension

Expand Down Expand Up @@ -278,13 +278,14 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
def unapply(vdef: ValDef): Option[(String, TypeTree, Option[Term])] =
Some((vdef.name.toString, vdef.tpt, optional(vdef.rhs)))

def let(name: String, rhs: Term)(body: Ident => Term): Term =
val vdef = tpd.SyntheticValDef(name.toTermName, rhs)
def let(name: String, rhs: Term)(body: Ident => Term)(using owner: Owner): Term =
val vdef = tpd.SyntheticValDef(name.toTermName, rhs)(using ctx.withOwner(owner))
val ref = tpd.ref(vdef.symbol).asInstanceOf[Ident]
Block(List(vdef), body(ref))

def let(terms: List[Term])(body: List[Ident] => Term): Term =
val vdefs = terms.map(term => tpd.SyntheticValDef("x".toTermName, term))
def let(terms: List[Term])(body: List[Ident] => Term)(using owner: Owner): Term =
val ctx1 = ctx.withOwner(owner)
val vdefs = terms.map(term => tpd.SyntheticValDef("x".toTermName, term)(using ctx1))
val refs = vdefs.map(vdef => tpd.ref(vdef.symbol).asInstanceOf[Ident])
Block(vdefs, body(refs))
end ValDef
Expand Down Expand Up @@ -365,15 +366,15 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
def tpe: TypeRepr = self.tpe
def underlyingArgument: Term = new tpd.TreeOps(self).underlyingArgument
def underlying: Term = new tpd.TreeOps(self).underlying
def etaExpand(owner: Symbol): Term = self.tpe.widen match {
def etaExpand(using owner: Owner): Term = self.tpe.widen match {
case mtpe: Types.MethodType if !mtpe.isParamDependent =>
val closureResType = mtpe.resType match {
case t: Types.MethodType => t.toFunctionType()
case t => t
}
val closureTpe = Types.MethodType(mtpe.paramNames, mtpe.paramInfos, closureResType)
val closureMethod = dotc.core.Symbols.newSymbol(owner, nme.ANON_FUN, Synthetic | Method, closureTpe)
tpd.Closure(closureMethod, tss => new tpd.TreeOps(self).appliedToArgs(tss.head).etaExpand(closureMethod))
tpd.Closure(closureMethod, tss => new tpd.TreeOps(self).appliedToArgs(tss.head).etaExpand(using closureMethod))
case _ => self
}

Expand Down Expand Up @@ -735,9 +736,9 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
end ClosureMethodsImpl

object Lambda extends LambdaModule:
def apply(owner: Symbol, tpe: MethodType, rhsFn: (Symbol, List[Tree]) => Tree): Block =
def apply(tpe: MethodType, rhsFn: Owner ?=> List[Tree] => Tree)(using owner: Owner): Block =
val meth = dotc.core.Symbols.newSymbol(owner, nme.ANON_FUN, Synthetic | Method, tpe)
tpd.Closure(meth, tss => yCheckedOwners(rhsFn(meth, tss.head), meth))
tpd.Closure(meth, tss => yCheckedOwners(rhsFn(using meth)(tss.head), meth))

def unapply(tree: Block): Option[(List[ValDef], Term)] = tree match {
case Block((ddef @ DefDef(_, _, params :: Nil, _, Some(body))) :: Nil, Closure(meth, _))
Expand Down Expand Up @@ -2200,22 +2201,35 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
case _ => None
end AmbiguousImplicitsTypeTest

type Owner = dotc.core.Symbols.Symbol
Copy link
Contributor

Choose a reason for hiding this comment

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

I still find this introduces unnecessary conceptual complexity to meta-programmers. The programming model with explicit owners seems to be better and simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want it explicit we would also need to use it explicitly in the TreeTraverser, TreeMap, TreeAccumulator and ValDef.let. We would also need to remove Symbol.currentOwner. That would be quite annoying.

Note that the current design already has a contextual owner mechanism but it is hidden behind the Context abstraction. No one has understood how to propagate this correctly so far. As this PR shows even I missed the connection between the two concepts in some cases. By making it clear what we actually propagate contextually it was trivially obvious where it was missing.


object Owner extends OwnerModule:
def apply(sym: Symbol): Owner =
assert(sym.exists, "Cannot convert NoSymbol into an Owner")
sym
def spliceOwner: Owner = ctx.owner
end Owner

object OwnerMethodsImpl extends OwnerMethods:
extension (self: Owner):
def symbol: Symbol = self
end OwnerMethodsImpl

type Symbol = dotc.core.Symbols.Symbol

object Symbol extends SymbolModule:
def currentOwner(using ctx: Context): Symbol = ctx.owner
def requiredPackage(path: String): Symbol = dotc.core.Symbols.requiredPackage(path)
def requiredClass(path: String): Symbol = dotc.core.Symbols.requiredClass(path)
def requiredModule(path: String): Symbol = dotc.core.Symbols.requiredModule(path)
def requiredMethod(path: String): Symbol = dotc.core.Symbols.requiredMethod(path)
def classSymbol(fullName: String): Symbol = dotc.core.Symbols.requiredClass(fullName)
def newMethod(owner: Symbol, name: String, tpe: TypeRepr): Symbol =
newMethod(owner, name, tpe, Flags.EmptyFlags, noSymbol)
def newMethod(owner: Symbol, name: String, tpe: TypeRepr, flags: Flags, privateWithin: Symbol): Symbol =
def newMethod(name: String, tpe: TypeRepr)(using owner: Owner): Symbol =
newMethod(name, tpe, Flags.EmptyFlags, noSymbol)
def newMethod(name: String, tpe: TypeRepr, flags: Flags, privateWithin: Symbol)(using owner: Owner): Symbol =
dotc.core.Symbols.newSymbol(owner, name.toTermName, flags | dotc.core.Flags.Method, tpe, privateWithin)
def newVal(owner: Symbol, name: String, tpe: TypeRepr, flags: Flags, privateWithin: Symbol): Symbol =
def newVal(name: String, tpe: TypeRepr, flags: Flags, privateWithin: Symbol)(using owner: Owner): Symbol =
dotc.core.Symbols.newSymbol(owner, name.toTermName, flags, tpe, privateWithin)
def newBind(owner: Symbol, name: String, flags: Flags, tpe: TypeRepr): Symbol =
def newBind(name: String, flags: Flags, tpe: TypeRepr)(using owner: Owner): Symbol =
dotc.core.Symbols.newSymbol(owner, name.toTermName, flags | Case, tpe)
def noSymbol: Symbol = dotc.core.Symbols.NoSymbol
Copy link
Contributor

Choose a reason for hiding this comment

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

Since creating a symbol is untrivial, it might be better to be explicit about the owner instead of being implicit. This way, they have a consistent model about how symbols work, the invariants, how to debug and fix owner-related problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The early owner check has proven to make simplify any bugs in the owners at the point where it is mishandled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think part of the ergonomics of the API is a simple and consistent programming model. As we cannot completely hide the technical detail of owners and the owner-invariant is error-prone, it might be better to make them explicit:

  • meta-programmers will learn a consistent model for symbols, where each symbol has an owner.
  • they will be more careful when using the APIs. If they mess up with owners, they will not blame the macro system. If the argument is implicit, part of the responsibility of the owner-errors is due to the macro system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I will try to make a version where we handle owners explicitly. I believe this will not be as simple to handle.

Copy link
Contributor Author

@nicolasstucki nicolasstucki Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I added a version of the ValDef.let fix in #10402

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added #10406 with a version where owners are handled explicitly

end Symbol
Expand Down Expand Up @@ -2593,6 +2607,7 @@ class QuoteContextImpl private (ctx: Context) extends QuoteContext, QuoteUnpickl
|which has the AST representation
|${TreeMethods.showExtractors(tree)}
|
|Owners can be adapted using `Tree.adaptOwner`.
|""".stripMargin)
case _ => traverseChildren(t)
}.traverse(tree)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/scala/quoted/internal/impl/printers/SourceCode.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1409,7 +1409,7 @@ object SourceCode {
private[this] val namesIndex = collection.mutable.Map.empty[String, Int]

private def splicedName(sym: Symbol): Option[String] = {
if sym.owner.isClassDef then None
if sym.maybeOwner.isClassDef then None
else names.get(sym).orElse {
val name0 = sym.name
val index = namesIndex.getOrElse(name0, 1)
Expand Down
34 changes: 16 additions & 18 deletions library/src-bootstrapped/scala/quoted/ExprMap.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ trait ExprMap:
import qctx.reflect._
final class MapChildren() {

def transformStatement(tree: Statement)(using ctx: Context): Statement = {
def localCtx(definition: Definition): Context = definition.symbol.localContext
def transformStatement(tree: Statement)(using owner: Owner): Statement = {
tree match {
case tree: Term =>
transformTerm(tree, TypeRepr.of[Any])
Expand All @@ -22,15 +21,14 @@ trait ExprMap:
}
}

def transformDefinition(tree: Definition)(using ctx: Context): Definition = {
def localCtx(definition: Definition): Context = definition.symbol.localContext
def transformDefinition(tree: Definition)(using owner: Owner): Definition = {
tree match {
case tree: ValDef =>
given Context = localCtx(tree)
given Owner = Owner(tree.symbol)
val rhs1 = tree.rhs.map(x => transformTerm(x, tree.tpt.tpe))
ValDef.copy(tree)(tree.name, tree.tpt, rhs1)
case tree: DefDef =>
given Context = localCtx(tree)
given Owner = Owner(tree.symbol)
DefDef.copy(tree)(tree.name, tree.typeParams, tree.paramss, tree.returnTpt, tree.rhs.map(x => transformTerm(x, tree.returnTpt.tpe)))
case tree: TypeDef =>
tree
Expand All @@ -40,7 +38,7 @@ trait ExprMap:
}
}

def transformTermChildren(tree: Term, tpe: TypeRepr)(using ctx: Context): Term = tree match {
def transformTermChildren(tree: Term, tpe: TypeRepr)(using owner: Owner): Term = tree match {
case Ident(name) =>
tree
case Select(qualifier, name) =>
Expand Down Expand Up @@ -94,7 +92,7 @@ trait ExprMap:
Inlined.copy(tree)(call, transformDefinitions(bindings), transformTerm(expansion, tpe)/*()call.symbol.localContext)*/)
}

def transformTerm(tree: Term, tpe: TypeRepr)(using ctx: Context): Term =
def transformTerm(tree: Term, tpe: TypeRepr)(using owner: Owner): Term =
tree match
case _: Closure =>
tree
Expand All @@ -109,39 +107,39 @@ trait ExprMap:
case _ =>
transformTermChildren(tree, tpe)

def transformTypeTree(tree: TypeTree)(using ctx: Context): TypeTree = tree
def transformTypeTree(tree: TypeTree)(using owner: Owner): TypeTree = tree

def transformCaseDef(tree: CaseDef, tpe: TypeRepr)(using ctx: Context): CaseDef =
def transformCaseDef(tree: CaseDef, tpe: TypeRepr)(using owner: Owner): CaseDef =
CaseDef.copy(tree)(tree.pattern, tree.guard.map(x => transformTerm(x, TypeRepr.of[Boolean])), transformTerm(tree.rhs, tpe))

def transformTypeCaseDef(tree: TypeCaseDef)(using ctx: Context): TypeCaseDef = {
def transformTypeCaseDef(tree: TypeCaseDef)(using owner: Owner): TypeCaseDef = {
TypeCaseDef.copy(tree)(transformTypeTree(tree.pattern), transformTypeTree(tree.rhs))
}

def transformStats(trees: List[Statement])(using ctx: Context): List[Statement] =
def transformStats(trees: List[Statement])(using owner: Owner): List[Statement] =
trees mapConserve (transformStatement(_))

def transformDefinitions(trees: List[Definition])(using ctx: Context): List[Definition] =
def transformDefinitions(trees: List[Definition])(using owner: Owner): List[Definition] =
trees mapConserve (transformDefinition(_))

def transformTerms(trees: List[Term], tpes: List[TypeRepr])(using ctx: Context): List[Term] =
def transformTerms(trees: List[Term], tpes: List[TypeRepr])(using owner: Owner): List[Term] =
var tpes2 = tpes // TODO use proper zipConserve
trees mapConserve { x =>
val tpe :: tail = tpes2
tpes2 = tail
transformTerm(x, tpe)
}

def transformTerms(trees: List[Term], tpe: TypeRepr)(using ctx: Context): List[Term] =
def transformTerms(trees: List[Term], tpe: TypeRepr)(using owner: Owner): List[Term] =
trees.mapConserve(x => transformTerm(x, tpe))

def transformTypeTrees(trees: List[TypeTree])(using ctx: Context): List[TypeTree] =
def transformTypeTrees(trees: List[TypeTree])(using owner: Owner): List[TypeTree] =
trees mapConserve (transformTypeTree(_))

def transformCaseDefs(trees: List[CaseDef], tpe: TypeRepr)(using ctx: Context): List[CaseDef] =
def transformCaseDefs(trees: List[CaseDef], tpe: TypeRepr)(using owner: Owner): List[CaseDef] =
trees mapConserve (x => transformCaseDef(x, tpe))

def transformTypeCaseDefs(trees: List[TypeCaseDef])(using ctx: Context): List[TypeCaseDef] =
def transformTypeCaseDefs(trees: List[TypeCaseDef])(using owner: Owner): List[TypeCaseDef] =
trees mapConserve (transformTypeCaseDef(_))

}
Expand Down
Loading