Introduce cleanthat refactorer

[blacelle]

Submission to introduce CleanThat as a Java refactoer step

[nedtwigg]

Cool!

• https://github.com/diffplug/spotless/blob/main/README.md#current-feature-matrix
• https://github.com/diffplug/spotless/tree/main/plugin-maven#table-of-contents

I know CleanThat has cloud aspects. This current thing is local-only, no network requests?

[blacelle]

I know CleanThat has cloud aspects. This current thing is local-only, no network requests?

Yes, this is totally local only.

The cloud app (with the same name, history not having yet split the two things apart) is a bot enabling CleanThat-Refactorer (and Spotless!) with no local environment.

[nedtwigg]

I would super duper recommend having a changelog, and linking to it from the maven README, but you don't have to. If it doesn't happen now though, people are going to eventally copy-paste this to the Gradle plugin, and the technical debt of not keeping a changelog will spread :)

[blacelle]

CleanThat definitely lacks a changelog. However, I do not get how it is related with the Gradle plugin?

[blacelle]

This relates with the expectations in #292

[nedtwigg]

However, I do not get how it is related with the Gradle plugin?

Just that examples spread. The standard thing in our docs is to link the changelog of the formatter, if it has one. Whether you add the Gradle integration or not, someone else might, and there's now yet one reference which would be mildly improved by a changelog. NBD, feel free to continue without a changelog, but I think they're deeply undervalued.

[blacelle]

NBD, feel free to continue without a changelog, but I think they're deeply undervalued.

joke I tend to be believe changelog for me are like loggers for you. We know their value, but for some reason, we have issues leveraging them xD.

[blacelle]

@nedtwigg Thanks for pushing me forward. I introduced a changelog and referred it here. Please tell me if I should refer it somewhere else. (The changelog is minimal, but like unitTests, it is easier to improve than to start from scratch...)

This PR holds something not acceptable: some unitTests dedicated to CleanThat are in lib/test. I failed reproducing compatKtLint0Dot48Dot0CompileAndTestOnly 'com.pinterest.ktlint:ktlint-ruleset-standard:0.48.0' to have some unittests, out of plugin-XXX. I would need a hand here.

---

As a whole: it does not work! Something wrong with the detection of default mutators (i.e. the refactoring rules) and I fail debugging the maven-plugin. Still investigating.

[blacelle]

OK.

Thread.currentThread().getContextClassLoader() is returning the main app classLoader, not the classLoader of currentState/jarState. While CleanThat bootstrap itself given the classLoader to detect default mutators (I'm not proud of this behavior, but it is supposed to simplify other matters on my side.).

[blacelle]

https://stackoverflow.com/questions/1771679/difference-between-threads-context-class-loader-and-normal-classloader

https://stackoverflow.com/questions/34787419/pmd-rule-use-proper-class-loader-explaination

[blacelle]

	@Override
	public String apply(String input) throws Exception {
		// https://stackoverflow.com/questions/1771679/difference-between-threads-context-class-loader-and-normal-classloader
		ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader();
		try {
			// Ensure CleanThat main Thread has its custom classLoader while executing its refactoring
			Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
			return doApply(input);
		} finally {
			// Restore the originalClassLoader
			Thread.currentThread().setContextClassLoader(originalClassLoader);
		}
	}

One may argue this should be the behavior for any step.

[blacelle]

I added Cleanthat as a Java option. Is there a way to get the default sourceCompatibility in JavaExtension? (https://www.baeldung.com/gradle-sourcecompatiblity-vs-targetcompatibility)

[nedtwigg]

Re: setting the current thread classloader over and over, you can definitely do that for your step if you need to. As for making it the default behavior, I'm curious about the performance impact of that and wary of relying on global state in general.

I just merged a fix for a very similar problem below:

• Load ScriptEngine plugins based on Nashorn.class jscriptbox#3

[blacelle]

As for making it the default behavior, I'm curious about the performance impact of that and wary of relying on global state in general.

I'm not used to manipulate the Thread classLoader. Looking at the javaCode, it looks a trivial change, hence free CPU-wise. I would anticipate a potential impact due to current thread working from a more heavy classLoader (even though it would have no impact if the step does not consume the classLoader, and a step consuming some classLoader would generally expect the step-specific classLoader (like Nashorn case, and Cleanthat case)). (But I'm not ClassLoader expert. )

[nedtwigg]

Two cases in a row is a pretty compelling case. If we make that change a default for all JarState formatters, that should be its own PR. Fwiw I'm still glad that Nashorn doesn't depend on global state anymore, but I can also see how it is now less flexible than it was before (no way to inject a global classloader into it),

[blacelle]

I gave a try to make the ClassLoader-per-state matter generic (in Formatter, while executing each Step). It was a failure. It is not the FormatterStep class ClassLoader which is relevant, but the FormatterFunc(https://github.com/diffplug/spotless/pull/1560/files#diff-26506e5262f1b9da1b3ffd0345462902931d28b7e3dfd4bf33f44dab333c36c4R62). Such instances are often wrapped (e.g. com.diffplug.spotless.Jvm.Support#suggestLaterVersionOnError), hence the relevant ClassLoader is deep inside. And loading the FormatterFunc given the proper (JarState based) classLoader is not a generic mechanism, but something done by each XXXFormatterStep.

Another example: In com.diffplug.spotless.generic.Jsr223Step.State#toFormatter, the FormatterFunc is not even based on the custom classLoader, but it refers to a custom class with the proper classLoader. I was considering glue-based formatters as a standard, but it is not yet the case.

[blacelle]

Just in case, in my perspective, this is ready-to-merge. Thanks @nedtwigg

[nedtwigg]

Released in plugin-gradle 6.15.0 and plugin-maven 2.33.0.

[blacelle]

Thanks a lot @nedtwigg !