From 6b7080e2c760105960b9fdfc56809ad8de0cffcb Mon Sep 17 00:00:00 2001 From: Jeff Johnston Date: Mon, 13 Jan 2025 18:22:05 -0500 Subject: [PATCH] Fix merge of if conditions to guard against pattern instanceof - fix MergeConditionalBlocksCleanup to handle instanceof patterns and not merge them when they have same name - add new test to CleanUpTest16 - fixes #1200 --- .../jdt/ui/tests/quickfix/CleanUpTest16.java | 200 ++++++++++-------- .../ui/fix/MergeConditionalBlocksCleanUp.java | 175 +++++++++------ 2 files changed, 224 insertions(+), 151 deletions(-) diff --git a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest16.java b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest16.java index 86c6f0d6629..34dd9a98b45 100644 --- a/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest16.java +++ b/org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/quickfix/CleanUpTest16.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2020, 2024 Red Hat Inc. and others. + * Copyright (c) 2020, 2025 Red Hat Inc. and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -57,7 +57,7 @@ public void testRegexPatternForRecord() throws Exception { IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + public record E(int width, int height) { public void foo() { String k = "bcd"; @@ -75,12 +75,12 @@ public void foo() { sample= """ package test1; - + import java.util.regex.Pattern; - + public record E(int width, int height) { private static final Pattern k_pattern = Pattern.compile("bcd"); - + public void foo() { Pattern k = k_pattern; String m = "abcdef"; @@ -100,7 +100,7 @@ public void testRemoveRedundantSemicolonsForRecord() throws Exception { IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + public record E(int width, int height) { public void foo() { } @@ -112,7 +112,7 @@ public void foo() { sample= """ package test1; - + public record E(int width, int height) { public void foo() { } @@ -128,9 +128,9 @@ public void testPatternMatchingForInstanceof() throws Exception { IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null); String given= """ package test1; - + import java.util.Date; - + public class E { public long matchPatternForInstanceof(Object object) { // Keep this comment @@ -139,10 +139,10 @@ public long matchPatternForInstanceof(Object object) { Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long matchPatternForInstanceofOnFinalVariable(Object object) { // Keep this comment if (object instanceof Date) { @@ -150,20 +150,20 @@ public long matchPatternForInstanceofOnFinalVariable(Object object) { final Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long matchPatternInConditionalAndExpression(Object object, boolean isValid) { if (isValid && object instanceof Date) { // Keep this comment Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long matchPatternInElse(Object object) { if (!(object instanceof Date)) { return 0; @@ -172,7 +172,7 @@ public long matchPatternInElse(Object object) { return date.getTime(); } } - + public long matchPatternInConditionalOrExpression(Object object, boolean isValid) { if (!(object instanceof Date) || isValid) { return 0; @@ -181,21 +181,21 @@ public long matchPatternInConditionalOrExpression(Object object, boolean isValid return date.getTime(); } } - + public long matchPatternInElse(Object object) { if (!(object instanceof Date)) { return 0; } - + Date date = (Date) object; return date.getTime(); } - + public int matchPatternOnLoneStatement(Object object) { // Keep this comment if (!(object instanceof Date)) object.toString(); else {Date date = (Date) object;} - + return 0; } } @@ -206,9 +206,9 @@ public int matchPatternOnLoneStatement(Object object) { String expected= """ package test1; - + import java.util.Date; - + public class E { public long matchPatternForInstanceof(Object object) { // Keep this comment @@ -216,29 +216,29 @@ public long matchPatternForInstanceof(Object object) { // Keep this comment too return date.getTime(); } - + return 0; } - + public long matchPatternForInstanceofOnFinalVariable(Object object) { // Keep this comment if (object instanceof final Date date) { // Keep this comment too return date.getTime(); } - + return 0; } - + public long matchPatternInConditionalAndExpression(Object object, boolean isValid) { if (isValid && object instanceof Date date) { // Keep this comment return date.getTime(); } - + return 0; } - + public long matchPatternInElse(Object object) { if (!(object instanceof Date date)) { return 0; @@ -246,7 +246,7 @@ public long matchPatternInElse(Object object) { return date.getTime(); } } - + public long matchPatternInConditionalOrExpression(Object object, boolean isValid) { if (!(object instanceof Date date) || isValid) { return 0; @@ -254,20 +254,20 @@ public long matchPatternInConditionalOrExpression(Object object, boolean isValid return date.getTime(); } } - + public long matchPatternInElse(Object object) { if (!(object instanceof Date date)) { return 0; } - + return date.getTime(); } - + public int matchPatternOnLoneStatement(Object object) { // Keep this comment if (!(object instanceof Date date)) object.toString(); else {} - + return 0; } } @@ -283,7 +283,7 @@ public void testPatternMatchingForInstanceof2() throws Exception { // https://gi IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + public class E1 { \s static class InternalStaticClass { @@ -297,16 +297,16 @@ public int getK() { return k; } } - + } """; // pack.createCompilationUnit("E1.java", sample, false, null); sample= """ package test1; - + import test1.E1.InternalStaticClass; - + public class E { \s public void foo(Object x) { @@ -316,7 +316,7 @@ public void foo(Object x) { System.out.println(t.getK()); } } - + } """; // ICompilationUnit cu= pack.createCompilationUnit("E.java", sample, false, null); @@ -325,7 +325,7 @@ public void foo(Object x) { String expected= """ package test1; - + public class E { \s public void foo(Object x) { @@ -334,7 +334,7 @@ public void foo(Object x) { System.out.println(t.getK()); } } - + } """; // assertRefactoringResultAsExpected(new ICompilationUnit[] { cu }, new String[] { expected }, @@ -346,11 +346,11 @@ public void testOneIfWithPatternInstanceof() throws Exception { // https://githu IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + public class E { - + protected String getString(Number number) { - + if (number instanceof Long n) { return n.toString(); } @@ -366,10 +366,10 @@ protected String getString(Number number) { if (number instanceof Double m && m.isInfinite()) { return "Inf"; //$NON-NLS-1$ } - + return null; } - + } """; // ICompilationUnit cu= pack.createCompilationUnit("E.java", sample, false, null); @@ -378,11 +378,11 @@ protected String getString(Number number) { String expected= """ package test1; - + public class E { - + protected String getString(Number number) { - + if (number instanceof Long n) { return n.toString(); } @@ -395,10 +395,10 @@ protected String getString(Number number) { if ((number instanceof Float n && n.isInfinite()) || (number instanceof Double m && m.isInfinite())) { return "Inf"; //$NON-NLS-1$ } - + return null; } - + } """; // assertRefactoringResultAsExpected(new ICompilationUnit[] { cu }, new String[] { expected }, @@ -410,29 +410,29 @@ public void testDoNotMatchPatternForInstanceof() throws Exception { IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + import java.util.Date; import java.util.List; - + public class E { public long doNotMatchInOppositeCondition(Object object) { if (!(object instanceof Date)) { Date theDate = (Date) object; return theDate.getTime(); } - + return 0; } - + public long doNotMatchWithBadOperator(Object object, boolean isEnabled) { if (object instanceof Date || isEnabled) { Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long doNotMatchElseWithAndOperator(Object object, boolean isValid) { if (isValid && !(object instanceof Date)) { return 0; @@ -441,79 +441,79 @@ public long doNotMatchElseWithAndOperator(Object object, boolean isValid) { return date.getTime(); } } - + public long doNotMatchWrongObject(Object object, Object object2) { if (object instanceof Date) { Date date = (Date) object2; return date.getTime(); } - + return 0; } - + public long doNotMatchWrongType(Object object) { if (object instanceof Date) { java.sql.Date date = (java.sql.Date) object; return date.getTime(); } - + return 0; } - + public long doNotMatchActiveExpression(List objects) { if (objects.remove(0) instanceof Date) { Date date = (Date) objects.remove(0); return date.getTime(); } - + return 0; } - + public long doNotMatchAlreadyMatchedInstanceof(Object object) { if (object instanceof Date anotherDate) { Date date = (Date) object; date = new Date(); return date.getTime(); } - + return 0; } - + public long doNotMatchReassignedObject(Object object, Object object2) { if (object instanceof Date) { object = object2; Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long doNotMatchPatternInWhile(Object object) { while (object instanceof Date) { Date date = (Date) object; return date.getTime(); } - + return 0; } - + public long doNotMatchMultiDeclaration(Object object) { if (object instanceof Date) { Date date = (Date) object, anotherDate = null; return date.getTime(); } - + return 0; } - + public void doNotMatchOppositeStatements() { Object bah = 1; if (bah instanceof Integer) return; Integer i = (Integer) bah; System.out.println(i); } - + public void doNotMatchBitWiseAnd(boolean useStrikethroughForCompleted, Object data) { if (data instanceof Long & useStrikethroughForCompleted) { Long task = (Long)data; @@ -522,7 +522,7 @@ public void doNotMatchBitWiseAnd(boolean useStrikethroughForCompleted, Object da } } } - + public void doNotMatchBitWiseOr(boolean useStrikethroughForCompleted, Object data) { if (data instanceof Long | useStrikethroughForCompleted) { Long task = (Long)data; @@ -545,7 +545,7 @@ public void testDoNotAddFinalForRecordComponent() throws Exception { IPackageFragment pack= fSourceFolder.createPackageFragment("test1", false, null); String sample= """ package test1; - + public record E (String abc) { } """; @@ -562,7 +562,7 @@ public void testStringBufferToStringBuilderLocalsOnly() throws Exception { IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); String sample0= """ package test1; - + public class SuperClass { public static StringBuffer field0; }"""; @@ -570,7 +570,7 @@ public class SuperClass { String sample= """ package test1; - + public class TestStringBuilderCleanup extends SuperClass { private TestStringBuilderCleanup(){ } @@ -593,7 +593,7 @@ public static void changeWithFieldAccess(StringBuffer parm) { sample= """ package test1; - + public class TestStringBuilderCleanup extends SuperClass { private TestStringBuilderCleanup(){ } @@ -619,7 +619,7 @@ public void testDoNotChangeStringBufferToStringBuilderLocalsOnly() throws Except IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); String sample0= """ package test1; - + public class SuperClass { public static StringBuffer field0; }"""; @@ -627,7 +627,7 @@ public class SuperClass { String sample= """ package test1; - + public class TestStringBuilderCleanup extends SuperClass { public static StringBuffer field1; public record K(StringBuffer comp1) { @@ -668,7 +668,7 @@ public void testChangeStringBufferToStringBuilderAll() throws Exception { IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); String sample0= """ package test1; - + public class SuperClass { public static StringBuffer field0; }"""; @@ -676,10 +676,10 @@ public class SuperClass { String sample= """ package test1; - + import java.util.List; import java.util.ArrayList; - + public class TestStringBuilderCleanup extends SuperClass { public static StringBuffer field1; public record K(StringBuffer comp1) { @@ -715,17 +715,17 @@ public static void changeWithMethodCall(StringBuffer parm) { String expected0= """ package test1; - + public class SuperClass { public static StringBuilder field0; }"""; String expected1= """ package test1; - + import java.util.List; import java.util.ArrayList; - + public class TestStringBuilderCleanup extends SuperClass { public static StringBuilder field1; public record K(StringBuilder comp1) { @@ -764,7 +764,7 @@ public void testDoNotRemoveParenthesesFromPatternInstanceof() throws Exception { String sample= """ package test1; - + public class TestParenthesesRemoval { public static void doNotChangeParenthesesForInstanceof(Object o) { if (!(o instanceof String)) { @@ -787,4 +787,32 @@ public static void doNotChangeParenthesesForPatternInstanceof(Object o) { assertRefactoringHasNoChange(new ICompilationUnit[] { cu1 }); } + @Test + public void testDoNotMergeIfsForPatternInstanceof() throws Exception { + IPackageFragment pack1= fSourceFolder.createPackageFragment("test1", false, null); + + String sample= """ + package test1; + + public class TestInstanceOfMerge { + + public String foo(Number number) { + if (number instanceof Long n) { + return n.toString(); + } else if (number instanceof Float n) { + return n.toString(); + } else if (number instanceof Double n) { + return n.toString(); + } + return null; + } + } + """; + ICompilationUnit cu1= pack1.createCompilationUnit("TestInstanceOfMerge.java", sample, false, null); + + enable(CleanUpConstants.MERGE_CONDITIONAL_BLOCKS); + + assertRefactoringHasNoChange(new ICompilationUnit[] { cu1 }); + } + } diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/MergeConditionalBlocksCleanUp.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/MergeConditionalBlocksCleanUp.java index c357388028a..0fb43a53dbc 100644 --- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/MergeConditionalBlocksCleanUp.java +++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/fix/MergeConditionalBlocksCleanUp.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2020 Fabrice TIERCELIN and others. + * Copyright (c) 2020, 2025 Fabrice TIERCELIN and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -15,8 +15,10 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.core.runtime.CoreException; @@ -31,7 +33,11 @@ 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.Statement; +import org.eclipse.jdt.core.dom.TypePattern; import org.eclipse.jdt.core.dom.rewrite.ASTRewrite; import org.eclipse.jdt.internal.corext.dom.ASTNodeFactory; @@ -82,8 +88,8 @@ public String getPreview() { } else { System.out.println("Different"); } - - + + """; //$NON-NLS-1$ } @@ -98,84 +104,123 @@ public String getPreview() { """; //$NON-NLS-1$ } - @Override - protected ICleanUpFix createFix(CompilationUnit unit) throws CoreException { - if (!isEnabled(CleanUpConstants.MERGE_CONDITIONAL_BLOCKS)) { - return null; - } - - final List rewriteOperations= new ArrayList<>(); - - unit.accept(new ASTVisitor() { - @Override - public boolean visit(final IfStatement visited) { - if (visited.getElseStatement() != null) { - IfStatement innerIf= ASTNodes.as(visited.getThenStatement(), IfStatement.class); - - if (innerIf != null - && innerIf.getElseStatement() != null - && !ASTNodes.asList(visited.getElseStatement()).isEmpty() - && ASTNodes.getNbOperands(visited.getExpression()) + ASTNodes.getNbOperands(innerIf.getExpression()) < ASTNodes.EXCESSIVE_OPERAND_NUMBER) { - if (ASTNodes.match(visited.getElseStatement(), innerIf.getElseStatement())) { - rewriteOperations.add(new InnerIfOperation(visited, innerIf, true)); - return false; - } - - if (ASTNodes.match(visited.getElseStatement(), innerIf.getThenStatement())) { - rewriteOperations.add(new InnerIfOperation(visited, innerIf, false)); - return false; - } - } + private final class IfVisitor extends ASTVisitor { + final List fRewriteOperations; - List duplicateIfBlocks= new ArrayList<>(4); - List isThenStatement= new ArrayList<>(4); - AtomicInteger operandCount= new AtomicInteger(ASTNodes.getNbOperands(visited.getExpression())); - duplicateIfBlocks.add(visited); - isThenStatement.add(Boolean.TRUE); + public IfVisitor(List rewriteOperations) { + fRewriteOperations= rewriteOperations; + } - while (addOneMoreIf(duplicateIfBlocks, isThenStatement, operandCount)) { - // OK continue + @Override + public boolean visit(final IfStatement visited) { + if (visited.getElseStatement() != null) { + IfStatement innerIf= ASTNodes.as(visited.getThenStatement(), IfStatement.class); + + if (innerIf != null + && innerIf.getElseStatement() != null + && !ASTNodes.asList(visited.getElseStatement()).isEmpty() + && ASTNodes.getNbOperands(visited.getExpression()) + ASTNodes.getNbOperands(innerIf.getExpression()) < ASTNodes.EXCESSIVE_OPERAND_NUMBER) { + if (ASTNodes.match(visited.getElseStatement(), innerIf.getElseStatement())) { + fRewriteOperations.add(new InnerIfOperation(visited, innerIf, true)); + return false; } - if (duplicateIfBlocks.size() > 1) { - rewriteOperations.add(new MergeConditionalBlocksOperation(duplicateIfBlocks, isThenStatement)); + if (ASTNodes.match(visited.getElseStatement(), innerIf.getThenStatement())) { + fRewriteOperations.add(new InnerIfOperation(visited, innerIf, false)); return false; } } + List duplicateIfBlocks= new ArrayList<>(4); + Set patternNames= new HashSet<>(); + PatternNameVisitor visitor= new PatternNameVisitor(); + visited.accept(visitor); + patternNames= visitor.getPatternNames(); + List isThenStatement= new ArrayList<>(4); + AtomicInteger operandCount= new AtomicInteger(ASTNodes.getNbOperands(visited.getExpression())); + duplicateIfBlocks.add(visited); + isThenStatement.add(Boolean.TRUE); + + while (addOneMoreIf(duplicateIfBlocks, isThenStatement, operandCount, patternNames)){ + // OK continue + } + + if (duplicateIfBlocks.size() > 1) { + fRewriteOperations.add(new MergeConditionalBlocksOperation(duplicateIfBlocks, isThenStatement)); + return false; + } + } + + return true; + } + + private class PatternNameVisitor extends ASTVisitor { + private Set patternNames= new HashSet<>(); + + @Override + public boolean visit(PatternInstanceofExpression node) { + Pattern p= node.getPattern(); + if (p instanceof TypePattern typePattern) { + SingleVariableDeclaration patternVariable= typePattern.getPatternVariable(); + patternNames.add(patternVariable.getName().getFullyQualifiedName()); + } return true; } - private boolean addOneMoreIf(final List duplicateIfBlocks, final List isThenStatement, final AtomicInteger operandCount) { - IfStatement lastBlock= getLast(duplicateIfBlocks); - Statement previousStatement= getLast(isThenStatement) ? lastBlock.getThenStatement() : lastBlock.getElseStatement(); - Statement nextStatement= getLast(isThenStatement) ? lastBlock.getElseStatement() : lastBlock.getThenStatement(); - - if (nextStatement != null) { - IfStatement nextElse= ASTNodes.as(nextStatement, IfStatement.class); - - if (nextElse != null - && operandCount.get() + ASTNodes.getNbOperands(nextElse.getExpression()) < ASTNodes.EXCESSIVE_OPERAND_NUMBER) { - if (ASTNodes.match(previousStatement, nextElse.getThenStatement())) { - operandCount.addAndGet(ASTNodes.getNbOperands(nextElse.getExpression())); - duplicateIfBlocks.add(nextElse); - isThenStatement.add(Boolean.TRUE); - return true; - } + public Set getPatternNames() { + return patternNames; + } + } - if (nextElse.getElseStatement() != null - && ASTNodes.match(previousStatement, nextElse.getElseStatement())) { - operandCount.addAndGet(ASTNodes.getNbOperands(nextElse.getExpression())); - duplicateIfBlocks.add(nextElse); - isThenStatement.add(Boolean.FALSE); - return true; + private boolean addOneMoreIf(final List duplicateIfBlocks, final List isThenStatement, final AtomicInteger operandCount, Set patternNames) { + IfStatement lastBlock= getLast(duplicateIfBlocks); + Statement previousStatement= getLast(isThenStatement) ? lastBlock.getThenStatement() : lastBlock.getElseStatement(); + Statement nextStatement= getLast(isThenStatement) ? lastBlock.getElseStatement() : lastBlock.getThenStatement(); + + if (nextStatement != null) { + IfStatement nextElse= ASTNodes.as(nextStatement, IfStatement.class); + + if (nextElse != null + && operandCount.get() + ASTNodes.getNbOperands(nextElse.getExpression()) < ASTNodes.EXCESSIVE_OPERAND_NUMBER) { + PatternNameVisitor visitor= new PatternNameVisitor(); + nextElse.getExpression().accept(visitor); + Set siblingPatternNames= visitor.getPatternNames(); + for (String siblingPatternName : siblingPatternNames) { + if (!patternNames.add(siblingPatternName)) { + return false; } } - } + if (ASTNodes.match(previousStatement, nextElse.getThenStatement())) { + operandCount.addAndGet(ASTNodes.getNbOperands(nextElse.getExpression())); + duplicateIfBlocks.add(nextElse); + isThenStatement.add(Boolean.TRUE); + return true; + } - return false; + if (nextElse.getElseStatement() != null + && ASTNodes.match(previousStatement, nextElse.getElseStatement())) { + operandCount.addAndGet(ASTNodes.getNbOperands(nextElse.getExpression())); + duplicateIfBlocks.add(nextElse); + isThenStatement.add(Boolean.FALSE); + return true; + } + } } - }); + + return false; + } + } + + @Override + protected ICleanUpFix createFix(CompilationUnit unit) throws CoreException { + if (!isEnabled(CleanUpConstants.MERGE_CONDITIONAL_BLOCKS)) { + return null; + } + + final List rewriteOperations= new ArrayList<>(); + + IfVisitor visitor= new IfVisitor(rewriteOperations); + unit.accept(visitor); if (rewriteOperations.isEmpty()) { return null;