Refine isParametric tests

odersky · odersky · commit d4c084c4269a · 2023-11-06T18:42:14.000+01:00
Mutable variables can appeal to parametricty only if they are not captured.
We use "not captured by any closure" as a sound approximation for that, since
variables themselves are currently not tracked, so we cannot use soemthing
more finegrained.
diff --git a/compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala b/compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala
@@ -16,7 +16,7 @@ import typer.ErrorReporting.{Addenda, err}
 import typer.ProtoTypes.{AnySelectionProto, LhsProto}
 import util.{SimpleIdentitySet, EqHashMap, EqHashSet, SrcPos, Property}
 import transform.SymUtils.*
-import transform.{Recheck, PreRecheck}
+import transform.{Recheck, PreRecheck, CapturedVars}
 import Recheck.*
 import scala.collection.mutable
 import CaptureSet.{withCaptureSetsExplained, IdempotentCaptRefMap, CompareResult}
@@ -149,15 +149,25 @@ object CheckCaptures:
 
       private val seen = new EqHashSet[TypeRef]
 
+      /** Check that there is at least one method containing carrier and defined
+       *  in the scope of tparam. E.g. this is OK:
+       *    def f[T] = { ... var x: T ... }
+       *  So is this:
+       *    class C[T] { def f() = { class D { var x: T }}}
+       *  But this is not OK:
+       *    class C[T] { object o { var x: T }}
+       */
       extension (tparam: Symbol) def isParametricIn(carrier: Symbol): Boolean =
-        val encl = carrier.maybeOwner.enclosingMethodOrClass
-        if encl.isClass then tparam.isParametricIn(encl)
-        else
-          def recur(encl: Symbol): Boolean =
-            if tparam.owner == encl then true
-            else if encl.isStatic || !encl.exists then false
-            else recur(encl.owner.enclosingMethodOrClass)
-          recur(encl)
+        carrier.exists && {
+          val encl = carrier.owner.enclosingMethodOrClass
+          if encl.isClass then tparam.isParametricIn(encl)
+          else
+            def recur(encl: Symbol): Boolean =
+              if tparam.owner == encl then true
+              else if encl.isStatic || !encl.exists then false
+              else recur(encl.owner.enclosingMethodOrClass)
+            recur(encl)
+        }
 
       def traverse(t: Type) =
         t.dealiasKeepAnnots match
@@ -168,9 +178,12 @@ object CheckCaptures:
               t.info match
                 case TypeBounds(_, hi) if !t.isSealed && !t.symbol.isParametricIn(carrier) =>
                   if hi.isAny then
+                    val detailStr =
+                      if t eq tp then "variable"
+                      else i"refers to the type variable $t, which"
                     report.error(
                       em"""$what cannot $have $tp since
-                          |that type refers to the type variable $t, which is not sealed.
+                          |that type $detailStr is not sealed.
                           |$addendum""",
                       pos)
                   else
@@ -549,7 +562,7 @@ class CheckCaptures extends Recheck, SymTransformer:
         for case (arg: TypeTree, formal, pname) <- args.lazyZip(polyType.paramRefs).lazyZip((polyType.paramNames)) do
           if formal.isSealed then
             def where = if fn.symbol.exists then i" in an argument of ${fn.symbol}" else ""
