Fix hole for recomputing denotations of NamedTypes

To recompute a denotation of a NamedType in a new phase we have a subtle logic
that sometimes recomputes the denotation from the prefix and the symbol or name,
and at other times recomputes it by transforming the previous denotation with
`current`. This logic was wrong for capturing types. We sometimes recomputed
by transforming the previous denotation where this led to wrong denotation.

We "fixed" problems this caused before by sometimes doing an explicit
`recomputeDenot`. But that's unsatisfactory since we can never know whether we do
that in all cases where it's needed.

This commit is a more principled fix where fix the logic that determines how
to recompute a NamedType's denotation by consulting sometimes the rechecker's advice.

 recomputed by transforming the previous denotation where this
led

Author: odersky

compiler/src/dotty/tools/dotc/cc/CaptureAnnotation.scala

@@ -62,14 +62,19 @@ case class CaptureAnnotation(refs: CaptureSet, boxed: Boolean)(cls: Symbol) exte
     case _ => false
 
   override def mapWith(tm: TypeMap)(using Context) =
-    val elems = refs.elems.toList
-    val elems1 = elems.mapConserve(tm.mapCapability(_))
-    if elems1 eq elems then this
-    else if elems1.forall:
-      case elem1: Capability => elem1.isWellformed
-      case _ => false
-    then derivedAnnotation(CaptureSet(elems1.asInstanceOf[List[Capability]]*), boxed)
-    else EmptyAnnotation
+    if ctx.phase.id > Phases.checkCapturesPhase.id then
+      // Annotation is no longer relevant, can be dropped.
+      // This avoids running into illegal states in mapCapability.
+      EmptyAnnotation
+    else
+      val elems = refs.elems.toList
+      val elems1 = elems.mapConserve(tm.mapCapability(_))
+      if elems1 eq elems then this
+      else if elems1.forall:
+        case elem1: Capability => elem1.isWellformed
+        case _ => false
+      then derivedAnnotation(CaptureSet(elems1.asInstanceOf[List[Capability]]*), boxed)
+      else EmptyAnnotation
 
   override def refersToParamOf(tl: TermLambda)(using Context): Boolean =
     refs.elems.exists {

compiler/src/dotty/tools/dotc/cc/CaptureSet.scala

@@ -1573,10 +1573,6 @@ object CaptureSet:
   /** The capture set of the type underlying the capability `c` */
   def ofInfo(c: Capability)(using Context): CaptureSet = c match
     case Reach(c1) =>
-      c1 match
-        case c1: TermRef => c1.safeRecomputeDenot()
-          // following widen would be wrong otherwise
-        case _ =>
       c1.widen.computeDeepCaptureSet(includeTypevars = true)
         .showing(i"Deep capture set of $c: ${c1.widen} = ${result}", capt)
     case Restricted(c1, cls) =>

compiler/src/dotty/tools/dotc/cc/CheckCaptures.scala

@@ -246,6 +246,29 @@ class CheckCaptures extends Recheck, SymTransformer:
 
   override def isRunnable(using Context) = super.isRunnable && Feature.ccEnabledSomewhere
 
+  /** We normally need a recompute if the prefix is a SingletonType and the
+   *  last denotation is not a SymDenotation. The SingletonType requirement is
+   *  so that we don't widen TermRefs with non-path prefixes to their underlying
+   *  type when recomputing their denotations with asSeenFrom. Such widened types
+   *  would become illegal members of capture sets.
+   *
+   *  The SymDenotation requirement is so that we don't recompute termRefs of Symbols
+   *  which should be handled by SymTransformers alone. However, if the underlying type
+   *  of the prefix is a capturing type, we do need to recompute since in that case
+   *  the prefix might carry a parameter refinement created in Setup, and we need to
+   *  take these refinements into account.
+   */
+  override def needsRecompute(tp: NamedType, lastDenotation: SingleDenotation)(using Context): Boolean =
+    tp.prefix match
+      case prefix: TermRef =>
+        prefix.info match
+          case CapturingType(_, _) => true
+          case _ => !lastDenotation.isInstanceOf[SymDenotation]
+      case prefix: SingletonType =>
+        !lastDenotation.isInstanceOf[SymDenotation]
+      case _ =>
+        false
+
   def newRechecker()(using Context) = CaptureChecker(ctx)
 
   override def run(using Context): Unit =
@@ -1087,11 +1110,11 @@ class CheckCaptures extends Recheck, SymTransformer:
         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
-            val addendum = capturedBy.get(sym) match
+            val addendum = setup.capturedBy.get(sym) match
               case Some(encl) =>
                 val enclStr =
                   if encl.isAnonymousFunction then
-                    val location = anonFunCallee.get(encl) match
+                    val location = setup.anonFunCallee.get(encl) match
                       case Some(meth) if meth.exists => i" argument in a call to $meth"
                       case _ => ""
                     s"an anonymous function$location"
@@ -1907,49 +1930,12 @@ class CheckCaptures extends Recheck, SymTransformer:
         traverseChildren(t)
     end checkOverrides
 
-    /** Used for error reporting:
-     *  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]()
-
-    /** Used for error reporting:
-     *  Maps anonymous functions appearing as function arguments to
-     *  the function that is called.
-     */
-    private val anonFunCallee = util.HashMap[Symbol, Symbol]()
-
-    /** Used for error reporting:
-     *  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.isMutableVar && 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)
-
     private val setup: SetupAPI = thisPhase.prev.asInstanceOf[Setup]
 
     override def checkUnit(unit: CompilationUnit)(using Context): Unit =
       capt.println(i"cc check ${unit.source}")
       ccState.start()
       setup.setupUnit(unit.tpdTree, this)
-      collectCapturedMutVars.traverse(unit.tpdTree)
 
       if ctx.settings.YccPrintSetup.value then
         val echoHeader = "[[syntax tree at end of cc setup]]"

compiler/src/dotty/tools/dotc/cc/Setup.scala

@@ -40,6 +40,20 @@ trait SetupAPI:
   /** Check to do after the capture checking traversal */
   def postCheck()(using Context): Unit
 
+  /** Used for error reporting:
+   *  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).
+   */
+  def capturedBy: collection.Map[Symbol, Symbol]
+
+  /** Used for error reporting:
+   *  Maps anonymous functions appearing as function arguments to
+   *  the function that is called.
+   */
+  def anonFunCallee: collection.Map[Symbol, Symbol]
+end SetupAPI
+
 object Setup:
 
   val name: String = "setupCC"
@@ -889,11 +903,40 @@ class Setup extends PreRecheck, SymTransformer, SetupAPI:
         else t
       case _ => mapFollowingAliases(t)
 
+  val capturedBy: mutable.HashMap[Symbol, Symbol] = mutable.HashMap[Symbol, Symbol]()
+
+  val anonFunCallee: mutable.HashMap[Symbol, Symbol] = mutable.HashMap[Symbol, Symbol]()
+
+  /** Used for error reporting:
+   *  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.isMutableVar && 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)
+
   /** Run setup on a compilation unit with given `tree`.
    *  @param recheckDef   the function to run for completing a val or def
    */
   def setupUnit(tree: Tree, checker: CheckerAPI)(using Context): Unit =
-    setupTraverser(checker).traverse(tree)(using ctx.withPhase(thisPhase))
+    inContext(ctx.withPhase(thisPhase)):
+      setupTraverser(checker).traverse(tree)
+      collectCapturedMutVars.traverse(tree)
 
   // ------ Checks to run at Setup ----------------------------------------
 

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

@@ -135,4 +135,8 @@ object ContextOps:
       if (pkg.is(Package)) ctx.fresh.setOwner(pkg.moduleClass).setTree(tree).setNewScope
       else ctx
     }
