From c11147ce043287c96d92e61277b8ac7f27ef64d4 Mon Sep 17 00:00:00 2001 From: Andrey Cherkasov Date: Wed, 23 Nov 2022 18:01:21 +0400 Subject: [PATCH] [java-inspections] MagicConstant: fixes after review, tests GitOrigin-RevId: a5a93b6e8e025b4831b75e00fc48683c30504db8 --- .../MagicConstantInspection.java | 106 +++-------- ...Expression.java => SwitchExpression1.java} | 2 +- .../inspection/magic/SwitchExpression2.java | 168 ++++++++++++++++++ .../MagicConstantInspectionTest.java | 5 +- 4 files changed, 195 insertions(+), 86 deletions(-) rename java/java-tests/testData/inspection/magic/{SwitchExpression.java => SwitchExpression1.java} (99%) create mode 100644 java/java-tests/testData/inspection/magic/SwitchExpression2.java diff --git a/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java b/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java index acc841f72cec..e02195cf1176 100644 --- a/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java @@ -21,7 +21,6 @@ import com.intellij.openapi.util.Key; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.codeStyle.JavaCodeStyleManager; -import com.intellij.psi.controlFlow.*; import com.intellij.psi.impl.JavaConstantExpressionEvaluator; import com.intellij.psi.search.GlobalSearchScope; import com.intellij.psi.search.LocalSearchScope; @@ -34,7 +33,6 @@ import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.callMatcher.CallMapper; import com.siyeh.ig.callMatcher.CallMatcher; import com.siyeh.ig.psiutils.ExpressionUtils; -import it.unimi.dsi.fastutil.ints.IntArrayList; import one.util.streamex.Joining; import one.util.streamex.StreamEx; import org.intellij.lang.annotations.MagicConstant; @@ -233,7 +231,7 @@ public final class MagicConstantInspection extends AbstractBaseJavaLocalInspecti if (allowed == null) return; PsiElement scope = PsiUtil.getTopLevelEnclosingCodeBlock(expression, null); if (scope == null) scope = expression; - if (!isAllowed(expression, scope, allowed, expression.getManager(), null)) { + if (!isAllowed(expression, scope, allowed, expression.getManager())) { registerProblem(expression, allowed, holder); } } @@ -296,7 +294,7 @@ public final class MagicConstantInspection extends AbstractBaseJavaLocalInspecti @NotNull ProblemsHolder holder) { final PsiManager manager = PsiManager.getInstance(holder.getProject()); - if (!argument.getTextRange().isEmpty() && !isAllowed(argument, parameter.getDeclarationScope(), allowedValues, manager, null)) { + if (!argument.getTextRange().isEmpty() && !isAllowed(argument, parameter.getDeclarationScope(), allowedValues, manager)) { registerProblem(argument, allowedValues, holder); } } @@ -382,117 +380,64 @@ public final class MagicConstantInspection extends AbstractBaseJavaLocalInspecti private static boolean isAllowed(@NotNull final PsiExpression argument, @NotNull final PsiElement scope, @NotNull final AllowedValues allowedValues, - @NotNull final PsiManager manager, - @Nullable Set visited) { - if (isGoodExpression(argument, allowedValues, scope, manager, visited)) return true; + @NotNull final PsiManager manager) { + if (isGoodExpression(argument, allowedValues, scope, manager)) return true; - return processValuesFlownTo(argument, scope, manager, - expression -> isGoodExpression(expression, allowedValues, scope, manager, visited)); + return processValuesFlownTo(argument, scope, manager, expression -> isGoodExpression(expression, allowedValues, scope, manager)); } private static boolean isGoodExpression(@NotNull PsiExpression argument, @NotNull AllowedValues allowedValues, @NotNull PsiElement scope, - @NotNull PsiManager manager, - @Nullable Set visited) { - PsiExpression expression = PsiUtil.deparenthesizeExpression(argument); - if (expression == null) return true; - if (visited == null) visited = new HashSet<>(); - if (!visited.add(expression)) return false; - if (expression instanceof PsiConditionalExpression cond) { - PsiExpression thenExpression = cond.getThenExpression(); - boolean thenAllowed = thenExpression == null || isAllowed(thenExpression, scope, allowedValues, manager, visited); - if (!thenAllowed) return false; - PsiExpression elseExpression = cond.getElseExpression(); - return elseExpression == null || isAllowed(elseExpression, scope, allowedValues, manager, visited); - } - else if (expression instanceof PsiSwitchExpression switchExpression) { - return isGoodSwitchExpression(switchExpression, allowedValues, scope, manager, visited); + @NotNull PsiManager manager) { + if (argument instanceof PsiParenthesizedExpression || + argument instanceof PsiConditionalExpression || + argument instanceof PsiSwitchExpression) { + return ExpressionUtils.nonStructuralChildren(argument).allMatch(e -> isAllowed(e, scope, allowedValues, manager)); } - if (isOneOf(expression, allowedValues, manager)) return true; + if (isOneOf(argument, allowedValues, manager)) return true; if (allowedValues.isFlagSet()) { - PsiExpression zero = getLiteralExpression(expression, manager, "0"); - if (MagicConstantUtils.same(expression, zero, manager) + PsiExpression zero = getLiteralExpression(argument, manager, "0"); + if (MagicConstantUtils.same(argument, zero, manager) // if for some crazy reason the constant with value "0" is included to allowed values for flags, do not treat literal "0" as allowed value anymore // see e.g. Font.BOLD=1, Font.ITALIC=2, Font.PLAIN=0 && !allowedValues.hasZeroValue()) return true; - PsiExpression minusOne = getLiteralExpression(expression, manager, "-1"); - if (MagicConstantUtils.same(expression, minusOne, manager)) return true; - if (expression instanceof PsiPolyadicExpression polyadic) { + PsiExpression minusOne = getLiteralExpression(argument, manager, "-1"); + if (MagicConstantUtils.same(argument, minusOne, manager)) return true; + if (argument instanceof PsiPolyadicExpression polyadic) { IElementType tokenType = polyadic.getOperationTokenType(); if (JavaTokenType.OR.equals(tokenType) || JavaTokenType.XOR.equals(tokenType) || JavaTokenType.AND.equals(tokenType) || JavaTokenType.PLUS.equals(tokenType)) { - for (PsiExpression operand : polyadic.getOperands()) { - if (!isAllowed(operand, scope, allowedValues, manager, visited)) return false; - } - return true; + return ContainerUtil.all(polyadic.getOperands(), e -> isAllowed(e, scope, allowedValues, manager)); } } - if (expression instanceof PsiPrefixExpression prefixExpression && + if (argument instanceof PsiPrefixExpression prefixExpression && JavaTokenType.TILDE.equals(prefixExpression.getOperationTokenType())) { PsiExpression operand = prefixExpression.getOperand(); - return operand == null || isAllowed(operand, scope, allowedValues, manager, visited); + return operand == null || isAllowed(operand, scope, allowedValues, manager); } } PsiModifierListOwner owner = null; AllowedValues allowedForRef = null; - if (expression instanceof PsiReference reference) { + if (argument instanceof PsiReference reference) { owner = ObjectUtils.tryCast(reference.resolve(), PsiModifierListOwner.class); } - else if (expression instanceof PsiMethodCallExpression call) { + else if (argument instanceof PsiMethodCallExpression call) { allowedForRef = SPECIAL_CASES.mapFirst(call); owner = call.resolveMethod(); } if (allowedForRef == null && owner != null) { - allowedForRef = MagicConstantUtils.getAllowedValues(owner, PsiUtil.getTypeByPsiElement(owner), expression); + allowedForRef = MagicConstantUtils.getAllowedValues(owner, PsiUtil.getTypeByPsiElement(owner), argument); } if (allowedForRef != null && allowedForRef.isSubsetOf(allowedValues, manager)) { return true; } - return PsiType.NULL.equals(expression.getType()); - } - - private static boolean isGoodSwitchExpression(PsiSwitchExpression switchExpression, - AllowedValues values, - PsiElement scope, - PsiManager manager, - Set visited) { - PsiCodeBlock body = switchExpression.getBody(); - if (body == null) return true; - List rules = ContainerUtil.filterIsInstance(body.getStatements(), PsiSwitchLabeledRuleStatement.class); - for (PsiSwitchLabeledRuleStatement rule : rules) { - if (rule.getBody() instanceof PsiExpressionStatement expressionStatement) { - if (!isAllowed(expressionStatement.getExpression(), scope, values, manager, visited)) { - return false; - } - } - } - final ControlFlow controlFlow; - try { - controlFlow = ControlFlowFactory.getInstance(switchExpression.getProject()) - .getControlFlow(switchExpression, LocalsOrMyInstanceFieldsControlFlowPolicy.getInstance()); - } - catch (AnalysisCanceledException ignored) { - return true; - } - int startOffset = controlFlow.getStartOffset(switchExpression); - int endOffset = controlFlow.getEndOffset(switchExpression); - if (startOffset < 0 || endOffset < 0) return true; - Collection exitStatements = - ControlFlowUtil.findExitPointsAndStatements(controlFlow, startOffset, endOffset, new IntArrayList(), PsiYieldStatement.class); - for (PsiStatement exitStatement : exitStatements) { - PsiYieldStatement yieldStatement = (PsiYieldStatement)exitStatement; - PsiExpression expression = yieldStatement.getExpression(); - if (expression != null && !isAllowed(expression, scope, values, manager, visited)) { - return false; - } - } - return true; + return PsiType.NULL.equals(argument.getType()); } private static final Key> LITERAL_EXPRESSION_CACHE = Key.create("LITERAL_EXPRESSION_CACHE"); @@ -512,10 +457,7 @@ public final class MagicConstantInspection extends AbstractBaseJavaLocalInspecti } private static boolean isOneOf(@NotNull PsiExpression expression, @NotNull AllowedValues allowedValues, @NotNull PsiManager manager) { - for (PsiAnnotationMemberValue allowedValue : allowedValues.getValues()) { - if (MagicConstantUtils.same(allowedValue, expression, manager)) return true; - } - return false; + return ContainerUtil.exists(allowedValues.getValues(), e -> MagicConstantUtils.same(e, expression, manager)); } static boolean processValuesFlownTo(@NotNull final PsiExpression argument, diff --git a/java/java-tests/testData/inspection/magic/SwitchExpression.java b/java/java-tests/testData/inspection/magic/SwitchExpression1.java similarity index 99% rename from java/java-tests/testData/inspection/magic/SwitchExpression.java rename to java/java-tests/testData/inspection/magic/SwitchExpression1.java index ab1a949d8cc6..9dbcbb08b8e4 100644 --- a/java/java-tests/testData/inspection/magic/SwitchExpression.java +++ b/java/java-tests/testData/inspection/magic/SwitchExpression1.java @@ -159,7 +159,7 @@ class Main { yield MARCH; } case 4 -> { - yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : 42); + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); } default -> 42; }; diff --git a/java/java-tests/testData/inspection/magic/SwitchExpression2.java b/java/java-tests/testData/inspection/magic/SwitchExpression2.java new file mode 100644 index 000000000000..ba26724552a1 --- /dev/null +++ b/java/java-tests/testData/inspection/magic/SwitchExpression2.java @@ -0,0 +1,168 @@ +import org.intellij.lang.annotations.MagicConstant; + +import static java.util.Calendar.*; + +class Main { + void acceptMonth(@MagicConstant(intValues = {JANUARY, FEBRUARY, MARCH, APRIL, MAY, JUNE, JULY, AUGUST, SEPTEMBER, OCTOBER, NOVEMBER, DECEMBER}) int month) { + } + + void test1(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test2(int num) { + int month = switch (num) { + case 1: + yield 42; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test3(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield 42; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test4(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield 42; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test5(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? 42 : APRIL) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test6(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : 42) : APRIL); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test7(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : 42); + default: + throw new IllegalStateException("Unexpected value: " + num); + }; + acceptMonth(month); + } + + void test8(int num) { + int month = switch (num) { + case 1: + yield JANUARY; + case 2: + System.out.println("hello"); + yield FEBRUARY; + case 3: + if (Math.random() > 0.5) { + yield MARCH; + } + yield MARCH; + case 4: + yield Math.random() > 0.5 ? APRIL : (Math.random() > 0.5 ? (Math.random() > 0.5 ? APRIL : APRIL) : APRIL); + default: + yield 42; + }; + acceptMonth(month); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java index 990e85eb1954..ba049305cf46 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java @@ -36,9 +36,8 @@ public class MagicConstantInspectionTest extends LightJavaCodeInsightFixtureTest public void testVarargMethodCall() { doTest(); } public void testEnumConstructor() { doTest(); } public void testSwitchBlock() { doTest(); } - public void testSwitchExpression() { - IdeaTestUtil.withLevel(getModule(), LanguageLevel.JDK_14, () -> doTest()); - } + public void testSwitchExpression1() { IdeaTestUtil.withLevel(getModule(), LanguageLevel.JDK_14, () -> doTest()); } + public void testSwitchExpression2() { IdeaTestUtil.withLevel(getModule(), LanguageLevel.JDK_14, () -> doTest()); } private void doTest() { myFixture.configureByFile(getTestName(false) + ".java");