Don't loose moduleDeps from super traits in PublishModule

[lolgab]

Instead of overriding moduleDeps to be PublishModules we iterate over the existing moduleDeps and we cast to PublishModules if they are of that type, otherwise we throw an exception with an error message to fix the problem.

• Fixes PublishModule deletes moduleDeps created by other traits #2082

Outdated docs

The first version of this PR was using a NoStackTrace exception. Now the following doesn't apply anymore

This is with the NoStackTrace custom exception:

lorenzo@MacBook-Pro-di-Lorenzo ~/s/snunit (bump-deps) [1]> ../mill/out/dev/launcher.dest/run --no-server show snunit-tapir[3.2.0].native.publishLocal
Compiling /Users/lorenzo/scala/snunit/versions.sc
Compiling /Users/lorenzo/scala/snunit/unitd.sc
Compiling /Users/lorenzo/scala/snunit/build.sc
[1/1] show 
1 targets failed
show mill.scalalib.PublishModule$Error: PublishModule moduleDeps need to be also PublishModules. snunit[3.2.0].native is not a PublishModule.

Since the Exception is create in the PublishModule class it's easy to locate where the error comes from.

This is with a normal Exception

lorenzo@MacBook-Pro-di-Lorenzo ~/s/snunit (bump-deps) [1]> ../mill/out/dev/launcher.dest/run --no-server show snunit-tapir[3.2.0].native.publishLocal
[1/1] show 
1 targets failed
show java.lang.Exception: PublishModule moduleDeps need to be also PublishModules. snunit[3.2.0].native is not a PublishModule.
        at mill.scalalib.PublishModule.$anonfun$moduleDeps$1(PublishModule.scala:19)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.scalalib.PublishModule.moduleDeps(PublishModule.scala:16)
        at mill.scalalib.PublishModule.moduleDeps$(PublishModule.scala:16)
        at ammonite.$file.build$SNUnitTapirModule$native$.moduleDeps(build.sc:148)
        at mill.scalalib.JavaModule.recursiveModuleDeps(JavaModule.scala:155)
        at mill.scalalib.JavaModule.recursiveModuleDeps$(JavaModule.scala:154)
        at ammonite.$file.build$SNUnitTapirModule$native$.recursiveModuleDeps(build.sc:148)
        at mill.scalalib.JavaModule.$anonfun$upstreamCompileOutput$1(JavaModule.scala:183)
        at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:454)
        at mill.moduledefs.Cacher.cachedTarget(Cacher.scala:10)
        at mill.moduledefs.Cacher.cachedTarget$(Cacher.scala:9)
        at mill.define.Module.cachedTarget(Module.scala:17)
        at mill.scalalib.JavaModule.upstreamCompileOutput(JavaModule.scala:182)
        at mill.scalalib.JavaModule.upstreamCompileOutput$(JavaModule.scala:182)
        at ammonite.$file.build$SNUnitTapirModule$native$.upstreamCompileOutput(build.sc:148)
        at mill.scalalib.ScalaModule.$anonfun$compile$1(ScalaModule.scala:208)
        at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:454)
        at mill.moduledefs.Cacher.cachedTarget(Cacher.scala:10)
        at mill.moduledefs.Cacher.cachedTarget$(Cacher.scala:9)
        at mill.define.Module.cachedTarget(Module.scala:17)
        at mill.scalalib.ScalaModule.compile(ScalaModule.scala:198)
        at mill.scalalib.ScalaModule.compile$(ScalaModule.scala:198)
        at ammonite.$file.build$SNUnitTapirModule$native$.compile(build.sc:148)
        at mill.scalalib.JavaModule.$anonfun$localClasspath$1(JavaModule.scala:308)
        at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:454)
        at mill.moduledefs.Cacher.cachedTarget(Cacher.scala:10)
        at mill.moduledefs.Cacher.cachedTarget$(Cacher.scala:9)
        at mill.define.Module.cachedTarget(Module.scala:17)
        at mill.scalalib.JavaModule.localClasspath(JavaModule.scala:307)
        at mill.scalalib.JavaModule.localClasspath$(JavaModule.scala:307)
        at ammonite.$file.build$SNUnitTapirModule$native$.localClasspath(build.sc:148)
        at mill.scalalib.JavaModule.$anonfun$jar$1(JavaModule.scala:420)
        at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:454)
        at mill.moduledefs.Cacher.cachedTarget(Cacher.scala:10)
        at mill.moduledefs.Cacher.cachedTarget$(Cacher.scala:9)
        at mill.define.Module.cachedTarget(Module.scala:17)
        at mill.scalalib.JavaModule.jar(JavaModule.scala:418)
        at mill.scalalib.JavaModule.jar$(JavaModule.scala:418)
        at ammonite.$file.build$SNUnitTapirModule$native$.jar(build.sc:148)
        at mill.scalalib.PublishModule.publishLocal(PublishModule.scala:108)
        at mill.scalalib.PublishModule.publishLocal$(PublishModule.scala:101)
        at ammonite.$file.build$SNUnitTapirModule$native$.publishLocal(build.sc:148)
        at ammonite.$file.build$.$anonfun$millDiscover$61(build.sc:61)
        at mainargs.Invoker$.$anonfun$invoke0$3(Invoker.scala:59)
        at mainargs.Result.flatMap(Result.scala:13)
        at mainargs.Result.flatMap$(Result.scala:12)
        at mainargs.Result$Success.flatMap(Result.scala:23)
        at mainargs.Invoker$.invoke0(Invoker.scala:58)
        at mainargs.Invoker$.invoke(Invoker.scala:70)
        at mill.main.Resolve$.$anonfun$invokeCommand$6(Resolve.scala:118)
        at mainargs.Result.flatMap(Result.scala:13)
        at mainargs.Result.flatMap$(Result.scala:12)
        at mainargs.Result$Success.flatMap(Result.scala:23)
        at mill.main.Resolve$.$anonfun$invokeCommand$5(Resolve.scala:114)
        at scala.collection.Iterator$$anon$9.next(Iterator.scala:584)
        at scala.collection.immutable.List.prependedAll(List.scala:153)
        at scala.collection.immutable.List$.from(List.scala:684)
        at scala.collection.immutable.List$.from(List.scala:681)
        at scala.collection.IterableOps$WithFilter.map(Iterable.scala:898)
        at mill.main.Resolve$.$anonfun$invokeCommand$3(Resolve.scala:105)
        at scala.collection.Iterator$$anon$10.nextCur(Iterator.scala:594)
        at scala.collection.Iterator$$anon$10.hasNext(Iterator.scala:608)
        at scala.collection.immutable.List.prependedAll(List.scala:152)
        at scala.collection.immutable.List$.from(List.scala:684)
        at scala.collection.immutable.List$.from(List.scala:681)
        at scala.collection.IterableFactory$Delegate.from(Factory.scala:288)
        at scala.collection.immutable.Iterable$.from(Iterable.scala:35)
        at scala.collection.immutable.Iterable$.from(Iterable.scala:32)
        at scala.collection.IterableOps$WithFilter.flatMap(Iterable.scala:901)
        at mill.main.Resolve$.invokeCommand(Resolve.scala:103)
        at mill.main.ResolveTasks$.endResolveLabel(ResolveTasks.scala:57)
        at mill.main.Resolve.resolve(Resolve.scala:188)
        at mill.main.Resolve.$anonfun$resolve$2(Resolve.scala:196)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.main.Resolve.recurse$1(Resolve.scala:196)
        at mill.main.Resolve.resolve(Resolve.scala:209)
        at mill.main.Resolve.$anonfun$resolve$2(Resolve.scala:196)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.main.Resolve.recurse$1(Resolve.scala:196)
        at mill.main.Resolve.resolve(Resolve.scala:260)
        at mill.main.Resolve.$anonfun$resolve$2(Resolve.scala:196)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.main.Resolve.recurse$1(Resolve.scala:196)
        at mill.main.Resolve.resolve(Resolve.scala:209)
        at mill.main.RunScript$.$anonfun$resolveTasks$7(RunScript.scala:249)
        at scala.util.Either.map(Either.scala:382)
        at mill.main.RunScript$.$anonfun$resolveTasks$6(RunScript.scala:233)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.main.RunScript$.$anonfun$resolveTasks$5(RunScript.scala:232)
        at scala.util.Either.flatMap(Either.scala:352)
        at mill.main.RunScript$.resolveTasks(RunScript.scala:229)
        at mill.main.RunScript$.$anonfun$resolveTasks$2(RunScript.scala:202)
        at scala.collection.immutable.List.map(List.scala:246)
        at scala.collection.immutable.List.map(List.scala:79)
        at mill.main.RunScript$.$anonfun$resolveTasks$1(RunScript.scala:201)
        at scala.util.Either.flatMap(Either.scala:352)
        at mill.main.RunScript$.resolveTasks(RunScript.scala:200)
        at mill.main.RunScript$.evaluateTasksNamed(RunScript.scala:331)
        at mill.main.MainModule$.evaluateTasksNamed(MainModule.scala:71)
        at mill.main.MainModule.$anonfun$show$1(MainModule.scala:276)
        at mill.define.Task$TraverseCtx.evaluate(Task.scala:380)
        at mill.eval.Evaluator.$anonfun$evaluateGroup$13(Evaluator.scala:627)
        at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
        at scala.Console$.withErr(Console.scala:193)
        at mill.eval.Evaluator.$anonfun$evaluateGroup$12(Evaluator.scala:627)
        at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
        at scala.Console$.withOut(Console.scala:164)
        at mill.eval.Evaluator.$anonfun$evaluateGroup$11(Evaluator.scala:626)
        at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
        at scala.Console$.withIn(Console.scala:227)
        at mill.eval.Evaluator.$anonfun$evaluateGroup$8(Evaluator.scala:625)
        at mill.eval.Evaluator.$anonfun$evaluateGroup$8$adapted(Evaluator.scala:586)
        at scala.collection.immutable.Vector.foreach(Vector.scala:1895)
        at mill.eval.Evaluator.evaluateGroup(Evaluator.scala:586)
        at mill.eval.Evaluator.$anonfun$evaluateGroupCached$21(Evaluator.scala:478)
        at scala.util.DynamicVariable.withValue(DynamicVariable.scala:59)
        at mill.eval.Evaluator.evaluateGroupCached(Evaluator.scala:469)
        at mill.eval.Evaluator.$anonfun$sequentialEvaluate$2(Evaluator.scala:202)
        at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:575)
        at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:573)
        at scala.collection.AbstractIterator.foreach(Iterator.scala:1300)
        at mill.eval.Evaluator.sequentialEvaluate(Evaluator.scala:177)
        at mill.eval.Evaluator.evaluate(Evaluator.scala:162)
        at mill.main.RunScript$.evaluateNamed(RunScript.scala:363)
        at mill.main.RunScript$.evaluate(RunScript.scala:349)
        at mill.main.RunScript$.$anonfun$evaluateTasks$1(RunScript.scala:314)
        at scala.util.Either.map(Either.scala:382)
        at mill.main.RunScript$.evaluateTasks(RunScript.scala:312)
        at mill.main.RunScript$.$anonfun$runScript$8(RunScript.scala:105)
        at ammonite.util.Res$Success.flatMap(Res.scala:62)
        at mill.main.RunScript$.runScript(RunScript.scala:104)
        at mill.main.MainRunner.$anonfun$runScript$1(MainRunner.scala:119)
        at mill.main.MainRunner.watchLoop2(MainRunner.scala:67)
        at mill.main.MainRunner.runScript(MainRunner.scala:92)
        at mill.MillMain$.main0(MillMain.scala:310)
        at mill.MillMain$.main(MillMain.scala:52)
        at mill.MillMain.main(MillMain.scala)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:566)
        at mill.main.client.IsolatedMillMainLoader.runMain(IsolatedMillMainLoader.java:58)
        at mill.main.client.MillClientMain.main(MillClientMain.java:64)

