Storing module reference in module causes resolve to stack overflow (500USD Bounty)
#3715
Comments
|
Thanks for the report. This is expected but not ideal. The way to work around this is to wrap the reference in a ModuleRef, and we should try to detect these infinite recursions and provide a proper error message suggesting that |
|
The proper way to refer to already existing modules is to use import mill._, scalalib._
trait CommonModule extends ScalaModule {
def scalaVersion = "2.13.12"
def m1 = ModuleRef(mym1)
def m2 = ModuleRef(mym2)
}
object mym1 extends M1
trait M1 extends CommonModule
object mym2 extends M2
trait M2 extends CommonModule {
override def moduleDeps = super.moduleDeps ++ Agg(m1())
}@lihaoyi We already try to detect cycles in |
|
Added a 500USD bounty if anyone wants to dig into this |
lihaoyi
changed the title
resolve to stack overflowresolve to stack overflow (500USD Bounty)
|
I'm taking a crack at this. |
|
I wrote the starts of two rudimentary tests. Although, I haven't made it much further. I was hoping to build something like I could be wrong, but I think as How can I check if two of these modules are the same and not just named the same and the same class? |
|
@myyk This is exactly the point where I stopped to investigate. Since this is reflection land, we can't know for sure whether the instance is meant to be a singleton. So, best is to always stick to the rule to use a |
|
We could try to look for patterns though. E.g. whenever we instantiate a module, we could look into the chain of parents. E.g. if we find the same types in a loop, multiple times (e.g. 3x), we should at least print a warning. So when we later run into a |
|
I think during module resolution, if the recursion "stack" contains the same module class twice, that's probably a good enough signal that we should error. Note that most modules are singleton Like @lefou suggests, we can probably say that all references to modules except their "primary" definition should be via ModuleRef, and therefore if during recursion we bump into the same exact class twice that should be grounds for raising an error |
|
If I'm not mistaken, we also do materialize the concrete module objects as well at some point after traversing their classes/method-names. But this only happens after the stackoverflow has already occurred, and so it would not be useful for us to report errors at this stage |
|
We do have |
Shouldn't those traits be wrapping those objects in |
Not if they meant to be concrete sub modules. |
Ah good point, that makes it a bit trickier since they will have the exact same class even if they are different instances. But even in that case, if the exact same class appears twice in the module stack during resolution, that is an indication that there is a loop in the resolution module graph, which is opens up the possibility of a stackoverflow. I think we should be able to safely disallow that and request that people use ModuleRef for such cases if they need references that do not convey a parent-child relationship |
|
I've caught some of the cyclic graphs but I'm not sure if my changes catches all of them. I added these test graphs: object CyclicModuleRefInitError extends TestBaseModule {
import mill.Agg
// See issue: https://github.com/com-lihaoyi/mill/issues/3715
trait CommonModule extends TestBaseModule {
def moduleDeps: Seq[CommonModule] = Seq.empty
def a = myA
def b = myB
}
object myA extends A
trait A extends CommonModule
object myB extends B
trait B extends CommonModule {
override def moduleDeps = super.moduleDeps ++ Agg(a)
}
}
object CyclicModuleRefInitError2 extends TestBaseModule {
// The cycle is in the child
def A = CyclicModuleRefInitError
}
object CyclicModuleRefInitError3 extends TestBaseModule {
// The cycle is in directly here
object A extends Module {
def b = B
}
object B extends Module {
def a = A
}
}
object CrossedCyclicModuleRefInitError extends TestBaseModule {
object cross extends mill.Cross[Cross]("210", "211", "212")
trait Cross extends Cross.Module[String] {
def suffix = Task { crossValue }
def c2 = cross2
}
object cross2 extends mill.Cross[Cross2]("210", "211", "212")
trait Cross2 extends Cross.Module[String] {
override def millSourcePath = super.millSourcePath / crossValue
def suffix = Task { crossValue }
def c1 = cross
}
}Can any of you think of any more test cases with cycles you'd like to see covered? |
|
@myyk I think that looks great. You covered the three major cases I can think of: "vanilla" circular references, circular references through inherited traits, and circular references through |
…#3878) This completes #3715 with all the cycle cases I could think of. It also touches `resolveDirectChildren` where I made some changes that I would help with readability. I thought I was going to need to build on those but didn't. These can be reverted if it's unwanted. --------- Co-authored-by: Li Haoyi <haoyi.sg@gmail.com>
From the maintainer Li Haoyi: I'm putting a 500USD bounty on this issue, payable by bank transfer on a merged PR implementing this.
The task is to appropriately detect module cycles during task resolution time, report a nice user-friendly error, and direct the user towards use of
ModuleRefto fix the issue. This should be covered by unit tests, and we should verify that the module cycles in the code sample below gets properly recognized and handled.I'm trying to work out a pattern that would allow the consumer of a foreign module swap out dependencies of modules. Given the following module definition:
One can then define another
build.sc, import the above, instantiate newM1andM2(for example with new Scala versions), and then have the newly instantiatedM2depend onM1:However this breaks
resolvewith a stack overflow error:After experimenting around a bit, it seems like the
def m1anddef m2module references are somehow picked up as submodules and thus resulted in an infinite loop. Wrapping the module references inSome(..)works just ok, however it's quite cumbersome to write and feels hacky. Can this be fixed somehow?The text was updated successfully, but these errors were encountered: