IDEA-86518 (Quickfix for identical catch branches produces incrorrect and incompilable code)

2026-09-27 10:03:11 +07:00 · 2012-08-20 17:03:17 +02:00
parent e52bfe669d
commit 1fb845cd24
5 changed files with 175 additions and 62 deletions
@@ -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=<code>#ref</code> 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=<html>Also report when there are no more statements after the 'if' statement</html>
 html.tag.can.be.javadoc.tag.display.name=<code>...</code> can be replaced with {@code ...}
 html.tag.can.be.javadoc.tag.problem.descriptor=<code>#ref...\\&lt;/code\\&gt;</code> can be replaced with '{@code ...}' #loc
@@ -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.<PsiVariable>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.<PsiVariable>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<PsiElement> 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<PsiElement> 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<PsiType> 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<PsiType> out) {
+      if (type instanceof PsiDisjunctionType) {
+        final PsiDisjunctionType disjunctionType = (PsiDisjunctionType)type;
+        final List<PsiType> 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);
+    }
  }
 }
@@ -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) {
+    }
+  }
 }
@@ -69,7 +69,7 @@ class TryIdenticalCatches {
     catch(ClassNotFoundException cnfe) {
       System.out.println();
     }
-     <warning descr="Identical 'catch' branches in 'try' statement">catch(NumberFormatException nfe)</warning> {
+     <warning descr="catch branch identical to 'ClassNotFoundException' branch">catch(NumberFormatException nfe)</warning> {
      System.out.println();
     }
   }
@@ -86,11 +86,26 @@ class TryIdenticalCatches {
    catch(ClassNotFoundException cnfe) {
      log(cnfe);
    }
-    <warning descr="Identical 'catch' branches in 'try' statement">catch(NumberFormatException n<caret>fe)</warning> {
+    <warning descr="catch branch identical to 'ClassNotFoundException' branch">catch(NumberFormatException n<caret>fe)</warning> {
      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) {
+    } <warning descr="catch branch identical to 'E4' branch">catch (E2 e)</warning> {
+    } <warning descr="catch branch identical to 'E4' branch">catch (E3 e)</warning> {
+    } <warning descr="catch branch identical to 'E2' branch">catch (E1 e)</warning> {
+    }
+  }
 }
@@ -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");