From 2278e7dbfbe434b344d71bed131c4dc20ff70013 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Thu, 18 Jul 2019 22:58:45 +0200 Subject: [PATCH 1/8] Don't exit with StaleSymbol errors when testing pickling. Stale symbol errors can happen when inspecting symbols while comparing the trees before and after pickling. --- compiler/src/dotty/tools/dotc/core/SymDenotations.scala | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 7c62a812595c..373fa83b1c96 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala @@ -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 = From 9b24e64c481ef45849d6c31a573364a215141a0b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 30 Jul 2019 16:13:43 +0200 Subject: [PATCH 2/8] Add @cached annotation Syntax: ``` @scala.annotation.meta.cached def foo = rhs ``` It's like a lazy val, except that - it's not synchronized - it uses `null` as uninitialized value - it can have given parameters Use the same logic for caching given aliases --- compiler/src/dotty/tools/dotc/Compiler.scala | 2 +- .../dotty/tools/dotc/core/Definitions.scala | 1 + .../tools/dotc/transform/CacheDefs.scala | 111 ++++++++++++++++++ .../src/scala/annotation/meta/cached.scala | 4 + 4 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 compiler/src/dotty/tools/dotc/transform/CacheDefs.scala create mode 100644 library/src/scala/annotation/meta/cached.scala diff --git a/compiler/src/dotty/tools/dotc/Compiler.scala b/compiler/src/dotty/tools/dotc/Compiler.scala index 56e71c29110b..4abd32ecd508 100644 --- a/compiler/src/dotty/tools/dotc/Compiler.scala +++ b/compiler/src/dotty/tools/dotc/Compiler.scala @@ -63,7 +63,7 @@ class Compiler { new ExpandSAMs, // Expand single abstract method closures to anonymous classes new ProtectedAccessors, // Add accessors for protected members new ExtensionMethods, // Expand methods of value classes with extension methods - new CacheAliasImplicits, // Cache RHS of parameterless alias implicits + new CacheDefs , // Cache RHS of parameterless alias implicits and @cached methods new ShortcutImplicits, // Allow implicit functions without creating closures new ByNameClosures, // Expand arguments to by-name parameters to closures new HoistSuperArgs, // Hoist complex arguments of supercalls to enclosing scope diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 6ef8a232318b..4f103f277ab5 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -787,6 +787,7 @@ class Definitions { @threadUnsafe lazy val FunctionalInterfaceAnnot: ClassSymbolPerRun = perRunClass(ctx.requiredClassRef("java.lang.FunctionalInterface")) @threadUnsafe lazy val InfixAnnot: ClassSymbolPerRun = perRunClass(ctx.requiredClassRef("scala.annotation.infix")) @threadUnsafe lazy val AlphaAnnot: ClassSymbolPerRun = perRunClass(ctx.requiredClassRef("scala.annotation.alpha")) + @threadUnsafe lazy val CachedAnnot: ClassSymbol = ctx.requiredClass("scala.annotation.meta.cached") // convenient one-parameter method types def methOfAny(tp: Type): MethodType = MethodType(List(AnyType), tp) diff --git a/compiler/src/dotty/tools/dotc/transform/CacheDefs.scala b/compiler/src/dotty/tools/dotc/transform/CacheDefs.scala new file mode 100644 index 000000000000..622596b1baa5 --- /dev/null +++ b/compiler/src/dotty/tools/dotc/transform/CacheDefs.scala @@ -0,0 +1,111 @@ +package dotty.tools.dotc +package transform + +import MegaPhase._ +import core._ +import DenotTransformers.{IdentityDenotTransformer} +import Symbols._, Contexts._, Types._, Flags._, NameOps._, Decorators._ +import StdNames.nme +import NameKinds.CacheName +import Constants.Constant +import ast.tpd +import collection.mutable + +object CacheDefs { + val name: String = "cacheDefs" + + /** Flags that disable caching */ + val NoCacheFlags = + StableRealizable | // It's a simple forwarder, leave it as one + Exported // Export forwarders are never cached +} + +/** This phase ensures that the right hand side of parameterless alias implicits + * 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 + * + * is expanded before this phase + * + * 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 + * + * 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 + * + * lazy implicit val a: TC = rhs + */ +class CacheDefs extends MiniPhase with IdentityDenotTransformer { thisPhase => + import tpd._ + + override def phaseName: String = CacheDefs.name + + override def transformDefDef(tree: DefDef) given (ctx: Context): Tree = { + val sym = tree.symbol.asTerm + val isCached = sym.hasAnnotation(defn.CachedAnnot) || { + sym.info match { + case _: ExprType 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) { + val cachedType = sym.info.widenExpr match { + case mt: MethodType => mt.finalResultType + case cachedType => cachedType + } + val cacheVar = ctx.newSymbol( + owner = sym.owner, + name = CacheName(sym.name), + flags = if (sym.owner.isClass) Private | Local | Mutable else Mutable, + info = OrType(cachedType, defn.NullType), + coord = tree.rhs.span + ).enteredAfter(thisPhase) + val cacheSetter = + if (sym.owner.is(Trait)) + ctx.newSymbol( + owner = sym.owner, + name = cacheVar.name.setterName, + flags = cacheVar.flags | Method | Accessor, + info = MethodType(cachedType :: Nil, defn.UnitType), + coord = tree.rhs.span + ).enteredAfter(thisPhase) + else cacheVar + val cachedDefs = new mutable.ListBuffer[Tree]() + cachedDefs += ValDef(cacheVar, nullLiteral) + if (cacheSetter ne cacheVar) + cachedDefs += DefDef(cacheSetter, _ => Literal(Constant(()))) + cachedDefs += cpy.DefDef(tree)(rhs = + Block( + If( + ref(cacheVar).select(defn.Any_==).appliedTo(nullLiteral), + ref(cacheSetter).becomes(tree.rhs), + unitLiteral) :: Nil, + ref(cacheVar).cast(cachedType) + ) + ) + Thicket(cachedDefs.toList) + } + else tree + } +} + + + diff --git a/library/src/scala/annotation/meta/cached.scala b/library/src/scala/annotation/meta/cached.scala new file mode 100644 index 000000000000..e13469e66886 --- /dev/null +++ b/library/src/scala/annotation/meta/cached.scala @@ -0,0 +1,4 @@ +package scala.annotation +package meta + +class cached extends Annotation From ad76cac3ec8bb67a1a5ca466f1e283e3daa0adc2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 30 Jul 2019 16:16:41 +0200 Subject: [PATCH 3/8] Make entered and enteredAfter also work for term members Restricting `entered` and `enteredAfter` to class members is more a trap to fall into than a helpful check. --- .../src/dotty/tools/dotc/core/Symbols.scala | 25 +++++++++++-------- .../dotc/transform/CacheAliasImplicits.scala | 2 +- .../tools/dotc/transform/HoistSuperArgs.scala | 6 ++--- .../tools/dotc/transform/LambdaLift.scala | 5 ++-- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Symbols.scala b/compiler/src/dotty/tools/dotc/core/Symbols.scala index 24d2c188a419..3aaf8cb80838 100644 --- a/compiler/src/dotty/tools/dotc/core/Symbols.scala +++ b/compiler/src/dotty/tools/dotc/core/Symbols.scala @@ -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 } @@ -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 */ diff --git a/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala b/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala index 29eae5107ec7..9cf5fd216388 100644 --- a/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala +++ b/compiler/src/dotty/tools/dotc/transform/CacheAliasImplicits.scala @@ -82,7 +82,7 @@ class CacheAliasImplicits extends MiniPhase with IdentityDenotTransformer { this 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) + .enteredAfter(thisPhase) val cacheDef = ValDef(cacheSym, tpd.defaultValue(rhsType)) val cachingDef = cpy.DefDef(tree)(rhs = Block( diff --git a/compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala b/compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala index 302d2330d056..d72f0895ca49 100644 --- a/compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala +++ b/compiler/src/dotty/tools/dotc/transform/HoistSuperArgs.scala @@ -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 */ diff --git a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala index 4bd73238797b..548f594865b1 100644 --- a/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala +++ b/compiler/src/dotty/tools/dotc/transform/LambdaLift.scala @@ -298,8 +298,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 From 181fccecf206413f2af974feb3195e57aeda0e10 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 30 Jul 2019 16:29:21 +0200 Subject: [PATCH 4/8] Streamline Typer#traverse --- compiler/src/dotty/tools/dotc/typer/Typer.scala | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 0c0f4e95dcb7..fe76f388b14b 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -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) } From 3fdaac1122353f0dc40ad152b0226cf1d78ac7c2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 28 Jul 2019 15:48:07 +0200 Subject: [PATCH 5/8] Add test --- tests/pos/i6909.scala | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/pos/i6909.scala diff --git a/tests/pos/i6909.scala b/tests/pos/i6909.scala new file mode 100644 index 000000000000..dd1f8e91d8da --- /dev/null +++ b/tests/pos/i6909.scala @@ -0,0 +1,14 @@ +import scala.compiletime.memo +trait Foo[A] + + +trait Qux { + private[this] var x: Int | Null = null + def f = { + if (x == null) x = 22 + x.asInstanceOf[Int] + } + def g = memo(new Foo[Int] {}) +// + //given as Foo[Int] = new Foo[Int] {} +} \ No newline at end of file From d03e69e3b6e21a8b1beba5a5effd3db537ee4365 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 26 Jul 2019 11:35:18 +0200 Subject: [PATCH 6/8] Update docs The new implementation does not optimize given aliases with pure paths as right hand sides to be vals anymore. The reason is that even a pure path might be expensive to compute so we might want the caching. And caching is cheap, so there's little downside. Therefore, it's attractive to go with the simpler two way choice: leave RHS as is, or wrap it in a memo. --- .../reference/contextual/relationship-implicits.md | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/docs/docs/reference/contextual/relationship-implicits.md b/docs/docs/reference/contextual/relationship-implicits.md index 95b1f160ed0f..956a5865cd0c 100644 --- a/docs/docs/reference/contextual/relationship-implicits.md +++ b/docs/docs/reference/contextual/relationship-implicits.md @@ -28,18 +28,12 @@ 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. If an alias has neither type parameters nor a given clause, its right-hand side is cached in a variable, 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 @@ -52,8 +46,6 @@ Given instances can be mapped to combinations of implicit objects, classes and i global$cache } - final implicit val config: Config = default.config - final implicit def Context_given = ctx ``` From e5fe3cf883c7db6b9c9fd44a02d9db145c8441a8 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 30 Jul 2019 16:35:34 +0200 Subject: [PATCH 7/8] Fix test --- tests/pos/i6909.scala | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/tests/pos/i6909.scala b/tests/pos/i6909.scala index dd1f8e91d8da..7bbfbe50d1f3 100644 --- a/tests/pos/i6909.scala +++ b/tests/pos/i6909.scala @@ -1,14 +1,7 @@ -import scala.compiletime.memo +import scala.compiletime trait Foo[A] trait Qux { - private[this] var x: Int | Null = null - def f = { - if (x == null) x = 22 - x.asInstanceOf[Int] - } - def g = memo(new Foo[Int] {}) -// - //given as Foo[Int] = new Foo[Int] {} + given as Foo[Int] = new Foo[Int] {} } \ No newline at end of file From 8789f97ce5b9a0c85af300ed1ef2c68e78d51e9b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 30 Jul 2019 17:07:58 +0200 Subject: [PATCH 8/8] Check @cached annotation for applicability and add pos and neg tests --- .../src/dotty/tools/dotc/typer/Checking.scala | 15 ++++++- tests/neg/cached.scala | 11 +++++ tests/run/cachedTest.checked | 7 +++ tests/run/cachedTest.scala | 43 +++++++++++++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 tests/neg/cached.scala create mode 100644 tests/run/cachedTest.checked create mode 100644 tests/run/cachedTest.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 0b0e06c6a652..7c95cbf03c5f 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -1147,7 +1147,20 @@ trait Checking { if (!sym.owner.is(Module) || !sym.owner.isStatic) ctx.error(em"$sym cannot be a @main method since it cannot be accessed statically", pos) } - // TODO: Add more checks here + else if (annotCls == defn.CachedAnnot) { + if (!sym.isRealMethod) + ctx.error(em"@cached annotation cannot be applied to $sym", pos) + def checkCachedInfo(tp: Type): Unit = tp match { + case _: PolyType => + ctx.error(em"@cached method cannot have type parameters", pos) + case mt: MethodType => + if (!mt.isImplicitMethod) + ctx.error(em"@cached method cannot have regular parameters", pos) + checkCachedInfo(mt.resultType) + case _ => + } + checkCachedInfo(sym.info) + } } /** Check that symbol's external name does not clash with symbols defined in the same scope */ diff --git a/tests/neg/cached.scala b/tests/neg/cached.scala new file mode 100644 index 000000000000..b33d320e917f --- /dev/null +++ b/tests/neg/cached.scala @@ -0,0 +1,11 @@ +import annotation.meta.cached +object Test { + + @cached val x: Int = 1 // error: @cached annotation cannot be applied + @cached type T = Int // error: @cached annotation cannot be applied + @cached class Foo // error: @cached annotation cannot be applied + @cached object Bar // error: @cached annotation cannot be applied + + @cached def foo(x: Int) = x // error: @cached method cannot have regular parameters + @cached def foo[T]: T = ??? // error: @cached method cannot have type parameters +} \ No newline at end of file diff --git a/tests/run/cachedTest.checked b/tests/run/cachedTest.checked new file mode 100644 index 000000000000..bcc6c67fe293 --- /dev/null +++ b/tests/run/cachedTest.checked @@ -0,0 +1,7 @@ +computing inner +computing f +1 +1 +computing f +1 +1 diff --git a/tests/run/cachedTest.scala b/tests/run/cachedTest.scala new file mode 100644 index 000000000000..cb82747e4cef --- /dev/null +++ b/tests/run/cachedTest.scala @@ -0,0 +1,43 @@ +object Test extends App { + import annotation.meta.cached + + @cached def bar given(x: Int) = x * x + 1 + + assert((bar given 1) + (bar given 2) == 4) + + trait T { + def x: Int + @cached def y: Int = { + @cached def inner = { + println("computing inner"); + x * x + } + inner + inner + } + } + val t = new T { + def x = 3 + assert(y == 18) + } + assert(t.y == 18) + + class Context(val n: Int) + def f(c: Context): Context = { + println("computing f") + Context(c.n + 1) + } + given as Context(0) + + locally { + @cached given as Context given (c: Context) = f(c) + println(the[Context].n) + println(the[Context].n) + } + + val ctx = f(the[Context]) + locally { + given as Context = ctx + println(the[Context].n) + println(the[Context].n) + } +} \ No newline at end of file