Skip to content

Commit 2f72e68

Browse files
timtebeektimtebeek
authored
Fix switch-related recipe bugs (#6, #9, #14, #687) (#852)
- FallThrough: don't add breaks inside nested switch expressions (#14) - MinimumSwitchCases: preserve comments from default case (#6) - MinimumSwitchCases: redeclare variables in else blocks (#687) - InlineVariable: add test confirming enum arrays are not inlined (#9)
1 parent 92a5960 commit 2f72e68

5 files changed

Lines changed: 279 additions & 12 deletions

File tree

‎src/main/java/org/openrewrite/staticanalysis/FallThroughVisitor.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,13 @@ public J.Case visitCase(J.Case case_, P p) {
8484
private static class AddBreak<P> extends JavaIsoVisitor<P> {
8585
private final J.Case scope;
8686

87+
@Override
88+
public J.Switch visitSwitch(J.Switch switch_, P p) {
89+
// Don't descend into nested switches (including switch expressions)
90+
// to avoid adding breaks inside their cases/blocks
91+
return switch_;
92+
}
93+
8794
@Override
8895
public J.Case visitCase(J.Case case_, P p) {
8996
if (scope.isScope(case_)) {
@@ -181,11 +188,21 @@ private static boolean breaks(Statement s) {
181188
}
182189
return true;
183190
}
191+
if (s instanceof J.Switch) {
192+
// Arrow-style switches used as statements don't prevent fall-through
193+
J.Switch sw = (J.Switch) s;
194+
List<Statement> cases = sw.getCases().getStatements();
195+
for (Statement cs : cases) {
196+
if (cs instanceof J.Case && ((J.Case) cs).getType() == J.Case.Type.Rule) {
197+
return false;
198+
}
199+
}
200+
return true;
201+
}
184202
return s instanceof J.Return ||
185203
s instanceof J.Break ||
186204
s instanceof J.Continue ||
187-
s instanceof J.Throw ||
188-
s instanceof J.Switch;
205+
s instanceof J.Throw;
189206
}
190207

191208
@Override

‎src/main/java/org/openrewrite/staticanalysis/MinimumSwitchCases.java‎

Lines changed: 92 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,7 @@
3232
import org.openrewrite.marker.Markers;
3333
import org.openrewrite.staticanalysis.csharp.CSharpFileChecker;
3434

35-
import java.util.ArrayList;
36-
import java.util.List;
37-
import java.util.Set;
38-
import java.util.UUID;
35+
import java.util.*;
3936

4037
import static java.util.Collections.singleton;
4138
import static java.util.Collections.singletonList;
@@ -162,20 +159,45 @@ public J visitSwitch(J.Switch switch_, ExecutionContext ctx) {
162159

163160
// move first case to "if"
164161
List<Statement> thenStatements = getStatements(cases[0]);
162+
List<Statement> filteredThen = removeBreaksPreservingComments(thenStatements);
165163

166-
generatedIf = generatedIf.withThenPart(((J.Block) generatedIf.getThenPart()).withStatements(ListUtils.map(thenStatements,
167-
s -> s instanceof J.Break ? null : s)));
164+
generatedIf = generatedIf.withThenPart(((J.Block) generatedIf.getThenPart()).withStatements(filteredThen));
165+
166+
// Collect variable declarations from first case for redeclaration in else block
167+
Map<String, J.VariableDeclarations> declaredVars = collectDeclaredVariables(filteredThen);
168168

169169
// move second case to "else"
170170
if (cases[1] != null) {
171171
assert generatedIf.getElsePart() != null;
172+
List<Statement> elseStatements = removeBreaksPreservingComments(getStatements(cases[1]));
173+
// Transfer comments from the switch block's end space to the else block
174+
Space switchEnd = sortedSwitch.getCases().getEnd();
175+
if (!switchEnd.getComments().isEmpty()) {
176+
if (elseStatements.isEmpty()) {
177+
// Create a placeholder empty statement to carry the comments
178+
J.Block elseBlock = (J.Block) (isDefault(cases[1]) ?
179+
generatedIf.getElsePart().getBody() :
180+
((J.If) generatedIf.getElsePart().getBody()).getThenPart());
181+
elseBlock = elseBlock.withEnd(elseBlock.getEnd().withComments(
182+
ListUtils.concatAll(switchEnd.getComments(), elseBlock.getEnd().getComments())));
183+
if (isDefault(cases[1])) {
184+
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(elseBlock));
185+
} else {
186+
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(
187+
((J.If) generatedIf.getElsePart().getBody()).withThenPart(elseBlock)));
188+
}
189+
} else {
190+
Statement first = elseStatements.get(0);
191+
elseStatements.set(0, first.withPrefix(first.getPrefix().withComments(
192+
ListUtils.concatAll(switchEnd.getComments(), first.getPrefix().getComments()))));
193+
}
194+
}
195+
elseStatements = redeclareAssignments(elseStatements, declaredVars);
172196
if (isDefault(cases[1])) {
173-
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(((J.Block) generatedIf.getElsePart().getBody()).withStatements(ListUtils.map(getStatements(cases[1]),
174-
s -> s instanceof J.Break ? null : s))));
197+
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(((J.Block) generatedIf.getElsePart().getBody()).withStatements(elseStatements)));
175198
} else {
176199
J.If elseIf = (J.If) generatedIf.getElsePart().getBody();
177-
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(elseIf.withThenPart(((J.Block) elseIf.getThenPart()).withStatements(ListUtils.map(getStatements(cases[1]),
178-
s -> s instanceof J.Break ? null : s)))));
200+
generatedIf = generatedIf.withElsePart(generatedIf.getElsePart().withBody(elseIf.withThenPart(((J.Block) elseIf.getThenPart()).withStatements(elseStatements))));
179201
}
180202
}
181203

