Address review comments

abeln · abeln · commit 66e83b4c83c8 · 2019-05-07T17:37:05.000-04:00
diff --git a/compiler/src/dotty/tools/dotc/core/FlowTyper.scala b/compiler/src/dotty/tools/dotc/core/FlowTyper.scala
@@ -207,7 +207,6 @@ object FlowTyper {
    *  1) a return
    *  2) an expression of type `Nothing` (in particular, usages of `throw`)
    *  3) a block where the last expression is non-local
-   *  4) nothing else is non-local
    *
    *  So, for example, if we know that `x` must be non-null if `cond` is true, and `else` is non-local,
    *  then in order for `s2` to execute `cond` must be true. We can thus soundly add `x` to our
diff --git a/compiler/src/dotty/tools/dotc/core/JavaNullInterop.scala b/compiler/src/dotty/tools/dotc/core/JavaNullInterop.scala
@@ -6,11 +6,10 @@ import dotty.tools.dotc.core.StdNames.{jnme, nme}
 import dotty.tools.dotc.core.Symbols.{Symbol, defn, _}
 import dotty.tools.dotc.core.Types.{AndType, AppliedType, LambdaType, MethodType, OrType, PolyType, Type, TypeAlias, TypeMap, TypeParamRef, TypeRef}
 
-/** This module defines methods to interpret Java types, which are implicitly nullable,
+/** This module defines methods to interpret types of Java symbols, which are implicitly nullable in Java,
  *  as Scala types, which are explicitly nullable.
  *
- *  The transformation from Java types to Scala types is (conceptually) a function `n`
- *  that adheres to the following rules:
+ *  The transformation is (conceptually) a function `n` that adheres to the following rules:
  *    (1) n(T)              = T|JavaNull              if T is a reference type
  *    (2) n(T)              = T                       if T is a value type
  *    (3) n(C[T])           = C[T]|JavaNull           if C is Java-defined
@@ -20,6 +19,15 @@ import dotty.tools.dotc.core.Types.{AndType, AppliedType, LambdaType, MethodType
  *    (7) n((A1, ..., Am)R) = (n(A1), ..., n(Am))n(R) for a method with arguments (A1, ..., Am) and return type R
  *    (8) n(T)              = T                       otherwise
  *
+ *   Treatment of generics (rules 3 and 4):
+ *     - if `C` is Java-defined, then `n(C[T]) = C[T]|JavaNull`. That is, we don't recurse
+ *       on the type argument, and only add JavaNull on the outside. This is because
+ *       `C` itself will be nullified, and in particular so will be usages of `C`'s type argument within C's body.
+ *       e.g. calling `get` on a `java.util.List[String]` already returns `String|Null` and not `String`, so
+ *       we don't need to write `java.util.List[String|Null]`.
+ *     - if `C` is Scala-defined, however, then we want `n(C[T]) = C[n(T)]|JavaNull`. This is because
+ *       `C` won't be nullified, so we need to indicate that its type argument is nullable.
+ *
  *   Notice that since the transformation is only applied to types attached to Java symbols, it doesn't need
  *   to handle the full spectrum of Scala types. Additionally, some kinds of symbols like constructors and
  *   enum instances get special treatment.
@@ -47,30 +55,22 @@ object JavaNullInterop {
     assert(ctx.explicitNulls)
     assert(sym.is(JavaDefined), "can only nullify java-defined members")
 
-    // A list of members that are special-cased.
+    // A list of "policies" that special-case certain members.
+    // The policies should be disjoint: we use the first one that is applicable.
     val whitelist: Seq[NullifyPolicy] = Seq(
       // The `TYPE` field in every class: don't nullify.
-      NoOpP(_.name == nme.TYPE_),
+      NoOpPolicy(_.name == nme.TYPE_),
       // The `toString` method: don't nullify the return type.
-      paramsOnlyP(_.name == nme.toString_),
+      paramsOnlyPolicy(_.name == nme.toString_),
       // Constructors: params are nullified, but the result type isn't.
-      paramsOnlyP(_.isConstructor),
+      paramsOnlyPolicy(_.isConstructor),
       // Java enum instances: don't nullify.
-      NoOpP(_.is(Flags.JavaEnumValue))
+      NoOpPolicy(_.is(Flags.JavaEnumValue))
     )
 
-    val (fromWhitelistTp, handled) = whitelist.foldLeft((tp, false)) {
-      case (res@(_, true), _) => res
-      case ((_, false), pol) =>
-        if (pol.isApplicable(sym)) (pol(tp), true)
-        else (tp, false)
-    }
-
-    if (handled) {
-      fromWhitelistTp
-    } else {
-      // Default case: nullify everything.
-      nullifyType(tp)
+    whitelist.find(_.isApplicable(sym)) match {
+      case Some(pol) => pol(tp)
+      case None => nullifyType(tp) // default case: nullify everything
     }
   }
 
@@ -83,7 +83,7 @@ object JavaNullInterop {
   }
 
   /** A policy that leaves the passed-in type unchanged. */
