From b8f8c71188be6cb8b60a67cf6b08f8817457d492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kaplan?= <228053663+olwispe@users.noreply.github.com> Date: Sat, 30 Aug 2025 10:02:44 +0200 Subject: [PATCH 1/7] Mergeable `if` statements should be combined - RSPEC-S1066 --- .../CombineMergeableIfStatements.java | 97 ++++ .../CombineMergeableIfStatementsTest.java | 420 ++++++++++++++++++ 2 files changed, 517 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java new file mode 100644 index 000000000..bea4b46bc --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -0,0 +1,97 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.ListUtils; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.Statement; + +import java.util.List; +import java.util.Set; + +import static java.util.Collections.singleton; +import static org.openrewrite.java.format.ShiftFormat.indent; + +public class CombineMergeableIfStatements extends Recipe { + @Override + public String getDisplayName() { + // language=markdown + return "Mergeable `if` statements should be combined"; + } + + @Override + public String getDescription() { + // language=markdown + return "Mergeable `if` statements should be combined."; + } + + @Override + public Set getTags() { + return singleton("RSPEC-S1066"); + } + + @Override + public TreeVisitor getVisitor() { + return new JavaIsoVisitor() { + @Override + public J.If visitIf(J.If iff, ExecutionContext ctx) { + J.If outerIf = super.visitIf(iff, ctx); + + if (outerIf.getElsePart() == null) { + // thenPart is either a single if or a block with a single if + J.Block outerBlock = null; + J.If innerIf = null; + if (outerIf.getThenPart() instanceof J.If) { + innerIf = (J.If) outerIf.getThenPart(); + } else if (outerIf.getThenPart() instanceof J.Block) { + outerBlock = (J.Block) outerIf.getThenPart(); + List statements = outerBlock.getStatements(); + if (statements.size() == 1 && statements.get(0) instanceof J.If) { + innerIf = (J.If) statements.get(0); + } + } + + if (innerIf != null && innerIf.getElsePart() == null) { + // thenPart of outer if is replaced with thenPart of innerIf + // combine conditions with logical AND : correct parenthesizing is handled by JavaTemplate + final Expression outerCondition = outerIf.getIfCondition().getTree(); + final Expression innerCondition = innerIf.getIfCondition().getTree(); + + innerIf = indent(innerIf, getCursor(), -1); + outerIf = outerIf.withThenPart(innerIf.getThenPart()); + outerIf = JavaTemplate.apply( + "#{any()} && #{any()}", + updateCursor(outerIf), + outerCondition.getCoordinates().replace(), + outerCondition, + innerCondition); + outerIf = outerIf.withComments(outerBlock != null ? + ListUtils.concatAll(outerBlock.getComments(), innerIf.getComments()) : + innerIf.getComments()); + } + } + + return outerIf; + } + }; + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java new file mode 100644 index 000000000..07615c51e --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java @@ -0,0 +1,420 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.version; + +class CombineMergeableIfStatementsTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new CombineMergeableIfStatements()); + } + + @DocumentExample + @Test + void combineMergeableIfStatements() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } + } + } + } + """, + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1 && condition2) { + System.out.println("OK"); + } + } + } + """ + ) + ); + } + + @Test + void simplifyWithPatternMatchingForInstanceOf() { + rewriteRun( + spec -> spec + .recipes(new InstanceOfPatternMatch(), new CombineMergeableIfStatements()) + .allSources(sourceSpec -> version(sourceSpec, 17)), + // language=java + java( + """ + class A { + void a(Object o) { + if (o instanceof String) { + String s = (String) o; + if (s.isEmpty()) { + System.out.println("OK"); + } + } + } + } + """, + """ + class A { + void a(Object o) { + if (o instanceof String s && s.isEmpty()) { + System.out.println("OK"); + } + } + } + """ + ) + ); + + } + + @Test + void simplifyWithMultiplePatternMatchingForInstanceOf() { + // This test doesn't fully simplify but could with an 'Inline Local Variable Used Once' recipe + rewriteRun( + spec -> spec + .recipes(new InstanceOfPatternMatch(), new CombineMergeableIfStatements()) + .allSources(sourceSpec -> version(sourceSpec, 17)), + // language=java + java( + """ + import java.util.List; + + class A { + void a(Object o1) { + if (o1 instanceof List) { + List list = (List) o1; + if (!list.isEmpty()) { + Object o2 = list.get(0); + if (o2 instanceof String) { + String s = (String) o2; + if (s.isEmpty()) { + System.out.println("OK"); + } + } + } + } + } + } + """, + """ + import java.util.List; + + class A { + void a(Object o1) { + if (o1 instanceof List list && !list.isEmpty()) { + Object o2 = list.get(0); + if (o2 instanceof String s && s.isEmpty()) { + System.out.println("OK"); + } + } + } + } + """ + ) + ); + } + + @Test + void combineWithoutBlocks() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) + if (condition2) + System.out.println("OK"); + } + } + """, + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1 && condition2) + System.out.println("OK"); + } + } + """ + ) + ); + } + + @Test + void combineSeveralNestedIfs() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + if (b1) { + if (b2) { + if (b3) { + if (b4) { + if (b5) { + if (b6) { + System.out.println("OK"); + } + } + } + } + } + } + } + } + """, + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + if (b1 && b2 && b3 && b4 && b5 && b6) { + System.out.println("OK"); + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } + } else { + System.out.println("KO"); + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasEmptyBlockAsElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } + } else { + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasEmptyStatementAsElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } + } else; + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasOneStatementInThenPartButIsNotIf() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + System.out.println("KO"); + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasOneStatementWithoutBlockInThenPartButIsNotIf() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) + System.out.println("KO"); + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenOuterIfHasTwoStatementsInThenPart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } + System.out.println("KO"); + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenInnerIfHasElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } else { + System.out.println("KO"); + } + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenInnerIfHasEmptyBlockAsElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } else { + } + } + } + } + """ + ) + ); + } + + @Test + void noSimplificationWhenInnerIfHasEmptyStatementAsElsePart() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) { + if (condition2) { + System.out.println("OK"); + } else; + } + } + } + """ + ) + ); + } + + @Test + void combineMergeableIfStatementsWithComments() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2) { + if (condition1) /* Comment 0 */ { // Comment 1 + // Comment 2 + if (condition2) /* Comment 3 */ { // Comment 4 + System.out.println("OK"); + } + } + } + } + """, + """ + class A { + void a(boolean condition1, boolean condition2) { + /* Comment 0 */ // Comment 1 + // Comment 2 + if (condition1 && condition2) /* Comment 3 */ { // Comment 4 + System.out.println("OK"); + } + } + } + """ + ) + ); + } +} From 866ae68867945700e76dc5a1033223bf3143b072 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 8 Sep 2025 18:52:45 +0200 Subject: [PATCH 2/7] Show correct handling of binary in inner condition --- .../CombineMergeableIfStatements.java | 4 +-- .../CombineMergeableIfStatementsTest.java | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java index bea4b46bc..f54d2f281 100644 --- a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -73,8 +73,8 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { if (innerIf != null && innerIf.getElsePart() == null) { // thenPart of outer if is replaced with thenPart of innerIf // combine conditions with logical AND : correct parenthesizing is handled by JavaTemplate - final Expression outerCondition = outerIf.getIfCondition().getTree(); - final Expression innerCondition = innerIf.getIfCondition().getTree(); + Expression outerCondition = outerIf.getIfCondition().getTree(); + Expression innerCondition = innerIf.getIfCondition().getTree(); innerIf = indent(innerIf, getCursor(), -1); outerIf = outerIf.withThenPart(innerIf.getThenPart()); diff --git a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java index 07615c51e..bdf9f00a2 100644 --- a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java @@ -417,4 +417,33 @@ void a(boolean condition1, boolean condition2) { ) ); } + + @Test + void combineBinaryConditions() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean condition1, boolean condition2, boolean condition3) { + if (condition1) { + if (condition2 || condition3) { + System.out.println("OK"); + } + } + } + } + """, + """ + class A { + void a(boolean condition1, boolean condition2, boolean condition3) { + if (condition1 && (condition2 || condition3)) { + System.out.println("OK"); + } + } + } + """ + ) + ); + } } From a7e45f75d8e62ffdc8bd5b78e91dbe33fbd15744 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 8 Sep 2025 19:00:48 +0200 Subject: [PATCH 3/7] Inline repeated assignments and add early return --- .../CombineMergeableIfStatements.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java index f54d2f281..e71e18694 100644 --- a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -77,16 +77,16 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { Expression innerCondition = innerIf.getIfCondition().getTree(); innerIf = indent(innerIf, getCursor(), -1); - outerIf = outerIf.withThenPart(innerIf.getThenPart()); - outerIf = JavaTemplate.apply( - "#{any()} && #{any()}", - updateCursor(outerIf), - outerCondition.getCoordinates().replace(), - outerCondition, - innerCondition); - outerIf = outerIf.withComments(outerBlock != null ? - ListUtils.concatAll(outerBlock.getComments(), innerIf.getComments()) : - innerIf.getComments()); + return JavaTemplate.apply( + "#{any()} && #{any()}", + updateCursor(outerIf), + outerCondition.getCoordinates().replace(), + outerCondition, + innerCondition) + .withThenPart(innerIf.getThenPart()) + .withComments(outerBlock != null ? + ListUtils.concatAll(outerBlock.getComments(), innerIf.getComments()) : + innerIf.getComments()); } } From 3441b42a27107a5ef45aab306661e13ab8f96803 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 8 Sep 2025 19:17:00 +0200 Subject: [PATCH 4/7] Show comment lost in current iteration. --- .../staticanalysis/CombineMergeableIfStatementsTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java index bdf9f00a2..b2493739c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java @@ -394,6 +394,7 @@ void combineMergeableIfStatementsWithComments() { """ class A { void a(boolean condition1, boolean condition2) { + // Comment -1 if (condition1) /* Comment 0 */ { // Comment 1 // Comment 2 if (condition2) /* Comment 3 */ { // Comment 4 @@ -406,6 +407,7 @@ void a(boolean condition1, boolean condition2) { """ class A { void a(boolean condition1, boolean condition2) { + // Comment -1 /* Comment 0 */ // Comment 1 // Comment 2 if (condition1 && condition2) /* Comment 3 */ { // Comment 4 From ecaad2a9aa179585b6e59a866c7b089bb6ef133c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kaplan?= <228053663+olwispe@users.noreply.github.com> Date: Sat, 13 Sep 2025 11:57:21 +0200 Subject: [PATCH 5/7] Preserve comments associated with outer `if` --- .../staticanalysis/CombineMergeableIfStatements.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java index e71e18694..b63dccdb2 100644 --- a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -18,7 +18,6 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; -import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.tree.Expression; @@ -28,7 +27,9 @@ import java.util.List; import java.util.Set; +import static java.util.Collections.emptyList; import static java.util.Collections.singleton; +import static org.openrewrite.internal.ListUtils.concatAll; import static org.openrewrite.java.format.ShiftFormat.indent; public class CombineMergeableIfStatements extends Recipe { @@ -79,14 +80,14 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { innerIf = indent(innerIf, getCursor(), -1); return JavaTemplate.apply( "#{any()} && #{any()}", - updateCursor(outerIf), + getCursor(), outerCondition.getCoordinates().replace(), outerCondition, innerCondition) .withThenPart(innerIf.getThenPart()) - .withComments(outerBlock != null ? - ListUtils.concatAll(outerBlock.getComments(), innerIf.getComments()) : - innerIf.getComments()); + .withComments(concatAll( + concatAll(outerIf.getComments(), outerBlock != null ? outerBlock.getComments() : emptyList()), + innerIf.getComments())); } } From c03e6676a5f8d4fec04edad6f72bbbfd7d924a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kaplan?= <228053663+olwispe@users.noreply.github.com> Date: Sat, 13 Sep 2025 23:47:29 +0200 Subject: [PATCH 6/7] Move inner `if` to a new line --- .../CombineMergeableIfStatements.java | 88 +++++++++++++++++-- .../CombineMergeableIfStatementsTest.java | 63 +++++++++++-- 2 files changed, 138 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java index b63dccdb2..3f33389ee 100644 --- a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -15,24 +15,37 @@ */ package org.openrewrite.staticanalysis; +import lombok.RequiredArgsConstructor; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; +import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.style.IntelliJ; +import org.openrewrite.java.style.TabsAndIndentsStyle; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaSourceFile; +import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; +import org.openrewrite.java.tree.TextComment; +import org.openrewrite.style.Style; import java.util.List; import java.util.Set; import static java.util.Collections.emptyList; import static java.util.Collections.singleton; +import static java.util.Objects.requireNonNull; import static org.openrewrite.internal.ListUtils.concatAll; import static org.openrewrite.java.format.ShiftFormat.indent; public class CombineMergeableIfStatements extends Recipe { + + private static final String CONTINUATION_KEY = "continuationAfterLogicalAnd"; + @Override public String getDisplayName() { // language=markdown @@ -78,12 +91,13 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { Expression innerCondition = innerIf.getIfCondition().getTree(); innerIf = indent(innerIf, getCursor(), -1); + doAfterVisit(new MergedConditionalVisitor<>()); return JavaTemplate.apply( - "#{any()} && #{any()}", - getCursor(), - outerCondition.getCoordinates().replace(), - outerCondition, - innerCondition) + String.format("#{any()} /*%s*/&& #{any()}", CONTINUATION_KEY), + getCursor(), + outerCondition.getCoordinates().replace(), + outerCondition, + innerCondition) .withThenPart(innerIf.getThenPart()) .withComments(concatAll( concatAll(outerIf.getComments(), outerBlock != null ? outerBlock.getComments() : emptyList()), @@ -95,4 +109,68 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { } }; } + + @RequiredArgsConstructor + private static class MergedConditionalVisitor

extends JavaIsoVisitor

{ + @Nullable + private TabsAndIndentsStyle tabsAndIndentsStyle; + + @Override + public @Nullable J visit(@Nullable Tree tree, P p) { + if (tree instanceof JavaSourceFile) { + JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); + tabsAndIndentsStyle = Style.from(TabsAndIndentsStyle.class, cu, IntelliJ::tabsAndIndents); + } + return super.visit(tree, p); + } + + @Override + public Space visitSpace(@Nullable Space space, Space.Location loc, P p) { + Space s = super.visitSpace(space, loc, p); + if (s.getComments().size() == 1 && + s.getComments().get(0) instanceof TextComment) { + TextComment onlyComment = (TextComment) s.getComments().get(0); + if (onlyComment.isMultiline() && + CONTINUATION_KEY.equals(onlyComment.getText()) && + getCursor().firstEnclosingOrThrow(J.Binary.class).getOperator() == J.Binary.Type.And) { + getCursor().putMessageOnFirstEnclosing(J.Binary.class, CONTINUATION_KEY, true); + s = s.withComments(emptyList()); + } + } + + return s; + } + + @Override + public J.Binary visitBinary(J.Binary binary, P p) { + J.Binary b = super.visitBinary(binary, p); + + if (Boolean.TRUE.equals(getCursor().getMessage(CONTINUATION_KEY))) { + J.If outerIf = getCursor().firstEnclosingOrThrow(J.If.class); + final String outerIfIndent = outerIf.getPrefix().getIndent(); + b = b.withRight(b.getRight().withPrefix( + Space.format("\n" + continuationIndent(requireNonNull(tabsAndIndentsStyle), outerIfIndent)))); + } + + return b; + } + + private String continuationIndent(TabsAndIndentsStyle tabsAndIndents, String currentIndent) { + char c; + int len; + if (tabsAndIndents.getUseTabCharacter()) { + c = '\t'; + len = tabsAndIndents.getContinuationIndent() / tabsAndIndents.getTabSize(); + } else { + c = ' '; + len = tabsAndIndents.getContinuationIndent(); + } + + StringBuilder sb = new StringBuilder(currentIndent); + for (int i = 0; i < len; i++) { + sb.append(c); + } + return sb.toString(); + } + } } diff --git a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java index b2493739c..6ece54a1c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java @@ -50,7 +50,8 @@ void a(boolean condition1, boolean condition2) { """ class A { void a(boolean condition1, boolean condition2) { - if (condition1 && condition2) { + if (condition1 && + condition2) { System.out.println("OK"); } } @@ -83,7 +84,8 @@ void a(Object o) { """ class A { void a(Object o) { - if (o instanceof String s && s.isEmpty()) { + if (o instanceof String s && + s.isEmpty()) { System.out.println("OK"); } } @@ -128,9 +130,11 @@ void a(Object o1) { class A { void a(Object o1) { - if (o1 instanceof List list && !list.isEmpty()) { + if (o1 instanceof List list && + !list.isEmpty()) { Object o2 = list.get(0); - if (o2 instanceof String s && s.isEmpty()) { + if (o2 instanceof String s && + s.isEmpty()) { System.out.println("OK"); } } @@ -158,7 +162,8 @@ void a(boolean condition1, boolean condition2) { """ class A { void a(boolean condition1, boolean condition2) { - if (condition1 && condition2) + if (condition1 && + condition2) System.out.println("OK"); } } @@ -194,7 +199,12 @@ void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { """ class A { void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { - if (b1 && b2 && b3 && b4 && b5 && b6) { + if (b1 && + b2 && + b3 && + b4 && + b5 && + b6) { System.out.println("OK"); } } @@ -410,7 +420,8 @@ void a(boolean condition1, boolean condition2) { // Comment -1 /* Comment 0 */ // Comment 1 // Comment 2 - if (condition1 && condition2) /* Comment 3 */ { // Comment 4 + if (condition1 && + condition2) /* Comment 3 */ { // Comment 4 System.out.println("OK"); } } @@ -439,7 +450,43 @@ void a(boolean condition1, boolean condition2, boolean condition3) { """ class A { void a(boolean condition1, boolean condition2, boolean condition3) { - if (condition1 && (condition2 || condition3)) { + if (condition1 && + (condition2 || condition3)) { + System.out.println("OK"); + } + } + } + """ + ) + ); + } + + @Test + void combineLogicalAndConditions() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + if (b1 && b2) { + if (b3 && + b4) { + if (b5 && b6) { + System.out.println("OK"); + } + } + } + } + } + """, + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + if (b1 && b2 && + b3 && + b4 && + b5 && b6) { System.out.println("OK"); } } From 709be2c4bd65a94e6799578818eb8079ab900b53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kaplan?= <228053663+olwispe@users.noreply.github.com> Date: Sat, 27 Sep 2025 20:55:50 +0200 Subject: [PATCH 7/7] Handle comments associated with inner conditional --- .../CombineMergeableIfStatements.java | 103 ++++++++++-- .../CombineMergeableIfStatementsTest.java | 149 +++++++++++++++++- 2 files changed, 230 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java index 3f33389ee..64a69a5f7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java +++ b/src/main/java/org/openrewrite/staticanalysis/CombineMergeableIfStatements.java @@ -21,10 +21,12 @@ import org.openrewrite.Recipe; import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.style.IntelliJ; import org.openrewrite.java.style.TabsAndIndentsStyle; +import org.openrewrite.java.tree.Comment; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaSourceFile; @@ -34,12 +36,13 @@ import org.openrewrite.style.Style; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.UUID; import static java.util.Collections.emptyList; import static java.util.Collections.singleton; import static java.util.Objects.requireNonNull; -import static org.openrewrite.internal.ListUtils.concatAll; import static org.openrewrite.java.format.ShiftFormat.indent; public class CombineMergeableIfStatements extends Recipe { @@ -90,18 +93,20 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { Expression outerCondition = outerIf.getIfCondition().getTree(); Expression innerCondition = innerIf.getIfCondition().getTree(); - innerIf = indent(innerIf, getCursor(), -1); + UUID innerIfId = Tree.randomId(); + getCursor().getRoot().putMessage(innerIfId.toString(), innerIf.getComments()); + UUID outerBlockId = Tree.randomId(); + getCursor().getRoot().putMessage(outerBlockId.toString(), + Optional.ofNullable(outerBlock).map(J::getComments).orElse(emptyList())); + doAfterVisit(new MergedConditionalVisitor<>()); return JavaTemplate.apply( - String.format("#{any()} /*%s*/&& #{any()}", CONTINUATION_KEY), + String.format("#{any()} /*%s,%s,%s*/&& #{any()}", CONTINUATION_KEY, innerIfId, outerBlockId), getCursor(), outerCondition.getCoordinates().replace(), outerCondition, innerCondition) - .withThenPart(innerIf.getThenPart()) - .withComments(concatAll( - concatAll(outerIf.getComments(), outerBlock != null ? outerBlock.getComments() : emptyList()), - innerIf.getComments())); + .withThenPart(indent(innerIf.getThenPart(), getCursor(), -1)); } } @@ -112,6 +117,7 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) { @RequiredArgsConstructor private static class MergedConditionalVisitor

extends JavaIsoVisitor

{ + @Nullable private TabsAndIndentsStyle tabsAndIndentsStyle; @@ -131,10 +137,16 @@ public Space visitSpace(@Nullable Space space, Space.Location loc, P p) { s.getComments().get(0) instanceof TextComment) { TextComment onlyComment = (TextComment) s.getComments().get(0); if (onlyComment.isMultiline() && - CONTINUATION_KEY.equals(onlyComment.getText()) && + onlyComment.getText().startsWith(CONTINUATION_KEY) && getCursor().firstEnclosingOrThrow(J.Binary.class).getOperator() == J.Binary.Type.And) { - getCursor().putMessageOnFirstEnclosing(J.Binary.class, CONTINUATION_KEY, true); - s = s.withComments(emptyList()); + final String[] arr = onlyComment.getText().split(","); + final String innerIfId = arr[1]; + final String outerBlockId = arr[2]; + List innerIfComments = Optional.ofNullable(getCursor().getRoot().>pollMessage(innerIfId)).orElse(emptyList()); + List outerBlockComments = Optional.ofNullable(getCursor().getRoot().>pollMessage(outerBlockId)).orElse(emptyList()); + + getCursor().putMessageOnFirstEnclosing(J.Binary.class, CONTINUATION_KEY, innerIfComments); + s = s.withComments(outerBlockComments); } } @@ -145,16 +157,77 @@ public Space visitSpace(@Nullable Space space, Space.Location loc, P p) { public J.Binary visitBinary(J.Binary binary, P p) { J.Binary b = super.visitBinary(binary, p); - if (Boolean.TRUE.equals(getCursor().getMessage(CONTINUATION_KEY))) { - J.If outerIf = getCursor().firstEnclosingOrThrow(J.If.class); - final String outerIfIndent = outerIf.getPrefix().getIndent(); - b = b.withRight(b.getRight().withPrefix( - Space.format("\n" + continuationIndent(requireNonNull(tabsAndIndentsStyle), outerIfIndent)))); + List comments = getCursor().pollMessage(CONTINUATION_KEY); + if (comments != null) { + final String outerIfIndent = getCursor().firstEnclosingOrThrow(J.If.class).getPrefix().getIndent(); + final String continuationIndent = continuationIndent(requireNonNull(tabsAndIndentsStyle), outerIfIndent); + + if (comments.isEmpty()) { + b = b.withRight(b.getRight() + .withPrefix(Space.format("\n" + continuationIndent))); + } else { + b = b.withRight(b.getRight() + .withComments(ListUtils.map(comments, c -> replaceIndent(c, continuationIndent)))); + } } return b; } + private Comment replaceIndent(Comment comment, String newIndent) { + Comment c = comment.withSuffix(replaceLastLineWithIndent(comment.getSuffix(), newIndent)); + if (c.isMultiline() && c instanceof TextComment) { + TextComment tc = (TextComment) c; + c = tc.withText(replaceTextIndent(tc.getText(), newIndent)); + } + return c; + } + + private String replaceTextIndent(final String text, final String newIndent) { + final StringBuilder sb = new StringBuilder(); + boolean found = false; + for (final char ch : text.toCharArray()) { + if (ch == ' ' || ch == '\t') { + if (!found) { + sb.append(ch); + } + } else if (ch == '\r' || ch == '\n') { + sb.append(ch); + found = true; + } else { + if (found) { + sb.append(newIndent); + if (ch == '*') { + sb.append(' '); + } + } + sb.append(ch); + found = false; + } + } + if (found) { + sb.append(newIndent); + sb.append(' '); + } + return sb.toString(); + } + + private String replaceLastLineWithIndent(String whitespace, String indent) { + int idx = whitespace.length() - 1; + while (idx >= 0) { + char c = whitespace.charAt(idx); + if (c == '\r' || c == '\n') { + break; + } + idx--; + } + if (idx >= 0) { + return whitespace.substring(0, idx + 1) + indent; + } else { + return whitespace; + } + } + private String continuationIndent(TabsAndIndentsStyle tabsAndIndents, String currentIndent) { char c; int len; diff --git a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java index 6ece54a1c..f18b3c760 100644 --- a/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/CombineMergeableIfStatementsTest.java @@ -214,6 +214,132 @@ void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { ); } + @Test + void combineSeveralNestedIfsWithLineComments() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + // Comment 1.0 + if (b1) { // Comment 1 + if (b2) { // Comment 2 + // Comment 3.0 + if (b3) { // Comment 3 + // Comment 4.0 + if (b4) { // Comment 4 + // Comment 5.0 + if (b5) { // Comment 5 + // Comment 6.0 + // Comment 6.1 + // Comment 6.2 + if (b6) { // Comment 6 + System.out.println("OK"); + } + } + } + } + } + } + } + } + """, + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + // Comment 1.0 + if (b1 && // Comment 1 + b2 && // Comment 2 + // Comment 3.0 + b3 && // Comment 3 + // Comment 4.0 + b4 && // Comment 4 + // Comment 5.0 + b5 && // Comment 5 + // Comment 6.0 + // Comment 6.1 + // Comment 6.2 + b6) { // Comment 6 + System.out.println("OK"); + } + } + } + """ + ) + ); + } + + @Test + void combineSeveralNestedIfsWithMultilineComments() { + rewriteRun( + // language=java + java( + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + /* Comment 1.0 */ + if (b1) { /* Comment 1 */ + if (b2) { /* Comment 2 */ + /* Comment 3.0 */ + if (b3) { /* Comment 3 */ + /* Comment 4.0 */ + if (b4) { /* + * Comment 4 + */ + /* + * Comment 5.0 + Comment 5.1 + */ + if (b5) { /* Comment 5 */ + /** Comment 6.0 */ + /** + * Comment 6.1 + Comment 6.2 + */ + if (b6) { /* Comment 6 */ + System.out.println("OK"); + } + } + } + } + } + } + } + } + """, + """ + class A { + void a(boolean b1, boolean b2, boolean b3, boolean b4, boolean b5, boolean b6) { + /* Comment 1.0 */ + if (b1 && /* Comment 1 */ + b2 && /* Comment 2 */ + /* Comment 3.0 */ + b3 && /* Comment 3 */ + /* Comment 4.0 */ + b4 && /* + * Comment 4 + */ + /* + * Comment 5.0 + Comment 5.1 + */ + b5 && /* Comment 5 */ + /** Comment 6.0 */ + /** + * Comment 6.1 + Comment 6.2 + */ + b6) { /* Comment 6 */ + System.out.println("OK"); + } + } + } + """ + ) + ); + } + @Test void noSimplificationWhenOuterIfHasElsePart() { rewriteRun( @@ -404,9 +530,14 @@ void combineMergeableIfStatementsWithComments() { """ class A { void a(boolean condition1, boolean condition2) { - // Comment -1 - if (condition1) /* Comment 0 */ { // Comment 1 - // Comment 2 + // Comment 1.0 + if (condition1) /* Comment 1.1 */ + /* Comment 1.2 */ // Comment 1.3 + /* Comment 1.4 */ { // Comment 2.0 + // Comment 2.1 + /* + * Comment 2.2 + */ // Comment 2.3 if (condition2) /* Comment 3 */ { // Comment 4 System.out.println("OK"); } @@ -417,10 +548,14 @@ void a(boolean condition1, boolean condition2) { """ class A { void a(boolean condition1, boolean condition2) { - // Comment -1 - /* Comment 0 */ // Comment 1 - // Comment 2 - if (condition1 && + // Comment 1.0 + if (condition1 /* Comment 1.1 */ + /* Comment 1.2 */ // Comment 1.3 + /* Comment 1.4 */ && // Comment 2.0 + // Comment 2.1 + /* + * Comment 2.2 + */ // Comment 2.3 condition2) /* Comment 3 */ { // Comment 4 System.out.println("OK"); }