Skip to content

Commit 31c8e0d

Browse files
jjohnstnjjohnstn
authored
Fix Pattern instanceof cleanup to handle casts in the if statement (eclipse-jdt#2104)
- modify PatterMatchingForInstanceofFixCore to handle casts in the if statement of the instanceof expression - add new scenario to CleanUpTest16 - fixes eclipse-jdt#2094
1 parent 4816a1e commit 31c8e0d

File tree

2 files changed

+119
-28
lines changed

2 files changed

+119
-28
lines changed

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

Lines changed: 93 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ public boolean visit(final Block visited) {
8383

8484
final class InstanceofVisitor extends ASTVisitor {
8585
private final Block startNode;
86+
8687
private boolean result= true;
88+
8789
private final Set<String> excludedNames= new HashSet<>();
8890

8991
public InstanceofVisitor(final Block startNode) {
@@ -117,9 +119,9 @@ public boolean visit(final InstanceofExpression visited) {
117119
}
118120
while (currentNode.getParent() != null
119121
&& (!(currentNode.getParent() instanceof IfStatement)
120-
|| currentNode.getLocationInParent() != IfStatement.EXPRESSION_PROPERTY)
122+
|| currentNode.getLocationInParent() != IfStatement.EXPRESSION_PROPERTY)
121123
&& (!(currentNode.getParent() instanceof ConditionalExpression)
122-
|| currentNode.getLocationInParent() != ConditionalExpression.EXPRESSION_PROPERTY)) {
124+
|| currentNode.getLocationInParent() != ConditionalExpression.EXPRESSION_PROPERTY)) {
123125
switch (currentNode.getParent().getNodeType()) {
124126
case ASTNode.PARENTHESIZED_EXPRESSION:
125127
break;
@@ -135,7 +137,7 @@ public boolean visit(final InstanceofExpression visited) {
135137
case ASTNode.INFIX_EXPRESSION:
136138
if (isPositiveCaseToAnalyze
137139
? !ASTNodes.hasOperator((InfixExpression) currentNode.getParent(), InfixExpression.Operator.CONDITIONAL_AND, InfixExpression.Operator.AND)
138-
: !ASTNodes.hasOperator((InfixExpression) currentNode.getParent(), InfixExpression.Operator.CONDITIONAL_OR, InfixExpression.Operator.OR)) {
140+
: !ASTNodes.hasOperator((InfixExpression) currentNode.getParent(), InfixExpression.Operator.CONDITIONAL_OR, InfixExpression.Operator.OR)) {
139141
return true;
140142
}
141143

@@ -183,6 +185,39 @@ public boolean visit(final InstanceofExpression visited) {
183185
collector.collect(ASTNodes.getNextSibling(ifStatement));
184186
}
185187

188+
final boolean isPositiveCaseToAnalyzeFinal= isPositiveCaseToAnalyze;
189+
ASTVisitor expressionVisitor= new ASTVisitor() {
190+
@Override
191+
public boolean visit(CastExpression node) {
192+
if (node.getStartPosition() > visited.getStartPosition()) {
193+
if (Objects.equals(getIdentifierName(node.getExpression()), getIdentifierName(visited.getLeftOperand()))) {
194+
ITypeBinding nodeBinding= node.resolveTypeBinding();
195+
if (nodeBinding != null && nodeBinding.isEqualTo(visited.getRightOperand().resolveBinding())) {
196+
Expression exp= node;
197+
InfixExpression infixExp= ASTNodes.getFirstAncestorOrNull(exp, InfixExpression.class);
198+
InfixExpression originalInfixExp= ASTNodes.getFirstAncestorOrNull(visited, InfixExpression.class);
199+
while (infixExp != null && infixExp != originalInfixExp) {
200+
infixExp= ASTNodes.getFirstAncestorOrNull(infixExp, InfixExpression.class);
201+
}
202+
if (infixExp != null) {
203+
if ((isPositiveCaseToAnalyzeFinal && infixExp.getOperator() != InfixExpression.Operator.CONDITIONAL_AND) ||
204+
(!isPositiveCaseToAnalyzeFinal && infixExp.getOperator() != InfixExpression.Operator.CONDITIONAL_OR)) {
205+
return true;
206+
}
207+
}
208+
while (exp.getLocationInParent() == ParenthesizedExpression.EXPRESSION_PROPERTY) {
209+
exp= (Expression) node.getParent();
210+
}
211+
collector.addExpressionToReplace(exp);
212+
}
213+
}
214+
}
215+
return true;
216+
}
217+
};
218+
219+
ifStatement.getExpression().accept(expressionVisitor);
220+
186221
if (collector.hasResult()) {
187222
fResult.add(collector.build());
188223
return false;
@@ -191,17 +226,29 @@ public boolean visit(final InstanceofExpression visited) {
191226
return true;
192227
}
193228

229+
private static String getIdentifierName(Expression expression) {
230+
if (expression instanceof SimpleName simpleName) {
231+
return simpleName.getIdentifier();
232+
}
233+
return null;
234+
}
235+
194236
static class ConditionalCollector extends ASTVisitor {
195237
final InstanceofExpression visited;
238+
196239
final Set<String> excludedNames;
240+
197241
final SimpleName variableName;
242+
198243
String replacementName= null;
199-
final List<Expression> expressionsToReplace = new ArrayList<>();
244+
245+
final List<Expression> expressionsToReplace= new ArrayList<>();
200246

201247
ConditionalCollector(InstanceofExpression visited, SimpleName name, Set<String> excludedNames) {
202-
this.visited = visited;
248+
this.visited= visited;
203249
this.excludedNames= excludedNames;
204-
this.variableName= name; }
250+
this.variableName= name;
251+
}
205252

206253
public PatternMatchingForConditionalInstanceofFixOperation build() {
207254
return new PatternMatchingForConditionalInstanceofFixOperation(visited, expressionsToReplace, replacementName);
@@ -235,7 +282,7 @@ void addMatching(final CastExpression castExp, final SimpleName name) {
235282
private String proposeLocalName(String baseName, CompilationUnit root, IJavaProject javaProject, MethodDeclaration methodDecl, String[] usedNames) {
236283
// don't propose names that are already in use:
237284
Collection<String> variableNames= new ScopeAnalyzer(root).getUsedVariableNames(methodDecl.getStartPosition(), methodDecl.getLength());
238-
String[] names = new String[variableNames.size() + usedNames.length];
285+
String[] names= new String[variableNames.size() + usedNames.length];
239286
variableNames.toArray(names);
240287
System.arraycopy(usedNames, 0, names, variableNames.size(), usedNames.length);
241288
return StubUtility.getLocalNameSuggestions(javaProject, baseName, 0, names)[0];
@@ -244,7 +291,7 @@ private String proposeLocalName(String baseName, CompilationUnit root, IJavaProj
244291
@Override
245292
public boolean visit(SimpleName node) {
246293
if (node.getLocationInParent() == CastExpression.EXPRESSION_PROPERTY) {
247-
addMatching((CastExpression)node.getParent(), node);
294+
addMatching((CastExpression) node.getParent(), node);
248295
}
249296
return false;
250297
}
@@ -253,23 +300,25 @@ public boolean visit(SimpleName node) {
253300

254301
static class ResultCollector {
255302
final InstanceofExpression visited;
303+
256304
SimpleName variableName;
257-
final List<VariableDeclarationStatement> statementsToRemove = new ArrayList<>();
258-
final List<VariableDeclarationStatement> statementsToConvert = new ArrayList<>();
305+
306+
final List<VariableDeclarationStatement> statementsToRemove= new ArrayList<>();
307+
308+
final List<VariableDeclarationStatement> statementsToConvert= new ArrayList<>();
309+
310+
final List<Expression> expressionsToReplace= new ArrayList<>();
259311

260312
ResultCollector(InstanceofExpression visited) {
261-
this.visited = visited;
313+
this.visited= visited;
262314
}
263315

264316
public PatternMatchingForInstanceofFixOperation build() {
265-
return new PatternMatchingForInstanceofFixOperation(visited, statementsToRemove, statementsToConvert, variableName);
317+
return new PatternMatchingForInstanceofFixOperation(visited, statementsToRemove, statementsToConvert, expressionsToReplace, variableName);
266318
}
267319

268-
private String getIdentifierName(Expression expression) {
269-
if (expression instanceof SimpleName simpleName) {
270-
return simpleName.getIdentifier();
271-
}
272-
return null;
320+
public void addExpressionToReplace(Expression exp) {
321+
expressionsToReplace.add(exp);
273322
}
274323

275324
boolean hasResult() {
@@ -278,7 +327,7 @@ boolean hasResult() {
278327

279328
void addMatching(final VariableDeclarationStatement statement, final SimpleName name, boolean toConvert) {
280329
if (this.variableName == null || this.variableName.getIdentifier().equals(name.getIdentifier())) {
281-
this.variableName = name;
330+
this.variableName= name;
282331
if (toConvert) {
283332
this.statementsToConvert.add(statement);
284333
} else {
@@ -289,12 +338,12 @@ void addMatching(final VariableDeclarationStatement statement, final SimpleName
289338

290339
boolean collect(final Statement conditionalStatements) {
291340
List<Statement> statements= ASTNodes.asList(conditionalStatements);
292-
boolean convertToAssignment = false;
341+
boolean convertToAssignment= false;
293342

294343
if (!statements.isEmpty()) {
295344
for (Statement statement : statements) {
296345
if (statement instanceof ExpressionStatement expressionStatement) {
297-
Assignment assignment = ASTNodes.as(expressionStatement.getExpression(), Assignment.class);
346+
Assignment assignment= ASTNodes.as(expressionStatement.getExpression(), Assignment.class);
298347
if (assignment != null) {
299348
if (Objects.equals(getIdentifierName(visited.getLeftOperand()), getIdentifierName(assignment.getLeftHandSide()))) {
300349
// The same variable is assigned, this can't be handled further, something like this:
@@ -328,11 +377,11 @@ boolean collect(final Statement conditionalStatements) {
328377
}
329378
if (statement instanceof IfStatement innerIf) {
330379
if (collect(innerIf.getThenStatement())) {
331-
convertToAssignment = true;
380+
convertToAssignment= true;
332381
}
333382
if (innerIf.getElseStatement() != null) {
334383
if (collect(innerIf.getElseStatement())) {
335-
convertToAssignment = true;
384+
convertToAssignment= true;
336385
}
337386
}
338387
}
@@ -348,7 +397,9 @@ boolean collect(final Statement conditionalStatements) {
348397

349398
public static class PatternMatchingForConditionalInstanceofFixOperation extends CompilationUnitRewriteOperation {
350399
private final InstanceofExpression nodeToComplete;
400+
351401
private final List<Expression> expressionsToReplace;
402+
352403
private final String replacementName;
353404

354405
public PatternMatchingForConditionalInstanceofFixOperation(final InstanceofExpression nodeToComplete, final List<Expression> expressionsToReplace, final String replacementName) {
@@ -388,14 +439,21 @@ public void rewriteAST(final CompilationUnitRewrite cuRewrite, final LinkedPropo
388439

389440
public static class PatternMatchingForInstanceofFixOperation extends CompilationUnitRewriteOperation {
390441
private final InstanceofExpression nodeToComplete;
442+
391443
private final List<VariableDeclarationStatement> statementsToRemove;
444+
392445
private final List<VariableDeclarationStatement> statementsToConvert;
446+
447+
private final List<Expression> expressionsToReplace;
448+
393449
private final SimpleName expressionToMove;
394450

395-
public PatternMatchingForInstanceofFixOperation(final InstanceofExpression nodeToComplete, final List<VariableDeclarationStatement> statementsToRemove, final List<VariableDeclarationStatement> statementsToConvert, final SimpleName expressionToMove) {
451+
public PatternMatchingForInstanceofFixOperation(final InstanceofExpression nodeToComplete, final List<VariableDeclarationStatement> statementsToRemove,
452+
final List<VariableDeclarationStatement> statementsToConvert, final List<Expression> expressionsToReplace, final SimpleName expressionToMove) {
396453
this.nodeToComplete= nodeToComplete;
397454
this.statementsToRemove= statementsToRemove;
398455
this.statementsToConvert= statementsToConvert;
456+
this.expressionsToReplace= expressionsToReplace;
399457
this.expressionToMove= expressionToMove;
400458
}
401459

@@ -442,12 +500,18 @@ public SourceRange computeSourceRange(final ASTNode nodeWithComment) {
442500
}
443501
importRemover.registerRemovedNode(statementToRemove);
444502
}
445-
for (var statementToConvert: statementsToConvert) {
446-
VariableDeclarationFragment fragment = ASTNodes.getUniqueFragment(statementToConvert);
447-
var assignment = ast.newAssignment();
503+
504+
for (var expressionToReplace : expressionsToReplace) {
505+
var newName= rewrite.createCopyTarget(expressionToMove);
506+
rewrite.replace(expressionToReplace, newName, group);
507+
}
508+
509+
for (var statementToConvert : statementsToConvert) {
510+
VariableDeclarationFragment fragment= ASTNodes.getUniqueFragment(statementToConvert);
511+
var assignment= ast.newAssignment();
448512
assignment.setLeftHandSide((Expression) ASTNode.copySubtree(ast, fragment.getName()));
449513
assignment.setRightHandSide((Expression) ASTNode.copySubtree(ast, fragment.getInitializer()));
450-
var replacement = ast.newExpressionStatement(assignment);
514+
var replacement= ast.newExpressionStatement(assignment);
451515
ASTNodes.replaceButKeepComment(rewrite, statementToConvert, replacement, group);
452516
}
453517
}
@@ -467,7 +531,8 @@ public static ICleanUpFix createCleanUp(final CompilationUnit compilationUnit) {
467531
return new PatternMatchingForInstanceofFixCore(FixMessages.PatternMatchingForInstanceofFix_refactor, compilationUnit, ops);
468532
}
469533

470-
protected PatternMatchingForInstanceofFixCore(final String name, final CompilationUnit compilationUnit, final CompilationUnitRewriteOperationsFixCore.CompilationUnitRewriteOperation[] fixRewriteOperations) {
534+
protected PatternMatchingForInstanceofFixCore(final String name, final CompilationUnit compilationUnit,
535+
final CompilationUnitRewriteOperationsFixCore.CompilationUnitRewriteOperation[] fixRewriteOperations) {
471536
super(name, compilationUnit, fixRewriteOperations);
472537
}
473538
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,20 @@ public int foo5(Object o, Object p) {
617617
}
618618
return !(o instanceof String) ? i : ((String)o).length();
619619
}
620+
public int foo6(Object o) {
621+
if (o instanceof String && ((String)o).length() > 3) {
622+
String s = (String) o;
623+
return s.length - 3();
624+
}
625+
return !(o instanceof String) ? 0 : ((String)o).length();
626+
}
627+
public int foo7(Object o) {
628+
if (!(o instanceof String) || ((String)o).length() > 3) {
629+
return -1;
630+
}
631+
String s = (String)o;
632+
return !(o instanceof String) ? 0 : ((String)o).length();
633+
}
620634
}
621635
""";
622636
ICompilationUnit cu= pack.createCompilationUnit("E.java", given, false, null);
@@ -686,6 +700,18 @@ public int foo5(Object o, Object p) {
686700
}
687701
return !(o instanceof String s2) ? i : s2.length();
688702
}
703+
public int foo6(Object o) {
704+
if (o instanceof String s && s.length() > 3) {
705+
return s.length - 3();
706+
}
707+
return !(o instanceof String s2) ? 0 : s2.length();
708+
}
709+
public int foo7(Object o) {
710+
if (!(o instanceof String s) || s.length() > 3) {
711+
return -1;
712+
}
713+
return !(o instanceof String s2) ? 0 : s2.length();
714+
}
689715
}
690716
""";
691717

0 commit comments

Comments
 (0)