Formatter is now AutoCloseable, so that facilitates FormatterFunc.Closeable works.

[nedtwigg]

• TODO: update changelog

For a long time, we've had FormatterFunc.Closeable in the codebase, but with no usages.

spotless/lib/src/main/java/com/diffplug/spotless/FormatterFunc.java

Lines 22 to 43 in f622379

	/** A `Function<String, String>` whose implementation requires a resource which should be released when the function is no longer needed. */
	interface Closeable extends FormatterFunc, AutoCloseable {
	@Override
	void close();
	/** Creates a {@link Closeable} from an AutoCloseable and a function. */
	public static Closeable of(AutoCloseable closeable, FormatterFunc function) {
	Objects.requireNonNull(closeable, "closeable");
	Objects.requireNonNull(function, "function");
	return new Closeable() {
	@Override
	public void close() {
	ThrowingEx.run(closeable::close);
	}
	@Override
	public String apply(String input) throws Exception {
	return function.apply(Objects.requireNonNull(input));
	}
	};
	}
	}

The idea was that a FormatterFunc with native integration might be holding native handles that would need to be released. Now that we're about to integrate with V8 thanks to #283, we need to make this dead code come to life. The good news is that the abstraction we laid down without a concrete usecase 2 years ago fits the need in #283 perfectly. Aside: terrible idea that we did this without a use-case, the git blame shows it's my fault :'(

The bare minimum that we'll need to do is make Formatter into an AutoCloseable, which then will cleanup all of its FormatterStep/FormatterFunc. Formatter is only used in a few places in our entire codebase (8 including tests), so it's an easy change.

The next choice is whether FormatterStep should also be AutoCloseable. This is problematic, because we use them willy-nilly all over the place. It's a very nice property to be able to use them willy-nilly.

It's important to note that the only time a FormatterStep ever needs to be closed is if it has actually been applied, because until then its FormatterFunc has not been created. The intention of the library is that no one will call FormatterStep.format except Formatter, so lifting the resource-management burden to Formatter is relatively safe.

It might be tempting here to think "aha! We don't need SpotlessCache anymore!", which is the mechanism we use for caching ClassLoaders. However, it's important to note that we do this to share ClassLoaders across spotless runs so that the JIT can warmup (we get huge speed improvements compared to creating a destroying classloader for each run of Spotless). So this doesn't affect ClassLoader resources, just other kinds of resources.

[jbduncan]

Hi @nedtwigg. I'm convinced by the reasoning you made in your message above for making these changes, so on that front, this PR LGTM!

My only reservation is with how FormatterStepImpl.Standard has cleanupFormatterFunc whereas FormatterStepImpl.NeverUpToDate doesn't. I suppose my fear is that we may use FormatterStepImpl.NeverUpToDate in the future expecting it to auto-close FormatterFunc.Closeables as well, but then for it to bite us back. But it's quite likely I'm just being a bit too anxious here - we'd probably remember to implement such a method if needed - which is why I've submitted my approval rather than specifically request a change here.

So regardless of whether you decide to make FormatterStepImpl.NeverUpToDate have a cleanupFormatterFunc method or not, consider this as a "LGTM" from me!

[nedtwigg]

I think that's a good point worth addressing, addressed in previous commit.

[jbduncan]

Cheers @nedtwigg! This all definitely LGTM now. 👍

[fvgh]

LGTM