Skip to content

Commit 63ace94

Browse files
committed
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.
1 parent c2e5011 commit 63ace94

File tree

5 files changed

+124
-74
lines changed

5 files changed

+124
-74
lines changed

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+19-16
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,9 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
311311
import tpd._
312312

313313
/** The purity level of this statement.
314-
* @return pure if statement has no side effects
315-
* idempotent if running the statement a second time has no side effects
316-
* impure otherwise
314+
* @return Pure if statement has no side effects
315+
* Idempotent if running the statement a second time has no side effects
316+
* Impure otherwise
317317
*/
318318
private def statPurity(tree: Tree)(implicit ctx: Context): PurityLevel = unsplice(tree) match {
319319
case EmptyTree
@@ -322,17 +322,18 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
322322
| DefDef(_, _, _, _, _) =>
323323
Pure
324324
case vdef @ ValDef(_, _, _) =>
325-
if (vdef.symbol.flags is Mutable) Impure else exprPurity(vdef.rhs)
325+
if (vdef.symbol.flags is Mutable) Impure else exprPurity(vdef.rhs) `min` Pure
326326
case _ =>
327327
Impure
328328
// TODO: It seem like this should be exprPurity(tree)
329329
// But if we do that the repl/vars test break. Need to figure out why that's the case.
330330
}
331331

332332
/** The purity level of this expression.
333-
* @return pure if expression has no side effects
334-
* idempotent if running the expression a second time has no side effects
335-
* impure otherwise
333+
* @return PurePath if expression has no side effects and cannot contain local definitions
334+
* Pure if expression has no side effects
335+
* Idempotent if running the expression a second time has no side effects
336+
* Impure otherwise
336337
*
337338
* Note that purity and idempotency are different. References to modules and lazy
338339
* 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] =>
345346
| Super(_, _)
346347
| Literal(_)
347348
| Closure(_, _, _) =>
348-
Pure
349+
PurePath
349350
case Ident(_) =>
350351
refPurity(tree)
351352
case Select(qual, _) =>
@@ -366,7 +367,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
366367
if (args.isEmpty && fn.symbol.is(Stable)) exprPurity(fn)
367368
else if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol))
368369
// A constant expression with pure arguments is pure.
369-
minOf(exprPurity(fn), args.map(exprPurity))
370+
minOf(exprPurity(fn), args.map(exprPurity)) `min` Pure
370371
else Impure
371372
case Typed(expr, _) =>
372373
exprPurity(expr)
@@ -382,25 +383,26 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
382383

383384
private def minOf(l0: PurityLevel, ls: List[PurityLevel]) = (l0 /: ls)(_ min _)
384385

385-
def isPureExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == Pure
386+
def isPurePath(tree: Tree)(implicit ctx: Context) = exprPurity(tree) == PurePath
387+
def isPureExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Pure
386388
def isIdempotentExpr(tree: Tree)(implicit ctx: Context) = exprPurity(tree) >= Idempotent
387389

388390
/** The purity level of this reference.
389391
* @return
390-
* pure if reference is (nonlazy and stable) or to a parameterized function
391-
* idempotent if reference is lazy and stable
392-
* impure otherwise
392+
* PurePath if reference is (nonlazy and stable) or to a parameterized function
393+
* Idempotent if reference is lazy and stable
394+
* Impure otherwise
393395
* @DarkDimius: need to make sure that lazy accessor methods have Lazy and Stable
394396
* flags set.
395397
*/
396398
private def refPurity(tree: Tree)(implicit ctx: Context): PurityLevel =
397-
if (!tree.tpe.widen.isParameterless) Pure
399+
if (!tree.tpe.widen.isParameterless) PurePath
398400
else if (!tree.symbol.isStable) Impure
399401
else if (tree.symbol.is(Lazy)) Idempotent // TODO add Module flag, sinxce Module vals or not Lazy from the start.
400-
else Pure
402+
else PurePath
401403

402404
def isPureRef(tree: Tree)(implicit ctx: Context) =
403-
refPurity(tree) == Pure
405+
refPurity(tree) == PurePath
404406
def isIdempotentRef(tree: Tree)(implicit ctx: Context) =
405407
refPurity(tree) >= Idempotent
406408

@@ -715,6 +717,7 @@ object TreeInfo {
715717
def min(that: PurityLevel) = new PurityLevel(x min that.x)
716718
}
717719

