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.

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

Check cycles on package level #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion acyclic/src-2/acyclic/plugin/Plugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,24 @@ class TestPlugin(val global: Global, cycleReporter: Seq[(Value, SortedSet[Int])]
val name = "acyclic"

var force = false
var forcePkg = false
var fatal = true

// Yeah processOptions is deprecated but keep using it anyway for 2.10.x compatibility
override def processOptions(options: List[String], error: String => Unit): Unit = {
if (options.contains("force")) {
force = true
}
if (options.contains("forcePkg")) {
forcePkg = true
}
if (options.contains("warn")) {
fatal = false
}
}
val description = "Allows the developer to prohibit inter-file dependencies"

val components = List[tools.nsc.plugins.PluginComponent](
new PluginPhase(this.global, cycleReporter, force, fatal)
new PluginPhase(this.global, cycleReporter, force, forcePkg, fatal)
)
}
4 changes: 4 additions & 0 deletions acyclic/src-2/acyclic/plugin/PluginPhase.scala
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class PluginPhase(
val global: Global,
cycleReporter: Seq[(Value, SortedSet[Int])] => Unit,
force: => Boolean,
forcePkg: => Boolean,
fatal: => Boolean
) extends PluginComponent { t =>

Expand All @@ -33,6 +34,7 @@ class PluginPhase(
private object base extends BasePluginPhase[CompilationUnit, Tree, Symbol] with GraphAnalysis[Tree] {
protected val cycleReporter = t.cycleReporter
protected lazy val force = t.force
protected lazy val forcePkg = t.forcePkg
protected lazy val fatal = t.fatal

def treeLine(tree: Tree): Int = tree.pos.line
Expand All @@ -50,6 +52,8 @@ class PluginPhase(
unit.body.collect { case x: PackageDef => x.pid.toString }.flatMap(_.split('.'))
def findPkgObjects(tree: Tree): List[Tree] = tree.collect { case x: ModuleDef if x.name.toString == "package" => x }
def pkgObjectName(pkgObject: Tree): String = pkgObject.symbol.enclosingPackageClass.fullName
def findPkgs(tree: Tree): List[Tree] = tree.collect { case x: PackageDef if x.symbol.fullName != "<empty>" => x }
def pkgName(pkg: Tree): String = pkg.symbol.fullName
def hasAcyclicImport(tree: Tree, selector: String): Boolean =
tree.collect {
case Import(expr, List(sel)) => expr.symbol.toString == "package acyclic" && sel.name.toString == selector
Expand Down
6 changes: 5 additions & 1 deletion acyclic/src-3/acyclic/plugin/Plugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class TestPlugin(cycleReporter: Seq[(Value, SortedSet[Int])] => Unit = _ => ())
val description = "Allows the developer to prohibit inter-file dependencies"

var force = false
var forcePkg = false
var fatal = true
var alreadyRun = false

Expand All @@ -22,7 +23,7 @@ class TestPlugin(cycleReporter: Seq[(Value, SortedSet[Int])] => Unit = _ => ())
override def run(using Context): Unit = {
if (!alreadyRun) {
alreadyRun = true
new acyclic.plugin.PluginPhase(cycleReporter, force, fatal).run()
new acyclic.plugin.PluginPhase(cycleReporter, force, forcePkg, fatal).run()
}
}
}
Expand All @@ -31,6 +32,9 @@ class TestPlugin(cycleReporter: Seq[(Value, SortedSet[Int])] => Unit = _ => ())
if (options.contains("force")) {
force = true
}
if (options.contains("forcePkg")) {
forcePkg = true
}
if (options.contains("warn")) {
fatal = false
}
Expand Down
12 changes: 12 additions & 0 deletions acyclic/src-3/acyclic/plugin/PluginPhase.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import dotty.tools.dotc.{CompilationUnit, report}
import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.core.Symbols.{NoSymbol, Symbol}
import dotty.tools.dotc.util.NoSource
import dotty.tools.dotc.core.Flags.Package

/**
* - Break dependency graph into strongly connected components
Expand All @@ -20,6 +21,7 @@ import dotty.tools.dotc.util.NoSource
class PluginPhase(
protected val cycleReporter: Seq[(Value, SortedSet[Int])] => Unit,
protected val force: Boolean,
protected val forcePkg: Boolean,
protected val fatal: Boolean
)(using ctx: Context) extends BasePluginPhase[CompilationUnit, tpd.Tree, Symbol], GraphAnalysis[tpd.Tree] {

Expand Down Expand Up @@ -53,6 +55,14 @@ class PluginPhase(
)
}

private val pkgAccumulator = new tpd.TreeAccumulator[List[tpd.Tree]] {
def apply(acc: List[tpd.Tree], tree: tpd.Tree)(using Context): List[tpd.Tree] =
foldOver(
if (tree.symbol.is(Package) && tree.symbol.fullName.toString != "<empty>") tree :: acc else acc,
tree
)
}

Comment on lines +58 to +65
Copy link
Author

@debs-sifive debs-sifive Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the pattern in pkgObjectAccumulator for this, but I'm not sure that the behavior is correct or efficient. The packages appear to be repeated many more times with the foldOver compared to collect. This is how the packages compare between this Scala 3 plugin and the Scala 2 plugin:

Scala 3

stdPackages
        Pkg(List(forcepkg, cyclicpackage, a))
        Pkg(List(forcepkg, cyclicpackage, a))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg))
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg))
        Pkg(List(scala, annotation)) <-- idk where these are coming from
        Pkg(List(scala))             <-- or if they should be here at all
        Pkg(List(acyclic))
        Pkg(List(acyclic))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg))
        Pkg(List(forcepkg, cyclicpackage, a))
        Pkg(List(forcepkg, cyclicpackage, a))

Scala 2

stdPackages
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage, a))
        Pkg(List(acyclic))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg, cyclicpackage, b))
        Pkg(List(forcepkg, cyclicpackage))
        Pkg(List(forcepkg, cyclicpackage, a))
        
        

private def hasAcyclicImportAccumulator(selector: String) = new tpd.TreeAccumulator[Boolean] {
def apply(acc: Boolean, tree: tpd.Tree)(using Context): Boolean = tree match {
case tpd.Import(expr, List(sel)) =>
Expand All @@ -71,6 +81,8 @@ class PluginPhase(
def unitPkgName(unit: CompilationUnit): List[String] = pkgNameAccumulator(Nil, unit.tpdTree).reverse.flatMap(_.split('.'))
def findPkgObjects(tree: tpd.Tree): List[tpd.Tree] = pkgObjectAccumulator(Nil, tree).reverse
def pkgObjectName(pkgObject: tpd.Tree): String = pkgObject.symbol.enclosingPackageClass.fullName.toString
def findPkgs(tree: tpd.Tree): List[tpd.Tree] = pkgAccumulator(Nil, tree).reverse
def pkgName(pkg: tpd.Tree): String = pkg.symbol.fullName.toString
def hasAcyclicImport(tree: tpd.Tree, selector: String): Boolean = hasAcyclicImportAccumulator(selector)(false, tree)

def extractDependencies(unit: CompilationUnit): Seq[(Symbol, tpd.Tree)] = DependencyExtraction(unit)
Expand Down
22 changes: 16 additions & 6 deletions acyclic/src/acyclic/plugin/BasePluginPhase.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import scala.collection.{mutable, SortedSet}
trait BasePluginPhase[CompilationUnit, Tree, Symbol] { self: GraphAnalysis[Tree] =>
protected val cycleReporter: Seq[(Value, SortedSet[Int])] => Unit
protected def force: Boolean
protected def forcePkg: Boolean
protected def fatal: Boolean

def treeLine(tree: Tree): Int
Expand All @@ -22,6 +23,8 @@ trait BasePluginPhase[CompilationUnit, Tree, Symbol] { self: GraphAnalysis[Tree]
def unitPkgName(unit: CompilationUnit): List[String]
def findPkgObjects(tree: Tree): List[Tree]
def pkgObjectName(pkgObject: Tree): String
def findPkgs(tree: Tree): List[Tree]
def pkgName(tree: Tree): String
def hasAcyclicImport(tree: Tree, selector: String): Boolean

def extractDependencies(unit: CompilationUnit): Seq[(Symbol, Tree)]
Expand All @@ -39,15 +42,22 @@ trait BasePluginPhase[CompilationUnit, Tree, Symbol] { self: GraphAnalysis[Tree]
} yield {
Value.File(unitPath(unit), unitPkgName(unit))
}

val acyclicPkgNames = for {
unit <- units
pkgObject <- findPkgObjects(unitTree(unit))
if hasAcyclicImport(pkgObject, "pkg")
} yield Value.Pkg(pkgObjectName(pkgObject).split('.').toList)
val stdPackages = if (forcePkg) packages else Seq.empty[Value.Pkg]
val acyclicPkgNames = packageObjects ++ stdPackages
(skipNodePaths, acyclicNodePaths, acyclicPkgNames)
}

private def packageObjects = for {
unit <- units
pkgObject <- findPkgObjects(unitTree(unit))
if hasAcyclicImport(pkgObject, "pkg")
} yield Value.Pkg(pkgObjectName(pkgObject).split('.').toList)

private def packages: Seq[Value.Pkg] = for {
unit <- units
pkgs <- findPkgs(unitTree(unit))
} yield Value.Pkg(pkgName(pkgs).split('.').toList)

final def runAllUnits(): Unit = {
val unitMap = units.map(u => unitPath(u) -> u).toMap
val nodes = for (unit <- units) yield {
Expand Down
6 changes: 6 additions & 0 deletions acyclic/test/resources/forcepkg/cyclicpackage/a/A1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package forcepkg.cyclicpackage
package a

class A1 extends b.B1{

}
5 changes: 5 additions & 0 deletions acyclic/test/resources/forcepkg/cyclicpackage/a/A2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package forcepkg.cyclicpackage.a

class A2 {

}
3 changes: 3 additions & 0 deletions acyclic/test/resources/forcepkg/cyclicpackage/b/B1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package forcepkg.cyclicpackage.b

class B1
4 changes: 4 additions & 0 deletions acyclic/test/resources/forcepkg/cyclicpackage/b/B2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package forcepkg.cyclicpackage
package b

class B2 extends a.A2
6 changes: 6 additions & 0 deletions acyclic/test/resources/forcepkg/simple/A.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package forcepkg.simple


class A {
val b: B = null
}
6 changes: 6 additions & 0 deletions acyclic/test/resources/forcepkg/simple/B.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package forcepkg.simple

class B {
val a1: A = new A
val a2: A = new A
}
6 changes: 4 additions & 2 deletions acyclic/test/src-2/acyclic/TestUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ object TestUtils extends BaseTestUtils {
path: String,
extraIncludes: Seq[String] = Seq("acyclic/src/acyclic/package.scala"),
force: Boolean = false,
forcePkg: Boolean = false,
warn: Boolean = false,
collectInfo: Boolean = true
): Seq[(String, String)] = {
Expand All @@ -47,6 +48,7 @@ object TestUtils extends BaseTestUtils {

val opts = List(
if (force) Seq("force") else Seq(),
if (forcePkg) Seq("forcePkg") else Seq(),
if (warn) Seq("warn") else Seq()
).flatten
if (opts.nonEmpty) {
Expand Down Expand Up @@ -78,13 +80,13 @@ object TestUtils extends BaseTestUtils {
storeReporter.map(_.infos.toSeq.map(i => (i.msg, i.severity.toString))).getOrElse(Seq.empty)
}

def makeFail(path: String, force: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*) = {
def makeFail(path: String, force: Boolean = false, forcePkg: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*) = {
def canonicalize(cycle: Seq[(Value, SortedSet[Int])]): Seq[(Value, SortedSet[Int])] = {
val startIndex = cycle.indexOf(cycle.minBy(_._1.toString))
cycle.toList.drop(startIndex) ++ cycle.toList.take(startIndex)
}

val ex = intercept[CompilationException] { make(path, force = force, collectInfo = false) }
val ex = intercept[CompilationException] { make(path, force = force, forcePkg = forcePkg,collectInfo = false) }
val cycles = ex.cycles
.map(canonicalize)
.map(
Expand Down
6 changes: 4 additions & 2 deletions acyclic/test/src-3/acyclic/TestUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ object TestUtils extends BaseTestUtils {
path: String,
extraIncludes: Seq[String] = Seq("acyclic/src/acyclic/package.scala"),
force: Boolean = false,
forcePkg: Boolean = false,
warn: Boolean = false,
collectInfo: Boolean = true
): Seq[(String, String)] = {
Expand All @@ -42,6 +43,7 @@ object TestUtils extends BaseTestUtils {

val opts = List(
if (force) Seq("force") else Seq(),
if (forcePkg) Seq("forcePkg") else Seq(),
if (warn) Seq("warn") else Seq()
).flatten

Expand Down Expand Up @@ -90,13 +92,13 @@ object TestUtils extends BaseTestUtils {
}))).getOrElse(Seq.empty)
}

def makeFail(path: String, force: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*) = {
def makeFail(path: String, force: Boolean = false, forcePkg: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*) = {
def canonicalize(cycle: Seq[(Value, SortedSet[Int])]): Seq[(Value, SortedSet[Int])] = {
val startIndex = cycle.indexOf(cycle.minBy(_._1.toString))
cycle.toList.drop(startIndex) ++ cycle.toList.take(startIndex)
}

val ex = intercept[CompilationException] { make(path, force = force, collectInfo = false) }
val ex = intercept[CompilationException] { make(path, force = force, forcePkg = forcePkg, collectInfo = false) }
val cycles = ex.cycles
.map(canonicalize)
.map(
Expand Down
22 changes: 22 additions & 0 deletions acyclic/test/src/acyclic/BaseCycleTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,28 @@ class BaseCycleTests(utils: BaseTestUtils) extends TestSuite {
))
test("pass") - make("force/simple")
test("skip") - make("force/skip", force = true)
test("mutualcyclic") - make("success/pkg/mutualcyclic", force = true)
}
test("forcepkg") - {
test("fail") - {
makeFail("forcepkg/cyclicpackage", force = false, forcePkg = true)(
Seq(
Pkg("forcepkg.cyclicpackage.b") -> SortedSet(4),
Pkg("forcepkg.cyclicpackage.a") -> SortedSet(4)
)
)
}
test("success") - {
make("forcepkg/simple", force = false, forcePkg = true)
}
test("fail") - {
makeFail("forcepkg/simple", force = true, forcePkg = true)(
Seq(
File("B.scala") -> SortedSet(4, 5),
File("A.scala") -> SortedSet(5)
)
)
}
}
}
}
3 changes: 2 additions & 1 deletion acyclic/test/src/acyclic/BaseTestUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ abstract class BaseTestUtils {
path: String,
extraIncludes: Seq[String] = Seq("acyclic/src/acyclic/package.scala"),
force: Boolean = false,
forcePkg: Boolean = false,
warn: Boolean = false,
collectInfo: Boolean = true
): Seq[(String, String)]

def makeFail(path: String, force: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*): Unit
def makeFail(path: String, force: Boolean = false, forcePkg: Boolean = false)(expected: Seq[(Value, SortedSet[Int])]*): Unit

case class CompilationException(cycles: Seq[Seq[(Value, SortedSet[Int])]]) extends Exception

Expand Down