Introduce Task.Command(exclusive = true) and convert Task.Persistent to Task(persistent = true)

[lihaoyi]

fixes #3566. Mostly a straightforward implementation of what was discussed.

We use Task.Command(exclusive = true) to make console/repl/clean/etc. run serially, all other commands should run in parallel. That includes most test commands. The name exclusive is taken from the Bazel action/target tag with the same meaning: that the tagged action runs alone with no other actions in parallel

Task.Persistent was changed to Task(persistent = true) for consistency, and also for other reasons: the new syntax is more composable, e.g. a user can more easily choose whether they want persistent = true or persistent = false based on a computed Boolean value, and Mill can in future extend it such that we can have Task(exclusive = true, persistent = true), Task.Command(exclusive = true, persistent = true), or other such combinations perhaps with even more flags (e.g. Bazel has a pretty long list https://bazel.build/reference/be/common-definitions#common.tags).

To make overload resolution work correctly, I make the first parameter of the (exclusive = true) or (persistent = true) parameter list dummy: NamedParameterOnlyDummy.type = NamedParameterOnlyDummy. This ensures that there is no normal value the user could pass in positionally that would select that overload of def Command or def apply, and the only way to select it is by passing in exclusive = true or persistent = true as a named parameter

Tested manually by adding printlns and making sure that leafSerialCommands no longer contains test tasks when I run tests in example/scalalib/basic/1-simple.

[lefou]

I'd prefer to explicitly denote commands that are not parallel capable, e.g. SerialCommand or InteractiveCommand. Since in Mill all other targets are suspected to run in parallel, having a ParallelCommand is irritating since it's pointing out the perceived normal ("beeing parallel"). Instead, we should explicitly point out the deviation from the rule.

Even in older Mill versions, we run all commands in parallel if the -j is different from 1.

[lefou]

It would be nice, if we could avoid the introduction of yet another target factory. Maybe, we could make it configurable via a parameter, e.g. Task.Command(parallel = false) { ...}. Don't know if the compiler can handle the overload and the implicit conversion at the same time, but it would be nice. Or we could make it Task.Command.serial { ... }. Either of both would also better reflect the fact, that the resulting type is still Command.

[lihaoyi]

@lefou I think Task.Command(serial = true) could be made to work, I originally went with a Task.ParallelCommand factory method because that's how we've handled things like T.persistent in the past (rather than T(persistent = true)). But maybe we can cut over to a flag-based approach since we're overhauling the syntax for 0.12.0 anyway. And it will scale better as more flags appear since you can mix and match the flags for any particular callsite

[lefou]

@lefou I think Task.Command(serial = true) could be made to work, I originally went with a Task.ParallelCommand factory method because that's how we've handled things like T.persistent in the past (rather than T(persistent = true)). But maybe we can cut over to a flag-based approach since we're overhauling the syntax for 0.12.0 anyway

Either that or the one with the dot notation. I think the flag-version is nicer (more idiomatic Scala), but if we provide a version without flags for conciseness accepting a parameter supposed to implicitly converted to Result and a version accepting two parameters, I could imagine, that the compiler will spit out misleading error messages if something is wrong.

[lefou]

@lefou I think Task.Command(serial = true) could be made to work, I originally went with a Task.ParallelCommand factory method because that's how we've handled things like T.persistent in the past (rather than T(persistent = true)). But maybe we can cut over to a flag-based approach since we're overhauling the syntax for 0.12.0 anyway. And it will scale better as more flags appear since you can mix and match the flags for any particular callsite

If we're doing this, we should review all target factories like Source, Sources and Input. But we should discuss this separately.

[lihaoyi]

@lefou I've updated the PR, the implementation to make it work is a bit hacky but seems to work and compiles/passes-tests

[lihaoyi]

I think Source, Sources, Input, etc. don't need support for flags yet. The only reason we need flags is if we have some behavioral change that does not change the type signature, as any change that affects the type signature has to be a new method. So persistent, exclusive, etc. can be flags, but changing from Source to Sources or Task to Task.Anon has to be a different method with a different signature. The current approach of defining a factory overload that takes named parameters can be applied to any of the other factory methods we have without issue

In theory we may find it useful to have T.Command(exclusive = true, persistent = true) or T(exclusive = true, persistent = true) and other such combinations, but I'll leave that for future follow ups to implement

[lefou]

I'd like to hear what @lolgab thinks about this new notation.

[bishabosha]

perhaps a SIP idea: an annotation that forces callsites to use named arguments?

[bishabosha]

or some new syntax at the declaration anyway

[lihaoyi]

Yeah that's a good idea!

[bishabosha]

why shouldn't this be a curried function?

[lihaoyi]

IIRC Scala 2 macros had an issue where they didnt allow you to call their parameters with explicit names. Actually I'm not sure if that still applies in 2.13.15 or 3.5.0, but it did at some point. Lemme try again to verify

[bishabosha]

works in scala 3.5 anyway:

scala> def mcr[T: Type](fn: Expr[Either[String, T]], caller: Expr[String], exclusive: Expr[Boolean])(using Quotes): Expr[Either[String, T]] = {
     |   import scala.concurrent.Future
     |   import scala.concurrent.ExecutionContext.Implicits.global
     |   import scala.concurrent.Await
     |   import scala.concurrent.duration.Duration
     |
     |   val exc = exclusive.valueOrAbort
     |   if exc then '{ println($caller); println("EXCLUSIVE"); $fn }
     |   else '{ println($caller); val fut = Future({Thread.sleep(1000); $fn}); Await.result(fut, Duration.Inf) }
     | }

scala> {
     | inline def task[T](inline op: Either[String, T]) = ${ mcr[T]('op, '{"original"}, 'false) }
     | inline def task[T](ignore: Int = -1, inline exclusive: Boolean = false)(inline op: Either[String, T]) = ${ mcr[T]('op, '{"overload"}, 'exclusive) }
     | }

scala> task(exclusive = true) { println(1 + 1); Right(23) }
overload
EXCLUSIVE
2

scala> task(exclusive = false) { println(1 + 1); Right(23) }
overload
2

scala> task{ println(1 + 1); Right(23) }
original
2
val res21: Either[String, Int] = Right(23)

[lihaoyi]

Yeah looks like it's still a limitation in 2.13.15

[14186] [error] /Users/lihaoyi/Github/mill/main/src/mill/main/MainModule.scala:403:17: macro applications do not support named and/or default arguments
[14186] [error]     Task.Command(exclusive = true) {
[14186] [error]                 ^

So likely we'll go with the boilerplate-y CommandFactory so this can land in 0.12.0, and in 0.13.0 we can convert to normal curried functions as one of the benefits of being on Scala 3.x. Doesn't need to be immediately, since the CommandFactory approach should work in Scala 3 as well I think

[lefou]

@lihaoyi Shouldn't this be a true or a !serialCommandExec?

Maybe, serialCommandExec isn't need anymore, since we now have fine control over exclusive execution.