Merge pull request #15364 from dotty-staging/fix-15363

Fix #15363: Improve error messages for leaking of this

Author: liufengyun

compiler/src/dotty/tools/dotc/transform/init/Errors.scala

@@ -19,9 +19,9 @@ object Errors:
     def pos(using Context): SourcePosition = trace.last.sourcePos
 
     def issue(using Context): Unit =
-      report.warning(show + stacktrace, this.pos)
+      report.warning(show, this.pos)
 
-    def stacktrace(using Context): String = if trace.isEmpty then "" else " Calling trace:\n" + {
+    def stacktrace(preamble: String = " Calling trace:\n")(using Context): String = if trace.isEmpty then "" else preamble + {
       var lastLineNum = -1
       var lines: mutable.ArrayBuffer[String] = new mutable.ArrayBuffer
       trace.foreach { tree =>
@@ -72,33 +72,62 @@ object Errors:
   case class AccessNonInit(field: Symbol, trace: Seq[Tree]) extends Error:
     def source: Tree = trace.last
     def show(using Context): String =
-      "Access non-initialized " + field.show + "."
+      "Access non-initialized " + field.show + "." + stacktrace()
 
     override def pos(using Context): SourcePosition = field.sourcePos
 
   /** Promote a value under initialization to fully-initialized */
   case class PromoteError(msg: String, trace: Seq[Tree]) extends Error:
-    def show(using Context): String = msg
+    def show(using Context): String = msg + stacktrace()
 
   case class AccessCold(field: Symbol, trace: Seq[Tree]) extends Error:
     def show(using Context): String =
-      "Access field on a value with an unknown initialization status."
+      "Access field on a value with an unknown initialization status." + stacktrace()
 
   case class CallCold(meth: Symbol, trace: Seq[Tree]) extends Error:
     def show(using Context): String =
-      "Call method on a value with an unknown initialization" + "."
+      "Call method on a value with an unknown initialization." + stacktrace()
 
   case class CallUnknown(meth: Symbol, trace: Seq[Tree]) extends Error:
     def show(using Context): String =
       val prefix = if meth.is(Flags.Method) then "Calling the external method " else "Accessing the external field"
-      prefix + meth.show + " may cause initialization errors" + "."
+      prefix + meth.show + " may cause initialization errors." + stacktrace()
 
   /** Promote a value under initialization to fully-initialized */
   case class UnsafePromotion(msg: String, trace: Seq[Tree], error: Error) extends Error:
-    override def issue(using Context): Unit =
-      report.warning(show, this.pos)
-
     def show(using Context): String =
-      msg + stacktrace + "\n" +
+      msg + stacktrace() + "\n" +
         "Promoting the value to fully initialized failed due to the following problem:\n" +
-        error.show + error.stacktrace
+        error.show
+
+  /** Unsafe leaking a non-hot value as constructor arguments
+   *
+   *  Invariant: argsIndices.nonEmpty
+   */
+  case class UnsafeLeaking(trace: Seq[Tree], error: Error, nonHotOuterClass: Symbol, argsIndices: List[Int]) extends Error:
+    def show(using Context): String =
+      "Problematic object instantiation: " + argumentInfo() + stacktrace() + "\n" +
+        "It leads to the following error during object initialization:\n" +
+        error.show
+
+    private def punctuation(i: Int): String =
+      if i == argsIndices.size - 2 then " and "
+      else if i == argsIndices.size - 1 then ""
+      else ", "
+
+    private def argumentInfo()(using Context): String =
+      val multiple = argsIndices.size > 1 || nonHotOuterClass.exists
+      val init =
+        if nonHotOuterClass.exists
+        then  "the outer " + nonHotOuterClass.name.show + ".this" + punctuation(-1)
+        else ""
+
+      val subject =
+        argsIndices.zipWithIndex.foldLeft(init) { case (acc, (pos, i)) =>
+          val text1 = "arg " + pos.toString
+          val text2 = text1 + punctuation(i)
+          acc + text2
+        }
+      val verb = if multiple then " are " else " is "
+      val adjective = "not fully initialized."
+      subject + verb + adjective

compiler/src/dotty/tools/dotc/transform/init/Semantic.scala

@@ -118,7 +118,7 @@ object Semantic:
   end Warm
 
   /** A function value */
-  case class Fun(expr: Tree, thisV: Ref, klass: ClassSymbol, env: Env) extends Value
+  case class Fun(expr: Tree, thisV: Ref, klass: ClassSymbol) extends Value
 
   /** A value which represents a set of addresses
    *
@@ -144,7 +144,7 @@ object Semantic:
 
     def hasField(f: Symbol) = fields.contains(f)
 
-  /** The environment for method parameters
+  /** The environment stores values for constructor parameters
    *
    *  For performance and usability, we restrict parameters to be either `Cold`
    *  or `Hot`.
@@ -162,6 +162,9 @@ object Semantic:
    *  key. The reason is that given the same receiver, a method or function may
    *  be called with different arguments -- they are not decided by the receiver
    *  anymore.
+   *
+   *  TODO: remove Env as it is only used to pass value from `callConstructor` -> `eval` -> `init`.
+   *  It goes through `eval` for caching (termination) purposes.
    */
   object Env:
     opaque type Env = Map[Symbol, Value]
@@ -588,15 +591,15 @@ object Semantic:
               Hot
 
         case fun: Fun =>
-          report.error("unexpected tree in selecting a function, fun = " + fun.expr.show, fun.expr)
+          report.error("[Internal error] unexpected tree in selecting a function, fun = " + fun.expr.show, fun.expr)
           Hot
 
         case RefSet(refs) =>
           refs.map(_.select(field)).join
       }
     }
 
-    def call(meth: Symbol, args: List[ArgInfo], receiver: Type, superType: Type, needResolve: Boolean = true): Contextual[Value] = log("call " + meth.show + ", args = " + args, printer, (_: Value).show) {
+    def call(meth: Symbol, args: List[ArgInfo], receiver: Type, superType: Type, needResolve: Boolean = true): Contextual[Value] = log("call " + meth.show + ", args = " + args.map(_.value.show), printer, (_: Value).show) {
       def promoteArgs(): Contextual[Unit] = args.foreach(_.promote)
 
       def isSyntheticApply(meth: Symbol) =
@@ -704,21 +707,19 @@ object Semantic:
             else
               value.select(target, needResolve = false)
 
-        case Fun(body, thisV, klass, env) =>
+        case Fun(body, thisV, klass) =>
           // meth == NoSymbol for poly functions
           if meth.name.toString == "tupled" then value // a call like `fun.tupled`
           else
             promoteArgs()
-            withEnv(env) {
-              eval(body, thisV, klass, cacheResult = true)
-            }
+            eval(body, thisV, klass, cacheResult = true)
 
         case RefSet(refs) =>
           refs.map(_.call(meth, args, receiver, superType)).join
       }
     }
 
-    def callConstructor(ctor: Symbol, args: List[ArgInfo]): Contextual[Value] = log("call " + ctor.show + ", args = " + args, printer, (_: Value).show) {
+    def callConstructor(ctor: Symbol, args: List[ArgInfo]): Contextual[Value] = log("call " + ctor.show + ", args = " + args.map(_.value.show), printer, (_: Value).show) {
       // init "fake" param fields for the secondary constructor
       def addParamsAsFields(env: Env, ref: Ref, ctorDef: DefDef) = {
         val paramSyms = ctorDef.termParamss.flatten.map(_.symbol)
@@ -775,7 +776,27 @@ object Semantic:
     }
 
     /** Handle a new expression `new p.C` where `p` is abstracted by `value` */
-    def instantiate(klass: ClassSymbol, ctor: Symbol, args: List[ArgInfo]): Contextual[Value] = log("instantiating " + klass.show + ", value = " + value + ", args = " + args, printer, (_: Value).show) {
+    def instantiate(klass: ClassSymbol, ctor: Symbol, args: List[ArgInfo]): Contextual[Value] = log("instantiating " + klass.show + ", value = " + value + ", args = " + args.map(_.value.show), printer, (_: Value).show) {
+      def tryLeak(warm: Warm, nonHotOuterClass: Symbol, argValues: List[Value]): Contextual[Value] =
+        val argInfos2 = args.zip(argValues).map { (argInfo, v) => argInfo.copy(value = v) }
+        val errors = Reporter.stopEarly {
+          given Trace = Trace.empty
+          warm.callConstructor(ctor, argInfos2)
+        }
+        if errors.nonEmpty then
+          val indices =
+            for
+              (arg, i) <- argValues.zipWithIndex
+              if arg.isCold
+            yield
+              i + 1
+
+          val error = UnsafeLeaking(trace.toVector, errors.head, nonHotOuterClass, indices)
+          reporter.report(error)
+          Hot
+        else
+          warm
+
       if promoted.isCurrentObjectPromoted then Hot
       else value match {
         case Hot  =>
@@ -792,9 +813,7 @@ object Semantic:
           else
             val outer = Hot
             val warm = Warm(klass, outer, ctor, args2).ensureObjectExists()
-            val argInfos2 = args.zip(args2).map { (argInfo, v) => argInfo.copy(value = v) }
-            warm.callConstructor(ctor, argInfos2)
-            warm
+            tryLeak(warm, NoSymbol, args2)
 
         case Cold =>
           val error = CallCold(ctor, trace.toVector)
@@ -810,13 +829,16 @@ object Semantic:
             case _ => ref
 
           val argsWidened = args.map(_.value).widenArgs
-          val argInfos2 = args.zip(argsWidened).map { (argInfo, v) => argInfo.copy(value = v) }
           val warm = Warm(klass, outer, ctor, argsWidened).ensureObjectExists()
-          warm.callConstructor(ctor, argInfos2)
-          warm
+          if argsWidened.exists(_.isCold) then
+            tryLeak(warm, klass.owner.lexicallyEnclosingClass, argsWidened)
+          else
+            val argInfos2 = args.zip(argsWidened).map { (argInfo, v) => argInfo.copy(value = v) }
+            warm.callConstructor(ctor, argInfos2)
+            warm
 
-        case Fun(body, thisV, klass, env) =>
-          report.error("unexpected tree in instantiating a function, fun = " + body.show, trace.toVector.last)
+        case Fun(body, thisV, klass) =>
+          report.error("[Internal error] unexpected tree in instantiating a function, fun = " + body.show, trace.toVector.last)
           Hot
 
         case RefSet(refs) =>
@@ -830,21 +852,14 @@ object Semantic:
       val sym = tmref.symbol
 
       if sym.is(Flags.Param) && sym.owner.isConstructor then
-        // if we can get the field from the Ref (which can only possibly be
-        // a secondary constructor parameter), then use it.
-        if (ref.objekt.hasField(sym))
-          ref.objekt.field(sym)
-        // instances of local classes inside secondary constructors cannot
-        // reach here, as those values are abstracted by Cold instead of Warm.
-        // This enables us to simplify the domain without sacrificing
-        // expressiveness nor soundess, as local classes inside secondary
-        // constructors are uncommon.
-        else if sym.isContainedIn(klass) then
-          env.lookup(sym)
-        else
-          // We don't know much about secondary constructor parameters in outer scope.
-          // It's always safe to approximate them with `Cold`.
-          Cold
+        val enclosingClass = sym.owner.enclosingClass.asClass
+        val thisValue2 = resolveThis(enclosingClass, ref, klass)
+        thisValue2 match
+        case Hot => Hot
+        case ref: Ref => ref.objekt.field(sym)
+        case _ =>
+            report.error("[Internal error] unexpected this value accessing local variable, sym = " + sym.show + ", thisValue = " + thisValue2.show, trace.toVector.last)
+            Hot
       else if sym.is(Flags.Param) then
         Hot
       else
@@ -861,7 +876,7 @@ object Semantic:
               case ref: Ref => eval(vdef.rhs, ref, enclosingClass)
 
               case _ =>
-                 report.error("unexpected defTree when accessing local variable, sym = " + sym.show + ", defTree = " + sym.defTree.show, trace.toVector.last)
+                 report.error("[Internal error] unexpected this value when accessing local variable, sym = " + sym.show + ", thisValue = " + thisValue2.show, trace.toVector.last)
                  Hot
             end match
 
@@ -932,10 +947,10 @@ object Semantic:
             if errors.nonEmpty then promoted.remove(warm)
             reporter.reportAll(errors)
 
-        case fun @ Fun(body, thisV, klass, env) =>
+        case fun @ Fun(body, thisV, klass) =>
           if !promoted.contains(fun) then
             val errors = Reporter.stopEarly {
-              val res = withEnv(env) {
+              val res = {
                 given Trace = Trace.empty
                 eval(body, thisV, klass)
               }
@@ -1120,7 +1135,7 @@ object Semantic:
     args.foreach { arg =>
       val res =
         if arg.isByName then
-          Fun(arg.tree, thisV, klass, env)
+          Fun(arg.tree, thisV, klass)
         else
           eval(arg.tree, thisV, klass)
 
@@ -1226,10 +1241,10 @@ object Semantic:
           }
 
       case closureDef(ddef) =>
-        Fun(ddef.rhs, thisV, klass, env)
+        Fun(ddef.rhs, thisV, klass)
 
       case PolyFun(body) =>
-        Fun(body, thisV, klass, env)
+        Fun(body, thisV, klass)
 
       case Block(stats, expr) =>
         eval(stats, thisV, klass)
@@ -1302,8 +1317,8 @@ object Semantic:
         Hot
 
       case _ =>
-        throw new Exception("unexpected tree: " + expr.show)
-
+        report.error("[Internal error] unexpected tree", expr)
+        Hot
 
   /** Handle semantics of leaf nodes */
   def cases(tp: Type, thisV: Ref, klass: ClassSymbol): Contextual[Value] = log("evaluating " + tp.show, printer, (_: Value).show) {
@@ -1331,7 +1346,8 @@ object Semantic:
         Hot
 
       case _ =>
-        throw new Exception("unexpected type: " + tp)
+        report.error("[Internal error] unexpected type " + tp, trace.toVector.last)
+        Hot
   }
 
   /** Resolve C.this that appear in `klass` */
@@ -1345,15 +1361,15 @@ object Semantic:
           val obj = ref.objekt
           val outerCls = klass.owner.lexicallyEnclosingClass.asClass
           if !obj.hasOuter(klass) then
-            val error = PromoteError("outer not yet initialized, target = " + target + ", klass = " + klass + ", object = " + obj, trace.toVector)
-            report.error(error.show + error.stacktrace, trace.toVector.last)
+            val error = PromoteError("[Internal error] outer not yet initialized, target = " + target + ", klass = " + klass + ", object = " + obj, trace.toVector)
+            report.error(error.show, trace.toVector.last)
             Hot
           else
             resolveThis(target, obj.outer(klass), outerCls)
         case RefSet(refs) =>
           refs.map(ref => resolveThis(target, ref, klass)).join
         case fun: Fun =>
-          report.warning("unexpected thisV = " + thisV + ", target = " + target.show + ", klass = " + klass.show, trace.toVector.last)
+          report.error("[Internal error] unexpected thisV = " + thisV + ", target = " + target.show + ", klass = " + klass.show, trace.toVector.last)
           Cold
         case Cold => Cold
 
@@ -1381,14 +1397,15 @@ object Semantic:
             resolveThis(target, thisV, cur)
 
           case None =>
-            report.warning("unexpected outerSelect, thisV = " + thisV + ", target = " + target.show + ", hops = " + hops, trace.toVector.last.srcPos)
+            // TODO: use error once we fix https://github.com/lampepfl/dotty/issues/15465
+            report.warning("[Internal error] unexpected outerSelect, thisV = " + thisV + ", target = " + target.show + ", hops = " + hops, trace.toVector.last.srcPos)
             Cold
 
       case RefSet(refs) =>
         refs.map(ref => resolveOuterSelect(target, ref, hops)).join
 
       case fun: Fun =>
-        report.warning("unexpected thisV = " + thisV + ", target = " + target.show + ", hops = " + hops, trace.toVector.last.srcPos)
+        report.error("[Internal error] unexpected thisV = " + thisV + ", target = " + target.show + ", hops = " + hops, trace.toVector.last.srcPos)
         Cold
 
       case Cold => Cold
@@ -1516,6 +1533,10 @@ object Semantic:
         eval(tree, thisV, klass)
     }
 
+    // ensure we try promotion once even if class body is empty
+    if fieldsChanged && thisV.isThisRef then
+      thisV.asInstanceOf[ThisRef].tryPromoteCurrentObject()
+
     // The result value is ignored, use Hot to avoid futile fixed point computation
     Hot
   }

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -86,7 +86,7 @@ object Implicits:
    */
   abstract class ImplicitRefs(initctx: Context) {
     val irefCtx =
-      if (initctx == NoContext) initctx else initctx.retractMode(Mode.ImplicitsEnabled)
+      if (initctx eq NoContext) initctx else initctx.retractMode(Mode.ImplicitsEnabled)
     protected given Context = irefCtx
 
     /** The nesting level of this context. Non-zero only in ContextialImplicits */

tests/init/neg/apply2.scala

@@ -3,8 +3,8 @@ object O:
     println(n)
 
   class B:
-    val a = A(this)
+    val a = A(this)  // error
 
   val b = new B
-  val n = 10         // error
+  val n = 10
 end O

tests/init/neg/as-instance-of-cold-field-access.scala

@@ -1,7 +1,7 @@
 final class MyAsInstanceOfClass(o: MyAsInstanceOfClass) {
   val other: MyAsInstanceOfClass = {
-    if (o.asInstanceOf[MyAsInstanceOfClass].oRef ne null) o // error
-    else new MyAsInstanceOfClass(this)
+    if (o.asInstanceOf[MyAsInstanceOfClass].oRef ne null) o
+    else new MyAsInstanceOfClass(this)  // error
   }
   val oRef = o
 }