[lefou]

I'm not fully convinced by the extends NoStackTrace exception. It's maybe because I never used it before, but also, as it is loosing valuable information and it might not be obvious to users why this information is missing. I wouldn't expect that all users will find the issue easily, and certainly not tools.

Also, I think we should instead handle proper error message formatting some levels above (we currently don't). Loosing the stack trace without any needs (performance) smells like an too-early optimization.

IMHO, we should simply throw an exception with stack trace here. The error case is rare enough.

If you have more convincing arguments, I'm happy to hear about. But we should consider to log these information anyways in debug mode then.

[lolgab]

Ideally what I wanted was to return a Result.Failure with the error message only, not throw an exception.
Unfortunately in these cases where the result type is not a Task we can only throw exceptions.
I used the no stack trace exception because the stacktrace is huge and it can be overwhelming to scroll up to find the actual error message. But we can postpone this change for later, since this is a rare enough issue.

[lefou]

But we can postpone this change for later, since this is a rare enough issue.

Thanks. I typically handle exception in the application entry point where we can distinguish between known error (which we can format properly) and unexpected ones.

[lefou]

The change itself looks good to me. But we have failing MiMa checks. Any idea? Also, the PR summary is no longer correct.

[lolgab]

The change itself looks good to me. But we have failing MiMa checks. Any idea? Also, the PR summary is no longer correct.

I saw we already ignored some other ReversedMissingMethodProblem. I'm not 100% sure but I think we can ignore that issue, also I think it is impossible to fix this bug without calling super. Updating Mill should fix the binary compatibility problem in case this introduces one, but this bug is unsolvable without this change.

Updated the PR description ✅

[lefou]

Thank you!