Add BufferedWriterCreation recipe & SimplifyConstantTernaryExecution

[JLLeitschuh]

• BufferedWriterCreation
• SimplifyConstantTernaryExecution

Signed-off-by: Jonathan Leitschuh Jonathan.Leitschuh@gmail.com

What's your motivation?

More recipes, more better?

Anything in particular you'd like reviewers to focus on?

There seems to be a bug in autoFormat where the J.Literal isn't having it's prefix padding removed.

Anyone you would like to review specifically?

• @knutwannheden

Have you considered any alternatives or workarounds?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[JLLeitschuh]

This PR is currently failing. It seems that autoFormat isn't working correctly in the SimplifyConstantTernaryExecution, and I have no idea what I'm doing wrong. Any insights here would be greatly appreciated.

[JLLeitschuh]

Also, waiting on openrewrite/rewrite#4015 to land too for this PR

[github-actions]

Suggested change

	public J visitTernary(J.Ternary ternary, ExecutionContext executionContext) {
	J.Ternary t = (J.Ternary) super.visitTernary(ternary, executionContext);
	Expression condition =
	SimplifyConstantIfBranchExecution.cleanupBooleanExpression(
	t.getCondition(),
	public J visitTernary(J.Ternary ternary, ExecutionContext ctx) {
	J.Ternary t = (J.Ternary) super.visitTernary(ternary, ctx);
	ctx
	return autoFormat(t.getTruePart(), ctx);
	return autoFormat(t.getFalsePart(), ctx);

[JLLeitschuh]

This is definitely wrong @timtebeek

[timtebeek]

Well that's disappointing; thanks for calling it out!
The underlying patch file looks ok-ish to me at first glance

diff --git a/src/main/java/org/openrewrite/staticanalysis/SimplifyConstantTernaryExecution.java b/src/main/java/org/openrewrite/staticanalysis/SimplifyConstantTernaryExecution.java
index d98e472..41983c2 100644
--- a/src/main/java/org/openrewrite/staticanalysis/SimplifyConstantTernaryExecution.java
+++ b/src/main/java/org/openrewrite/staticanalysis/SimplifyConstantTernaryExecution.java
@@ -46,18 +46,18 @@ public class SimplifyConstantTernaryExecution extends Recipe {
     public TreeVisitor<?, ExecutionContext> getVisitor() {
         JavaVisitor<ExecutionContext> v = new JavaVisitor<ExecutionContext>() {
             @Override
-            public J visitTernary(J.Ternary ternary, ExecutionContext executionContext) {
-                J.Ternary t = (J.Ternary) super.visitTernary(ternary, executionContext);
+            public J visitTernary(J.Ternary ternary, ExecutionContext ctx) {
+                J.Ternary t = (J.Ternary) super.visitTernary(ternary, ctx);
                 Expression condition =
                         SimplifyConstantIfBranchExecution.cleanupBooleanExpression(
                                 t.getCondition(),
                                 getCursor(),
-                                executionContext
+                                ctx
                         );
                 if (J.Literal.isLiteralValue(condition, true)) {
-                    return autoFormat(t.getTruePart(), executionContext);
+                    return autoFormat(t.getTruePart(), ctx);
                 } else if (J.Literal.isLiteralValue(condition, false)) {
-                    return autoFormat(t.getFalsePart(), executionContext);
+                    return autoFormat(t.getFalsePart(), ctx);
                 }
                 return t;
             }

So then it must be https://github.com/googleapis/code-suggester that's somehow thrown off. Unless you spot anything else in the diff above.

[JLLeitschuh]

Looks otherwise fine. Sounds like a fun bug to fix

[knutwannheden]

Can we add this to the SimplifyTernary Refaster recipe?

[JLLeitschuh]

Not if I want to include the call to cleanupBooleanExpression

[knutwannheden]

I think if we were to make sure the existing SimplifyBooleanExpressionVisitor can handle this case, then we should be fine, as we have the dedicated SIMPLIFY_BOOLEANS embedding option for that.

[JLLeitschuh]

@knutwannheden this test is failing and is currently generating a change that looks like this:

System.out.println( "foo");

instead of the expected:

System.out.println("foo");

Same issue with the test above. Not sure what's going on here. Is this something broken with the AutoFormat visitor?

[knutwannheden]

Could be. I can investigate.

[knutwannheden]

The "problem" here is that what gets auto-formatted is the true or false branch of the ternary only. In an of itself it isn't incorrectly formatted and removing the whitespace prefix would require checking what it is contained in.

While it might be possible to change how auto-formatting works (although that currently seems quite difficult, since our cursor doesn't know which "feature" of the containing element is currently being visited), I think we should here instead just format the containing element. I've updated the PR accordingly.