Skip to content

Fix #7434: Tighten a condition in LazyVals #7435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 26, 2019
Merged

Fix #7434: Tighten a condition in LazyVals #7435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f92d31
Fix #7434: Tighten condition for optimizing synthetic objects in LazyVal
odersky Oct 18, 2019
2e46d55
Put back private modifier in test
odersky Oct 18, 2019
af1bde2
Fix handling of private trait objects
odersky Oct 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/LazyVals.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.core.Decorators._
import dotty.tools.dotc.core.DenotTransformers.IdentityDenotTransformer
import dotty.tools.dotc.core.Flags._
import dotty.tools.dotc.core.NameKinds.{LazyBitMapName, LazyLocalInitName, LazyLocalName}
import dotty.tools.dotc.core.NameKinds.{LazyBitMapName, LazyLocalInitName, LazyLocalName, ExpandedName}
import dotty.tools.dotc.core.StdNames.nme
import dotty.tools.dotc.core.Symbols._
import dotty.tools.dotc.core.Types._
Expand Down Expand Up @@ -73,7 +73,18 @@ class LazyVals extends MiniPhase with IdentityDenotTransformer {
else {
val isField = sym.owner.isClass
if (isField)
if (sym.isAllOf(SyntheticModule))
if sym.isAllOf(SyntheticModule)
&& sym.allOverriddenSymbols.isEmpty
&& !sym.name.is(ExpandedName) then
// I am not sure what the conditions for this optimization should be.
// It was applied for all synthetic objects, but this is clearly false, as t704 demonstrates.
// It seems we have to at least exclude synthetic objects that derive from mixins.
// This is done by demanding that the object does not override anything.
// Figuring out whether a symbol implements a trait module is not so simple.
// For non-private trait members we can check whether there is an overridden symbol.
// For private trait members this does not work, since `ensureNotPrivate` in phase Mixins
// does change the name but does not update the owner's scope, so `allOverriddenSymbols` does
// not work in that case. However, we can check whether the name is an ExpandedName instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is a little brittle.

The optimization was introduced in 8f47f16 to fix #676 . Since #4090, we no longer generate companion objects in FirstTransform anymore, it seems we can just disable the optimization.

If the optimization is needed, what about using a compiler flag to flag an object as eager, and we only check flags here? That flag could also be reused to enable more optimization opportunities without worrying about semantic problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with automatic generation of companion objects disabled, I believe there are still a lot of them, so it's worthwhile to optimize. But the criterion as it stands is somewhat arbitrary and could be improved. E.g. we could also strictify companion objects that are pure.

transformSyntheticModule(tree)
else if (sym.isThreadUnsafe || ctx.settings.scalajs.value)
if (sym.is(Module) && !ctx.settings.scalajs.value) {
Expand Down
6 changes: 6 additions & 0 deletions tests/run/t704.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
xxxx should appear twice
zzzz should appear twice
yyyy should appear twice
xxxx should appear twice
zzzz should appear twice
yyyy should appear twice
9 changes: 8 additions & 1 deletion tests/pos/t704.scala → tests/run/t704.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,27 @@ trait D {
private val x = "xxxx should appear twice"
private object xxxx { Console.println(x) }
def get_xxxx: AnyRef = xxxx

private val z = "zzzz should appear twice"
private lazy val zzzz = new ZZZZ
class ZZZZ { Console.println(z) }
def get_zzzz: AnyRef = zzzz
}

trait E extends D {
def f(): Unit = {
val y = "yyyy should appear twice"
object yyyy {
val x1 = get_xxxx
val z1 = get_zzzz
Console.println(y)
}
yyyy
}
}
class C extends E {}
object Go extends D {
object Test extends D {
object E
def main(args : Array[String]): Unit = {
new C().f()
new C().f()
Expand Down