720+
val PurePath = new PurityLevel(3)
718721
val Pure = new PurityLevel(2)
719722
val Idempotent = new PurityLevel(1)
720723
val Impure = new PurityLevel(0)

compiler/src/dotty/tools/dotc/transform/PhantomArgLift.scala

+2-2
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import dotty.tools.dotc.core.Contexts._
55
import dotty.tools.dotc.core.NameKinds._
66
import dotty.tools.dotc.core.Types._
77
import dotty.tools.dotc.transform.MegaPhase.MiniPhase
8-
import dotty.tools.dotc.typer.EtaExpansion
8+
import dotty.tools.dotc.typer.LiftImpure
99

1010
import scala.collection.mutable.ListBuffer
1111

@@ -46,7 +46,7 @@ class PhantomArgLift extends MiniPhase {
4646
if (!hasImpurePhantomArgs(tree)) tree
4747
else {
4848
val buffer = ListBuffer.empty[Tree]
49-
val app = EtaExpansion.liftApp(buffer, tree)
49+
val app = LiftImpure.liftApp(buffer, tree)
5050
if (buffer.isEmpty) app
5151
else Block(buffer.result(), app)
5252
}

compiler/src/dotty/tools/dotc/typer/Applications.scala

+9-6
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import Names._
2222
import StdNames._
2323
import NameKinds.DefaultGetterName
2424
import ProtoTypes._
25-
import EtaExpansion._
2625
import Inferencing._
2726

2827
import collection.mutable
@@ -426,7 +425,8 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
426425
}
427426

