From 790370d3d8b59228f543c9117f83e3eec4050125 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Fri, 26 Mar 2021 17:54:59 +0100 Subject: [PATCH 1/4] Fix #9176: fast check of cyclic object initialization --- .../dotc/transform/init/CheckGlobal.scala | 145 ++++++++++++++++++ .../tools/dotc/transform/init/Checker.scala | 6 + tests/init/neg/i9176.scala | 9 ++ 3 files changed, 160 insertions(+) create mode 100644 compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala create mode 100644 tests/init/neg/i9176.scala diff --git a/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala new file mode 100644 index 000000000000..fd79a8187216 --- /dev/null +++ b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala @@ -0,0 +1,145 @@ +package dotty.tools.dotc +package transform +package init + +import core._ +import Flags._ +import Contexts._ +import Types._ +import Symbols._ +import Decorators._ +import printing.SyntaxHighlighting +import reporting.trace +import config.Printers.init + +import ast.Trees._ +import ast.tpd + +import scala.collection.mutable + + +/** Check that static objects can be initialized without cycles + * + * For the check to be fast, the algorithm uses coarse approximation. + * We construct a dependency graph as follows: + * + * - if a static object `O` is used in another class/static-object `B`, + * then O -> B + * - if a class `C` is instantiated in a another class/static-object `B`, + * then C -> B + * - if a static-object/class `A` extends another class `B`, + * then A -> B + * + * Given the graph above, we check if there exists cycles. + * + * This check does not need to care about objects in libraries, as separate + * compilation ensures that there cannot be cyles between two separately + * compiled projects. + */ +class CheckGlobal { + case class Dependency(sym: Symbol, source: tpd.Tree) + + /** Checking state */ + case class State(var visited: Set[Symbol], path: Vector[tpd.Tree], obj: Symbol) { + def cyclicPath(using Context): String = if (path.isEmpty) "" else " Cyclic path:\n" + { + var indentCount = 0 + var last: String = "" + val sb = new StringBuilder + path.foreach { tree => + indentCount += 1 + val pos = tree.sourcePos + val prefix = s"${ " " * indentCount }-> " + val line = + if pos.source.exists then + val loc = "[ " + pos.source.file.name + ":" + (pos.line + 1) + " ]" + val code = SyntaxHighlighting.highlight(pos.lineContent.trim) + i"$code\t$loc" + else + tree.show + + if (last != line) sb.append(prefix + line + "\n") + + last = line + } + sb.toString + } + } + + case class Error(state: State) { + def issue(using Context): Unit = + report.warning("Cylic object dependencies detected." + state.cyclicPath, state.obj.defTree.srcPos) + } + + /** Summary of dependencies */ + private val summaryCache = mutable.Map.empty[Symbol, List[Dependency]] + + def check(obj: Symbol)(using Context): Unit = trace("checking " + obj.show, init) { + checkDependencies(obj, State(visited = Set.empty, path = Vector.empty, obj)) match + case Some(err) => err.issue + case _ => + } + + private def check(sym: Symbol, state: State)(using Context): Option[Error] = trace("checking " + sym.show, init) { + if sym == state.obj then + Some(Error(state)) + else if state.visited.contains(sym) then + None + else + state.visited = state.visited + sym + checkDependencies(sym, state) + } + + private def checkDependencies(sym: Symbol, state: State)(using Context): Option[Error] = trace("checking dependencies of " + sym.show, init) { + val cls = if sym.is(Module) then sym.moduleClass.asClass else sym.asClass + val deps = analyze(cls) + Util.traceIndented("dependencies of " + sym.show + " = " + deps.map(_.sym.show).mkString(","), init) + var res: Option[Error] = None + // TODO: stop early + deps.foreach { dep => + val state2: State = state.copy(path = state.path :+ dep.source) + if res.isEmpty then res = check(dep.sym, state2) + } + res + } + + private def analyze(cls: ClassSymbol)(using Context): List[Dependency] = + def isStaticObjectRef(sym: Symbol) = + sym.isTerm && !sym.is(Package) && sym.is(Module) + && sym.isStatic && sym.moduleClass != cls + + if (cls.defTree.isEmpty) Nil + else if (summaryCache.contains(cls)) summaryCache(cls) + else { + val cdef = cls.defTree.asInstanceOf[tpd.TypeDef] + val tpl = cdef.rhs.asInstanceOf[tpd.Template] + var dependencies: List[Dependency] = Nil + val traverser = new tpd.TreeTraverser { + override def traverse(tree: tpd.Tree)(using Context): Unit = + tree match { + case tree: tpd.RefTree if isStaticObjectRef(tree.symbol) => + dependencies = Dependency(tree.symbol, tree) :: dependencies + + case tdef: tpd.TypeDef => + // don't go into nested classes + + case tree: tpd.New => + dependencies = Dependency(tree.tpe.classSymbol, tree) :: dependencies + + case _ => + traverseChildren(tree) + } + } + + // TODO: the traverser might create duplicate entries for parents + tpl.parents.foreach { tree => + dependencies = Dependency(tree.tpe.classSymbol, tree) :: dependencies + } + + traverser.traverse(tpl) + summaryCache(cls) = dependencies + dependencies + } + + def debugCache(using Context) = + summaryCache.map(_.show + " -> " + _.map(_.sym.show).mkString(",")).mkString("\n") +} \ No newline at end of file diff --git a/compiler/src/dotty/tools/dotc/transform/init/Checker.scala b/compiler/src/dotty/tools/dotc/transform/init/Checker.scala index c7607827f639..43cc94ed60da 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/Checker.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/Checker.scala @@ -26,6 +26,8 @@ class Checker extends MiniPhase { // cache of class summary private val baseEnv = Env(null) + val globalChecker = new CheckGlobal + override val runsAfter = Set(Pickler.name) override def isEnabled(using Context): Boolean = @@ -58,6 +60,10 @@ class Checker extends MiniPhase { ) Checking.checkClassBody(tree) + + // check cycles of object dependencies + if cls.is(Flags.Module) && cls.isStatic then + globalChecker.check(cls.sourceModule) } tree diff --git a/tests/init/neg/i9176.scala b/tests/init/neg/i9176.scala new file mode 100644 index 000000000000..c70e87d1872a --- /dev/null +++ b/tests/init/neg/i9176.scala @@ -0,0 +1,9 @@ +class Foo(val opposite: Foo) +case object A extends Foo(B) // error +case object B extends Foo(A) // error +object Test { + def main(args: Array[String]): Unit = { + println(A.opposite) + println(B.opposite) + } +} \ No newline at end of file From 8c28e9b4bd1a1e23f0664b8dd4c4e7b764955ef4 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Fri, 26 Mar 2021 20:26:57 +0100 Subject: [PATCH 2/4] Add test --- tests/init/neg/t92661.scala | 3 +++ tests/init/neg/t9312.scala | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 tests/init/neg/t92661.scala create mode 100644 tests/init/neg/t9312.scala diff --git a/tests/init/neg/t92661.scala b/tests/init/neg/t92661.scala new file mode 100644 index 000000000000..378dad51b3a6 --- /dev/null +++ b/tests/init/neg/t92661.scala @@ -0,0 +1,3 @@ +sealed abstract class OrderType(val reverse: OrderType) +case object Buy extends OrderType(Sell) // error +case object Sell extends OrderType(Buy) // error diff --git a/tests/init/neg/t9312.scala b/tests/init/neg/t9312.scala new file mode 100644 index 000000000000..fb3d53c81eb9 --- /dev/null +++ b/tests/init/neg/t9312.scala @@ -0,0 +1,23 @@ +object DeadLockTest { + def main(args: Array[String]): Unit = { + def run(block: => Unit): Unit = + new Thread(new Runnable {def run(): Unit = block}).start() + + run {println(Parent.Child1)} + run {println(Parent.Child2)} + + } + + object Parent { // error + trait Child { + Thread.sleep(2000) // ensure concurrent behavior + val parent = Parent + def siblings = parent.children - this + } + + object Child1 extends Child // error + object Child2 extends Child // error + + final val children = Set(Child1, Child2) // error + } +} From 6b4fd1bd296da2fca9aab2d3df71c6993a5c6a3d Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Fri, 26 Mar 2021 20:56:26 +0100 Subject: [PATCH 3/4] Handle static objects as outer --- .../dotc/transform/init/CheckGlobal.scala | 53 +++++++++++++------ .../tools/dotc/transform/init/Checking.scala | 34 ++++++++---- tests/init/neg/t9115.scala | 8 +++ 3 files changed, 68 insertions(+), 27 deletions(-) create mode 100644 tests/init/neg/t9115.scala diff --git a/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala index fd79a8187216..3ce0b6044226 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala @@ -12,8 +12,7 @@ import printing.SyntaxHighlighting import reporting.trace import config.Printers.init -import ast.Trees._ -import ast.tpd +import ast.tpd._ import scala.collection.mutable @@ -37,10 +36,10 @@ import scala.collection.mutable * compiled projects. */ class CheckGlobal { - case class Dependency(sym: Symbol, source: tpd.Tree) + case class Dependency(sym: Symbol, source: Tree) /** Checking state */ - case class State(var visited: Set[Symbol], path: Vector[tpd.Tree], obj: Symbol) { + case class State(visited: mutable.Set[Symbol], path: Vector[Tree], obj: Symbol) { def cyclicPath(using Context): String = if (path.isEmpty) "" else " Cyclic path:\n" + { var indentCount = 0 var last: String = "" @@ -74,7 +73,7 @@ class CheckGlobal { private val summaryCache = mutable.Map.empty[Symbol, List[Dependency]] def check(obj: Symbol)(using Context): Unit = trace("checking " + obj.show, init) { - checkDependencies(obj, State(visited = Set.empty, path = Vector.empty, obj)) match + checkDependencies(obj, State(visited = mutable.Set.empty, path = Vector.empty, obj)) match case Some(err) => err.issue case _ => } @@ -85,7 +84,7 @@ class CheckGlobal { else if state.visited.contains(sym) then None else - state.visited = state.visited + sym + state.visited += sym checkDependencies(sym, state) } @@ -96,8 +95,9 @@ class CheckGlobal { var res: Option[Error] = None // TODO: stop early deps.foreach { dep => - val state2: State = state.copy(path = state.path :+ dep.source) - if res.isEmpty then res = check(dep.sym, state2) + if res.isEmpty then + val state2: State = state.copy(path = state.path :+ dep.source) + res = check(dep.sym, state2) } res } @@ -110,19 +110,19 @@ class CheckGlobal { if (cls.defTree.isEmpty) Nil else if (summaryCache.contains(cls)) summaryCache(cls) else { - val cdef = cls.defTree.asInstanceOf[tpd.TypeDef] - val tpl = cdef.rhs.asInstanceOf[tpd.Template] + val cdef = cls.defTree.asInstanceOf[TypeDef] + val tpl = cdef.rhs.asInstanceOf[Template] var dependencies: List[Dependency] = Nil - val traverser = new tpd.TreeTraverser { - override def traverse(tree: tpd.Tree)(using Context): Unit = + val traverser = new TreeTraverser { + override def traverse(tree: Tree)(using Context): Unit = tree match { - case tree: tpd.RefTree if isStaticObjectRef(tree.symbol) => + case tree: RefTree if isStaticObjectRef(tree.symbol) => dependencies = Dependency(tree.symbol, tree) :: dependencies - case tdef: tpd.TypeDef => + case tdef: TypeDef => // don't go into nested classes - case tree: tpd.New => + case tree: New => dependencies = Dependency(tree.tpe.classSymbol, tree) :: dependencies case _ => @@ -130,9 +130,30 @@ class CheckGlobal { } } + def typeRefOf(tp: Type): TypeRef = tp.dealias.typeConstructor match { + case tref: TypeRef => tref + case hklambda: HKTypeLambda => typeRefOf(hklambda.resType) + } + + def addStaticOuterDep(tp: Type, source: Tree): Unit = + tp match + case NoPrefix => + case tmref: TermRef => + if isStaticObjectRef(tmref.symbol) then + dependencies = Dependency(tmref.symbol, source) :: dependencies + case ThisType(tref) => + val obj = tref.symbol.sourceModule + if isStaticObjectRef(obj) then + dependencies = Dependency(obj, source) :: dependencies + case _ => + throw new Exception("unexpected type: " + tp) + // TODO: the traverser might create duplicate entries for parents tpl.parents.foreach { tree => - dependencies = Dependency(tree.tpe.classSymbol, tree) :: dependencies + val tp = tree.tpe + val tref = typeRefOf(tp) + dependencies = Dependency(tp.classSymbol, tree) :: dependencies + addStaticOuterDep(tref.prefix, tree) } traverser.traverse(tpl) diff --git a/compiler/src/dotty/tools/dotc/transform/init/Checking.scala b/compiler/src/dotty/tools/dotc/transform/init/Checking.scala index e47c3b08d264..8e3cc2d4af5c 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/Checking.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/Checking.scala @@ -39,7 +39,18 @@ object Checking { safePromoted: mutable.Set[Potential], // Potentials that can be safely promoted env: Env ) { - def withOwner(sym: Symbol): State = copy(env = env.withOwner(sym)) + def withOwner[T](sym: Symbol)(op: State ?=> T): T = + val state = this.copy(env = env.withOwner(sym)) + val res = op(using state) + this.visited = state.visited + res + + + def withStep[T](step: Tree)(op: State ?=> T): T = + val state: State = this.copy(path = path :+ step) + val res = op(using state) + this.visited = state.visited + res def test(op: State ?=> Errors): Errors = { val savedVisited = visited @@ -60,11 +71,12 @@ object Checking { } else { state.visited = state.visited + eff - val state2: State = state.copy(path = state.path :+ eff.source) - eff match { - case eff: Promote => Checking.checkPromote(eff)(using state2) - case eff: FieldAccess => Checking.checkFieldAccess(eff)(using state2) - case eff: MethodCall => Checking.checkMethodCall(eff)(using state2) + state.withStep(eff.source) { + eff match { + case eff: Promote => Checking.checkPromote(eff) + case eff: FieldAccess => Checking.checkFieldAccess(eff) + case eff: MethodCall => Checking.checkMethodCall(eff) + } } } } @@ -118,11 +130,11 @@ object Checking { def checkConstructor(ctor: Symbol, tp: Type, source: Tree)(using state: State): Unit = traceOp("checking " + ctor.show, init) { val cls = ctor.owner val classDef = cls.defTree - if (!classDef.isEmpty) { - given State = state.withOwner(cls) - if (ctor.isPrimaryConstructor) checkClassBody(classDef.asInstanceOf[TypeDef]) - else checkSecondaryConstructor(ctor) - } + if (!classDef.isEmpty) + state.withOwner(cls) { + if (ctor.isPrimaryConstructor) checkClassBody(classDef.asInstanceOf[TypeDef]) + else checkSecondaryConstructor(ctor) + } } def checkSecondaryConstructor(ctor: Symbol)(using state: State): Unit = traceOp("checking " + ctor.show, init) { diff --git a/tests/init/neg/t9115.scala b/tests/init/neg/t9115.scala new file mode 100644 index 000000000000..fe204e64c998 --- /dev/null +++ b/tests/init/neg/t9115.scala @@ -0,0 +1,8 @@ +object D { // error + def aaa = 1 //that’s the reason + class Z (depends: Any) + case object D1 extends Z(aaa) // 'null' when calling D.D1 first time // error + case object D2 extends Z(aaa) // 'null' when calling D.D2 first time // error + println(D1) + println(D2) +} \ No newline at end of file From b81361e2bb2b8c3ee739d6130f2917a1c4ed656d Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Fri, 26 Mar 2021 21:04:32 +0100 Subject: [PATCH 4/4] Add test --- .../tools/dotc/transform/init/CheckGlobal.scala | 4 ++++ tests/init/neg/t5366.scala | 15 +++++++++++++++ tests/init/neg/{t92661.scala => t9261.scala} | 0 3 files changed, 19 insertions(+) create mode 100644 tests/init/neg/t5366.scala rename tests/init/neg/{t92661.scala => t9261.scala} (100%) diff --git a/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala index 3ce0b6044226..11a88b5d348c 100644 --- a/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala +++ b/compiler/src/dotty/tools/dotc/transform/init/CheckGlobal.scala @@ -112,6 +112,10 @@ class CheckGlobal { else { val cdef = cls.defTree.asInstanceOf[TypeDef] val tpl = cdef.rhs.asInstanceOf[Template] + + // ignore separately compiled classes + if !tpl.unforced.isInstanceOf[List[_]] then return Nil + var dependencies: List[Dependency] = Nil val traverser = new TreeTraverser { override def traverse(tree: Tree)(using Context): Unit = diff --git a/tests/init/neg/t5366.scala b/tests/init/neg/t5366.scala new file mode 100644 index 000000000000..0d2fe6e9c384 --- /dev/null +++ b/tests/init/neg/t5366.scala @@ -0,0 +1,15 @@ +class IdAndMsg(val id: Int, val msg: String = "") + +case object ObjA extends IdAndMsg(1) // error +case object ObjB extends IdAndMsg(2) // error + +object IdAndMsg { // error + val values = List(ObjA , ObjB) +} + +object Test { + def main(args: Array[String]): Unit = { + ObjA + println(IdAndMsg.values) + } +} \ No newline at end of file diff --git a/tests/init/neg/t92661.scala b/tests/init/neg/t9261.scala similarity index 100% rename from tests/init/neg/t92661.scala rename to tests/init/neg/t9261.scala