@@ -234,6 +256,66 @@ private List<Statement> getStatements(J.Case aCase) {
234256
return statements;
235257
}
236258

259+
private List<Statement> removeBreaksPreservingComments(List<Statement> statements) {
260+
return ListUtils.map(statements, (i, s) -> {
261+
if (s instanceof J.Break) {
262+
return null;
263+
}
264+
// Collect comments from any following break statement
265+
if (i + 1 < statements.size() && statements.get(i + 1) instanceof J.Break) {
266+
List<Comment> breakComments = statements.get(i + 1).getComments();
267+
if (!breakComments.isEmpty()) {
268+
return s.withPrefix(s.getPrefix().withComments(
269+
ListUtils.concatAll(s.getPrefix().getComments(), breakComments)));
270+
}
271+
}
272+
return s;
273+
});
274+
}
275+
276+
private Map<String, J.VariableDeclarations> collectDeclaredVariables(List<Statement> statements) {
277+
Map<String, J.VariableDeclarations> vars = new LinkedHashMap<>();
278+
for (Statement s : statements) {
279+
if (s instanceof J.VariableDeclarations) {
280+
J.VariableDeclarations vd = (J.VariableDeclarations) s;
281+
for (J.VariableDeclarations.NamedVariable v : vd.getVariables()) {
282+
vars.put(v.getSimpleName(), vd);
283+
}
284+
}
285+
}
286+
return vars;
287+
}
288+
289+
private List<Statement> redeclareAssignments(List<Statement> statements, Map<String, J.VariableDeclarations> declaredVars) {
290+
if (declaredVars.isEmpty()) {
291+
return statements;
292+
}
293+
List<Statement> result = new ArrayList<>();
294+
for (Statement s : statements) {
295+
if (s instanceof J.Assignment) {
296+
J.Assignment assignment = (J.Assignment) s;
297+
if (assignment.getVariable() instanceof J.Identifier) {
298+
String name = ((J.Identifier) assignment.getVariable()).getSimpleName();
299+
J.VariableDeclarations originalDecl = declaredVars.get(name);
300+
if (originalDecl != null) {
301+
J.VariableDeclarations.NamedVariable newVar = originalDecl.getVariables().get(0)
302+
.withName(((J.Identifier) assignment.getVariable()).withPrefix(Space.EMPTY))
303+
.withInitializer(assignment.getAssignment())
304+
.withId(randomId());
305+
J.VariableDeclarations newDecl = originalDecl
306+
.withVariables(singletonList(newVar))
307+
.withPrefix(s.getPrefix())
308+
.withId(randomId());
309+
result.add(newDecl);
310+
continue;
311+
}
312+
}
313+
}
314+
result.add(s);
315+
}
316+
return result;
317+
}
318+
237319
private boolean isDefault(J.Case case_) {
238320
return case_.getPattern() instanceof J.Identifier && "default".equals(((J.Identifier) case_.getPattern()).getSimpleName());
239321
}

‎src/test/java/org/openrewrite/staticanalysis/FallThroughTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,61 @@ void test(int day) {
102102
);
103103
}
104104

