Skip to content
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.

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

Fix #5350: Fix purity of local lazy vals #5703

Merged
merged 3 commits into from
Jan 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,12 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
def isKnownPureOp(sym: Symbol) =
sym.owner.isPrimitiveValueClass || sym.owner == defn.StringClass
if (tree.tpe.isInstanceOf[ConstantType] && isKnownPureOp(tree.symbol) // A constant expression with pure arguments is pure.
|| fn.symbol.isStable
|| (fn.symbol.isStable && !fn.symbol.is(Lazy))
|| fn.symbol.isPrimaryConstructor && fn.symbol.owner.isNoInitsClass) // TODO: include in isStable?
minOf(exprPurity(fn), args.map(exprPurity)) `min` Pure
else if (fn.symbol.is(Erased)) Pure
else if (fn.symbol.isStable /* && fn.symbol.is(Lazy) */)
minOf(exprPurity(fn), args.map(exprPurity)) `min` Idempotent
else Impure
case Typed(expr, _) =>
exprPurity(expr)
Expand Down
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/printing/ReplPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class ReplPrinter(_ctx: Context) extends DecompilerPrinter(_ctx) {
else Str(const.value.toString)

override def dclText(sym: Symbol): Text = {
("lazy": Text).provided(sym.is(Lazy)) ~~
toText(sym) ~ {
if (sym.is(Method)) toText(sym.info)
else if (sym.isType && sym.info.isTypeAlias) toText(sym.info)
Expand Down
7 changes: 2 additions & 5 deletions compiler/src/dotty/tools/repl/Rendering.scala
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,8 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) {
val dcl = d.symbol.showUser

try {
val resultValue =
if (d.symbol.is(Flags.Lazy)) Some("<lazy>")
else valueOf(d.symbol)

resultValue.map(value => s"$dcl = $value")
if (d.symbol.is(Flags.Lazy)) Some(dcl)
else valueOf(d.symbol).map(value => s"$dcl = $value")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 on printing the flag to the left (in ReplPrinter). On this change, FWIW, Scala 2 also uses <lazy> here. Not sure anybody cares tho, so merging anyway.

scala> lazy val x = 1
x: Int = <lazy>

}
catch { case ex: InvocationTargetException => Some(renderError(ex)) }
}
Expand Down
7 changes: 7 additions & 0 deletions compiler/test-resources/repl/i5350
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
scala> def foo = { lazy val bar: Unit = { println("Hello") }; bar }
def foo: Unit
scala> foo
Hello
scala> { lazy val bar: Unit = { println("Hello") }; bar }
Hello
lazy val bar: Unit
1 change: 1 addition & 0 deletions tests/run/i5350.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello
10 changes: 10 additions & 0 deletions tests/run/i5350.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
object Test {
def foo = {
lazy val bar: Unit = println("Hello")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Blaisorblade does it make sense for this lazy val to be stable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. The definition of isStable doesn't check for Lazy, so Symbol.isStable should only imply idempotence.

To detect if a value is pure, Realizability checks sym.isStable && !sym.is(Lazy | Erased, butNot = Module). Maybe here you should check
sym.isStable && !sym.is(Lazy, butNot = Module)? But on modules, this overlaps with #2266 and #4765.

That all needs testcases and consideration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually modules do not reach this line. Hence the current condition does not affect the module init elimination.

bar
}

def main(args: Array[String]): Unit = {
foo
}
}
1 change: 1 addition & 0 deletions tests/run/i5350b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello
10 changes: 10 additions & 0 deletions tests/run/i5350b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
object Test {
def foo: Unit = {
object bar { println("Hello") }
bar
}

def main(args: Array[String]): Unit = {
foo
}
}
1 change: 1 addition & 0 deletions tests/run/i5350c.check
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello
10 changes: 10 additions & 0 deletions tests/run/i5350c.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
object Test {
def foo: Unit = {
bar
}

def main(args: Array[String]): Unit = {
foo
}
}
object bar { println("Hello") }
2 changes: 2 additions & 0 deletions tests/run/i5350d.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Hello
apply
13 changes: 13 additions & 0 deletions tests/run/i5350d.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
object Test {
def foo: Unit = {
bar()
}

def main(args: Array[String]): Unit = {
foo
}
}
object bar {
println("Hello")
def apply(): Unit = println("apply")
}