From 22a959a4f514328d6bd955a8681163ff25734f26 Mon Sep 17 00:00:00 2001 From: odersky Date: Tue, 12 Dec 2023 10:57:18 +0100 Subject: [PATCH 1/3] Make more anonymous functions static An anonymous function in a static object was previously mapped to a member of that object. We now map it to a static member of the toplevel class instead. This causes the backend to memoize the function, which fixes #19224. On the other hand, we don't do that for anonymous functions nested in the object constructor, since that can cause deadlocks (see run/deadlock.scala). Scala 2's behavior is different: it does lift lambdas in constructors to be static, too, which can cause deadlocks. Fixes #19224 --- .../tools/dotc/transform/Dependencies.scala | 17 +++++++++----- tests/run/i19224.scala | 22 +++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 tests/run/i19224.scala diff --git a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala index fe429992fd46..57a0468422bb 100644 --- a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala +++ b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala @@ -1,4 +1,5 @@ -package dotty.tools.dotc +package dotty.tools +package dotc package transform import core.* @@ -184,7 +185,6 @@ abstract class Dependencies(root: ast.tpd.Tree, @constructorOnly rootContext: Co def setLogicOwner(local: Symbol) = val encClass = local.owner.enclosingClass val preferEncClass = - ( encClass.isStatic // non-static classes can capture owners, so should be avoided && (encClass.isProperlyContainedIn(local.topLevelClass) @@ -192,15 +192,22 @@ abstract class Dependencies(root: ast.tpd.Tree, @constructorOnly rootContext: Co || encClass.is(ModuleClass, butNot = Package) // needed to not cause deadlocks in classloader. see t5375.scala ) - ) - || ( + && (!sym.isAnonymousFunction || sym.owner.ownersIterator.exists(_.isConstructor)) + // For anonymous functions in static objects, we prefer them to be static because + // that means lambdas are memoized and can be serialized even if the enclosing object + // is not serializable. See run/lambda-serialization-gc.scala and run/i19224.scala. + // On the other hand, we don't want to lift anonymous functions from inside the + // object constructor to be static since that can cause deadlocks by its interaction + // with class initialization. See run/deadlock.scala, which works in Scala 3 + // but deadlocks in Scala 2. + || /* Scala.js: Never move any member beyond the boundary of a DynamicImportThunk. * DynamicImportThunk subclasses are boundaries between the eventual ES modules * that can be dynamically loaded. Moving members across that boundary changes * the dynamic and static dependencies between ES modules, which is forbidden. */ ctx.settings.scalajs.value && encClass.isSubClass(jsdefn.DynamicImportThunkClass) - ) + logicOwner(sym) = if preferEncClass then encClass else local.enclosingPackageClass tree match diff --git a/tests/run/i19224.scala b/tests/run/i19224.scala new file mode 100644 index 000000000000..3d076e35ee7e --- /dev/null +++ b/tests/run/i19224.scala @@ -0,0 +1,22 @@ +// scalajs: --skip + +object Test extends App { + val field = 1 + def x(): Int => String = (i: Int) => i.toString + def y(): () => String = () => field.toString + + locally { + assert(x() == x()) // true on Scala 2, was false on Scala 3... + assert(y() == y()) // also true if `y` accesses object-local fields + + def z(): Int => String = (i: Int) => i.toString + assert(z() != z()) // lambdas in constructor are not lifted to static, so no memoization (Scala 2 lifts them, though). + } + + val t1 = new C + val t2 = new C + + locally { + assert(t1.x() == t2.x()) // true on Scala 2, was false on Scala 3... + } +} From 5b8e6c9ca997efd6e981398a291e9196958a373d Mon Sep 17 00:00:00 2001 From: odersky Date: Wed, 13 Dec 2023 21:38:37 +0100 Subject: [PATCH 2/3] Fix test and add more comments --- .../tools/dotc/transform/Dependencies.scala | 33 ++++++++++++++----- tests/run/i19224.scala | 3 ++ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala index 57a0468422bb..692ec86495f0 100644 --- a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala +++ b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala @@ -182,24 +182,39 @@ abstract class Dependencies(root: ast.tpd.Tree, @constructorOnly rootContext: Co if enclClass.isContainedIn(thisClass) then thisClass else enclClass) // unknown this reference, play it safe and assume the narrowest possible owner + /** Set the first owner of a local method or class that's nested inside a term. + * This is either the enclosing package or the enclosing class. If the former, + * the method will be be translated to a static method of its toplevel class. + * In that case, we might later re-adjust the owner to a nested class via + * `narrowTo` when we see that the method refers to the this-type of that class. + * We choose the enclosing package when there's something potentially to gain from this + * and when it is safe to do so + */ def setLogicOwner(local: Symbol) = val encClass = local.owner.enclosingClass + // When to [efer enclosing class over enclosing package: val preferEncClass = encClass.isStatic - // non-static classes can capture owners, so should be avoided + // If class is not static, we try to hoist the method out of + // the class to avoid the outer pointer. && (encClass.isProperlyContainedIn(local.topLevelClass) - // can be false for symbols which are defined in some weird combination of supercalls. + // If class is nested in an outer object, we prefer to leave the method in the class, + // since putting it in the outer object makes access more complicated || encClass.is(ModuleClass, butNot = Package) - // needed to not cause deadlocks in classloader. see t5375.scala + // If class is an outermost object we also want to avoid making the + // method static since that could cause deadlocks in interacting + // with class initialization. See deadlock.scala ) && (!sym.isAnonymousFunction || sym.owner.ownersIterator.exists(_.isConstructor)) - // For anonymous functions in static objects, we prefer them to be static because - // that means lambdas are memoized and can be serialized even if the enclosing object + // The previous conditions mean methods in static objects and nested static classes + // don't get lifted out to be static. In general it is prudent to do that. However, + // for anonymous functions, we prefer them to be static because that means lambdas + // are memoized and can be serialized even if the enclosing object or class // is not serializable. See run/lambda-serialization-gc.scala and run/i19224.scala. // On the other hand, we don't want to lift anonymous functions from inside the - // object constructor to be static since that can cause deadlocks by its interaction - // with class initialization. See run/deadlock.scala, which works in Scala 3 - // but deadlocks in Scala 2. + // object or class constructor to be static since that can cause again deadlocks + // by its interaction with class initialization. See run/deadlock.scala, which works + // in Scala 3 but deadlocks in Scala 2. || /* Scala.js: Never move any member beyond the boundary of a DynamicImportThunk. * DynamicImportThunk subclasses are boundaries between the eventual ES modules @@ -207,7 +222,7 @@ abstract class Dependencies(root: ast.tpd.Tree, @constructorOnly rootContext: Co * the dynamic and static dependencies between ES modules, which is forbidden. */ ctx.settings.scalajs.value && encClass.isSubClass(jsdefn.DynamicImportThunkClass) - + logicOwner(sym) = if preferEncClass then encClass else local.enclosingPackageClass tree match diff --git a/tests/run/i19224.scala b/tests/run/i19224.scala index 3d076e35ee7e..76396206dc77 100644 --- a/tests/run/i19224.scala +++ b/tests/run/i19224.scala @@ -20,3 +20,6 @@ object Test extends App { assert(t1.x() == t2.x()) // true on Scala 2, was false on Scala 3... } } +class C { + def x(): Int => String = (i: Int) => i.toString +} \ No newline at end of file From 7878dbc3d08625aa33c6344eeec700f276a5d0d5 Mon Sep 17 00:00:00 2001 From: odersky Date: Wed, 13 Dec 2023 21:55:02 +0100 Subject: [PATCH 3/3] Update compiler/src/dotty/tools/dotc/transform/Dependencies.scala --- compiler/src/dotty/tools/dotc/transform/Dependencies.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala index 692ec86495f0..523ea75be912 100644 --- a/compiler/src/dotty/tools/dotc/transform/Dependencies.scala +++ b/compiler/src/dotty/tools/dotc/transform/Dependencies.scala @@ -192,7 +192,7 @@ abstract class Dependencies(root: ast.tpd.Tree, @constructorOnly rootContext: Co */ def setLogicOwner(local: Symbol) = val encClass = local.owner.enclosingClass - // When to [efer enclosing class over enclosing package: + // When to prefer the enclosing class over the enclosing package: val preferEncClass = encClass.isStatic // If class is not static, we try to hoist the method out of