Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #15363: Improve error messages for leaking of this #15364

Merged
merged 10 commits into from
Jun 16, 2022
Merged

Fix #15363: Improve error messages for leaking of this #15364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
eff2f41
Fix #15363: Improve error messages of leaked non-hot values
liufengyun Jun 2, 2022
a41fa4f
Update tests
liufengyun Jun 2, 2022
04b8f94
Refine the logic for accessing secondary ctor params
liufengyun Jun 2, 2022
0754aa0
Remove env for Fun
liufengyun Jun 2, 2022
6bb7b5e
Refactor code
liufengyun Jun 3, 2022
7671826
Better division of Error.show and Error.issue
liufengyun Jun 3, 2022
f19faeb
Show which argument is non-hot
liufengyun Jun 3, 2022
0734d59
Make internal error more explicit in the message
liufengyun Jun 3, 2022
b8a7f64
Be more specific about the non-hot outer in error messages
liufengyun Jun 13, 2022
73b3331
Ignore outer errors temporarily
liufengyun Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions compiler/src/dotty/tools/dotc/transform/init/Errors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down Expand Up @@ -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
115 changes: 68 additions & 47 deletions compiler/src/dotty/tools/dotc/transform/init/Semantic.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand All @@ -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`.
Expand All @@ -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]
Expand Down Expand Up @@ -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) =
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 =>
Expand All @@ -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)
Expand All @@ -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) =>
Expand All @@ -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
olhotak marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -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

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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` */
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
Loading