Java: Reverted the name and the functionality of ConfusingElseInspection. Changed default settings, corrected text messages. Added tests. (IDEA-163393)

2026-09-27 10:03:11 +07:00 · 2016-12-08 14:59:11 +03:00
parent 44c4191d91
commit 10dd5478f6
9 changed files with 168 additions and 15 deletions
@@ -15,7 +15,7 @@
 */
 package com.intellij.codeInsight.daemon.quickFix;

-import com.siyeh.ig.controlflow.RedundantElseInspection;
+import com.siyeh.ig.controlflow.ConfusingElseInspection;

 /**
 * User: anna
@@ -25,7 +25,7 @@ public class RemoveRedundantElseFixTest extends LightQuickFixParameterizedTestCa
  @Override
  protected void setUp() throws Exception {
    super.setUp();
-    enableInspectionTool(new RedundantElseInspection());
+    enableInspectionTool(new ConfusingElseInspection());
  }

  public void test() throws Exception { doAllTests(); }
@@ -89,7 +89,7 @@ com.siyeh.ig.classmetrics.FieldCountInspection
 com.siyeh.ig.classmetrics.MethodCountInspection
 com.siyeh.ig.cloneable.CloneableImplementsCloneInspection
 com.siyeh.ig.controlflow.ConditionalExpressionInspection
-com.siyeh.ig.controlflow.RedundantElseInspection
+com.siyeh.ig.controlflow.ConfusingElseInspection
 com.siyeh.ig.controlflow.DuplicateConditionInspection
 com.siyeh.ig.controlflow.EnumSwitchStatementWhichMissesCasesInspection
 com.siyeh.ig.controlflow.ForLoopReplaceableByWhileInspection
@@ -606,9 +606,9 @@
                     key="conditional.expression.with.identical.branches.display.name" groupBundle="messages.InspectionsBundle"
                     groupKey="group.names.control.flow.issues" enabledByDefault="false" level="WARNING"
                     implementationClass="com.siyeh.ig.controlflow.ConditionalExpressionWithIdenticalBranchesInspection" cleanupTool="true"/>
-    <localInspection groupPath="Java" language="JAVA" suppressId="ConfusingElseBranch" shortName="RedundantElse" bundle="com.siyeh.InspectionGadgetsBundle"
+    <localInspection groupPath="Java" language="JAVA" suppressId="ConfusingElseBranch" shortName="ConfusingElse" bundle="com.siyeh.InspectionGadgetsBundle"
                     key="redundant.else.display.name" groupBundle="messages.InspectionsBundle" groupKey="group.names.control.flow.issues"
-                     enabledByDefault="true" level="INFORMATION" implementationClass="com.siyeh.ig.controlflow.RedundantElseInspection"/>
+                     enabledByDefault="true" level="INFORMATION" implementationClass="com.siyeh.ig.controlflow.ConfusingElseInspection"/>
    <localInspection groupPath="Java" language="JAVA" shortName="ConstantConditionalExpression" bundle="com.siyeh.InspectionGadgetsBundle"
                     key="constant.conditional.expression.display.name" groupBundle="messages.InspectionsBundle"
                     groupKey="group.names.control.flow.issues" enabledByDefault="true" level="WARNING"
@@ -1835,7 +1835,7 @@ if.can.be.switch.null.safe.option=Only suggest on null-safe expressions
 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
-confusing.else.option=<html>Also report when there are no more statements after the 'if' statement</html>
+confusing.else.option=Report when there are no more statements after the 'if' statement
 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
 html.tag.can.be.javadoc.tag.quickfix=Replace with '{@code ...}'
@@ -16,8 +16,10 @@
 package com.siyeh.ig.controlflow;

 import com.intellij.codeInspection.ProblemDescriptor;
+import com.intellij.codeInspection.ui.SingleCheckboxOptionsPanel;
 import com.intellij.openapi.project.Project;
 import com.intellij.psi.*;
+import com.intellij.psi.util.PsiTreeUtil;
 import com.intellij.util.IncorrectOperationException;
 import com.siyeh.InspectionGadgetsBundle;
 import com.siyeh.ig.BaseInspection;
@@ -28,7 +30,12 @@ import org.intellij.lang.annotations.Pattern;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

-public class RedundantElseInspection extends BaseInspection {
+import javax.swing.*;
+
+public class ConfusingElseInspection extends BaseInspection {
+
+  @SuppressWarnings({"PublicField"})
+  public boolean reportWhenNoStatementFollow = true;

  @Pattern(VALID_ID_PATTERN)
  @Override
@@ -49,9 +56,14 @@ public class RedundantElseInspection extends BaseInspection {
    return InspectionGadgetsBundle.message("redundant.else.problem.descriptor");
  }

+  @Override
+  public JComponent createOptionsPanel() {
+    return new SingleCheckboxOptionsPanel(InspectionGadgetsBundle.message("confusing.else.option"), this, "reportWhenNoStatementFollow");
+  }
+
  @Override
  public BaseInspectionVisitor buildVisitor() {
-    return new RedundantElseVisitor();
+    return new ConfusingElseVisitor();
  }

  @Override
@@ -100,7 +112,7 @@ public class RedundantElseInspection extends BaseInspection {
    }
  }

-  private static class RedundantElseVisitor extends BaseInspectionVisitor {
+  private class ConfusingElseVisitor extends BaseInspectionVisitor {

    @Override
    public void visitIfStatement(@NotNull PsiIfStatement statement) {
@@ -116,6 +128,17 @@ public class RedundantElseInspection extends BaseInspection {
      if (ControlFlowUtils.statementMayCompleteNormally(thenBranch)) {
        return;
      }
+      if (!reportWhenNoStatementFollow) {
+        final PsiStatement nextStatement = getNextStatement(statement);
+        if (nextStatement == null) {
+          return;
+        }
+        if (!ControlFlowUtils.statementMayCompleteNormally(elseBranch)) {
+          return;
+          // protecting against an edge case where both branches return
+          // and are followed by a case label
+        }
+      }
      final PsiElement elseToken = statement.getElseElement();
      if (elseToken == null) {
        return;
@@ -126,7 +149,7 @@ public class RedundantElseInspection extends BaseInspection {
      registerError(elseToken);
    }

-    private static boolean parentCompletesNormally(PsiElement element) {
+    private boolean parentCompletesNormally(PsiElement element) {
      PsiElement parent = element.getParent();
      while (parent instanceof PsiIfStatement) {
        final PsiIfStatement ifStatement = (PsiIfStatement)parent;
@@ -143,5 +166,21 @@ public class RedundantElseInspection extends BaseInspection {
      }
      return !(parent instanceof PsiCodeBlock);
    }
+
+    @Nullable
+    private PsiStatement getNextStatement(PsiIfStatement statement) {
+      while (true) {
+        final PsiElement parent = statement.getParent();
+        if (parent instanceof PsiIfStatement) {
+          final PsiIfStatement parentIfStatement = (PsiIfStatement)parent;
+          final PsiStatement elseBranch = parentIfStatement.getElseBranch();
+          if (elseBranch == statement) {
+            statement = parentIfStatement;
+            continue;
+          }
+        }
+        return PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class);
+      }
+    }
  }
 }
@@ -1,12 +1,12 @@
 package com.siyeh.igtest.controlflow.confusing_else;

-public class RedundantElse {
+public class ConfusingElse {

    public static void main(String[] args) {
        if (foo()) {
            return;
        } <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> {
-            System.out.println("RedundantElseInspection.main");
+            System.out.println("ConfusingElseInspection.main");
        }
        bar();
    }
@@ -52,6 +52,14 @@ public class RedundantElse {
        return o;
    }

+    void lastElse(int i) {
+        if (i == 1) {
+            return;
+        } <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> {
+            System.out.println("i = " + i);
+        }
+    }
+
    void elseIf(int i) {
        if (i == 1) {
            return;
@@ -0,0 +1,93 @@
+package com.siyeh.igtest.controlflow.confusing_else;
+
+public class ConfusingLastElse {
+
+    public static void main(String[] args) {
+        if (foo()) {
+            return;
+        } <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> {
+            System.out.println("ConfusingElseInspection.main");
+        }
+        bar();
+    }
+
+    private static void bar() {
+    }
+
+    private static boolean foo() {
+        return true;
+    }
+
+    void two(boolean b) {
+        if (foo()) {
+          System.out.println(0);
+        } else if (b) {
+            return;
+        } else {
+          System.out.println(1);
+        }
+        bar();
+    }
+
+    void three(boolean b) {
+        switch (3) {
+            case 2:
+                if (foo()) {
+                    return;
+                } else {
+                    return;
+                }
+            case 3:
+        }
+    }
+
+    public int foo(int o) {
+        if (o == 1) {
+            o = 2;
+        } else if (o == 2) {
+            return 1;
+        } else {
+            o = 4;
+        }
+        return o;
+    }
+
+    void lastElse(int i) {
+        if (i == 1) {
+            return;
+        } else {
+          System.out.println("i = " + i);
+        }
+    }
+
+    void elseIf(int i) {
+        if (i == 1) {
+            return;
+        } <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> if (i == 3) {
+            System.out.println("i = " + i);
+        }
+        System.out.println();
+    }
+
+    void elseIfChain(int i) {
+        while (true) {
+            if (i == 0) {
+                System.exit(i);
+            }
+            else if (i == 1) {
+                throw new RuntimeException();
+            }
+            else if (i == 2) {
+                return;
+            }
+            else if (i == 3) {
+                break;
+            }
+            else if (i == 4) {
+                continue;
+            } else {
+              System.out.println(i);
+            }
+        }
+    }
+}
@@ -22,15 +22,28 @@ import org.jetbrains.annotations.Nullable;
 /**
 * @author Bas Leijdekkers
 */
-public class RedundantElseInspectionTest extends LightInspectionTestCase {
+public class ConfusingElseInspectionTest extends LightInspectionTestCase {

-  public void testRedundantElse() {
+  private ConfusingElseInspection myInspection = new ConfusingElseInspection();
+
+  public void testConfusingElse() {
    doTest();
  }

+  public void testConfusingLastElse() {
+    boolean oldValue = myInspection.reportWhenNoStatementFollow;
+    myInspection.reportWhenNoStatementFollow = false;
+    try {
+      doTest();
+    }
+    finally {
+      myInspection.reportWhenNoStatementFollow = oldValue;
+    }
+  }
+
  @Nullable
  @Override
  protected InspectionProfileEntry getInspection() {
-    return new RedundantElseInspection();
+    return myInspection;
  }
 }