Execute the configuration actions lazily

[nedtwigg]

The only substantive change is that instead of this in the legacy SpotlessExtension...

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionBase.java

Lines 203 to 206 in 1e2801d

	public <T extends FormatExtension> void format(String name, Class<T> clazz, Action<T> configure) {
	T format = maybeCreate(name, clazz);
	configure.execute(format);
	}

...we add a field to FormatExtension, and populate it in each call to SpotlessExtension::format(name, clazz, action)

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Line 60 in 1e2801d

final List<Action<FormatExtension>> modernLazyActions = new ArrayList<>();

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionModern.java

Lines 52 to 54 in 1e2801d

	public <T extends FormatExtension> void format(String name, Class<T> clazz, Action<T> configure) {
	maybeCreate(name, clazz).modernLazyActions.add((Action<FormatExtension>) configure);
	}

Instead of executing the closures when they are declared, we store them, and only execute them when the task is configured:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionModern.java

Lines 70 to 79 in 1e2801d

	project.afterEvaluate(unused -> {
	spotlessTask.configure(task -> {
	// now that the task is being configured, we execute our actions
	for (Action<FormatExtension> lazyAction : formatExtension.modernLazyActions) {
	lazyAction.execute(formatExtension);
	}
	// and now we'll setup the task
	formatExtension.setupTask(task);
	});
	});

In my head, I thought that we would be able to remove the afterEvaluate block, but now I see that it was a mirage. If someone eagerly creates the task, and then calls java { blah }, it's important for that blah to have effect, but it wouldn't if you don't do the afterEvaluate.

It seems to me that this is as lazy as it could possibly be - we don't describe any of the properties of a task, or even execute the code that describes those those properties, until/unless that task is being configured. It also seems to me like it's quite difficult to avoid the afterEvaluate, which I guess is the point of the Provider<> stuff. We could change FormatExtension so that it applied itself directly against the SpotlessTask, rather than storing its own field for encoding, etc. The only hard part against that is cases where a user sets UTF-8 at the level of the Spotless block, but has some legacy subproject where encoding is CP-1252, cases like that.

Is afterEvaluate in any long-term trouble? Whaddya think @bigdaz?

[bigdaz]

@nedtwigg, I think your change makes sense to defer the possibly expensive configuration of each FormatExtension until we know that the output is really required. The only downside would be if someone queries the values of the extension before the "owning" task is created and configured.

Is afterEvaluate in any long-term trouble?

Very long term, we'd like to deprecate and remove. In the meantime, it's just considered poor form. In this case it's usage probably won't bite, but when layering multiple plugins the multiple afterEvaluate blocks can be problematic, particularly in the order they are executed.

The use of Property/Provider types would replace both the lazy configuration you propose for FormatExtension as well as the afterEvaluate. All properties on FormatExtension would be declared as either Provider or Property, with no setters on FormatExtension. You can then wire these properties directly into the task configuration at any time, since the values are ready lazily.

One nice thing is that you can declare FormatExtension as an abstract class, and simply add abstract getters for each property. This is called a "managed property" and Gradle will take care of instantiating the Provider for each property.

We are currently working on better documentation on what we consider to be idiomatic Gradle usage with Gradle 6+. Following these patterns will ensure that plugins/builds work better with the upcoming configuration-cache as well as providing a more concise, consistent user experience.

[nedtwigg]

I look forward to the docs! Fwiw, I'm a big fan of all the cool properties that you get from lazily computing immutable values, ala LazyForwardingEquality, so I think I should like the Property/Provider stuff. But so far I don't, because they seem difficult to coordinate.

What I mean by coordinate, is that I usually don't need a lazily-computed immutable integer, or lazily computed immutable String - I need a lazily computed immutable state, which usually has multiple components, and the inputs which eventually determine that state often have a different shape than the state they determine.

For example, if a user says ratchetFrom 'branch', that gets lazily turned into one immutable sha for the repository root, and also a set of immutable shas for each git tree that happens to be at the root of each subproject. So we have input {String reference} -> state { Sha rootTree, Map<Project, Sha> perProjectTrees }.

Also, when determining that input, mutability is quite handy. It's nice to apply a plugin which sets a bunch of defaults, and then be able to override values here and there on a per-project basis. Definitely there can be some footguns there, but the beauty of the groovy scripts, imo, is how easy it is for naive scripting logic to do what people expect.

So to me, the idea of "mutate inputs" -> afterEvaluate -> "now everything is frozen, compute immutable states out of the inputs" is easy to use and easy to reason about. The only problem is that composing afterEvaluate is disallowed, but I don't understand why the afterEvaluate calls can't be trampolined.

You have thought about this much more than me, I'm sure, and I look forward to the future releases, but my hope is that Gradle continues to have as few Gradle-specific concepts as possible. So far it has mostly been "normal script operating on normal values builds a task graph". If it becomes "normal script operating on special gradle-y values builds a task graph", I worry a bit about how much "magic" script and plugin authors will have to master. But you guys are batting like 99%, so I'm on board.

</rant>. No response expected, just wanted to share my thoughts since I've got a Gradle person's attention for a bit :). I'll merge this, and let it rest for a bit in case we have other ideas for things to improve.

[nedtwigg]

Part of #601