From 37e7e2a6802d51eee403133b40bcd3048c9ceb47 Mon Sep 17 00:00:00 2001 From: David Hua Date: Tue, 18 Feb 2025 23:31:43 -0500 Subject: [PATCH 1/2] Fix crash by using special resolveEnv function for local read and write --- .../tools/dotc/transform/init/Objects.scala | 58 +++++++++++++++---- tests/{init => init-global}/pos/byname.scala | 6 +- tests/init-global/warn/lazy-local-val.scala | 2 +- 3 files changed, 51 insertions(+), 15 deletions(-) rename tests/{init => init-global}/pos/byname.scala (60%) diff --git a/compiler/src/dotty/tools/dotc/transform/init/Objects.scala b/compiler/src/dotty/tools/dotc/transform/init/Objects.scala index 316213a94f8d..d1015c3594a2 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/Objects.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/Objects.scala @@ -436,6 +436,39 @@ class Objects(using Context @constructorOnly): case _ => throw new RuntimeException("Incorrect local environment for initializing " + x.show) + /** + * Resolve the environment by searching for a given symbol. + * + * Searches for the environment that owns `target`, starting from `env` as the innermost. + * + * Due to widening, the corresponding environment might not exist. As a result reading the local + * variable will return `Cold` and it's forbidden to write to the local variable. + * + * @param target The symbol to search for. + * @param thisV The value for `this` of the enclosing class where the local variable is referenced. + * @param env The local environment where the local variable is referenced. + * + * @return the environment that owns the `target` and value for `this` owned by the given method. + */ + def resolveEnvByValue(target: Symbol, thisV: ThisValue, env: Data)(using Context): Option[(ThisValue, Data)] = log("Resolving env by value for " + target.show + ", this = " + thisV.show + ", env = " + env.show, printer) { + env match + case localEnv: LocalEnv => + if localEnv.getVal(target).isDefined then Some(thisV -> localEnv) + else if localEnv.getVar(target).isDefined then Some(thisV -> localEnv) + else resolveEnvByValue(target, thisV, localEnv.outer) + case NoEnv => + thisV match + case ref: OfClass => + ref.outer match + case outer : ThisValue => + resolveEnvByValue(target, outer, ref.env) + case _ => + // TODO: properly handle the case where ref.outer is ValueSet + None + case _ => + None + } + /** * Resolve the environment owned by the given method. * @@ -451,17 +484,17 @@ class Objects(using Context @constructorOnly): * * @return the environment and value for `this` owned by the given method. */ - def resolveEnv(meth: Symbol, thisV: ThisValue, env: Data)(using Context): Option[(ThisValue, Data)] = log("Resolving env for " + meth.show + ", this = " + thisV.show + ", env = " + env.show, printer) { + def resolveEnvByOwner(meth: Symbol, thisV: ThisValue, env: Data)(using Context): Option[(ThisValue, Data)] = log("Resolving env by owner for " + meth.show + ", this = " + thisV.show + ", env = " + env.show, printer) { env match case localEnv: LocalEnv => if localEnv.meth == meth then Some(thisV -> env) - else resolveEnv(meth, thisV, localEnv.outer) + else resolveEnvByOwner(meth, thisV, localEnv.outer) case NoEnv => thisV match case ref: OfClass => ref.outer match case outer : ThisValue => - resolveEnv(meth, outer, ref.env) + resolveEnvByOwner(meth, outer, ref.env) case _ => // TODO: properly handle the case where ref.outer is ValueSet None @@ -724,7 +757,7 @@ class Objects(using Context @constructorOnly): if meth.owner.isClass then (ref, Env.NoEnv) else - Env.resolveEnv(meth.owner.enclosingMethod, ref, summon[Env.Data]).getOrElse(Cold -> Env.NoEnv) + Env.resolveEnvByOwner(meth.owner.enclosingMethod, ref, summon[Env.Data]).getOrElse(Cold -> Env.NoEnv) val env2 = Env.ofDefDef(ddef, args.map(_.value), outerEnv) extendTrace(ddef) { @@ -771,9 +804,9 @@ class Objects(using Context @constructorOnly): end if case _ => - // by-name closure - given Env.Data = env - extendTrace(code) { eval(code, thisV, klass, cacheResult = true) } + // Should be unreachable, by-name closures are handled by readLocal + report.warning("[Internal error] Only DefDef should be possible here, but found " + code.show + ". " + Trace.show, Trace.position) + Bottom case ValueSet(vs) => vs.map(v => call(v, meth, args, receiver, superType)).join @@ -962,7 +995,7 @@ class Objects(using Context @constructorOnly): (thisV.widenRefOrCold(1), Env.NoEnv) else // klass.enclosingMethod returns its primary constructor - Env.resolveEnv(klass.owner.enclosingMethod, thisV, summon[Env.Data]).getOrElse(Cold -> Env.NoEnv) + Env.resolveEnvByOwner(klass.owner.enclosingMethod, thisV, summon[Env.Data]).getOrElse(Cold -> Env.NoEnv) val instance = OfClass(klass, outerWidened, ctor, args.map(_.value), envWidened) callConstructor(instance, ctor, args) @@ -992,7 +1025,9 @@ class Objects(using Context @constructorOnly): */ def readLocal(thisV: ThisValue, sym: Symbol): Contextual[Value] = log("reading local " + sym.show, printer, (_: Value).show) { def isByNameParam(sym: Symbol) = sym.is(Flags.Param) && sym.info.isInstanceOf[ExprType] - Env.resolveEnv(sym.enclosingMethod, thisV, summon[Env.Data]) match + // Can't use enclosingMethod here because values defined in a by-name closure will have the wrong enclosingMethod, + // since our phase is before elimByName. + Env.resolveEnvByValue(sym, thisV, summon[Env.Data]) match case Some(thisV -> env) => if sym.is(Flags.Mutable) then // Assume forward reference check is doing a good job @@ -1047,8 +1082,9 @@ class Objects(using Context @constructorOnly): */ def writeLocal(thisV: ThisValue, sym: Symbol, value: Value): Contextual[Value] = log("write local " + sym.show + " with " + value.show, printer, (_: Value).show) { assert(sym.is(Flags.Mutable), "Writing to immutable variable " + sym.show) - - Env.resolveEnv(sym.enclosingMethod, thisV, summon[Env.Data]) match + // Can't use enclosingMethod here because values defined in a by-name closure will have the wrong enclosingMethod, + // since our phase is before elimByName. + Env.resolveEnvByValue(sym, thisV, summon[Env.Data]) match case Some(thisV -> env) => given Env.Data = env Env.getVar(sym) match diff --git a/tests/init/pos/byname.scala b/tests/init-global/pos/byname.scala similarity index 60% rename from tests/init/pos/byname.scala rename to tests/init-global/pos/byname.scala index fdfbd101cc93..65dddf51512d 100644 --- a/tests/init/pos/byname.scala +++ b/tests/init-global/pos/byname.scala @@ -5,12 +5,12 @@ class A extends T: override def bar(i: => Int): Int = i + 1 class B extends T: - override def bar(i: => Int): Int = i + 2 + override def bar(i: => Int): Int = i object A: val a: T = if ??? then new A else new B - def foo(b: List[Int]) = a.bar(b match { - case x :: xs => 1 + def foo(b: List[Int]): Int = a.bar(b match { + case head :: rest => head + foo(rest) + a.bar(head) case Nil => 0 }) diff --git a/tests/init-global/warn/lazy-local-val.scala b/tests/init-global/warn/lazy-local-val.scala index 30ae864ab169..8a8ca90e4ed8 100644 --- a/tests/init-global/warn/lazy-local-val.scala +++ b/tests/init-global/warn/lazy-local-val.scala @@ -15,5 +15,5 @@ object B: lazy val b = a Box(b) - val box = f(n) // warn + val box = f(n) val n = 10 From f3c47519c814c5206e6d6dbff372f6430ef405db Mon Sep 17 00:00:00 2001 From: David Hua Date: Sun, 2 Mar 2025 21:48:01 -0500 Subject: [PATCH 2/2] Adjust some documentation and add assertion --- .../tools/dotc/transform/init/Objects.scala | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/transform/init/Objects.scala b/compiler/src/dotty/tools/dotc/transform/init/Objects.scala index d1015c3594a2..e7c4a9d62b1e 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/Objects.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/Objects.scala @@ -444,9 +444,9 @@ class Objects(using Context @constructorOnly): * Due to widening, the corresponding environment might not exist. As a result reading the local * variable will return `Cold` and it's forbidden to write to the local variable. * - * @param target The symbol to search for. - * @param thisV The value for `this` of the enclosing class where the local variable is referenced. - * @param env The local environment where the local variable is referenced. + * @param target The symbol to search for. + * @param thisV The value for `this` of the enclosing class where the local variable is referenced. + * @param env The local environment where the local variable is referenced. * * @return the environment that owns the `target` and value for `this` owned by the given method. */ @@ -470,7 +470,7 @@ class Objects(using Context @constructorOnly): } /** - * Resolve the environment owned by the given method. + * Resolve the environment owned by the given method `enclosing`. * * The method could be located in outer scope with intermixed classes between its definition * site and usage site. @@ -478,23 +478,25 @@ class Objects(using Context @constructorOnly): * Due to widening, the corresponding environment might not exist. As a result reading the local * variable will return `Cold` and it's forbidden to write to the local variable. * - * @param meth The method which owns the environment - * @param thisV The value for `this` of the enclosing class where the local variable is referenced. - * @param env The local environment where the local variable is referenced. + * @param enclosing The method which owns the environment. This method is called to look up the environment + * owned by the enclosing method of some symbol. + * @param thisV The value for `this` of the enclosing class where the local variable is referenced. + * @param env The local environment where the local variable is referenced. * * @return the environment and value for `this` owned by the given method. */ - def resolveEnvByOwner(meth: Symbol, thisV: ThisValue, env: Data)(using Context): Option[(ThisValue, Data)] = log("Resolving env by owner for " + meth.show + ", this = " + thisV.show + ", env = " + env.show, printer) { + def resolveEnvByOwner(enclosing: Symbol, thisV: ThisValue, env: Data)(using Context): Option[(ThisValue, Data)] = log("Resolving env by owner for " + enclosing.show + ", this = " + thisV.show + ", env = " + env.show, printer) { + assert(enclosing.is(Flags.Method), "Only method symbols allows, got " + enclosing.show) env match case localEnv: LocalEnv => - if localEnv.meth == meth then Some(thisV -> env) - else resolveEnvByOwner(meth, thisV, localEnv.outer) + if localEnv.meth == enclosing then Some(thisV -> env) + else resolveEnvByOwner(enclosing, thisV, localEnv.outer) case NoEnv => thisV match case ref: OfClass => ref.outer match case outer : ThisValue => - resolveEnvByOwner(meth, outer, ref.env) + resolveEnvByOwner(enclosing, outer, ref.env) case _ => // TODO: properly handle the case where ref.outer is ValueSet None