[java-intentions] ConvertSwitchToIf: fix the intention according a switch throws NPE if the selector expression is null

IDEA-300120

GitOrigin-RevId: 5d77f98e7931e32a06cb3c54076978d317f78a98
This commit is contained in:
Andrey.Cherkasov
2022-08-17 11:39:15 +04:00
committed by intellij-monorepo-bot
parent cd9e96567a
commit 108dc80b56
33 changed files with 226 additions and 79 deletions

View File

@@ -1,4 +1,4 @@
// Copyright 2000-2021 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInsight.daemon.impl.quickfix;
import com.intellij.codeInsight.BlockUtils;
@@ -7,6 +7,7 @@ import com.intellij.codeInsight.intention.FileModifier;
import com.intellij.codeInspection.CommonQuickFixBundle;
import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.project.Project;
import com.intellij.pom.java.LanguageLevel;
import com.intellij.psi.*;
import com.intellij.psi.codeStyle.JavaCodeStyleManager;
import com.intellij.psi.codeStyle.VariableKind;
@@ -62,12 +63,12 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
private static boolean mayFallThroughNonTerminalDefaultCase(PsiCodeBlock body) {
List<PsiSwitchLabelStatementBase> labels = PsiTreeUtil.getChildrenOfTypeAsList(body, PsiSwitchLabelStatementBase.class);
return StreamEx.of(labels).pairMap((prev, next) -> {
if (SwitchUtils.isDefaultLabel(prev)) {
Set<PsiSwitchLabelStatementBase> targets = getFallThroughTargets(body);
return targets.contains(prev) || targets.contains(next);
}
return false;
}).has(true);
if (SwitchUtils.isDefaultLabel(prev)) {
Set<PsiSwitchLabelStatementBase> targets = getFallThroughTargets(body);
return targets.contains(prev) || targets.contains(next);
}
return false;
}).has(true);
}
@Override
@@ -96,13 +97,6 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
return;
}
CommentTracker commentTracker = new CommentTracker();
final boolean isSwitchOnString = switchExpressionType.equalsToText(CommonClassNames.JAVA_LANG_STRING);
boolean useEquals = isSwitchOnString;
if (!useEquals) {
final PsiClass aClass = PsiUtil.resolveClassInType(switchExpressionType);
String fqn;
useEquals = aClass != null && !aClass.isEnum() && ((fqn = aClass.getQualifiedName()) == null || !TypeConversionUtil.isPrimitiveWrapper(fqn));
}
PsiCodeBlock body = switchStatement.getBody();
if (body == null) {
return;
@@ -114,6 +108,17 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
if (converter == null) return;
converter.process();
final List<SwitchStatementBranch> allBranches = extractBranches(body, fallThroughTargets);
boolean needNullCheck = false;
final boolean isSwitchOnString = switchExpressionType.equalsToText(CommonClassNames.JAVA_LANG_STRING);
boolean useEquals = isSwitchOnString;
if (!useEquals) {
final PsiClass aClass = PsiUtil.resolveClassInType(switchExpressionType);
if (aClass != null) {
String fqn = aClass.getQualifiedName();
useEquals = !aClass.isEnum() && (fqn == null || !TypeConversionUtil.isPrimitiveWrapper(fqn));
needNullCheck = (fqn == null || !TypeConversionUtil.isPrimitiveWrapper(fqn)) && !hasNullCase(allBranches);
}
}
final String declarationString;
final boolean hadSideEffects;
@@ -142,15 +147,22 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
final StringBuilder ifStatementBuilder = new StringBuilder();
boolean firstBranch = true;
SwitchStatementBranch defaultBranch = null;
boolean java7plus = PsiUtil.getLanguageLevel(project).isAtLeast(LanguageLevel.JDK_1_7);
for (SwitchStatementBranch branch : allBranches) {
if (branch.isDefault()) {
defaultBranch = branch;
}
else {
dumpBranch(branch, expressionText, firstBranch, useEquals, ifStatementBuilder, commentTracker);
dumpBranch(branch, expressionText, firstBranch, useEquals, needNullCheck && java7plus, ifStatementBuilder, commentTracker);
firstBranch = false;
}
}
if (needNullCheck && !java7plus) {
if (!firstBranch) {
ifStatementBuilder.append("else ");
}
ifStatementBuilder.append("if(").append(expressionText).append("==null) { throw new NullPointerException(); }");
}
boolean unwrapDefault = false;
if (defaultBranch != null) {
unwrapDefault = defaultBranch.isAlwaysExecuted() || (switchStatement.getParent() instanceof PsiCodeBlock && !mayCompleteNormally);
@@ -204,6 +216,11 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
}
}
private static boolean hasNullCase(@NotNull List<SwitchStatementBranch> allBranches) {
return ContainerUtil.or(allBranches, br -> ContainerUtil.or(br.getCaseElements(), element -> element instanceof PsiExpression expr &&
ExpressionUtils.isNullLiteral(expr)));
}
@NotNull
private static List<SwitchStatementBranch> extractBranches(PsiCodeBlock body,
Set<PsiSwitchLabelStatementBase> fallThroughTargets) {
@@ -277,7 +294,7 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
private static Set<PsiSwitchLabelStatementBase> getFallThroughTargets(PsiCodeBlock body) {
return StreamEx.of(body.getStatements())
.pairMap((s1, s2) -> s2 instanceof PsiSwitchLabelStatement && !(s1 instanceof PsiSwitchLabeledRuleStatement) &&
.pairMap((s1, s2) -> s2 instanceof PsiSwitchLabelStatement && !(s1 instanceof PsiSwitchLabeledRuleStatement) &&
ControlFlowUtils.statementMayCompleteNormally(s1) ? (PsiSwitchLabelStatement)s2 : null)
.nonNull().collect(Collectors.toSet());
}
@@ -286,18 +303,21 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
String expressionText,
boolean firstBranch,
boolean useEquals,
boolean useRequireNonNullMethod,
@NonNls StringBuilder out,
CommentTracker commentTracker) {
if (!firstBranch) {
out.append("else ");
}
dumpCaseValues(expressionText, branch.getCaseElements(), useEquals, commentTracker, out);
dumpCaseValues(expressionText, branch.getCaseElements(), firstBranch, useEquals, useRequireNonNullMethod, commentTracker, out);
dumpBody(branch, out, commentTracker);
}
private static void dumpCaseValues(String expressionText,
List<PsiElement> caseElements,
boolean firstBranch,
boolean useEquals,
boolean useRequireNonNullMethod,
CommentTracker commentTracker,
@NonNls StringBuilder out) {
out.append("if(");
@@ -306,6 +326,10 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
if (!firstCaseValue) {
out.append("||");
}
final String newExpressionText =
firstBranch && firstCaseValue && useRequireNonNullMethod
? "java.util.Objects.requireNonNull(" + expressionText + ")"
: expressionText;
firstCaseValue = false;
if (caseElement instanceof PsiExpression) {
PsiExpression caseExpression = PsiUtil.skipParenthesizedExprDown((PsiExpression)caseElement);
@@ -314,20 +338,20 @@ public class ConvertSwitchToIfIntention implements IntentionActionWithFixAllOpti
if (PsiPrecedenceUtil.getPrecedence(caseExpression) > PsiPrecedenceUtil.METHOD_CALL_PRECEDENCE) {
caseValue = "(" + caseValue + ")";
}
out.append(caseValue).append(".equals(").append(expressionText).append(')');
out.append(expressionText).append(".equals(").append(caseValue).append(')');
}
else if (caseValue.equals("true")) {
out.append(expressionText);
out.append(newExpressionText);
}
else if (caseValue.equals("false")) {
out.append("!(").append(expressionText).append(")");
out.append("!(").append(newExpressionText).append(")");
}
else {
out.append(expressionText).append("==").append(caseValue);
out.append(newExpressionText).append("==").append(caseValue);
}
}
else if (caseElement instanceof PsiPattern) {
String patternCondition = createIfCondition((PsiPattern)caseElement, expressionText, commentTracker);
String patternCondition = createIfCondition((PsiPattern)caseElement, newExpressionText, commentTracker);
if (patternCondition != null) {
out.append(patternCondition);
} else {

View File

@@ -17,7 +17,7 @@ package com.intellij.codeInsight.daemon.impl.quickfix;
import com.intellij.psi.*;
import com.siyeh.ig.psiutils.ControlFlowUtils;
import com.siyeh.ig.psiutils.SwitchUtils;
import com.siyeh.ig.psiutils.ExpressionUtils;
import java.util.*;
@@ -31,16 +31,12 @@ class SwitchStatementBranch {
private boolean myHasStatements;
private boolean myAlwaysExecuted;
public void addCaseValue(PsiElement caseElement) {
myCaseElements.add(caseElement);
}
public void addStatement(PsiStatement statement) {
void addStatement(PsiStatement statement) {
myHasStatements = myHasStatements || !ControlFlowUtils.isEmpty(statement, false, true);
addElement(statement);
}
public void addComment(PsiElement comment) {
void addComment(PsiElement comment) {
addElement(comment);
}
@@ -50,41 +46,33 @@ class SwitchStatementBranch {
myBodyElements.add(element);
}
public void addWhiteSpace(PsiElement statement) {
void addWhiteSpace(PsiElement statement) {
if (!myBodyElements.isEmpty()) {
myPendingWhiteSpace.add(statement);
}
}
public List<PsiElement> getCaseElements() {
List<PsiElement> getCaseElements() {
return Collections.unmodifiableList(myCaseElements);
}
public List<PsiElement> getBodyElements() {
List<PsiElement> getBodyElements() {
return Collections.unmodifiableList(myBodyElements);
}
public boolean isDefault() {
boolean isDefault() {
return myDefault;
}
public void setDefault() {
myDefault = true;
}
boolean isAlwaysExecuted() {
return myAlwaysExecuted;
}
void setAlwaysExecuted(boolean alwaysExecuted) {
myAlwaysExecuted = alwaysExecuted;
}
public boolean hasStatements() {
boolean hasStatements() {
return myHasStatements;
}
public void addPendingDeclarations(Set<? extends PsiElement> vars) {
void addPendingDeclarations(Set<? extends PsiElement> vars) {
myPendingDeclarations.addAll(vars);
}
@@ -93,15 +81,27 @@ class SwitchStatementBranch {
}
void addCaseValues(PsiSwitchLabelStatementBase label, boolean defaultAlwaysExecuted) {
if (SwitchUtils.isDefaultLabel(label)) {
setDefault();
setAlwaysExecuted(defaultAlwaysExecuted);
}
else {
if (label.isDefaultCase()) {
myDefault = true;
myAlwaysExecuted = defaultAlwaysExecuted;
} else {
PsiExpression nullCase = null;
PsiCaseLabelElementList labelElementList = label.getCaseLabelElementList();
if (labelElementList != null) {
for (PsiCaseLabelElement labelElement : labelElementList.getElements()) {
addCaseValue(labelElement);
if (labelElement instanceof PsiDefaultCaseLabelElement) {
myDefault = true;
myAlwaysExecuted = defaultAlwaysExecuted;
break;
}
else if (labelElement instanceof PsiExpression expression && ExpressionUtils.isNullLiteral(expression)) {
nullCase = expression;
}
}
if (!myDefault) {
Collections.addAll(myCaseElements, labelElementList.getElements());
} else if (nullCase != null) {
myCaseElements.add(nullCase);
}
}
}

View File

@@ -4,8 +4,8 @@ abstract class Test {
void foo() {
Class<?> aClass = getObject().getClass();
if (RuntimeException.class.equals(aClass)) {
} else if (IOException.class.equals(aClass)) {
if (aClass.equals(RuntimeException.class)) {
} else if (aClass.equals(IOException.class)) {
}
}
}

View File

@@ -2,9 +2,9 @@
class X {
int m(String s, boolean r) {
if (r) return 1;
else if ("a".equals(s)) {
else if (s.equals("a")) {
return 1;
} else if ("b".equals(s)) {
} else if (s.equals("b")) {
return 2;
} else {
return 3;

View File

@@ -1,9 +1,9 @@
// "Replace 'switch' with 'if'" "true-preview"
class X {
int m(String s, boolean r) {
if ("a".equals(s)) {
if (s.equals("a")) {
return 1;
} else if ("b".equals(s)) {
} else if (s.equals("b")) {
return 2;
}
return 3;

View File

@@ -1,7 +1,7 @@
// "Replace 'switch' with 'if'" "true-preview"
class X {
void m(String s, boolean r) {
if ("a".equals(s)) {
if (s.equals("a")) {
System.out.println("a");
if (r) {
return;

View File

@@ -1,7 +1,7 @@
// "Replace 'switch' with 'if'" "true-preview"
class X {
void m(String s, boolean r) {
if ("a".equals(s)) {
if (s.equals("a")) {
System.out.println("a");
if (r) {
} else {

View File

@@ -4,7 +4,7 @@ class X {
if (x > 0) {
SWITCH:
{
if ("a".equals(s)) {
if (s.equals("a")) {
System.out.println("a");
for (int i = 0; i < 10; i++) {
System.out.println(i);

View File

@@ -1,10 +1,14 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
abstract class Test {
abstract Object getObject();
void foo(Object o) {
if (o instanceof String) {
if (Objects.requireNonNull(o) instanceof String) {
System.out.println("one");
} else if (o instanceof Integer) {
System.out.println("two");
} else {
System.out.println("default");
}

View File

@@ -0,0 +1,12 @@
// "Replace 'switch' with 'if'" "true-preview"
abstract class Test {
abstract Object getObject();
void foo(Object o) {
if (o instanceof String) {
System.out.println("one");
} else {
System.out.println("default");
}
}
}

View File

@@ -1,7 +1,7 @@
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test(String str) {
if (("foo" + "bar").equals(str)) {
if (str.equals(("foo" + "bar"))) {
}
}
}

View File

@@ -1,7 +1,9 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test(Object obj) {
if (obj == 1) {
if (Objects.requireNonNull(obj) == 1) {
}
}
}

View File

@@ -1,9 +1,9 @@
// "Replace 'switch' with 'if'" "true-preview"
class X {
void m(String s) {
if ("foo".equals(s)) {
if (s.equals("foo")) {
System.out.println(1);
} else if ("bar".equals(s)) {
} else if (s.equals("bar")) {
System.out.println(3);
} else {
System.out.println(2);

View File

@@ -2,7 +2,7 @@
class X {
int m(String s, boolean r) {
//ignore
if ("x".equals(s)) {
if (s.equals("x")) {
System.out.println("foo");
}
}

View File

@@ -1,7 +1,9 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test(Object obj) {
if (obj == obj instanceof String s) {
if (Objects.requireNonNull(obj) == obj instanceof String s) {
}
}
}

View File

@@ -1,7 +1,9 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test(Object obj) {
if (obj == obj instanceof String) {
if (Objects.requireNonNull(obj) == obj instanceof String) {
}
}
}

View File

@@ -1,3 +1,5 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test() {
@@ -5,7 +7,7 @@ class Test {
s;
}
P p = null;
if (p == P.s) {
if (Objects.requireNonNull(p) == P.s) {
}
}
}

View File

@@ -0,0 +1,13 @@
import java.util.Objects;
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test() {
enum P {
s, l;
}
P p = null;
if (Objects.requireNonNull(p) == P.s || p == P.l) {
}
}
}

View File

@@ -0,0 +1,11 @@
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test() {
enum P {
s, l;
}
P p = null;
if (p == P.s || p == null) {
}
}
}

View File

@@ -2,7 +2,7 @@
class X {
void m(String s, boolean r) {
if (r) {
if ("a".equals(s)) {
if (s.equals("a")) {
System.out.println("a");
}
System.out.println("d");

View File

@@ -2,7 +2,7 @@
class X {
void m(String s, int a) throws IOException {
if ("a".equals(s)) {
if (s.equals("a")) {
a();
} else {
d();

View File

@@ -2,9 +2,9 @@
class X {
void test4() throws IOException {
String variable = "abc";
if ("abc".equals(variable)) {
if (variable.equals("abc")) {
String s1 = "abcd";
} else if ("def".equals(variable)) {
} else if (variable.equals("def")) {
String s1 = "abcd";
myFunction(s1);
} else {

View File

@@ -3,10 +3,10 @@ class X {
public void doSomething( String value) {
//comment6
//comment7
if ("case1".equals(value)) {//comment1
if (value.equals("case1")) {//comment1
//comment2
//comment3
} else if ("case2".equals(value)) {//comment4
} else if (value.equals("case2")) {//comment4
//comment5
}//comment8
}

View File

@@ -3,9 +3,9 @@ abstract class Test {
abstract Object getObject();
void foo(String s) {
if (s == null || "zero".equals(s)) {
if (s == null || s.equals("zero")) {
System.out.println(0);
} else if ("one".equals(s)) {
} else if (s.equals("one")) {
System.out.println(1);
}
}

View File

@@ -1,7 +1,7 @@
// "Replace 'switch' with 'if'" "true-preview"
public class One {
void f1(String a) {
if ("one".equals(a)) {
if (a.equals("one")) {
System.out.println(1);
}
System.out.println("default");

View File

@@ -3,7 +3,7 @@ abstract class Test {
abstract Object getObject();
void foo() {
if (RuntimeException.class.equals(getObject().getClass())) {
if (getObject().getClass().equals(RuntimeException.class)) {
System.out.println("RuntimeException");
} else {
System.out.println("Other");

View File

@@ -5,7 +5,8 @@ abstract class Test {
void foo(Object o) {
<caret>switch (o) {
case String s -> System.out.println("one");
case null, default -> System.out.println("default");
case Integer i -> System.out.println("two");
case default -> System.out.println("default");
}
}
}

View File

@@ -0,0 +1,11 @@
// "Replace 'switch' with 'if'" "true-preview"
abstract class Test {
abstract Object getObject();
void foo(Object o) {
<caret>switch (o) {
case String s -> System.out.println("one");
case null, default -> System.out.println("default");
}
}
}

View File

@@ -0,0 +1,13 @@
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test() {
enum P {
s, l;
}
P p = null;
<caret>switch (p) {
case s, l -> {}
default -> {}
}
}
}

View File

@@ -0,0 +1,13 @@
// "Replace 'switch' with 'if'" "true-preview"
class Test {
void test() {
enum P {
s, l;
}
P p = null;
<caret>switch (p) {
case s, null -> {}
default -> {}
}
}
}

View File

@@ -0,0 +1,16 @@
class Test {
void test() {
enum P {
a, b, c;
}
P p = null;
<caret>switch (p) {
case a, b -> {
}
case c -> {
}
default -> {
}
}
}
}

View File

@@ -0,0 +1,13 @@
class Test {
void test() {
enum P {
a, b, c;
}
P p = null;
if (p == P.a || p == P.b) {
} else if (p == P.c) {
} else if (p == null) {
throw new NullPointerException();
}
}
}

View File

@@ -1,8 +1,10 @@
// 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.
// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.siyeh.ipp.switchtoif;
import com.intellij.codeInspection.CommonQuickFixBundle;
import com.intellij.pom.java.LanguageLevel;
import com.intellij.psi.PsiKeyword;
import com.intellij.testFramework.IdeaTestUtil;
import com.siyeh.ipp.IPPTestCase;
public class ReplaceSwitchWithIfIntentionTest extends IPPTestCase {
@@ -27,6 +29,12 @@ public class ReplaceSwitchWithIfIntentionTest extends IPPTestCase {
assertIntentionNotAvailable();
}
public void testReplaceEnum() {
IdeaTestUtil.withLevel(getModule(), LanguageLevel.JDK_1_6, () -> {
doTest();
});
}
@Override
protected String getIntentionName() {
return CommonQuickFixBundle.message("fix.replace.x.with.y", PsiKeyword.SWITCH, PsiKeyword.IF);