Skip to content

Fix #2772: Special case Devalify for java.lang.System.* #2781

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

Merged
merged 7 commits into from
Jun 25, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ class Definitions {
lazy val BoxedFloatModule = ctx.requiredModule("java.lang.Float")
lazy val BoxedDoubleModule = ctx.requiredModule("java.lang.Double")
lazy val BoxedUnitModule = ctx.requiredModule("java.lang.Void")
lazy val SystemModule = ctx.requiredModule("java.lang.System")

lazy val ByNameParamClass2x = enterSpecialPolyClass(tpnme.BYNAME_PARAM_CLASS, Covariant, Seq(AnyType))
lazy val EqualsPatternClass = enterSpecialPolyClass(tpnme.EQUALS_PATTERN, EmptyFlags, Seq(AnyType))
Expand Down
17 changes: 15 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/localopt/Devalify.scala
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,12 @@ class Devalify extends Optimisation {

def readingOnlyVals(t: Tree)(implicit ctx: Context): Boolean = dropCasts(t) match {
case Typed(exp, _) => readingOnlyVals(exp)

case TypeApply(fun @ Select(rec, _), List(tp)) =>
if ((fun.symbol eq defn.Any_asInstanceOf) && rec.tpe.derivesFrom(tp.tpe.classSymbol))
readingOnlyVals(rec)
else false

case Apply(Select(rec, _), Nil) =>
def isGetterOfAImmutableField = t.symbol.isGetter && !t.symbol.is(Mutable)
def isCaseClassWithVar = t.symbol.info.decls.exists(_.is(Mutable))
Expand All @@ -182,8 +184,10 @@ class Devalify extends Optimisation {
if (isGetterOfAImmutableField || isAccessingProductField || isImmutableCaseAccessor)
readingOnlyVals(rec)
else false

case Select(rec, _) if t.symbol.is(Method) =>
if (t.symbol.isGetter && !t.symbol.is(Mutable)) readingOnlyVals(rec) // getter of a immutable field
if (t.symbol.isGetter && !t.symbol.is(Mutable))
readingOnlyVals(rec) // getter of a immutable field
Copy link
Member

Choose a reason for hiding this comment

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

typo: a -> an

else if (t.symbol.owner.derivesFrom(defn.ProductClass) && t.symbol.owner.is(CaseClass) && t.symbol.name.isSelectorName) {
def isImmutableField = {
val fieldId = t.symbol.name.toString.drop(1).toInt - 1
Expand All @@ -194,13 +198,22 @@ class Devalify extends Optimisation {
} else if (t.symbol.is(CaseAccessor) && !t.symbol.is(Mutable))
readingOnlyVals(rec)
else false

case Select(qual, _) if !t.symbol.is(Mutable) =>
readingOnlyVals(qual)
if (t.symbol == defn.SystemModule) {
// System.in is static final fields that, for legacy reasons, must be
// allowed to be changed by the methods System.setIn...
// https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.4
false
} else
readingOnlyVals(qual)

case t: Ident if !t.symbol.is(Mutable) && !t.symbol.is(Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
Copy link
Member

Choose a reason for hiding this comment

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

Nothing says that there might not be an Ident corresponding to System.in, and in that case you would still inline it. Maybe add a isReallyMutable instead that you would use everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation of this case calls desugarIdent and recuse, I think the case where the Ident is System.in is covered.

@DarkDimius Could you explain why we even need this desugarIdent, why not use Select everywhere directly? Also where would be the proper was to move that function to factor out the version that's currently copy pasted in the backend?

desugarIdent(t) match {
case Some(t) => readingOnlyVals(t)
case None => true
}

case t: This => true
// null => false, or the following fails devalify:
// trait I {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ class DropNoEffects(val simplifyPhase: Simplify) extends Optimisation {
keepOnlySideEffects(rec)

// !name.eq(nme.TYPE_) && // Keep the .TYPE added by ClassOf, would be needed for AfterErasure
case s @ Select(qual, name) if !s.symbol.is(Mutable | Lazy | Method) =>
// Without is(JavaStatic), { System.out } becomes { System }, but "Java class can't be used as value"
case s @ Select(qual, name) if !s.symbol.is(Mutable | Lazy | Method | JavaStatic) =>
keepOnlySideEffects(qual)

case Block(List(t: DefDef), s: Closure) =>
Expand Down
14 changes: 14 additions & 0 deletions tests/run/2772.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import java.io.OutputStream

object Test {
def main(args: Array[String]): Unit = {
val oldErr = System.err
System.setErr(null) // setOut(null) confuses the testing framework...
val a = () => foo(oldErr)
a()
}

def foo(err: OutputStream): Unit = {
err.write(0)
}
}