Make `T` an alias for `Task` instead of `Target`, move `T.` operations to `Task.` #3356

lihaoyi · 2024-08-08T20:45:00Z

This PR attempts to fix #3338:

T is now an alias for Task instead of Target. We expect to use foo: Task[V], Task { ... }, Task.dest, etc. APIs going forward, but all existing APIs remain on the T aliases for backwards source compatibility
Task.apply or Task { ... } continues to return a Target instance, but now is typed as returning the supertype Task. This allows people to write def foo: Task[T] = Task { ... } and have it do the right thing, rather than the otherwise necessary def foo: Target[T] = Task { ... } which is pretty awkward
In order to support using Task[T] everywhere as types, I flattened out the type hierarchy further: now, most APIs work directly with Task instead of Target or NamedTask
1. Task now supports .ctx, which throws an UnsupportedOperationException by default. NamedTask overrides it with an implementation.
2. Rather than relying on methods returning Target[_] for resolution, we now rely on methods returning Task[_] with zero argument lists. As the number of argument lists is not available through Java reflect (we can only see the total number of arguments), I rely on information collected by the Discover[T] macro.
I made Input[_], Source[_], Target[_], etc. type aliases of Task[_]. This avoids issues where people might accidentally declare def foo: Target[V] and find themselves unable to override it later as type def foo: Task[V] = Task { ... }. This is similar to how Make Target type abstract to allow overriding by different concrete implementations #2402 earlier made Input Source etc. type aliases of Target[_], and serves a similar purpose.
Distinguishing of cached targets (zero argument lists) with uncached other things (1 or more argument lists) is done via the information we already collect in the Discover macro. For safety, I made the def millDiscover abstract, because it cannot really be implemented in abstract superclasses or traits (due to is macro implementation) so it really needs to be implemented in every concrete object. This should avoid a common issue where we forget to override it in the test suite and command resolution wouldn't work.

The flattening out of the Task[_] type hierarchy is a compromise. def ctx: Ctx = throws on the Task trait is gross. We lost the ability to distinguish between various subtypes of Task based on method return types, and instead have to go through other ad-hoc mechanisms like number-of-argument-lists. But in exchange for a bit of sloppiness in the type signatures, users can now use Task{ ... } to construct their most common tasks (targets), : Task[V] to annotate their types, and Task.sources and Task.dest for most common task APIs

This is a binary incompatible change and will have to go into 0.13.0. I tried to leave the existing T/Target/Input/etc. aliases in place, so hopefully it's mostly source compatible (though probably not 100%)

This PR is a bit messy, but important files to review include:

main/src/mill/package.scala, which re-writes the various type and val aliases
main/define/src/mill/define/Task.scala, which moves the various T.* operations from object Target onto a shared trait TaskCompanion that can be shared with object Task, and makes them all return Task[_] instead of a more specific subtype
main/define/src/mill/define/Discover.scala, which tweaks the class-task-name-discovery logic to make it look for Task instead of Target, and make it precise enough to work as the primary resolution mechanism (previously there were some bugs, that didn't matter when it was just used for error message reporting, but weren't acceptable now that it is becoming the primary way we find tasks in a module)
main/resolve/src/mill/resolve/ResolveCore.scala, which converts the task resolution logic to use the information already collected by the updated Discover macro
main/define/src/mill/define/BaseModuleTree.scala, which pre-computes a bunch of values used throughout the task resolution logic
main/resolve/src/mill/resolve/Resolve.scala has more changes to use BaseModuleTree pre-computed values

lefou · 2024-08-09T08:32:16Z

main/define/test/src/mill/define/ApplicativeTests.scala

@@ -28,7 +28,7 @@ object ApplicativeTests extends TestSuite {
      value
    }
  }
-  @compileTimeOnly("Target.ctx() can only be used with a T{...} block")
+  @compileTimeOnly("Target.ctx() can only be used with a Task {...} block")


Suggested change

@compileTimeOnly("Target.ctx() can only be used with a Task {...} block")

@compileTimeOnly("Target.ctx() can only be used within a Task {...} block")

This doesn't work yet; need to make changes in the mill-moduledefs repo. TBH I feel like just in-sourcing it again, since even when in-sourced people can depend on the artifact separately if necessary, and it makes doing these changes in one PR a lot more convenient

Since it's a compiler module, it needs releases for every Scala version we want to use. That's the main reason we out-sourced it. We post-release for every new Scala 2.13 version.

It might be ok for a development version to use a in-sourced patched version, but I think the goal should be to push any changes back to the external project before we reach a stable version.

How about we in-source it to the Mill repo but still keep the publishing separate? That would mean it's treated like the bridge modules, and would keep everything in one place while still preserving the per-scala-version releases and separate publishing

That will work too, if it's well documented. We do these things only occasionally, there is no muscle memory for the process, so we need a good step-by-step documentation targeting the developer/publisher.

lefou

Why is all the mill-moduledefs meta info removed. It does still have a dedicated release cycle, so we should manage it via the build.sc and generated BuildInfo.

If we release it with every Mill release, we also need to post-release for newer Scala versions all those plugins. For example, external plugin authors need to build against older Mill versions to ensure they don't accidentally use newer API. But they sometimes want and sometimes must update the Scala version.

lefou · 2024-08-10T09:19:40Z

main/moduledefs/src/NullaryMethod.java

+@Target({ ElementType.TYPE, ElementType.METHOD })
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+public @interface NullaryMethod {}


API doc needed. What is @NullaryMethod for?

lihaoyi · 2024-08-10T14:02:24Z

I think I've come up with a way go avoid modifying the compiler plugims by moving the info tonthe discover macro. That will let us avoid having to republish them, and also avoid failure modes of people forgetting to include the plugins in their published mill plugins. Will see if i can make it work