-            disallowRootCapabilitiesIn(arg.knownType, fn.symbol,
+            disallowRootCapabilitiesIn(arg.knownType, NoSymbol,
               i"Sealed type variable $pname", "be instantiated to",
               i"This is often caused by a local capability$where\nleaking as part of its result.",
               tree.srcPos)
@@ -590,13 +603,58 @@ class CheckCaptures extends Recheck, SymTransformer:
         openClosures = openClosures.tail
     end recheckClosureBlock
 
+    /** Maps mutable variables to the symbols that capture them (in the
+     *  CheckCaptures sense, i.e. symbol is referred to from a different method
+     *  than the one it is defined in).
+     */
+    private val capturedBy = util.HashMap[Symbol, Symbol]()
+
+    /** Maps anonymous functions appearing as function arguments to
+     *  the function that is called.
+     */
+    private val anonFunCallee = util.HashMap[Symbol, Symbol]()
+
+    /** Populates `capturedBy` and `anonFunCallee`. Called by `checkUnit`.
+     */
+    private def collectCapturedMutVars(using Context) = new TreeTraverser:
+      def traverse(tree: Tree)(using Context) = tree match
+        case id: Ident =>
+          val sym = id.symbol
+          if sym.is(Mutable, butNot = Method) && sym.owner.isTerm then
+            val enclMeth = ctx.owner.enclosingMethod
+            if sym.enclosingMethod != enclMeth then
+              capturedBy(sym) = enclMeth
+        case Apply(fn, args) =>
+          for case closureDef(mdef) <- args do
+            anonFunCallee(mdef.symbol) = fn.symbol
+          traverseChildren(tree)
+        case Inlined(_, bindings, expansion) =>
+          traverse(bindings)
+          traverse(expansion)
+        case mdef: DefDef =>
+          if !mdef.symbol.isInlineMethod then traverseChildren(tree)
+        case _ =>
+          traverseChildren(tree)
+
     override def recheckValDef(tree: ValDef, sym: Symbol)(using Context): Type =
       try
         if sym.is(Module) then sym.info // Modules are checked by checking the module class
         else
           if sym.is(Mutable) && !sym.hasAnnotation(defn.UncheckedCapturesAnnot) then
-            disallowRootCapabilitiesIn(tree.tpt.knownType, sym,
-              i"mutable $sym", "have type", "", sym.srcPos)
+            val (carrier, addendum) = capturedBy.get(sym) match
+              case Some(encl) =>
+                val enclStr =
+                  if encl.isAnonymousFunction then
+                    val location = anonFunCallee.get(encl) match
+                      case Some(meth) if meth.exists => i" argument in a call to $meth"
+                      case _ => ""
+                    s"an anonymous function$location"
+                  else encl.show
+                (NoSymbol, i"\nNote that $sym does not count as local since it is captured by $enclStr")
+              case _ =>
+                (sym, "")
+            disallowRootCapabilitiesIn(
+              tree.tpt.knownType, carrier, i"Mutable $sym", "have type", addendum, sym.srcPos)
           checkInferredResult(super.recheckValDef(tree, sym), tree)
       finally
         if !sym.is(Param) then
@@ -1168,11 +1226,12 @@ class CheckCaptures extends Recheck, SymTransformer:
     private val setup: SetupAPI = thisPhase.prev.asInstanceOf[Setup]
 
     override def checkUnit(unit: CompilationUnit)(using Context): Unit =
-      setup.setupUnit(ctx.compilationUnit.tpdTree, completeDef)
+      setup.setupUnit(unit.tpdTree, completeDef)
+      collectCapturedMutVars.traverse(unit.tpdTree)
 
       if ctx.settings.YccPrintSetup.value then
         val echoHeader = "[[syntax tree at end of cc setup]]"
-        val treeString = show(ctx.compilationUnit.tpdTree)
+        val treeString = show(unit.tpdTree)
         report.echo(s"$echoHeader\n$treeString\n")
 
       withCaptureSetsExplained:
diff --git a/tests/neg-custom-args/captures/buffers.check b/tests/neg-custom-args/captures/buffers.check
@@ -1,7 +1,7 @@
 -- Error: tests/neg-custom-args/captures/buffers.scala:11:6 ------------------------------------------------------------
 11 |  var elems: Array[A] = new Array[A](10) // error // error
    |      ^
-   |      mutable variable elems cannot have type Array[A] since
+   |      Mutable variable elems cannot have type Array[A] since
    |      that type refers to the type variable A, which is not sealed.
 -- Error: tests/neg-custom-args/captures/buffers.scala:16:38 -----------------------------------------------------------
 16 |  def make[A: ClassTag](xs: A*) = new ArrayBuffer: // error
@@ -14,13 +14,13 @@
 11 |  var elems: Array[A] = new Array[A](10) // error // error
    |             ^^^^^^^^
    |             Array cannot have element type A since
-   |             that type refers to the type variable A, which is not sealed.
+   |             that type variable is not sealed.
    |             Since arrays are mutable, they have to be treated like variables,
    |             so their element type must be sealed.
 -- Error: tests/neg-custom-args/captures/buffers.scala:22:9 ------------------------------------------------------------
 22 |  val x: Array[A] = new Array[A](10) // error
    |         ^^^^^^^^
    |         Array cannot have element type A since
-   |         that type refers to the type variable A, which is not sealed.
+   |         that type variable is not sealed.
    |         Since arrays are mutable, they have to be treated like variables,
    |         so their element type must be sealed.
diff --git a/tests/neg-custom-args/captures/levels.check b/tests/neg-custom-args/captures/levels.check
@@ -1,8 +1,8 @@
 -- Error: tests/neg-custom-args/captures/levels.scala:6:16 -------------------------------------------------------------
 6 |    private var v: T = init  // error
   |                ^
-  |                mutable variable v cannot have type T since
-  |                that type refers to the type variable T, which is not sealed.
+  |                Mutable variable v cannot have type T since
+  |                that type variable is not sealed.
 -- Error: tests/neg-custom-args/captures/levels.scala:17:13 ------------------------------------------------------------
 17 |  val _ = Ref[String => String]((x: String) => x) // error
    |          ^^^^^^^^^^^^^^^^^^^^^
diff --git a/tests/neg-custom-args/captures/sealed-leaks.check b/tests/neg-custom-args/captures/sealed-leaks.check
@@ -0,0 +1,50 @@
+-- [E129] Potential Issue Warning: tests/neg-custom-args/captures/sealed-leaks.scala:31:6 ------------------------------
+31 |      ()
+   |      ^^
+   |      A pure expression does nothing in statement position
+   |
+   | longer explanation available when compiling with `-explain`
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:12:27 ------------------------------------------------------
+12 |  val later2 = usingLogFile[(() => Unit) | Null] { f => () => f.write(0) } // error
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |               Sealed type variable T cannot be instantiated to (() => Unit) | Null since
+   |               that type captures the root capability `cap`.
+   |               This is often caused by a local capability in an argument of method usingLogFile
+   |               leaking as part of its result.
+-- [E007] Type Mismatch Error: tests/neg-custom-args/captures/sealed-leaks.scala:19:26 ---------------------------------
+19 |  usingLogFile { f => x = f }  // error
+   |                          ^
+   |                          Found:    (f : java.io.FileOutputStream^)
+   |                          Required: (java.io.FileOutputStream | Null)^{cap[Test2]}
+   |
+   | longer explanation available when compiling with `-explain`
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:30:10 ------------------------------------------------------
+30 |      var x: T = y    // error
+   |          ^
+   |          Mutable variable x cannot have type T since
+   |          that type variable is not sealed.
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:39:8 -------------------------------------------------------
+39 |    var x: T = y     // error
+   |        ^
+   |        Mutable variable x cannot have type T since
+   |        that type variable is not sealed.
+   |
+   |        Note that variable x does not count as local since it is captured by an anonymous function
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:43:8 -------------------------------------------------------
+43 |    var x: T = y     // error
+   |        ^
+   |Mutable variable x cannot have type T since
+   |that type variable is not sealed.
+   |
+   |Note that variable x does not count as local since it is captured by an anonymous function argument in a call to method identity
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:47:8 -------------------------------------------------------
+47 |    var x: T = y     // error
+   |        ^
+   |        Mutable variable x cannot have type T since
+   |        that type variable is not sealed.
+   |
+   |        Note that variable x does not count as local since it is captured by method foo
+-- Error: tests/neg-custom-args/captures/sealed-leaks.scala:11:14 ------------------------------------------------------
+11 |  val later = usingLogFile { f => () => f.write(0) } // error
+   |              ^^^^^^^^^^^^
+   |              local reference f leaks into outer capture set of type parameter T of method usingLogFile
diff --git a/tests/neg-custom-args/captures/sealed-leaks.scala b/tests/neg-custom-args/captures/sealed-leaks.scala
@@ -18,4 +18,34 @@ def Test2 =
 
   usingLogFile { f => x = f }  // error
 
-  later()
+  later()
+
+def Test3 =
+  def f[T](y: T) =
+    var x: T = y
+    ()
+
+  class C[T](y: T):
+    object o:
+      var x: T = y    // error
+      ()
+
+  class C2[T](y: T):
+    def f =
+      var x: T = y    // ok
+      ()
+
+  def g1[T](y: T): T => Unit =
+    var x: T = y     // error
+    y => x = y
+
+  def g2[T](y: T): T => Unit =
+    var x: T = y     // error
+    identity(y => x = y)
+
+  def g3[T](y: T): Unit =
+    var x: T = y     // error
+    def foo =
+      x = y
+    ()
+
