Better define how lazy task registration and task replacement work together

[adammurdoch]

The introduction of the lazy task API gives us an opportunity to improve the mechanism by which tasks are replaced. (Note that "improve" might mean "remove").

Task replacement provides a mechanism whereby build logic can replace an implementation of a task defined by some other build logic with a different implementation. If we want to support this use case, we should move towards making the solution safer, as there are a number of issues with the current approach. The basic cause of these issues is that a task instance observed by some build logic can disappear and be replaced later by a different instance.

We should:

• Disallow replacement of a task that has been realized (including implicit realization through create()).
• Offer an API by which a task registration can be replaced with another registration of a compatible type.
• Ensure that all providers returned by the registration methods (register() and whatever API the previous item adds) always return the same instance for a given name.

[adammurdoch]

Whatever we come up with for this should also apply to remove() in the short term. That is, we should treat replace as a remove + register. Later, I think we should disallow remove() entirely.

[adammurdoch]

To flesh out the suggestion in the description, we would do something like this:

Add a method called replace() with the same signature as register(). It would be an error to call this method if the task has been realized:

tasks.register("one")
tasks.replace("one") // Ok

tasks.create("two")
tasks.replace("two") // fails

tasks.register("three").get()
tasks.replace("three") // fails

tasks.register("four")
tasks.all { ... }
tasks.replace("four") // fails

tasks.register("five")
tasks.replace("five")
tasks.replace("five") // ok

It would also be an error to replace a task using an implementation type that is not compatible:

tasks.register("one", JavaCompile)
tasks.replace("one", MyJavaCompile) // ok

tasks.register("one", JavaCompile)
tasks.replace("one", Jar) // fails

Given this, every provider created with a given name would then always return the same instance:

val p1 = tasks.register("one")
val p2 = tasks.named("one")
val p3 = tasks.replace("one")

assert p1.get() == p2.get()
assert p1.get() == p3.get()
assert p1.get() == tasks.getByName("one")

And configuration actions would apply to the same instance:

tasks.register("one") { it.prop = "value" }
tasks.replace("one") { assert it.prop == "value"; it.prop = "value 2" }
tasks.all { assert it.prop == "value 2" }

remove() should work in a similar way. It would be an error to remove a task that has been realized (for whatever reason). It would be an error to call get() on a provider for a task that has been removed:

val p1 = tasks.register("one")
tasks.remove("one")
p1.get() // fails

isPresent() should return false after remove() has successfully been used.

We would start by deprecating using remove() on a realized task, making it an error in 6.0. However, it might be better to deprecate remove() entirely.

We also have the "overwrite" option on the eager task API. This should end up working the same way as replace(). We would start by deprecating replacing a realized task, making it an error in 6.0. Again, it might be better to deprecate "overwrite" or even the eager API entirely.

[big-guy]

As brought up in #644, remove() needs to remove tasks that are added via register() if they have not been realized and fail if they have been realized.

[lacasseio]

I can start working on this, do we agree with the behaviors explained in this issue? I have no objection. Basically, we deprecate remove entirely, add error case for specific replacement scenario with register() and deprecate the other scenario with the current API.

[ghale]

SGTM

[lacasseio]

Here is the PR for deprecating all removal method on TaskContainer: gradle/gradle#6049

[lacasseio]

Should the following become an error when task foo doesn't exist?

tasks.replace('foo')

Right now, it just adds the task to the container.

[lacasseio]

The following case cannot work right now because replace return a Task instance instead of TaskProvider. We will need to add a new method replaceLater 😄 Or do we have a better idea?

tasks.register("five")
tasks.replace("five")
tasks.replace("five") // ok

[lacasseio]

The removal is almost merged. I'm going away on vacation so I will move this back to the iteration.

We agreed that we need another API to replace that is lazy. I suggest deprecating replace, add the stricter check when replacing an unrealized task. The new replace method should only allow for the stricter check and disallow replacing realized task.

Possible candidate for the new method name:

• reregister
• swap
• substitute
• supersede
• change
• overwrite <- I like this one simply because it's the same as the option for eager
• alter
• changeType
• convert

[adammurdoch]

We shouldn't deprecate replace() until there's a stable replacement. We should deprecate using the method in an unsafe way.

[lacasseio]

PR: gradle/gradle#6407