+
+    def isRechecking: Boolean =
+      (ctx.base.recheckPhaseIds & (1L << ctx.phaseId)) != 0
+
 end ContextOps

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

@@ -41,6 +41,18 @@ object Phases {
     // drop NoPhase at beginning
     def allPhases: Array[Phase] = (if (fusedPhases.nonEmpty) fusedPhases else phases).tail
 
+    private var myRecheckPhaseIds: Long = 0
+
+    /** A bitset of the ids of the phases extending `transform.Recheck`.
+     *  Recheck phases must have id 63 or less.
+     */
+    def recheckPhaseIds: Long = myRecheckPhaseIds
+
+    def recordRecheckPhase(phase: Recheck): Unit =
+      val id = phase.id
+      assert(id < 64, s"Recheck phase with id $id outside permissible range 0..63")
+      myRecheckPhaseIds |= (1L << id)
+
     object SomePhase extends Phase {
       def phaseName: String = "<some phase>"
       def run(using Context): Unit = unsupported("run")

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

@@ -38,9 +38,11 @@ import config.Printers.{core, typr, matchTypes}
 import reporting.{trace, Message}
 import java.lang.ref.WeakReference
 import compiletime.uninitialized
+import ContextOps.isRechecking
 import cc.*
 import CaptureSet.IdentityCaptRefMap
 import Capabilities.*
+import transform.Recheck.currentRechecker
 
 import scala.annotation.internal.sharable
 import scala.annotation.threadUnsafe
@@ -2509,15 +2511,31 @@ object Types extends TypeUtils {
       lastDenotation match {
         case lastd0: SingleDenotation =>
           val lastd = lastd0.skipRemoved
-          if lastd.validFor.runId == ctx.runId && checkedPeriod.code != NowhereCode then
+          var needsRecompute = false
+          if lastd.validFor.runId == ctx.runId
+              && checkedPeriod.code != NowhereCode
+              && !(ctx.isRechecking
+                    && {
+                      needsRecompute = currentRechecker.needsRecompute(this, lastd)
+                      needsRecompute
+                    }
+                  )
+          then
             finish(lastd.current)
-          else lastd match {
-            case lastd: SymDenotation =>
-              if stillValid(lastd) && checkedPeriod.code != NowhereCode then finish(lastd.current)
-              else finish(memberDenot(lastd.initial.name, allowPrivate = lastd.is(Private)))
-            case _ =>
-              fromDesignator
-          }
+          else
+            val newd = lastd match
+              case lastd: SymDenotation =>
+                if stillValid(lastd) && checkedPeriod.code != NowhereCode && !needsRecompute
+                then finish(lastd.current)
+                else finish(memberDenot(lastd.initial.name, allowPrivate = lastd.is(Private)))
+              case _ =>
+                fromDesignator
+            if needsRecompute && (newd.info ne lastd.info) then
+              // Record the previous denotation, so that it can be reset at the end
+              // of the rechecker phase
+              currentRechecker.prevSelDenots(this) = lastd
+              //println(i"NEW PATH $this: ${newd.info} at ${ctx.phase}, prefix = $prefix")
+            newd
         case _ => fromDesignator
       }
     }
@@ -2601,10 +2619,6 @@ object Types extends TypeUtils {
     def recomputeDenot()(using Context): Unit =
       setDenot(memberDenot(name, allowPrivate = !symbol.exists || symbol.is(Private)))
 
-    def safeRecomputeDenot()(using Context): Unit =
-      val newDenot = memberDenot(name, allowPrivate = !symbol.exists || symbol.is(Private))
-      if newDenot.exists then setDenot(newDenot)
-
     private def setDenot(denot: Denotation)(using Context): Unit = {
       if ctx.base.checkNoDoubleBindings then
         checkSymAssign(denot.symbol)

compiler/src/dotty/tools/dotc/transform/Recheck.scala

@@ -50,6 +50,12 @@ object Recheck:
       case None =>
         tree
 
+  /** The currently running rechecker
+   *  @pre   ctx.isRechecking
+   */
+  def currentRechecker(using Context): Recheck =
+    ctx.phase.asInstanceOf[Recheck]
+
   extension (sym: Symbol)(using Context)
 
     /** Update symbol's info to newInfo after `prevPhase`.
@@ -143,6 +149,7 @@ abstract class Recheck extends Phase, SymTransformer:
     else symd
 
   def run(using Context): Unit =
+    ctx.base.recordRecheckPhase(this)
     val rechecker = newRechecker()
     rechecker.checkUnit(ctx.compilationUnit)
     rechecker.reset()
@@ -151,6 +158,19 @@ abstract class Recheck extends Phase, SymTransformer:
     try super.runOn(units)
     finally preRecheckPhase.pastRecheck = true
 
+  /** A hook to determine whether the denotation of a NamedType should be recomputed
+   *  from its symbol and prefix, instead of just evolving the previous denotation with
+   *  `current`. This should return true if there are complex changes to types that
+   *  are not reflected in `current`.
+   */
+  def needsRecompute(tp: NamedType, lastDenotation: SingleDenotation)(using Context): Boolean =
+    false
+
+  /** A map from NamedTypes to the denotations they had before this phase.
+   *  Needed so that we can `reset` them after this phase.
+   */
+  val prevSelDenots = util.HashMap[NamedType, Denotation]()
+
   def newRechecker()(using Context): Rechecker
 
   /** The typechecker pass */
@@ -192,17 +212,13 @@ abstract class Recheck extends Phase, SymTransformer:
     def resetNuTypes()(using Context): Unit =
       nuTypes.clear(resetToInitial = false)
 
-    /** A map from NamedTypes to the denotations they had before this phase.
-     *  Needed so that we can `reset` them after this phase.
-     */
-    private val prevSelDenots = util.HashMap[NamedType, Denotation]()
-
     /** Reset all references in `prevSelDenots` to the denotations they had
      *  before this phase.
      */
     def reset()(using Context): Unit =
       for (ref, mbr) <- prevSelDenots.iterator do
         ref.withDenot(mbr)
+      prevSelDenots.clear()
 
     /** Constant-folded rechecked type `tp` of tree `tree` */
     protected def constFold(tree: Tree, tp: Type)(using Context): Type =

compiler/src/dotty/tools/dotc/transform/TreeChecker.scala

@@ -412,16 +412,16 @@ object TreeChecker {
             assert(false, s"The type of a non-Super tree must not be a SuperType, but $tree has type $tp")
           case _ =>
 
-    override def typed(tree: untpd.Tree, pt: Type = WildcardType)(using Context): Tree = {
-      val tpdTree = super.typed(tree, pt)
-      Typer.assertPositioned(tree)
-      checkSuper(tpdTree)
-      if (ctx.erasedTypes)
-        // Can't be checked in earlier phases since `checkValue` is only run in
-        // Erasure (because running it in Typer would force too much)
-        checkIdentNotJavaClass(tpdTree)
-      tpdTree
-    }
+    override def typed(tree: untpd.Tree, pt: Type = WildcardType)(using Context): Tree =
+      trace(i"checking $tree against $pt"):
+        val tpdTree = super.typed(tree, pt)
+        Typer.assertPositioned(tree)
+        checkSuper(tpdTree)
+        if (ctx.erasedTypes)
+          // Can't be checked in earlier phases since `checkValue` is only run in
+          // Erasure (because running it in Typer would force too much)
+          checkIdentNotJavaClass(tpdTree)
+        tpdTree
 
     override def typedUnadapted(tree: untpd.Tree, pt: Type, locked: TypeVars)(using Context): Tree = {
       try