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

Detect when resolve needs ModuleRef when likely cyclic references #3878

Merged
merged 18 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
feat: detect some cyclic references in resolve
  • Loading branch information
myyk committed Oct 30, 2024
commit 9fce280cf8c19d90d4ac36a0936586e1707c9cc3
47 changes: 30 additions & 17 deletions main/resolve/src/mill/resolve/ResolveCore.scala
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ private object ResolveCore {
m.cls,
None,
current.segments,
Nil
Nil,
Set.empty // TODO: We should pass the seenModules set
)

transitiveOrErr.map(transitive => self ++ transitive)
Expand All @@ -122,7 +123,8 @@ private object ResolveCore {
m.cls,
None,
current.segments,
typePattern
typePattern,
Set.empty // TODO: We should pass the seenModules set
)

transitiveOrErr.map(transitive => self ++ transitive)
Expand Down Expand Up @@ -244,26 +246,37 @@ private object ResolveCore {
cls: Class[_],
nameOpt: Option[String],
segments: Segments,
typePattern: Seq[String]
typePattern: Seq[String],
seenModules: Set[Class[_]],
): Either[String, Set[Resolved]] = {
val direct = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
val errOrDirect = resolveDirectChildren(rootModule, cls, nameOpt, segments, typePattern)
val directTraverse = resolveDirectChildren(rootModule, cls, nameOpt, segments, Nil)

val indirect0 = directTraverse match {
case Right(modules) =>
modules.flatMap {
case m: Resolved.Module =>
// TODO: Add cycle detection here
Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern))
case _ => None
}
case Left(err) => Seq(Left(err))
val errOrModules = directTraverse.map { modules =>
modules.flatMap {
case m: Resolved.Module => Some(m)
case _ => None
}
}

for {
d <- direct
i <- EitherOps.sequence(indirect0).map(_.flatten)
} yield d ++ i
if (seenModules.contains(cls)) {
Left(s"Cyclic module reference detected: ${cls.getName}, it's required to wrap it in ModuleRef. See documentation: https://mill-build.org/mill/0.12.1/fundamentals/modules.html#_abstract_modules_references")
} else {
val errOrIndirect0 = errOrModules match {
case Right(modules) =>
modules.flatMap { m =>
Some(resolveTransitiveChildren(rootModule, m.cls, nameOpt, m.segments, typePattern, seenModules + cls))
}
case Left(err) => Seq(Left(err))
}

val errOrIndirect = EitherOps.sequence(errOrIndirect0).map(_.flatten)

for {
direct <- errOrDirect
indirect <- errOrIndirect
} yield direct ++ indirect
}
}

private def resolveParents(c: Class[_]): Seq[Class[_]] =
Expand Down
8 changes: 3 additions & 5 deletions main/resolve/test/src/mill/main/ResolveTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1112,11 +1112,9 @@ object ResolveTests extends TestSuite {
}
test("cyclicModuleRefInitError") {
val check = new Checker(cyclicModuleRefInitError)
test - check(
"__",
Left(
"blah blah blah." // TODO: complete this
)
test - check.checkSeq0(
Seq("__"),
isShortError(_, "Cyclic module reference detected")
)
}
test("nonCyclicModules") {
Expand Down
12 changes: 6 additions & 6 deletions main/test/src/mill/util/TestGraphs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,18 @@ class TestGraphs() {

// The module names repeat, but it's not actually cyclic and is meant to confuse the cycle detection.
object nonCyclicModules extends TestBaseModule {
object A extends TestBaseModule {
object A extends Module {
def b = B
}
object B extends TestBaseModule {
object A extends TestBaseModule {
object B extends Module {
object A extends Module {
def b = B
}
def a = A

object B extends TestBaseModule {
object B extends TestBaseModule {}
object A extends TestBaseModule {
object B extends Module {
object B extends Module {}
object A extends Module {
def b = B
}
def a = A
Expand Down