IDEA-CR-18229 Improve MagicConstantInspection in case of two constants sources:

* Reverted origin test, add new test case
* Throw exception if both 'flags' and 'values' used.
* Fix tests: `PsiAnnotation#findAttributeValue` may return default values, ignore them

java/java-impl/src/com/intellij/codeInspection/magicConstant/MagicConstantInspection.java

@@ -46,6 +46,7 @@ import com.intellij.psi.tree.IElementType;
 import com.intellij.psi.util.*;
 import com.intellij.slicer.*;
 import com.intellij.util.ArrayUtil;
+import com.intellij.util.IncorrectOperationException;
 import com.intellij.util.Processor;
 import com.intellij.util.containers.ContainerUtil;
 import com.intellij.util.indexing.FileBasedIndex;
@@ -331,29 +332,37 @@ public class MagicConstantInspection extends BaseJavaLocalInspectionTool {
     if (TypeConversionUtil.getTypeRank(type) <= TypeConversionUtil.LONG_RANK) {
       PsiAnnotationMemberValue intValues = magic.findAttributeValue("intValues");
       if (intValues instanceof PsiArrayInitializerMemberValue) {
-        allowedValues = ((PsiArrayInitializerMemberValue)intValues).getInitializers();
-        values = true;
+        final PsiAnnotationMemberValue[] initializers = ((PsiArrayInitializerMemberValue)intValues).getInitializers();
+        if (initializers.length != 0) {
+          allowedValues = initializers;
+          values = true;
+        }
       }
-      else {
+      if (!values) {
         PsiAnnotationMemberValue orValue = magic.findAttributeValue("flags");
         if (orValue instanceof PsiArrayInitializerMemberValue) {
-          allowedValues = ((PsiArrayInitializerMemberValue)orValue).getInitializers();
-          flags = true;
+          final PsiAnnotationMemberValue[] initializers = ((PsiArrayInitializerMemberValue)orValue).getInitializers();
+          if (initializers.length != 0) {
+            allowedValues = initializers;
+            flags = true;
+          }
         }
       }
     }
     else if (type.equals(PsiType.getJavaLangString(manager, GlobalSearchScope.allScope(manager.getProject())))) {
       PsiAnnotationMemberValue strValuesAttr = magic.findAttributeValue("stringValues");
       if (strValuesAttr instanceof PsiArrayInitializerMemberValue) {
-        allowedValues = ((PsiArrayInitializerMemberValue)strValuesAttr).getInitializers();
-        values = true;
+        final PsiAnnotationMemberValue[] initializers = ((PsiArrayInitializerMemberValue)strValuesAttr).getInitializers();
+        if (initializers.length != 0) {
+          allowedValues = initializers;
+          values = true;
+        }
       }
     }
     else {
       return null; //other types not supported
     }
 
-    // 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);
@@ -368,7 +377,8 @@ public class MagicConstantInspection extends BaseJavaLocalInspectionTool {
       return null;
     }
     if (values && flags) {
-      // Combination of 'flags' and 'values', that's weird TODO: Log?
+      throw new IncorrectOperationException(
+        "Misconfiguration of @MagicConstant annotation: 'flags' and 'values' shouldn't be used at the same time");
     }
     return new AllowedValues(allowedValues, flags);
   }

java/java-tests/testData/inspection/magic/manyConstantSources/expected.xml

@@ -0,0 +1,79 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<problems>
+  <problem>
+    <file>X.java</file>
+    <line>31</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>32</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>33</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I</description>
+  </problem>
+
+  <problem>
+    <file>X.java</file>
+    <line>44</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const2.I, Const1.X</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>45</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const2.I, Const1.X</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>46</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const2.I, Const1.X</description>
+  </problem>
+
+  <problem>
+    <file>X.java</file>
+    <line>57</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I or their combination</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>58</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I or their combination</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>59</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>61</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const1.X, Const2.I or their combination</description>
+  </problem>
+
+  <problem>
+    <file>X.java</file>
+    <line>72</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const2.I, Const1.X or their combination</description>
+  </problem>
+  <problem>
+    <file>X.java</file>
+    <line>73</line>
+    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
+    <description>Should be one of: Const2.I, Const1.X or their combination</description>
+  </problem>
+
+</problems>

java/java-tests/testData/inspection/magic/manyConstantSources/src/X.java

@@ -0,0 +1,82 @@
+/*
+ * 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;
+
+class Const1 {
+  public static final int X = 1;
+}
+
+class Const2 {
+  public static final int I = 4;
+}
+
+public class X {
+
+  void f(@MagicConstant(intValues = {Const1.X, Const2.I}) int x) {
+    /////////// BAD
+    f(0);
+    f(1);
+    f(Const1.X | Const2.I);
+
+    ////////////// GOOD
+    f(Const1.X);
+    f(Const2.I);
+
+    f2(x);
+  }
+
+  void f2(@MagicConstant(valuesFromClass = Const1.class, intValues = {Const2.I}) int x) {
+    /////////// BAD
+    f2(0);
+    f2(1);
+    f2(Const1.X | Const2.I);
+
+    ////////////// GOOD
+    f2(Const1.X);
+    f2(Const2.I);
+
+    f(x);
+  }
+
+  void f3(@MagicConstant(flags = {Const1.X, Const2.I}) int x) {
+    /////////// BAD
+    f3(2);
+    f3(1);
+    f(Const1.X | Const2.I);
+    int i = Const1.X | 4;
+    f3(i);
+
+    ////////////// GOOD
+    f3(Const1.X);
+    f3(Const2.I);
+
+    f4(x);
+  }
+
+  void f4(@MagicConstant(flagsFromClass = Const1.class, flags = {Const2.I}) int x) {
+    /////////// BAD
+    f4(-3);
+    f4(1);
+    
+    ////////////// GOOD
+    f4(Const1.X);
+    f4(Const2.I);
+    f4(Const1.X | Const2.I);
+
+    f3(x);
+  }
+}

java/java-tests/testData/inspection/magic/simple/expected.xml

@@ -14,7 +14,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -26,7 +26,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -38,7 +38,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -50,7 +50,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -62,7 +62,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -74,7 +74,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -86,7 +86,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
 
@@ -98,7 +98,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -110,7 +110,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -122,7 +122,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -134,7 +134,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -146,7 +146,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -158,7 +158,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -170,7 +170,7 @@
   
   
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -178,7 +178,7 @@
    <file>X.java</file>
    <line>81</line>
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
 
   
@@ -189,7 +189,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
   
   
@@ -201,7 +201,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z</description>
   </problem>
   
   
@@ -213,7 +213,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
   
   
@@ -225,7 +225,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
   
   
@@ -237,7 +237,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
   
   
@@ -249,7 +249,7 @@
     
     
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
   
   
@@ -257,14 +257,14 @@
     <file>X.java</file>
     <line>118</line>
     <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-    <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+    <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
   </problem>
 
  <problem>
    <file>X.java</file>
    <line>119</line>
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
  
  
@@ -276,7 +276,7 @@
    
    
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
  
  
@@ -288,7 +288,7 @@
    
    
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
  
  
@@ -300,7 +300,7 @@
    
    
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
  
  
@@ -312,7 +312,7 @@
    
    
    <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-   <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z or their combination</description>
+   <description>Should be one of: Const.X, Const.Y, Const.Z or their combination</description>
  </problem>
  
 
@@ -322,7 +322,7 @@
   <file>X.java</file>
   <line>173</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -331,7 +331,7 @@
   <file>X.java</file>
   <line>174</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -340,7 +340,7 @@
   <file>X.java</file>
   <line>175</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -349,7 +349,7 @@
   <file>X.java</file>
   <line>177</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
   
 
@@ -357,7 +357,7 @@
   <file>X.java</file>
   <line>178</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -366,7 +366,7 @@
   <file>X.java</file>
   <line>179</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
 
 
@@ -375,7 +375,7 @@
   <file>X.java</file>
   <line>180</line>
   <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">magic constant</problem_class>
-  <description>Should be one of: Const2.I, Const.X, Const.Y, Const.Z</description>
+  <description>Should be one of: Const.X, Const.Y, Const.Z</description>
 </problem>
   
 

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, Const2.I}) int x) {
+  void f(@MagicConstant(intValues={Const.X, Const.Y, Const.Z}) int x) {
     /////////// BAD
     f(0);
     f(1);
@@ -50,7 +50,7 @@ public class X {
     f2(x);
   }
 
-  void f2(@MagicConstant(valuesFromClass =Const.class, intValues={Const2.I}) int x) {
+  void f2(@MagicConstant(valuesFromClass =Const.class) 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(Const2.I);
+    f2(Const.Z);
     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, Const2.I}) int x) {
+  void f3(@MagicConstant(flags ={Const.X, Const.Y, Const.Z}) int x) {
     /////////// BAD
     f3(2);
     f3(1);
@@ -113,7 +113,7 @@ public class X {
     f4(x);
   }
 
-  void f4(@MagicConstant(flagsFromClass =Const.class, flags={Const2.I}) int x) {
+  void f4(@MagicConstant(flagsFromClass =Const.class) 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(Const2.I);
+    f4(Const.Z);
+
     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, Const2.I})
+    @MagicConstant(intValues={Const.X, Const.Y, Const.Z})
     @interface IntEnum{}
 
     void f(@IntEnum int x) {
@@ -268,6 +268,3 @@ public class X {
     font(0);
   }
 }
-class Const2 {
-  public static final int I = 0x10;
-}

java/java-tests/testSrc/com/intellij/codeInspection/MagicConstantInspectionTest.java

@@ -92,6 +92,8 @@ public class MagicConstantInspectionTest extends InspectionTestCase {
   }
 
   public void testSimple() throws Exception { doTest(); }
+  
+  public void testManyConstantSources() throws Exception { doTest(); }
   // test that the optimisation for not loading AST works
   public void testWithLibrary() throws Exception { doTest(); }
 }