Java: Merged ConfusingElseInspection and RemoveRedundantElseAction into RedundantElseInspection, made it an INFORMATION-level inspection (IDEA-162191)

This commit is contained in:
Pavel Dolgov
2016-12-06 17:01:18 +03:00
parent 8aca86cf3e
commit 05744d1ffd
20 changed files with 175 additions and 275 deletions

View File

@@ -1,107 +0,0 @@
/*
* Copyright 2000-2009 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.intellij.codeInsight.daemon.impl.quickfix;
import com.intellij.codeInsight.daemon.QuickFixBundle;
import com.intellij.codeInsight.intention.PsiElementBaseIntentionAction;
import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.project.Project;
import com.intellij.psi.*;
import com.intellij.psi.controlFlow.*;
import com.intellij.psi.util.PsiTreeUtil;
import com.intellij.util.BitUtil;
import com.intellij.util.IncorrectOperationException;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* @author ven
*/
public class RemoveRedundantElseAction extends PsiElementBaseIntentionAction {
private static final Logger LOG = Logger.getInstance("#com.intellij.codeInsight.daemon.impl.quickfix.RemoveRedundantElseAction");
@Override
@NotNull
public String getText() {
return QuickFixBundle.message("remove.redundant.else.fix");
}
@Override
@NotNull
public String getFamilyName() {
return QuickFixBundle.message("remove.redundant.else.fix");
}
@Override
public boolean isAvailable(@NotNull Project project, Editor editor, @NotNull PsiElement element) {
if (element instanceof PsiKeyword &&
element.getParent() instanceof PsiIfStatement &&
PsiKeyword.ELSE.equals(element.getText())) {
PsiIfStatement ifStatement = (PsiIfStatement)element.getParent();
if (ifStatement.getElseBranch() == null) return false;
PsiStatement thenBranch = ifStatement.getThenBranch();
if (thenBranch == null) return false;
PsiElement block = PsiTreeUtil.getParentOfType(ifStatement, PsiCodeBlock.class);
if (block != null) {
while (cantCompleteNormally(thenBranch, block)) {
thenBranch = getPrevThenBranch(thenBranch);
if (thenBranch == null) return true;
}
return false;
}
}
return false;
}
@Nullable
private static PsiStatement getPrevThenBranch(@NotNull PsiElement thenBranch) {
final PsiElement ifStatement = thenBranch.getParent();
final PsiElement parent = ifStatement.getParent();
if (parent instanceof PsiIfStatement && ((PsiIfStatement)parent).getElseBranch() == ifStatement) {
return ((PsiIfStatement)parent).getThenBranch();
}
return null;
}
private static boolean cantCompleteNormally(@NotNull PsiStatement thenBranch, PsiElement block) {
try {
ControlFlow controlFlow = ControlFlowFactory.getInstance(thenBranch.getProject()).getControlFlow(block, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance());
int startOffset = controlFlow.getStartOffset(thenBranch);
int endOffset = controlFlow.getEndOffset(thenBranch);
return startOffset != -1 && endOffset != -1 && !BitUtil.isSet(ControlFlowUtil.getCompletionReasons(controlFlow, startOffset, endOffset), ControlFlowUtil.NORMAL_COMPLETION_REASON);
}
catch (AnalysisCanceledException e) {
return false;
}
}
@Override
public void invoke(@NotNull Project project, Editor editor, @NotNull PsiElement element) throws IncorrectOperationException {
PsiIfStatement ifStatement = (PsiIfStatement)element.getParent();
LOG.assertTrue(ifStatement != null && ifStatement.getElseBranch() != null);
PsiStatement elseBranch = ifStatement.getElseBranch();
if (elseBranch instanceof PsiBlockStatement) {
PsiElement[] statements = ((PsiBlockStatement)elseBranch).getCodeBlock().getStatements();
if (statements.length > 0) {
ifStatement.getParent().addRangeAfter(statements[0], statements[statements.length-1], ifStatement);
}
} else {
ifStatement.getParent().addAfter(elseBranch, ifStatement);
}
ifStatement.getElseBranch().delete();
}
}

