Skip to content

Commit

Permalink
Make Target type abstract to allow overriding by different concrete…
Browse files Browse the repository at this point in the history
… implementations (#2402)

This PR makes all of the `T.{apply, input, source, sources, persistent}`
functions return the same `Target` type, so that you can easily override
one with another. `T.{command, worker}` are still distinct types.

## Motivation

This allows us to override one with another, e.g. override a `T.sources`
with a `T.apply` if we want to replace source files with computed
sources.

```scala
trait Foo extends Module{
  def thing = T.source(millSourcePath / "foo.txt")
}
trait Bar extends Foo{
  def thing = T{ 
    os.write(T.dest / "foo.txt", "hello")
    PathRef(T.dest / "foo.txt") 
  }
}
```

Currently, the workaround is to make `T.source` do the computation, as
follows:

```scala
trait Foo extends Module{
  def thing = T.source(millSourcePath / "foo.txt")
}
trait Bar extends Foo{
  def thing = T.source{ 
    os.write(T.dest / "foo.txt", "hello")
    PathRef(T.dest / "foo.txt") 
  }
}
```

But the status quo is wasteful: it would begin hashing `foo.txt` at the
start of every evaluation, it will watch `foo.txt` for changes, etc..
Even though we know it could never change since it's a generated file.
This is because we are not allowed to change the method type of `Source`
to `T[PathRef]` during the override, and thus we have to preserve all
the `Source` characteristics even though we know they no longer apply

With this PR, we can simply replace the `T.sources` with a `T{...}`, and
Mill makes use of that information to avoid hashing/watching the
`PathRef` unnecessarily

## Implementation

1. `NamedTask` and `NamedTaskImpl` were merged

2. `Target`'s logic was mostly hoisted into `NamedTask`, leaving
`Target` an empty marker trait

3. `TargetImpl` is unchanged

4. `Input`, `Source` and `Sources` have been renamed `InputImpl`,
`SourceImpl`, and `SourcesImplt`

5. All the functions that used to return
`Source`/`Sources`/`Persistent`/etc. now return the same type `Target`,
meaning that we can easily override one with the other.

6. I added stubs in `mill/define/package.scala` to make existing type
annotations `: Sources`, `: Input`, etc. continue to work as type
aliases to `Target[T]`, and our large codebase and test suite required
relatively few changes

7. `Command`/`T.command` and `Worker`/`T.worker` continue to return
their specific type `Command[T]`/`Worker[T]`, since they are not
sub-types of `Target[T]`.


The `Task` type hierarchy is considerably flatter and simpler:

Before:

- `Task`
- `Task.Sequence`, `Task.TraverseCtx`, `Task.Mapped`, `Task.Zipped`,
`T.task`, etc.
    - `NamedTask`
        - `NamedTaskImpl`
            - `Command`
            - `Worker`
            - `Input`
                - `Source`
                - `Sources`
    - `Target`
        -  `TargetImpl` (also inherits from `NamedTaskImpl`)
            - `Persistent`


After:

- `Task`
- `Task.Sequence`, `Task.TraverseCtx`, `Task.Mapped`, `Task.Zipped`,
`T.task`, etc.
    - `NamedTask`
        - `Command`
        - `Worker`
        - `Target`
            -  `TargetImpl`
                - `PersistentImpl`
            - `InputImpl`
                - `SourceImpl`
                - `SourcesImpl`



## Testing

Added some tests to `TaskTests.scala` to demonstrate and validate the
behavior when people override with different types. This was previously
a compile error

I had to update the error message for the wrong number of param lists,
from `T{...} definitions must have 0 parameter lists` to `Target
definitions must have 0 parameter lists`, since we no longer know
*which* target sub-type a method returns based on its signature

## Notes

- Despite the overhaul of the type hierarchy, this should mostly be a
transparent change. Hopefully it would be minimally source-incompatible
with existing code, so even though there's a huge bin-compat breakage
here, people upgrading shouldn't be too affected.

Pull request: #2402
  • Loading branch information
lihaoyi committed Apr 3, 2023
1 parent 2abecd5 commit 2a7d77d
Show file tree
Hide file tree
Showing 16 changed files with 228 additions and 180 deletions.
2 changes: 1 addition & 1 deletion example/web/4-webapp-scalajs/build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ object app extends RootModule with ScalaModule{
ivy"com.lihaoyi::scalatags:0.12.0"
)

def resources = T.sources{
def resources = T{
os.makeDir(T.dest / "webapp")
val jsPath = client.fastLinkJS().dest.path
// Move the main.js[.map] files into the proper filesystem position
Expand Down
2 changes: 1 addition & 1 deletion example/web/5-webapp-scalajs-shared/build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ object app extends RootModule with AppScalaModule{

def ivyDeps = Agg(ivy"com.lihaoyi::cask:0.9.0")

def resources = T.sources{
def resources = T{
os.makeDir(T.dest / "webapp")
val jsPath = client.fastLinkJS().dest.path
os.copy(jsPath / "main.js", T.dest / "webapp" / "main.js")
Expand Down
18 changes: 6 additions & 12 deletions main/core/src/mill/define/Discover.scala
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,14 @@ object Discover {
cases: (Type, Int, String)*
): Unit = {
for (m <- methods.toList) {
for ((tt, n, label) <- cases) {
if (
m.returnType <:< tt &&
m.paramLists.length != n
) {
c.abort(
cases
.find{case (tt, n, label) => m.returnType <:< tt}
.foreach{case (tt, n, label) =>
if (m.paramLists.length != n) c.abort(
m.pos,
s"$label definitions must have $n parameter list" + (if (n == 1) "" else "s")
)
}
}
}
}
val mapping = for {
Expand All @@ -84,11 +81,8 @@ object Discover {
overridesRoutes = {
assertParamListCounts(
methods,
(weakTypeOf[mill.define.Sources], 0, "`T.sources`"),
(weakTypeOf[mill.define.Input[_]], 0, "`T.input`"),
(weakTypeOf[mill.define.Persistent[_]], 0, "`T.persistent`"),
(weakTypeOf[mill.define.Target[_]], 0, "`T{...}`"),
(weakTypeOf[mill.define.Command[_]], 1, "`T.command`")
(weakTypeOf[mill.define.Command[_]], 1, "`T.command`"),
(weakTypeOf[mill.define.Target[_]], 0, "Target"),
)

for {
Expand Down
Loading

0 comments on commit 2a7d77d

Please sign in to comment.