IDEA-196805 Warn about switch statement with single 'default'

This commit is contained in:
Tagir Valeev
2018-09-25 14:19:43 +07:00
parent e02761f313
commit 9530731ea5
26 changed files with 488 additions and 39 deletions

View File

@@ -902,6 +902,11 @@
groupKey="group.names.verbose.or.redundant.code.constructs" groupBundle="messages.InspectionsBundle"
enabledByDefault="true" level="WEAK WARNING"
implementationClass="com.intellij.codeInspection.duplicateExpressions.DuplicateExpressionsInspection" />
<localInspection groupPath="Java" language="JAVA" shortName="SwitchStatementWithSingleDefault"
key="inspection.switch.statement.with.single.default.display.name" bundle="messages.InspectionsBundle"
groupKey="group.names.verbose.or.redundant.code.constructs" groupBundle="messages.InspectionsBundle"
enabledByDefault="true" level="WARNING"
implementationClass="com.siyeh.ig.redundancy.SwitchStatementWithSingleDefaultInspection" />
<globalInspection groupPath="Java" language="JAVA" shortName="EmptyMethod" displayName="Empty method" groupKey="group.names.declaration.redundancy" enabledByDefault="true" groupBundle="messages.InspectionsBundle"
level="WARNING" implementationClass="com.intellij.codeInspection.emptyMethod.EmptyMethodInspection"/>

View File

@@ -13,7 +13,6 @@ import com.intellij.openapi.project.Project;
import com.intellij.openapi.util.Ref;
import com.intellij.openapi.util.text.StringUtil;
import com.intellij.psi.*;
import com.intellij.psi.codeStyle.CodeStyleManager;
import com.intellij.psi.controlFlow.AnalysisCanceledException;
import com.intellij.psi.controlFlow.ControlFlowUtil;
import com.intellij.psi.tree.IElementType;
@@ -222,10 +221,15 @@ public class SimplifyBooleanExpressionFix extends LocalQuickFixOnPsiElement {
}
if (parent instanceof PsiCodeBlock) {
if (statement instanceof PsiBlockStatement &&
!BlockUtils.containsConflictingDeclarations(((PsiBlockStatement)statement).getCodeBlock(), (PsiCodeBlock)parent)) {
inlineBlockStatements(orig, (PsiBlockStatement)statement, parent);
return;
if (statement instanceof PsiBlockStatement) {
// See IDEADEV-24277
// Code block can only be inlined into another (parent) code block.
// Code blocks, which are if or loop statement branches should not be inlined.
PsiCodeBlock codeBlock = ((PsiBlockStatement)statement).getCodeBlock();
if (!BlockUtils.containsConflictingDeclarations(codeBlock, (PsiCodeBlock)parent)) {
BlockUtils.inlineCodeBlock(orig, codeBlock);
return;
}
}
if (hasConflictingDeclarations(statement, (PsiCodeBlock)parent)) {
orig.replace(wrapWithCodeBlock(statement));
@@ -247,37 +251,11 @@ public class SimplifyBooleanExpressionFix extends LocalQuickFixOnPsiElement {
}
private static PsiBlockStatement wrapWithCodeBlock(PsiStatement replacement) {
PsiBlockStatement newBlock = createBlockStatement(replacement.getProject());
PsiBlockStatement newBlock = BlockUtils.createBlockStatement(replacement.getProject());
newBlock.getCodeBlock().add(replacement);
return newBlock;
}
private static PsiBlockStatement createBlockStatement(Project project) {
return (PsiBlockStatement)JavaPsiFacade.getElementFactory(project).createStatementFromText("{}", null);
}
private static void inlineBlockStatements(@NotNull PsiStatement orig, @NotNull PsiBlockStatement statement, PsiElement parent) {
// See IDEADEV-24277
// Code block can only be inlined into another (parent) code block.
// Code blocks, which are if or loop statement branches should not be inlined.
PsiCodeBlock codeBlock = statement.getCodeBlock();
PsiJavaToken lBrace = codeBlock.getLBrace();
PsiJavaToken rBrace = codeBlock.getRBrace();
if (lBrace == null || rBrace == null) return;
final PsiElement[] children = codeBlock.getChildren();
if (children.length > 2) {
final PsiElement added =
parent.addRangeBefore(
children[1],
children[children.length - 2],
orig);
final CodeStyleManager codeStyleManager = CodeStyleManager.getInstance(orig.getManager());
codeStyleManager.reformat(added);
}
orig.delete();
}
private static boolean blockAlwaysReturns(@Nullable PsiStatement statement) {
if (statement == null) return false;
try {
@@ -439,7 +417,7 @@ public class SimplifyBooleanExpressionFix extends LocalQuickFixOnPsiElement {
if (expressions.isEmpty()) {
resultExpression = negate ? trueExpression : falseExpression;
} else {
String simplifiedText = StringUtil.join(expressions, expression1 -> expression1.getText(), " ^ ");
String simplifiedText = StringUtil.join(expressions, PsiElement::getText, " ^ ");
if (negate) {
if (expressions.size() > 1) {
simplifiedText = "!(" + simplifiedText + ")";

View File

@@ -0,0 +1,7 @@
<html>
<body>
Reports switch statements which have only <code>default</code> branch and can be replaced with default branch content.
<!-- tooltip end -->
<p><small>New in 2018.3</small></p>
</body>
</html>

View File

@@ -3,6 +3,7 @@ package com.intellij.codeInsight;
import com.intellij.openapi.project.Project;
import com.intellij.psi.*;
import com.intellij.psi.codeStyle.CodeStyleManager;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.SmartList;
import org.jetbrains.annotations.NotNull;
@@ -197,4 +198,26 @@ public class BlockUtils {
}
return false;
}
public static void inlineCodeBlock(@NotNull PsiStatement orig, PsiCodeBlock codeBlock) {
PsiJavaToken lBrace = codeBlock.getLBrace();
PsiJavaToken rBrace = codeBlock.getRBrace();
if (lBrace == null || rBrace == null) return;
final PsiElement[] children = codeBlock.getChildren();
if (children.length > 2) {
final PsiElement added =
orig.getParent().addRangeBefore(
children[1],
children[children.length - 2],
orig);
final CodeStyleManager codeStyleManager = CodeStyleManager.getInstance(orig.getManager());
codeStyleManager.reformat(added);
}
orig.delete();
}
public static PsiBlockStatement createBlockStatement(Project project) {
return (PsiBlockStatement)JavaPsiFacade.getElementFactory(project).createStatementFromText("{}", null);
}
}

View File

@@ -0,0 +1,10 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
if(c == 'a') {
System.out.println("foo");
} else {
}
System.out.println("oops");
}
}

View File

@@ -0,0 +1,9 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
if(c == 'a') {
System.out.println("foo");
}
System.out.println("oops");
}
}

View File

@@ -0,0 +1,11 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
if(c == 'a') {
System.out.println("foo");
return "";
}
System.out.println("oops");
return "";
}
}

View File

@@ -0,0 +1,8 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
if(c == 'a') {
System.out.println("foo");
}
}
}