428427
def tryDefault(n: Int, args1: List[Arg]): Unit = {
429-
if (!isJavaAnnotConstr(methRef.symbol)) liftFun()
428+
if (!isJavaAnnotConstr(methRef.symbol))
429+
liftFun()
430430
val getter = findDefaultGetter(n + numArgs(normalizedFun))
431431
if (getter.isEmpty) missingArg(n)
432432
else {
@@ -571,10 +571,13 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
571571

572572
def normalizedFun = myNormalizedFun
573573

574+
private def lifter(implicit ctx: Context) =
575+
if (methRef.symbol.hasDefaultParams) LiftComplex else LiftImpure
576+
574577
override def liftFun(): Unit =
575578
if (liftedDefs == null) {
576579
liftedDefs = new mutable.ListBuffer[Tree]
577-
myNormalizedFun = liftApp(liftedDefs, myNormalizedFun)
580+
myNormalizedFun = lifter.liftApp(liftedDefs, myNormalizedFun)
578581
}
579582

580583
/** 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 =>
608611

609612
// lift arguments in the definition order
610613
val argDefBuf = mutable.ListBuffer.empty[Tree]
611-
typedArgs = liftArgs(argDefBuf, methType, typedArgs)
614+
typedArgs = lifter.liftArgs(argDefBuf, methType, typedArgs)
612615

613616
// Lifted arguments ordered based on the original order of typedArgBuf and
614617
// with all non-explicit default parameters at the end in declaration order.
615618
val orderedArgDefs = {
616619
// List of original arguments that are lifted by liftArgs
617-
val impureArgs = typedArgBuf.filterNot(isPureExpr)
620+
val impureArgs = typedArgBuf.filterNot(lifter.noLift)
618621
// Assuming stable sorting all non-explicit default parameters will remain in the end with the same order
619622
val defaultParamIndex = args.size
620623
// Mapping of index of each `liftable` into original args ordering
@@ -746,7 +749,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
746749
val Apply(Select(lhs, name), rhss) = tree
747750
val lhs1 = typedExpr(lhs)
748751
val liftedDefs = new mutable.ListBuffer[Tree]
749-
val lhs2 = untpd.TypedSplice(liftAssigned(liftedDefs, lhs1))
752+
val lhs2 = untpd.TypedSplice(LiftComplex.liftAssigned(liftedDefs, lhs1))
750753
val assign = untpd.Assign(lhs2,
751754
untpd.Apply(untpd.Select(lhs2, name.asSimpleName.dropRight(1)), rhss))
752755
wrapDefs(liftedDefs, typed(assign))

compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala

+61-50
Original file line numberDiff line numberDiff line change
@@ -13,23 +13,43 @@ import Decorators._
1313
import Names._
1414
import StdNames._
1515
import NameKinds.UniqueName
16-
import Trees._
1716
import Inferencing._
1817
import util.Positions._
1918
import collection.mutable
19+
import Trees._
2020

21-
object EtaExpansion {
22-
21+
/** A class that handles argument lifting. Argument lifting is needed in the following
22+
* scenarios:
23+
* - eta expansion
24+
* - applications with default arguments
25+
* - applications with out-of-order named arguments
26+
* Lifting generally lifts impure expressions only, except in the case of possible
27+
* default arguments, where we lift also complex pure expressions, since in that case
28+
* arguments can be duplicated as arguments to default argument methods.
29+
*/
30+
abstract class Lifter {
2331
import tpd._
2432

33+
/** Test indicating `expr` does not need lifting */
34+
def noLift(expr: Tree)(implicit ctx: Context): Boolean
35+
36+
/** The corresponding lifter for paam-by-name arguments */
37+
protected def exprLifter: Lifter = NoLift
38+
39+
/** The flags of a lifted definition */
40+
protected def liftedFlags: FlagSet = EmptyFlags
41+
42+
/** The tree of a lifted definition */
43+
protected def liftedDef(sym: TermSymbol, rhs: Tree)(implicit ctx: Context): MemberDef = ValDef(sym, rhs)
44+
2545
private def lift(defs: mutable.ListBuffer[Tree], expr: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree =
26-
if (isPureExpr(expr)) expr
46+
if (noLift(expr)) expr
2747
else {
2848
val name = UniqueName.fresh(prefix)
2949
val liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos)
30-
val sym = ctx.newSymbol(ctx.owner, name, EmptyFlags, liftedType, coord = positionCoord(expr.pos))
31-
defs += ValDef(sym, expr).withPos(expr.pos.focus)
32-
ref(sym.termRef).withPos(expr.pos)
50+
val lifted = ctx.newSymbol(ctx.owner, name, liftedFlags, liftedType, coord = positionCoord(expr.pos))
51+
defs += liftedDef(lifted, expr).withPos(expr.pos.focus)
52+
ref(lifted.termRef).withPos(expr.pos)
3353
}
3454

3555
/** Lift out common part of lhs tree taking part in an operator assignment such as
@@ -49,7 +69,7 @@ object EtaExpansion {
4969
}
5070

5171
/** Lift a function argument, stripping any NamedArg wrapper */
52-
def liftArg(defs: mutable.ListBuffer[Tree], arg: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree =
72+
private def liftArg(defs: mutable.ListBuffer[Tree], arg: Tree, prefix: TermName = EmptyTermName)(implicit ctx: Context): Tree =
5373
arg match {
5474
case arg @ NamedArg(name, arg1) => cpy.NamedArg(arg)(name, lift(defs, arg1, prefix))
5575
case arg => lift(defs, arg, prefix)
@@ -61,12 +81,12 @@ object EtaExpansion {
6181
def liftArgs(defs: mutable.ListBuffer[Tree], methRef: Type, args: List[Tree])(implicit ctx: Context) =
6282
methRef.widen match {
6383
case mt: MethodType =>
64-
(args, mt.paramNames, mt.paramInfos).zipped map { (arg, name, tp) =>
65-
if (tp.isInstanceOf[ExprType]) arg
66-
else liftArg(defs, arg, if (name.firstPart contains '$') EmptyTermName else name)
84+
(args, mt.paramNames, mt.paramInfos).zipped.map { (arg, name, tp) =>
85+
val lifter = if (tp.isInstanceOf[ExprType]) exprLifter else this
86+
lifter.liftArg(defs, arg, if (name.firstPart contains '$') EmptyTermName else name)
6787
}
6888
case _ =>
69-
args map (liftArg(defs, _))
89+
args.map(liftArg(defs, _))
7090
}
7191

7292
/** Lift out function prefix and all arguments from application
@@ -108,6 +128,35 @@ object EtaExpansion {
108128
case New(_) => tree
109129
case _ => if (isIdempotentExpr(tree)) tree else lift(defs, tree)
110130
}
131+
}
132+
133+
/** No lifting at all */
134+
object NoLift extends Lifter {
135+
def noLift(expr: tpd.Tree)(implicit ctx: Context) = true
136+
}
137+
138+
/** Lift all impure arguments */
139+
class LiftImpure extends Lifter {
140+
def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isPureExpr(expr)
141+
}
142+
object LiftImpure extends LiftImpure
143+
144+
/** Lift all impure or complex arguments */
145+
class LiftComplex extends Lifter {
146+
def noLift(expr: tpd.Tree)(implicit ctx: Context) = tpd.isPurePath(expr)
147+
override def exprLifter = LiftToDefs
148+
}
149+
object LiftComplex extends LiftComplex
150+
151+
/** Lift all impure or complex arguments to `def`s */
152+
object LiftToDefs extends LiftComplex {
153+
override def liftedFlags: FlagSet = Method
154+
override def liftedDef(sym: TermSymbol, rhs: tpd.Tree)(implicit ctx: Context) = tpd.DefDef(sym, rhs)
155+
}
156+
157+
/** Lifter for eta expansion */
158+
object EtaExpansion extends LiftImpure {
159+
import tpd._
111160

112161
/** Eta-expanding a tree means converting a method reference to a function value.
113162
* @param tree The tree to expand
@@ -152,41 +201,3 @@ object EtaExpansion {
152201
if (defs.nonEmpty) untpd.Block(defs.toList map (untpd.TypedSplice(_)), fn) else fn
153202
}
154203
}
155-
156-
/** <p> not needed
157-
* Expand partial function applications of type `type`.
158-
* </p><pre>
159-
* p.f(es_1)...(es_n)
160-
* ==> {
161-
* <b>private synthetic val</b> eta$f = p.f // if p is not stable
162-
* ...
163-
* <b>private synthetic val</b> eta$e_i = e_i // if e_i is not stable
164-
* ...
165-
* (ps_1 => ... => ps_m => eta$f([es_1])...([es_m])(ps_1)...(ps_m))
166-
* }</pre>
167-
* <p>
168-
* tree is already attributed
169-
* </p>
170-
def etaExpandUntyped(tree: Tree)(implicit ctx: Context): untpd.Tree = { // kept as a reserve for now
171-
def expand(tree: Tree): untpd.Tree = tree.tpe match {
172-
case mt @ MethodType(paramNames, paramTypes) if !mt.isImplicit =>
173-
val paramsArgs: List[(untpd.ValDef, untpd.Tree)] =
174-
(paramNames, paramTypes).zipped.map { (name, tp) =>
175-
val droppedStarTpe = defn.underlyingOfRepeated(tp)
176-
val param = ValDef(
177-
Modifiers(Param), name,
178-
untpd.TypedSplice(TypeTree(droppedStarTpe)), untpd.EmptyTree)
179-
var arg: untpd.Tree = Ident(name)
180-
if (defn.isRepeatedParam(tp))
181-
arg = Typed(arg, Ident(tpnme.WILDCARD_STAR))
182-
(param, arg)
183-
}
184-
val (params, args) = paramsArgs.unzip
185-
untpd.Function(params, Apply(untpd.TypedSplice(tree), args))
186-
}
187-
188-
val defs = new mutable.ListBuffer[Tree]
189-
val tree1 = liftApp(defs, tree)
190-
Block(defs.toList map untpd.TypedSplice, expand(tree1))
191-
}
192-
*/

tests/run/i2939.scala

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import scala.collection.mutable._
2+
3+
class Tag(val name: String, val buffer: Buffer[Tag] = ArrayBuffer()) {
4+
def space(n: Int = 0): String = {
5+
s"${" " * n}<$name>\n" +
6+
(if(buffer.isEmpty) "" else buffer.map(_.space(n + 4)).mkString("", "\n", "\n")) +
7+
s"${" " * n}</$name>"
8+
}
9+
10+
def apply[U](f: implicit Tag => U)(implicit tag: Tag = null): this.type = {
11+
f(this)
12+
if(tag != null) tag.buffer += this
13+
this
14+
}
15+
16+
override def toString(): String = space(0)
17+
}
18+
19+
object Tag {
20+
implicit def toTag(symbol: Symbol): Tag = new Tag(symbol.name)
21+
22+
def main(args: Array[String]): Unit = {
23+
}
24+
}
25+
26+
27+
object Test {
28+
def foo(x: Int => Int)(y: Int = 0) = {}
29+
30+
def main(args: Array[String]): Unit = {
31+
foo(x => x)()
32+
}
33+
}

0 commit comments

Comments
 (0)