From 07b27a76a9863333b974c7d7b095c6e601a2bd3e Mon Sep 17 00:00:00 2001 From: Vladislav Rassokhin Date: Tue, 14 Feb 2017 18:38:48 +0300 Subject: [PATCH] Improve MagicConstantInspection in case of two constants sources --- .../MagicConstantInspection.java | 52 ++++++++------ .../inspection/magic/simple/expected.xml | 68 +++++++++---------- .../inspection/magic/simple/src/X.java | 21 +++--- 3 files changed, 77 insertions(+), 64 deletions(-) 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 07e430afcb76..15c64e2d345f 100644 --- a/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java @@ -45,6 +45,7 @@ import com.intellij.psi.search.LocalSearchScope; import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.*; import com.intellij.slicer.*; +import com.intellij.util.ArrayUtil; import com.intellij.util.Processor; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.indexing.FileBasedIndex; @@ -325,42 +326,51 @@ public class MagicConstantInspection extends BaseJavaLocalInspectionTool { private static AllowedValues getAllowedValuesFromMagic(@NotNull PsiType type, @NotNull PsiAnnotation magic, @NotNull PsiManager manager) { - PsiAnnotationMemberValue[] allowedValues; - final boolean canBeOred; + PsiAnnotationMemberValue[] allowedValues = PsiAnnotationMemberValue.EMPTY_ARRAY; + boolean values = false, flags = false; if (TypeConversionUtil.getTypeRank(type) <= TypeConversionUtil.LONG_RANK) { PsiAnnotationMemberValue intValues = magic.findAttributeValue("intValues"); - allowedValues = intValues instanceof PsiArrayInitializerMemberValue ? ((PsiArrayInitializerMemberValue)intValues).getInitializers() : PsiAnnotationMemberValue.EMPTY_ARRAY; - if (allowedValues.length == 0) { - PsiAnnotationMemberValue orValue = magic.findAttributeValue("flags"); - allowedValues = orValue instanceof PsiArrayInitializerMemberValue ? ((PsiArrayInitializerMemberValue)orValue).getInitializers() : PsiAnnotationMemberValue.EMPTY_ARRAY; - canBeOred = true; + if (intValues instanceof PsiArrayInitializerMemberValue) { + allowedValues = ((PsiArrayInitializerMemberValue)intValues).getInitializers(); + values = true; } else { - canBeOred = false; + PsiAnnotationMemberValue orValue = magic.findAttributeValue("flags"); + if (orValue instanceof PsiArrayInitializerMemberValue) { + allowedValues = ((PsiArrayInitializerMemberValue)orValue).getInitializers(); + flags = true; + } } } else if (type.equals(PsiType.getJavaLangString(manager, GlobalSearchScope.allScope(manager.getProject())))) { PsiAnnotationMemberValue strValuesAttr = magic.findAttributeValue("stringValues"); - allowedValues = strValuesAttr instanceof PsiArrayInitializerMemberValue ? ((PsiArrayInitializerMemberValue)strValuesAttr).getInitializers() : PsiAnnotationMemberValue.EMPTY_ARRAY; - canBeOred = false; + if (strValuesAttr instanceof PsiArrayInitializerMemberValue) { + allowedValues = ((PsiArrayInitializerMemberValue)strValuesAttr).getInitializers(); + values = true; + } } else { return null; //other types not supported } - if (allowedValues.length != 0) { - return new AllowedValues(allowedValues, canBeOred); + // Also there're could be valuesFromClass of flagsFromClass + PsiAnnotationMemberValue[] valuesFromClass = readFromClass("valuesFromClass", magic, type, manager); + if (valuesFromClass != null) { + allowedValues = ArrayUtil.mergeArrays(allowedValues, valuesFromClass, PsiAnnotationMemberValue.ARRAY_FACTORY); + values = true; } - - // last resort: try valuesFromClass - PsiAnnotationMemberValue[] values = readFromClass("valuesFromClass", magic, type, manager); - boolean ored = false; - if (values == null) { - values = readFromClass("flagsFromClass", magic, type, manager); - ored = true; + PsiAnnotationMemberValue[] flagsFromClass = readFromClass("flagsFromClass", magic, type, manager); + if (flagsFromClass != null) { + allowedValues = ArrayUtil.mergeArrays(allowedValues, flagsFromClass, PsiAnnotationMemberValue.ARRAY_FACTORY); + flags = true; } - if (values == null) return null; - return new AllowedValues(values, ored); + if (allowedValues.length == 0) { + return null; + } + if (values && flags) { + // Combination of 'flags' and 'values', that's weird TODO: Log? + } + return new AllowedValues(allowedValues, flags); } private static PsiAnnotationMemberValue[] readFromClass(@NonNls @NotNull String attributeName, diff --git a/java/java-tests/testData/inspection/magic/simple/expected.xml b/java/java-tests/testData/inspection/magic/simple/expected.xml index 062b3aa7245c..b54c2f926aab 100644 --- a/java/java-tests/testData/inspection/magic/simple/expected.xml +++ b/java/java-tests/testData/inspection/magic/simple/expected.xml @@ -14,7 +14,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -26,7 +26,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -38,7 +38,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -50,7 +50,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -62,7 +62,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -74,7 +74,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -86,7 +86,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -98,7 +98,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -110,7 +110,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -122,7 +122,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -134,7 +134,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -146,7 +146,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -158,7 +158,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -170,7 +170,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -178,7 +178,7 @@ X.java 81 magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -189,7 +189,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -201,7 +201,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -213,7 +213,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -225,7 +225,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -237,7 +237,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -249,7 +249,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -257,14 +257,14 @@ X.java 118 magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, 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 + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -276,7 +276,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -288,7 +288,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -300,7 +300,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -312,7 +312,7 @@ magic constant - Should be one of: Const.X, Const.Y, Const.Z or their combination + Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination @@ -322,7 +322,7 @@ X.java 173 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -331,7 +331,7 @@ X.java 174 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -340,7 +340,7 @@ X.java 175 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -349,7 +349,7 @@ X.java 177 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -357,7 +357,7 @@ X.java 178 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -366,7 +366,7 @@ X.java 179 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z @@ -375,7 +375,7 @@ X.java 180 magic constant - Should be one of: Const.X, Const.Y, Const.Z + Should be one of: Const2.I, Const.X, Const.Y, Const.Z diff --git a/java/java-tests/testData/inspection/magic/simple/src/X.java b/java/java-tests/testData/inspection/magic/simple/src/X.java index 043a4037e241..21a95e66a51d 100644 --- a/java/java-tests/testData/inspection/magic/simple/src/X.java +++ b/java/java-tests/testData/inspection/magic/simple/src/X.java @@ -24,7 +24,7 @@ class Const { } public class X { - void f(@MagicConstant(intValues={Const.X, Const.Y, Const.Z}) int x) { + void f(@MagicConstant(intValues={Const.X, Const.Y, Const.Z, Const2.I}) int x) { /////////// BAD f(0); f(1); @@ -50,7 +50,7 @@ public class X { f2(x); } - void f2(@MagicConstant(valuesFromClass =Const.class) int x) { + void f2(@MagicConstant(valuesFromClass =Const.class, intValues={Const2.I}) int x) { /////////// BAD f2(0); f2(1); @@ -61,11 +61,11 @@ public class X { x = 2; assert x != 1; } - ////////////// GOOD f2(Const.X); f2(Const.Y); - f2(Const.Z); + f2(Const.Z); + f2(Const2.I); int i2 = this == null ? Const.X : Const.Y; f2(i2); if (x == Const.X) { @@ -76,7 +76,7 @@ public class X { f(x); } - void f3(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) { + void f3(@MagicConstant(flags ={Const.X, Const.Y, Const.Z, Const2.I}) int x) { /////////// BAD f3(2); f3(1); @@ -113,7 +113,7 @@ public class X { f4(x); } - void f4(@MagicConstant(flagsFromClass =Const.class) int x) { + void f4(@MagicConstant(flagsFromClass =Const.class, flags={Const2.I}) int x) { /////////// BAD f4(-3); f4(1); @@ -128,8 +128,8 @@ public class X { ////////////// GOOD f4(Const.X); f4(Const.Y); - f4(Const.Z); - + f4(Const.Z); + f4(Const2.I); int i2 = this == null ? Const.X : Const.Y; f4(i2); int ix = Const.X | Const.Y; @@ -152,7 +152,7 @@ public class X { class Alias { - @MagicConstant(intValues={Const.X, Const.Y, Const.Z}) + @MagicConstant(intValues={Const.X, Const.Y, Const.Z, Const2.I}) @interface IntEnum{} void f(@IntEnum int x) { @@ -268,3 +268,6 @@ public class X { font(0); } } +class Const2 { + public static final int I = 0x10; +}