View File

@@ -0,0 +1,13 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
for(int i=0; i<10; i++)
if(c == 'a') {
System.out.println("foo");
continue;
}
System.out.println("bar");
System.out.println("oops");
return "";
}
}

View File

@@ -0,0 +1,9 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
{
char a, b = c;
}
int b;
}
}

View File

@@ -0,0 +1,6 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
return "foo";
}
}

View File

@@ -0,0 +1,13 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
if(c == 'a') {
System.out.println("foo");
break;
} else break;
}
System.out.println("oops");
}
}

View File

@@ -0,0 +1,14 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
if(c == 'a') {
System.out.println("foo");
break;
}
break;
}
System.out.println("oops");
}
}

View File

@@ -0,0 +1,15 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
if(c == 'a') {
System.out.println("foo");
break;
}
System.out.println("oops");
break;
}
return "";
}
}

View File

@@ -0,0 +1,12 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
if(c == 'a') {
System.out.println("foo");
}
break;
}
}
}

View File

@@ -0,0 +1,16 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
for(int i=0; i<10; i++)
s<caret>witch (c) {
default:
if(c == 'a') {
System.out.println("foo");
break;
}
System.out.println("bar");
}
System.out.println("oops");
return "";
}
}

View File

@@ -0,0 +1,17 @@
// "Unwrap 'switch' statement" "false"
class X {
String test(char c) {
for(int i=0; i<10; i++) {
s<caret>witch (c){
default:
if (c == 'a') {
System.out.println("foo");
break;
}
System.out.println("bar");
}
System.out.println("oops");
}
return "";
}
}

View File

@@ -0,0 +1,10 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
char a, b = c;
}
int b;
}
}

View File

@@ -0,0 +1,9 @@
// "Unwrap 'switch' statement" "true"
class X {
String test(char c) {
s<caret>witch (c) {
default:
return "foo";
}
}
}

View File

@@ -1012,4 +1012,7 @@ inspection.duplicate.expressions.introduce.variable.fix.name=Introduce variable
inspection.duplicate.expressions.reuse.variable.fix.family.name=Reuse variable
inspection.duplicate.expressions.reuse.variable.fix.name=Reuse variable ''{0}'' for ''{1}''
inspection.duplicate.expressions.replace.other.occurrences.fix.family.name=Replace with variable other occurrences of expression
inspection.duplicate.expressions.replace.other.occurrences.fix.name=Replace with ''{0}'' other occurrences of ''{1}''
inspection.duplicate.expressions.replace.other.occurrences.fix.name=Replace with ''{0}'' other occurrences of ''{1}''
inspection.switch.statement.with.single.default.display.name='switch' statement with 'default' case only
inspection.switch.statement.with.single.default.message=Switch statement has only 'default' case

View File

@@ -0,0 +1,148 @@
// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
package com.siyeh.ig.psiutils;
import com.intellij.psi.*;
import com.intellij.psi.util.PsiTreeUtil;
import com.siyeh.ig.fixes.DeleteUnnecessaryStatementFix;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.ArrayList;
import java.util.List;
/**
* A class which converts switch statement breaks before switch statement
* is unwrapped.
*/
public class BreakConverter {
private final PsiSwitchStatement mySwitchStatement;
private final String myReplacement;
public BreakConverter(PsiSwitchStatement switchStatement, String replacement) {
mySwitchStatement = switchStatement;
myReplacement = replacement;
}
public void process(boolean removeRemovable) {
List<PsiBreakStatement> breaks = collectBreaks();
for (PsiBreakStatement breakStatement : breaks) {
if (isRemovable(mySwitchStatement, breakStatement)) {
if (removeRemovable) {
DeleteUnnecessaryStatementFix.deleteUnnecessaryStatement(breakStatement);
}
} else {
assert myReplacement != null;
new CommentTracker().replaceAndRestoreComments(breakStatement, myReplacement);
}
}
}
@NotNull
private List<PsiBreakStatement> collectBreaks() {
List<PsiBreakStatement> breaks = new ArrayList<>();
mySwitchStatement.accept(new JavaRecursiveElementWalkingVisitor() {
@Override
public void visitBreakStatement(PsiBreakStatement statement) {
super.visitBreakStatement(statement);
if (statement.findExitedStatement() == mySwitchStatement) {
breaks.add(statement);
}
}
@Override
public void visitExpression(PsiExpression expression) {
// Going down into any expression seems redundant
}
@Override
public void visitClass(PsiClass aClass) {}
});
return breaks;
}
@Nullable
private static String getReplacement(PsiStatement statement) {
PsiElement parent = statement.getParent();
if (parent instanceof PsiIfStatement || parent instanceof PsiLabeledStatement) {
return getReplacement((PsiStatement)parent);
}
PsiStatement nextStatement = PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class);
if (nextStatement != null) {
if (nextStatement instanceof PsiContinueStatement ||
nextStatement instanceof PsiBreakStatement ||
nextStatement instanceof PsiReturnStatement ||
nextStatement instanceof PsiThrowStatement) {
return nextStatement.getText();
}
return null;
}
if (parent == null) return null;
if (parent instanceof PsiLoopStatement) {
return "continue;";
}
if (parent instanceof PsiCodeBlock) {
PsiElement grandParent = parent.getParent();
if (grandParent instanceof PsiMethod && PsiType.VOID.equals(((PsiMethod)grandParent).getReturnType()) ||
grandParent instanceof PsiLambdaExpression &&
PsiType.VOID.equals(LambdaUtil.getFunctionalInterfaceReturnType((PsiFunctionalExpression)grandParent))) {
return "return;";
}
if (grandParent instanceof PsiBlockStatement) {
return getReplacement((PsiStatement)grandParent);
}
}
return null;
}
private static boolean isRemovable(PsiSwitchStatement switchStatement, PsiStatement statement) {
PsiElement parent = statement.getParent();
if (parent instanceof PsiIfStatement || parent instanceof PsiLabeledStatement) {
return isRemovable(switchStatement, (PsiStatement)parent);
}
PsiStatement nextStatement = PsiTreeUtil.getNextSiblingOfType(statement, PsiStatement.class);
if (nextStatement != null) {
return nextStatement instanceof PsiBreakStatement &&
((PsiBreakStatement)nextStatement).findExitedStatement() == switchStatement;
}
if (parent == null) return false;
if (parent instanceof PsiCodeBlock) {
PsiElement grandParent = parent.getParent();
if (grandParent instanceof PsiBlockStatement) {
return isRemovable(switchStatement, (PsiStatement)grandParent);
}
return grandParent == switchStatement;
}
return false;
}
@Nullable
public static BreakConverter from(PsiSwitchStatement switchStatement) {
String replacement = getReplacement(switchStatement);
if (replacement == null) {
class Visitor extends JavaRecursiveElementWalkingVisitor {
boolean hasNonRemovableBreak;
@Override
public void visitBreakStatement(PsiBreakStatement statement) {
super.visitBreakStatement(statement);
if (statement.findExitedStatement() == switchStatement && !isRemovable(switchStatement, statement)) {
hasNonRemovableBreak = true;
stopWalking();
}
}
@Override
public void visitExpression(PsiExpression expression) {
// Going down into any expression seems redundant
}
@Override
public void visitClass(PsiClass aClass) {}
}
Visitor visitor = new Visitor();
switchStatement.accept(visitor);
if (visitor.hasNonRemovableBreak) return null;
}
return new BreakConverter(switchStatement, replacement);
}
}

