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
12 changes: 3 additions & 9 deletions compiler/src/dotty/tools/dotc/transform/localopt/Devalify.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import core.Types._
import ast.Trees._
import scala.collection.mutable
import config.Printers.simplify
import Simplify.desugarIdent
import Simplify.{desugarIdent, isMutable}
import transform.SymUtils._

/** Inline vals and remove vals that are aliases to other vals
Expand Down Expand Up @@ -201,14 +201,8 @@ class Devalify extends Optimisation {
readingOnlyVals(rec)
else false

case Select(qual, _) if !t.symbol.is(Mutable) =>
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 @ Select(qual, _) if !isMutable(t) =>
readingOnlyVals(qual)

case t: Ident if !t.symbol.is(Mutable | Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
desugarIdent(t) match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import core.Types._
import core.Flags._
import ast.Trees._
import transform.SymUtils._
import Simplify.isMutable

/** Eliminated casts and equality tests whose results can be locally
* determined at compile time:
Expand Down Expand Up @@ -77,7 +78,8 @@ import transform.SymUtils._

case TypeApply(fun @ Select(x, _), List(tp))
if fun.symbol.eq(defn.Any_isInstanceOf) &&
!x.symbol.is(Method | Mutable) &&
!isMutable(x) &&
!x.symbol.is(Method) &&
x.symbol.exists && !x.symbol.owner.isClass =>
(x.symbol, tp.tpe) :: Nil

Expand All @@ -89,14 +91,16 @@ import transform.SymUtils._
def collectNullTests(t: Tree)(implicit ctx: Context): List[Symbol] = {
def recur(t: Tree): List[Symbol] =
t match {
case Apply(x, _) if (x.symbol == defn.Boolean_! || x.symbol == defn.Boolean_||) => Nil
case Apply(x, _) if (x.symbol == defn.Boolean_! || x.symbol == defn.Boolean_||) =>
Nil

case Apply(fun @ Select(x, _), y) if (fun.symbol == defn.Boolean_&&) =>
recur(x) ++ recur(y.head)

case Apply(fun @ Select(x, _), List(tp))
if fun.symbol.eq(defn.Object_ne) &&
!x.symbol.is(Method | Mutable) &&
!isMutable(x) &&
!x.symbol.is(Method) &&
x.symbol.exists && !x.symbol.owner.isClass =>
x.symbol :: Nil

Expand Down
11 changes: 11 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/localopt/Simplify.scala
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,15 @@ object Simplify {
case _ => None
}
}

// System.in is static final fields that, for legacy reasons, must be
Copy link
Member

Choose a reason for hiding this comment

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

is -> is a
fields -> field

This description is a bit confusing: it starts by talking just about System.in and then mention the rest.
The name of the method is also not great, it's too easy to confuse with is(Mutable), I would call it something like isEffectivelyMutable, isReallyMutable or isChanging

// allowed to be changed by the methods System.setIn. `isMutable` is true
// is the field is Mutable or if it's a field of java.lang.System.
// https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.4
def isMutable(t: Tree)(implicit ctx: Context): Boolean = t match {
case _ if t.symbol.is(Mutable) => true
case s: Symbol => (s.symbol == defn.SystemModule)
Copy link
Member

Choose a reason for hiding this comment

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

I think you want s.owner.symbol here.

case i: Ident => desugarIdent(i).exists(isMutable)
case _ => false
}
}