Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix one if cleanup to watch for PatternInstanceof nodes #1201

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Fix one if cleanup to watch for PatternInstanceof nodes #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
733c313
Fix one if cleanup to watch for PatternInstanceof nodes
jjohnstn Feb 16, 2024
3587fd2
Remove warning
jjohnstn Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions org.eclipse.jdt.core.manipulation/.settings/.api_filters
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading