From 6e52484698f2a84915981c6297019825111f00d3 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 15 Jan 2018 10:28:31 +0100 Subject: [PATCH 1/4] Fix #2939: Avoid duplicating symbols in default arguments Previously a method argument could be duplicated because it was passed to the method as well as to its default argument methods. This was fatal if the argument contained local definitions or was a closure. We now unconditionally lift such arguments if the method has default parameters. --- .../src/dotty/tools/dotc/ast/TreeInfo.scala | 35 +++--- .../tools/dotc/transform/PhantomArgLift.scala | 4 +- .../dotty/tools/dotc/typer/Applications.scala | 15 ++- .../dotty/tools/dotc/typer/EtaExpansion.scala | 111 ++++++++++-------- tests/run/i2939.scala | 33 ++++++ 5 files changed, 124 insertions(+), 74 deletions(-) create mode 100644 tests/run/i2939.scala diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index f9d76d083cbd..2c41dfbc90de 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -311,9 +311,9 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => import tpd._ /** The purity level of this statement. - * @return pure if statement has no side effects - * idempotent if running the statement a second time has no side effects - * impure otherwise + * @return Pure if statement has no side effects + * Idempotent if running the statement a second time has no side effects + * Impure otherwise */ private def statPurity(tree: Tree)(implicit ctx: Context): PurityLevel = unsplice(tree) match { case EmptyTree @@ -322,7 +322,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => | DefDef(_, _, _, _, _) => Pure case vdef @ ValDef(_, _, _) => - if (vdef.symbol.flags is Mutable) Impure else exprPurity(vdef.rhs) + if (vdef.symbol.flags is Mutable) Impure else exprPurity(vdef.rhs) `min` Pure case _ => Impure // TODO: It seem like this should be exprPurity(tree) @@ -330,9 +330,10 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => } /** The purity level of this expression. - * @return pure if expression has no side effects - * idempotent if running the expression a second time has no side effects - * impure otherwise + * @return PurePath if expression has no side effects and cannot contain local definitions + * Pure if expression has no side effects + * Idempotent if running the expression a second time has no side effects + * Impure otherwise * * Note that purity and idempotency are different. References to modules and lazy * vals are impure (side-effecting) both because side-effecting code may be executed and because the first reference @@ -345,7 +346,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => | Super(_, _) | Literal(_) | Closure(_, _, _) => - Pure + PurePath case Ident(_) => refPurity(tree) case Select(qual, _) => @@ -366,7 +367,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => if (args.isEmpty && fn.symbol.is(Stable)) exprPurity(fn) else if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol)) // A constant expression with pure arguments is pure. - minOf(exprPurity(fn), args.map(exprPurity)) + minOf(exprPurity(fn), args.map(exprPurity)) `min` Pure else Impure case Typed(expr, _) => exprPurity(expr) @@ -382,25 +383,26 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => private def minOf(l0: PurityLevel, ls: List[PurityLevel]) = (l0 /: ls)(_ min _) - def isPureExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == Pure + def isPurePath(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == PurePath + def isPureExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Pure def isIdempotentExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Idempotent /** The purity level of this reference. * @return - * pure if reference is (nonlazy and stable) or to a parameterized function - * idempotent if reference is lazy and stable - * impure otherwise + * PurePath if reference is (nonlazy and stable) or to a parameterized function + * Idempotent if reference is lazy and stable + * Impure otherwise * @DarkDimius: need to make sure that lazy accessor methods have Lazy and Stable * flags set. */ private def refPurity(tree: Tree)(implicit ctx: Context): PurityLevel = - if (!tree.tpe.widen.isParameterless) Pure + if (!tree.tpe.widen.isParameterless) PurePath else if (!tree.symbol.isStable) Impure else if (tree.symbol.is(Lazy)) Idempotent // TODO add Module flag, sinxce Module vals or not Lazy from the start. - else Pure + else PurePath def isPureRef(tree: Tree)(implicit ctx: Context) = - refPurity(tree) == Pure + refPurity(tree) == PurePath def isIdempotentRef(tree: Tree)(implicit ctx: Context) = refPurity(tree) >= Idempotent @@ -724,6 +726,7 @@ object TreeInfo { def min(that: PurityLevel) = new PurityLevel(x min that.x) } + val PurePath = new PurityLevel(3) val Pure = new PurityLevel(2) val Idempotent = new PurityLevel(1) val Impure = new PurityLevel(0) diff --git a/compiler/src/dotty/tools/dotc/transform/PhantomArgLift.scala b/compiler/src/dotty/tools/dotc/transform/PhantomArgLift.scala index 7df418e598d6..3c4b439f7060 100644 --- a/compiler/src/dotty/tools/dotc/transform/PhantomArgLift.scala +++ b/compiler/src/dotty/tools/dotc/transform/PhantomArgLift.scala @@ -5,7 +5,7 @@ import dotty.tools.dotc.core.Contexts._ import dotty.tools.dotc.core.NameKinds._ import dotty.tools.dotc.core.Types._ import dotty.tools.dotc.transform.MegaPhase.MiniPhase -import dotty.tools.dotc.typer.EtaExpansion +import dotty.tools.dotc.typer.LiftImpure import scala.collection.mutable.ListBuffer @@ -46,7 +46,7 @@ class PhantomArgLift extends MiniPhase { if (!hasImpurePhantomArgs(tree)) tree else { val buffer = ListBuffer.empty[Tree] - val app = EtaExpansion.liftApp(buffer, tree) + val app = LiftImpure.liftApp(buffer, tree) if (buffer.isEmpty) app else Block(buffer.result(), app) } diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index a6cdd7b8c9fd..0b5c3f5cb3c0 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -22,7 +22,6 @@ import Names._ import StdNames._ import NameKinds.DefaultGetterName import ProtoTypes._ -import EtaExpansion._ import Inferencing._ import collection.mutable @@ -426,7 +425,8 @@ trait Applications extends Compatibility { self: Typer with Dynamic => } def tryDefault(n: Int, args1: List[Arg]): Unit = { - if (!isJavaAnnotConstr(methRef.symbol)) liftFun() + if (!isJavaAnnotConstr(methRef.symbol)) + liftFun() val getter = findDefaultGetter(n + numArgs(normalizedFun)) if (getter.isEmpty) missingArg(n) else { @@ -571,10 +571,13 @@ trait Applications extends Compatibility { self: Typer with Dynamic => def normalizedFun = myNormalizedFun + private def lifter(implicit ctx: Context) = + if (methRef.symbol.hasDefaultParams) LiftComplex else LiftImpure + override def liftFun(): Unit = if (liftedDefs == null) { liftedDefs = new mutable.ListBuffer[Tree] - myNormalizedFun = liftApp(liftedDefs, myNormalizedFun) + myNormalizedFun = lifter.liftApp(liftedDefs, myNormalizedFun) } /** The index of the first difference between lists of trees `xs` and `ys` @@ -608,13 +611,13 @@ trait Applications extends Compatibility { self: Typer with Dynamic => // lift arguments in the definition order val argDefBuf = mutable.ListBuffer.empty[Tree] - typedArgs = liftArgs(argDefBuf, methType, typedArgs) + typedArgs = lifter.liftArgs(argDefBuf, methType, typedArgs) // Lifted arguments ordered based on the original order of typedArgBuf and // with all non-explicit default parameters at the end in declaration order. val orderedArgDefs = { // List of original arguments that are lifted by liftArgs - val impureArgs = typedArgBuf.filterNot(isPureExpr) + val impureArgs = typedArgBuf.filterNot(lifter.noLift) // Assuming stable sorting all non-explicit default parameters will remain in the end with the same order val defaultParamIndex = args.size // Mapping of index of each `liftable` into original args ordering @@ -746,7 +749,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic => val Apply(Select(lhs, name), rhss) = tree val lhs1 = typedExpr(lhs) val liftedDefs = new mutable.ListBuffer[Tree] - val lhs2 = untpd.TypedSplice(liftAssigned(liftedDefs, lhs1)) + val lhs2 = untpd.TypedSplice(LiftComplex.liftAssigned(liftedDefs, lhs1)) val assign = untpd.Assign(lhs2, untpd.Apply(untpd.Select(lhs2, name.asSimpleName.dropRight(1)), rhss)) wrapDefs(liftedDefs, typed(assign)) diff --git a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala index d198341884d0..56c10d614b96 100644 --- a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala +++ b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala @@ -13,23 +13,43 @@ import Decorators._ import Names._ import StdNames._ import NameKinds.UniqueName -import Trees._ import Inferencing._ import util.Positions._ import collection.mutable +import Trees._ -object EtaExpansion { - +/** A class that handles argument lifting. Argument lifting is needed in the following + * scenarios: + * - eta expansion + * - applications with default arguments + * - applications with out-of-order named arguments + * Lifting generally lifts impure expressions only, except in the case of possible + * default arguments, where we lift also complex pure expressions, since in that case + * arguments can be duplicated as arguments to default argument methods. + */ +abstract class Lifter { import tpd._ + /** Test indicating `expr` does not need lifting */ + def noLift(expr: Tree)(implicit ctx: Context): Boolean + + /** The corresponding lifter for paam-by-name arguments */ + protected def exprLifter: Lifter = NoLift + + /** The flags of a lifted definition */ + protected def liftedFlags: FlagSet = EmptyFlags + + /** The tree of a lifted definition */ + protected def liftedDef(sym: TermSymbol, rhs: Tree)(implicit ctx: Context): MemberDef = ValDef(sym, rhs) + private def lift(defs: mutable.ListBuffer[Tree], expr: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree = - if (isPureExpr(expr)) expr + if (noLift(expr)) expr else { val name = UniqueName.fresh(prefix) val liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos) - val sym = ctx.newSymbol(ctx.owner, name, EmptyFlags, liftedType, coord = positionCoord(expr.pos)) - defs += ValDef(sym, expr).withPos(expr.pos.focus) - ref(sym.termRef).withPos(expr.pos) + val lifted = ctx.newSymbol(ctx.owner, name, liftedFlags, liftedType, coord = positionCoord(expr.pos)) + defs += liftedDef(lifted, expr).withPos(expr.pos.focus) + ref(lifted.termRef).withPos(expr.pos) } /** Lift out common part of lhs tree taking part in an operator assignment such as @@ -49,7 +69,7 @@ object EtaExpansion { } /** Lift a function argument, stripping any NamedArg wrapper */ - def liftArg(defs: mutable.ListBuffer[Tree], arg: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree = + private def liftArg(defs: mutable.ListBuffer[Tree], arg: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree = arg match { case arg @ NamedArg(name, arg1) => cpy.NamedArg(arg)(name, lift(defs, arg1, prefix)) case arg => lift(defs, arg, prefix) @@ -61,12 +81,12 @@ object EtaExpansion { def liftArgs(defs: mutable.ListBuffer[Tree], methRef: Type, args: List[Tree])(implicit ctx: Context) = methRef.widen match { case mt: MethodType => - (args, mt.paramNames, mt.paramInfos).zipped map { (arg, name, tp) => - if (tp.isInstanceOf[ExprType]) arg - else liftArg(defs, arg, if (name.firstPart contains '$') EmptyTermName else name) + (args, mt.paramNames, mt.paramInfos).zipped.map { (arg, name, tp) => + val lifter = if (tp.isInstanceOf[ExprType]) exprLifter else this + lifter.liftArg(defs, arg, if (name.firstPart contains '$') EmptyTermName else name) } case _ => - args map (liftArg(defs, _)) + args.map(liftArg(defs, _)) } /** Lift out function prefix and all arguments from application @@ -108,6 +128,35 @@ object EtaExpansion { case New(_) => tree case _ => if (isIdempotentExpr(tree)) tree else lift(defs, tree) } +} + +/** No lifting at all */ +object NoLift extends Lifter { + def noLift(expr: tpd.Tree)(implicit ctx: Context) = true +} + +/** Lift all impure arguments */ +class LiftImpure extends Lifter { + def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isPureExpr(expr) +} +object LiftImpure extends LiftImpure + +/** Lift all impure or complex arguments */ +class LiftComplex extends Lifter { + def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isPurePath(expr) + override def exprLifter = LiftToDefs +} +object LiftComplex extends LiftComplex + +/** Lift all impure or complex arguments to `def`s */ +object LiftToDefs extends LiftComplex { + override def liftedFlags: FlagSet = Method + override def liftedDef(sym: TermSymbol, rhs: tpd.Tree)(implicit ctx: Context) = tpd.DefDef(sym, rhs) +} + +/** Lifter for eta expansion */ +object EtaExpansion extends LiftImpure { + import tpd._ /** Eta-expanding a tree means converting a method reference to a function value. * @param tree The tree to expand @@ -152,41 +201,3 @@ object EtaExpansion { if (defs.nonEmpty) untpd.Block(defs.toList map (untpd.TypedSplice(_)), fn) else fn } } - - /**

not needed - * Expand partial function applications of type `type`. - * 

-   *  p.f(es_1)...(es_n)
-   *     ==>  {
-   *            private synthetic val eta$f   = p.f   // if p is not stable
-   *            ...
-   *            private synthetic val eta$e_i = e_i    // if e_i is not stable
-   *            ...
-   *            (ps_1 => ... => ps_m => eta$f([es_1])...([es_m])(ps_1)...(ps_m))
-   *          }
- *

- * tree is already attributed - *

- def etaExpandUntyped(tree: Tree)(implicit ctx: Context): untpd.Tree = { // kept as a reserve for now - def expand(tree: Tree): untpd.Tree = tree.tpe match { - case mt @ MethodType(paramNames, paramTypes) if !mt.isImplicit => - val paramsArgs: List[(untpd.ValDef, untpd.Tree)] = - (paramNames, paramTypes).zipped.map { (name, tp) => - val droppedStarTpe = defn.underlyingOfRepeated(tp) - val param = ValDef( - Modifiers(Param), name, - untpd.TypedSplice(TypeTree(droppedStarTpe)), untpd.EmptyTree) - var arg: untpd.Tree = Ident(name) - if (defn.isRepeatedParam(tp)) - arg = Typed(arg, Ident(tpnme.WILDCARD_STAR)) - (param, arg) - } - val (params, args) = paramsArgs.unzip - untpd.Function(params, Apply(untpd.TypedSplice(tree), args)) - } - - val defs = new mutable.ListBuffer[Tree] - val tree1 = liftApp(defs, tree) - Block(defs.toList map untpd.TypedSplice, expand(tree1)) - } - */ diff --git a/tests/run/i2939.scala b/tests/run/i2939.scala new file mode 100644 index 000000000000..36a5a809e246 --- /dev/null +++ b/tests/run/i2939.scala @@ -0,0 +1,33 @@ +import scala.collection.mutable._ + +class Tag(val name: String, val buffer: Buffer[Tag] = ArrayBuffer()) { + def space(n: Int = 0): String = { + s"${" " * n}<$name>\n" + + (if(buffer.isEmpty) "" else buffer.map(_.space(n + 4)).mkString("", "\n", "\n")) + + s"${" " * n}" + } + + def apply[U](f: implicit Tag => U)(implicit tag: Tag = null): this.type = { + f(this) + if(tag != null) tag.buffer += this + this + } + + override def toString(): String = space(0) +} + +object Tag { + implicit def toTag(symbol: Symbol): Tag = new Tag(symbol.name) + + def main(args: Array[String]): Unit = { + } +} + + +object Test { + def foo(x: Int => Int)(y: Int = 0) = {} + + def main(args: Array[String]): Unit = { + foo(x => x)() + } +} From a7ddb3b6739aaaa06b6cf8794910b4a6608399cd Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 15 Jan 2018 22:02:55 +0100 Subject: [PATCH 2/4] Test lifting of call-by-name args --- tests/run/i2939.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/run/i2939.scala b/tests/run/i2939.scala index 36a5a809e246..a9fa9ee22962 100644 --- a/tests/run/i2939.scala +++ b/tests/run/i2939.scala @@ -26,8 +26,10 @@ object Tag { object Test { def foo(x: Int => Int)(y: Int = 0) = {} + def bar(x: => Int)(y: Int = 0) = {} def main(args: Array[String]): Unit = { foo(x => x)() + bar(args.length)() } } From 52d4a14c4cf2544151429dadc0f3f8dd8ab25d2e Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 21 Jan 2018 19:07:34 +0100 Subject: [PATCH 3/4] Fix types of lifted DefDefs. --- compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala index 56c10d614b96..9b0e8975f17e 100644 --- a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala +++ b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala @@ -46,7 +46,8 @@ abstract class Lifter { if (noLift(expr)) expr else { val name = UniqueName.fresh(prefix) - val liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos) + var liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos) + if (liftedFlags.is(Method)) liftedType = ExprType(liftedType) val lifted = ctx.newSymbol(ctx.owner, name, liftedFlags, liftedType, coord = positionCoord(expr.pos)) defs += liftedDef(lifted, expr).withPos(expr.pos.focus) ref(lifted.termRef).withPos(expr.pos) From b1908a0245290ad7646435a3a20e17d35ac4ba3b Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 2 Feb 2018 22:07:34 +0100 Subject: [PATCH 4/4] Address reviewers comments --- compiler/src/dotty/tools/dotc/ast/TreeInfo.scala | 16 ++++++++-------- .../dotty/tools/dotc/typer/EtaExpansion.scala | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index 2c41dfbc90de..8ff90e65462c 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -330,7 +330,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => } /** The purity level of this expression. - * @return PurePath if expression has no side effects and cannot contain local definitions + * @return SimplyPure if expression has no side effects and cannot contain local definitions * Pure if expression has no side effects * Idempotent if running the expression a second time has no side effects * Impure otherwise @@ -346,7 +346,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => | Super(_, _) | Literal(_) | Closure(_, _, _) => - PurePath + SimplyPure case Ident(_) => refPurity(tree) case Select(qual, _) => @@ -383,26 +383,26 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => private def minOf(l0: PurityLevel, ls: List[PurityLevel]) = (l0 /: ls)(_ min _) - def isPurePath(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == PurePath + def isSimplyPure(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == SimplyPure def isPureExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Pure def isIdempotentExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Idempotent /** The purity level of this reference. * @return - * PurePath if reference is (nonlazy and stable) or to a parameterized function + * SimplyPure if reference is (nonlazy and stable) or to a parameterized function * Idempotent if reference is lazy and stable * Impure otherwise * @DarkDimius: need to make sure that lazy accessor methods have Lazy and Stable * flags set. */ private def refPurity(tree: Tree)(implicit ctx: Context): PurityLevel = - if (!tree.tpe.widen.isParameterless) PurePath + if (!tree.tpe.widen.isParameterless) SimplyPure else if (!tree.symbol.isStable) Impure else if (tree.symbol.is(Lazy)) Idempotent // TODO add Module flag, sinxce Module vals or not Lazy from the start. - else PurePath + else SimplyPure def isPureRef(tree: Tree)(implicit ctx: Context) = - refPurity(tree) == PurePath + refPurity(tree) == SimplyPure def isIdempotentRef(tree: Tree)(implicit ctx: Context) = refPurity(tree) >= Idempotent @@ -726,7 +726,7 @@ object TreeInfo { def min(that: PurityLevel) = new PurityLevel(x min that.x) } - val PurePath = new PurityLevel(3) + val SimplyPure = new PurityLevel(3) val Pure = new PurityLevel(2) val Idempotent = new PurityLevel(1) val Impure = new PurityLevel(0) diff --git a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala index 9b0e8975f17e..be84427a83a0 100644 --- a/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala +++ b/compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala @@ -33,7 +33,7 @@ abstract class Lifter { /** Test indicating `expr` does not need lifting */ def noLift(expr: Tree)(implicit ctx: Context): Boolean - /** The corresponding lifter for paam-by-name arguments */ + /** The corresponding lifter for pass-by-name arguments */ protected def exprLifter: Lifter = NoLift /** The flags of a lifted definition */ @@ -144,7 +144,7 @@ object LiftImpure extends LiftImpure /** Lift all impure or complex arguments */ class LiftComplex extends Lifter { - def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isPurePath(expr) + def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isSimplyPure(expr) override def exprLifter = LiftToDefs } object LiftComplex extends LiftComplex