Skip to content

Commit

Permalink
Fix one if cleanup to watch for PatternInstanceof nodes (#1201)
Browse files Browse the repository at this point in the history 
* Fix one if cleanup to watch for PatternInstanceof nodes

- fixes #1200
- add new PatternInstanceof logic to
  OneIfRatherThanDuplicateBlocksThatFallThroughFixCore
- add new test to CleanUpTest16
  • Loading branch information
@jjohnstn
jjohnstn authored Mar 9, 2024
1 parent bef8bb1 commit 16ca3ea
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 3 deletions.
9 changes: 9 additions & 0 deletions org.eclipse.jdt.core.manipulation/.settings/.api_filters
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@
</message_arguments>
</filter>
</resource>
<resource path="core extension/org/eclipse/jdt/internal/corext/fix/OneIfRatherThanDuplicateBlocksThatFallThroughFixCore.java" type="org.eclipse.jdt.internal.corext.fix.OneIfRatherThanDuplicateBlocksThatFallThroughFixCore$OneIfRatherThanDuplicateBlocksThatFallThroughFinder$SuccessiveIfVisitor$PatternNameVisitor">
<filter comment="Allow TypePattern.patternVariables() call" id="640712815">
<message_arguments>
<message_argument value="TypePattern"/>
<message_argument value="PatternNameVisitor"/>
<message_argument value="patternVariables()"/>
</message_arguments>
</filter>
</resource>
<resource path="core extension/org/eclipse/jdt/internal/corext/fix/PatternMatchingForInstanceofFixCore.java" type="org.eclipse.jdt.internal.corext.fix.PatternMatchingForInstanceofFixCore$PatternMatchingForInstanceofFixOperation">
<filter comment="Need to set pattern of TypePatter" id="640712815">
<message_arguments>
Expand Down
44 changes: 41 additions & 3 deletions ...eclipse/jdt/internal/corext/fix/OneIfRatherThanDuplicateBlocksThatFallThroughFixCore.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2021 Fabrice TIERCELIN and others.
* Copyright (c) 2021, 2024 Fabrice TIERCELIN and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -14,7 +14,9 @@
package org.eclipse.jdt.internal.corext.fix;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;

import org.eclipse.core.runtime.CoreException;
Expand All @@ -29,6 +31,10 @@
import org.eclipse.jdt.core.dom.Expression;
import org.eclipse.jdt.core.dom.IfStatement;
import org.eclipse.jdt.core.dom.InfixExpression;
import org.eclipse.jdt.core.dom.Pattern;
import org.eclipse.jdt.core.dom.PatternInstanceofExpression;
import org.eclipse.jdt.core.dom.SingleVariableDeclaration;
import org.eclipse.jdt.core.dom.TypePattern;
import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
import org.eclipse.jdt.core.dom.rewrite.TargetSourceRangeComputer;

Expand Down Expand Up @@ -72,10 +78,14 @@ public boolean visit(final Block node) {
public boolean visit(final IfStatement visited) {
if (result && ASTNodes.fallsThrough(visited.getThenStatement())) {
List<IfStatement> duplicateIfBlocks= new ArrayList<>(ASTNodes.EXCESSIVE_OPERAND_NUMBER - 1);
Set<String> patternNames= new HashSet<>();
PatternNameVisitor visitor= new PatternNameVisitor();
visited.accept(visitor);
patternNames= visitor.getPatternNames();
AtomicInteger operandCount= new AtomicInteger(ASTNodes.getNbOperands(visited.getExpression()));
duplicateIfBlocks.add(visited);

while (addOneMoreIf(duplicateIfBlocks, operandCount)) {
while (addOneMoreIf(duplicateIfBlocks, patternNames, operandCount)) {
// OK continue
}

Expand All @@ -89,7 +99,27 @@ public boolean visit(final IfStatement visited) {
return true;
}

private boolean addOneMoreIf(final List<IfStatement> duplicateIfBlocks, final AtomicInteger operandCount) {
private class PatternNameVisitor extends ASTVisitor {
private Set<String> patternNames= new HashSet<>();

@Override
public boolean visit(PatternInstanceofExpression node) {
Pattern p= node.getPattern();
if (p instanceof TypePattern typePattern) {
List<SingleVariableDeclaration> patternVariables= typePattern.patternVariables();
for (SingleVariableDeclaration patternVariable : patternVariables) {
patternNames.add(patternVariable.getName().getFullyQualifiedName());
}
}
return true;
}

public Set<String> getPatternNames() {
return patternNames;
}
}

private boolean addOneMoreIf(final List<IfStatement> duplicateIfBlocks, final Set<String> patternNames, final AtomicInteger operandCount) {
IfStatement lastBlock= duplicateIfBlocks.get(duplicateIfBlocks.size() - 1);

if (lastBlock.getElseStatement() == null) {
Expand All @@ -99,6 +129,14 @@ private boolean addOneMoreIf(final List<IfStatement> duplicateIfBlocks, final At
&& nextSibling.getElseStatement() == null
&& operandCount.get() + ASTNodes.getNbOperands(nextSibling.getExpression()) < ASTNodes.EXCESSIVE_OPERAND_NUMBER
&& ASTNodes.match(lastBlock.getThenStatement(), nextSibling.getThenStatement())) {
PatternNameVisitor visitor= new PatternNameVisitor();
nextSibling.getExpression().accept(visitor);
Set<String> siblingPatternNames= visitor.getPatternNames();
for (String siblingPatternName : siblingPatternNames) {
if (!patternNames.add(siblingPatternName)) {
return false;
}
}
operandCount.addAndGet(ASTNodes.getNbOperands(nextSibling.getExpression()));
duplicateIfBlocks.add(nextSibling);
return true;
Expand Down
62 changes: 62 additions & 0 deletions org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest16.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,68 @@ public void testPatternMatchingForInstanceof2() throws Exception { // https://gi
new HashSet<>(Arrays.asList(MultiFixMessages.PatternMatchingForInstanceofCleanup_description)));
}

@Test
public void testOneIfWithPatternInstanceof() throws Exception { // https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1200
IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null);
String sample= "" //
+ "package test1;\n" //
+ "\n" //
+ "public class E {\n" //
+ "\n" //
+ " protected String getString(Number number) {\n" //
+ "\n" //
+ " if (number instanceof Long n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if (number instanceof Float n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if (number instanceof Double n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if (number instanceof Float n && n.isInfinite()) {\n" //
+ " return \"Inf\"; //$NON-NLS-1$\n" //
+ " }\n" //
+ " if (number instanceof Double m && m.isInfinite()) {\n" //
+ " return \"Inf\"; //$NON-NLS-1$\n" //
+ " }\n" //
+ "\n" //
+ " return null;\n" //
+ " }\n" //
+ "\n" //
+ "}\n"; //
ICompilationUnit cu= pack.createCompilationUnit("E.java", sample, false, null);

enable(CleanUpConstants.ONE_IF_RATHER_THAN_DUPLICATE_BLOCKS_THAT_FALL_THROUGH);

String expected= "" //
+ "package test1;\n" //
+ "\n" //
+ "public class E {\n" //
+ "\n" //
+ " protected String getString(Number number) {\n" //
+ "\n" //
+ " if (number instanceof Long n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if (number instanceof Float n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if (number instanceof Double n) {\n" //
+ " return n.toString();\n" //
+ " }\n" //
+ " if ((number instanceof Float n && n.isInfinite()) || (number instanceof Double m && m.isInfinite())) {\n" //
+ " return \"Inf\"; //$NON-NLS-1$\n" //
+ " }\n" //
+ "\n" //
+ " return null;\n" //
+ " }\n" //
+ "\n" //
+ "}\n"; //
assertRefactoringResultAsExpected(new ICompilationUnit[] { cu }, new String[] { expected },
new HashSet<>(Arrays.asList(MultiFixMessages.OneIfRatherThanDuplicateBlocksThatFallThroughCleanUp_description)));
}

@Test
public void testDoNotMatchPatternForInstanceof() throws Exception {
IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null);
Expand Down

0 comments on commit 16ca3ea

Please sign in to comment.