View File

@@ -0,0 +1,82 @@
/*
* Copyright 2003-2015 Dave Griffith, Bas Leijdekkers
*
* 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.siyeh.ig.redundancy;
import com.intellij.codeInsight.BlockUtils;
import com.intellij.codeInspection.*;
import com.intellij.openapi.project.Project;
import com.intellij.psi.*;
import com.intellij.psi.util.PsiTreeUtil;
import com.siyeh.ig.psiutils.BreakConverter;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
import java.util.Objects;
public class SwitchStatementWithSingleDefaultInspection extends AbstractBaseJavaLocalInspectionTool {
@NotNull
@Override
public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
return new JavaElementVisitor() {
@Override
public void visitSwitchStatement(PsiSwitchStatement statement) {
PsiCodeBlock body = statement.getBody();
if (body == null) return;
PsiElement anchor = Objects.requireNonNull(statement.getFirstChild());
PsiStatement[] statements = body.getStatements();
if (statements.length == 0) return;
if (!(statements[0] instanceof PsiSwitchLabelStatement) || !((PsiSwitchLabelStatement)statements[0]).isDefaultCase()) return;
if (StreamEx.of(statements).skip(1).anyMatch(PsiSwitchLabelStatement.class::isInstance)) return;
if (BreakConverter.from(statement) == null) return;
holder.registerProblem(anchor, InspectionsBundle.message("inspection.switch.statement.with.single.default.message"),
new UnwrapSwitchStatementFix());
}
};
}
private static class UnwrapSwitchStatementFix implements LocalQuickFix {
@Nls(capitalization = Nls.Capitalization.Sentence)
@NotNull
@Override
public String getFamilyName() {
return "Unwrap 'switch' statement";
}
@Override
public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
PsiSwitchStatement statement = PsiTreeUtil.getParentOfType(descriptor.getStartElement(), PsiSwitchStatement.class);
if (statement == null) return;
PsiCodeBlock body = statement.getBody();
if (body == null) return;
BreakConverter breakConverter = BreakConverter.from(statement);
if (breakConverter == null) return;
breakConverter.process(true);
PsiSwitchLabelStatement defaultCase = PsiTreeUtil.getChildOfType(body, PsiSwitchLabelStatement.class);
if (defaultCase == null || !defaultCase.isDefaultCase()) return;
defaultCase.delete();
PsiElement parent = statement.getParent();
if (!(parent instanceof PsiCodeBlock) || !BlockUtils.containsConflictingDeclarations(body, (PsiCodeBlock)parent)) {
BlockUtils.inlineCodeBlock(statement, body);
}
else {
PsiBlockStatement blockStatement = BlockUtils.createBlockStatement(project);
blockStatement.getCodeBlock().replace(body);
statement.replace(blockStatement);
}
}
}
}

View File

@@ -0,0 +1,20 @@
// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
package com.siyeh.ig.fixes;
import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
import com.intellij.codeInspection.LocalInspectionTool;
import com.siyeh.ig.redundancy.SwitchStatementWithSingleDefaultInspection;
import org.jetbrains.annotations.NotNull;
public class UnwrapSwitchStatementFixTest extends LightQuickFixParameterizedTestCase {
@NotNull
@Override
protected LocalInspectionTool[] configureLocalInspectionTools() {
return new LocalInspectionTool[] {new SwitchStatementWithSingleDefaultInspection()};
}
@Override
protected String getBasePath() {
return "/codeInsight/daemonCodeAnalyzer/quickFix/switchDefault";
}
}

View File

@@ -52,14 +52,12 @@ class SwitchPredicate implements PsiElementPredicate {
if (ErrorUtil.containsError(switchStatement)) {
return false;
}
boolean hasLabel = false;
final PsiStatement[] statements = body.getStatements();
for (PsiStatement statement : statements) {
if (statement instanceof PsiSwitchLabelStatement) {
hasLabel = true;
break;
if (statement instanceof PsiSwitchLabelStatement && !((PsiSwitchLabelStatement)statement).isDefaultCase()) {
return true;
}
}
return hasLabel;
return false;
}
}

View File

@@ -0,0 +1,9 @@
// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
class T {
void foo(Character i) {
sw<caret>itch (i) {
default:
System.out.println(i);
}
}
}

View File

@@ -21,6 +21,10 @@ public class ReplaceSwitchWithIflIntentionTest extends IPPTestCase {
doTest();
}
public void testDefaultOnly() {
assertIntentionNotAvailable();
}
@Override
protected String getIntentionName() {
return "Replace 'switch' with 'if'";