View File

@@ -0,0 +1,10 @@
// "Remove redundant 'else'" "true"
class T {
static void foo(boolean something) {
if (something) {
return;
} // a
System.out.println(); // b
// c
}
}

View File

@@ -0,0 +1,13 @@
// "Remove redundant 'else'" "true"
class T {
void three(boolean b) {
switch (3) {
case 2:
if (foo()) {
return;
}
return;
case 3:
}
}
}

View File

@@ -0,0 +1,12 @@
// "Remove redundant 'else'" "true"
class T {
static void foo(boolean something) {
if (something) {
return;
}
else<caret> { // a
System.out.println(); // b
// c
}
}
}

View File

@@ -0,0 +1,15 @@
// "Remove redundant 'else'" "true"
class T {
void three(boolean b) {
switch (3) {
case 2:
if (foo()) {
return;
}
else<caret> {
return;
}
case 3:
}
}
}

View File

@@ -15,11 +15,18 @@
*/
package com.intellij.codeInsight.daemon.quickFix;
import com.siyeh.ig.controlflow.RedundantElseInspection;
/**
* User: anna
* Date: Aug 30, 2010
*/
public class RemoveRedundantElseActionTest extends LightQuickFixParameterizedTestCase {
public class RemoveRedundantElseFixTest extends LightQuickFixParameterizedTestCase {
@Override
protected void setUp() throws Exception {
super.setUp();
enableInspectionTool(new RedundantElseInspection());
}
public void test() throws Exception { doAllTests(); }

View File

@@ -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.ConfusingElseInspection
com.siyeh.ig.controlflow.RedundantElseInspection
com.siyeh.ig.controlflow.DuplicateConditionInspection
com.siyeh.ig.controlflow.EnumSwitchStatementWhichMissesCasesInspection
com.siyeh.ig.controlflow.ForLoopReplaceableByWhileInspection

View File

@@ -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="ConfusingElse" bundle="com.siyeh.InspectionGadgetsBundle"
key="confusing.else.display.name" groupBundle="messages.InspectionsBundle" groupKey="group.names.control.flow.issues"
enabledByDefault="false" level="WARNING" implementationClass="com.siyeh.ig.controlflow.ConfusingElseInspection"/>
<localInspection groupPath="Java" language="JAVA" suppressId="ConfusingElseBranch" shortName="RedundantElse" 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"/>
<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"

View File

@@ -788,7 +788,7 @@ default.not.last.case.in.switch.display.name='default' not last case in 'switch'
nested.synchronized.statement.display.name=Nested 'synchronized' statement
constant.conditional.expression.display.name=Constant conditional expression
unused.catch.parameter.display.name=Unused 'catch' parameter
confusing.else.display.name=Confusing 'else' branch
redundant.else.display.name=Redundant 'else'
public.field.accessed.in.synchronized.context.display.name=Non-private field accessed in synchronized context
string.replaceable.by.string.buffer.display.name=Non-constant String should be StringBuilder
junit.test.class.naming.convention.display.name=JUnit test class naming convention
@@ -878,7 +878,7 @@ octal.literal.problem.descriptor=Octal integer <code>#ref</code> #loc
implicit.call.to.super.problem.descriptor=Implicit call to 'super()' #loc
negated.if.else.problem.descriptor=<code>#ref</code> statement with negated condition #loc
negated.conditional.problem.descriptor=Conditional expression with negated condition #loc
confusing.else.problem.descriptor=<code>#ref</code> branch may be unwrapped, as the 'if' branch never completes #loc
redundant.else.problem.descriptor=<code>#ref</code> branch may be unwrapped, as the 'if' branch never completes normally #loc
switch.statement.with.confusing.declaration.problem.descriptor=Local variable <code>#ref</code> declared in one 'switch' branch and used in another #loc
raw.use.of.parameterized.type.problem.descriptor=Raw use of parameterized class <code>#ref</code> #loc
final.class.problem.descriptor=Class declared <code>#ref</code> #loc
@@ -1121,7 +1121,7 @@ boolean.method.name.must.start.with.question.table.column.name=Boolean method na
conditional.expression.with.identical.branches.collapse.quickfix=Collapse conditional expression
conditional.expression.with.identical.branches.push.inside.quickfix=Push conditional inside expression
conditional.expression.with.identical.branches.collapse.quickfix.family=Conditional expression can be simplified
confusing.else.unwrap.quickfix=Remove redundant 'else'
redundant.else.unwrap.quickfix=Remove redundant 'else'
constant.conditional.expression.problem.descriptor=<code>#ref</code> can be simplified to ''{0}'' #loc
constant.conditional.expression.simplify.quickfix=Simplify
enum.switch.statement.which.misses.cases.problem.descriptor=<code>#ref</code> statement on enumerated type ''{0}'' misses cases #loc

View File

@@ -16,26 +16,21 @@
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;
import com.siyeh.ig.BaseInspectionVisitor;
import com.siyeh.ig.InspectionGadgetsFix;
import com.siyeh.ig.psiutils.ControlFlowUtils;
import org.intellij.lang.annotations.Pattern;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import javax.swing.*;
public class ConfusingElseInspection extends BaseInspection {
@SuppressWarnings({"PublicField"})
public boolean reportWhenNoStatementFollow = false;
public class RedundantElseInspection extends BaseInspection {
@Pattern(VALID_ID_PATTERN)
@Override
@NotNull
public String getID() {
@@ -45,37 +40,32 @@ public class ConfusingElseInspection extends BaseInspection {
@Override
@NotNull
public String getDisplayName() {
return InspectionGadgetsBundle.message("confusing.else.display.name");
return InspectionGadgetsBundle.message("redundant.else.display.name");
}
@Override
@NotNull
protected String buildErrorString(Object... infos) {
return InspectionGadgetsBundle.message("confusing.else.problem.descriptor");
}
@Override
public JComponent createOptionsPanel() {
return new SingleCheckboxOptionsPanel(InspectionGadgetsBundle.message("confusing.else.option"), this, "reportWhenNoStatementFollow");
return InspectionGadgetsBundle.message("redundant.else.problem.descriptor");
}
@Override
public BaseInspectionVisitor buildVisitor() {
return new ConfusingElseVisitor();
return new RedundantElseVisitor();
}
@Override
@Nullable
protected InspectionGadgetsFix buildFix(Object... infos) {
return new ConfusingElseFix();
return new RemoveRedundantElseFix();
}
private static class ConfusingElseFix extends InspectionGadgetsFix {
private static class RemoveRedundantElseFix extends InspectionGadgetsFix {
@Override
@NotNull
public String getFamilyName() {
return InspectionGadgetsBundle.message("confusing.else.unwrap.quickfix");
return InspectionGadgetsBundle.message("redundant.else.unwrap.quickfix");
}
@Override
@@ -110,7 +100,7 @@ public class ConfusingElseInspection extends BaseInspection {
}
}
private class ConfusingElseVisitor extends BaseInspectionVisitor {
private static class RedundantElseVisitor extends BaseInspectionVisitor {
@Override
public void visitIfStatement(@NotNull PsiIfStatement statement) {
@@ -126,17 +116,6 @@ public class ConfusingElseInspection 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;
@@ -147,7 +126,7 @@ public class ConfusingElseInspection extends BaseInspection {
registerError(elseToken);
}
private boolean parentCompletesNormally(PsiElement element) {
private static boolean parentCompletesNormally(PsiElement element) {
PsiElement parent = element.getParent();
while (parent instanceof PsiIfStatement) {
final PsiIfStatement ifStatement = (PsiIfStatement)parent;
@@ -164,21 +143,5 @@ public class ConfusingElseInspection 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);
}
}
}
}

View File

@@ -1,16 +0,0 @@
<html>
<body>
Reports confusing <b>else</b> branches. <b>else</b> branches are confusing
when the <b>if</b> statement is followed by other statements and the <b>if</b> branch
cannot complete normally, for example because it ends with a <b>return</b> statement. In these
cases the statements in the <b>else</b> can be moved after the <b>if</b> statement and
the <b>else</b> branch removed.
<!-- tooltip end -->
<p>
Use the checkbox below to also report <b>else</b> branches of <b>if</b> statements whose
<b>if</b> branch cannot complete normally and which are not followed by more statements,
similar to the Redundant Else intention.
<p>
</body>
</html>

View File

@@ -0,0 +1,11 @@
<html>
<body>
Reports redundant <b>else</b> keywords in <b>if</b>&mdash;<b>else</b> statements and statement chains.
An <b>else</b> keyword is redundant when all previous <b>if</b> branches in the chain don't complete normally
because they end with <b>return</b>, <b>throw</b>, <b>break</b>, or <b>continue</b> statement.
<p>
In these cases the statements from the <b>else</b> branch can be placed after the <b>if</b> statement and
the <b>else</b> keyword can be removed.
<!-- tooltip end -->
</body>
</html>

View File

@@ -1,63 +0,0 @@
package com.siyeh.igtest.controlflow.confusing_else;
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">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 elseIf(int i) {
if (i == 1) {
return;
} <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes">else</warning> if (i == 3) {
System.out.println("i = " + i);
}
System.out.println();
}
}

View File

@@ -0,0 +1,85 @@
package com.siyeh.igtest.controlflow.confusing_else;
public class RedundantElse {
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");
}
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;
} <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> {
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 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);
}
<warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> if (i == 1) {
throw new RuntimeException();
}
<warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> if (i == 2) {
return;
}
<warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> if (i == 3) {
break;
}
<warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> if (i == 4) {
continue;
} <warning descr="'else' branch may be unwrapped, as the 'if' branch never completes normally">else</warning> {
System.out.println(i);
}
}
}
}

