Merge pull request scala#26 from noti0na1/dotty-explicit-nulls

noti0na1 · web-flow · commit 0a54fe6cc415 · 2019-09-27T13:01:41.000-07:00
* ValDefInBlockCompleter is replaced by general Completer
* Let flow typing work for all definitions (DefDef and ValDef) in methods
* Disable flow typing when forward references exist
* Modify tests
diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala
@@ -376,17 +376,6 @@ class Namer { typer: Typer =>
           case tree: TypeDef =>
             if (flags.is(Opaque) && ctx.owner.isClass) ctx.owner.setFlag(Opaque)
             new TypeDefCompleter(tree)(cctx)
-          case tree: ValDef if ctx.explicitNulls && ctx.owner.is(Method) && !tree.mods.isOneOf(Lazy | Implicit) =>
-            // Use a completer that completes itself with the completion text, so
-            // we can use flow typing for `ValDef`s.
-            // Don't use this special kind of completer for `lazy` or `implicit` symbols,
-            // because those could be completed through a forward reference:
-            // e.g.
-            //   val x: String|Null = ???
-            //   y /* y is completed through a forward reference! */
-            //   if (x == null) throw new NullPointerException()
-            //   lazy val y: Int = x.length
-            new ValDefInBlockCompleter(tree)(cctx)
           case _ =>
             new Completer(tree)(cctx)
         }
