Fix incorrect prepareOffline in AssemblyModule

[unlsycn]

prepareOffline method should be a member of the trait, otherwise it will not be evaluated by downstream modules.

[lihaoyi]

Looks like we'll have to leave the trait on the companion object to preserve binary compatibility (hence the mima failure)

[unlsycn]

Looks like we'll have to leave the trait on the companion object to preserve binary compatibility (hence the mima failure)

Mima would still fail even if we keep the prepareOfflinemethod in the companion object:

* static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule is non-static in current version

It seems to be impossible to make this change without breaking binary compatibility..Since this method should not be static yet it has been in the current version.

[lefou]

This is really unfortunate. Esp. since we probably never need the conveniently created static forwarder. (Would be great to know a way to disable this opportunistic behavior of the scala compiler for task-defs in external modules.)

If we're going to say that we want ignore static method binary compatibility (since we don't expect or support non-scala API clients or reflective access) like this:

+trait AssemblyModuleOfflineSupport extends OfflineSupportModule {
+  override def prepareOffline(all: mainargs.Flag): Task.Command[Seq[PathRef]] = Task.Command {
+    AssemblyModule.prepareOffline(all)()
+  }
+}

-trait AssemblyModule extends mill.api.Module with OfflineSupportModule {
+trait AssemblyModule extends AssemblyModuleOfflineSupport {

we could produce a different MiMa error:

> mill libs.javalib.mimaReportBinaryIssues
[503/503] libs.javalib.mimaReportBinaryIssues
[503] Scanning binary compatibility in /home/lefou/work/opensource/mill/out/libs/javalib/jar.dest/out.jar ...
[503] Found 1 issue when checking against com.lihaoyi:mill-libs-javalib_3:VersionConstraint.Lazy(1.0.0, [unparsed])
[503]  * static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule does not have a correspondent in current version
[503]    filter with: ProblemFilter.exclude[DirectMissingMethodProblem]("mill.javalib.AssemblyModule.prepareOffline")

Which could likely be safe to be ignored and excluded.

[unlsycn]

This is really unfortunate. Esp. since we probably never need the conveniently created static forwarder. (Would be great to know a way to disable this opportunistic behavior of the scala compiler for task-defs in external modules.)

If we're going to say that we want ignore static method binary compatibility (since we don't expect or support non-scala API clients or reflective access) like this:

+trait AssemblyModuleOfflineSupport extends OfflineSupportModule {
+  override def prepareOffline(all: mainargs.Flag): Task.Command[Seq[PathRef]] = Task.Command {
+    AssemblyModule.prepareOffline(all)()
+  }
+}

-trait AssemblyModule extends mill.api.Module with OfflineSupportModule {
+trait AssemblyModule extends AssemblyModuleOfflineSupport {

we could produce a different MiMa error:

> mill libs.javalib.mimaReportBinaryIssues
[503/503] libs.javalib.mimaReportBinaryIssues
[503] Scanning binary compatibility in /home/lefou/work/opensource/mill/out/libs/javalib/jar.dest/out.jar ...
[503] Found 1 issue when checking against com.lihaoyi:mill-libs-javalib_3:VersionConstraint.Lazy(1.0.0, [unparsed])
[503]  * static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule does not have a correspondent in current version
[503]    filter with: ProblemFilter.exclude[DirectMissingMethodProblem]("mill.javalib.AssemblyModule.prepareOffline")

Which could likely be safe to be ignored and excluded.

Yes, this is actually the first commit did and ran into with😔.

[lefou]

Maybe another take away could be to avoid external modules having the same name as their traits. We could conveniently have some suffix that we auto-strip/append when working from the CLI.

[lefou]

Yes, this is actually the first commit did and ran into with😔.

This is not exactly the same as in the first commit, since the def prepareOffline is still present in the object AssemblyModule, it's just no longer marked static in the bytecode.

[lihaoyi]

I think @scala.annotation.static is now available in Scala 3 to force the creation of a static forwarder. Could that be used to fix the binary compatibility issue?

[lefou]

I think @scala.annotation.static is now available in Scala 3 to force the creation of a static forwarder. Could that be used to fix the binary compatibility issue?

I think no, as in Java bytecode all methods share the same namespace, and the non-static method (in the trait) has the same signature.

The method ended up being a static method because the compiler "thought" it might be a good idea. Technically, methods of Scala objects aren't static, but it better matched the static concept of the Java world. So this is probably done for better interoperability.

If I'm not mistaken, the static methods in Foo just forwards to the non-static method in the Foo$ class.

[unlsycn]

Yes, this is actually the first commit did and ran into with😔.

This is not exactly the same as in the first commit, since the def prepareOffline is still present in the object AssemblyModule, it's just no longer marked static in the bytecode.

You are right, I see it. Thanks for your guidance and I am terribly sorry fir the delayed reply (I have been busy preparing for an exam🥲)

[unlsycn]

This is really unfortunate. Esp. since we probably never need the conveniently created static forwarder. (Would be great to know a way to disable this opportunistic behavior of the scala compiler for task-defs in external modules.)

If we're going to say that we want ignore static method binary compatibility (since we don't expect or support non-scala API clients or reflective access) like this:

+trait AssemblyModuleOfflineSupport extends OfflineSupportModule {
+  override def prepareOffline(all: mainargs.Flag): Task.Command[Seq[PathRef]] = Task.Command {
+    AssemblyModule.prepareOffline(all)()
+  }
+}

-trait AssemblyModule extends mill.api.Module with OfflineSupportModule {
+trait AssemblyModule extends AssemblyModuleOfflineSupport {

we could produce a different MiMa error:

> mill libs.javalib.mimaReportBinaryIssues
[503/503] libs.javalib.mimaReportBinaryIssues
[503] Scanning binary compatibility in /home/lefou/work/opensource/mill/out/libs/javalib/jar.dest/out.jar ...
[503] Found 1 issue when checking against com.lihaoyi:mill-libs-javalib_3:VersionConstraint.Lazy(1.0.0, [unparsed])
[503]  * static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule does not have a correspondent in current version
[503]    filter with: ProblemFilter.exclude[DirectMissingMethodProblem]("mill.javalib.AssemblyModule.prepareOffline")

Which could likely be safe to be ignored and excluded.

I wonder about the difference between the two MiMa errors:

* static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule is non-static in current version

and

* static method prepareOffline(mainargs.Flag)mill.api.Task#Command in interface mill.javalib.AssemblyModule does not have a correspondent in current version

The only visible distinction seems to be whether the method signature exists in the interface AssemblyModule. And imo both errors can likely be safely ignored.

[lefou]

To summarize:

The current AssemblyModule does not come with a proper prepareOffline task, risking the next mill --offline run to fail.

The proper solution (add prepareOffline task to AssemblyModule) isn't fully binary compatible. We need to compromize:

1. Ignore binary compatibility of the static forwarder method prepareOffline in AssemblyModule. It won't break classes deriving trait AssemblyModule since, it's static, so this is likely "good enough" for Mill. There are still chances of breakages due to unintended use, hence we should properly communicate in the changelog.

2. Work around and limit the impact, by invoking the required preparations from known sub-classes (e.g. JavaModule). While preserving full binary compatibility, we increase the maintenance burden and decrease code cleanliness, while still risking an incomplete offline preparation in rare cases, e.g. where users directly extend AssemblyModule.

To move forward, let's agree on the best solution here. @lihaoyi @vaslabs @lolgab

[vaslabs]

I need some time to read more on this but I got flashbacks from kamon-io/Kamon#1191 😅

EDIT:

basically, for companion objects (with their methods being compiled to static) the scala 2 vs scala 3 difference is summarised here and this feels related.

[lefou]

This is really unfortunate. Esp. since we probably never need the conveniently created static forwarder. (Would be great to know a way to disable this opportunistic behavior of the scala compiler for task-defs in external modules.)

To give more details on this topic. You can use -Xno-forwards flag, to tell the Scala compiler to not generate static forwarders. This, combined with the @static annotation seems to be the preferable options we want to use in Mill. That way, we would never risk that any object pollutes the namespace of its companion class/trait.

The downside is, that main methods need to be explicitly marked with @static to work as expected. Also, this can't be enabled afterwards, without manually adding a lot of @static annotations to preserve binary compatibility.

[lihaoyi]

what if we made AssemblyModule#prepareOffline a submodule rather than a task? It could have its own defaultTask so that __.prepareOffline works, but that might let us avoid the conflict with the static def prepareOffline method on AssemblyModule

[lefou]

what if we made AssemblyModule#prepareOffline a submodule rather than a task? It could have its own defaultTask so that __.prepareOffline works, but that might let us avoid the conflict with the static def prepareOffline method on AssemblyModule

My first thought was, that it will clash due to the same name, but we should try it. Looks like a doable compromise. It will definitely needs some in-code comments.

[lefou]

On a second thought, having a prepareOffline submodule in each JavaModule isn't nice in many ways, since it will show up in various outputs and shell completion.

[lihaoyi]

what if we kept it as a command, but gave it a different signature? Like we could give it a , dummy: Int = 0 parameter so it has a different signature from the existing method and won't collide

[lefou]

In that case, it wouldn't implement the trait OfflineSupport anymore.