View File

@@ -22,15 +22,15 @@ import org.jetbrains.annotations.Nullable;
/**
* @author Bas Leijdekkers
*/
public class ConfusingElseInspectionTest extends LightInspectionTestCase {
public class RedundantElseInspectionTest extends LightInspectionTestCase {
public void testConfusingElse() {
public void testRedundantElse() {
doTest();
}
@Nullable
@Override
protected InspectionProfileEntry getInspection() {
return new ConfusingElseInspection();
return new RedundantElseInspection();
}
}

View File

@@ -1,8 +0,0 @@
public class X {
void f(int i) {
if (i==0) {
return;
}
int j = 0;
}
}

View File

@@ -1,10 +0,0 @@
public class X {
void f(int i) {
if (i==0) {
return;
}
<spot>else</spot> {
int j = 0;
}
}
}

View File

@@ -1,7 +0,0 @@
<html>
<body>
This intention detaches <b>else</b> clause from the <b>if</b> statement,
if corresponding <b>then</b> clause never completes normally.
</body>
</html>

View File

@@ -149,7 +149,6 @@ negation.broader.scope.text=Change to ''!({0})''
optimize.imports.fix=Optimize imports
remove.qualifier.fix=Remove qualifier
remove.redundant.else.fix=Remove redundant 'else'
remove.unused.parameter.family=Remove unused parameter
remove.unused.parameter.text=Remove parameter ''{0}''
remove.unused.variable.family=Remove unused variable

View File

@@ -912,10 +912,6 @@
<className>com.intellij.codeInsight.intention.impl.ExtractIfConditionAction</className>
<category>Java/Control Flow</category>
</intentionAction>
<intentionAction>
<className>com.intellij.codeInsight.daemon.impl.quickfix.RemoveRedundantElseAction</className>
<category>Java/Control Flow</category>
</intentionAction>
<intentionAction>
<className>com.intellij.codeInsight.intention.impl.AddNotNullAnnotationIntention</className>
<category>Java/Annotations</category>