105+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/14")
106+
@Test
107+
void switchExpressionInsideSwitchStatement() {
108+
rewriteRun(
109+
//language=java
110+
java(
111+
"""
112+
class Test {
113+
void statement() {
114+
}
115+
116+
void test() {
117+
switch (2) {
118+
case 0:
119+
switch (3) {
120+
case 1 -> statement();
121+
case 3 -> {
122+
statement();
123+
statement();
124+
}
125+
}
126+
case 2:
127+
statement();
128+
break;
129+
}
130+
}
131+
}
132+
""",
133+
"""
134+
class Test {
135+
void statement() {
136+
}
137+
138+
void test() {
139+
switch (2) {
140+
case 0:
141+
switch (3) {
142+
case 1 -> statement();
143+
case 3 -> {
144+
statement();
145+
statement();
146+
}
147+
}
148+
break;
149+
case 2:
150+
statement();
151+
break;
152+
}
153+
}
154+
}
155+
"""
156+
)
157+
);
158+
}
159+
105160
@Test
106161
void switchExpressions() {
107162
rewriteRun(

‎src/test/java/org/openrewrite/staticanalysis/InlineVariableTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,32 @@ int[] test() {
253253
);
254254
}
255255

256+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/9")
257+
@Test
258+
void preserveEnumArray() {
259+
rewriteRun(
260+
//language=java
261+
java(
262+
"""
263+
enum ImportType {
264+
TIMETABLE, DUTIES
265+
}
266+
"""
267+
),
268+
//language=java
269+
java(
270+
"""
271+
class Test {
272+
static ImportType[] getList() {
273+
ImportType[] list = {ImportType.TIMETABLE, ImportType.DUTIES};
274+
return list;
275+
}
276+
}
277+
"""
278+
)
279+
);
280+
}
281+
256282
@Test
257283
void inlineAssignmentReturn() {
258284
rewriteRun(

‎src/test/java/org/openrewrite/staticanalysis/MinimumSwitchCasesTest.java‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,6 +1044,93 @@ void doSomething() {}
10441044
);
10451045
}
10461046

1047+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/6")
1048+
@Test
1049+
void preserveDefaultCaseComments() {
1050+
rewriteRun(
1051+
//language=java
1052+
java(
1053+
"""
1054+
class Test {
1055+
int variable;
1056+
void test() {
1057+
switch (variable) {
1058+
case 0:
1059+
doSomething();
1060+
break;
1061+
default:
1062+
// Pass other keys to children
1063+
}
1064+
}
1065+
void doSomething() {}
1066+
}
1067+
""",
1068+
"""
1069+
class Test {
1070+
int variable;
1071+
void test() {
1072+
if (variable == 0) {
1073+
doSomething();
1074+
} else {
1075+
// Pass other keys to children
1076+
}
1077+
}
1078+
void doSomething() {}
1079+
}
1080+
"""
1081+
)
1082+
);
1083+
}
1084+
1085+
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/687")
1086+
@Test
1087+
void variableRedeclaredInElseBlock() {
1088+
rewriteRun(
1089+
//language=java
1090+
java(
1091+
"""
1092+
@SuppressWarnings("ConstantConditions")
1093+
class Test {
1094+
int variable;
1095+
void test() {
1096+
switch (variable) {
1097+
case 1:
1098+
String data = getSomeString();
1099+
doThingWith(data);
1100+
break;
1101+
case 2:
1102+
data = getSomeOtherString();
1103+
doThingWith(data);
1104+
break;
1105+
}
1106+
}
1107+
String getSomeString() { return ""; }
1108+
String getSomeOtherString() { return ""; }
1109+
void doThingWith(String s) {}
1110+
}
1111+
""",
1112+
"""
1113+
@SuppressWarnings("ConstantConditions")
1114+
class Test {
1115+
int variable;
1116+
void test() {
1117+
if (variable == 1) {
1118+
String data = getSomeString();
1119+
doThingWith(data);
1120+
} else if (variable == 2) {
1121+
String data = getSomeOtherString();
1122+
doThingWith(data);
1123+
}
1124+
}
1125+
String getSomeString() { return ""; }
1126+
String getSomeOtherString() { return ""; }
1127+
void doThingWith(String s) {}
1128+
}
1129+
"""
1130+
)
1131+
);
1132+
}
1133+
10471134
@Issue("https://github.com/moderneinc/customer-requests/issues/1536")
10481135
@Test
10491136
void skipsTransformationWhenTypeInfoMissing() {

0 commit comments

Comments
 (0)
⚡