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

compiler/src/dotty/tools/dotc/core/Definitions.scala

@@ -515,6 +515,9 @@ class Definitions {
   lazy val JavaSerializableClass     = ctx.requiredClass("java.io.Serializable")
   lazy val ComparableClass           = ctx.requiredClass("java.lang.Comparable")
 
+  lazy val SystemClass               = ctx.requiredClass("java.lang.System")
+  lazy val SystemModule              = SystemClass.linkedClass
+
   // in scalac modified to have Any as parent
 
   lazy val SerializableType: TypeRef       = ctx.requiredClassRef("scala.Serializable")

compiler/src/dotty/tools/dotc/transform/localopt/Devalify.scala

@@ -10,7 +10,7 @@ import core.Types._
 import ast.Trees._
 import scala.collection.mutable
 import config.Printers.simplify
-import Simplify.desugarIdent
+import Simplify.{desugarIdent, isEffectivelyMutable}
 import transform.SymUtils._
 
 /** Inline vals and remove vals that are aliases to other vals
@@ -164,56 +164,66 @@ class Devalify extends Optimisation {
     case _ => t
   }
 
-  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))
-      def isAccessingProductField   = t.symbol.exists                               &&
-                                      t.symbol.owner.derivesFrom(defn.ProductClass) &&
-                                      t.symbol.owner.is(CaseClass)                  &&
-                                      t.symbol.name.isSelectorName                  &&
-                                      !isCaseClassWithVar // Conservative Covers case class A(var x: Int)
-      def isImmutableCaseAccessor   = t.symbol.is(CaseAccessor) && !t.symbol.is(Mutable)
-      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
-      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
-          !t.symbol.owner.caseAccessors(ctx)(fieldId).is(Mutable)
-        }
-        if (isImmutableField) readingOnlyVals(rec) // accessing a field of a product
+  def readingOnlyVals(t: Tree)(implicit ctx: Context): Boolean = {
+    def isGetterOfAImmutableField = t.symbol.isGetter && !t.symbol.is(Mutable)
+    def isCaseClassWithVar        = t.symbol.info.decls.exists(_.is(Mutable))
+    def isAccessingProductField   = t.symbol.exists                               &&
+                                    t.symbol.owner.derivesFrom(defn.ProductClass) &&
+                                    t.symbol.owner.is(CaseClass)                  &&
+                                    t.symbol.name.isSelectorName                  &&
+                                    !isCaseClassWithVar // Conservatively covers case class A(var x: Int)
+    def isImmutableCaseAccessor   = t.symbol.is(CaseAccessor) && !t.symbol.is(Mutable)
+
+    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
-      } else if (t.symbol.is(CaseAccessor) && !t.symbol.is(Mutable))
-        readingOnlyVals(rec)
-      else false
-    case Select(qual, _) if !t.symbol.is(Mutable) =>
-      readingOnlyVals(qual)
-    case t: Ident if !t.symbol.is(Mutable) && !t.symbol.is(Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
-      desugarIdent(t) match {
-        case Some(t) => readingOnlyVals(t)
-        case None => true
-      }
-    case t: This => true
-    // null => false, or the following fails devalify:
-    // trait I {
-    //   def foo: Any = null
-    // }
-    // object Main {
-    //   def main = {
-    //     val s: I = null
-    //     s.foo
-    //   }
-    // }
-    case Literal(Constant(null)) => false
-    case t: Literal => true
-    case _ => false
+
+      case Apply(Select(rec, _), Nil) =>
+        if (isGetterOfAImmutableField || isAccessingProductField || isImmutableCaseAccessor)
+          readingOnlyVals(rec)
+        else false
+
+      case Select(rec, _) if t.symbol.is(Method) =>
+        if (isGetterOfAImmutableField)
+          readingOnlyVals(rec) // Getter of an immutable field
+        else if (isAccessingProductField) {
+          def isImmutableField = {
+            val fieldId = t.symbol.name.toString.drop(1).toInt - 1
+            !t.symbol.owner.caseAccessors(ctx)(fieldId).is(Mutable)
+          }
+          if (isImmutableField) readingOnlyVals(rec) // Accessing a field of a product
+          else false
+        } else if (isImmutableCaseAccessor)
+          readingOnlyVals(rec)
+        else false
+
+      case t @ Select(qual, _) if !isEffectivelyMutable(t) =>
+        readingOnlyVals(qual)
+
+      case t: Ident if !t.symbol.is(Mutable | Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
+        desugarIdent(t) match {
+          case Some(t) => readingOnlyVals(t)
+          case None => true
+        }
+
+      case t: This => true
+      // null => false, or the following fails devalify:
+      // trait I {
+      //   def foo: Any = null
+      // }
+      // object Main {
+      //   def main = {
+      //     val s: I = null
+      //     s.foo
+      //   }
+      // }
+      case Literal(Constant(null)) => false
+      case t: Literal => true
+      case _ => false
+    }
   }
 }

compiler/src/dotty/tools/dotc/transform/localopt/DropGoodCasts.scala

@@ -10,6 +10,7 @@ import core.Types._
 import core.Flags._
 import ast.Trees._
 import transform.SymUtils._
+import Simplify.isEffectivelyMutable
 
 /** Eliminated casts and equality tests whose results can be locally
  *  determined at compile time:
@@ -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)       &&
+             !isEffectivelyMutable(x)             &&
+             !x.symbol.is(Method)                 &&
              x.symbol.exists && !x.symbol.owner.isClass =>
           (x.symbol, tp.tpe) :: Nil
 
@@ -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) &&
+               !isEffectivelyMutable(x)       &&
+               !x.symbol.is(Method)           &&
                x.symbol.exists && !x.symbol.owner.isClass =>
           x.symbol :: Nil
 

compiler/src/dotty/tools/dotc/transform/localopt/DropNoEffects.scala

@@ -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) =>

compiler/src/dotty/tools/dotc/transform/localopt/Simplify.scala

@@ -146,4 +146,15 @@ object Simplify {
       case _ => None
     }
   }
+
+  /** Is this tree mutable, or java.lang.System.{in, out, err}? These three
+   *  System members are the only static final fields that are mutable.
+   *  See https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.4
+   */
+  def isEffectivelyMutable(t: Tree)(implicit ctx: Context): Boolean = t match {
+    case _ if t.symbol.is(Mutable) => true
+    case s: Select => s.symbol.owner == defn.SystemModule
+    case i: Ident  => desugarIdent(i).exists(isEffectivelyMutable)
+    case _ => false
+  }
 }

docs/docs/reference/optimisations.md

@@ -0,0 +1,8 @@
+---
+layout: doc-page
+title: "Local Optimisations"
+---
+
+The Dotty compiler implements several local optimisations to generate faster bytecode. These optimisations can be enabled by compiling with the `-optimise` flag. The current best source of documentation about what tree transformations are performed under `-optimise` is the Scaladoc classes in the [localopt package](https://github.com/lampepfl/dotty/tree/master/compiler/src/dotty/tools/dotc/transform/localopt).
+
+Local optimisations assumes no use of Java Reflection to mutate final fields.

docs/sidebar.yml

@@ -51,6 +51,8 @@ sidebar:
               url: docs/reference/auto-parameter-tupling.html
             - title: Named Type Arguments
               url: docs/reference/named-typeargs.html
+            - title: Local Optimisations
+              url: docs/reference/optimisations.html
         - title: Changed Features
           subsection:
             - title: Volatile Lazy Vals

tests/run/2772.scala

@@ -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)
+  }
+}