Make mill.define.Module a trait to allow abstract/virtual module lazy vals to be traits rather than classes

[lihaoyi]

The basic problem is that when Module was a class, and you had trait FooModule extends Module, Java reflection would not see that FooModule <: Module, and thus Reflect.reflectNestedObjects0[Module] which relies on java reflect would not detect def foo: FooModule as a module. This is because trait extends class is a Scala compiler concept, and does not exist in the JVM bytecode.

However, we do need Module to be a class, because we rely on implicit parameters to propagate context down the module tree. traits cannot take implicit parameters

The solution here is to make Module a trait, and make it inherit from a Module.BaseClass class that takes the implicit parameter and passes it to Module. Thus whenever we do reflection to find FooModule <: Module, it behaves expected, and we are still able to receive implicit parameters via Module.BaseClass

One side effect is that we need a way to mark things like JavaModule#zincWorker as not a child module. Previously, we skipped such modules during target resolution due to the buggy behavior above. Now, such modules get picked up, which is not what we want: in the common case, zincWorker is just a reference to the shared external module mill.scalalib.ZincWorkerModule, and is not a child module of each individual JavaModule. To do this, I introduce a mill.define.ModuleRef wrapper that simply provides a thin layer of indirection, letting us refer to the module via zincWorker() but blocking identification as a child-module since ModuleRef is not a sub-type of Module.

Tested via the mill.resolve.ResolveTests.overridenModule tests, which now pass when the abstract/overriden module is a trait, whereas previously they required that it be a class. Should also fix the flakiness we've been seeing in mill.resolve.ResolveTests.overridenModule.0.

[lihaoyi]

This is binary-incompatible with 0.11.0-M9, but probably worth getting in before 0.11.0-final. It fixes I believe the last edge case where someone would be forced to define a Module as a class, and not for any good reason but rather due to obscure edge case behavior in how the Scala compiler outputs Java classfiles

[lefou]

Is this only to be lazy, or is it fixing some early initialization stuff? If this is a (required) pattern, we should probably document it in Module.

[lihaoyi]

It doesn't need to be lazy, but I thought that's probably what we should tell people to do. After all, objects are lazy, so using lazy val for these would avoid unnecessarily changing the evaluation model. It also helps avoid NullPointerExceptions from popping up in surprising places, especially once multiple trait inheritance and overriding is involved. Not a problem with this specific definition, but definitely a potential problem in general

[lefou]

Explicit return type would be nice. Wouldn't be a normal (non-case) class enough?

[lihaoyi]

It would. It would just be a bit more boilerplate to define

[lefou]

I like this change. It especially improves the case where one needs to define some inner workers which typically work out of the box but need some customization in some rare cases, like ScoverageData. I think we should be clear whether the holder needs to be lazy to work correctly (avoid nulls) or if this is only for optimization.

[ckipp01]

Small suggestion, but could you be more meaningful with some of these names? For example in mill-scip I have some diffs now that look like

-              val zincWorker = sm.zincWorker.worker()
+              val zincWorker = sm.zincWorker.t.worker()

And it's really odd to have just t here.

[lefou]

I think, the envisioned usage is to use the def apply() = t.

-              val zincWorker = sm.zincWorker.worker()
+              val zincWorker = sm.zincWorker().worker()

[ckipp01]

Ahh duh,that makes more sense indeed.

[lolgab]

Shouldn't t be private then?

[lefou]

Doesn't it invalidate the case class contract then? E.g. when pattern matching.

[lihaoyi]

this doesn't need to be a case class i think, just need a trait with an apply method