Skip to content

Commit 0c6efc9

Browse files
Jenson3210github-actions[bot]pdelagravetimtebeek
authored
NullCheckAsSwitchCase should only add null case if the switch is exhaustive (#769)
* Only add null case when all values covered * Use Switch expression/statement syntax accordingly * Update src/main/java/org/openrewrite/java/migrate/lang/NullCheckAsSwitchCase.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add Precondition and test to run them * Add Precondition and test to run them * Comment * Hold off on inclusion until we've validated at scale * Remove unnecessary else * Return early if we discover a default case * Name the two branches of case rules vs case statements --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Pierre Delagrave <pierre@moderne.io> Co-authored-by: Tim te Beek <tim@moderne.io>
1 parent 1940f67 commit 0c6efc9

File tree

7 files changed

+287
-17
lines changed

7 files changed

+287
-17
lines changed

‎src/main/java/org/openrewrite/java/migrate/lang/NullCheckAsSwitchCase.java‎

Lines changed: 87 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,15 @@
2323
import org.openrewrite.java.JavaTemplate;
2424
import org.openrewrite.java.JavaVisitor;
2525
import org.openrewrite.java.search.SemanticallyEqual;
26-
import org.openrewrite.java.tree.Expression;
27-
import org.openrewrite.java.tree.J;
28-
import org.openrewrite.java.tree.Space;
29-
import org.openrewrite.java.tree.Statement;
26+
import org.openrewrite.java.search.UsesJavaVersion;
27+
import org.openrewrite.java.tree.*;
28+
import org.openrewrite.staticanalysis.groovy.GroovyFileChecker;
3029
import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker;
3130

3231
import java.time.Duration;
32+
import java.util.ArrayList;
33+
import java.util.List;
34+
import java.util.Objects;
3335
import java.util.Optional;
3436
import java.util.concurrent.atomic.AtomicReference;
3537

@@ -58,7 +60,13 @@ public Duration getEstimatedEffortPerOccurrence() {
5860

5961
@Override
6062
public TreeVisitor<?, ExecutionContext> getVisitor() {
61-
return Preconditions.check(Preconditions.not(new KotlinFileChecker<>()), new JavaVisitor<ExecutionContext>() {
63+
TreeVisitor<?, ExecutionContext> preconditions = Preconditions.and(
64+
new UsesJavaVersion<>(21),
65+
Preconditions.not(new KotlinFileChecker<>()),
66+
Preconditions.not(new GroovyFileChecker<>())
67+
);
68+
69+
return Preconditions.check(preconditions, new JavaVisitor<ExecutionContext>() {
6270
@Override
6371
public J visitBlock(J.Block block, ExecutionContext ctx) {
6472
AtomicReference<@Nullable NullCheck> nullCheck = new AtomicReference<>();
@@ -68,13 +76,18 @@ public J visitBlock(J.Block block, ExecutionContext ctx) {
6876
if (nullCheckOpt.isPresent()) {
6977
NullCheck check = nullCheckOpt.get();
7078
J nextStatement = index + 1 < block.getStatements().size() ? block.getStatements().get(index + 1) : null;
71-
if (!(nextStatement instanceof J.Switch) ||
72-
hasNullCase((J.Switch) nextStatement) ||
73-
!SemanticallyEqual.areEqual(((J.Switch) nextStatement).getSelector().getTree(), check.getNullCheckedParameter()) ||
74-
check.returns() ||
75-
check.couldModifyNullCheckedValue()) {
79+
if (!(nextStatement instanceof J.Switch) || check.returns() || check.couldModifyNullCheckedValue()) {
80+
return statement;
81+
}
82+
J.Switch nextSwitch = (J.Switch) nextStatement;
83+
// Only if the switch does not have a null case and switches on the same value as the null check, we can remove the null check
84+
// It must have all possible input values covered
85+
if (hasNullCase(nextSwitch) ||
86+
!SemanticallyEqual.areEqual(nextSwitch.getSelector().getTree(), check.getNullCheckedParameter()) ||
87+
!coversAllPossibleValues(nextSwitch)) {
7688
return statement;
7789
}
90+
7891
nullCheck.set(check);
7992
return null;
8093
}
@@ -106,6 +119,16 @@ private boolean hasNullCase(J.Switch switch_) {
106119
}
107120

108121
private J.Case createNullCase(J.Switch aSwitch, Statement whenNull) {
122+
J.Case currentFirstCase = aSwitch.getCases().getStatements().isEmpty() ||
123+
!(aSwitch.getCases().getStatements().get(0) instanceof J.Case) ?
124+
null : (J.Case) aSwitch.getCases().getStatements().get(0);
125+
if (currentFirstCase == null || J.Case.Type.Rule == currentFirstCase.getType()) {
126+
return createCaseRule(aSwitch, whenNull);
127+
}
128+
return createCaseStatement(aSwitch, whenNull, currentFirstCase);
129+
}
130+
131+
private J.Case createCaseRule(J.Switch aSwitch, Statement whenNull) {
109132
if (whenNull instanceof J.Block && ((J.Block) whenNull).getStatements().size() == 1) {
110133
Statement firstStatement = ((J.Block) whenNull).getStatements().get(0);
111134
if (firstStatement instanceof Expression || firstStatement instanceof J.Throw) {
@@ -122,6 +145,60 @@ private J.Case createNullCase(J.Switch aSwitch, Statement whenNull) {
122145
J.Case nullCase = (J.Case) switchWithNullCase.getCases().getStatements().get(0);
123146
return nullCase.withBody(requireNonNull(nullCase.getBody()).withPrefix(Space.SINGLE_SPACE));
124147
}
148+
149+
private J.Case createCaseStatement(J.Switch aSwitch, Statement whenNull, J.Case currentFirstCase) {
150+
List<J> statements = new ArrayList<>();
151+
statements.add(aSwitch.getSelector().getTree());
152+
if (whenNull instanceof J.Block) {
153+
statements.addAll(((J.Block) whenNull).getStatements());
154+
} else {
155+
statements.add(whenNull);
156+
}
157+
StringBuilder template = new StringBuilder("switch(#{any()}) {\ncase null:");
158+
for (int i = 1; i < statements.size(); i++) {
159+
template.append("\n#{any()};");
160+
}
161+
template.append("\nbreak;\n}");
162+
J.Switch switchWithNullCase = JavaTemplate.apply(
163+
template.toString(),
164+
new Cursor(getCursor(), aSwitch),
165+
aSwitch.getCoordinates().replace(),
166+
statements.toArray());
167+
J.Case nullCase = (J.Case) switchWithNullCase.getCases().getStatements().get(0);
168+
Space currentFirstCaseIndentation = currentFirstCase.getStatements().stream().map(J::getPrefix).findFirst().orElse(Space.SINGLE_SPACE);
169+
170+
return nullCase.withStatements(ListUtils.mapFirst(nullCase.getStatements(), s -> s == null ? null : s.withPrefix(currentFirstCaseIndentation)));
171+
}
172+
173+
private boolean coversAllPossibleValues(J.Switch switch_) {
174+
List<J> labels = new ArrayList<>();
175+
for (Statement statement : switch_.getCases().getStatements()) {
176+
for (J j : ((J.Case) statement).getCaseLabels()) {
177+
if (j instanceof J.Identifier && "default".equals(((J.Identifier) j).getSimpleName())) {
178+
return true;
179+
}
180+
labels.add(j);
181+
}
182+
}
183+
JavaType javaType = switch_.getSelector().getTree().getType();
184+
if (javaType instanceof JavaType.Class && ((JavaType.Class) javaType).getKind() == JavaType.FullyQualified.Kind.Enum) {
185+
// Every enum value must be present in the switch
186+
return ((JavaType.Class) javaType).getMembers().stream().allMatch(variable ->
187+
labels.stream().anyMatch(label -> {
188+
if (!(label instanceof TypeTree && TypeUtils.isOfType(((TypeTree) label).getType(), javaType))) {
189+
return false;
190+
}
191+
J.Identifier enumName = null;
192+
if (label instanceof J.Identifier) {
193+
enumName = (J.Identifier) label;
194+
} else if (label instanceof J.FieldAccess) {
195+
enumName = ((J.FieldAccess) label).getName();
196+
}
197+
return enumName != null && Objects.equals(variable.getName(), enumName.getSimpleName());
198+
}));
199+
}
200+
return false;
201+
}
125202
});
126203
}
127204
}

‎src/main/java/org/openrewrite/java/migrate/lang/RefineSwitchCases.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@
2020
import org.openrewrite.*;
2121
import org.openrewrite.internal.ListUtils;
2222
import org.openrewrite.java.JavaIsoVisitor;
23+
import org.openrewrite.java.search.UsesJavaVersion;
2324
import org.openrewrite.java.tree.Expression;
2425
import org.openrewrite.java.tree.J;
2526
import org.openrewrite.java.tree.Space;
2627
import org.openrewrite.java.tree.Statement;
28+
import org.openrewrite.staticanalysis.groovy.GroovyFileChecker;
2729
import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker;
2830

2931
import java.util.ArrayList;
@@ -50,7 +52,13 @@ public String getDescription() {
5052

5153
@Override
5254
public TreeVisitor<?, ExecutionContext> getVisitor() {
53-
return Preconditions.check(Preconditions.not(new KotlinFileChecker<>()), new JavaIsoVisitor<ExecutionContext>() {
55+
TreeVisitor<?, ExecutionContext> preconditions = Preconditions.and(
56+
new UsesJavaVersion<>(21),
57+
Preconditions.not(new KotlinFileChecker<>()),
58+
Preconditions.not(new GroovyFileChecker<>())
59+
);
60+
61+
return Preconditions.check(preconditions, new JavaIsoVisitor<ExecutionContext>() {
5462
@Override
5563
public J.Switch visitSwitch(J.Switch sw, ExecutionContext ctx) {
5664
J.Switch switch_ = super.visitSwitch(sw, ctx);

‎src/main/java/org/openrewrite/java/migrate/lang/SwitchCaseEnumGuardToLabel.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,12 @@
2424
import org.openrewrite.TreeVisitor;
2525
import org.openrewrite.java.JavaIsoVisitor;
2626
import org.openrewrite.java.JavaVisitor;
27+
import org.openrewrite.java.search.UsesJavaVersion;
2728
import org.openrewrite.java.tree.Expression;
2829
import org.openrewrite.java.tree.J;
2930
import org.openrewrite.java.tree.JavaType;
3031
import org.openrewrite.java.tree.TypeUtils;
32+
import org.openrewrite.staticanalysis.groovy.GroovyFileChecker;
3133
import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker;
3234

3335
import static java.util.Collections.singletonList;
@@ -47,7 +49,13 @@ public String getDescription() {
4749

4850
@Override
4951
public TreeVisitor<?, ExecutionContext> getVisitor() {
50-
return Preconditions.check(Preconditions.not(new KotlinFileChecker<>()), new JavaIsoVisitor<ExecutionContext>() {
52+
TreeVisitor<?, ExecutionContext> preconditions = Preconditions.and(
53+
new UsesJavaVersion<>(21),
54+
Preconditions.not(new KotlinFileChecker<>()),
55+
Preconditions.not(new GroovyFileChecker<>())
56+
);
57+
58+
return Preconditions.check(preconditions, new JavaIsoVisitor<ExecutionContext>() {
5159

5260
@Override
5361
public J.Case visitCase(J.Case case_, ExecutionContext ctx) {

‎src/main/resources/META-INF/rewrite/java-version-21.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ recipeList:
3939
- org.openrewrite.java.migrate.UpgradePluginsForJava21
4040
- org.openrewrite.java.migrate.DeleteDeprecatedFinalize
4141
- org.openrewrite.java.migrate.RemovedSubjectMethods
42-
# - org.openrewrite.java.migrate.SwitchPatternMatching
42+
#- org.openrewrite.java.migrate.SwitchPatternMatching
43+
#- org.openrewrite.java.migrate.lang.NullCheckAsSwitchCase
4344

4445
---
4546
type: specs.openrewrite.org/v1beta/recipe
@@ -142,6 +143,5 @@ tags:
142143
- java21
143144
recipeList:
144145
- org.openrewrite.java.migrate.lang.IfElseIfConstructToSwitch
145-
- org.openrewrite.java.migrate.lang.NullCheckAsSwitchCase
146146
- org.openrewrite.java.migrate.lang.RefineSwitchCases
147147
- org.openrewrite.java.migrate.lang.SwitchCaseEnumGuardToLabel

0 commit comments

Comments
 (0)