-  private case class NoOpP(trigger: Symbol => Boolean) extends NullifyPolicy {
+  private case class NoOpPolicy(trigger: Symbol => Boolean) extends NullifyPolicy {
     override def isApplicable(sym: Symbol): Boolean = trigger(sym)
 
     override def apply(tp: Type): Type = tp
@@ -98,9 +98,9 @@ object JavaNullInterop {
    *  this applies only at the top level. e.g. suppose we have a Java result type `Array[String]` and `nnRes` is set.
    *  Scala will see `Array[String|JavaNull]`; the array element type is still nullified.
    */
-  private case class MethodP(trigger: Symbol => Boolean,
-                             nnParams: Seq[Int],
-                             nnRes: Boolean)(implicit ctx: Context) extends TypeMap with NullifyPolicy {
+  private case class MethodPolicy(trigger: Symbol => Boolean,
+                                  nnParams: Seq[Int],
+                                  nnRes: Boolean)(implicit ctx: Context) extends TypeMap with NullifyPolicy {
     override def isApplicable(sym: Symbol): Boolean = trigger(sym)
 
     private def spare(tp: Type): Type = {
@@ -125,8 +125,8 @@ object JavaNullInterop {
   }
 
   /** A policy that nullifies only method parameters (but not result types). */
-  private def paramsOnlyP(trigger: Symbol => Boolean)(implicit ctx: Context): MethodP = {
-    MethodP(trigger, nnParams = Seq.empty, nnRes = true)
+  private def paramsOnlyPolicy(trigger: Symbol => Boolean)(implicit ctx: Context): MethodPolicy = {
+    MethodPolicy(trigger, nnParams = Seq.empty, nnRes = true)
   }
 
   /** Nullifies a Java type by adding `| JavaNull` in the relevant places. */
@@ -140,7 +140,7 @@ object JavaNullInterop {
    *                         This is needed so that `JavaNullMap(A | B)` gives back `(A | B) | JavaNull`,
    *                         instead of `(A|JavaNull | B|JavaNull) | JavaNull`.
    */
-  private class JavaNullMap(alreadyNullable: Boolean)(implicit ctx: Context) extends TypeMap {
+  private class JavaNullMap(var alreadyNullable: Boolean)(implicit ctx: Context) extends TypeMap {
     /** Should we nullify `tp` at the outermost level? */
     def needsTopLevelNull(tp: Type): Boolean = {
       !alreadyNullable && (tp match {
@@ -158,30 +158,26 @@ object JavaNullInterop {
      *  `java.util.List[String|Null]` contain nullable elements.
      */
     def needsNullArgs(tp: AppliedType): Boolean = {
-      val AppliedType(tycons, _) = tp
-      tycons.widenDealias match {
-        case tp: TypeRef if !tp.symbol.is(JavaDefined) => true
-        case _ => false
-      }
+      !tp.classSymbol.is(JavaDefined)
     }
 
     override def apply(tp: Type): Type = {
       tp match {
+        case tp: TypeRef if needsTopLevelNull(tp) => tp.toJavaNullableUnion
+        case appTp @ AppliedType(tycons, targs) =>
+          val targs2 = if (needsNullArgs(appTp)) targs map this else targs
+          derivedAppliedType(appTp, tycons, targs2).toJavaNullableUnion
         case tp: LambdaType => mapOver(tp)
         case tp: TypeAlias => mapOver(tp)
-        case tp@AndType(tp1, tp2) =>
+        case tp @ AndType(tp1, tp2) =>
           // nullify(A & B) = (nullify(A) & nullify(B)) | JavaNull, but take care not to add
           // duplicate `JavaNull`s at the outermost level inside `A` and `B`.
-          val newMap = new JavaNullMap(alreadyNullable = true)
-          derivedAndType(tp, newMap(tp1), newMap(tp2)).toJavaNullableUnion
-        case tp@OrType(tp1, tp2) if !tp.isJavaNullableUnion =>
-          val newMap = new JavaNullMap(alreadyNullable = true)
-          derivedOrType(tp, newMap(tp1), newMap(tp2)).toJavaNullableUnion
-        case tp: TypeRef if needsTopLevelNull(tp) => tp.toJavaNullableUnion
+          alreadyNullable = true
+          derivedAndType(tp, this(tp1), this(tp2)).toJavaNullableUnion
+        case tp @ OrType(tp1, tp2) if !tp.isJavaNullableUnion =>
+          alreadyNullable = true
+          derivedOrType(tp, this(tp1), this(tp2)).toJavaNullableUnion
         case tp: TypeParamRef if needsTopLevelNull(tp) => tp.toJavaNullableUnion
-        case appTp@AppliedType(tycons, targs) =>
-          val targs2 = if (needsNullArgs(appTp)) targs map this else targs
-          derivedAppliedType(appTp, tycons, targs2).toJavaNullableUnion
         case _ => tp
       }
     }
diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala
@@ -693,11 +693,8 @@ object SymDenotations {
 
     /** Is this symbol a class of which `null` is a value? */
     final def isNullableClass(implicit ctx: Context): Boolean = {
-      if (ctx.explicitNulls && !ctx.phase.erasedTypes) {
-        symbol == defn.NullClass || symbol == defn.AnyClass
-      } else {
-        isNullableClassAfterErasure
-      }
+      if (ctx.explicitNulls && !ctx.phase.erasedTypes) symbol == defn.NullClass || symbol == defn.AnyClass
+      else isNullableClassAfterErasure
     }
 
     /** Is this symbol a class of which `null` is a value after erasure?
diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala
@@ -418,13 +418,15 @@ object Types {
       all.tail.foldLeft(all.head)(OrType.apply)
     }
 
-    /** Strips types according to `stripPred` from a nullable union. */
-    private def stripNullWithPred(stripPred: Type => Boolean)(implicit ctx: Context): Type = {
+    /** Strips `Null` from type unions in this type.
+     *  @param onlyJavaNull if true, we delete only `JavaNull` and not vanilla `Null`.
+     */
+    private def stripNullImpl(onlyJavaNull: Boolean)(implicit ctx: Context): Type = {
       assert(ctx.explicitNulls)
       def strip(tp: Type, changed: Boolean): (Type, Boolean) = {
         tp match {
           case OrType(left, right) if right.isNullType =>
-            if (stripPred(right)) strip(left, changed = true)
+            if (!onlyJavaNull || right.isJavaNullType) strip(left, changed = true)
             else {
               val (tp1, changed1) = strip(left, changed)
               (OrType(tp1, right), changed1)
@@ -446,13 +448,13 @@ object Types {
      */
     def stripNull(implicit ctx: Context): Type = {
       assert(ctx.explicitNulls)
-      stripNullWithPred(_.isNullType)
+      stripNullImpl(onlyJavaNull = false)
     }
 
     /** Like `stripNull`, but removes only the `JavaNull`s. */
     def stripJavaNull(implicit ctx: Context): Type = {
       assert(ctx.explicitNulls)
-      stripNullWithPred(_.isJavaNullType)
+      stripNullImpl(onlyJavaNull = true)
     }
 
     /** Collapses all `JavaNull` unions within this type, and not just the outermost ones (as `stripJavaNull` does).
@@ -2028,19 +2030,15 @@ object Types {
             // If the denotation is computed for the first time, or if it's ever updated, make sure
             // that the `info` is non-null.
             d.mapInfo(_.stripNull)
-          } else {
-            d
-          }
+          } else d
           // Avoid storing NoDenotations in the cache - we will not be able to recover from
           // them. The situation might arise that a type has NoDenotation in some later
           // phase but a defined denotation earlier (e.g. a TypeRef to an abstract type
           // is undefined after erasure.) We need to be able to do time travel back and
           // forth also in these cases.
           setDenot(d1)
           d1
-        } else {
-          d
-        }
+        } else d
       }
 
       def fromDesignator = designator match {
diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
@@ -270,9 +270,7 @@ class ClassfileParser(
         if ((denot is Flags.Method) && (jflags & JAVA_ACC_VARARGS) != 0)
           denot.info = arrayToRepeated(denot.info)
 
-        if (ctx.explicitNulls) {
-          denot.info = JavaNullInterop.nullifyMember(denot.symbol, denot.info)
-        }
+        if (ctx.explicitNulls) denot.info = JavaNullInterop.nullifyMember(denot.symbol, denot.info)
 
         // seal java enums
         if (isEnum) {
diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala
@@ -837,18 +837,16 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
         }
 
         // Separate into a function so we can pass the updated context.
-        def proc(implicit ctx: Context): tpd.Tree = {
-          methPart(fun1).tpe match {
-            case funRef: TermRef =>
-              val app =
-                if (proto.allArgTypesAreCurrent())
-                  new ApplyToTyped(tree, fun1, funRef, proto.unforcedTypedArgs, pt)
-                else
-                  new ApplyToUntyped(tree, fun1, funRef, proto, pt)(argCtx(tree))
-              convertNewGenericArray(app.result)
-            case _ =>
-              handleUnexpectedFunType(tree, fun1)
-          }
+        def proc(implicit ctx: Context): tpd.Tree = methPart(fun1).tpe match {
+          case funRef: TermRef =>
+            val app =
+              if (proto.allArgTypesAreCurrent())
+                new ApplyToTyped(tree, fun1, funRef, proto.unforcedTypedArgs, pt)
+              else
+                new ApplyToUntyped(tree, fun1, funRef, proto, pt)(argCtx(tree))
+            convertNewGenericArray(app.result)
+          case _ =>
+            handleUnexpectedFunType(tree, fun1)
         }
 
         proc(ctx1)
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -418,11 +418,7 @@ class Typer extends Namer
       } else
         errorType(new MissingIdent(tree, kind, name.show), tree.sourcePos)
 
-    val ownType1 = if (ctx.explicitNulls) {
-      FlowTyper.refineType(ownType)
-    } else {
-      ownType
-    }
+    val ownType1 = if (ctx.explicitNulls) FlowTyper.refineType(ownType) else ownType
 
     val tree1 = ownType1 match {
       case ownType1: NamedType if !prefixIsElidable(ownType1) =>
diff --git a/tests/explicit-nulls/pos/interop-tostring.scala b/tests/explicit-nulls/pos/interop-tostring.scala
@@ -0,0 +1,9 @@
+// Test that `toString` has been special-cased to
+// return a non-nullable value.
+
+class Foo {
+  val x: java.lang.Integer = 42
+  val y: String = x.toString // would fail if toString returns nullable value
+  val y2 = x.toString // test interaction with type inference
+  val z: String = y2
+}

Original file line number	Diff line number	Diff line change
`@@ -207,7 +207,6 @@ object FlowTyper {`
`207`	`207`	`* 1) a return`
`208`	`208`	* 2) an expression of type `Nothing` (in particular, usages of `throw`)
`209`	`209`	`* 3) a block where the last expression is non-local`
`210`		`- * 4) nothing else is non-local`
`211`	`210`	`*`
`212`	`211`	* So, for example, if we know that `x` must be non-null if `cond` is true, and `else` is non-local,
`213`	`212`	* then in order for `s2` to execute `cond` must be true. We can thus soundly add `x` to our