Skip to content

Commit 0e8c1c3

Browse files
jjohnstnjjohnstn
authored
Enable null case recognition in PatternInstanceofToSwitchFixCore (eclipse-jdt#2077)
- modify PatternInstanceofToSwitchFixCore to recognize an if equals to null in the chain of ifs and create a separate null case for it - add and modify CleanUpTest21 with new null case logic - fixes eclipse-jdt#2073
1 parent 91b44c8 commit 0e8c1c3

File tree

2 files changed

+157
-31
lines changed

2 files changed

+157
-31
lines changed

‎org.eclipse.jdt.core.manipulation/core extension/org/eclipse/jdt/internal/corext/fix/PatternInstanceofToSwitchFixCore.java‎

Lines changed: 82 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@
1717
import java.lang.reflect.Modifier;
1818
import java.util.ArrayList;
1919
import java.util.Collections;
20+
import java.util.HashSet;
2021
import java.util.Iterator;
2122
import java.util.List;
23+
import java.util.Set;
2224

2325
import org.eclipse.core.runtime.CoreException;
2426

@@ -44,8 +46,11 @@
4446
import org.eclipse.jdt.core.dom.ITypeBinding;
4547
import org.eclipse.jdt.core.dom.IVariableBinding;
4648
import org.eclipse.jdt.core.dom.IfStatement;
49+
import org.eclipse.jdt.core.dom.InfixExpression;
50+
import org.eclipse.jdt.core.dom.InfixExpression.Operator;
4751
import org.eclipse.jdt.core.dom.Modifier.ModifierKeyword;
4852
import org.eclipse.jdt.core.dom.Name;
53+
import org.eclipse.jdt.core.dom.NullLiteral;
4954
import org.eclipse.jdt.core.dom.PatternInstanceofExpression;
5055
import org.eclipse.jdt.core.dom.ReturnStatement;
5156
import org.eclipse.jdt.core.dom.SimpleName;
@@ -113,6 +118,7 @@ public void setPatternNameUsed(boolean value) {
113118
}
114119

115120
private List<CompilationUnitRewriteOperation> fResult;
121+
private Set<IfStatement> ifsProcessed= new HashSet<>();
116122

117123
public SwitchStatementsFinder(List<CompilationUnitRewriteOperation> ops) {
118124
fResult= ops;
@@ -129,6 +135,9 @@ final class SeveralIfVisitor extends ASTVisitor {
129135

130136
@Override
131137
public boolean visit(final IfStatement visited) {
138+
if (ifsProcessed.contains(visited)) {
139+
return false;
140+
}
132141
TypeVariable variable= extractVariableAndValues(visited);
133142

134143
if (variable == null) {
@@ -147,8 +156,13 @@ public boolean visit(final IfStatement visited) {
147156
IfStatement currentNode= ifStatement;
148157

149158
while (ASTNodes.isSameVariable(switchExpression, variable.name)) {
150-
cases.add(new SwitchCaseSection(variable.getTypePattern(), variable.isPatternNameUsed(),
151-
ASTNodes.asList(currentNode.getThenStatement())));
159+
if (variable.getTypePattern() == null) {
160+
cases.add(new NullSwitchCaseSection(variable.getTypePattern(), variable.isPatternNameUsed(),
161+
ASTNodes.asList(currentNode.getThenStatement())));
162+
} else {
163+
cases.add(new SwitchCaseSection(variable.getTypePattern(), variable.isPatternNameUsed(),
164+
ASTNodes.asList(currentNode.getThenStatement())));
165+
}
152166

153167
if (!ASTNodes.fallsThrough(currentNode.getThenStatement())) {
154168
isFallingThrough= false;
@@ -184,6 +198,7 @@ public boolean visit(final IfStatement visited) {
184198
private boolean maybeReplaceWithSwitchStatement(final List<IfStatement> ifStatements, final Expression switchExpression,
185199
final List<SwitchCaseSection> cases, final Statement remainingStatement) {
186200
if (switchExpression != null && cases.size() > 2) {
201+
ifsProcessed.addAll(ifStatements);
187202
PatternToSwitchExpressionOperation op= getOperation(ifStatements, switchExpression, cases, remainingStatement);
188203
if (op != null) {
189204
fResult.add(op);
@@ -339,15 +354,17 @@ private TypeVariable extractVariableAndValues(final Statement statement) {
339354
CompilationUnit cu= (CompilationUnit) ifStmt.getRoot();
340355
if (JavaModelUtil.is22OrHigher(cu.getJavaElement().getJavaProject())) {
341356
TypePattern pattern= result.getTypePattern();
342-
VariableDeclaration vd= pattern.getPatternVariable2();
343-
SimpleName name= vd.getName();
344-
IBinding binding= name.resolveBinding();
345-
NameUsedVisitor visitor= new NameUsedVisitor(binding);
346-
try {
347-
ifStmt.getThenStatement().accept(visitor);
348-
result.setPatternNameUsed(false);
349-
} catch (AbortSearchException e) {
350-
result.setPatternNameUsed(true);
357+
if (pattern != null) {
358+
VariableDeclaration vd= pattern.getPatternVariable2();
359+
SimpleName name= vd.getName();
360+
IBinding binding= name.resolveBinding();
361+
NameUsedVisitor visitor= new NameUsedVisitor(binding);
362+
try {
363+
ifStmt.getThenStatement().accept(visitor);
364+
result.setPatternNameUsed(false);
365+
} catch (AbortSearchException e) {
366+
result.setPatternNameUsed(true);
367+
}
351368
}
352369
}
353370
return result;
@@ -363,6 +380,9 @@ private TypeVariable extractVariableAndValues(final Expression expression) {
363380
return extractVariableAndValuesFromPatternExpression(pattern);
364381
}
365382

383+
if (expression instanceof InfixExpression infix && infix.getOperator() == Operator.EQUALS) {
384+
return extractVariableAndValuesFromInfixExpression(infix);
385+
}
366386
return null;
367387
}
368388

@@ -374,6 +394,14 @@ private TypeVariable extractVariableAndValuesFromPatternExpression(final Pattern
374394
return null;
375395
}
376396

397+
private TypeVariable extractVariableAndValuesFromInfixExpression(InfixExpression infix) {
398+
if (infix.getLeftOperand() instanceof NullLiteral) {
399+
return new TypeVariable(infix.getRightOperand(), null);
400+
} else if (infix.getRightOperand() instanceof NullLiteral) {
401+
return new TypeVariable(infix.getLeftOperand(), null);
402+
}
403+
return null;
404+
}
377405
}
378406
}
379407

@@ -413,15 +441,21 @@ public SourceRange computeSourceRange(final ASTNode nodeWithComment) {
413441

414442
switchStatement.setExpression((Expression) rewrite.createCopyTarget(switchExpression));
415443

444+
boolean haveNullCase= false;
416445
for (SwitchCaseSection aCase : cases) {
417-
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, aCase.typePattern, aCase.isNameUsed, aCase.statements);
446+
if (aCase instanceof NullSwitchCaseSection) {
447+
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, aCase.typePattern, true, false, aCase.isNameUsed, aCase.statements);
448+
haveNullCase= true;
449+
} else {
450+
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, aCase.typePattern, false, haveNullCase, aCase.isNameUsed, aCase.statements);
451+
}
418452
}
419453

420454
if (remainingStatement != null) {
421455
remainingStatement.setProperty(UNTOUCH_COMMENT_PROPERTY, Boolean.TRUE);
422-
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, null, false, ASTNodes.asList(remainingStatement));
456+
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, null, false, haveNullCase, false, ASTNodes.asList(remainingStatement));
423457
} else {
424-
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, null, false, Collections.emptyList());
458+
addCaseWithStatements(rewrite, importRewrite, ast, switchStatement, null, false, haveNullCase, false, Collections.emptyList());
425459
}
426460

427461
for (int i= 0; i < ifStatements.size() - 1; i++) {
@@ -434,6 +468,8 @@ public SourceRange computeSourceRange(final ASTNode nodeWithComment) {
434468
private void addCaseWithStatements(final ASTRewrite rewrite, final ImportRewrite importRewrite,
435469
final AST ast, final SwitchStatement switchStatement,
436470
final TypePattern caseValueOrNullForDefault,
471+
final boolean isNullCase,
472+
final boolean haveNullCase,
437473
final boolean isNameUsed,
438474
final List<Statement> innerStatements) {
439475
List<Statement> switchStatements= switchStatement.statements();
@@ -467,8 +503,12 @@ private void addCaseWithStatements(final ASTRewrite rewrite, final ImportRewrite
467503
} else {
468504
SwitchCase newSwitchCase= ast.newSwitchCase();
469505
newSwitchCase.setSwitchLabeledRule(true);
470-
newSwitchCase.expressions().add(ast.newNullLiteral());
471-
newSwitchCase.expressions().add(ast.newCaseDefaultExpression());
506+
if (!haveNullCase) {
507+
newSwitchCase.expressions().add(ast.newNullLiteral());
508+
}
509+
if (!isNullCase && !haveNullCase) {
510+
newSwitchCase.expressions().add(ast.newCaseDefaultExpression());
511+
}
472512
switchStatements.add(newSwitchCase);
473513
}
474514

@@ -541,6 +581,7 @@ public void rewriteAST(CompilationUnitRewrite cuRewrite, LinkedProposalModelCore
541581
newSwitchExpression.setExpression(newSwitchExpressionExpression);
542582

543583
// build switch expression
584+
boolean haveNullCase= false;
544585
for (SwitchCaseSection switchCaseSection : cases) {
545586
List<Statement> oldStatements= switchCaseSection.statements;
546587
SwitchCase switchCase= null;
@@ -549,11 +590,15 @@ public void rewriteAST(CompilationUnitRewrite cuRewrite, LinkedProposalModelCore
549590
newSwitchCase.setSwitchLabeledRule(true);
550591
switchCase= newSwitchCase;
551592
if (switchCaseSection.typePattern == null) {
552-
// for the empty pattern, create a null/default case
553-
Expression nullExpression= ast.newNullLiteral();
554-
Expression defaultExpression= ast.newCaseDefaultExpression();
555-
switchCase.expressions().add(nullExpression);
556-
switchCase.expressions().add(defaultExpression);
593+
if (!haveNullCase) {
594+
Expression nullExpression= ast.newNullLiteral();
595+
switchCase.expressions().add(nullExpression);
596+
}
597+
if (!(switchCaseSection instanceof NullSwitchCaseSection) && !haveNullCase) {
598+
Expression defaultExpression= ast.newCaseDefaultExpression();
599+
switchCase.expressions().add(defaultExpression);
600+
}
601+
haveNullCase= true;
557602
} else {
558603
Expression oldExpression= switchCaseSection.typePattern;
559604
Expression newExpression= (Expression)rewrite.createCopyTarget(oldExpression);
@@ -683,7 +728,9 @@ public void rewriteAST(CompilationUnitRewrite cuRewrite, LinkedProposalModelCore
683728
rewrite.replace(ifStatements.get(0), newExpressionStatement, group);
684729
}
685730
for (int i= 1; i< ifStatements.size(); ++i) {
686-
rewrite.remove(ifStatements.get(i), group);
731+
if (ifStatements.get(i).getLocationInParent() != IfStatement.ELSE_STATEMENT_PROPERTY) {
732+
rewrite.remove(ifStatements.get(i), group);
733+
}
687734
}
688735
}
689736

@@ -733,14 +780,25 @@ protected PatternInstanceofToSwitchFixCore(final String name, final CompilationU
733780
super(name, compilationUnit, fixRewriteOperations);
734781
}
735782

783+
/**
784+
* Represents a null switch case section
785+
*/
786+
private static class NullSwitchCaseSection extends SwitchCaseSection {
787+
private NullSwitchCaseSection(final TypePattern typePattern,
788+
final boolean isNameUsed,
789+
final List<Statement> statements) {
790+
super(typePattern, isNameUsed, statements);
791+
}
792+
}
793+
736794
/**
737795
* Represents a switch case section (cases + statements).
738796
* <p>
739797
* It can represent a switch case to build (when converting if else if
740798
* statements), or existing switch cases when representing the structure of a
741799
* whole switch.
742800
*/
743-
private static final class SwitchCaseSection {
801+
private static class SwitchCaseSection {
744802
/**
745803
* Must resolve to constant values. Used when representing switch cases to
746804
* build.
@@ -758,6 +816,7 @@ private static final class SwitchCaseSection {
758816
* Used for if statements, only constant expressions are used.
759817
*
760818
* @param typePattern The type pattern
819+
* @param isNameUsed Whether or not the instanceof variable name is used in statements
761820
* @param statements The statements
762821
*/
763822
private SwitchCaseSection(final TypePattern typePattern,

‎org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest21.java‎

Lines changed: 75 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ public int foo(Object x, Object y) {
326326
return square(8); // square
327327
} else if (y instanceof final Boolean xboolean) {
328328
throw new NullPointerException();
329-
} else if (obj == null) {
329+
} else if (y == null) {
330330
System.out.println("null");
331331
} else {
332332
System.out.println("unknown");
@@ -356,13 +356,8 @@ public int foo(Object x, Object y) {
356356
return square(8); // square
357357
}
358358
case Boolean xboolean -> throw new NullPointerException();
359-
case null, default -> {
360-
if (obj == null) {
361-
System.out.println("null");
362-
} else {
363-
System.out.println("unknown");
364-
}
365-
}
359+
case null -> System.out.println("null");
360+
default -> System.out.println("unknown");
366361
}
367362
}
368363
}
@@ -628,6 +623,78 @@ public int foo(Object y) {
628623
assertRefactoringResultAsExpected(new ICompilationUnit[] { cu1 }, new String[] { expected1 }, null);
629624
}
630625

626+
@Test
627+
public void testPatternInstanceofToSwitchExpression4() throws Exception {
628+
Hashtable<String, String> options= JavaCore.getOptions();
629+
options.put(DefaultCodeFormatterConstants.FORMATTER_TAB_CHAR, JavaCore.TAB);
630+
JavaCore.setOptions(options);
631+
IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null);
632+
String sample= """
633+
package test1;
634+
635+
public class E {
636+
int i;
637+
double d;
638+
boolean b;
639+
640+
public int square(int x) {
641+
return x*x;
642+
}
643+
public int foo(Object y) {
644+
if (y instanceof final Integer xint) {
645+
return xint;
646+
}
647+
if (y instanceof final Double xdouble) {
648+
return square(8); // square
649+
} else if (y instanceof final Boolean xboolean) {
650+
throw new NullPointerException();
651+
} else if (y == null) {
652+
return 7;
653+
} else {
654+
i = 0;
655+
d = 0.0D;
656+
b = false;
657+
return 11;
658+
}
659+
}
660+
}
661+
""";
662+
ICompilationUnit cu1= pack1.createCompilationUnit("E.java", sample, false, null);
663+
664+
enable(CleanUpConstants.USE_SWITCH_FOR_INSTANCEOF_PATTERN);
665+
666+
sample= """
667+
package test1;
668+
669+
public class E {
670+
int i;
671+
double d;
672+
boolean b;
673+
674+
public int square(int x) {
675+
return x*x;
676+
}
677+
public int foo(Object y) {
678+
return switch (y) {
679+
case Integer xint -> xint;
680+
case Double xdouble -> square(8); // square
681+
case Boolean xboolean -> throw new NullPointerException();
682+
case null -> 7;
683+
default -> {
684+
i = 0;
685+
d = 0.0D;
686+
b = false;
687+
yield 11;
688+
}
689+
};
690+
}
691+
}
692+
""";
693+
String expected1= sample;
694+
695+
assertRefactoringResultAsExpected(new ICompilationUnit[] { cu1 }, new String[] { expected1 }, null);
696+
}
697+
631698
public void testNoPatternInstanceofToSwitch1() throws Exception {
632699
Hashtable<String, String> options= JavaCore.getOptions();
633700
options.put(DefaultCodeFormatterConstants.FORMATTER_TAB_CHAR, JavaCore.TAB);

0 commit comments

Comments
 (0)