@@ -767,23 +756,22 @@ class Namer { typer: Typer =>
 
     protected def localContext(owner: Symbol): FreshContext = ctx.fresh.setOwner(owner).setTree(original)
 
-    /** The context in which the completer should be typed: usually it's the creation context,
-     *  but could also be the completion context.
-     */
-    protected def typingContext(owner: Symbol, completionCtx: Context): FreshContext = localContext(owner)
-
     /** The context with which this completer was created */
     def creationContext: Context = ctx
     ctx.typerState.markShared()
 
-    protected def typeSig(sym: Symbol, completionCtx: Context): Type = original match {
+    protected def typeSig(sym: Symbol, ctx: Context): Type = original match {
       case original: ValDef =>
         if (sym.is(Module)) moduleValSig(sym)
-        else valOrDefDefSig(original, sym, Nil, Nil, identity)(typingContext(sym, completionCtx).setNewScope)
+        else {
+          val withNewScope = ctx.fresh.setOwner(sym).setTree(original).setNewScope
+          valOrDefDefSig(original, sym, Nil, Nil, identity)(withNewScope)
+        }
       case original: DefDef =>
         val typer1 = ctx.typer.newLikeThis
         nestedTyper(sym) = typer1
-        typer1.defDefSig(original, sym)(localContext(sym).setTyper(typer1))
+        val withTyper = ctx.fresh.setOwner(sym).setTree(original).setTyper(typer1)
+        typer1.defDefSig(original, sym)(withTyper)
       case imp: Import =>
         try {
           val expr1 = typedAheadExpr(imp.expr, AnySelectionProto)
@@ -810,7 +798,8 @@ class Namer { typer: Typer =>
         denot.info = UnspecifiedErrorType
       }
       else {
-        completeInContext(denot, ctx)
+        // the default behaviour is to complete using creation context
+        completeInContext(denot, this.ctx)
         if (denot.isCompleted) registerIfChild(denot)
       }
     }
@@ -893,49 +882,24 @@ class Namer { typer: Typer =>
       }
     }
 
-    /** Intentionally left without `implicit ctx` parameter. We need
-     *  to pick up the context at the point where the completer was created.
+    /** Intentionally left without `implicit ctx` parameter.
+     *  We use the given ctx to complete this Completer.
      *
-     *  If -Yexplicit-nulls, is enabled, we sometimes use the completion context.
-     *  See `ValDefInBlockCompleter`.
+     *  Normally, the creation context is passed, as passed in the complete method.
+     *  However, if -Yexplicit-nulls, is enabled, we pass in the current context
+     *  so that flow typing works with blocks.
      */
-    def completeInContext(denot: SymDenotation, completionContext: Context): Unit = {
+    def completeInContext(denot: SymDenotation, ctx: Context): Unit = {
       val sym = denot.symbol
       addAnnotations(sym)
       addInlineInfo(sym)
-      denot.info = typeSig(sym, completionContext)
+      denot.info = typeSig(sym, ctx)
       invalidateIfClashingSynthetic(denot)
       Checking.checkWellFormed(sym)
       denot.info = avoidPrivateLeaks(sym)
     }
   }
 
-  /** A completer that uses its completion context (as opposed to the creation context)
-   *  to complete itself. This is used so that flow typing can handle `ValDef`s that appear within a block.
-   *
-   *  Example:
-   *  Suppose we have a block containing
-   *
-   *    1. val x: String|Null = ???
-   *    2. if (x == null) throw NPE
-   *    3. val y = x
-   *
-   *  We want to infer y: String on line 3, but if the completer for `y` uses its creation context,
-   *  then we won't have the additional flow facts that say that `y` is not null.
-   *
-   *  The solution is to use a completer that completes itself using the context at completion time,
-   *  as opposed to creation time. Normally, we need to use the creation context, because a completer
-   *  can be completed at any point in the future. However, for `ValDef`s within a block, we know they'll
-   *  be completed immediately after the symbols are created, so it's safe to use this new kind of completer.
-   */
-  class ValDefInBlockCompleter(original: ValDef)(implicit ctx: Context) extends Completer(original)(ctx) {
-    assert(ctx.explicitNulls)
-
-    override def typingContext(owner: Symbol, completionCtx: Context): FreshContext = {
-      completionCtx.fresh.setOwner(owner).setTree(original)
-    }
-  }
-
   class TypeDefCompleter(original: TypeDef)(ictx: Context) extends Completer(original)(ictx) with TypeParamsCompleter {
     private[this] var myTypeParams: List[TypeSymbol] = null
     private[this] var nestedCtx: Context = null
@@ -964,7 +928,7 @@ class Namer { typer: Typer =>
       myTypeParams
     }
 
-    override protected def typeSig(sym: Symbol, completionContext: Context): Type =
+    override protected def typeSig(sym: Symbol, ctx: Context): Type =
       typeDefSig(original, sym, completerTypeParams(sym)(ictx))(nestedCtx)
   }
 
@@ -1134,7 +1098,7 @@ class Namer { typer: Typer =>
     }
 
     /** The type signature of a ClassDef with given symbol */
-    override def completeInContext(denot: SymDenotation, completionContext: Context): Unit = {
+    override def completeInContext(denot: SymDenotation, ctx: Context): Unit = {
       val parents = impl.parents
 
       /* The type of a parent constructor. Types constructor arguments
@@ -1170,26 +1134,26 @@ class Namer { typer: Typer =>
        *  (4) If the class is sealed, it is defined in the same compilation unit as the current class
        */
       def checkedParentType(parent: untpd.Tree): Type = {
-        val ptype = parentType(parent)(ctx.superCallContext).dealiasKeepAnnots
+        val ptype = parentType(parent)(this.ctx.superCallContext).dealiasKeepAnnots
         if (cls.isRefinementClass) ptype
         else {
           val pt = checkClassType(ptype, parent.sourcePos,
               traitReq = parent ne parents.head, stablePrefixReq = true)
           if (pt.derivesFrom(cls)) {
             val addendum = parent match {
-              case Select(qual: Super, _) if ctx.scala2Mode =>
+              case Select(qual: Super, _) if this.ctx.scala2Mode =>
                 "\n(Note that inheriting a class of the same name is no longer allowed)"
               case _ => ""
             }
-            ctx.error(CyclicInheritance(cls, addendum), parent.sourcePos)
+            this.ctx.error(CyclicInheritance(cls, addendum), parent.sourcePos)
             defn.ObjectType
           }
           else {
             val pclazz = pt.typeSymbol
             if (pclazz.is(Final))
-              ctx.error(ExtendFinalClass(cls, pclazz), cls.sourcePos)
+              this.ctx.error(ExtendFinalClass(cls, pclazz), cls.sourcePos)
             if (pclazz.is(Sealed) && pclazz.associatedFile != cls.associatedFile)
-              ctx.error(UnableToExtendSealedClass(pclazz), cls.sourcePos)
+              this.ctx.error(UnableToExtendSealedClass(pclazz), cls.sourcePos)
             pt
           }
         }
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -2211,16 +2211,38 @@ class Typer extends Namer
             traverse(xtree :: rest, facts)
           case none =>
             import untpd.modsDeco
-            val ctx1 = if (ctx.explicitNulls) {
-              mdef match {
-                case mdef: untpd.ValDef if ctx.owner.is(Method) && !mdef.mods.isOneOf(Lazy | Implicit) =>
-                  ctx.fresh.addFlowFacts(facts)
-                case _ => ctx
+            // Force completer to use calling context (as opposed to the creation context)
+            // to complete itself. This approach is used so that flow typing can handle definitions
+            // appearing within a block.
+            //
+            // Example:
+            // Suppose we have a block containing
+            // 1. val x: String|Null = ???
+            // 2. if (x == null) throw NPE
+            // 3. val y = x
+            //
+            // We want to infer y: String on line 3, but if the completer for `y` uses its creation
+            // context, then we won't have the additional flow facts that say that `y` is not null.
+            //
+            // The solution is to use the context containing more information about the statements above.
+            val ctx2 = if (ctx.explicitNulls && ctx.owner.is(Method)) {
+              // We cannot use mdef.symbol to get the symbol of the tree here,
+              // since the tree has not been completed and doesn't have a denotation.
+              mdef.getAttachment(SymOfTree).map(s => (s, s.infoOrCompleter)) match {
+                case Some((sym, completer: Namer#Completer)) =>
+                  val ctx1 = ctx.fresh.addFlowFacts(facts)
+                  completer.completeInContext(sym, ctx1)
+                  ctx1
+                case _ =>
+                  // If it has been completed, then it must be because there is a forward reference
+                  // to the definition in the program. We use the default (creation) context, and flow
+                  // typing will not be applied.
+                  ctx
               }
-            } else {
-              ctx
             }
-            typed(mdef)(ctx1) match {
+            else ctx
+
+            typed(mdef)(ctx2) match {
               case mdef1: DefDef if !Inliner.bodyToInline(mdef1.symbol).isEmpty =>
                 buf += inlineExpansion(mdef1)
                   // replace body with expansion, because it will be used as inlined body
diff --git a/tests/explicit-nulls/neg/flow6.scala b/tests/explicit-nulls/neg/flow6.scala
@@ -1,70 +1,50 @@
-
-// Test that flow inference behaves soundly within blocks.
-// This means that flow facts are propagated to ValDefs, but not
-// to DefDefs or lazy vals or implicit vals.
-// The reason is that forward references are allowed for `defs`,
-// so when we see
-// ```
-// f
-// if (x == null) return "foo"
-// def f = x.length
-// ```
-// we can't assume that f is executed _after_ the if statement
-// is executed.
+// Test forward references handled with flow typing
+// Currently, the flow typing will not be applied to definitions forwardly referred.
 class Foo {
 
   def test1(): Unit = {
     val x: String|Null = ???
     if (x == null) return ()
-    val y = x.length // ok: x: String inferred
+    def y = x.length // ok: x: String inferred
     ()
   }
 
+  // This test is similar to test1, but a forward DefDef referring
+  // to y is added after x. y is completed before knowing the
+  // fact "x != null", hence, the type x: String|Null is used.
   def test2(): Unit = {
     val x: String|Null = ???
+    def z = y
     if (x == null) return ()
-    def y = x.length // error: x: String|Null inferred
+    def y = x.length // error: x: String|Null is inferred
     ()
   }
 
+  // Since y is referred before definition, flow typing is not used here.
   def test3(): Unit = {
     val x: String|Null = ???
+    lazy val z = y
     if (x == null) return ()
-    lazy val y = x.length // error: x: String|Null inferred
+    lazy val y = x.length // error: x: String|Null is inferred
     ()
   }
 
+  // This case is invalid because z has an implicit forward reference to y,
+  // but x, y and z aren't lazy (only forward references to lazy vals are allowed).
+  // Since y is referred (by z) before definition, flow typing is not used here.
+  // Only the typing error is shown because reference check is after typing.
   def test4(): Unit = {
+    val z = implicitly[Int]
     val x: String|Null = ???
     if (x == null) return ()
     implicit val y: Int = x.length // error: x: String|Null inferred
   }
 
-  // This case is different from #3 because the type of y doesn't need
-  // to be inferred, which triggers a different codepath within the completer.
+  // Since z is referred before definition, flow typing is not used here.
   def test5(): Unit = {
     val x: String|Null = ???
     if (x == null) return ()
-    lazy val y: Int = x.length // error: x: String|Null inferred
-    ()
-  }
-
-  def test6(): Unit = {
-    val x: String|Null = ???
-    if (x == null) return ()
-    def y: Int = x.length // error: x: String|Null inferred
-    ()
-  }
-
-  // This test checks that flow facts are forgotten for defs, but only
-  // the facts gathered within the current block are forgotten.
-  // Other facts from outer blocks are remembered.
-  def test7(): Unit = {
-    val x: String|Null = ???
-    if (x == null) {
-    } else {
-      def f = x.length       // ok
-      def f2: Int = x.length // ok
-    }
+    def y = z
+    def z = x.length // error: x: String|Null inferred
   }
-}
+}
diff --git a/tests/explicit-nulls/pos/flow6.scala b/tests/explicit-nulls/pos/flow6.scala
@@ -0,0 +1,74 @@
+
+// Test that flow inference behaves soundly within blocks.
+// This means that flow facts are propagated to all ValDef and DefDef.
+class Foo {
+
+  def test1(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    val y = x.length // ok: x: String inferred
+    ()
+  }
+
+  def test2(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    lazy val y = x.length // ok: x: String inferred
+    ()
+  }
+
+  def test3(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    implicit val y = x.length // ok: x: String inferred
+    ()
+  }
+
+  def test4(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    def y = x.length // ok: x: String inferred
+    ()
+  }
+
+  // This case is different from #3 because the type of y doesn't need
+  // to be inferred, which triggers a different codepath within the completer.
+  def test5(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    implicit val y: Int = x.length // ok: x: String inferred
+  }
+
+  def test6(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    lazy val y: Int = x.length // ok: x: String inferred
+    ()
+  }
+
+  def test7(): Unit = {
+    val x: String|Null = ???
+    if (x == null) return ()
+    def y: Int = x.length // ok: x: String inferred
+    ()
+  }
+
+  def test8(): Unit = {
+    lazy val x: String|Null = ???
+    if (x == null) return ()
+    val y = x.length // ok: x: String inferred
+    ()
+  }
+
+  // This test checks that flow facts are forgotten for defs, but only
+  // the facts gathered within the current block are forgotten.
+  // Other facts from outer blocks are remembered.
+  def test9(): Unit = {
+    val x: String|Null = ???
+    if (x == null) {
+    } else {
+      def f = x.length       // ok
+      def f2: Int = x.length // ok
+    }
+  }
+}
