From 1fb845cd240f3182abb754508b052da8dfa125af Mon Sep 17 00:00:00 2001 From: Bas Leijdekkers Date: Mon, 20 Aug 2012 17:03:17 +0200 Subject: [PATCH] IDEA-86518 (Quickfix for identical catch branches produces incrorrect and incompilable code) --- .../siyeh/InspectionGadgetsBundle.properties | 4 +- .../TryWithIdenticalCatchesInspection.java | 197 +++++++++++++----- .../TryIdenticalCatches.after.java | 15 ++ .../TryIdenticalCatches.java | 19 +- .../TryWithIdenticalCatchesTest.java | 2 +- 5 files changed, 175 insertions(+), 62 deletions(-) diff --git a/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties b/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties index e938e1bea904..f6103d4acf1e 100644 --- a/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties +++ b/plugins/InspectionGadgets/src/com/siyeh/InspectionGadgetsBundle.properties @@ -1859,7 +1859,7 @@ package.dot.html.convert.command=package.html to package-info.java conversion choose.super.class.to.ignore=Choose class ignore.anonymous.inner.classes=Ignore anonymous inner classes try.with.identical.catches.display.name=Identical 'catch' branches in 'try' statement -try.with.identical.catches.problem.descriptor=Identical 'catch' branches in 'try' statement #loc +try.with.identical.catches.problem.descriptor='catch' branch identical to ''{0}'' branch #loc if.can.be.switch.display.name='if' replaceable with 'switch' if.can.be.switch.problem.descriptor=#ref statement replaceable with 'switch' statement #loc if.can.be.switch.quickfix=Replace with 'switch' @@ -1868,7 +1868,7 @@ if.can.be.switch.int.option=Suggest switch on numbers if.can.be.switch.enum.option=Suggest switch on enums unnecessarily.qualified.inner.class.access.option=Ignore references for which an import is needed unqualified.inner.class.access.option=Ignore references to local inner classes -try.with.identical.catches.quickfix=Collapse catch blocks into multi-catch +try.with.identical.catches.quickfix=Collapse 'catch' blocks confusing.else.option=Also report when there are no more statements after the 'if' statement html.tag.can.be.javadoc.tag.display.name=... can be replaced with {@code ...} html.tag.can.be.javadoc.tag.problem.descriptor=#ref...\\</code\\> can be replaced with '{@code ...}' #loc diff --git a/plugins/InspectionGadgets/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java b/plugins/InspectionGadgets/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java index 3889d0d53af9..a904e414b16a 100644 --- a/plugins/InspectionGadgets/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java +++ b/plugins/InspectionGadgets/src/com/siyeh/ig/migration/TryWithIdenticalCatchesInspection.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2011 JetBrains s.r.o. + * Copyright 2000-2012 JetBrains s.r.o. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,14 +30,14 @@ import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; import com.siyeh.ig.InspectionGadgetsFix; import org.jetbrains.annotations.Nls; -import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; +import java.util.ArrayList; import java.util.Collections; import java.util.List; /** - * @author yole + * @author yole, Bas Leijdekkers */ public class TryWithIdenticalCatchesInspection extends BaseInspection { @@ -49,7 +49,8 @@ public class TryWithIdenticalCatchesInspection extends BaseInspection { @NotNull @Override protected String buildErrorString(Object... infos) { - return InspectionGadgetsBundle.message("try.with.identical.catches.problem.descriptor"); + final PsiType type = (PsiType)infos[1]; + return InspectionGadgetsBundle.message("try.with.identical.catches.problem.descriptor", type.getPresentableText()); } @Override @@ -66,54 +67,95 @@ public class TryWithIdenticalCatchesInspection extends BaseInspection { @Override protected InspectionGadgetsFix buildFix(Object... infos) { - return new CollapseCatchSectionsFix((Integer)infos[0]); + return new CollapseCatchSectionsFix(((Integer)infos[0]).intValue()); } private static class TryWithIdenticalCatchesVisitor extends BaseInspectionVisitor { + @Override public void visitTryStatement(PsiTryStatement statement) { super.visitTryStatement(statement); if (!PsiUtil.isLanguageLevel7OrHigher(statement)) { return; } - PsiCatchSection[] catchSections = statement.getCatchSections(); - boolean[] duplicates = new boolean[catchSections.length]; - for (int i = 0; i < catchSections.length; i++) { - if (duplicates[i]) continue; + final PsiCatchSection[] catchSections = statement.getCatchSections(); + if (catchSections.length < 2) { + return; + } + final boolean[] duplicates = new boolean[catchSections.length]; + for (int i = 0; i < catchSections.length - 1; i++) { final PsiCatchSection catchSection = catchSections[i]; final PsiCodeBlock catchBlock = catchSection.getCatchBlock(); - if (catchBlock == null) continue; + if (catchBlock == null) { + continue; + } final PsiParameter parameter = catchSection.getParameter(); - if (parameter == null) continue; - InputVariables inputVariables = new InputVariables(Collections.singletonList(parameter), - statement.getProject(), - new LocalSearchScope(catchBlock), - false); - DuplicatesFinder finder = new DuplicatesFinder(new PsiElement[]{catchBlock}, - inputVariables, null, Collections.emptyList()); - for (int j = 0; j < catchSections.length; j++) { - if (i == j || duplicates[j]) continue; + if (parameter == null) { + continue; + } + final InputVariables inputVariables = new InputVariables(Collections.singletonList(parameter), + statement.getProject(), + new LocalSearchScope(catchBlock), + false); + final DuplicatesFinder finder = new DuplicatesFinder(new PsiElement[]{catchBlock}, + inputVariables, null, Collections.emptyList()); + final PsiParameter[] parameters = statement.getCatchBlockParameters(); + for (int j = i + 1; j < catchSections.length; j++) { + if (duplicates[j]) { + continue; + } final PsiCatchSection otherSection = catchSections[j]; final PsiCodeBlock otherCatchBlock = otherSection.getCatchBlock(); - if (otherCatchBlock == null) continue; - Match match = finder.isDuplicate(otherCatchBlock, true); - if (match != null && match.getReturnValue() == null) { - final List parameterValues = match.getParameterValues(parameter); - if (parameterValues == null || (parameterValues.size() == 1 && parameterValues.get(0) instanceof PsiReferenceExpression)) { - PsiJavaToken rParenth = otherSection.getRParenth(); - if (rParenth != null) { - registerErrorAtOffset(otherSection, 0, rParenth.getStartOffsetInParent() + 1, i); - } - duplicates[i] = true; - duplicates[j] = true; - } + if (otherCatchBlock == null) { + continue; + } + final Match match = finder.isDuplicate(otherCatchBlock, true); + if (match == null || match.getReturnValue() != null) { + continue; + } + final List parameterValues = match.getParameterValues(parameter); + if (parameterValues != null && (parameterValues.size() != 1 || !(parameterValues.get(0) instanceof PsiReferenceExpression))) { + continue; + } + if (!canCollapse(parameters, i, j)) { + continue; + } + final PsiJavaToken rParenth = otherSection.getRParenth(); + if (rParenth != null) { + registerErrorAtOffset(otherSection, 0, rParenth.getStartOffsetInParent() + 1, Integer.valueOf(i), parameter.getType()); + } + duplicates[i] = true; + duplicates[j] = true; + } + } + } + + private static boolean canCollapse(PsiParameter[] parameters, int index1, int index2) { + if (index2 > index1) { + final PsiType type = parameters[index2].getType(); + for (int i = index1 + 1; i < index2; i++) { + final PsiType otherType = parameters[i].getType(); + if (TypeConversionUtil.isAssignable(type, otherType)) { + return false; } } + return true; + } + else { + final PsiType type = parameters[index1].getType(); + for (int i = index2 + 1; i < index1; i++) { + final PsiType otherType = parameters[i].getType(); + if (TypeConversionUtil.isAssignable(otherType, type)) { + return false; + } + } + return true; } } } private static class CollapseCatchSectionsFix extends InspectionGadgetsFix { + private final int myCollapseIntoIndex; public CollapseCatchSectionsFix(int collapseIntoIndex) { @@ -121,35 +163,76 @@ public class TryWithIdenticalCatchesInspection extends BaseInspection { } @Override - protected void doFix(Project project, ProblemDescriptor descriptor) throws IncorrectOperationException { - PsiCatchSection section = (PsiCatchSection)descriptor.getPsiElement(); - PsiTryStatement stmt = (PsiTryStatement)section.getParent(); - PsiCatchSection[] catchSections = stmt.getCatchSections(); - if (myCollapseIntoIndex >= catchSections.length) { - return; // something has gone stale - } - PsiCatchSection collapseInto = catchSections[myCollapseIntoIndex]; - PsiParameter parameter1 = collapseInto.getParameter(); - PsiParameter parameter2 = section.getParameter(); - if (parameter1 == null || parameter2 == null) { - return; - } - String typeText = parameter1.getTypeElement().getText() + " | " + parameter2.getTypeElement().getText() + " e"; - if (TypeConversionUtil.isAssignable(parameter1.getType(), parameter2.getType())) { - typeText = parameter1.getText(); - } - else if (TypeConversionUtil.isAssignable(parameter2.getType(), parameter1.getType())) { - typeText = parameter2.getText(); - } - @NonNls String text = "try { } catch(" + typeText + ") { }"; - PsiTryStatement newTryCatch = (PsiTryStatement)JavaPsiFacade.getElementFactory(project).createStatementFromText(text, stmt); - parameter1.getTypeElement().replace(newTryCatch.getCatchSections()[0].getParameter().getTypeElement()); - section.delete(); - } - @NotNull public String getName() { return InspectionGadgetsBundle.message("try.with.identical.catches.quickfix"); } + + @Override + protected void doFix(Project project, ProblemDescriptor descriptor) throws IncorrectOperationException { + final PsiCatchSection section = (PsiCatchSection)descriptor.getPsiElement(); + final PsiTryStatement tryStatement = (PsiTryStatement)section.getParent(); + final PsiCatchSection[] catchSections = tryStatement.getCatchSections(); + if (myCollapseIntoIndex >= catchSections.length) { + return; // something has gone stale + } + final PsiCatchSection collapseInto = catchSections[myCollapseIntoIndex]; + final PsiParameter parameter1 = collapseInto.getParameter(); + final PsiParameter parameter2 = section.getParameter(); + if (parameter1 == null || parameter2 == null) { + return; + } + final PsiType type1 = parameter1.getType(); + final PsiType type2 = parameter2.getType(); + if (TypeConversionUtil.isAssignable(type1, type2)) { + section.delete(); + return; + } + else if (TypeConversionUtil.isAssignable(type2, type1)) { + collapseInto.delete(); + return; + } + final List types = new ArrayList(); + collectDisjunctTypes(type1, types); + collectDisjunctTypes(type2, types); + final StringBuilder typeText = new StringBuilder(); + for (PsiType type : types) { + if (typeText.length() > 0) { + typeText.append(" | "); + } + typeText.append(type.getCanonicalText()); + } + final PsiTypeElement newTypeElement = + JavaPsiFacade.getElementFactory(project).createTypeElementFromText(typeText.toString(), tryStatement); + final PsiTypeElement typeElement = parameter1.getTypeElement(); + if (typeElement == null) { + return; + } + typeElement.replace(newTypeElement); + section.delete(); + } + + private static void collectDisjunctTypes(PsiType type, List out) { + if (type instanceof PsiDisjunctionType) { + final PsiDisjunctionType disjunctionType = (PsiDisjunctionType)type; + final List disjunctions = disjunctionType.getDisjunctions(); + for (PsiType disjunction : disjunctions) { + collectDisjunctTypes(disjunction, out); + } + return; + } + final int size = out.size(); + for (int i = 0; i < size; i++) { + final PsiType collectedType = out.get(i); + if (TypeConversionUtil.isAssignable(type, collectedType)) { + out.remove(i); + out.add(type); + return; + } else if (TypeConversionUtil.isAssignable(collectedType, type)) { + return; + } + } + out.add(type); + } } } diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.after.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.after.java index 15a89ae955c8..83163330ca7e 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.after.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.after.java @@ -90,4 +90,19 @@ class TryIdenticalCatches { private void log(Exception e) { } + + class E1 extends RuntimeException {} + class E2 extends E1 {} + class E3 extends RuntimeException {} + class E4 extends E3 {} + + void p() { + try { + + } catch (E4 e) { + } catch (E2 e) { + } catch (E3 e) { + } catch (E1 e) { + } + } } \ No newline at end of file diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.java index 46fb07a6c669..683d22eef5ba 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.java @@ -69,7 +69,7 @@ class TryIdenticalCatches { catch(ClassNotFoundException cnfe) { System.out.println(); } - catch(NumberFormatException nfe) { + catch(NumberFormatException nfe) { System.out.println(); } } @@ -86,11 +86,26 @@ class TryIdenticalCatches { catch(ClassNotFoundException cnfe) { log(cnfe); } - catch(NumberFormatException nfe) { + catch(NumberFormatException nfe) { log(nfe); } } private void log(Exception e) { } + + class E1 extends RuntimeException {} + class E2 extends E1 {} + class E3 extends RuntimeException {} + class E4 extends E3 {} + + void p() { + try { + + } catch (E4 e) { + } catch (E2 e) { + } catch (E3 e) { + } catch (E1 e) { + } + } } \ No newline at end of file diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/migration/TryWithIdenticalCatchesTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/migration/TryWithIdenticalCatchesTest.java index 275bde1618d6..55682f6bc880 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/migration/TryWithIdenticalCatchesTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/migration/TryWithIdenticalCatchesTest.java @@ -27,7 +27,7 @@ public class TryWithIdenticalCatchesTest extends LightCodeInsightFixtureTestCase myFixture.enableInspections(TryWithIdenticalCatchesInspection.class); myFixture.configureByFile("com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.java"); myFixture.checkHighlighting(true, false, false); - IntentionAction intention = myFixture.findSingleIntention("Collapse catch blocks into multi-catch"); + IntentionAction intention = myFixture.findSingleIntention("Collapse 'catch' blocks"); assertNotNull(intention); myFixture.launchAction(intention); myFixture.checkResultByFile("com/siyeh/igtest/errorhandling/try_identical_catches/TryIdenticalCatches.after.java");