Skip to content

Fix #6909: Cache alias givens in lazy vals #7006

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

Merged
merged 10 commits into from
Aug 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,10 @@ class JSCodeGen()(implicit ctx: Context) {
case EmptyTree => ()
case dd: DefDef => generatedMethods ++= genMethod(dd)
case _ =>
throw new FatalError("Illegal tree in gen of genInterface(): " + tree)
throw new FatalError(
i"""Illegal tree in gen of genInterface(): $tree
|class = $td
|in ${ctx.compilationUnit}""")
}
}

Expand Down
42 changes: 22 additions & 20 deletions compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
Original file line number Diff line number Diff line change
Expand Up @@ -243,20 +243,6 @@ trait TreeInfo[T >: Untyped <: Type] { self: Trees.Instance[T] =>
case y => y
}

/** The largest subset of {NoInits, PureInterface} that a
* trait or class enclosing this statement can have as flags.
*/
def defKind(tree: Tree)(implicit ctx: Context): FlagSet = unsplice(tree) match {
case EmptyTree | _: Import => NoInitsInterface
case tree: TypeDef => if (tree.isClassDef) NoInits else NoInitsInterface
case tree: DefDef =>
if (tree.unforcedRhs == EmptyTree &&
tree.vparamss.forall(_.forall(_.rhs.isEmpty))) NoInitsInterface
else NoInits
case tree: ValDef => if (tree.unforcedRhs == EmptyTree) NoInitsInterface else EmptyFlags
case _ => EmptyFlags
}

/** The largest subset of {NoInits, PureInterface} that a
* trait or class with these parents can have as flags.
*/
Expand All @@ -266,12 +252,6 @@ trait TreeInfo[T >: Untyped <: Type] { self: Trees.Instance[T] =>
case _ :: parents1 => parentsKind(parents1)
}

/** The largest subset of {NoInits, PureInterface} that a
* trait or class with this body can have as flags.
*/
def bodyKind(body: List[Tree])(implicit ctx: Context): FlagSet =
(NoInitsInterface /: body)((fs, stat) => fs & defKind(stat))

/** Checks whether predicate `p` is true for all result parts of this expression,
* where we zoom into Ifs, Matches, and Blocks.
*/
Expand Down Expand Up @@ -342,6 +322,28 @@ trait UntypedTreeInfo extends TreeInfo[Untyped] { self: Trees.Instance[Untyped]
case _ => false
}

/** The largest subset of {NoInits, PureInterface} that a
* trait or class enclosing this statement can have as flags.
*/
def defKind(tree: Tree)(implicit ctx: Context): FlagSet = unsplice(tree) match {
case EmptyTree | _: Import => NoInitsInterface
case tree: TypeDef => if (tree.isClassDef) NoInits else NoInitsInterface
case tree: DefDef =>
if (tree.unforcedRhs == EmptyTree &&
tree.vparamss.forall(_.forall(_.rhs.isEmpty))) NoInitsInterface
else if (tree.mods.is(Given) && tree.tparams.isEmpty && tree.vparamss.isEmpty)
EmptyFlags // might become a lazy val: TODO: check whether we need to suppress NoInits once we have new lazy val impl
else NoInits
case tree: ValDef => if (tree.unforcedRhs == EmptyTree) NoInitsInterface else EmptyFlags
case _ => EmptyFlags
}

/** The largest subset of {NoInits, PureInterface} that a
* trait or class with this body can have as flags.
*/
def bodyKind(body: List[Tree])(implicit ctx: Context): FlagSet =
(NoInitsInterface /: body)((fs, stat) => fs & defKind(stat))

// todo: fill with other methods from TreeInfo that only apply to untpd.Tree's
}

Expand Down
8 changes: 6 additions & 2 deletions compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,12 @@ trait SymDenotations { this: Context =>
}
}

/** Configurable: Accept stale symbol with warning if in IDE */
def staleOK: Boolean = Config.ignoreStaleInIDE && mode.is(Mode.Interactive)
/** Configurable: Accept stale symbol with warning if in IDE
* Always accept stale symbols when testing pickling.
*/
def staleOK: Boolean =
Config.ignoreStaleInIDE && mode.is(Mode.Interactive) ||
settings.YtestPickler.value

/** Possibly accept stale symbol with warning if in IDE */
def acceptStale(denot: SingleDenotation): Boolean =
Expand Down
25 changes: 14 additions & 11 deletions compiler/src/dotty/tools/dotc/core/Symbols.scala
Original file line number Diff line number Diff line change
Expand Up @@ -553,9 +553,10 @@ object Symbols {

/** This symbol entered into owner's scope (owner must be a class). */
final def entered(implicit ctx: Context): this.type = {
assert(this.owner.isClass, s"symbol ($this) entered the scope of non-class owner ${this.owner}") // !!! DEBUG
this.owner.asClass.enter(this)
if (this.is(Module)) this.owner.asClass.enter(this.moduleClass)
if (this.owner.isClass) {
this.owner.asClass.enter(this)
if (this.is(Module)) this.owner.asClass.enter(this.moduleClass)
}
this
}

Expand All @@ -566,14 +567,16 @@ object Symbols {
*/
def enteredAfter(phase: DenotTransformer)(implicit ctx: Context): this.type =
if (ctx.phaseId != phase.next.id) enteredAfter(phase)(ctx.withPhase(phase.next))
else {
if (this.owner.is(Package)) {
denot.validFor |= InitialPeriod
if (this.is(Module)) this.moduleClass.validFor |= InitialPeriod
}
else this.owner.asClass.ensureFreshScopeAfter(phase)
assert(isPrivate || phase.changesMembers, i"$this entered in ${this.owner} at undeclared phase $phase")
entered
else this.owner match {
case owner: ClassSymbol =>
if (owner.is(Package)) {
denot.validFor |= InitialPeriod
if (this.is(Module)) this.moduleClass.validFor |= InitialPeriod
}
else owner.ensureFreshScopeAfter(phase)
assert(isPrivate || phase.changesMembers, i"$this entered in $owner at undeclared phase $phase")
entered
case _ => this
}

/** Remove symbol from scope of owning class */
Expand Down
77 changes: 25 additions & 52 deletions compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,18 @@ object CacheAliasImplicits {
* is cached. It applies to all alias implicits that have neither type parameters
* nor a given clause. Example: The alias
*
* implicit a for TC = rhs
* given a as TC = rhs
*
* is expanded before this phase
* is expanded before this phase to:
*
* implicit def a: TC = rhs
*
* It is then expanded further as follows:
*
* 1. If `rhs` is a simple name `x` (possibly with a `this.` prefix), leave the definition as is.
* 2. Otherwise, if `rhs` is a pure path, replace the definition with
* 1. If `rhs` is a simple name `x` (possibly with a `this.` prefix) that
* refers to a value, leave it as is.
*
* implicit val a: TC = rhs
*
* 3. Otherwise, if `TC` is a reference type, replace the definition with
*
* private[this] var a$_cache: TC = null
* implicit def a: TC = { if (a$_cache == null) a$_cache = rhs; a$_cache }
*
* 4. Otherwise `TC` is a value type. Replace the definition with
* 2. Otherwise, replace the definition with
*
* lazy implicit val a: TC = rhs
*/
Expand All @@ -56,47 +49,27 @@ class CacheAliasImplicits extends MiniPhase with IdentityDenotTransformer { this

override def transformDefDef(tree: DefDef)(implicit ctx: Context): Tree = {
val sym = tree.symbol
sym.info match {
case ExprType(rhsType) if sym.is(Given, butNot = CacheAliasImplicits.NoCacheFlags) =>
// If rhs is a simple TermRef, leave a def.
tree.rhs.tpe match {
case TermRef(pre, _) =>
pre match {
case NoPrefix => return tree
case pre: ThisType if pre.cls == ctx.owner.enclosingClass => return tree
case _ =>
}
case _ =>
}
def makeVal(additionalFlags: FlagSet) = {
sym.copySymDenotation(
initFlags = sym.flags &~ Method | additionalFlags,
info = rhsType)
.installAfter(thisPhase)
cpy.ValDef(tree)(tree.name, tree.tpt, tree.rhs)
}
if (isPurePath(tree.rhs)) makeVal(EmptyFlags)
else if (rhsType.classSymbol.isValueClass ||
!erasure(rhsType).typeSymbol.derivesFrom(defn.ObjectClass)) makeVal(Lazy)
else {
val cacheFlags = if (ctx.owner.isClass) Private | Local | Mutable else Mutable
val cacheSym =
ctx.newSymbol(ctx.owner, CacheName(tree.name), cacheFlags, rhsType, coord = sym.coord)
if (ctx.owner.isClass) cacheSym.enteredAfter(thisPhase)
val cacheDef = ValDef(cacheSym, tpd.defaultValue(rhsType))
val cachingDef = cpy.DefDef(tree)(rhs =
Block(
If(
ref(cacheSym).select(defn.Any_==).appliedTo(nullLiteral),
Assign(ref(cacheSym), tree.rhs),
unitLiteral) :: Nil,
ref(cacheSym)
)
)
Thicket(cacheDef, cachingDef)
}
case _ => tree
val isCached = !sym.is(Inline) && {
sym.info match {
case ExprType(resTpe) if sym.is(Given, butNot = CacheAliasImplicits.NoCacheFlags) =>
tree.rhs.tpe match {
case rhsTpe @ TermRef(NoPrefix, _)
if rhsTpe.isStable => false
case rhsTpe @ TermRef(pre: ThisType, _)
if rhsTpe.isStable && pre.cls == sym.owner.enclosingClass => false
case _ => true
}
case _ => false
}
}
if (isCached) {
sym.copySymDenotation(
initFlags = sym.flags &~ Method | Lazy,
info = sym.info.widenExpr)
.installAfter(thisPhase)
cpy.ValDef(tree)(tree.name, tree.tpt, tree.rhs)
}
else tree
}
}

Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ class HoistSuperArgs extends MiniPhase with IdentityDenotTransformer { thisPhase
val argTypeWrtConstr = argType.subst(origParams, allParamRefs(constr.info))
// argType with references to paramRefs of the primary constructor instead of
// local parameter accessors
val meth = ctx.newSymbol(
ctx.newSymbol(
owner = methOwner,
name = SuperArgName.fresh(cls.name.toTermName),
flags = Synthetic | Private | Method | staticFlag,
info = replaceResult(constr.info, argTypeWrtConstr),
coord = constr.coord)
if (methOwner.isClass) meth.enteredAfter(thisPhase) else meth
coord = constr.coord
).enteredAfter(thisPhase)
}

/** Type of a reference implies that it needs to be hoisted */
Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/LambdaLift.scala
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,9 @@ object LambdaLift {
proxyMap(owner) = {
for (fv <- freeValues.toList) yield {
val proxyName = newName(fv)
val proxy = ctx.newSymbol(owner, proxyName.asTermName, newFlags, fv.info, coord = fv.coord)
if (owner.isClass) proxy.enteredAfter(thisPhase)
val proxy =
ctx.newSymbol(owner, proxyName.asTermName, newFlags, fv.info, coord = fv.coord)
.enteredAfter(thisPhase)
(fv, proxy)
}
}.toMap
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,7 @@ class Namer { typer: Typer =>
if (isDerivedValueClass(cls)) cls.setFlag(Final)
cls.info = avoidPrivateLeaks(cls)
cls.baseClasses.foreach(_.invalidateBaseTypeCache()) // we might have looked before and found nothing
cls.setNoInitsFlags(parentsKind(parents), bodyKind(rest))
cls.setNoInitsFlags(parentsKind(parents), untpd.bodyKind(rest))
if (cls.isNoInitsClass) cls.primaryConstructor.setFlag(StableRealizable)
processExports(localCtx)
}
Expand Down
14 changes: 6 additions & 8 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2132,15 +2132,13 @@ class Typer extends Namer
buf += inlineExpansion(mdef1)
// replace body with expansion, because it will be used as inlined body
// from separately compiled files - the original BodyAnnotation is not kept.
case mdef1: TypeDef if mdef1.symbol.is(Enum, butNot = Case) =>
enumContexts(mdef1.symbol) = ctx
buf += mdef1
case EmptyTree =>
// clashing synthetic case methods are converted to empty trees, drop them here
case mdef1 =>
import untpd.modsDeco
mdef match {
case mdef: untpd.TypeDef if mdef.mods.isEnumClass =>
enumContexts(mdef1.symbol) = ctx
case _ =>
}
if (!mdef1.isEmpty) // clashing synthetic case methods are converted to empty trees
buf += mdef1
buf += mdef1
}
traverse(rest)
}
Expand Down
19 changes: 4 additions & 15 deletions docs/docs/reference/contextual/relationship-implicits.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,32 +28,21 @@ Given instances can be mapped to combinations of implicit objects, classes and i
class ListOrd[T](implicit ord: Ord[T]) extends Ord[List[T]] { ... }
final implicit def ListOrd[T](implicit ord: Ord[T]): ListOrd[T] = new ListOrd[T]
```
3. Alias givens map to implicit methods. If an alias has neither type parameters nor a given clause, its right-hand side is cached in a variable. There are two cases that can be optimized:

- If the right hand side is a simple reference, we can
use a forwarder to that reference without caching it.
- If the right hand side is more complex, but still known to be pure, we can
create a `val` that computes it ahead of time.
3. Alias givens map to implicit methods or implicit lazy vals. If an alias has neither type parameters nor a given clause,
it is treated as a lazy val, unless the right hand side is a simple reference, in which case we can use a forwarder to that
reference without caching it.

Examples:

```scala
given global as ExecutionContext = new ForkJoinContext()
given config as Config = default.config

val ctx: Context
given as Context = ctx
```
would map to
```scala
private[this] var global$cache: ExecutionContext | Null = null
final implicit def global: ExecutionContext = {
if (global$cache == null) global$cache = new ForkJoinContext()
global$cache
}

final implicit val config: Config = default.config

final implicit lazy val global: ExecutionContext = new ForkJoinContext()
final implicit def Context_given = ctx
```

Expand Down
7 changes: 7 additions & 0 deletions tests/pos/i6909.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import scala.compiletime
trait Foo[A]


trait Qux {
given as Foo[Int] = new Foo[Int] {}
}