From 233c16b3e4b30ea7bf93c052655d1bb36ae2af10 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 19 Jun 2018 17:55:27 +0700 Subject: [PATCH] MagicConstantInspection: handle special case of Calendar.get() Fixes IDEA-144891 Good code is red: "Magic Constant" with Calendar.get() Tests modernized --- .../MagicConstantInspection.java | 67 ++- .../src/X.java => ManyConstantSources.java} | 26 +- .../testData/inspection/magic/Simple.java | 270 +++++++++++ .../inspection/magic/SpecialCases.java | 30 ++ .../src/X.java => WithLibrary.java} | 4 +- .../magic/manyConstantSources/expected.xml | 79 --- .../inspection/magic/simple/expected.xml | 455 ------------------ .../inspection/magic/simple/src/X.java | 270 ----------- .../inspection/magic/withLibrary/expected.xml | 9 - .../MagicConstantInspectionTest.java | 87 ++-- java/jdkAnnotations/java/util/annotations.xml | 5 - 11 files changed, 400 insertions(+), 902 deletions(-) rename java/java-tests/testData/inspection/magic/{manyConstantSources/src/X.java => ManyConstantSources.java} (58%) create mode 100644 java/java-tests/testData/inspection/magic/Simple.java create mode 100644 java/java-tests/testData/inspection/magic/SpecialCases.java rename java/java-tests/testData/inspection/magic/{withLibrary/src/X.java => WithLibrary.java} (72%) delete mode 100644 java/java-tests/testData/inspection/magic/manyConstantSources/expected.xml delete mode 100644 java/java-tests/testData/inspection/magic/simple/expected.xml delete mode 100644 java/java-tests/testData/inspection/magic/simple/src/X.java delete mode 100644 java/java-tests/testData/inspection/magic/withLibrary/expected.xml 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 0fa9924ff758..22bf8f86cfbb 100644 --- a/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java @@ -34,7 +34,11 @@ import com.intellij.util.IncorrectOperationException; import com.intellij.util.Processor; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.indexing.FileBasedIndex; +import com.siyeh.ig.callMatcher.CallMapper; +import com.siyeh.ig.callMatcher.CallMatcher; +import com.siyeh.ig.psiutils.ExpressionUtils; import gnu.trove.THashSet; +import one.util.streamex.StreamEx; import org.intellij.lang.annotations.MagicConstant; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NonNls; @@ -42,11 +46,18 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; +import java.util.function.Function; import java.util.stream.Collectors; +import static com.intellij.util.ObjectUtils.tryCast; + public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool { private static final Key NO_ANNOTATIONS_FOUND = Key.create("REPORTED_NO_ANNOTATIONS_FOUND"); + private static final CallMapper SPECIAL_CASES = new CallMapper() + .register(CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_CALENDAR, "get").parameterTypes("int"), + MagicConstantInspection::getCalendarGetValues); + @Nls @NotNull @Override @@ -136,9 +147,11 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool } } else if (l instanceof PsiMethodCallExpression) { - PsiMethod method = ((PsiMethodCallExpression)l).resolveMethod(); + PsiMethodCallExpression call = (PsiMethodCallExpression)l; + PsiMethod method = call.resolveMethod(); if (method != null) { checkExpression(r, method, method.getReturnType(), holder); + checkExpression(r, holder, SPECIAL_CASES.mapFirst(call)); } } } @@ -204,6 +217,12 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool @Nullable PsiType type, @NotNull ProblemsHolder holder) { AllowedValues allowed = getAllowedValues(owner, type, null); + checkExpression(expression, holder, allowed); + } + + private static void checkExpression(@NotNull PsiExpression expression, + @NotNull ProblemsHolder holder, + AllowedValues allowed) { if (allowed == null) return; PsiElement scope = PsiUtil.getTopLevelEnclosingCodeBlock(expression, null); if (scope == null) scope = expression; @@ -213,10 +232,12 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool } private static void checkCall(@NotNull PsiCallExpression methodCall, @NotNull ProblemsHolder holder) { + PsiExpressionList argumentList = methodCall.getArgumentList(); + if (argumentList == null) return; PsiMethod method = methodCall.resolveMethod(); if (method == null) return; PsiParameter[] parameters = method.getParameterList().getParameters(); - PsiExpression[] arguments = methodCall.getArgumentList().getExpressions(); + PsiExpression[] arguments = argumentList.getExpressions(); for (int i = 0; i < parameters.length; i++) { PsiParameter parameter = parameters[i]; AllowedValues values = getAllowedValues(parameter, parameter.getType(), null); @@ -285,17 +306,8 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool } boolean isSubsetOf(@NotNull AllowedValues other, @NotNull PsiManager manager) { - for (PsiAnnotationMemberValue value : values) { - boolean found = false; - for (PsiAnnotationMemberValue otherValue : other.values) { - if (same(value, otherValue, manager)) { - found = true; - break; - } - } - if (!found) return false; - } - return true; + return Arrays.stream(values).allMatch( + value -> Arrays.stream(other.values).anyMatch(otherValue -> same(value, otherValue, manager))); } } @@ -406,6 +418,30 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool return parseBeanInfo(element, manager); } + private static AllowedValues getCalendarGetValues(PsiMethodCallExpression call) { + Integer argument = tryCast(ExpressionUtils.computeConstantExpression(call.getArgumentList().getExpressions()[0]), Integer.class); + PsiMethod method = call.resolveMethod(); + if (method == null || argument == null) return null; + return CachedValuesManager.getCachedValue(method, () -> { + final String[] days = {"SUNDAY", "MONDAY", "TUESDAY", "WEDNESDAY", "THURSDAY", "FRIDAY", "SATURDAY"}; + final String[] months = {"JANUARY", "FEBRUARY", "MARCH", "APRIL", "MAY", "JUNE", "JULY", + "AUGUST", "SEPTEMBER", "OCTOBER", "NOVEMBER", "DECEMBER"}; + final String[] amPm = {"AM", "PM"}; + PsiElementFactory factory = JavaPsiFacade.getElementFactory(method.getProject()); + Function converter = strings -> { + String expression = StreamEx.of(strings) + .map((CommonClassNames.JAVA_UTIL_CALENDAR + ".")::concat).joining(",", "{", "}"); + PsiArrayInitializerExpression initializer = (PsiArrayInitializerExpression)factory.createExpressionFromText(expression, method); + return new AllowedValues(initializer.getInitializers(), false); + }; + Map map = new HashMap<>(); + map.put(Calendar.DAY_OF_WEEK, converter.apply(days)); + map.put(Calendar.MONTH, converter.apply(months)); + map.put(Calendar.AM_PM, converter.apply(amPm)); + return CachedValueProvider.Result.create(map, method); + }).get(argument); + } + @NotNull private static PsiAnnotation[] getAllAnnotations(@NotNull PsiModifierListOwner element) { return CachedValuesManager.getCachedValue(element, () -> @@ -708,7 +744,7 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool static boolean processValuesFlownTo(@NotNull final PsiExpression argument, @NotNull PsiElement scope, @NotNull PsiManager manager, - @NotNull final Processor processor) { + @NotNull final Processor processor) { SliceAnalysisParams params = new SliceAnalysisParams(); params.dataFlowToThis = true; params.scope = new AnalysisScope(new LocalSearchScope(scope), manager.getProject()); @@ -744,7 +780,8 @@ public class MagicConstantInspection extends AbstractBaseJavaLocalInspectionTool @NotNull @Override public String getText() { - List names = myMemberValuePointers.stream().map(SmartPsiElementPointer::getElement).map(PsiElement::getText).collect(Collectors.toList()); + List names = myMemberValuePointers.stream().map(SmartPsiElementPointer::getElement).filter(Objects::nonNull) + .map(PsiElement::getText).collect(Collectors.toList()); String expression = StringUtil.join(names, " | "); return "Replace with '" + expression + "'"; } diff --git a/java/java-tests/testData/inspection/magic/manyConstantSources/src/X.java b/java/java-tests/testData/inspection/magic/ManyConstantSources.java similarity index 58% rename from java/java-tests/testData/inspection/magic/manyConstantSources/src/X.java rename to java/java-tests/testData/inspection/magic/ManyConstantSources.java index 950683a5e24d..58fb48141d1b 100644 --- a/java/java-tests/testData/inspection/magic/manyConstantSources/src/X.java +++ b/java/java-tests/testData/inspection/magic/ManyConstantSources.java @@ -24,13 +24,13 @@ class Const2 { public static final int I = 4; } -public class X { +class X { void f(@MagicConstant(intValues = {Const1.X, Const2.I}) int x) { /////////// BAD - f(0); - f(1); - f(Const1.X | Const2.I); + f(0); + f(1); + f(Const1.X | Const2.I); ////////////// GOOD f(Const1.X); @@ -41,9 +41,9 @@ public class X { void f2(@MagicConstant(valuesFromClass = Const1.class, intValues = {Const2.I}) int x) { /////////// BAD - f2(0); - f2(1); - f2(Const1.X | Const2.I); + f2(0); + f2(1); + f2(Const1.X | Const2.I); ////////////// GOOD f2(Const1.X); @@ -54,11 +54,11 @@ public class X { void f3(@MagicConstant(flags = {Const1.X, Const2.I}) int x) { /////////// BAD - f3(2); - f3(1); - f(Const1.X | Const2.I); + f3(2); + f3(1); + f(Const1.X | Const2.I); int i = Const1.X | 4; - f3(i); + f3(i); ////////////// GOOD f3(Const1.X); @@ -69,8 +69,8 @@ public class X { void f4(@MagicConstant(flagsFromClass = Const1.class, flags = {Const2.I}) int x) { /////////// BAD - f4(-3); - f4(1); + f4(-3); + f4(1); ////////////// GOOD f4(Const1.X); diff --git a/java/java-tests/testData/inspection/magic/Simple.java b/java/java-tests/testData/inspection/magic/Simple.java new file mode 100644 index 000000000000..c0b01a90c5c7 --- /dev/null +++ b/java/java-tests/testData/inspection/magic/Simple.java @@ -0,0 +1,270 @@ +/* + * Copyright 2000-2011 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. + */ +import org.intellij.lang.annotations.MagicConstant; + +import java.io.*; + +class Const { + public static final int X = 1; + public static final int Y = 2; + public static final int Z = 4; +} +public class X { + + void f(@MagicConstant(intValues={Const.X, Const.Y, Const.Z}) int x) { + /////////// BAD + f(0); + f(1); + f(Const.X | Const.Y); + int i = Const.X | Const.Y; + f(i); + if (x == 3) { + x = 2; + assert x != 1; + } + + ////////////// GOOD + f(Const.X); + f(Const.Y); + f(Const.Z); + int i2 = this == null ? Const.X : Const.Y; + f(i2); + if (x == Const.X) { + x = Const.Y; + assert x != Const.Z; + } + + f2(x); + } + + void f2(@MagicConstant(valuesFromClass =Const.class) int x) { + /////////// BAD + f2(0); + f2(1); + f2(Const.X | Const.Y); + int i = Const.X | Const.Y; + f2(i); + if (x == 3) { + x = 2; + assert x != 1; + } + + ////////////// GOOD + f2(Const.X); + f2(Const.Y); + f2(Const.Z); + int i2 = this == null ? Const.X : Const.Y; + f2(i2); + if (x == Const.X) { + x = Const.Y; + assert x != Const.Z; + } + + f(x); + } + + void f3(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) { + /////////// BAD + f3(2); + f3(1); + f(Const.X | Const.Y); + int i = Const.X | 4; + f3(i); + if (x == 3) { + x = 2; + assert x != 1; + } + + ////////////// GOOD + f3(Const.X); + f3(Const.Y); + f3(Const.Z); + + int i2 = this == null ? Const.X : Const.Y; + f3(i2); + int ix = Const.X | Const.Y; + f3(ix); + f3(0); + f3(-1); + int f = 0; + if (x == Const.X) { + x = Const.Y; + assert x != Const.Z; + f |= Const.Y; f &= Const.X & ~(Const.Z | Const.X); + } + else { + f |= Const.X; f = f & ~(Const.X | Const.X); + } + f3(f); + + f4(x); + } + + void f4(@MagicConstant(flagsFromClass =Const.class) int x) { + /////////// BAD + f4(-3); + f4(1); + f4(Const.X | Const.Y); + int i = Const.X | 4; + f4(i); + if (x == 3) { + x = 2; + assert x != 1; + } + + ////////////// GOOD + f4(Const.X); + f4(Const.Y); + f4(Const.Z); + + int i2 = this == null ? Const.X : Const.Y; + f4(i2); + int ix = Const.X | Const.Y; + f4(ix); + f4(0); + f4(-1); + int f = 0; + if (x == Const.X) { + x = Const.Y; + assert x != Const.Z; + f |= Const.Y; + } + else { + f |= Const.X; + } + f4(f); + + f3(x); + } + + @MagicConstant(intValues={Const.X, Const.Y, Const.Z}) + @interface IntEnum{} + + class Alias { + + void f(@IntEnum int x) { + ////////////// GOOD + f(Const.X); + f(Const.Y); + f(Const.Z); + int i2 = this == null ? Const.X : Const.Y; + f(i2); + if (x == Const.X) { + x = Const.Y; + assert x != Const.Z; + } + + f2(x); + + /////////// BAD + f(0); + f(1); + f(Const.X | Const.Y); + int i = Const.X | Const.Y; + f(i); + if (x == 3 || getClass().isInterface()) { + x = 2; + assert x != 1; + } + + f2(x); + } + } + + @interface III { + @MagicConstant(intValues = {Const.X, Const.Y}) int val(); + } + class MagicAnnoInsideAnnotationUsage { + + // bad + @III(val = 2) + int h; + @III(val = Const.X | Const.Y) + void f(){} + + // good + @III(val = Const.X) + int h2; + } + + abstract class BeanInfoParsing { + /** + * @see java.lang.Runtime#exit(int) + * + * @beaninfo + * preferred: true + * bound: true + * enum: DO_NOTHING_ON_CLOSE Const.X + * HIDE_ON_CLOSE Const.Y + * description: The frame's default close operation. + */ + public void setX(int operation) { + + } + + public abstract int getX(); + + { + // good + setX(Const.X); + setX(Const.Y); + if (getX() == Const.X || getX() == Const.Y) {} + + // bad + setX(0); + setX(-1); + setX(Const.Z); + if (getX() == 1) {} + if (getX() == Const.Z) {} + } + + } + + class ExternalAnnotations { + void f() { + java.util.Calendar.getInstance().set(2000,9,0) + new javax.swing.JLabel("text", 3); + } + } + static class OverrideX extends X { + void f(int x) { + super.f(x); + } + } + + void plusSupportedInFlags(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) { + ////////////// GOOD + plusSupportedInFlags(Const.X + Const.Y); + plusSupportedInFlags(Const.Z + Const.X + Const.Y); + plusSupportedInFlags(Const.Z + (Const.X + Const.Y)); + + int ix = Const.X + Const.Y; + plusSupportedInFlags(ix); + plusSupportedInFlags(0); + plusSupportedInFlags(-1); + } + + /////////////////////////////////////// + static class FontType { + public static final int PLAIN = 0; + public static final int BOLD = 1; + public static final int ITALIC = 2; + } + void font(@MagicConstant(flags = {FontType.PLAIN, FontType.BOLD, FontType.ITALIC}) int x) { + // 0 is not allowed despite the fact that it's flags parameter + font(0); + } +} diff --git a/java/java-tests/testData/inspection/magic/SpecialCases.java b/java/java-tests/testData/inspection/magic/SpecialCases.java new file mode 100644 index 000000000000..9c81dfaf5397 --- /dev/null +++ b/java/java-tests/testData/inspection/magic/SpecialCases.java @@ -0,0 +1,30 @@ +/* + * Copyright 2000-2011 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. + */ +import java.util.Calendar; + +class SpecialCases { + void test() { + Calendar instance = Calendar.getInstance(); + if(instance.get(Calendar.DAY_OF_WEEK) == Calendar.MONDAY) {} + if(instance.get(Calendar.DAY_OF_WEEK) == Calendar.FEBRUARY) {} + if(instance.get(Calendar.MONTH) == Calendar.MONDAY) {} + if(instance.get(Calendar.MONTH) == Calendar.FEBRUARY) {} + if(instance.get(Calendar.MONTH) == Calendar.AM) {} + if(instance.get(Calendar.AM_PM) == Calendar.AM) {} + if(instance.get(Calendar.AM_PM) == Calendar.MONDAY) {} + if(instance.get(Calendar.DATE) == 1) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/magic/withLibrary/src/X.java b/java/java-tests/testData/inspection/magic/WithLibrary.java similarity index 72% rename from java/java-tests/testData/inspection/magic/withLibrary/src/X.java rename to java/java-tests/testData/inspection/magic/WithLibrary.java index c07d84684cf6..f0b70b7bfec7 100644 --- a/java/java-tests/testData/inspection/magic/withLibrary/src/X.java +++ b/java/java-tests/testData/inspection/magic/WithLibrary.java @@ -18,9 +18,9 @@ import org.intellij.lang.annotations.MagicConstant; import java.awt.*; import javax.swing.*; -public class X { +class X { void f(JFrame frame) { - frame.setDefaultCloseOperation(2); // there is beanInfo in in JFrame.java, have to parse (but added to exceptions, so ok) + frame.setDefaultCloseOperation(2); // there is beanInfo in in JFrame.java, have to parse (but added to exceptions, so ok) // despite JFrame.EXIT_ON_CLOSE (incorrectly) not mentioned in beaninfo, we override it in our annotations.xml, see IDEA-186767 frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE); diff --git a/java/java-tests/testData/inspection/magic/manyConstantSources/expected.xml b/java/java-tests/testData/inspection/magic/manyConstantSources/expected.xml deleted file mode 100644 index acae8fed90c2..000000000000 --- a/java/java-tests/testData/inspection/magic/manyConstantSources/expected.xml +++ /dev/null @@ -1,79 +0,0 @@ - - - - X.java - 31 - magic constant - Should be one of: Const1.X, Const2.I - - - X.java - 32 - magic constant - Should be one of: Const1.X, Const2.I - - - X.java - 33 - magic constant - Should be one of: Const1.X, Const2.I - - - - X.java - 44 - magic constant - Should be one of: Const2.I, Const1.X - - - X.java - 45 - magic constant - Should be one of: Const2.I, Const1.X - - - X.java - 46 - magic constant - Should be one of: Const2.I, Const1.X - - - - X.java - 57 - magic constant - Should be one of: Const1.X, Const2.I or their combination - - - X.java - 58 - magic constant - Should be one of: Const1.X, Const2.I or their combination - - - X.java - 59 - magic constant - Should be one of: Const1.X, Const2.I - - - X.java - 61 - magic constant - Should be one of: Const1.X, Const2.I or their combination - - - - X.java - 72 - magic constant - Should be one of: Const2.I, Const1.X or their combination - - - X.java - 73 - magic constant - Should be one of: Const2.I, Const1.X or their combination - - - diff --git a/java/java-tests/testData/inspection/magic/simple/expected.xml b/java/java-tests/testData/inspection/magic/simple/expected.xml deleted file mode 100644 index 062b3aa7245c..000000000000 --- a/java/java-tests/testData/inspection/magic/simple/expected.xml +++ /dev/null @@ -1,455 +0,0 @@ - - - - - - - - - - - X.java - 29 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 30 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 31 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 33 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 34 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 35 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 36 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 55 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 56 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 57 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 59 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 60 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 61 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 62 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - X.java - 81 - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - X.java - 82 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 83 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 85 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 86 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 87 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 88 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - X.java - 118 - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - X.java - 119 - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 122 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 123 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 124 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - X.java - 125 - - - - magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination - - - - - - - X.java - 173 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 174 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 175 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 177 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - X.java - 178 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 179 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - - X.java - 180 - magic constant - Should be one of: Const.X, Const.Y, Const.Z - - - - - X.java - 193 - magic constant - Should be one of: Const.X, Const.Y - - - - - X.java - 195 - magic constant - Should be one of: Const.X, Const.Y - - - - X.java - 227 - magic constant - Should be one of: Const.X, Const.Y - - - - - X.java - 228 - magic constant - Should be one of: Const.X, Const.Y - - - - - X.java - 229 - magic constant - Should be one of: Const.X, Const.Y - - - - - X.java - 230 - magic constant - Should be one of: Const.X, Const.Y - - - - X.java - 231 - magic constant - Should be one of: Const.X, Const.Y - - - - X.java - 238 - Magic Constant - Should be one of: Calendar.JANUARY, Calendar.FEBRUARY, Calendar.MARCH, Calendar.APRIL, Calendar.MAY, Calendar.JUNE, Calendar.JULY, Calendar.AUGUST, Calendar.SEPTEMBER, Calendar.OCTOBER, Calendar.NOVEMBER, Calendar.DECEMBER - - - - X.java - 239 - Magic Constant - Should be one of: SwingConstants.LEFT, SwingConstants.CENTER, SwingConstants.RIGHT, SwingConstants.LEADING, SwingConstants.TRAILING - - - - X.java - 268 - Magic Constant - Should be one of: FontType.PLAIN, FontType.BOLD, FontType.ITALIC or their combination - - diff --git a/java/java-tests/testData/inspection/magic/simple/src/X.java b/java/java-tests/testData/inspection/magic/simple/src/X.java deleted file mode 100644 index 043a4037e241..000000000000 --- a/java/java-tests/testData/inspection/magic/simple/src/X.java +++ /dev/null @@ -1,270 +0,0 @@ -/* - * Copyright 2000-2011 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. - */ -import org.intellij.lang.annotations.MagicConstant; - -import java.io.*; - -class Const { - public static final int X = 1; - public static final int Y = 2; - public static final int Z = 4; -} -public class X { - - void f(@MagicConstant(intValues={Const.X, Const.Y, Const.Z}) int x) { - /////////// BAD - f(0); - f(1); - f(Const.X | Const.Y); - int i = Const.X | Const.Y; - f(i); - if (x == 3) { - x = 2; - assert x != 1; - } - - ////////////// GOOD - f(Const.X); - f(Const.Y); - f(Const.Z); - int i2 = this == null ? Const.X : Const.Y; - f(i2); - if (x == Const.X) { - x = Const.Y; - assert x != Const.Z; - } - - f2(x); - } - - void f2(@MagicConstant(valuesFromClass =Const.class) int x) { - /////////// BAD - f2(0); - f2(1); - f2(Const.X | Const.Y); - int i = Const.X | Const.Y; - f2(i); - if (x == 3) { - x = 2; - assert x != 1; - } - - ////////////// GOOD - f2(Const.X); - f2(Const.Y); - f2(Const.Z); - int i2 = this == null ? Const.X : Const.Y; - f2(i2); - if (x == Const.X) { - x = Const.Y; - assert x != Const.Z; - } - - f(x); - } - - void f3(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) { - /////////// BAD - f3(2); - f3(1); - f(Const.X | Const.Y); - int i = Const.X | 4; - f3(i); - if (x == 3) { - x = 2; - assert x != 1; - } - - ////////////// GOOD - f3(Const.X); - f3(Const.Y); - f3(Const.Z); - - int i2 = this == null ? Const.X : Const.Y; - f3(i2); - int ix = Const.X | Const.Y; - f3(ix); - f3(0); - f3(-1); - int f = 0; - if (x == Const.X) { - x = Const.Y; - assert x != Const.Z; - f |= Const.Y; f &= Const.X & ~(Const.Z | Const.X); - } - else { - f |= Const.X; f = f & ~(Const.X | Const.X); - } - f3(f); - - f4(x); - } - - void f4(@MagicConstant(flagsFromClass =Const.class) int x) { - /////////// BAD - f4(-3); - f4(1); - f4(Const.X | Const.Y); - int i = Const.X | 4; - f4(i); - if (x == 3) { - x = 2; - assert x != 1; - } - - ////////////// GOOD - f4(Const.X); - f4(Const.Y); - f4(Const.Z); - - int i2 = this == null ? Const.X : Const.Y; - f4(i2); - int ix = Const.X | Const.Y; - f4(ix); - f4(0); - f4(-1); - int f = 0; - if (x == Const.X) { - x = Const.Y; - assert x != Const.Z; - f |= Const.Y; - } - else { - f |= Const.X; - } - f4(f); - - f3(x); - } - - - class Alias { - @MagicConstant(intValues={Const.X, Const.Y, Const.Z}) - @interface IntEnum{} - - void f(@IntEnum int x) { - ////////////// GOOD - f(Const.X); - f(Const.Y); - f(Const.Z); - int i2 = this == null ? Const.X : Const.Y; - f(i2); - if (x == Const.X) { - x = Const.Y; - assert x != Const.Z; - } - - f2(x); - - /////////// BAD - f(0); - f(1); - f(Const.X | Const.Y); - int i = Const.X | Const.Y; - f(i); - if (x == 3 || getClass().isInterface()) { - x = 2; - assert x != 1; - } - - f2(x); - } - } - - class MagicAnnoInsideAnnotationUsage { - @interface III { - @MagicConstant(intValues = {Const.X, Const.Y}) int val(); - } - - // bad - @III(val = 2) - int h; - @III(val = Const.X | Const.Y) - void f(){} - - // good - @III(val = Const.X) - int h2; - } - - abstract class BeanInfoParsing { - /** - * @see java.lang.Runtime#exit(int) - * - * @beaninfo - * preferred: true - * bound: true - * enum: DO_NOTHING_ON_CLOSE Const.X - * HIDE_ON_CLOSE Const.Y - * description: The frame's default close operation. - */ - public void setX(int operation) { - - } - - public abstract int getX(); - - { - // good - setX(Const.X); - setX(Const.Y); - if (getX() == Const.X || getX() == Const.Y) {} - - // bad - setX(0); - setX(-1); - setX(Const.Z); - if (getX() == 1) {} - if (getX() == Const.Z) {} - } - - } - - class ExternalAnnotations { - void f() { - java.util.Calendar.getInstance().set(2000,9,0) - new javax.swing.JLabel("text", 3); - } - } - static class OverrideX extends X { - void f(int x) { - super.f(x); - } - } - - void plusSupportedInFlags(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) { - ////////////// GOOD - plusSupportedInFlags(Const.X + Const.Y); - plusSupportedInFlags(Const.Z + Const.X + Const.Y); - plusSupportedInFlags(Const.Z + (Const.X + Const.Y)); - - int ix = Const.X + Const.Y; - plusSupportedInFlags(ix); - plusSupportedInFlags(0); - plusSupportedInFlags(-1); - } - - /////////////////////////////////////// - static class FontType { - public static final int PLAIN = 0; - public static final int BOLD = 1; - public static final int ITALIC = 2; - } - void font(@MagicConstant(flags = {FontType.PLAIN, FontType.BOLD, FontType.ITALIC}) int x) { - // 0 is not allowed despite the fact that it's flags parameter - font(0); - } -} diff --git a/java/java-tests/testData/inspection/magic/withLibrary/expected.xml b/java/java-tests/testData/inspection/magic/withLibrary/expected.xml deleted file mode 100644 index 33405f685a37..000000000000 --- a/java/java-tests/testData/inspection/magic/withLibrary/expected.xml +++ /dev/null @@ -1,9 +0,0 @@ - - - - X.java - 23 - Magic Constant - Should be one of: WindowConstants.DO_NOTHING_ON_CLOSE, WindowConstants.HIDE_ON_CLOSE, WindowConstants.DISPOSE_ON_CLOSE, WindowConstants.EXIT_ON_CLOSE - - 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 27b5c7c06132..5751c0f48645 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/MagicConstantInspectionTest.java @@ -17,73 +17,38 @@ package com.intellij.java.codeInspection; import com.intellij.JavaTestUtil; -import com.intellij.codeInspection.ex.LocalInspectionToolWrapper; import com.intellij.codeInspection.magicConstant.MagicConstantInspection; import com.intellij.openapi.projectRoots.Sdk; -import com.intellij.openapi.vfs.LocalFileSystem; -import com.intellij.openapi.vfs.VfsUtilCore; -import com.intellij.openapi.vfs.VirtualFile; -import com.intellij.openapi.vfs.VirtualFileVisitor; -import com.intellij.psi.JavaPsiFacade; +import com.intellij.psi.CommonClassNames; import com.intellij.psi.PsiClass; -import com.intellij.psi.impl.PsiManagerEx; +import com.intellij.psi.PsiElement; import com.intellij.psi.impl.source.PsiClassImpl; -import com.intellij.psi.search.GlobalSearchScope; -import com.intellij.testFramework.FileTreeAccessFilter; import com.intellij.testFramework.IdeaTestUtil; -import com.intellij.testFramework.InspectionTestCase; +import com.intellij.testFramework.LightProjectDescriptor; import com.intellij.testFramework.PsiTestUtil; +import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; -import java.io.File; - -public class MagicConstantInspectionTest extends InspectionTestCase { - private FileTreeAccessFilter myFilter; +public class MagicConstantInspectionTest extends LightCodeInsightFixtureTestCase { + private static final LightProjectDescriptor DESCRIPTOR = new LightProjectDescriptor() { + @Nullable + @Override + public Sdk getSdk() { + // has to have JFrame and sources + return PsiTestUtil.addJdkAnnotations(IdeaTestUtil.getMockJdk17()); + } + }; @Override - protected String getTestDataPath() { - return JavaTestUtil.getJavaTestDataPath() + "/inspection"; + protected String getBasePath() { + return JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/magic/"; } + @NotNull @Override - protected void setUp() throws Exception { - super.setUp(); - myFilter = new FileTreeAccessFilter(); - PsiManagerEx.getInstanceEx(getProject()).setAssertOnFileLoadingFilter(myFilter, getTestRootDisposable()); - } - - @Override - protected Sdk getTestProjectSdk() { - // has to have JFrame and sources - return PsiTestUtil.addJdkAnnotations(IdeaTestUtil.getMockJdk17()); - } - - private void doTest() { - doTest("magic/" + getTestName(true), new LocalInspectionToolWrapper(new MagicConstantInspection()), "jdk 1.7"); - } - - @Override - protected void setupRootModel(@NotNull String testDir, @NotNull VirtualFile[] sourceDir, String sdkName) { - super.setupRootModel(testDir, sourceDir, sdkName); - - PsiClass jframe = getJavaFacade().findClass("javax.swing.JFrame", GlobalSearchScope.allScope(getProject())); - assertNotNull("configure decent JDK for testsake", jframe); - - VirtualFile projectDir = LocalFileSystem.getInstance().refreshAndFindFileByIoFile(new File(testDir)); - // allow to load AST for all files to highlight - VfsUtilCore.visitChildrenRecursively(projectDir, new VirtualFileVisitor() { - @Override - public boolean visitFile(@NotNull VirtualFile v) { - myFilter.allowTreeAccessForFile(v); - return super.visitFile(v); - } - }); - // and JFrame - PsiClass cls = JavaPsiFacade.getInstance(getProject()).findClass("javax.swing.JFrame", GlobalSearchScope.allScope(getProject())); - PsiClass aClass = (PsiClass)cls.getNavigationElement(); - assertTrue(aClass instanceof PsiClassImpl); // must to have sources - - myFilter.allowTreeAccessForFile(aClass.getContainingFile().getVirtualFile()); + protected LightProjectDescriptor getProjectDescriptor() { + return DESCRIPTOR; } public void testSimple() { doTest(); } @@ -91,4 +56,18 @@ public class MagicConstantInspectionTest extends InspectionTestCase { public void testManyConstantSources() { doTest(); } // test that the optimisation for not loading AST works public void testWithLibrary() { doTest(); } + public void testSpecialCases() { doTest(); } + + private void doTest() { + myFixture.configureByFile(getTestName(false) + ".java"); + + PsiClass calendarClass = myFixture.getJavaFacade().findClass(CommonClassNames.JAVA_UTIL_CALENDAR); + assertNotNull("No Calendar class in mockJDK", calendarClass); + PsiElement calendarSource = calendarClass.getNavigationElement(); + assertTrue(calendarSource instanceof PsiClassImpl); + myFixture.allowTreeAccessForFile(calendarSource.getContainingFile().getVirtualFile()); + + myFixture.enableInspections(new MagicConstantInspection()); + myFixture.testHighlighting(true, false, false); + } } diff --git a/java/jdkAnnotations/java/util/annotations.xml b/java/jdkAnnotations/java/util/annotations.xml index a3e6b560cb94..33bbd52c6092 100644 --- a/java/jdkAnnotations/java/util/annotations.xml +++ b/java/jdkAnnotations/java/util/annotations.xml @@ -835,11 +835,6 @@ - - - - -