From 3fe1ea9b0ed0e3317addb03de100488e180fb1f1 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sat, 6 May 2023 23:16:28 +0200 Subject: [PATCH 01/45] feat: create new recipe Ternary operators should not be nested https://github.com/openrewrite/rewrite-static-analysis/issues/51 add recipe and test --- .../TernaryOperatorsShouldNotBeNested.java | 52 +++++++++++++++++++ ...TernaryOperatorsShouldNotBeNestedTest.java | 43 +++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java new file mode 100644 index 0000000000..5766c22baf --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -0,0 +1,52 @@ +package org.openrewrite.staticanalysis; + +import java.time.Duration; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.lang.Nullable; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.tree.J; + +public class TernaryOperatorsShouldNotBeNested extends Recipe { + @Override + public String getDisplayName() { + return "Ternary operators should not be nested"; + } + + @Override + public String getDescription() { + return "Just because you can do something, doesn’t mean you should, and that’s the case with nested ternary operations. " + + "Nesting ternary operators results in the kind of code that may seem clear as day when you write it, " + + "but six months later will leave maintainers (or worse - future you) scratching their heads and cursing.\n\n" + + "Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement."; + } + + @Override + public Duration getEstimatedEffortPerOccurrence() { + return Duration.ofMinutes(5); + } + + @Override + public TreeVisitor getVisitor() { + return new TernaryOperatorsShouldNotBeNestedVisitor(); + } + + private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { + + @Override + public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { + if(ternary.getFalsePart() instanceof J.Ternary){ + System.out.println("Ternary nesting found: " + ternary.getFalsePart()); + //replace with: + // if(${ternary.getCondition()}){ + // return ${ternary.getTruePart()}; + // } + } + return super.visitTernary(ternary, executionContext); + } + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java new file mode 100644 index 0000000000..0721e02cf3 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -0,0 +1,43 @@ +package org.openrewrite.staticanalysis; + +import static org.junit.jupiter.api.Assertions.*; +import static org.openrewrite.java.Assertions.java; + +import org.junit.jupiter.api.Test; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +class TernaryOperatorsShouldNotBeNestedTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new TernaryOperatorsShouldNotBeNested()); + } + + @Test + void doReplaceNestedTernaryWithIfFollowedByNestedTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineHardThing(String a, String b) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineHardThing(String a, String b) { + if("a".equals(a)){ + return "a"; + } + return "b".equals(b) ? "b" : "nope"; + } + } + """ + ) + ); + } + +} \ No newline at end of file From 0ac3205ff4e3884fb22636e8ebfd9b9d99a2a62d Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sat, 6 May 2023 23:17:00 +0200 Subject: [PATCH 02/45] cleanup imports --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 5766c22baf..be50594760 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -4,10 +4,7 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; -import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; -import org.openrewrite.internal.lang.Nullable; -import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; From f3b83bccf89403cbc9ed6d5f41843a5f3ba6304c Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sat, 6 May 2023 23:17:48 +0200 Subject: [PATCH 03/45] update todo comment to complete solution --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index be50594760..2b51436084 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -38,10 +38,11 @@ private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisito public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { if(ternary.getFalsePart() instanceof J.Ternary){ System.out.println("Ternary nesting found: " + ternary.getFalsePart()); - //replace with: + //todo replace with: // if(${ternary.getCondition()}){ // return ${ternary.getTruePart()}; // } + // return ternary.getFalsePart(); } return super.visitTernary(ternary, executionContext); } From 2d317760c91bc7c7a722a833177ac55a81936b9e Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sat, 6 May 2023 23:18:57 +0200 Subject: [PATCH 04/45] add another todo true part --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 2b51436084..55f37178c1 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -43,6 +43,9 @@ public J visitTernary(final J.Ternary ternary, final ExecutionContext executionC // return ${ternary.getTruePart()}; // } // return ternary.getFalsePart(); + + //todo consider recursion + //todo consider nesting in truePart } return super.visitTernary(ternary, executionContext); } From 6ddccf81bd3f5368a4f94112b54cfc89d3f138e2 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sun, 7 May 2023 08:33:30 +0200 Subject: [PATCH 05/45] Add more tests --- .../TernaryOperatorsShouldNotBeNested.java | 34 +++++- ...TernaryOperatorsShouldNotBeNestedTest.java | 114 +++++++++++++++++- 2 files changed, 141 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 55f37178c1..9975a3b888 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -1,12 +1,19 @@ package org.openrewrite.staticanalysis; import java.time.Duration; +import java.util.List; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; +import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.Statement; +import org.openrewrite.marker.Markers; + public class TernaryOperatorsShouldNotBeNested extends Recipe { @Override @@ -34,18 +41,33 @@ public TreeVisitor getVisitor() { private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { + private final JavaTemplate splitNestedFalsePart = JavaTemplate.builder( + this::getCursor, + "if(#{any(boolean)})" + ) + .build(); + + @Override + public J visitStatement(final Statement statement, final ExecutionContext executionContext) { + //if statement contains a nested ternary, clone "statement part" and split? + // return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + + + + return super.visitStatement(statement, executionContext); + } + @Override public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { - if(ternary.getFalsePart() instanceof J.Ternary){ + if (ternary.getFalsePart() instanceof J.Ternary) { System.out.println("Ternary nesting found: " + ternary.getFalsePart()); //todo replace with: - // if(${ternary.getCondition()}){ - // return ${ternary.getTruePart()}; + // if(ternary.getCondition()){ + // return ternary.getTruePart(); // } // return ternary.getFalsePart(); - - //todo consider recursion - //todo consider nesting in truePart + // + // return is not actually part of the ternary, so how to "clone" that? } return super.visitTernary(ternary, executionContext); } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 0721e02cf3..f56757129e 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -15,7 +15,7 @@ public void defaults(RecipeSpec spec) { } @Test - void doReplaceNestedTernaryWithIfFollowedByNestedTernary() { + void doReplaceNestedOrTernaryWithIfFollowedByTernary() { rewriteRun( //language=java java( @@ -40,4 +40,116 @@ public String determineHardThing(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryWithIfFollowedByTernaryWithoutReturn() { + rewriteRun( + //language=java + java( + """ + class Test { + public void doThing(String a, String b) { + String result = "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + System.out.println(result); + } + } + """, + """ + class Test { + public void doThing(String a, String b) { + String result; + if("a".equals(a)){ + result = "a"; + } + else { + result = "b".equals(b) ? "b" : "nope"; + } + System.out.println(result); + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedOrTernaryRecursive() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineHardThing(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; + } + } + """, + """ + class Test { + public String determineHardThing(String a, String b, String c) { + if("a".equals(a)){ + return "a"; + } + if("b".equals(b)){ + return "b"; + } + return "c".equals(b) ? "c" : "nope"; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedAndTernaryWithIfThenTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineHardThing(String a, String b) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; + } + } + """, + """ + class Test { + public String determineHardThing(String a, String b) { + if("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "nope"; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineHardThing(String a, String b, String c) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; + } + } + """, + """ + class Test { + public String determineHardThing(String a, String b) { + if("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "c".equals(b) ? "c" : "nope"; + } + } + """ + ) + ); + } + } \ No newline at end of file From 67f4d010cc2f7415462a1721a796c44b2ee013e3 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sun, 7 May 2023 21:01:39 +0200 Subject: [PATCH 06/45] try something different --- .../TernaryOperatorsShouldNotBeNested.java | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 9975a3b888..d155b4dbd7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -1,7 +1,8 @@ package org.openrewrite.staticanalysis; import java.time.Duration; -import java.util.List; +import java.util.Arrays; +import java.util.Collections; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; @@ -10,8 +11,8 @@ import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JRightPadded; import org.openrewrite.java.tree.Space; -import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; @@ -34,6 +35,7 @@ public Duration getEstimatedEffortPerOccurrence() { return Duration.ofMinutes(5); } + @Override public TreeVisitor getVisitor() { return new TernaryOperatorsShouldNotBeNestedVisitor(); @@ -41,35 +43,37 @@ public TreeVisitor getVisitor() { private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { - private final JavaTemplate splitNestedFalsePart = JavaTemplate.builder( + final JavaTemplate iffTemplate = JavaTemplate.builder( this::getCursor, - "if(#{any(boolean)})" + "if(#{any(boolean)}) {}" ) .build(); @Override - public J visitStatement(final Statement statement, final ExecutionContext executionContext) { - //if statement contains a nested ternary, clone "statement part" and split? - // return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; - - - - return super.visitStatement(statement, executionContext); - } - - @Override - public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { - if (ternary.getFalsePart() instanceof J.Ternary) { - System.out.println("Ternary nesting found: " + ternary.getFalsePart()); - //todo replace with: - // if(ternary.getCondition()){ - // return ternary.getTruePart(); - // } - // return ternary.getFalsePart(); - // - // return is not actually part of the ternary, so how to "clone" that? + public J visitReturn(final J.Return retrn, final ExecutionContext executionContext) { + J possiblyTernary = retrn.getExpression(); + if (possiblyTernary instanceof J.Ternary) { + J.Ternary ternary = (J.Ternary) possiblyTernary; + if (ternary.getFalsePart() instanceof J.Ternary) { + J.If iff = retrn.withTemplate( + iffTemplate, + retrn.getCoordinates().replace(), + ternary.getCondition() + ); + iff = iff.withThenPart(new J.Block( + Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + JRightPadded.build(false), + Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getTruePart()))), + Space.EMPTY + )); + J result = getCursor().firstEnclosingOrThrow(J.Block.class).withStatements(Arrays.asList(iff, + retrn.withExpression(ternary.getFalsePart()))); + return autoFormat(result, executionContext); + } } - return super.visitTernary(ternary, executionContext); + return super.visitReturn(retrn, executionContext); } } } From e80ae76440d106a0472462b0b566f7480afcc8ff Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sun, 7 May 2023 21:19:24 +0200 Subject: [PATCH 07/45] reorder tests --- .../TernaryOperatorsShouldNotBeNested.java | 23 +++++- ...TernaryOperatorsShouldNotBeNestedTest.java | 70 +++++++++---------- 2 files changed, 55 insertions(+), 38 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d155b4dbd7..e0023f74be 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -13,6 +13,7 @@ import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JRightPadded; import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; @@ -49,6 +50,12 @@ private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisito ) .build(); + @Override + public J visitAssignment(final J.Assignment assignment, final ExecutionContext executionContext) { + //todo + return super.visitAssignment(assignment, executionContext); + } + @Override public J visitReturn(final J.Return retrn, final ExecutionContext executionContext) { J possiblyTernary = retrn.getExpression(); @@ -67,10 +74,20 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte JRightPadded.build(false), Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getTruePart()))), Space.EMPTY + )).withElsePart(new J.If.Else( + Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + JRightPadded.build(new J.Block( + Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + JRightPadded.build(false), + Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getFalsePart()))), + Space.EMPTY + )) )); - J result = getCursor().firstEnclosingOrThrow(J.Block.class).withStatements(Arrays.asList(iff, - retrn.withExpression(ternary.getFalsePart()))); - return autoFormat(result, executionContext); + return autoFormat(iff, executionContext); } } return super.visitReturn(retrn, executionContext); diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index f56757129e..20ade9052d 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -1,6 +1,5 @@ package org.openrewrite.staticanalysis; -import static org.junit.jupiter.api.Assertions.*; import static org.openrewrite.java.Assertions.java; import org.junit.jupiter.api.Test; @@ -21,14 +20,14 @@ void doReplaceNestedOrTernaryWithIfFollowedByTernary() { java( """ class Test { - public String determineHardThing(String a, String b) { + public String determineSomething(String a, String b) { return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; } } """, """ class Test { - public String determineHardThing(String a, String b) { + public String determineSomething(String a, String b) { if("a".equals(a)){ return "a"; } @@ -41,29 +40,27 @@ public String determineHardThing(String a, String b) { } @Test - void doReplaceNestedOrTernaryWithIfFollowedByTernaryWithoutReturn() { + void doReplaceNestedOrTernaryRecursive() { rewriteRun( //language=java java( """ class Test { - public void doThing(String a, String b) { - String result = "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; - System.out.println(result); + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; } } """, """ class Test { - public void doThing(String a, String b) { - String result; + public String determineSomething(String a, String b, String c) { if("a".equals(a)){ - result = "a"; + return "a"; } - else { - result = "b".equals(b) ? "b" : "nope"; + if("b".equals(b)){ + return "b"; } - System.out.println(result); + return "c".equals(b) ? "c" : "nope"; } } """ @@ -72,27 +69,24 @@ public void doThing(String a, String b) { } @Test - void doReplaceNestedOrTernaryRecursive() { + void doReplaceNestedAndTernaryWithIfThenTernary() { rewriteRun( //language=java java( """ class Test { - public String determineHardThing(String a, String b, String c) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; + public String determineSomething(String a, String b) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; } } """, """ class Test { - public String determineHardThing(String a, String b, String c) { - if("a".equals(a)){ - return "a"; - } - if("b".equals(b)){ - return "b"; + public String determineSomething(String a, String b) { + if("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; } - return "c".equals(b) ? "c" : "nope"; + return "nope"; } } """ @@ -101,24 +95,24 @@ public String determineHardThing(String a, String b, String c) { } @Test - void doReplaceNestedAndTernaryWithIfThenTernary() { + void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { rewriteRun( //language=java java( """ class Test { - public String determineHardThing(String a, String b) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; } } """, """ class Test { - public String determineHardThing(String a, String b) { + public String determineSomething(String a, String b) { if("a".equals(a)) { return "b".equals(b) ? "b" : "a"; } - return "nope"; + return "c".equals(b) ? "c" : "nope"; } } """ @@ -126,25 +120,31 @@ public String determineHardThing(String a, String b) { ); } + @Test - void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { + void doReplaceNestedOrAssignmentTernaryWithIfElse() { rewriteRun( //language=java java( """ class Test { - public String determineHardThing(String a, String b, String c) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; + public void doThing(String a, String b) { + String result = "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + System.out.println(result); } } """, """ class Test { - public String determineHardThing(String a, String b) { - if("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; + public void doThing(String a, String b) { + String result; + if("a".equals(a)){ + result = "a"; } - return "c".equals(b) ? "c" : "nope"; + else { + result = "b".equals(b) ? "b" : "nope"; + } + System.out.println(result); } } """ From 44b6f6600ff8cbd3b4776d29e3d4e93168ab74a1 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sun, 7 May 2023 21:29:50 +0200 Subject: [PATCH 08/45] revert to return extra block --- .../TernaryOperatorsShouldNotBeNested.java | 22 +++++++++---------- ...TernaryOperatorsShouldNotBeNestedTest.java | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index e0023f74be..e9a7ecb492 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -13,7 +13,6 @@ import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JRightPadded; import org.openrewrite.java.tree.Space; -import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; @@ -74,20 +73,19 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte JRightPadded.build(false), Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getTruePart()))), Space.EMPTY - )).withElsePart(new J.If.Else( + )); + J result = new J.Block( Tree.randomId(), Space.EMPTY, Markers.EMPTY, - JRightPadded.build(new J.Block( - Tree.randomId(), - Space.EMPTY, - Markers.EMPTY, - JRightPadded.build(false), - Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getFalsePart()))), - Space.EMPTY - )) - )); - return autoFormat(iff, executionContext); + JRightPadded.build(false), + Arrays.asList( + JRightPadded.build(iff), + JRightPadded.build(retrn.withExpression(ternary.getFalsePart())) + ), + Space.EMPTY + ); + return autoFormat(result, executionContext); } } return super.visitReturn(retrn, executionContext); diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 20ade9052d..d0019faa98 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -120,7 +120,7 @@ public String determineSomething(String a, String b) { ); } - + //todo maybe not take this into consideration at all. @Test void doReplaceNestedOrAssignmentTernaryWithIfElse() { rewriteRun( From fab13fa5f7623bf85302e0ccc844dbae151f4d03 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Sun, 7 May 2023 21:33:31 +0200 Subject: [PATCH 09/45] add todo --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index e9a7ecb492..d1e4bae13d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -60,6 +60,7 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte J possiblyTernary = retrn.getExpression(); if (possiblyTernary instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) possiblyTernary; + //todo handle TruePart being nested ternary and combine with below if (ternary.getFalsePart() instanceof J.Ternary) { J.If iff = retrn.withTemplate( iffTemplate, From 5495fd35c4f51f8380127f750036228edb97eced Mon Sep 17 00:00:00 2001 From: pstreef Date: Sun, 7 May 2023 21:46:58 +0200 Subject: [PATCH 10/45] helpers and dont reuse old return (maybe ids will conflict?) --- .../TernaryOperatorsShouldNotBeNested.java | 33 ++++++++----------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d1e4bae13d..a8a867fc54 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -2,7 +2,7 @@ import java.time.Duration; import java.util.Arrays; -import java.util.Collections; +import java.util.stream.Collectors; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; @@ -10,9 +10,11 @@ import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JRightPadded; import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; @@ -67,29 +69,20 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte retrn.getCoordinates().replace(), ternary.getCondition() ); - iff = iff.withThenPart(new J.Block( - Tree.randomId(), - Space.EMPTY, - Markers.EMPTY, - JRightPadded.build(false), - Collections.singletonList(JRightPadded.build(retrn.withExpression(ternary.getTruePart()))), - Space.EMPTY - )); - J result = new J.Block( - Tree.randomId(), - Space.EMPTY, - Markers.EMPTY, - JRightPadded.build(false), - Arrays.asList( - JRightPadded.build(iff), - JRightPadded.build(retrn.withExpression(ternary.getFalsePart())) - ), - Space.EMPTY - ); + iff = iff.withThenPart(block(returnOf(ternary.getTruePart()))); + J result = block(iff, returnOf(ternary.getFalsePart())); return autoFormat(result, executionContext); } } return super.visitReturn(retrn, executionContext); } } + + private static J.Return returnOf(Expression expression) { + return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); + } + + private static J.Block block(Statement... statements) { + return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); + } } From b4409cb41e17db179f47cfe1536832d03d73c238 Mon Sep 17 00:00:00 2001 From: pstreef Date: Sun, 7 May 2023 21:59:37 +0200 Subject: [PATCH 11/45] add truePart --- .../TernaryOperatorsShouldNotBeNested.java | 2 +- ...TernaryOperatorsShouldNotBeNestedTest.java | 52 +++++++++---------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index a8a867fc54..95593e309b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -63,7 +63,7 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte if (possiblyTernary instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) possiblyTernary; //todo handle TruePart being nested ternary and combine with below - if (ternary.getFalsePart() instanceof J.Ternary) { + if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { J.If iff = retrn.withTemplate( iffTemplate, retrn.getCoordinates().replace(), diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index d0019faa98..c5871b96d9 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -28,10 +28,10 @@ public String determineSomething(String a, String b) { """ class Test { public String determineSomething(String a, String b) { - if("a".equals(a)){ - return "a"; - } - return "b".equals(b) ? "b" : "nope"; + if ("a".equals(a)) { + return "a"; + } + return "b".equals(b) ? "b" : "nope"; } } """ @@ -54,13 +54,13 @@ public String determineSomething(String a, String b, String c) { """ class Test { public String determineSomething(String a, String b, String c) { - if("a".equals(a)){ - return "a"; - } - if("b".equals(b)){ - return "b"; - } - return "c".equals(b) ? "c" : "nope"; + if ("a".equals(a)) { + return "a"; + } + if ("b".equals(b)) { + return "b"; + } + return "c".equals(b) ? "c" : "nope"; } } """ @@ -83,10 +83,10 @@ public String determineSomething(String a, String b) { """ class Test { public String determineSomething(String a, String b) { - if("a".equals(a)) { + if ("a".equals(a)) { return "b".equals(b) ? "b" : "a"; - } - return "nope"; + } + return "nope"; } } """ @@ -109,10 +109,10 @@ public String determineSomething(String a, String b, String c) { """ class Test { public String determineSomething(String a, String b) { - if("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; - } - return "c".equals(b) ? "c" : "nope"; + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "c".equals(b) ? "c" : "nope"; } } """ @@ -137,14 +137,14 @@ public void doThing(String a, String b) { """ class Test { public void doThing(String a, String b) { - String result; - if("a".equals(a)){ - result = "a"; - } - else { - result = "b".equals(b) ? "b" : "nope"; - } - System.out.println(result); + String result; + if ("a".equals(a)) { + result = "a"; + } + else { + result = "b".equals(b) ? "b" : "nope"; + } + System.out.println(result); } } """ From 0741ab1c390775f68fc3907a245fc42f62e407e0 Mon Sep 17 00:00:00 2001 From: pstreef Date: Sun, 7 May 2023 22:00:04 +0200 Subject: [PATCH 12/45] and remove todo --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 95593e309b..69890564b4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -62,7 +62,6 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte J possiblyTernary = retrn.getExpression(); if (possiblyTernary instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) possiblyTernary; - //todo handle TruePart being nested ternary and combine with below if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { J.If iff = retrn.withTemplate( iffTemplate, From f4d10b4d397858fcbc970ad8bdd7bef68d982193 Mon Sep 17 00:00:00 2001 From: pstreef Date: Sun, 7 May 2023 22:01:34 +0200 Subject: [PATCH 13/45] add todo --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 69890564b4..2c1a4839e1 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -69,6 +69,7 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte ternary.getCondition() ); iff = iff.withThenPart(block(returnOf(ternary.getTruePart()))); + //todo in stead of returning a new block here this should be inserted into the existing one. J result = block(iff, returnOf(ternary.getFalsePart())); return autoFormat(result, executionContext); } From a45963bf90b39e29d2d72162c3bae270e100c53b Mon Sep 17 00:00:00 2001 From: pstreef Date: Sun, 7 May 2023 22:22:00 +0200 Subject: [PATCH 14/45] override tags --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 2c1a4839e1..f0f3c4701d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -2,6 +2,8 @@ import java.time.Duration; import java.util.Arrays; +import java.util.Collections; +import java.util.Set; import java.util.stream.Collectors; import org.openrewrite.ExecutionContext; @@ -37,6 +39,10 @@ public Duration getEstimatedEffortPerOccurrence() { return Duration.ofMinutes(5); } + @Override + public Set getTags() { + return Collections.singleton("RSPEC-3358"); + } @Override public TreeVisitor getVisitor() { From 8d817c3437f77d4d1ca11892a382486b053fd996 Mon Sep 17 00:00:00 2001 From: pstreef Date: Mon, 8 May 2023 08:49:45 +0200 Subject: [PATCH 15/45] Fix tests --- .../TernaryOperatorsShouldNotBeNested.java | 19 +++++++++---------- ...TernaryOperatorsShouldNotBeNestedTest.java | 11 +++++++---- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index f0f3c4701d..d34b419f1e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -4,17 +4,17 @@ import java.util.Arrays; import java.util.Collections; import java.util.Set; -import java.util.stream.Collectors; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.lang.Nullable; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JRightPadded; +import org.openrewrite.java.tree.JavaSourceFile; import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; @@ -51,16 +51,15 @@ public TreeVisitor getVisitor() { private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { - final JavaTemplate iffTemplate = JavaTemplate.builder( - this::getCursor, - "if(#{any(boolean)}) {}" - ) - .build(); + final JavaTemplate iffTemplate = JavaTemplate.builder(this::getCursor, "if(#{any(boolean)}) {}").build(); @Override - public J visitAssignment(final J.Assignment assignment, final ExecutionContext executionContext) { - //todo - return super.visitAssignment(assignment, executionContext); + public @Nullable J visit(@Nullable final Tree tree, final ExecutionContext executionContext) { + J result = super.visit(tree, executionContext); + if (tree instanceof JavaSourceFile) { + result = (J) new RemoveUnneededBlock().getVisitor().visit(result, executionContext); + } + return result; } @Override diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index c5871b96d9..1fb498f61d 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -3,6 +3,8 @@ import static org.openrewrite.java.Assertions.java; import org.junit.jupiter.api.Test; +import org.junitpioneer.jupiter.ExpectedToFail; +import org.openrewrite.Issue; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; @@ -84,7 +86,7 @@ public String determineSomething(String a, String b) { class Test { public String determineSomething(String a, String b) { if ("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; + return "b".equals(b) ? "b" : "a"; } return "nope"; } @@ -108,11 +110,11 @@ public String determineSomething(String a, String b, String c) { """, """ class Test { - public String determineSomething(String a, String b) { + public String determineSomething(String a, String b, String c) { if ("a".equals(a)) { return "b".equals(b) ? "b" : "a"; } - return "c".equals(b) ? "c" : "nope"; + return "c".equals(c) ? "c" : "nope"; } } """ @@ -120,7 +122,8 @@ public String determineSomething(String a, String b) { ); } - //todo maybe not take this into consideration at all. + @Issue("todo") + @ExpectedToFail("only directly returned ternaries are taken into account") @Test void doReplaceNestedOrAssignmentTernaryWithIfElse() { rewriteRun( From 6fa72874f8fa93ed7a4403da51cb348f5d0820e0 Mon Sep 17 00:00:00 2001 From: pstreef Date: Mon, 8 May 2023 08:53:58 +0200 Subject: [PATCH 16/45] remove todo --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d34b419f1e..4d34162839 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -74,7 +74,6 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte ternary.getCondition() ); iff = iff.withThenPart(block(returnOf(ternary.getTruePart()))); - //todo in stead of returning a new block here this should be inserted into the existing one. J result = block(iff, returnOf(ternary.getFalsePart())); return autoFormat(result, executionContext); } From 00eb8f854644517c297fb8c5eb09c2424b2bf1fa Mon Sep 17 00:00:00 2001 From: pstreef Date: Mon, 8 May 2023 08:54:45 +0200 Subject: [PATCH 17/45] remove todo & rename method --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 4d34162839..22a293d69e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -73,8 +73,8 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte retrn.getCoordinates().replace(), ternary.getCondition() ); - iff = iff.withThenPart(block(returnOf(ternary.getTruePart()))); - J result = block(iff, returnOf(ternary.getFalsePart())); + iff = iff.withThenPart(blockOf(returnOf(ternary.getTruePart()))); + J result = blockOf(iff, returnOf(ternary.getFalsePart())); return autoFormat(result, executionContext); } } @@ -86,7 +86,7 @@ private static J.Return returnOf(Expression expression) { return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); } - private static J.Block block(Statement... statements) { + private static J.Block blockOf(Statement... statements) { return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); } } From a942e5e52e3a017ada24c1b0579d51952ff6cd03 Mon Sep 17 00:00:00 2001 From: pstreef Date: Mon, 8 May 2023 19:16:51 +0200 Subject: [PATCH 18/45] add more tests --- .../TernaryOperatorsShouldNotBeNested.java | 5 +- ...TernaryOperatorsShouldNotBeNestedTest.java | 161 ++++++++++++++++++ 2 files changed, 162 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 22a293d69e..4ec75f844a 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -28,10 +28,7 @@ public String getDisplayName() { @Override public String getDescription() { - return "Just because you can do something, doesn’t mean you should, and that’s the case with nested ternary operations. " + - "Nesting ternary operators results in the kind of code that may seem clear as day when you write it, " + - "but six months later will leave maintainers (or worse - future you) scratching their heads and cursing.\n\n" + - "Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement."; + return "Nested ternary operators can be hard to read quickly. Prefer simpler constructs for improved readability."; } @Override diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 1fb498f61d..5b8008bc2d 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -155,4 +155,165 @@ public void doThing(String a, String b) { ); } + @Issue("todo") + @ExpectedToFail("Not yet implemented") + @Test + void doReplaceNestedOrTernaryInStreamWithIfInBlock() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> { + if (item.startsWith("a")) { + return "a"; + } + return item.startsWith("b") ? "b" : "nope"; + }).collect(Collectors.toSet()); + } + } + """ + ) + ); + } + + + + @Test + void doReplaceNestedOrTernaryContainingNull() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? null : "b".equals(b) ? "b" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return null; + } + return "b".equals(b) ? "b" : null; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "foo" + "bar" : "b".equals(b) ? a + b : b + a; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "foo" + "bar"; + } + return "b".equals(b) ? a + b : b + a; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; //this should be behind the ternary + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + if ("a".equals(a)) { + return "a"; + } + return "b".equals(b) ? "b" : "nope"; //this should be behind the ternary + } + } + """ + ) + ); + } + + + + @Test + void doReplaceNestedOrTernaryContainingMethodCall() { + //language=java + rewriteRun( + java(""" + class M{ + static String a(){return "a";} + static String b(){return "b";} + static String c(){return "c";} + static String nope(){return "nope";} + } + """), + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? M.a() : "b".equals(b) ? M.b() : M.nope(); + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return M.a(); + } + return "b".equals(b) ? M.b() : M.nope(); + } + } + """ + ) + ); + } + + } \ No newline at end of file From dadbc955bf1dcf5c91a185562fd4682244744793 Mon Sep 17 00:00:00 2001 From: pstreef Date: Mon, 8 May 2023 21:53:02 +0200 Subject: [PATCH 19/45] add license --- .../TernaryOperatorsShouldNotBeNested.java | 15 +++++++++++++++ .../TernaryOperatorsShouldNotBeNestedTest.java | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 4ec75f844a..9b116faf4b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -1,3 +1,18 @@ +/* + * Copyright 2022 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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 java.time.Duration; diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 5b8008bc2d..69d43ea8aa 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2022 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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 static org.openrewrite.java.Assertions.java; From bbdc5de85f9c311d7c722223afdb5527ad8a09ef Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 9 May 2023 09:03:30 +0200 Subject: [PATCH 20/45] add test with whitespace --- ...TernaryOperatorsShouldNotBeNestedTest.java | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 69d43ea8aa..c73fcddef6 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -294,8 +294,6 @@ public String determineSomething(String a, String b) { ); } - - @Test void doReplaceNestedOrTernaryContainingMethodCall() { //language=java @@ -330,5 +328,37 @@ public String determineSomething(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryContainingNewlines() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) + ? null + : "b".equals(b) + ? "b" + : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return null; + } + return "b".equals(b) + ? "b" + : null; + } + } + """ + ) + ); + } + } \ No newline at end of file From e376b96ee9faba79dfbc010e256bf5106d9d5798 Mon Sep 17 00:00:00 2001 From: pstreef Date: Thu, 18 May 2023 14:12:25 +0200 Subject: [PATCH 21/45] Handle ternary in lambda --- .../TernaryOperatorsShouldNotBeNested.java | 36 +++++++++---- ...TernaryOperatorsShouldNotBeNestedTest.java | 54 ++++++++++++++++--- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 9b116faf4b..a5c4062644 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -20,6 +20,7 @@ import java.util.Collections; import java.util.Set; +import org.jetbrains.annotations.NotNull; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; @@ -29,6 +30,7 @@ import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JRightPadded; import org.openrewrite.java.tree.JavaSourceFile; import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; @@ -63,8 +65,6 @@ public TreeVisitor getVisitor() { private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { - final JavaTemplate iffTemplate = JavaTemplate.builder(this::getCursor, "if(#{any(boolean)}) {}").build(); - @Override public @Nullable J visit(@Nullable final Tree tree, final ExecutionContext executionContext) { J result = super.visit(tree, executionContext); @@ -74,19 +74,23 @@ private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisito return result; } + @Override + public J visitLambda(final J.Lambda lambda, final ExecutionContext executionContext) { + if (lambda.getBody() instanceof J.Ternary) { + J.Ternary ternary = (J.Ternary) lambda.getBody(); + J.If iff = ifOf(ternary); + return autoFormat(lambda.withBody(blockOf(iff, returnOf(ternary.getFalsePart())).withPrefix(ternary.getPrefix())), executionContext); + } + return super.visitLambda(lambda, executionContext); + } + @Override public J visitReturn(final J.Return retrn, final ExecutionContext executionContext) { J possiblyTernary = retrn.getExpression(); if (possiblyTernary instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) possiblyTernary; if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { - J.If iff = retrn.withTemplate( - iffTemplate, - retrn.getCoordinates().replace(), - ternary.getCondition() - ); - iff = iff.withThenPart(blockOf(returnOf(ternary.getTruePart()))); - J result = blockOf(iff, returnOf(ternary.getFalsePart())); + J result = blockOf(ifOf(ternary).withPrefix(retrn.getPrefix()), returnOf(ternary.getFalsePart())); return autoFormat(result, executionContext); } } @@ -94,6 +98,20 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte } } + @NotNull + private static J.If ifOf(final J.Ternary ternary) { + return new J.If( + Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, + JRightPadded.build(ternary.getCondition()) + ), + JRightPadded.build(blockOf(returnOf(ternary.getTruePart()))), + null + ); + } + private static J.Return returnOf(Expression expression) { return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index c73fcddef6..34f2ff7d28 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -170,8 +170,6 @@ public void doThing(String a, String b) { ); } - @Issue("todo") - @ExpectedToFail("Not yet implemented") @Test void doReplaceNestedOrTernaryInStreamWithIfInBlock() { rewriteRun( @@ -198,10 +196,10 @@ class Test { public Set makeASet() { List s = Arrays.asList("a","b","c","nope"); return s.stream().map(item -> { - if (item.startsWith("a")) { - return "a"; - } - return item.startsWith("b") ? "b" : "nope"; + if (item.startsWith("a")) { + return "a"; + } + return item.startsWith("b") ? "b" : "nope"; }).collect(Collectors.toSet()); } } @@ -210,6 +208,50 @@ public Set makeASet() { ); } + @Test + void doReplaceNestedOrTernaryInStreamContainingComments() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope" + ).collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + { + if (item.startsWith("a")) { + return "a"; + } + return item.startsWith("b") ? "b" : "nope"; + } + ).collect(Collectors.toSet()); + } + } + """ + ) + ); + } + @Test From 4f29673eaefacc922e30f3704b9b5fef9f1d94c1 Mon Sep 17 00:00:00 2001 From: pstreef Date: Thu, 18 May 2023 14:13:01 +0200 Subject: [PATCH 22/45] cleanup --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index a5c4062644..33fafe2aeb 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -98,7 +98,6 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte } } - @NotNull private static J.If ifOf(final J.Ternary ternary) { return new J.If( Tree.randomId(), From 8bd105f81aa1d5531ddede345b28015fccbbfc45 Mon Sep 17 00:00:00 2001 From: pstreef Date: Thu, 18 May 2023 14:13:27 +0200 Subject: [PATCH 23/45] cleanup --- .../TernaryOperatorsShouldNotBeNested.java | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 33fafe2aeb..fe94734484 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -96,26 +96,26 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte } return super.visitReturn(retrn, executionContext); } - } - private static J.If ifOf(final J.Ternary ternary) { - return new J.If( - Tree.randomId(), - Space.EMPTY, - Markers.EMPTY, - new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, - JRightPadded.build(ternary.getCondition()) - ), - JRightPadded.build(blockOf(returnOf(ternary.getTruePart()))), - null - ); - } + private static J.If ifOf(final J.Ternary ternary) { + return new J.If( + Tree.randomId(), + Space.EMPTY, + Markers.EMPTY, + new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, + JRightPadded.build(ternary.getCondition()) + ), + JRightPadded.build(blockOf(returnOf(ternary.getTruePart()))), + null + ); + } - private static J.Return returnOf(Expression expression) { - return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); - } + private static J.Return returnOf(Expression expression) { + return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); + } - private static J.Block blockOf(Statement... statements) { - return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); + private static J.Block blockOf(Statement... statements) { + return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); + } } } From 83ddd72ca8d24797f7c777be7823cef9599b449b Mon Sep 17 00:00:00 2001 From: pstreef Date: Thu, 18 May 2023 14:45:10 +0200 Subject: [PATCH 24/45] only when nested ternary --- .../TernaryOperatorsShouldNotBeNested.java | 9 ++++++-- ...TernaryOperatorsShouldNotBeNestedTest.java | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index fe94734484..9a806ee23d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -78,8 +78,13 @@ private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisito public J visitLambda(final J.Lambda lambda, final ExecutionContext executionContext) { if (lambda.getBody() instanceof J.Ternary) { J.Ternary ternary = (J.Ternary) lambda.getBody(); - J.If iff = ifOf(ternary); - return autoFormat(lambda.withBody(blockOf(iff, returnOf(ternary.getFalsePart())).withPrefix(ternary.getPrefix())), executionContext); + if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { + J.If iff = ifOf(ternary); + return autoFormat(lambda.withBody(blockOf( + iff, + returnOf(ternary.getFalsePart()) + ).withPrefix(ternary.getPrefix())), executionContext); + } } return super.visitLambda(lambda, executionContext); } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 34f2ff7d28..296ce8d07c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -170,6 +170,27 @@ public void doThing(String a, String b) { ); } + @Test + void doNotReplaceNonNestedOrTernaryInStream() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); + } + } + """ + ) + ); + } + @Test void doReplaceNestedOrTernaryInStreamWithIfInBlock() { rewriteRun( From 226bad08b590c2a30504d4a3e5b78444c8e80fb9 Mon Sep 17 00:00:00 2001 From: pstreef Date: Thu, 18 May 2023 14:48:43 +0200 Subject: [PATCH 25/45] add @Issue link --- .../staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 296ce8d07c..d1af8eb616 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -137,7 +137,7 @@ public String determineSomething(String a, String b, String c) { ); } - @Issue("todo") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/112") @ExpectedToFail("only directly returned ternaries are taken into account") @Test void doReplaceNestedOrAssignmentTernaryWithIfElse() { From d024aa1f92f1f89bfc3dc571b09383d9772c5884 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 23 May 2023 09:57:34 +0200 Subject: [PATCH 26/45] recursive approach for multi level ternaries. Add failing test for comment issue --- .../TernaryOperatorsShouldNotBeNested.java | 74 ++++++--- ...TernaryOperatorsShouldNotBeNestedTest.java | 145 ++++++++++++++++++ 2 files changed, 195 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 9a806ee23d..12189de785 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -20,13 +20,11 @@ import java.util.Collections; import java.util.Set; -import org.jetbrains.annotations.NotNull; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; import org.openrewrite.internal.lang.Nullable; -import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; @@ -76,51 +74,79 @@ private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisito @Override public J visitLambda(final J.Lambda lambda, final ExecutionContext executionContext) { - if (lambda.getBody() instanceof J.Ternary) { - J.Ternary ternary = (J.Ternary) lambda.getBody(); - if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { - J.If iff = ifOf(ternary); - return autoFormat(lambda.withBody(blockOf( - iff, - returnOf(ternary.getFalsePart()) - ).withPrefix(ternary.getPrefix())), executionContext); - } + J result = rewriteNestedTernary(lambda); + if (result == lambda) { + return super.visitLambda(lambda, executionContext); } - return super.visitLambda(lambda, executionContext); + return autoFormat(lambda.withBody(result.withPrefix(lambda.getBody().getPrefix())), executionContext); } @Override public J visitReturn(final J.Return retrn, final ExecutionContext executionContext) { - J possiblyTernary = retrn.getExpression(); - if (possiblyTernary instanceof J.Ternary) { - J.Ternary ternary = (J.Ternary) possiblyTernary; - if (ternary.getFalsePart() instanceof J.Ternary || ternary.getTruePart() instanceof J.Ternary) { - J result = blockOf(ifOf(ternary).withPrefix(retrn.getPrefix()), returnOf(ternary.getFalsePart())); - return autoFormat(result, executionContext); + J result = rewriteNestedTernary(retrn); + if (result == retrn) { + return super.visitReturn(retrn, executionContext); + } + return autoFormat(result, executionContext); + } + + private Statement rewriteNestedTernary(final Statement parent) { + J possibleTernary; + if (parent instanceof J.Return) { + possibleTernary = ((J.Return) parent).getExpression(); + } else if (parent instanceof J.Lambda) { + possibleTernary = ((J.Lambda) parent).getBody(); + } else { + return parent; + } + if (possibleTernary instanceof J.Ternary) { + J.Ternary ternary = (J.Ternary) possibleTernary; + if (!isNestedTernary(ternary)) { + return parent; } + J.If iff = ifOf(ternary); + J.Return otherwise = returnOf(ternary.getFalsePart()); + return blockOf(iff, rewriteNestedTernary(otherwise)).withPrefix(parent.getPrefix()); } - return super.visitReturn(retrn, executionContext); + return parent; } - private static J.If ifOf(final J.Ternary ternary) { + + private J.If ifOf(final J.Ternary ternary) { return new J.If( Tree.randomId(), Space.EMPTY, Markers.EMPTY, new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(ternary.getCondition()) - ), - JRightPadded.build(blockOf(returnOf(ternary.getTruePart()))), + ).withComments(ternary.getCondition().getComments()), + JRightPadded.build(blockOf(rewriteNestedTernary(returnOf(ternary.getTruePart())))), null ); } - private static J.Return returnOf(Expression expression) { - return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression); + private J.Return returnOf(Expression expression) { + return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression.withPrefix(Space.EMPTY)) + .withComments(expression.getComments()); } private static J.Block blockOf(Statement... statements) { return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); } + + private static boolean isNestedTernary(final J possibleTernary) { + int result = determineNestingLevels(possibleTernary, 0); + return result > 1; + } + + private static int determineNestingLevels(final J possibleTernary, final int level) { + if (!(possibleTernary instanceof J.Ternary)) { + return level; + } + J.Ternary ternary = (J.Ternary) possibleTernary; + int truePath = determineNestingLevels(ternary.getTruePart(), level + 1); + int falsePath = determineNestingLevels(ternary.getFalsePart(), level + 1); + return Math.max(falsePath, truePath); + } } } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index d1af8eb616..f3d56850e8 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -137,6 +137,151 @@ public String determineSomething(String a, String b, String c) { ); } + @Test + void doReplaceMultiLevelTernaries() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : + "b".equals(letter) ? "b" : + "c".equals(letter) ? "c" : + letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : + "e".equals(letter) ? "e" : + "f".equals(letter) ? "f" : + "g".equals(letter) ? "g" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + if ("a".equals(letter)) { + return "a"; + } + if ("b".equals(letter)) { + return "b"; + } + if ("c".equals(letter)) { + return "c"; + } + if (letter.contains("d")) { + if (letter.startsWith("d")) { + return letter.equals("d") ? "equals" : "startsWith"; + } + return "contains"; + } + if ("e".equals(letter)) { + return "e"; + } + if ("f".equals(letter)) { + return "f"; + } + return "g".equals(letter) ? "g" : "nope"; + } + } + """ + ) + ); + } + + @ExpectedToFail("Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, not sure how to extract that") + @Issue("todo") + @Test + void doReplaceMultiLevelTernariesWithComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : //look its a + "b".equals(letter) ? "b" : //b is also here + "c".equals(letter) ? "c" /* dont forget about c */ : + // d is important too + letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : + "e".equals(letter) ? "e" : //e + "f".equals(letter) ? "f" : //f + "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + if ("a".equals(letter)) { + return "a"; + }//look its a + if ("b".equals(letter)) { + return "b"; + }//b is also here + if ("c".equals(letter)) { + return "c"; /* dont forget about c */ + }// d is important too + if (letter.contains("d")) { + if (letter.startsWith("d")) { + return letter.equals("d") ? "equals" : "startsWith"; + } + return "contains"; + } + if ("e".equals(letter)) { + return "e"; + }//e + if ("f".equals(letter)) { + return "f"; + }//f + return "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """ + ) + ); + } + + @ExpectedToFail("Not yet implemented") + @Issue("todo") + @Test + void doReplaceMultiLevelTernariesWithSwitch() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : //look its a + "b".equals(letter) ? "b" : //b is also here + "c".equals(letter) ? "c" : /* dont forget about c */ + // d is important too + "d".equals(letter) ? "d" : + "e".equals(letter) ? "e" : //e + "f".equals(letter) ? "f" : //f + "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + return switch(letter) { + case "a" -> "a"; //look its a + case "b" -> "b"; //b is also here + case "c" -> "c"; /* dont forget about c */ + // d is important too + case "d" -> "d"; + case "e" -> "e"; //e + case "f" -> "f"; //f + case "g" -> "g"; + default -> "nope"; //and nope if nope + }; + } + } + """ + ) + ); + } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/112") @ExpectedToFail("only directly returned ternaries are taken into account") @Test From 97bc24ab382146626ab487aae69c1c1c2c72f7d2 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 09:13:56 +0200 Subject: [PATCH 27/45] getting there! --- .../staticanalysis/LowercasePackage.java | 2 +- .../staticanalysis/MethodNameCasing.java | 2 +- ...TernaryOperatorsShouldNotBeNestedTest.java | 1507 +++++++++++------ 3 files changed, 999 insertions(+), 512 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/LowercasePackage.java b/src/main/java/org/openrewrite/staticanalysis/LowercasePackage.java index f36d3d1a0f..ea857d8a9c 100644 --- a/src/main/java/org/openrewrite/staticanalysis/LowercasePackage.java +++ b/src/main/java/org/openrewrite/staticanalysis/LowercasePackage.java @@ -51,7 +51,7 @@ public Set getTags() { } @Override - public Map getInitialValue() { + public Map getInitialValue(final ExecutionContext ctx) { return new HashMap<>(); } diff --git a/src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java b/src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java index e173d41a81..5a4b88dd5f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java +++ b/src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java @@ -74,7 +74,7 @@ public Duration getEstimatedEffortPerOccurrence() { } @Override - public List getInitialValue() { + public List getInitialValue(final ExecutionContext ctx) { return new ArrayList<>(); } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index f3d56850e8..9d1f7be46c 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -17,555 +17,1042 @@ import static org.openrewrite.java.Assertions.java; +import java.util.function.Consumer; + +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.ExpectedToFail; import org.openrewrite.Issue; +import org.openrewrite.Tree; +import org.openrewrite.java.marker.JavaVersion; +import org.openrewrite.java.tree.J; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; +import org.openrewrite.test.SourceSpec; class TernaryOperatorsShouldNotBeNestedTest implements RewriteTest { + private static final Consumer> JAVA_17 = (spec) -> spec.markers(new JavaVersion( + Tree.randomId(), + "createdBy", + "vmVendor", + "17", + "17" + )); + + private static final Consumer> JAVA_11 = (spec) -> spec.markers(new JavaVersion( + Tree.randomId(), + "createdBy", + "vmVendor", + "11", + "11" + )); + + TernaryOperatorsShouldNotBeNested recipe = new TernaryOperatorsShouldNotBeNested(); + @Override public void defaults(RecipeSpec spec) { - spec.recipe(new TernaryOperatorsShouldNotBeNested()); + recipe = new TernaryOperatorsShouldNotBeNested(); + spec.recipe(recipe); } - @Test - void doReplaceNestedOrTernaryWithIfFollowedByTernary() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return "a"; - } - return "b".equals(b) ? "b" : "nope"; - } - } - """ - ) - ); - } + @Nested + class SwitchExpressionNotSupported { - @Test - void doReplaceNestedOrTernaryRecursive() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b, String c) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b, String c) { - if ("a".equals(a)) { - return "a"; - } - if ("b".equals(b)) { - return "b"; - } - return "c".equals(b) ? "c" : "nope"; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryWithIfFollowedByTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "a"; + } + return "b".equals(b) ? "b" : "nope"; + } + } + """, + JAVA_11 + ) + ); + } - @Test - void doReplaceNestedAndTernaryWithIfThenTernary() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; - } - return "nope"; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryRecursive() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "a"; + } + if ("b".equals(b)) { + return "b"; + } + return "c".equals(b) ? "c" : "nope"; + } + } + """, + JAVA_11 + ) + ); + } - @Test - void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b, String c) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b, String c) { - if ("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; - } - return "c".equals(c) ? "c" : "nope"; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedAndTernaryWithIfThenTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "nope"; + } + } + """, + JAVA_11 + ) + ); + } - @Test - void doReplaceMultiLevelTernaries() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String letter) { - return "a".equals(letter) ? "a" : - "b".equals(letter) ? "b" : - "c".equals(letter) ? "c" : - letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : - "e".equals(letter) ? "e" : - "f".equals(letter) ? "f" : - "g".equals(letter) ? "g" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String letter) { - if ("a".equals(letter)) { - return "a"; - } - if ("b".equals(letter)) { - return "b"; - } - if ("c".equals(letter)) { - return "c"; - } - if (letter.contains("d")) { - if (letter.startsWith("d")) { - return letter.equals("d") ? "equals" : "startsWith"; + @Test + void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "c".equals(c) ? "c" : "nope"; + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceMultiLevelTernaries() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : + "b".equals(letter) ? "b" : + "c".equals(letter) ? "c" : + letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : + "e".equals(letter) ? "e" : + "f".equals(letter) ? "f" : + "g".equals(letter) ? "g" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + if ("a".equals(letter)) { + return "a"; + } + if ("b".equals(letter)) { + return "b"; + } + if ("c".equals(letter)) { + return "c"; + } + if (letter.contains("d")) { + if (letter.startsWith("d")) { + return letter.equals("d") ? "equals" : "startsWith"; + } + return "contains"; + } + if ("e".equals(letter)) { + return "e"; + } + if ("f".equals(letter)) { + return "f"; + } + return "g".equals(letter) ? "g" : "nope"; + } + } + """, + JAVA_11 + ) + ); + } + + @ExpectedToFail( + "Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, not sure how to extract that") + @Issue("todo") + @Test + void doReplaceMultiLevelTernariesWithComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : //look its a + "b".equals(letter) ? "b" : //b is also here + "c".equals(letter) ? "c" /* dont forget about c */ : + // d is important too + letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : + "e".equals(letter) ? "e" : //e + "f".equals(letter) ? "f" : //f + "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + if ("a".equals(letter)) { + return "a"; + }//look its a + if ("b".equals(letter)) { + return "b"; + }//b is also here + if ("c".equals(letter)) { + return "c"; /* dont forget about c */ + }// d is important too + if (letter.contains("d")) { + if (letter.startsWith("d")) { + return letter.equals("d") ? "equals" : "startsWith"; + } + return "contains"; + } + if ("e".equals(letter)) { + return "e"; + }//e + if ("f".equals(letter)) { + return "f"; + }//f + return "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """, + JAVA_11 + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/112") + @ExpectedToFail("only directly returned ternaries are taken into account") + @Test + void doReplaceNestedOrAssignmentTernaryWithIfElse() { + rewriteRun( + //language=java + java( + """ + class Test { + public void doThing(String a, String b) { + String result = "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + System.out.println(result); + } + } + """, + """ + class Test { + public void doThing(String a, String b) { + String result; + if ("a".equals(a)) { + result = "a"; + } + else { + result = "b".equals(b) ? "b" : "nope"; + } + System.out.println(result); + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doNotReplaceNonNestedOrTernaryInStream() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryInStreamWithIfInBlock() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> { + if (item.startsWith("a")) { + return "a"; + } + return item.startsWith("b") ? "b" : "nope"; + }).collect(Collectors.toSet()); + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryInStreamContainingComments() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope" + ).collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + { + if (item.startsWith("a")) { + return "a"; + } + return item.startsWith("b") ? "b" : "nope"; + } + ).collect(Collectors.toSet()); + } + } + """, + JAVA_11 + ) + ); + } + + + @Test + void doReplaceNestedOrTernaryContainingNull() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? null : "b".equals(b) ? "b" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return null; + } + return "b".equals(b) ? "b" : null; + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "foo" + "bar" : "b".equals(b) ? a + b : b + a; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "foo" + "bar"; } - return "contains"; + return "b".equals(b) ? a + b : b + a; } - if ("e".equals(letter)) { - return "e"; + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; //this should be behind the ternary } - if ("f".equals(letter)) { - return "f"; + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + if ("a".equals(a)) { + return "a"; + } + return "b".equals(b) ? "b" : "nope"; //this should be behind the ternary } - return "g".equals(letter) ? "g" : "nope"; - } - } - """ - ) - ); - } + } + """, + JAVA_11 + ) + ); + } - @ExpectedToFail("Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, not sure how to extract that") - @Issue("todo") - @Test - void doReplaceMultiLevelTernariesWithComments() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String letter) { - return "a".equals(letter) ? "a" : //look its a - "b".equals(letter) ? "b" : //b is also here - "c".equals(letter) ? "c" /* dont forget about c */ : - // d is important too - letter.contains("d") ? letter.startsWith("d") ? letter.equals("d") ? "equals" : "startsWith" : "contains" : - "e".equals(letter) ? "e" : //e - "f".equals(letter) ? "f" : //f - "g".equals(letter) ? "g" : "nope"; //and nope if nope + @Test + void doReplaceNestedOrTernaryContainingMethodCall() { + //language=java + rewriteRun( + java(""" + class M{ + static String a(){return "a";} + static String b(){return "b";} + static String c(){return "c";} + static String nope(){return "nope";} } - } - """, - """ - class Test { - public String determineSomething(String letter) { - if ("a".equals(letter)) { - return "a"; - }//look its a - if ("b".equals(letter)) { - return "b"; - }//b is also here - if ("c".equals(letter)) { - return "c"; /* dont forget about c */ - }// d is important too - if (letter.contains("d")) { - if (letter.startsWith("d")) { - return letter.equals("d") ? "equals" : "startsWith"; + """), + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? M.a() : "b".equals(b) ? M.b() : M.nope(); + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return M.a(); } - return "contains"; - } - if ("e".equals(letter)) { - return "e"; - }//e - if ("f".equals(letter)) { - return "f"; - }//f - return "g".equals(letter) ? "g" : "nope"; //and nope if nope - } - } - """ - ) - ); - } + return "b".equals(b) ? M.b() : M.nope(); + } + } + """, + JAVA_11 + ) + ); + } - @ExpectedToFail("Not yet implemented") - @Issue("todo") - @Test - void doReplaceMultiLevelTernariesWithSwitch() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String letter) { - return "a".equals(letter) ? "a" : //look its a - "b".equals(letter) ? "b" : //b is also here - "c".equals(letter) ? "c" : /* dont forget about c */ - // d is important too - "d".equals(letter) ? "d" : - "e".equals(letter) ? "e" : //e - "f".equals(letter) ? "f" : //f - "g".equals(letter) ? "g" : "nope"; //and nope if nope - } - } - """, - """ - class Test { - public String determineSomething(String letter) { - return switch(letter) { - case "a" -> "a"; //look its a - case "b" -> "b"; //b is also here - case "c" -> "c"; /* dont forget about c */ - // d is important too - case "d" -> "d"; - case "e" -> "e"; //e - case "f" -> "f"; //f - case "g" -> "g"; - default -> "nope"; //and nope if nope - }; - } - } - """ - ) - ); + @Test + void doReplaceNestedOrTernaryContainingNewlines() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) + ? null + : "b".equals(b) + ? "b" + : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return null; + } + return "b".equals(b) + ? "b" + : null; + } + } + """, + JAVA_11 + ) + ); + } } - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/112") - @ExpectedToFail("only directly returned ternaries are taken into account") - @Test - void doReplaceNestedOrAssignmentTernaryWithIfElse() { - rewriteRun( - //language=java - java( - """ - class Test { - public void doThing(String a, String b) { - String result = "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; - System.out.println(result); - } - } - """, - """ - class Test { - public void doThing(String a, String b) { - String result; - if ("a".equals(a)) { - result = "a"; - } - else { - result = "b".equals(b) ? "b" : "nope"; - } - System.out.println(result); - } - } - """ - ) - ); - } + @Nested + class SwitchExpressionSupported { - @Test - void doNotReplaceNonNestedOrTernaryInStream() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryWithSwitchExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }; + } + } + """, + JAVA_17 + ) + ); + } - @Test - void doReplaceNestedOrTernaryInStreamWithIfInBlock() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); - } - } - """, - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> { - if (item.startsWith("a")) { - return "a"; - } - return item.startsWith("b") ? "b" : "nope"; - }).collect(Collectors.toSet()); - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryRecursive() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "a"; + } + if ("b".equals(b)) { + return "b"; + } + return "c".equals(b) ? "c" : "nope"; + } + } + """, + JAVA_17 + ) + ); + } - @Test - void doReplaceNestedOrTernaryInStreamContainingComments() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> - //look a ternary - item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope" - ).collect(Collectors.toSet()); - } - } - """, - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> - //look a ternary - { - if (item.startsWith("a")) { - return "a"; - } - return item.startsWith("b") ? "b" : "nope"; - } - ).collect(Collectors.toSet()); - } - } - """ - ) - ); - } + @Test + void doReplaceNestedAndTernaryWithIfThenTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "nope"; + } + } + """, + JAVA_17 + ) + ); + } + @Test + void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "c".equals(c) ? "c" : "nope"; + } + } + """ + ) + ); + } + @Test + void doReplaceMultiLevelTernariesWithSwitchExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : //look its a + "b".equals(letter) ? "b" : //b is also here + "c".equals(letter) ? "c" : /* dont forget about c */ + // d is important too + "d".equals(letter) ? "d" : + "e".equals(letter) ? "e" : //e + "f".equals(letter) ? "f" : //f + "g".equals(letter) ? "g" : "nope"; //and nope if nope + } + } + """, + """ + class Test { + public String determineSomething(String letter) { + return switch (letter) { + case "a" -> "a"; //look its a + case "b" -> "b"; //b is also here + case "c" -> "c"; /* dont forget about c */ + // d is important too + case "d" -> "d"; + case "e" -> "e"; //e + case "f" -> "f"; //f + case "g" -> "g"; + default -> "nope"; + }; //and nope if nope + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingNull() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? null : "b".equals(b) ? "b" : null; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return null; - } - return "b".equals(b) ? "b" : null; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrAssignmentTernaryWithSwitch() { + rewriteRun( + //language=java + java( + """ + class Test { + public void doThing(String a) { + String result = "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; + System.out.println(result); + } + } + """, + """ + class Test { + public void doThing(String a, String b) { + String result = switch (a) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }; + System.out.println(result); + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingExpression() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "foo" + "bar" : "b".equals(b) ? a + b : b + a; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return "foo" + "bar"; - } - return "b".equals(b) ? a + b : b + a; - } - } - """ - ) - ); - } + @Test + void doNotReplaceNonNestedOrTernaryInStreamWithSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingComments() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - //this should be before the if and followed by a new line - - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; //this should be behind the ternary - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - //this should be before the if and followed by a new line + @Test + void doReplaceNestedOrTernaryInStreamWithSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.equals("a") ? "a" : item.equals("b") ? "b" : "nope").collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> switch (item) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }).collect(Collectors.toSet()); + } + } + """, + JAVA_17 + ) + ); + } + + @ExpectedToFail("Pattern matching not yet implemented") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryInStreamWithPatternMatchingSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> + switch (item) { + case String st && st.startsWith("a") -> "a"; + case String st && st.startsWith("b") -> "b"; + default -> "nope"; + } + ).collect(Collectors.toSet()); + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryInStreamContainingComments() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + item.equals("a") ? "a" : item.equals("b") ? "b" : "nope" + ).collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + switch (item) { + case "a" -> "a"; + case item -> "b"; + default -> "nope"; + } + ).collect(Collectors.toSet()); + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingNull() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a) { + return "a".equals(a) ? null : "b".equals(a) ? "b" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a) { + return switch (a) { + case "a" -> null; + case "b" -> "b"; + default -> null; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @ExpectedToFail("not yet implemented collapsing cases") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryContainingNullCollapsingSameCases() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a) { + return "a".equals(a) ? null : "b".equals(a) ? "b" : "c".equals(a) ? "c" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "b" -> "b"; + case "c" -> "c"; + default -> null; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "foo" + "bar" : "b".equals(a) ? a + b : b + a; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "foo" + "bar"; + case "b" -> a + b; + default -> b + a; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line - if ("a".equals(a)) { - return "a"; + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; //this should be behind the ternary } - return "b".equals(b) ? "b" : "nope"; //this should be behind the ternary - } - } - """ - ) - ); - } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + return switch (a) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }; //this should be behind the ternary + } + } + """, + JAVA_17 + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingMethodCall() { - //language=java - rewriteRun( - java(""" - class M{ - static String a(){return "a";} - static String b(){return "b";} - static String c(){return "c";} - static String nope(){return "nope";} - } - """), - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? M.a() : "b".equals(b) ? M.b() : M.nope(); + @Test + void doReplaceNestedOrTernaryContainingMethodCall() { + //language=java + rewriteRun( + java(""" + class M{ + static String a(){return "a";} + static String b(){return "b";} + static String c(){return "c";} + static String nope(){return "nope";} } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return M.a(); - } - return "b".equals(b) ? M.b() : M.nope(); - } - } - """ - ) - ); - } + """), + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? M.a() : "b".equals(a) ? M.b() : M.nope(); + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> M.a(); + case "b" -> M.b(); + default -> M.nope(); + }; + } + } + """, + JAVA_17 + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingNewlines() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) - ? null - : "b".equals(b) - ? "b" - : null; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return null; - } - return "b".equals(b) - ? "b" - : null; - } - } - """ - ) - ); } From f1ab378a037dfe0b763f7a32549faecd22b0321e Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 09:14:02 +0200 Subject: [PATCH 28/45] getting there! --- .../TernaryOperatorsShouldNotBeNested.java | 204 +++++++++++++++--- 1 file changed, 169 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 12189de785..b365bfcf54 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -15,27 +15,37 @@ */ package org.openrewrite.staticanalysis; +import static org.openrewrite.Tree.randomId; + import java.time.Duration; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; -import org.openrewrite.internal.lang.Nullable; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.format.AutoFormat; +import org.openrewrite.java.marker.JavaVersion; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JContainer; import org.openrewrite.java.tree.JRightPadded; -import org.openrewrite.java.tree.JavaSourceFile; import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; public class TernaryOperatorsShouldNotBeNested extends Recipe { + @Override public String getDisplayName() { return "Ternary operators should not be nested"; @@ -43,7 +53,8 @@ public String getDisplayName() { @Override public String getDescription() { - return "Nested ternary operators can be hard to read quickly. Prefer simpler constructs for improved readability."; + return "Nested ternary operators can be hard to read quickly. Prefer simpler constructs for improved readability. " + + "If supported, this recipe will try to replace nested ternaries with switch expressions."; } @Override @@ -58,19 +69,25 @@ public Set getTags() { @Override public TreeVisitor getVisitor() { - return new TernaryOperatorsShouldNotBeNestedVisitor(); + return new JavaIsoVisitor() { + @Override + public J.CompilationUnit visitCompilationUnit( + final J.CompilationUnit cu, + final ExecutionContext executionContext + ) { + if (cu.getMarkers() + .findFirst(JavaVersion.class) + .filter(javaVersion -> javaVersion.getMajorVersion() >= 14) + .isPresent()) { + doAfterVisit(new NestedTernaryToSwitchExpressionVisitor()); + } + doAfterVisit(new NestedTernaryToIfVisitor()); + return super.visitCompilationUnit(cu, executionContext); + } + }; } - private static class TernaryOperatorsShouldNotBeNestedVisitor extends JavaVisitor { - - @Override - public @Nullable J visit(@Nullable final Tree tree, final ExecutionContext executionContext) { - J result = super.visit(tree, executionContext); - if (tree instanceof JavaSourceFile) { - result = (J) new RemoveUnneededBlock().getVisitor().visit(result, executionContext); - } - return result; - } + private static class NestedTernaryToIfVisitor extends JavaVisitor { @Override public J visitLambda(final J.Lambda lambda, final ExecutionContext executionContext) { @@ -78,6 +95,7 @@ public J visitLambda(final J.Lambda lambda, final ExecutionContext executionCont if (result == lambda) { return super.visitLambda(lambda, executionContext); } + doAfterVisit(new RemoveUnneededBlock()); return autoFormat(lambda.withBody(result.withPrefix(lambda.getBody().getPrefix())), executionContext); } @@ -87,28 +105,19 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte if (result == retrn) { return super.visitReturn(retrn, executionContext); } + doAfterVisit(new RemoveUnneededBlock()); return autoFormat(result, executionContext); } private Statement rewriteNestedTernary(final Statement parent) { - J possibleTernary; - if (parent instanceof J.Return) { - possibleTernary = ((J.Return) parent).getExpression(); - } else if (parent instanceof J.Lambda) { - possibleTernary = ((J.Lambda) parent).getBody(); - } else { - return parent; - } - if (possibleTernary instanceof J.Ternary) { - J.Ternary ternary = (J.Ternary) possibleTernary; + return findTernary(parent).map(ternary -> { if (!isNestedTernary(ternary)) { return parent; } J.If iff = ifOf(ternary); J.Return otherwise = returnOf(ternary.getFalsePart()); return blockOf(iff, rewriteNestedTernary(otherwise)).withPrefix(parent.getPrefix()); - } - return parent; + }).orElse(parent); } @@ -125,15 +134,6 @@ private J.If ifOf(final J.Ternary ternary) { ); } - private J.Return returnOf(Expression expression) { - return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression.withPrefix(Space.EMPTY)) - .withComments(expression.getComments()); - } - - private static J.Block blockOf(Statement... statements) { - return J.Block.createEmptyBlock().withStatements(Arrays.asList(statements)); - } - private static boolean isNestedTernary(final J possibleTernary) { int result = determineNestingLevels(possibleTernary, 0); return result > 1; @@ -148,5 +148,139 @@ private static int determineNestingLevels(final J possibleTernary, final int lev int falsePath = determineNestingLevels(ternary.getFalsePart(), level + 1); return Math.max(falsePath, truePath); } + + private static Optional findTernary(Statement parent) { + J possibleTernary = parent; + if (parent instanceof J.Return) { + possibleTernary = ((J.Return) parent).getExpression(); + } else if (parent instanceof J.Lambda) { + possibleTernary = ((J.Lambda) parent).getBody(); + } + if (possibleTernary instanceof J.Ternary) { + return Optional.of(possibleTernary).map(J.Ternary.class::cast); + } + return Optional.empty(); + } + } + + + static class NestedTernaryToSwitchExpressionVisitor extends JavaVisitor { + + @Override + public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { + return findConditionIdentifier(ternary).map(switchVar -> { + List nestList = new ArrayList<>(); + J.Ternary next = ternary; + while (next.getFalsePart() instanceof J.Ternary) { + if (next.getTruePart() instanceof J.Ternary) { + return null; + } + if (!findConditionIdentifier(next).filter(found -> found.equals(switchVar)).isPresent()) { + return null; + } + J.Ternary nested = (J.Ternary) next.getFalsePart(); + nestList.add(next); + next = nested; + } + nestList.add(next); + if (nestList.size() < 2) { + return null; + } + return autoFormat(toSwitch(switchVar, nestList), executionContext); + }).map(J.class::cast) + .orElseGet(() -> super.visitTernary(ternary, executionContext)); + } + + private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List nestList) { + J first = nestList.get(0); + J.Ternary last = nestList.get(nestList.size() - 1); + return new J.SwitchExpression( + Tree.randomId(), + first.getPrefix(), + first.getMarkers(), + new J.ControlParentheses<>( + Tree.randomId(), + switchVar.getPrefix().withWhitespace(" "), + switchVar.getMarkers(), + JRightPadded.build(switchVar) + ), + blockOf(Stream.concat( + nestList.stream().map(ternary -> toCase(switchVar, ternary)), + Stream.of(toDefault(last)) + ).collect(Collectors.toList())) + .withPrefix(Space.SINGLE_SPACE) + ); + } + + private J.Case toCase(final J.Identifier switchVar, final J.Ternary ternary) { + //todo could be something else + J.MethodInvocation inv = ((J.MethodInvocation) ternary.getCondition()); + Expression compare = inv.getSelect() == switchVar ? inv.getArguments().get(0) : inv.getSelect(); + return new J.Case( + Tree.randomId(), + ternary.getPrefix().withWhitespace(" "), + ternary.getMarkers(), + J.Case.Type.Rule, + JContainer.build( + Collections.singletonList(JRightPadded.build(compare.withPrefix(Space.SINGLE_SPACE)) + .withAfter(Space.SINGLE_SPACE)) + ), + JContainer.build(Collections.emptyList()), + JRightPadded.build(ternary.getTruePart()) + ); + } + + private J.Case toDefault(final J.Ternary ternary) { + return new J.Case( + Tree.randomId(), + Space.EMPTY, + ternary.getMarkers(), + J.Case.Type.Rule, + JContainer.build(Collections.singletonList(JRightPadded.build(new J.Identifier( + randomId(), + Space.EMPTY, + Markers.EMPTY, + "default", + null, + null + )).withAfter(Space.SINGLE_SPACE))), + JContainer.build(Collections.emptyList()), + JRightPadded.build(ternary.getFalsePart()) + ); + } + + private Optional findConditionIdentifier(final J.Ternary ternary) { + if (!(ternary.getCondition() instanceof J.MethodInvocation)) { + return Optional.empty(); + } + J.MethodInvocation inv = (J.MethodInvocation) ternary.getCondition(); + //todo get a if inv is ~like~ "a".equals(a) or a.equals("a") or Object.equals(a,"a") or Object.equals("a",a) + if (!inv.getSimpleName().equals("equals")) { + return Optional.empty(); + } + J.Identifier result = null; + if (inv.getSelect() instanceof J.Identifier) { + result = (J.Identifier) inv.getSelect(); + } + if (inv.getArguments().size() == 1 && inv.getArguments().get(0) instanceof J.Identifier) { + result = (J.Identifier) inv.getArguments().get(0); + } + return Optional.ofNullable(result); + } + } + + private static J.Return returnOf(Expression expression) { + return new J.Return(Tree.randomId(), Space.EMPTY, Markers.EMPTY, expression.withPrefix(Space.EMPTY)) + .withComments(expression.getComments()); + } + + private static J.Block blockOf(Statement... statements) { + return blockOf(Arrays.asList(statements)); + } + + private static J.Block blockOf(List statements) { + return J.Block.createEmptyBlock().withStatements(statements); + } + } From a88091483386169cc63cce1b2bc17d1d748fca4d Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 09:30:50 +0200 Subject: [PATCH 29/45] fix another test --- .../TernaryOperatorsShouldNotBeNested.java | 46 +++++++++++-------- ...TernaryOperatorsShouldNotBeNestedTest.java | 12 ++--- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index b365bfcf54..334b2d9a97 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -27,13 +27,13 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import org.jetbrains.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.JavaVisitor; -import org.openrewrite.java.format.AutoFormat; import org.openrewrite.java.marker.JavaVersion; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; @@ -79,15 +79,15 @@ public J.CompilationUnit visitCompilationUnit( .findFirst(JavaVersion.class) .filter(javaVersion -> javaVersion.getMajorVersion() >= 14) .isPresent()) { - doAfterVisit(new NestedTernaryToSwitchExpressionVisitor()); + doAfterVisit(new UseSwitchExpressionVisitor()); } - doAfterVisit(new NestedTernaryToIfVisitor()); + doAfterVisit(new UseIfVisitor()); return super.visitCompilationUnit(cu, executionContext); } }; } - private static class NestedTernaryToIfVisitor extends JavaVisitor { + private static class UseIfVisitor extends JavaVisitor { @Override public J visitLambda(final J.Lambda lambda, final ExecutionContext executionContext) { @@ -165,25 +165,13 @@ private static Optional findTernary(Statement parent) { } - static class NestedTernaryToSwitchExpressionVisitor extends JavaVisitor { + static class UseSwitchExpressionVisitor extends JavaVisitor { @Override public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { return findConditionIdentifier(ternary).map(switchVar -> { - List nestList = new ArrayList<>(); - J.Ternary next = ternary; - while (next.getFalsePart() instanceof J.Ternary) { - if (next.getTruePart() instanceof J.Ternary) { - return null; - } - if (!findConditionIdentifier(next).filter(found -> found.equals(switchVar)).isPresent()) { - return null; - } - J.Ternary nested = (J.Ternary) next.getFalsePart(); - nestList.add(next); - next = nested; - } - nestList.add(next); + List nestList = findNestedTernaries(ternary, switchVar); + if (nestList.size() < 2) { return null; } @@ -192,6 +180,26 @@ public J visitTernary(final J.Ternary ternary, final ExecutionContext executionC .orElseGet(() -> super.visitTernary(ternary, executionContext)); } + private List findNestedTernaries(final J.Ternary ternary, final J.Identifier switchVar) { + List nestList = new ArrayList<>(); + J.Ternary next = ternary; + while (next.getFalsePart() instanceof J.Ternary) { + if (next.getTruePart() instanceof J.Ternary) { + //todo this could become complicated if this is also nested. Lets skip it for now? + // maybe we can just leave a (nested) ternary here though. + return Collections.emptyList(); + } + J.Ternary nested = (J.Ternary) next.getFalsePart(); + if (!findConditionIdentifier(nested).filter(found -> found.equals(switchVar)).isPresent()) { + return Collections.emptyList(); + } + nestList.add(next); + next = nested; + } + nestList.add(next); + return nestList; + } + private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List nestList) { J first = nestList.get(0); J.Ternary last = nestList.get(nestList.size() - 1); diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 9d1f7be46c..97ac26ef54 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -609,21 +609,21 @@ void doReplaceNestedOrTernaryRecursive() { java( """ class Test { - public String determineSomething(String a, String b, String c) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(b) ? "c" :"nope"; + public String determineSomething(String aString, String bString, String cString) { + return "a".equals(aString) ? "a" : "b".equals(bString) ? "b" : "c".equals(cString) ? "c" :"nope"; } } """, """ class Test { - public String determineSomething(String a, String b, String c) { - if ("a".equals(a)) { + public String determineSomething(String aString, String bString, String cString) { + if ("a".equals(aString)) { return "a"; } - if ("b".equals(b)) { + if ("b".equals(bString)) { return "b"; } - return "c".equals(b) ? "c" : "nope"; + return "c".equals(cString) ? "c" : "nope"; } } """, From eb65ba1a841c17c25e092ff4d6f6202ce3fb80dc Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 11:14:16 +0200 Subject: [PATCH 30/45] add Objects.equals handling --- .../TernaryOperatorsShouldNotBeNested.java | 72 ++++++++-- ...TernaryOperatorsShouldNotBeNestedTest.java | 124 +++++++++++++++--- 2 files changed, 169 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 334b2d9a97..10aa291397 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -22,25 +22,29 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import org.jetbrains.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.lang.Nullable; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.RemoveImport; import org.openrewrite.java.marker.JavaVersion; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JContainer; import org.openrewrite.java.tree.JRightPadded; +import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; +import org.openrewrite.java.tree.TypeUtils; import org.openrewrite.marker.Markers; @@ -171,7 +175,6 @@ static class UseSwitchExpressionVisitor extends JavaVisitor { public J visitTernary(final J.Ternary ternary, final ExecutionContext executionContext) { return findConditionIdentifier(ternary).map(switchVar -> { List nestList = findNestedTernaries(ternary, switchVar); - if (nestList.size() < 2) { return null; } @@ -190,7 +193,9 @@ private List findNestedTernaries(final J.Ternary ternary, final J.Ide return Collections.emptyList(); } J.Ternary nested = (J.Ternary) next.getFalsePart(); - if (!findConditionIdentifier(nested).filter(found -> found.equals(switchVar)).isPresent()) { + if (!findConditionIdentifier(nested) + .filter(found -> isEqualVariable(switchVar, found)) + .isPresent()) { return Collections.emptyList(); } nestList.add(next); @@ -200,18 +205,25 @@ private List findNestedTernaries(final J.Ternary ternary, final J.Ide return nestList; } + private static boolean isEqualVariable(final J.Identifier switchVar, @Nullable final J found) { + if (!(found instanceof J.Identifier)) { + return false; + } + J.Identifier foundVar = (J.Identifier) found; + return Objects.equals(foundVar.getFieldType(), switchVar.getFieldType()); + } + private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List nestList) { - J first = nestList.get(0); J.Ternary last = nestList.get(nestList.size() - 1); return new J.SwitchExpression( Tree.randomId(), - first.getPrefix(), - first.getMarkers(), + Space.SINGLE_SPACE, + Markers.EMPTY, new J.ControlParentheses<>( Tree.randomId(), switchVar.getPrefix().withWhitespace(" "), switchVar.getMarkers(), - JRightPadded.build(switchVar) + JRightPadded.build(switchVar.withPrefix(Space.EMPTY)) ), blockOf(Stream.concat( nestList.stream().map(ternary -> toCase(switchVar, ternary)), @@ -224,7 +236,15 @@ private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List("java.util.Objects")); + compare = isVariable(inv.getArguments().get(0)) ? inv.getArguments().get(1) : inv.getArguments().get(0); + } else { + compare = isEqualVariable(switchVar, inv.getSelect()) + ? inv.getArguments().get(0) + : inv.getSelect(); + } return new J.Case( Tree.randomId(), ternary.getPrefix().withWhitespace(" "), @@ -268,14 +288,48 @@ private Optional findConditionIdentifier(final J.Ternary ternary) return Optional.empty(); } J.Identifier result = null; - if (inv.getSelect() instanceof J.Identifier) { + if (isVariable(inv.getSelect())) { result = (J.Identifier) inv.getSelect(); } if (inv.getArguments().size() == 1 && inv.getArguments().get(0) instanceof J.Identifier) { result = (J.Identifier) inv.getArguments().get(0); } + if (isObjectsEquals(inv)) { + //one has to be constant, other not + J first = inv.getArguments().get(0); + J second = inv.getArguments().get(1); + if (isVariable(first) && isVariable(second)) { + return Optional.empty(); + } + if (isVariable(first)) { + result = (J.Identifier) first; + } + if (isVariable(second)) { + result = (J.Identifier) second; + } + } return Optional.ofNullable(result); } + + private static boolean isVariable(@Nullable J maybeVariable) { + if (maybeVariable == null) { + return false; + } + if (!(maybeVariable instanceof J.Identifier)) { + return false; + } + J.Identifier identifier = (J.Identifier) maybeVariable; + return identifier.getFieldType() != null; + } + + private static boolean isObjectsEquals(J.MethodInvocation inv) { + if (inv.getSelect() instanceof J.Identifier) { + J.Identifier maybeObjects = (J.Identifier) inv.getSelect(); + boolean isObjects = TypeUtils.isOfType(maybeObjects.getType(), JavaType.buildType("java.util.Objects")); + return isObjects && "equals".equals(inv.getSimpleName()); + } + return false; + } } private static J.Return returnOf(Expression expression) { diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 97ac26ef54..67e41a731f 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -582,7 +582,7 @@ void doReplaceNestedOrTernaryWithSwitchExpression() { """ class Test { public String determineSomething(String a, String b) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "nope"; + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; } } """, @@ -602,6 +602,92 @@ public String determineSomething(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionInvertedEquals() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return a.equals("a") ? "a" : a.equals("b") ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEquals() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEqualsInverted() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals("a", a) ? "a" : Objects.equals("b", a) ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """, + JAVA_17 + ) + ); + } + @Test void doReplaceNestedOrTernaryRecursive() { rewriteRun( @@ -680,7 +766,8 @@ public String determineSomething(String a, String b, String c) { return "c".equals(c) ? "c" : "nope"; } } - """ + """, + JAVA_17 ) ); } @@ -720,7 +807,8 @@ public String determineSomething(String letter) { }; //and nope if nope } } - """ + """, + JAVA_17 ) ); } @@ -732,9 +820,9 @@ void doReplaceNestedOrAssignmentTernaryWithSwitch() { java( """ class Test { - public void doThing(String a) { - String result = "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; - System.out.println(result); + public void doThing(String a, String b) { + String result = "a".equals(a) ? "a" : "b".equals(a) ? b : "nope"; + System.out.println(result); } } """, @@ -742,14 +830,15 @@ public void doThing(String a) { class Test { public void doThing(String a, String b) { String result = switch (a) { - case "a" -> "a"; - case "b" -> "b"; - default -> "nope"; + case "a" -> "a"; + case "b" -> b; + default -> "nope"; }; System.out.println(result); } } - """ + """, + JAVA_17 ) ); } @@ -770,7 +859,8 @@ public Set makeASet() { return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); } } - """ + """, + JAVA_17 ) ); } @@ -883,13 +973,11 @@ public Set makeASet() { class Test { public Set makeASet() { List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> - //look a ternary - switch (item) { - case "a" -> "a"; - case item -> "b"; - default -> "nope"; - } + return s.stream().map( /* look a lambda */ item -> switch (item) { //look a ternary + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + } ).collect(Collectors.toSet()); } } From 8dc7e3e5261a472473ca96917234b11bb801a4b6 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 11:33:17 +0200 Subject: [PATCH 31/45] add binary conditions --- .../TernaryOperatorsShouldNotBeNested.java | 88 +++++++++----- ...TernaryOperatorsShouldNotBeNestedTest.java | 110 ++++++++++++++++++ 2 files changed, 166 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 10aa291397..d3272df214 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -234,17 +234,30 @@ private J.SwitchExpression toSwitch(final J.Identifier switchVar, final List("java.util.Objects")); - compare = isVariable(inv.getArguments().get(0)) ? inv.getArguments().get(1) : inv.getArguments().get(0); - } else { - compare = isEqualVariable(switchVar, inv.getSelect()) - ? inv.getArguments().get(0) - : inv.getSelect(); + if (ternary.getCondition() instanceof J.MethodInvocation) { + J.MethodInvocation inv = ((J.MethodInvocation) ternary.getCondition()); + if (isObjectsEquals(inv)) { + doAfterVisit(new RemoveImport<>("java.util.Objects")); + compare = isVariable(inv.getArguments().get(0)) + ? inv.getArguments().get(1) + : inv.getArguments().get(0); + } else { + compare = isEqualVariable(switchVar, inv.getSelect()) + ? inv.getArguments().get(0) + : inv.getSelect(); + } + } + else if (ternary.getCondition() instanceof J.Binary) { + J.Binary bin = ((J.Binary) ternary.getCondition()); + compare = isEqualVariable(switchVar, bin.getLeft()) + ? bin.getRight() + : bin.getLeft(); + } + else { + throw new IllegalArgumentException("Only J.Binary or J.MethodInvocation are expected as ternary conditions when creating a switch case"); } + return new J.Case( Tree.randomId(), ternary.getPrefix().withWhitespace(" "), @@ -279,36 +292,47 @@ private J.Case toDefault(final J.Ternary ternary) { } private Optional findConditionIdentifier(final J.Ternary ternary) { - if (!(ternary.getCondition() instanceof J.MethodInvocation)) { - return Optional.empty(); - } - J.MethodInvocation inv = (J.MethodInvocation) ternary.getCondition(); - //todo get a if inv is ~like~ "a".equals(a) or a.equals("a") or Object.equals(a,"a") or Object.equals("a",a) - if (!inv.getSimpleName().equals("equals")) { - return Optional.empty(); - } J.Identifier result = null; - if (isVariable(inv.getSelect())) { - result = (J.Identifier) inv.getSelect(); - } - if (inv.getArguments().size() == 1 && inv.getArguments().get(0) instanceof J.Identifier) { - result = (J.Identifier) inv.getArguments().get(0); - } - if (isObjectsEquals(inv)) { - //one has to be constant, other not - J first = inv.getArguments().get(0); - J second = inv.getArguments().get(1); - if (isVariable(first) && isVariable(second)) { + if (ternary.getCondition() instanceof J.MethodInvocation) { + J.MethodInvocation inv = (J.MethodInvocation) ternary.getCondition(); + //todo get a if inv is ~like~ "a".equals(a) or a.equals("a") or Object.equals(a,"a") or Object.equals("a",a) + if (!inv.getSimpleName().equals("equals")) { return Optional.empty(); } - if (isVariable(first)) { - result = (J.Identifier) first; + if (isVariable(inv.getSelect())) { + result = (J.Identifier) inv.getSelect(); + } + if (inv.getArguments().size() == 1 && inv.getArguments().get(0) instanceof J.Identifier) { + result = (J.Identifier) inv.getArguments().get(0); } - if (isVariable(second)) { - result = (J.Identifier) second; + if (isObjectsEquals(inv)) { + //one has to be constant, other not + J first = inv.getArguments().get(0); + J second = inv.getArguments().get(1); + result = xorVariable(first, second); } } + if (ternary.getCondition() instanceof J.Binary) { + J.Binary bin = (J.Binary) ternary.getCondition(); + result = xorVariable(bin.getLeft(), bin.getRight()); + } return Optional.ofNullable(result); + + } + + @Nullable + private static J.Identifier xorVariable(J first, J second) { + J.Identifier result = null; + if (isVariable(first) && isVariable(second)) { + return null; + } + if (isVariable(first)) { + result = (J.Identifier) first; + } + if (isVariable(second)) { + result = (J.Identifier) second; + } + return result; } private static boolean isVariable(@Nullable J maybeVariable) { diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 67e41a731f..2e08f7bac5 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -86,6 +86,60 @@ public String determineSomething(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryPrimitiveCondition() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a, int b) { + return a == 1 ? "one" : b == 2 ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a, int b) { + if (a == 1) { + return "one"; + } + return b == 2 ? "two" : "other"; + } + } + """, + JAVA_11 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryPrimitiveConditionInverted() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a, int b) { + return 1 == a ? "one" : 2 == b ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a, int b) { + if (1 == a) { + return "one"; + } + return 2 == b ? "two" : "other"; + } + } + """, + JAVA_11 + ) + ); + } + @Test void doReplaceNestedOrTernaryRecursive() { rewriteRun( @@ -602,6 +656,62 @@ public String determineSomething(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryPrimitiveCondition() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return a == 1 ? "one" : a == 2 ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a) { + return switch (a) { + case 1 -> "one"; + case 2 -> "two"; + default -> "other"; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryPrimitiveConditionInverted() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return 1 == a ? "one" : 2 == a ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a) { + return switch (a) { + case 1 -> "one"; + case 2 -> "two"; + default -> "other"; + }; + } + } + """, + JAVA_17 + ) + ); + } + @Test void doReplaceNestedOrTernaryWithSwitchExpressionInvertedEquals() { rewriteRun( From a0dea8ebbfaa78022d062d8fe65c833becac5cd2 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 11:39:37 +0200 Subject: [PATCH 32/45] add more tests --- .../TernaryOperatorsShouldNotBeNested.java | 1 - ...TernaryOperatorsShouldNotBeNestedTest.java | 45 +++++++++++++++---- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d3272df214..784c44cf5a 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -295,7 +295,6 @@ private Optional findConditionIdentifier(final J.Ternary ternary) J.Identifier result = null; if (ternary.getCondition() instanceof J.MethodInvocation) { J.MethodInvocation inv = (J.MethodInvocation) ternary.getCondition(); - //todo get a if inv is ~like~ "a".equals(a) or a.equals("a") or Object.equals(a,"a") or Object.equals("a",a) if (!inv.getSimpleName().equals("equals")) { return Optional.empty(); } diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 2e08f7bac5..f7dcd984bb 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -275,8 +275,8 @@ public String determineSomething(String letter) { ); } - @ExpectedToFail( - "Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, not sure how to extract that") + @ExpectedToFail("Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, " + + "not sure how to extract that") @Issue("todo") @Test void doReplaceMultiLevelTernariesWithComments() { @@ -805,21 +805,50 @@ void doReplaceNestedOrTernaryRecursive() { java( """ class Test { - public String determineSomething(String aString, String bString, String cString) { - return "a".equals(aString) ? "a" : "b".equals(bString) ? "b" : "c".equals(cString) ? "c" :"nope"; + public String determineSomething(String a) { + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "c".equals(a) ? "c" : "nope"; } } """, """ class Test { - public String determineSomething(String aString, String bString, String cString) { - if ("a".equals(aString)) { + public String determineSomething(String a) { + return switch (a) { + case "a" -> "a"; + case "b" -> "b"; + case "c" -> "c"; + default -> "nope"; + }; + } + } + """, + JAVA_17 + ) + ); + } + + @Test + void doReplaceNestedOrTernaryRecursiveWithIfsWhenMultipleVariablesAreUsed() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(c) ? "c" :"nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { return "a"; } - if ("b".equals(bString)) { + if ("b".equals(b)) { return "b"; } - return "c".equals(cString) ? "c" : "nope"; + return "c".equals(c) ? "c" : "nope"; } } """, From 81c40a095cd0d80351bfcd0946b29b7c8955ed02 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 11:52:09 +0200 Subject: [PATCH 33/45] remove todo --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 784c44cf5a..d8bd670f4e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -188,8 +188,8 @@ private List findNestedTernaries(final J.Ternary ternary, final J.Ide J.Ternary next = ternary; while (next.getFalsePart() instanceof J.Ternary) { if (next.getTruePart() instanceof J.Ternary) { - //todo this could become complicated if this is also nested. Lets skip it for now? - // maybe we can just leave a (nested) ternary here though. + //as long as we do not use pattern matching, an "and" nested ternary will never work for a switch: + // Example: a equals a and a equals b will never be true return Collections.emptyList(); } J.Ternary nested = (J.Ternary) next.getFalsePart(); From 885a61aecab9b038af0d6a9a5e7f2849f8460c66 Mon Sep 17 00:00:00 2001 From: Peter Streef Date: Tue, 30 May 2023 14:53:09 +0200 Subject: [PATCH 34/45] end visiting of subtree at CompilationUnit Co-authored-by: Knut Wannheden --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d8bd670f4e..965d222c18 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -86,7 +86,7 @@ public J.CompilationUnit visitCompilationUnit( doAfterVisit(new UseSwitchExpressionVisitor()); } doAfterVisit(new UseIfVisitor()); - return super.visitCompilationUnit(cu, executionContext); + return cu; } }; } From c0963862fd73abc8e9eb8e532dbd98182f3be54a Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 14:57:28 +0200 Subject: [PATCH 35/45] use javaVersion in tests --- .../TernaryOperatorsShouldNotBeNested.java | 13 +- ...TernaryOperatorsShouldNotBeNestedTest.java | 161 ++++++------------ 2 files changed, 57 insertions(+), 117 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 965d222c18..e4dee30a7d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -35,7 +35,6 @@ import org.openrewrite.internal.lang.Nullable; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; -import org.openrewrite.java.RemoveImport; import org.openrewrite.java.marker.JavaVersion; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; @@ -238,7 +237,7 @@ private J.Case toCase(final J.Identifier switchVar, final J.Ternary ternary) { if (ternary.getCondition() instanceof J.MethodInvocation) { J.MethodInvocation inv = ((J.MethodInvocation) ternary.getCondition()); if (isObjectsEquals(inv)) { - doAfterVisit(new RemoveImport<>("java.util.Objects")); + maybeRemoveImport("java.util.Objects"); compare = isVariable(inv.getArguments().get(0)) ? inv.getArguments().get(1) : inv.getArguments().get(0); @@ -247,17 +246,15 @@ private J.Case toCase(final J.Identifier switchVar, final J.Ternary ternary) { ? inv.getArguments().get(0) : inv.getSelect(); } - } - else if (ternary.getCondition() instanceof J.Binary) { + } else if (ternary.getCondition() instanceof J.Binary) { J.Binary bin = ((J.Binary) ternary.getCondition()); compare = isEqualVariable(switchVar, bin.getLeft()) ? bin.getRight() : bin.getLeft(); + } else { + throw new IllegalArgumentException( + "Only J.Binary or J.MethodInvocation are expected as ternary conditions when creating a switch case"); } - else { - throw new IllegalArgumentException("Only J.Binary or J.MethodInvocation are expected as ternary conditions when creating a switch case"); - } - return new J.Case( Tree.randomId(), ternary.getPrefix().withWhitespace(" "), diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index f7dcd984bb..f2bfdd29b8 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -16,48 +16,24 @@ package org.openrewrite.staticanalysis; import static org.openrewrite.java.Assertions.java; - -import java.util.function.Consumer; +import static org.openrewrite.java.Assertions.javaVersion; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.ExpectedToFail; import org.openrewrite.Issue; -import org.openrewrite.Tree; -import org.openrewrite.java.marker.JavaVersion; -import org.openrewrite.java.tree.J; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; -import org.openrewrite.test.SourceSpec; - -class TernaryOperatorsShouldNotBeNestedTest implements RewriteTest { - - private static final Consumer> JAVA_17 = (spec) -> spec.markers(new JavaVersion( - Tree.randomId(), - "createdBy", - "vmVendor", - "17", - "17" - )); - private static final Consumer> JAVA_11 = (spec) -> spec.markers(new JavaVersion( - Tree.randomId(), - "createdBy", - "vmVendor", - "11", - "11" - )); - - TernaryOperatorsShouldNotBeNested recipe = new TernaryOperatorsShouldNotBeNested(); - - @Override - public void defaults(RecipeSpec spec) { - recipe = new TernaryOperatorsShouldNotBeNested(); - spec.recipe(recipe); - } +class TernaryOperatorsShouldNotBeNestedTest { @Nested - class SwitchExpressionNotSupported { + class SwitchExpressionNotSupported implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new TernaryOperatorsShouldNotBeNested()).allSources(s -> s.markers(javaVersion(11))); + } @Test void doReplaceNestedOrTernaryWithIfFollowedByTernary() { @@ -80,8 +56,7 @@ public String determineSomething(String a, String b) { return "b".equals(b) ? "b" : "nope"; } } - """, - JAVA_11 + """ ) ); } @@ -107,8 +82,7 @@ public String determineSomething(int a, int b) { return b == 2 ? "two" : "other"; } } - """, - JAVA_11 + """ ) ); } @@ -134,8 +108,7 @@ public String determineSomething(int a, int b) { return 2 == b ? "two" : "other"; } } - """, - JAVA_11 + """ ) ); } @@ -164,8 +137,7 @@ public String determineSomething(String a, String b, String c) { return "c".equals(b) ? "c" : "nope"; } } - """, - JAVA_11 + """ ) ); } @@ -191,8 +163,7 @@ public String determineSomething(String a, String b) { return "nope"; } } - """, - JAVA_11 + """ ) ); } @@ -218,8 +189,7 @@ public String determineSomething(String a, String b, String c) { return "c".equals(c) ? "c" : "nope"; } } - """, - JAVA_11 + """ ) ); } @@ -269,8 +239,7 @@ public String determineSomething(String letter) { return "g".equals(letter) ? "g" : "nope"; } } - """, - JAVA_11 + """ ) ); } @@ -324,8 +293,7 @@ public String determineSomething(String letter) { return "g".equals(letter) ? "g" : "nope"; //and nope if nope } } - """, - JAVA_11 + """ ) ); } @@ -358,8 +326,7 @@ public void doThing(String a, String b) { System.out.println(result); } } - """, - JAVA_11 + """ ) ); } @@ -380,8 +347,7 @@ public Set makeASet() { return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); } } - """, - JAVA_11 + """ ) ); } @@ -419,8 +385,7 @@ public Set makeASet() { }).collect(Collectors.toSet()); } } - """, - JAVA_11 + """ ) ); } @@ -464,8 +429,7 @@ public Set makeASet() { ).collect(Collectors.toSet()); } } - """, - JAVA_11 + """ ) ); } @@ -492,8 +456,7 @@ public String determineSomething(String a, String b) { return "b".equals(b) ? "b" : null; } } - """, - JAVA_11 + """ ) ); } @@ -519,8 +482,7 @@ public String determineSomething(String a, String b) { return "b".equals(b) ? a + b : b + a; } } - """, - JAVA_11 + """ ) ); } @@ -550,8 +512,7 @@ public String determineSomething(String a, String b) { return "b".equals(b) ? "b" : "nope"; //this should be behind the ternary } } - """, - JAVA_11 + """ ) ); } @@ -585,8 +546,7 @@ public String determineSomething(String a, String b) { return "b".equals(b) ? M.b() : M.nope(); } } - """, - JAVA_11 + """ ) ); } @@ -618,15 +578,19 @@ public String determineSomething(String a, String b) { : null; } } - """, - JAVA_11 + """ ) ); } } @Nested - class SwitchExpressionSupported { + class SwitchExpressionSupported implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new TernaryOperatorsShouldNotBeNested()).allSources(s -> s.markers(javaVersion(14))); + } @Test void doReplaceNestedOrTernaryWithSwitchExpression() { @@ -650,8 +614,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -678,8 +641,7 @@ public String determineSomething(int a) { }; } } - """, - JAVA_17 + """ ) ); } @@ -706,8 +668,7 @@ public String determineSomething(int a) { }; } } - """, - JAVA_17 + """ ) ); } @@ -734,8 +695,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -763,8 +723,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -792,8 +751,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -821,8 +779,7 @@ public String determineSomething(String a) { }; } } - """, - JAVA_17 + """ ) ); } @@ -851,8 +808,7 @@ public String determineSomething(String a, String b, String c) { return "c".equals(c) ? "c" : "nope"; } } - """, - JAVA_17 + """ ) ); } @@ -878,8 +834,7 @@ public String determineSomething(String a, String b) { return "nope"; } } - """, - JAVA_17 + """ ) ); } @@ -905,8 +860,7 @@ public String determineSomething(String a, String b, String c) { return "c".equals(c) ? "c" : "nope"; } } - """, - JAVA_17 + """ ) ); } @@ -946,8 +900,7 @@ public String determineSomething(String letter) { }; //and nope if nope } } - """, - JAVA_17 + """ ) ); } @@ -976,8 +929,7 @@ public void doThing(String a, String b) { System.out.println(result); } } - """, - JAVA_17 + """ ) ); } @@ -998,8 +950,7 @@ public Set makeASet() { return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); } } - """, - JAVA_17 + """ ) ); } @@ -1036,8 +987,7 @@ public Set makeASet() { }).collect(Collectors.toSet()); } } - """, - JAVA_17 + """ ) ); } @@ -1078,8 +1028,7 @@ public Set makeASet() { ).collect(Collectors.toSet()); } } - """, - JAVA_17 + """ ) ); } @@ -1120,8 +1069,7 @@ public Set makeASet() { ).collect(Collectors.toSet()); } } - """, - JAVA_17 + """ ) ); } @@ -1148,8 +1096,7 @@ public String determineSomething(String a) { }; } } - """, - JAVA_17 + """ ) ); } @@ -1178,8 +1125,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -1206,8 +1152,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } @@ -1238,8 +1183,7 @@ public String determineSomething(String a, String b) { }; //this should be behind the ternary } } - """, - JAVA_17 + """ ) ); } @@ -1274,8 +1218,7 @@ public String determineSomething(String a, String b) { }; } } - """, - JAVA_17 + """ ) ); } From 4ff8cbd8d0946baeb3f52d835c2df4624cba488e Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 15:33:39 +0200 Subject: [PATCH 36/45] fixes --- .../TernaryOperatorsShouldNotBeNested.java | 13 ++++-- ...TernaryOperatorsShouldNotBeNestedTest.java | 42 +++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index e4dee30a7d..b316862b87 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -16,6 +16,7 @@ package org.openrewrite.staticanalysis; import static org.openrewrite.Tree.randomId; +import static org.openrewrite.java.tree.J.Binary.Type.Equal; import java.time.Duration; import java.util.ArrayList; @@ -132,7 +133,8 @@ private J.If ifOf(final J.Ternary ternary) { new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(ternary.getCondition()) ).withComments(ternary.getCondition().getComments()), - JRightPadded.build(blockOf(rewriteNestedTernary(returnOf(ternary.getTruePart())))), + JRightPadded.build(blockOf(rewriteNestedTernary(returnOf(ternary.getTruePart() + .withComments(ternary.getTruePart().getComments()))))), null ); } @@ -246,7 +248,7 @@ private J.Case toCase(final J.Identifier switchVar, final J.Ternary ternary) { ? inv.getArguments().get(0) : inv.getSelect(); } - } else if (ternary.getCondition() instanceof J.Binary) { + } else if (isEqualsBinary(ternary.getCondition())) { J.Binary bin = ((J.Binary) ternary.getCondition()); compare = isEqualVariable(switchVar, bin.getLeft()) ? bin.getRight() @@ -307,8 +309,7 @@ private Optional findConditionIdentifier(final J.Ternary ternary) J second = inv.getArguments().get(1); result = xorVariable(first, second); } - } - if (ternary.getCondition() instanceof J.Binary) { + } else if (isEqualsBinary(ternary.getCondition())) { J.Binary bin = (J.Binary) ternary.getCondition(); result = xorVariable(bin.getLeft(), bin.getRight()); } @@ -350,6 +351,10 @@ private static boolean isObjectsEquals(J.MethodInvocation inv) { } return false; } + + private static boolean isEqualsBinary(J maybeEqualsBinary) { + return maybeEqualsBinary instanceof J.Binary && ((J.Binary) maybeEqualsBinary).getOperator().equals(Equal); + } } private static J.Return returnOf(Expression expression) { diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index f2bfdd29b8..92e35192cd 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -244,9 +244,7 @@ public String determineSomething(String letter) { ); } - @ExpectedToFail("Comment `dont forget about c` is dropped as it is part of a `before` in leftPad, " + - "not sure how to extract that") - @Issue("todo") + @ExpectedToFail("Comment `dont forget about c` falls off. It is part of a `before` that is dropped when falsePart is extracted") @Test void doReplaceMultiLevelTernariesWithComments() { rewriteRun( @@ -271,13 +269,16 @@ class Test { public String determineSomething(String letter) { if ("a".equals(letter)) { return "a"; - }//look its a + } + //look its a if ("b".equals(letter)) { return "b"; - }//b is also here + } + //b is also here if ("c".equals(letter)) { return "c"; /* dont forget about c */ - }// d is important too + } + // d is important too if (letter.contains("d")) { if (letter.startsWith("d")) { return letter.equals("d") ? "equals" : "startsWith"; @@ -286,7 +287,8 @@ public String determineSomething(String letter) { } if ("e".equals(letter)) { return "e"; - }//e + } + //e if ("f".equals(letter)) { return "f"; }//f @@ -646,6 +648,32 @@ public String determineSomething(int a) { ); } + @Test + void doReplaceNestedOrTernaryPrimitiveNotEqualsCondition() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return a >= 3 ? "a lot" : a == 2 ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a) { + if (a >= 3) { + return "a lot"; + } + return a == 2 ? "two" : "other"; + } + } + """ + ) + ); + } + @Test void doReplaceNestedOrTernaryPrimitiveConditionInverted() { rewriteRun( From 5d0b21b2b0af7f28942ab8ee89a74934d65bc43b Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 15:37:03 +0200 Subject: [PATCH 37/45] add constant handling for binaries --- .../TernaryOperatorsShouldNotBeNested.java | 6 +++- ...TernaryOperatorsShouldNotBeNestedTest.java | 31 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index b316862b87..fd72e2a255 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -38,6 +38,7 @@ import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.marker.JavaVersion; import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JContainer; import org.openrewrite.java.tree.JRightPadded; @@ -340,7 +341,10 @@ private static boolean isVariable(@Nullable J maybeVariable) { return false; } J.Identifier identifier = (J.Identifier) maybeVariable; - return identifier.getFieldType() != null; + if (identifier.getFieldType() == null) { + return false; + } + return !identifier.getFieldType().hasFlags(Flag.Final) || !identifier.getFieldType().hasFlags(Flag.Static); } private static boolean isObjectsEquals(J.MethodInvocation inv) { diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 92e35192cd..56945b403e 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -648,6 +648,37 @@ public String determineSomething(int a) { ); } + @Test + void doReplaceNestedOrTernaryPrimitiveConditionWithConstants() { + rewriteRun( + //language=java + java( + """ + class Test { + private static final int ONE = 1; + private static final int TWO = 2; + public String determineSomething(int a) { + return a == ONE ? "one" : a == TWO ? "two" : "other"; + } + } + """, + """ + class Test { + private static final int ONE = 1; + private static final int TWO = 2; + public String determineSomething(int a) { + return switch (a) { + case ONE -> "one"; + case TWO -> "two"; + default -> "other"; + }; + } + } + """ + ) + ); + } + @Test void doReplaceNestedOrTernaryPrimitiveNotEqualsCondition() { rewriteRun( From 896784f620c4bba08a837a3e705094cacf195b90 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 15:56:37 +0200 Subject: [PATCH 38/45] other side of equals must be constant --- .../TernaryOperatorsShouldNotBeNested.java | 36 +++++++++++++++---- ...TernaryOperatorsShouldNotBeNestedTest.java | 27 ++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index fd72e2a255..2721042494 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -298,17 +298,24 @@ private Optional findConditionIdentifier(final J.Ternary ternary) if (!inv.getSimpleName().equals("equals")) { return Optional.empty(); } - if (isVariable(inv.getSelect())) { - result = (J.Identifier) inv.getSelect(); - } - if (inv.getArguments().size() == 1 && inv.getArguments().get(0) instanceof J.Identifier) { - result = (J.Identifier) inv.getArguments().get(0); - } if (isObjectsEquals(inv)) { //one has to be constant, other not J first = inv.getArguments().get(0); J second = inv.getArguments().get(1); result = xorVariable(first, second); + } else if (inv.getArguments().size() == 1) { + J other = null; + if (isVariable(inv.getSelect())) { + result = (J.Identifier) inv.getSelect(); + other = inv.getArguments().get(0); + } + if (inv.getArguments().get(0) instanceof J.Identifier) { + result = (J.Identifier) inv.getArguments().get(0); + other = inv.getSelect(); + } + if(!isConstant(other)){ + return Optional.empty(); + } } } else if (isEqualsBinary(ternary.getCondition())) { J.Binary bin = (J.Binary) ternary.getCondition(); @@ -347,6 +354,23 @@ private static boolean isVariable(@Nullable J maybeVariable) { return !identifier.getFieldType().hasFlags(Flag.Final) || !identifier.getFieldType().hasFlags(Flag.Static); } + private static boolean isConstant(@Nullable J maybeConstant) { + if (maybeConstant == null) { + return false; + } + if (maybeConstant instanceof J.Literal) { + return true; + } + if (!(maybeConstant instanceof J.Identifier)) { + return false; + } + J.Identifier identifier = (J.Identifier) maybeConstant; + if (identifier.getFieldType() == null) { + return false; + } + return !identifier.getFieldType().hasFlags(Flag.Final) || !identifier.getFieldType().hasFlags(Flag.Static); + } + private static boolean isObjectsEquals(J.MethodInvocation inv) { if (inv.getSelect() instanceof J.Identifier) { J.Identifier maybeObjects = (J.Identifier) inv.getSelect(); diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 56945b403e..8ab3668728 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -679,6 +679,7 @@ public String determineSomething(int a) { ); } + @Test void doReplaceNestedOrTernaryPrimitiveNotEqualsCondition() { rewriteRun( @@ -1282,6 +1283,32 @@ public String determineSomething(String a, String b) { ); } + @Test + void doReplaceNestedOrTernaryWithNonConstantEqualsWithIf() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return a.equals("a".toString()) ? "a" : "b".equals(a) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if (a.equals("a".toString())) { + return "a"; + } + return "b".equals(a) ? "b" : "nope"; + } + } + """ + ) + ); + } + } From 905207b1abaa49d53f23547325c6c412991ee7b5 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 30 May 2023 15:59:14 +0200 Subject: [PATCH 39/45] use helper --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 2721042494..c9ed7ac69b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -374,7 +374,7 @@ private static boolean isConstant(@Nullable J maybeConstant) { private static boolean isObjectsEquals(J.MethodInvocation inv) { if (inv.getSelect() instanceof J.Identifier) { J.Identifier maybeObjects = (J.Identifier) inv.getSelect(); - boolean isObjects = TypeUtils.isOfType(maybeObjects.getType(), JavaType.buildType("java.util.Objects")); + boolean isObjects = TypeUtils.isOfClassType(maybeObjects.getType(), "java.util.Objects"); return isObjects && "equals".equals(inv.getSimpleName()); } return false; From 1aeba335c1c41b1ee450d768e25230129daccfbb Mon Sep 17 00:00:00 2001 From: pstreef Date: Wed, 31 May 2023 13:29:19 +0200 Subject: [PATCH 40/45] fix build with latest rewrite:main --- .../staticanalysis/TernaryOperatorsShouldNotBeNested.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index c9ed7ac69b..2e4d7ab094 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -42,7 +42,6 @@ import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JContainer; import org.openrewrite.java.tree.JRightPadded; -import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.Space; import org.openrewrite.java.tree.Statement; import org.openrewrite.java.tree.TypeUtils; @@ -100,7 +99,7 @@ public J visitLambda(final J.Lambda lambda, final ExecutionContext executionCont if (result == lambda) { return super.visitLambda(lambda, executionContext); } - doAfterVisit(new RemoveUnneededBlock()); + doAfterVisit(new RemoveUnneededBlock().getVisitor()); return autoFormat(lambda.withBody(result.withPrefix(lambda.getBody().getPrefix())), executionContext); } @@ -110,7 +109,7 @@ public J visitReturn(final J.Return retrn, final ExecutionContext executionConte if (result == retrn) { return super.visitReturn(retrn, executionContext); } - doAfterVisit(new RemoveUnneededBlock()); + doAfterVisit(new RemoveUnneededBlock().getVisitor()); return autoFormat(result, executionContext); } @@ -313,7 +312,7 @@ private Optional findConditionIdentifier(final J.Ternary ternary) result = (J.Identifier) inv.getArguments().get(0); other = inv.getSelect(); } - if(!isConstant(other)){ + if (!isConstant(other)) { return Optional.empty(); } } From f7beb4c7f56874be882f1cedd7bdc98e11f90371 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 22 Aug 2023 11:44:12 +0200 Subject: [PATCH 41/45] remove default duration move comment --- .../TernaryOperatorsShouldNotBeNested.java | 11 ++--------- .../TernaryOperatorsShouldNotBeNestedTest.java | 5 ++--- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index 2e4d7ab094..d90ab52c42 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -18,7 +18,6 @@ import static org.openrewrite.Tree.randomId; import static org.openrewrite.java.tree.J.Binary.Type.Equal; -import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -60,12 +59,6 @@ public String getDescription() { return "Nested ternary operators can be hard to read quickly. Prefer simpler constructs for improved readability. " + "If supported, this recipe will try to replace nested ternaries with switch expressions."; } - - @Override - public Duration getEstimatedEffortPerOccurrence() { - return Duration.ofMinutes(5); - } - @Override public Set getTags() { return Collections.singleton("RSPEC-3358"); @@ -100,7 +93,7 @@ public J visitLambda(final J.Lambda lambda, final ExecutionContext executionCont return super.visitLambda(lambda, executionContext); } doAfterVisit(new RemoveUnneededBlock().getVisitor()); - return autoFormat(lambda.withBody(result.withPrefix(lambda.getBody().getPrefix())), executionContext); + return autoFormat(lambda.withBody(result.withPrefix(Space.SINGLE_SPACE)), executionContext); } @Override @@ -128,7 +121,7 @@ private Statement rewriteNestedTernary(final Statement parent) { private J.If ifOf(final J.Ternary ternary) { return new J.If( Tree.randomId(), - Space.EMPTY, + ternary.getPrefix(), Markers.EMPTY, new J.ControlParentheses<>(Tree.randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(ternary.getCondition()) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 8ab3668728..146bde6ea9 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -420,9 +420,8 @@ public Set makeASet() { class Test { public Set makeASet() { List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> - //look a ternary - { + return s.stream().map( /* look a lambda */ item -> { + //look a ternary if (item.startsWith("a")) { return "a"; } From 8d57951e509ec956b8157bd70eb8dc1e8dcde6d2 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 22 Aug 2023 12:02:59 +0200 Subject: [PATCH 42/45] Ignore Objects.equals when finding switch expression candidates as it breaks null safety --- .../TernaryOperatorsShouldNotBeNested.java | 7 +--- ...TernaryOperatorsShouldNotBeNestedTest.java | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java index d90ab52c42..8ffefb526d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java +++ b/src/main/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNested.java @@ -290,12 +290,7 @@ private Optional findConditionIdentifier(final J.Ternary ternary) if (!inv.getSimpleName().equals("equals")) { return Optional.empty(); } - if (isObjectsEquals(inv)) { - //one has to be constant, other not - J first = inv.getArguments().get(0); - J second = inv.getArguments().get(1); - result = xorVariable(first, second); - } else if (inv.getArguments().size() == 1) { + if (inv.getArguments().size() == 1) { J other = null; if (isVariable(inv.getSelect())) { result = (J.Identifier) inv.getSelect(); diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 146bde6ea9..20e9d67b85 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -759,6 +759,8 @@ public String determineSomething(String a, String b) { ); } + @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") + @Issue("todo") @Test void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEquals() { rewriteRun( @@ -787,6 +789,8 @@ public String determineSomething(String a, String b) { ); } + @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") + @Issue("todo") @Test void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEqualsInverted() { rewriteRun( @@ -993,6 +997,34 @@ public void doThing(String a, String b) { ); } + @Test + void doNotReplaceNestedOrTernaryWithSwitchExpressionButUseIfWhenUsingNullSafeEquals() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; + } + } + """, + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + if (Objects.equals(a, "a")) { + return "a"; + } + return Objects.equals(a, "b") ? b : "nope"; + } + } + """ + ) + ); + } + @Test void doNotReplaceNonNestedOrTernaryInStreamWithSwitch() { rewriteRun( From e8a0848a162531fd7cf88b03c3fe4e9b8ebb57c4 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 22 Aug 2023 12:14:38 +0200 Subject: [PATCH 43/45] Add more test nesting --- ...TernaryOperatorsShouldNotBeNestedTest.java | 1433 +++++++++-------- 1 file changed, 724 insertions(+), 709 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 20e9d67b85..8a3b2443cc 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -588,756 +588,771 @@ public String determineSomething(String a, String b) { @Nested class SwitchExpressionSupported implements RewriteTest { + @Override public void defaults(RecipeSpec spec) { spec.recipe(new TernaryOperatorsShouldNotBeNested()).allSources(s -> s.markers(javaVersion(14))); } - @Test - void doReplaceNestedOrTernaryWithSwitchExpression() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> "a"; - case "b" -> "b"; - default -> "nope"; - }; - } - } - """ - ) - ); - } + @Nested + class ReplaceWithSwitchExpression { + @Test + void doReplaceNestedOrTernaryWithSwitchExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryPrimitiveCondition() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(int a) { - return a == 1 ? "one" : a == 2 ? "two" : "other"; - } - } - """, - """ - class Test { - public String determineSomething(int a) { - return switch (a) { - case 1 -> "one"; - case 2 -> "two"; - default -> "other"; - }; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryPrimitiveCondition() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return a == 1 ? "one" : a == 2 ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a) { + return switch (a) { + case 1 -> "one"; + case 2 -> "two"; + default -> "other"; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryPrimitiveConditionWithConstants() { - rewriteRun( - //language=java - java( - """ - class Test { - private static final int ONE = 1; - private static final int TWO = 2; - public String determineSomething(int a) { - return a == ONE ? "one" : a == TWO ? "two" : "other"; - } - } - """, - """ - class Test { - private static final int ONE = 1; - private static final int TWO = 2; - public String determineSomething(int a) { - return switch (a) { - case ONE -> "one"; - case TWO -> "two"; - default -> "other"; - }; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryPrimitiveConditionWithConstants() { + rewriteRun( + //language=java + java( + """ + class Test { + private static final int ONE = 1; + private static final int TWO = 2; + public String determineSomething(int a) { + return a == ONE ? "one" : a == TWO ? "two" : "other"; + } + } + """, + """ + class Test { + private static final int ONE = 1; + private static final int TWO = 2; + public String determineSomething(int a) { + return switch (a) { + case ONE -> "one"; + case TWO -> "two"; + default -> "other"; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryPrimitiveNotEqualsCondition() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(int a) { - return a >= 3 ? "a lot" : a == 2 ? "two" : "other"; - } - } - """, - """ - class Test { - public String determineSomething(int a) { - if (a >= 3) { - return "a lot"; + @Test + void doReplaceNestedOrTernaryPrimitiveNotEqualsCondition() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return a >= 3 ? "a lot" : a == 2 ? "two" : "other"; } - return a == 2 ? "two" : "other"; - } - } - """ - ) - ); - } - - @Test - void doReplaceNestedOrTernaryPrimitiveConditionInverted() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(int a) { - return 1 == a ? "one" : 2 == a ? "two" : "other"; - } - } - """, - """ - class Test { - public String determineSomething(int a) { - return switch (a) { - case 1 -> "one"; - case 2 -> "two"; - default -> "other"; - }; - } - } - """ - ) - ); - } + } + """, + """ + class Test { + public String determineSomething(int a) { + if (a >= 3) { + return "a lot"; + } + return a == 2 ? "two" : "other"; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryWithSwitchExpressionInvertedEquals() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return a.equals("a") ? "a" : a.equals("b") ? b : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> "a"; - case "b" -> b; - default -> "nope"; - }; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryPrimitiveConditionInverted() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(int a) { + return 1 == a ? "one" : 2 == a ? "two" : "other"; + } + } + """, + """ + class Test { + public String determineSomething(int a) { + return switch (a) { + case 1 -> "one"; + case 2 -> "two"; + default -> "other"; + }; + } + } + """ + ) + ); + } - @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") - @Issue("todo") - @Test - void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEquals() { - rewriteRun( - //language=java - java( - """ - import java.util.Objects; - class Test { - public String determineSomething(String a, String b) { - return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> "a"; - case "b" -> b; - default -> "nope"; - }; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionInvertedEquals() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return a.equals("a") ? "a" : a.equals("b") ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """ + ) + ); + } - @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") - @Issue("todo") - @Test - void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEqualsInverted() { - rewriteRun( - //language=java - java( - """ - import java.util.Objects; - class Test { - public String determineSomething(String a, String b) { - return Objects.equals("a", a) ? "a" : Objects.equals("b", a) ? b : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> "a"; - case "b" -> b; - default -> "nope"; - }; - } - } - """ - ) - ); - } + @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEquals() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryRecursive() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a) { - return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "c".equals(a) ? "c" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a) { - return switch (a) { - case "a" -> "a"; - case "b" -> "b"; - case "c" -> "c"; - default -> "nope"; - }; - } - } - """ - ) - ); - } + @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEqualsInverted() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals("a", a) ? "a" : Objects.equals("b", a) ? b : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryRecursiveWithIfsWhenMultipleVariablesAreUsed() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b, String c) { - return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(c) ? "c" :"nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b, String c) { - if ("a".equals(a)) { - return "a"; + @Test + void doReplaceNestedOrTernaryRecursive() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a) { + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "c".equals(a) ? "c" : "nope"; } - if ("b".equals(b)) { - return "b"; + } + """, + """ + class Test { + public String determineSomething(String a) { + return switch (a) { + case "a" -> "a"; + case "b" -> "b"; + case "c" -> "c"; + default -> "nope"; + }; } - return "c".equals(c) ? "c" : "nope"; - } - } - """ - ) - ); - } + } + """ + ) + ); + } - @Test - void doReplaceNestedAndTernaryWithIfThenTernary() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if ("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; + + @Test + void doReplaceMultiLevelTernariesWithSwitchExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String letter) { + return "a".equals(letter) ? "a" : //look its a + "b".equals(letter) ? "b" : //b is also here + "c".equals(letter) ? "c" : /* dont forget about c */ + // d is important too + "d".equals(letter) ? "d" : + "e".equals(letter) ? "e" : //e + "f".equals(letter) ? "f" : //f + "g".equals(letter) ? "g" : "nope"; //and nope if nope } - return "nope"; - } - } - """ - ) - ); - } + } + """, + """ + class Test { + public String determineSomething(String letter) { + return switch (letter) { + case "a" -> "a"; //look its a + case "b" -> "b"; //b is also here + case "c" -> "c"; /* dont forget about c */ + // d is important too + case "d" -> "d"; + case "e" -> "e"; //e + case "f" -> "f"; //f + case "g" -> "g"; + default -> "nope"; + }; //and nope if nope + } + } + """ + ) + ); + } - @Test - void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b, String c) { - return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b, String c) { - if ("a".equals(a)) { - return "b".equals(b) ? "b" : "a"; + @Test + void doReplaceNestedOrAssignmentTernaryWithSwitch() { + rewriteRun( + //language=java + java( + """ + class Test { + public void doThing(String a, String b) { + String result = "a".equals(a) ? "a" : "b".equals(a) ? b : "nope"; + System.out.println(result); } - return "c".equals(c) ? "c" : "nope"; - } - } - """ - ) - ); - } - - @Test - void doReplaceMultiLevelTernariesWithSwitchExpression() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String letter) { - return "a".equals(letter) ? "a" : //look its a - "b".equals(letter) ? "b" : //b is also here - "c".equals(letter) ? "c" : /* dont forget about c */ - // d is important too - "d".equals(letter) ? "d" : - "e".equals(letter) ? "e" : //e - "f".equals(letter) ? "f" : //f - "g".equals(letter) ? "g" : "nope"; //and nope if nope - } - } - """, - """ - class Test { - public String determineSomething(String letter) { - return switch (letter) { - case "a" -> "a"; //look its a - case "b" -> "b"; //b is also here - case "c" -> "c"; /* dont forget about c */ - // d is important too - case "d" -> "d"; - case "e" -> "e"; //e - case "f" -> "f"; //f - case "g" -> "g"; - default -> "nope"; - }; //and nope if nope - } - } - """ - ) - ); - } - - @Test - void doReplaceNestedOrAssignmentTernaryWithSwitch() { - rewriteRun( - //language=java - java( - """ - class Test { - public void doThing(String a, String b) { - String result = "a".equals(a) ? "a" : "b".equals(a) ? b : "nope"; - System.out.println(result); - } - } - """, - """ - class Test { - public void doThing(String a, String b) { - String result = switch (a) { - case "a" -> "a"; - case "b" -> b; - default -> "nope"; - }; - System.out.println(result); - } - } - """ - ) - ); - } + } + """, + """ + class Test { + public void doThing(String a, String b) { + String result = switch (a) { + case "a" -> "a"; + case "b" -> b; + default -> "nope"; + }; + System.out.println(result); + } + } + """ + ) + ); + } - @Test - void doNotReplaceNestedOrTernaryWithSwitchExpressionButUseIfWhenUsingNullSafeEquals() { - rewriteRun( - //language=java - java( - """ - import java.util.Objects; - class Test { - public String determineSomething(String a, String b) { - return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; - } - } - """, - """ - import java.util.Objects; - class Test { - public String determineSomething(String a, String b) { - if (Objects.equals(a, "a")) { - return "a"; + @Test + void doReplaceNestedOrTernaryInStreamWithSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.equals("a") ? "a" : item.equals("b") ? "b" : "nope").collect(Collectors.toSet()); } - return Objects.equals(a, "b") ? b : "nope"; - } - } - """ - ) - ); - } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> switch (item) { + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + }).collect(Collectors.toSet()); + } + } + """ + ) + ); + } - @Test - void doNotReplaceNonNestedOrTernaryInStreamWithSwitch() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryInStreamContainingComments() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> + //look a ternary + item.equals("a") ? "a" : item.equals("b") ? "b" : "nope" + ).collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map( /* look a lambda */ item -> switch (item) { //look a ternary + case "a" -> "a"; + case "b" -> "b"; + default -> "nope"; + } + ).collect(Collectors.toSet()); + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryInStreamWithSwitch() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> item.equals("a") ? "a" : item.equals("b") ? "b" : "nope").collect(Collectors.toSet()); - } - } - """, - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> switch (item) { - case "a" -> "a"; - case "b" -> "b"; - default -> "nope"; - }).collect(Collectors.toSet()); - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryContainingNull() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a) { + return "a".equals(a) ? null : "b".equals(a) ? "b" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a) { + return switch (a) { + case "a" -> null; + case "b" -> "b"; + default -> null; + }; + } + } + """ + ) + ); + } - @ExpectedToFail("Pattern matching not yet implemented") - @Issue("todo") - @Test - void doReplaceNestedOrTernaryInStreamWithPatternMatchingSwitch() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); - } - } - """, - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map(item -> - switch (item) { - case String st && st.startsWith("a") -> "a"; - case String st && st.startsWith("b") -> "b"; - default -> "nope"; + @Test + void doReplaceNestedOrTernaryContainingExpression() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "foo" + "bar" : "b".equals(a) ? a + b : b + a; } - ).collect(Collectors.toSet()); - } - } - """ - ) - ); - } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> "foo" + "bar"; + case "b" -> a + b; + default -> b + a; + }; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryInStreamContainingComments() { - rewriteRun( - //language=java - java( - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> - //look a ternary - item.equals("a") ? "a" : item.equals("b") ? "b" : "nope" - ).collect(Collectors.toSet()); - } - } - """, - """ - import java.util.Set; - import java.util.Arrays; - import java.util.List; - import java.util.stream.Collectors; - class Test { - public Set makeASet() { - List s = Arrays.asList("a","b","c","nope"); - return s.stream().map( /* look a lambda */ item -> switch (item) { //look a ternary + @Test + void doReplaceNestedOrTernaryContainingComments() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; //this should be behind the ternary + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + //this should be before the if and followed by a new line + + return switch (a) { case "a" -> "a"; case "b" -> "b"; default -> "nope"; + }; //this should be behind the ternary + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedOrTernaryContainingMethodCall() { + //language=java + rewriteRun( + java(""" + class M{ + static String a(){return "a";} + static String b(){return "b";} + static String c(){return "c";} + static String nope(){return "nope";} + } + """), + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? M.a() : "b".equals(a) ? M.b() : M.nope(); + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "a" -> M.a(); + case "b" -> M.b(); + default -> M.nope(); + }; + } + } + """ + ) + ); + } + + @ExpectedToFail("Pattern matching not yet implemented") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryInStreamWithPatternMatchingSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : item.startsWith("b") ? "b" : "nope").collect(Collectors.toSet()); + } + } + """, + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> + switch (item) { + case String st && st.startsWith("a") -> "a"; + case String st && st.startsWith("b") -> "b"; + default -> "nope"; } - ).collect(Collectors.toSet()); - } - } - """ - ) - ); - } + ).collect(Collectors.toSet()); + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingNull() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a) { - return "a".equals(a) ? null : "b".equals(a) ? "b" : null; - } - } - """, - """ - class Test { - public String determineSomething(String a) { - return switch (a) { - case "a" -> null; - case "b" -> "b"; - default -> null; - }; - } - } - """ - ) - ); + @ExpectedToFail("not yet implemented collapsing cases") + @Issue("todo") + @Test + void doReplaceNestedOrTernaryContainingNullCollapsingSameCases() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a) { + return "a".equals(a) ? null : "b".equals(a) ? "b" : "c".equals(a) ? "c" : null; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + return switch (a) { + case "b" -> "b"; + case "c" -> "c"; + default -> null; + }; + } + } + """ + ) + ); + } } - @ExpectedToFail("not yet implemented collapsing cases") - @Issue("todo") - @Test - void doReplaceNestedOrTernaryContainingNullCollapsingSameCases() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a) { - return "a".equals(a) ? null : "b".equals(a) ? "b" : "c".equals(a) ? "c" : null; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "b" -> "b"; - case "c" -> "c"; - default -> null; - }; - } - } - """ - ) - ); - } + @Nested + class ReplaceWithIf { - @Test - void doReplaceNestedOrTernaryContainingExpression() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? "foo" + "bar" : "b".equals(a) ? a + b : b + a; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> "foo" + "bar"; - case "b" -> a + b; - default -> b + a; - }; - } - } - """ - ) - ); - } + @Test + void doReplaceNestedOrTernaryRecursiveWithIfsWhenMultipleVariablesAreUsed() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "a" : "b".equals(b) ? "b" : "c".equals(c) ? "c" :"nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "a"; + } + if ("b".equals(b)) { + return "b"; + } + return "c".equals(c) ? "c" : "nope"; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingComments() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - //this should be before the if and followed by a new line - - return "a".equals(a) ? "a" : "b".equals(a) ? "b" : "nope"; //this should be behind the ternary - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - //this should be before the if and followed by a new line - - return switch (a) { - case "a" -> "a"; - case "b" -> "b"; - default -> "nope"; - }; //this should be behind the ternary - } - } - """ - ) - ); - } + @Test + void doReplaceNestedAndTernaryWithIfThenTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "nope"; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedAndOrTernaryWithIfThenTernaryElseTernary() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b, String c) { + return "a".equals(a) ? "b".equals(b) ? "b" : "a" : "c".equals(c) ? "c" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b, String c) { + if ("a".equals(a)) { + return "b".equals(b) ? "b" : "a"; + } + return "c".equals(c) ? "c" : "nope"; + } + } + """ + ) + ); + } + + @Test + void doNotReplaceNestedOrTernaryWithIfWhenUsingNullSafeEquals() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + return Objects.equals(a, "a") ? "a" : Objects.equals(a, "b") ? b : "nope"; + } + } + """, + """ + import java.util.Objects; + class Test { + public String determineSomething(String a, String b) { + if (Objects.equals(a, "a")) { + return "a"; + } + return Objects.equals(a, "b") ? b : "nope"; + } + } + """ + ) + ); + } + + @Test + void doReplaceNestedOrTernaryWithNonConstantEqualsWithIf() { + rewriteRun( + //language=java + java( + """ + class Test { + public String determineSomething(String a, String b) { + return a.equals("a".toString()) ? "a" : "b".equals(a) ? "b" : "nope"; + } + } + """, + """ + class Test { + public String determineSomething(String a, String b) { + if (a.equals("a".toString())) { + return "a"; + } + return "b".equals(a) ? "b" : "nope"; + } + } + """ + ) + ); + } - @Test - void doReplaceNestedOrTernaryContainingMethodCall() { - //language=java - rewriteRun( - java(""" - class M{ - static String a(){return "a";} - static String b(){return "b";} - static String c(){return "c";} - static String nope(){return "nope";} - } - """), - java( - """ - class Test { - public String determineSomething(String a, String b) { - return "a".equals(a) ? M.a() : "b".equals(a) ? M.b() : M.nope(); - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - return switch (a) { - case "a" -> M.a(); - case "b" -> M.b(); - default -> M.nope(); - }; - } - } - """ - ) - ); } - @Test - void doReplaceNestedOrTernaryWithNonConstantEqualsWithIf() { - rewriteRun( - //language=java - java( - """ - class Test { - public String determineSomething(String a, String b) { - return a.equals("a".toString()) ? "a" : "b".equals(a) ? "b" : "nope"; - } - } - """, - """ - class Test { - public String determineSomething(String a, String b) { - if (a.equals("a".toString())) { - return "a"; + @Nested + class DoNotReplace { + + @Test + void doNotReplaceNonNestedOrTernaryInStreamWithSwitch() { + rewriteRun( + //language=java + java( + """ + import java.util.Set; + import java.util.Arrays; + import java.util.List; + import java.util.stream.Collectors; + class Test { + public Set makeASet() { + List s = Arrays.asList("a","b","c","nope"); + return s.stream().map(item -> item.startsWith("a") ? "a" : "nope").collect(Collectors.toSet()); } - return "b".equals(a) ? "b" : "nope"; - } - } - """ - ) - ); + } + """ + ) + ); + } + } } From 2a0cf6e7fc7501879dc94bc20adff9f3ee4d7eb2 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Aug 2023 16:57:08 +0200 Subject: [PATCH 44/45] Add two DocumentExamples --- .../staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 8a3b2443cc..9ed6ca0a56 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -21,6 +21,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junitpioneer.jupiter.ExpectedToFail; +import org.openrewrite.DocumentExample; import org.openrewrite.Issue; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; @@ -36,6 +37,7 @@ public void defaults(RecipeSpec spec) { } @Test + @DocumentExample void doReplaceNestedOrTernaryWithIfFollowedByTernary() { rewriteRun( //language=java @@ -597,6 +599,7 @@ public void defaults(RecipeSpec spec) { @Nested class ReplaceWithSwitchExpression { @Test + @DocumentExample void doReplaceNestedOrTernaryWithSwitchExpression() { rewriteRun( //language=java From a2d96ab8ae74042ff6e938c6caa4437c34e54a21 Mon Sep 17 00:00:00 2001 From: pstreef Date: Tue, 22 Aug 2023 20:43:51 +0200 Subject: [PATCH 45/45] add links to issues --- .../TernaryOperatorsShouldNotBeNestedTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java index 9ed6ca0a56..4192c97325 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TernaryOperatorsShouldNotBeNestedTest.java @@ -766,7 +766,7 @@ public String determineSomething(String a, String b) { } @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") - @Issue("todo") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/157") @Test void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEquals() { rewriteRun( @@ -796,7 +796,7 @@ public String determineSomething(String a, String b) { } @ExpectedToFail("switch(null) is not supported before Java 18. This would break null safety.") - @Issue("todo") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/157") @Test void doReplaceNestedOrTernaryWithSwitchExpressionNullSafeEqualsInverted() { rewriteRun( @@ -1122,7 +1122,7 @@ public String determineSomething(String a, String b) { } @ExpectedToFail("Pattern matching not yet implemented") - @Issue("todo") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/158") @Test void doReplaceNestedOrTernaryInStreamWithPatternMatchingSwitch() { rewriteRun( @@ -1163,7 +1163,7 @@ public Set makeASet() { } @ExpectedToFail("not yet implemented collapsing cases") - @Issue("todo") + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/159") @Test void doReplaceNestedOrTernaryContainingNullCollapsingSameCases() { rewriteRun(