UnnecessaryLambdaEnclosingParameters is turning" not actually turning

[gbq6]

Steps

1. Have a snippet like these somewhere in your code:

(final String x) -> true

(String x) -> true

2. Run cleanthat

Expected

Cleanthat either does not print anything, or turns them into:

x -> true

Actual

Cleanthat does not change the code, but prints one of the below messages:

UnnecessaryLambdaEnclosingParameters is turning `(final String x) -> true` into `final String x -> true`

UnnecessaryLambdaEnclosingParameters is turning `(String x) -> true` into `String x -> true`

Notes

Actually doing the printed change would cause a compile error.

Versions:

• Java: 21
• Spotless: 2.46.1
• Cleanthat: 2.24

[blacelle]

@gbq6 Thanks for the report, and the fix!

Would a Cleanthat release help, or should I wait a bit for any reason (e.g. more feedback from yourself)?

[gbq6]

I guess it would, but it's not urgent, so thank you, but feel free to keep the planned schedule. :)

[gbq6]

I realized that although my change would have been an improvement - if I did it right, but I did not, so another PR is on the way - the actual issue is that JavaParser seems to ignore the setEnclosingParameters(false) setting. Should I report it to that project's maintainers?

[blacelle]

the actual issue is that JavaParser seems to ignore the setEnclosingParameters(false) setting. Should I report it to that project's maintainers?

Definitely. I would be good to make a reproduction scenario not involving cleanthat, to make sure it is an issue in the upstream dependency.

[gbq6]

Could you please give me a hint where the actual "rewriting" happens, as the node replacement seems to succeed?

[blacelle]

Mmm. What do you mean by rewriting? The node is replaced right into the method you call:

	protected boolean tryReplace(Node node, Node replacement) {
		if (cancelDueToComment(node)) {
			LOGGER.info("We skip replacing {} due to the presence of a comment", node);
			return false;
		}

		LOGGER.info("{} is turning `{}` into `{}`", getClass().getSimpleName(), node, replacement);

		var result = node.replace(replacement);

		if (!result) {
			nbReplaceIssues.incrementAndGet();
			LOGGER.warn("{} failed turning `{}` into `{}`", getClass().getSimpleName(), node, replacement);
		}

		return result;
	}

The next tricky step is the actual writing of the modified node/AST into text/.java file. eu.solven.cleanthat.engine.java.refactorer.AstRefactorerInstance.applyMutator(AtomicReference<String>, AtomicReference<AST>, Path, IWalkingMutator<AST, R>) is probably some location involving both node replacement and conversion to String.

[gbq6]

I think that step might be the one I'm looking for, as cleanthat logs, that it's turning (x) -> true into x -> true, but it does not do so (and does not log the failure message either.

[blacelle]

javaparser/javaparser#3936 holds an example snippet when I parse similarly to CLeanthat, modify given JavaParser API, and write as String similarly to Cleanthat. It may help writing a simple reproducing scenario.

[gbq6]

javaparser/javaparser#4856 (comment) - If I understand correctly, maybe we should change the printer used based on the applied mutator if possible?

[blacelle]

Mmm. I believe the code-snippet is not representative of our situation. Or something is wrong somewhere else:

Given the relevant feedback, you may adjust your case with

class Scratch {

	static final String CLASS_AS_STRING = "package com.github.test;\n"
			+ "import java.util.function.Function;\n"
			+ "public class TestClass {\n"
			+ "  Function<Integer, Boolean> function = (a) -> true;\n"
			+ "}";

	public static void main(String[] args) {
		var unit = StaticJavaParser.parse(CLASS_AS_STRING);
		unit = LexicalPreservingPrinter.setup(unit);

		var lambda = unit.findFirst(LambdaExpr.class).get();
		System.out.println("Before: " + LexicalPreservingPrinter.print(unit));
		System.out.println("---");
		System.out.println("Before (Pretty):  " + new DefaultPrettyPrinter().print(unit));
		System.out.println("---");

		var newLambda = lambda.clone();
		newLambda.setEnclosingParameters(false);
		
		if (!lambda.replace(newLambda)) {
			throw new IllegalStateException("Replacement failed");
		}

		System.out.println("After:  " + LexicalPreservingPrinter.print(unit));
		System.out.println("---");
		System.out.println("After (Pretty):  " + new DefaultPrettyPrinter().print(unit));
		System.out.println("---");
	}
}

-> This outputs the same: something is fishy with new DefaultPrettyPrinter()

(In fact, it did work with LexicalPreservingPrinter, so the feedback is awkward and it was already pin-pointing a weird behavior. Still, let's provide them clearer case, following Javaparser conventions).

[gbq6]

Still, let's provide them clearer case, following Javaparser conventions).

Although initially I was ambitious in trying to resolve this, but now I'm starting to feel like I need a lot more time to understand how this works exactly. :/

[blacelle]

LexicalPreservingPrinter is a painful static method based on some static state, receiving events from AST modification... (...). ( hence the initial LexicalPreservingPrinter.setup(unit) ).