From 526c3072761ad1190b5cd33fcc919f8ecb238368 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Thu, 29 May 2025 04:28:02 -0700 Subject: [PATCH] Lambda-owned param ref in ctor incurs no field --- .../tools/dotc/transform/Constructors.scala | 88 ++++++++++--------- tests/neg/i22979.scala | 11 +++ tests/pos/i22979.scala | 18 ++++ tests/run/i22979/Leak.scala | 13 +++ tests/run/i22979/test.scala | 23 +++++ 5 files changed, 112 insertions(+), 41 deletions(-) create mode 100644 tests/neg/i22979.scala create mode 100644 tests/pos/i22979.scala create mode 100644 tests/run/i22979/Leak.scala create mode 100644 tests/run/i22979/test.scala diff --git a/compiler/src/dotty/tools/dotc/transform/Constructors.scala b/compiler/src/dotty/tools/dotc/transform/Constructors.scala index b373565489f0..7a631ade6e84 100644 --- a/compiler/src/dotty/tools/dotc/transform/Constructors.scala +++ b/compiler/src/dotty/tools/dotc/transform/Constructors.scala @@ -45,10 +45,10 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = // to more symbols being retained as parameters. Test case in run/capturing.scala. /** The private vals that are known to be retained as class fields */ - private val retainedPrivateVals = mutable.Set[Symbol]() + private val retainedPrivateVals = mutable.Set.empty[Symbol] /** The private vals whose definition comes before the current focus */ - private val seenPrivateVals = mutable.Set[Symbol]() + private val seenPrivateVals = mutable.Set.empty[Symbol] // Collect all private parameter accessors and value definitions that need // to be retained. There are several reasons why a parameter accessor or @@ -57,31 +57,37 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = // 2. It is accessed before it is defined // 3. It is accessed on an object other than `this` // 4. It is a mutable parameter accessor - // 5. It is has a wildcard initializer `_` - private def markUsedPrivateSymbols(tree: RefTree)(using Context): Unit = { + // 5. It has a wildcard initializer `_` + private def markUsedPrivateSymbols(tree: RefTree)(using Context): Unit = val sym = tree.symbol def retain() = retainedPrivateVals.add(sym) - if (sym.exists && sym.owner.isClass && mightBeDropped(sym)) { - val owner = sym.owner.asClass - - tree match { - case Ident(_) | Select(This(_), _) => - def inConstructor = { - val method = ctx.owner.enclosingMethod - method.isPrimaryConstructor && ctx.owner.enclosingClass == owner - } - if (inConstructor && - (sym.is(ParamAccessor) || seenPrivateVals.contains(sym))) { - // used inside constructor, accessed on this, - // could use constructor argument instead, no need to retain field - } - else retain() - case _ => retain() - } - } - } + if sym.exists && sym.owner.isClass && mightBeDropped(sym) then + tree match + case Ident(_) | Select(This(_), _) => + val method = ctx.owner.enclosingMethod + // template exprs are moved (below) to constructor, where lifted anonfun will take its captured env as an arg + inline def inAnonFunInCtor = + method.isAnonymousFunction + && ( + method.owner.isLocalDummy + || + method.owner.owner == sym.owner && !method.owner.isOneOf(MethodOrLazy) + ) + && !sym.owner.is(Module) // lambdalift doesn't transform correctly (to do) + val inConstructor = + (method.isPrimaryConstructor || inAnonFunInCtor) + && ctx.owner.enclosingClass == sym.owner + val noField = + inConstructor + && (sym.is(ParamAccessor) || seenPrivateVals.contains(sym)) + // used inside constructor, accessed on this, + // could use constructor argument instead, no need to retain field + if !noField then + retain() + case _ => + retain() override def transformIdent(tree: tpd.Ident)(using Context): tpd.Tree = { markUsedPrivateSymbols(tree) @@ -184,6 +190,7 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = transform(tree).changeOwnerAfter(prevOwner, constr.symbol, thisPhase) } + // mightBeDropped is trivially false for NoSymbol -> NoSymbol isRetained def isRetained(acc: Symbol) = !mightBeDropped(acc) || retainedPrivateVals(acc) @@ -209,30 +216,28 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = } } - val dropped = mutable.Set[Symbol]() + val dropped = mutable.Set.empty[Symbol] // Split class body into statements that go into constructor and // definitions that are kept as members of the class. - def splitStats(stats: List[Tree]): Unit = stats match { - case stat :: stats1 => + def splitStats(stats: List[Tree]): Unit = stats match + case stat :: stats => + val sym = stat.symbol stat match { - case stat @ ValDef(name, tpt, _) if !stat.symbol.is(Lazy) && !stat.symbol.hasAnnotation(defn.ScalaStaticAnnot) => - val sym = stat.symbol + case stat @ ValDef(name, tpt, _) if !sym.is(Lazy) && !sym.hasAnnotation(defn.ScalaStaticAnnot) => if (isRetained(sym)) { if (!stat.rhs.isEmpty && !isWildcardArg(stat.rhs)) constrStats += Assign(ref(sym), intoConstr(stat.rhs, sym)).withSpan(stat.span) clsStats += cpy.ValDef(stat)(rhs = EmptyTree) } - else if (!stat.rhs.isEmpty) { - dropped += sym - sym.copySymDenotation( - initFlags = sym.flags &~ Private, - owner = constr.symbol).installAfter(thisPhase) - constrStats += intoConstr(stat, sym) - } else + else dropped += sym + if !stat.rhs.isEmpty then + sym.copySymDenotation( + initFlags = sym.flags &~ Private, + owner = constr.symbol).installAfter(thisPhase) + constrStats += intoConstr(stat, sym) case stat @ DefDef(name, _, tpt, _) if stat.symbol.isGetter && !stat.symbol.is(Lazy) => - val sym = stat.symbol assert(isRetained(sym), sym) if sym.isConstExprFinalVal then if stat.rhs.isInstanceOf[Literal] then @@ -271,9 +276,9 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = case _ => constrStats += intoConstr(stat, tree.symbol) } - splitStats(stats1) + splitStats(stats) case Nil => - } + end splitStats /** Check that we do not have both a private field with name `x` and a private field * with name `FieldName(x)`. These will map to the same JVM name and therefore cause @@ -303,14 +308,15 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase = dropped += acc Nil } - else if (!isRetained(acc.field)) { // It may happen for unit fields, tests/run/i6987.scala + else if (acc.field.exists && !isRetained(acc.field)) { // It may happen for unit fields, tests/run/i6987.scala dropped += acc.field Nil } else { val param = acc.subst(accessors, paramSyms) - if (param.hasAnnotation(defn.ConstructorOnlyAnnot)) - report.error(em"${acc.name} is marked `@constructorOnly` but it is retained as a field in ${acc.owner}", acc.srcPos) + if param.hasAnnotation(defn.ConstructorOnlyAnnot) then + val msg = em"${acc.name} is marked `@constructorOnly` but it is retained as a field in ${acc.owner}" + report.error(msg, acc.srcPos) val target = if (acc.is(Method)) acc.field else acc if (!target.exists) Nil // this case arises when the parameter accessor is an alias else { diff --git a/tests/neg/i22979.scala b/tests/neg/i22979.scala new file mode 100644 index 000000000000..1f3175e80b03 --- /dev/null +++ b/tests/neg/i22979.scala @@ -0,0 +1,11 @@ + +import annotation.* + +class C(@constructorOnly s: String): // error + def g: Any => String = _ => s + def f[A](xs: List[A]): List[String] = xs.map(g) + +import scala.util.boundary + +class Leak()(using @constructorOnly l: boundary.Label[String]): // error + lazy val broken = Option("stop").foreach(boundary.break(_)) diff --git a/tests/pos/i22979.scala b/tests/pos/i22979.scala new file mode 100644 index 000000000000..153b113e4770 --- /dev/null +++ b/tests/pos/i22979.scala @@ -0,0 +1,18 @@ + +import annotation.* + +class C(@constructorOnly s: String): + val g: Any => String = _ => s + def f[A](xs: List[A]): List[String] = xs.map(g) + +import scala.util.boundary + +class Leak()(using @constructorOnly l: boundary.Label[String]): + Option("stop").foreach(boundary.break(_)) + + +class Lapse: + def f = Lapse.DefaultSentinelFn() +object Lapse: + private val DefaultSentinel: AnyRef = new AnyRef + private val DefaultSentinelFn: () => AnyRef = () => DefaultSentinel diff --git a/tests/run/i22979/Leak.scala b/tests/run/i22979/Leak.scala new file mode 100644 index 000000000000..792473e333d7 --- /dev/null +++ b/tests/run/i22979/Leak.scala @@ -0,0 +1,13 @@ +import annotation.* + +class Leak()(using @constructorOnly l: boundary.Label[String]) { + Seq("a", "b").foreach(_ => boundary.break("stop")) +} + +object boundary { + final class Break[T] private[boundary](val label: Label[T], val value: T) + extends RuntimeException( + /*message*/ null, /*cause*/ null, /*enableSuppression=*/ false, /*writableStackTrace*/ false) + final class Label[-T] + def break[T](value: T)(using label: Label[T]): Nothing = throw new Break(label, value) +} diff --git a/tests/run/i22979/test.scala b/tests/run/i22979/test.scala new file mode 100644 index 000000000000..2dd3bec1aab0 --- /dev/null +++ b/tests/run/i22979/test.scala @@ -0,0 +1,23 @@ +// scalajs: --skip + +import util.Try + +@main def Test = + assert(Try(classOf[Leak].getDeclaredField("l")).isFailure) + assert(classOf[Leak].getFields.length == 0) + //classOf[Leak].getFields.map(_.getName).foreach(println) //DEBUG + assert(classOf[C].getFields.length == 0) + //classOf[Lapse.type].getFields.map(_.getName).foreach(println) //DEBUG + +class C: + private val x = 42 + println(x) + println(List(27).map(_ + x)) + +// The easy tweak for lambdas does not work for module owner. +// The lambdalifted anonfun is not transformed correctly. +class Lapse: + def f = Lapse.DefaultSentinelFn() +object Lapse: + private val DefaultSentinel: AnyRef = new AnyRef + private val DefaultSentinelFn: () => AnyRef = () => DefaultSentinel