Skip to content

Commit ddece14

Browse files
gzsomborgzsombor
authored
Improve the instanceof pattern matching refactoring (eclipse-jdt#1964)
* Improve the pattern matching refactoring - Check if the smart casting can be used in deeper parts of the AST, not just directly as the first expression, which is surprisingly common. - Add new tests - fixes eclipse-jdt#2095
1 parent 7f07e65 commit ddece14

File tree

2 files changed

+355
-31
lines changed

2 files changed

+355
-31
lines changed

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

Lines changed: 116 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,12 @@
2424
import org.eclipse.jdt.core.dom.AST;
2525
import org.eclipse.jdt.core.dom.ASTNode;
2626
import org.eclipse.jdt.core.dom.ASTVisitor;
27+
import org.eclipse.jdt.core.dom.Assignment;
2728
import org.eclipse.jdt.core.dom.Block;
2829
import org.eclipse.jdt.core.dom.CastExpression;
2930
import org.eclipse.jdt.core.dom.CompilationUnit;
31+
import org.eclipse.jdt.core.dom.Expression;
32+
import org.eclipse.jdt.core.dom.ExpressionStatement;
3033
import org.eclipse.jdt.core.dom.IfStatement;
3134
import org.eclipse.jdt.core.dom.InfixExpression;
3235
import org.eclipse.jdt.core.dom.InfixExpression.Operator;
@@ -142,55 +145,127 @@ public boolean visit(final InstanceofExpression visited) {
142145

143146
IfStatement ifStatement= (IfStatement) currentNode.getParent();
144147

148+
ResultCollector collector = new ResultCollector(visited);
145149
if (isPositiveCaseToAnalyze) {
146-
return maybeMatchPattern(visited, ifStatement.getThenStatement());
150+
collector.collect(ifStatement.getThenStatement());
151+
} else if (ifStatement.getElseStatement() != null) {
152+
collector.collect(ifStatement.getElseStatement());
153+
} else if (ASTNodes.fallsThrough(ifStatement.getThenStatement())) {
154+
collector.collect(ASTNodes.getNextSibling(ifStatement));
147155
}
148156

149-
if (ifStatement.getElseStatement() != null) {
150-
return maybeMatchPattern(visited, ifStatement.getElseStatement());
157+
if (collector.hasResult()) {
158+
fResult.add(collector.build());
159+
return false;
151160
}
161+
return true;
162+
}
163+
164+
static class ResultCollector {
165+
final InstanceofExpression visited;
166+
SimpleName variableName;
167+
final List<VariableDeclarationStatement> statementsToRemove = new ArrayList<>();
168+
final List<VariableDeclarationStatement> statementsToConvert = new ArrayList<>();
152169

153-
if (ASTNodes.fallsThrough(ifStatement.getThenStatement())) {
154-
return maybeMatchPattern(visited, ASTNodes.getNextSibling(ifStatement));
170+
ResultCollector(InstanceofExpression visited) {
171+
this.visited = visited;
155172
}
156173

157-
return true;
158-
}
174+
public PatternMatchingForInstanceofFixOperation build() {
175+
return new PatternMatchingForInstanceofFixOperation(visited, statementsToRemove, statementsToConvert, variableName);
176+
}
159177

160-
private boolean maybeMatchPattern(final InstanceofExpression visited, final Statement conditionalStatements) {
161-
List<Statement> statements= ASTNodes.asList(conditionalStatements);
178+
private String getIdentifierName(Expression expression) {
179+
if (expression instanceof SimpleName simpleName) {
180+
return simpleName.getIdentifier();
181+
}
182+
return null;
183+
}
162184

163-
if (!statements.isEmpty()) {
164-
VariableDeclarationStatement variableDeclarationExpression= ASTNodes.as(statements.get(0), VariableDeclarationStatement.class);
165-
VariableDeclarationFragment variableDeclarationFragment= ASTNodes.getUniqueFragment(variableDeclarationExpression);
185+
boolean hasResult() {
186+
return variableName != null && !statementsToRemove.isEmpty();
187+
}
166188

167-
if (variableDeclarationFragment != null
168-
&& Objects.equals(visited.getRightOperand().resolveBinding(), variableDeclarationExpression.getType().resolveBinding())) {
169-
CastExpression castExpression= ASTNodes.as(variableDeclarationFragment.getInitializer(), CastExpression.class);
189+
void addMatching(final VariableDeclarationStatement statement, final SimpleName name, boolean toConvert) {
190+
if (this.variableName == null || this.variableName.getIdentifier().equals(name.getIdentifier())) {
191+
this.variableName = name;
192+
if (toConvert) {
193+
this.statementsToConvert.add(statement);
194+
} else {
195+
this.statementsToRemove.add(statement);
196+
}
197+
}
198+
}
170199

171-
if (castExpression != null
172-
&& Objects.equals(visited.getRightOperand().resolveBinding(), castExpression.getType().resolveBinding())
173-
&& ASTNodes.match(visited.getLeftOperand(), castExpression.getExpression())
174-
&& ASTNodes.isPassive(visited.getLeftOperand())) {
175-
fResult.add(new PatternMatchingForInstanceofFixOperation(visited, variableDeclarationExpression, variableDeclarationFragment.getName()));
176-
return false;
200+
boolean collect(final Statement conditionalStatements) {
201+
List<Statement> statements= ASTNodes.asList(conditionalStatements);
202+
boolean convertToAssignment = false;
203+
204+
if (!statements.isEmpty()) {
205+
for (Statement statement : statements) {
206+
if (statement instanceof ExpressionStatement expressionStatement) {
207+
Assignment assignment = ASTNodes.as(expressionStatement.getExpression(), Assignment.class);
208+
if (assignment != null) {
209+
if (Objects.equals(getIdentifierName(visited.getLeftOperand()), getIdentifierName(assignment.getLeftHandSide()))) {
210+
// The same variable is assigned, this can't be handled further, something like this:
211+
// if (x instanceof T) {
212+
// x = ...
213+
//
214+
return true;
215+
}
216+
if (Objects.equals(getIdentifierName(variableName), getIdentifierName(assignment.getLeftHandSide()))) {
217+
// The same variable is assigned, this can't be handled further, something like this:
218+
// if (x instanceof T y) {
219+
// y = ...
220+
//
221+
return true;
222+
}
223+
}
224+
}
225+
if (statement instanceof VariableDeclarationStatement variableDeclarationExpression) {
226+
VariableDeclarationFragment variableDeclarationFragment= ASTNodes.getUniqueFragment(variableDeclarationExpression);
227+
if (variableDeclarationFragment != null
228+
&& Objects.equals(visited.getRightOperand().resolveBinding(), variableDeclarationExpression.getType().resolveBinding())) {
229+
CastExpression castExpression= ASTNodes.as(variableDeclarationFragment.getInitializer(), CastExpression.class);
230+
231+
if (castExpression != null
232+
&& Objects.equals(visited.getRightOperand().resolveBinding(), castExpression.getType().resolveBinding())
233+
&& ASTNodes.match(visited.getLeftOperand(), castExpression.getExpression())
234+
&& ASTNodes.isPassive(visited.getLeftOperand())) {
235+
addMatching(variableDeclarationExpression, variableDeclarationFragment.getName(), convertToAssignment);
236+
}
237+
}
238+
}
239+
if (statement instanceof IfStatement innerIf) {
240+
if (collect(innerIf.getThenStatement())) {
241+
convertToAssignment = true;
242+
}
243+
if (innerIf.getElseStatement() != null) {
244+
if (collect(innerIf.getElseStatement())) {
245+
convertToAssignment = true;
246+
}
247+
}
248+
}
177249
}
178250
}
251+
return false;
179252
}
180253

181-
return true;
182254
}
255+
183256
}
184257
}
185258

186259
public static class PatternMatchingForInstanceofFixOperation extends CompilationUnitRewriteOperation {
187260
private final InstanceofExpression nodeToComplete;
188-
private final VariableDeclarationStatement statementToRemove;
261+
private final List<VariableDeclarationStatement> statementsToRemove;
262+
private final List<VariableDeclarationStatement> statementsToConvert;
189263
private final SimpleName expressionToMove;
190264

191-
public PatternMatchingForInstanceofFixOperation(final InstanceofExpression nodeToComplete, final VariableDeclarationStatement statementToRemove, final SimpleName expressionToMove) {
265+
public PatternMatchingForInstanceofFixOperation(final InstanceofExpression nodeToComplete, final List<VariableDeclarationStatement> statementsToRemove, final List<VariableDeclarationStatement> statementsToConvert, final SimpleName expressionToMove) {
192266
this.nodeToComplete= nodeToComplete;
193-
this.statementToRemove= statementToRemove;
267+
this.statementsToRemove= statementsToRemove;
268+
this.statementsToConvert= statementsToConvert;
194269
this.expressionToMove= expressionToMove;
195270
}
196271

@@ -216,7 +291,7 @@ public SourceRange computeSourceRange(final ASTNode nodeWithComment) {
216291
SingleVariableDeclaration newSVDecl= ast.newSingleVariableDeclaration();
217292
newSVDecl.setName(ASTNodes.createMoveTarget(rewrite, expressionToMove));
218293
newSVDecl.setType(ASTNodes.createMoveTarget(rewrite, nodeToComplete.getRightOperand()));
219-
if (Modifier.isFinal(statementToRemove.getModifiers())) {
294+
if (statementsToRemove.stream().allMatch(varDec -> Modifier.isFinal(varDec.getModifiers()))) {
220295
newSVDecl.modifiers().add(ast.newModifier(ModifierKeyword.fromFlagValue(Modifier.FINAL)));
221296
}
222297
if ((ast.apiLevel() == AST.JLS20 && ast.isPreviewEnabled()) || ast.apiLevel() > AST.JLS20) {
@@ -229,12 +304,22 @@ public SourceRange computeSourceRange(final ASTNode nodeWithComment) {
229304

230305
ASTNodes.replaceButKeepComment(rewrite, nodeToComplete, newInstanceof, group);
231306

232-
if (ASTNodes.canHaveSiblings(statementToRemove) || statementToRemove.getLocationInParent() == IfStatement.ELSE_STATEMENT_PROPERTY) {
233-
ASTNodes.removeButKeepComment(rewrite, statementToRemove, group);
234-
} else {
235-
ASTNodes.replaceButKeepComment(rewrite, statementToRemove, ast.newBlock(), group);
307+
for (var statementToRemove : statementsToRemove) {
308+
if (ASTNodes.canHaveSiblings(statementToRemove) || statementToRemove.getLocationInParent() == IfStatement.ELSE_STATEMENT_PROPERTY) {
309+
ASTNodes.removeButKeepComment(rewrite, statementToRemove, group);
310+
} else {
311+
ASTNodes.replaceButKeepComment(rewrite, statementToRemove, ast.newBlock(), group);
312+
}
313+
importRemover.registerRemovedNode(statementToRemove);
314+
}
315+
for (var statementToConvert: statementsToConvert) {
316+
VariableDeclarationFragment fragment = ASTNodes.getUniqueFragment(statementToConvert);
317+
var assignment = ast.newAssignment();
318+
assignment.setLeftHandSide((Expression) ASTNode.copySubtree(ast, fragment.getName()));
319+
assignment.setRightHandSide((Expression) ASTNode.copySubtree(ast, fragment.getInitializer()));
320+
var replacement = ast.newExpressionStatement(assignment);
321+
ASTNodes.replaceButKeepComment(rewrite, statementToConvert, replacement, group);
236322
}
237-
importRemover.registerRemovedNode(statementToRemove);
238323
}
239324
}
240325

0 commit comments

Comments
 (0)