…3565) Essentially a minimized binary-compatible version of #3356, renaming all the factory methods for various tasks to follow more standard naming conventions: * `T {...}` -> `Task {...}` * `T.command {...}` -> `Task.Command {...}` * `T.input {...}` -> `Task.Input {...}` * `T.source {...}` -> `Task.Source {...}` * `T.sources {...}` -> `Task.Sources {...}` * `T.persistent {...}` -> `Task.Persistent {...}` * `T.task {...}` -> `Task.Anon {...}` The type `T[_]` remains an alias for `Target[_]`, and `Task{ ... }` returns a `T[_]`, to maintain binary compatibility. Not quite ideal but can probably be hand-waved away until Mill 0.13.0 when we are allowed to break binary compatibility. All the `T.*` operations have been duplicated to `Task.*` by sharing them via a `trait TargetBase`, except the factory methods which were copied over and upper-cased while the old version deprecated. I have updated all the code and examples to use `Task` instead of `T` where relevant. The only exceptions are the `implicit def apply`s which needed to be manually copied without the `implicit` (otherwise the multiple implicits cause ambiguity). This gets us most of the user-facing benefits of #3356 without the bin-compat breakage: users no longer see an odd `T { ... }` syntax in the docs and in their build files, and now see `Task { ... }` which should be much more familiar. Although it does not allow us to do the type-hierarchy cleanups that the other PR provides, it's still worth doing so we can get it in in 0.12.0 The old `T { ... }` and `T.*` syntaxes should continue to work, and are exercised via the bootstrap tests as they continue to be used in Mill's own build. This PR should be source compatible to avoid migration pains, and given the prevalence of `T` everywhere we probably should just support it forever

lihaoyi added 7 commits August 8, 2024 13:43

.

baecec8

.

692bcd3

task -> Task

c8293e7

fix

ed2355b

fix

866c4e1

.

06bf3f1

.

92a372b

lihaoyi changed the title ~~[WIP] Experiment with renaming T to task~~ Move T.* operations to Task.* Aug 8, 2024

lefou reviewed Aug 9, 2024

View reviewed changes

lihaoyi added 4 commits August 9, 2024 11:15

insource scalac plugins

4a82ee8

wip trying to get nullary method task discovery working

e1bf391

wip

036a4c7

.

c67b7c0

lihaoyi changed the title ~~Move T.* operations to Task.*~~ Make T an alias for Task instead of Target, move T.* operations to Task.* Aug 9, 2024

lihaoyi added 8 commits August 9, 2024 16:21

hack in support for val targets for testing purposes

7bfe0eb

ignore non-targets in module.targets

9f1e0b6

wip

f0ccd21

tweak

a5df563

.

4db8868

.

a519243

.

ab3af31

.

0fb9457

lefou reviewed Aug 10, 2024

View reviewed changes

lihaoyi added 6 commits August 10, 2024 08:37

main.resolve.test passes with Discover-based target selection

75113bb

fix

42d374c

handle edge case

2afff74

add explicit discover calls everywhere

69879e3

.

51e8884

refix methodCodeHashSignatures

559d9f6

lihaoyi added 2 commits August 10, 2024 11:05

.

ad14dc8

Completely replace reflection-based target discovery with discover macro

488c2cb

lihaoyi force-pushed the task-name branch from 023156f to 488c2cb Compare August 10, 2024 18:25

.

edaaf12

lihaoyi added this to the 0.13.0 milestone Aug 10, 2024

lihaoyi mentioned this pull request Aug 10, 2024

[RFC] Rename T{...} and T[_] to task{...} and Task[_], standardize "Task"/"Target" terminology around "Task" #3338

Open

.

022a08b

This was referenced Sep 16, 2024

Make ./mill without any arguments point you towards --help, flesh out --help into a cheat sheet #3556

Merged

Use Task instead of T when constructing targets and other tasks #3565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `T` an alias for `Task` instead of `Target`, move `T.` operations to `Task.` #3356

Make `T` an alias for `Task` instead of `Target`, move `T.` operations to `Task.` #3356

lihaoyi commented Aug 8, 2024 •

edited

Loading

lefou Aug 9, 2024

lihaoyi Aug 9, 2024

lefou Aug 9, 2024 •

edited

Loading

lihaoyi Aug 9, 2024

lefou Aug 10, 2024 •

edited

Loading

lefou left a comment •

edited

Loading

lefou Aug 10, 2024

lihaoyi commented Aug 10, 2024

	@compileTimeOnly("Target.ctx() can only be used with a Task {...} block")
	@compileTimeOnly("Target.ctx() can only be used within a Task {...} block")

Make T an alias for Task instead of Target, move T.* operations to Task.* #3356

Are you sure you want to change the base?

Make T an alias for Task instead of Target, move T.* operations to Task.* #3356

Conversation

lihaoyi commented Aug 8, 2024 • edited Loading

lefou Aug 9, 2024

Choose a reason for hiding this comment

lihaoyi Aug 9, 2024

Choose a reason for hiding this comment

lefou Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

lihaoyi Aug 9, 2024

Choose a reason for hiding this comment

lefou Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

lefou left a comment • edited Loading

Choose a reason for hiding this comment

lefou Aug 10, 2024

Choose a reason for hiding this comment

lihaoyi commented Aug 10, 2024

Make `T` an alias for `Task` instead of `Target`, move `T.` operations to `Task.` #3356

Make `T` an alias for `Task` instead of `Target`, move `T.` operations to `Task.` #3356

lihaoyi commented Aug 8, 2024 •

edited

Loading

lefou Aug 9, 2024 •

edited

Loading

lefou Aug 10, 2024 •

edited

Loading

lefou left a comment •

edited

Loading