Illegal Type Check, updated default illegal types, added memberModifi…

…ers option, issue #567

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java

@@ -24,6 +24,9 @@
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 import com.puppycrawl.tools.checkstyle.checks.AbstractFormatCheck;
 import com.puppycrawl.tools.checkstyle.checks.CheckUtils;
+
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Set;
 
 /**
@@ -66,7 +69,22 @@
  * will be ok.
  * </p>
  * <p>
- * <b>ignoredMethodNames</b> - Methods that should not be checked..
+ * <b>ignoredMethodNames</b> - Methods that should not be checked.
+ * </p>
+ * <p>
+ * <b>memberModifiers</b> - To check only methods and fields with only specified modifiers.
+ * </p>
+ * <p>
+ * In most cases it's justified to put following classes to <b>illegalClassNames</b>:
+ * <ul>
+ * <li>GregorianCalendar</li>
+ * <li>Hashtable</li>
+ * <li>ArrayList</li>
+ * <li>LinkedList</li>
+ * <li>Vector</li>
+ * </ul>
+ * as methods that are differ from interface methods are rear used, so in most cases user will
+ *  benefit from checking for them.
  * </p>
  *
  * @author <a href="mailto:simon@redhillconsulting.com.au">Simon Harris</a>
@@ -80,28 +98,18 @@
     private static final String[] DEFAULT_LEGAL_ABSTRACT_NAMES = {};
     /** Types illegal by default. */
     private static final String[] DEFAULT_ILLEGAL_TYPES = {
-        "GregorianCalendar",
-        "Hashtable",
         "HashSet",
         "HashMap",
-        "ArrayList",
-        "LinkedList",
         "LinkedHashMap",
         "LinkedHashSet",
         "TreeSet",
         "TreeMap",
-        "Vector",
-        "java.util.GregorianCalendar",
-        "java.util.Hashtable",
         "java.util.HashSet",
         "java.util.HashMap",
-        "java.util.ArrayList",
-        "java.util.LinkedList",
         "java.util.LinkedHashMap",
         "java.util.LinkedHashSet",
         "java.util.TreeSet",
         "java.util.TreeMap",
-        "java.util.Vector",
     };
 
     /** Default ignored method names. */
@@ -116,6 +124,8 @@
     private final Set<String> legalAbstractClassNames = Sets.newHashSet();
     /** methods which should be ignored. */
     private final Set<String> ignoredMethodNames = Sets.newHashSet();
+    /** check methods and fields with only corresponding modifiers. */
+    private List<Integer> memberModifiers;
 
     /** Creates new instance of the check. */
     public IllegalTypeCheck()
@@ -142,10 +152,14 @@ public void visitToken(DetailAST ast)
     {
         switch (ast.getType()) {
             case TokenTypes.METHOD_DEF:
-                visitMethodDef(ast);
+                if (isVerifiable(ast)) {
+                    visitMethodDef(ast);
+                }
                 break;
             case TokenTypes.VARIABLE_DEF:
-                visitVariableDef(ast);
+                if (isVerifiable(ast)) {
+                    visitVariableDef(ast);
+                }
                 break;
             case TokenTypes.PARAMETER_DEF:
                 visitParameterDef(ast);
@@ -158,6 +172,32 @@ public void visitToken(DetailAST ast)
         }
     }
 
+    /**
+     * Checks if current method's return type or variable's type is verifiable
+     * according to <b>memberModifiers</b> option.
+     * @param methodOrVariableDef METHOD_DEF or VARIABLE_DEF ast node.
+     * @return true if member is verifiable according to <b>memberModifiers</b> option.
+     */
+    private boolean isVerifiable(DetailAST methodOrVariableDef)
+    {
+        boolean result = true;
+        if (memberModifiers != null) {
+            result = false;
+            final DetailAST modifiersAst = methodOrVariableDef.
+                    findFirstToken(TokenTypes.MODIFIERS);
+            if (modifiersAst.getFirstChild() != null) {
+                for (DetailAST modifier = modifiersAst.getFirstChild(); modifier != null;
+                         modifier = modifier.getNextSibling())
+                {
+                    if (memberModifiers.contains(modifier.getType())) {
+                        result = true;
+                    }
+                }
+            }
+        }
+        return result;
+    }
+
     /**
      * Checks return type of a given method.
      * @param methodDef method for check.
@@ -404,4 +444,17 @@ public void setLegalAbstractClassNames(String[] classNames)
         return legalAbstractClassNames.toArray(
             new String[legalAbstractClassNames.size()]);
     }
+
+    /**
+     * Set the list of member modifiers (of methods and fields) which should be checked.
+     * @param modifiers String contains modifiers.
+     */
+    public void setMemberModifiers(String modifiers)
+    {
+        final List<Integer> modifiersList = new ArrayList<Integer>(modifiers.length());
+        for (String modifier : modifiers.split(", ")) {
+            modifiersList.add(TokenTypes.getTokenId(modifier));
+        }
+        this.memberModifiers = modifiersList;
+    }
 }

src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheckTest.java

@@ -42,8 +42,8 @@ public void testDefaults() throws Exception
             "9:13: Declaring variables, return values or parameters of type "
                 + "'com.puppycrawl.tools.checkstyle.coding.InputIllegalType.AbstractClass'"
                 + " is not allowed.",
-            "16:13: Declaring variables, return values or parameters of type 'java.util.Hashtable' is not allowed.",
-            "17:13: Declaring variables, return values or parameters of type 'Hashtable' is not allowed.",
+            "16:13: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
+            "17:13: Declaring variables, return values or parameters of type 'TreeSet' is not allowed.",
         };
 
         verify(checkConfig, getPath("coding" + File.separator + "InputIllegalType.java"), expected);
@@ -59,7 +59,7 @@ public void testIgnoreMethodNames() throws Exception
             "9:13: Declaring variables, return values or parameters of type "
                 + "'com.puppycrawl.tools.checkstyle.coding.InputIllegalType.AbstractClass'"
                 + " is not allowed.",
-            "16:13: Declaring variables, return values or parameters of type 'java.util.Hashtable' is not allowed.",
+            "16:13: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
         };
 
         verify(checkConfig, getPath("coding" + File.separator + "InputIllegalType.java"), expected);
@@ -71,8 +71,8 @@ public void testFormat() throws Exception
         checkConfig.addAttribute("format", "^$");
 
         String[] expected = {
-            "16:13: Declaring variables, return values or parameters of type 'java.util.Hashtable' is not allowed.",
-            "17:13: Declaring variables, return values or parameters of type 'Hashtable' is not allowed.",
+            "16:13: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
+            "17:13: Declaring variables, return values or parameters of type 'TreeSet' is not allowed.",
         };
 
         verify(checkConfig, getPath("coding" + File.separator + "InputIllegalType.java"), expected);
@@ -87,8 +87,8 @@ public void testLegalAbstractClassNames() throws Exception
             "9:13: Declaring variables, return values or parameters of type "
                 + "'com.puppycrawl.tools.checkstyle.coding.InputIllegalType.AbstractClass'"
                 + " is not allowed.",
-            "16:13: Declaring variables, return values or parameters of type 'java.util.Hashtable' is not allowed.",
-            "17:13: Declaring variables, return values or parameters of type 'Hashtable' is not allowed.",
+            "16:13: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
+            "17:13: Declaring variables, return values or parameters of type 'TreeSet' is not allowed.",
         };
 
         verify(checkConfig, getPath("coding" + File.separator + "InputIllegalType.java"), expected);
@@ -152,4 +152,27 @@ public void testStaticImports() throws Exception
         verify(checkConfig, getPath("coding" + File.separator
                 + "InputIllegalTypeStaticImports.java"), expected);
     }
+
+    @Test
+    public void testMemberModifiers() throws Exception
+    {
+        checkConfig.addAttribute("memberModifiers", "LITERAL_PRIVATE, LITERAL_PROTECTED,"
+                + " LITERAL_STATIC");
+        String[] expected = {
+            "6:13: Declaring variables, return values or parameters of type 'AbstractClass' is not allowed.",
+            "9:13: Declaring variables, return values or parameters of type "
+                + "'com.puppycrawl.tools.checkstyle.coding.InputIllegalTypeMemberModifiers.AbstractClass'"
+                + " is not allowed.",
+            "16:13: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
+            "17:13: Declaring variables, return values or parameters of type 'TreeSet' is not allowed.",
+            "23:15: Declaring variables, return values or parameters of type "
+                    + "'com.puppycrawl.tools.checkstyle.coding.InputIllegalTypeMemberModifiers.AbstractClass'"
+                    + " is not allowed.",
+            "25:25: Declaring variables, return values or parameters of type 'java.util.TreeSet' is not allowed.",
+            "33:15: Declaring variables, return values or parameters of type 'AbstractClass' is not allowed.",
+        };
+
+        verify(checkConfig, getPath("coding" + File.separator
+                + "InputIllegalTypeMemberModifiers.java"), expected);
+    }
 }

src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalType.java

@@ -1,5 +1,5 @@
 package com.puppycrawl.tools.checkstyle.coding;
-
+import java.util.TreeSet;
 import java.util.Hashtable;
 //configuration: default
 public class InputIllegalType {
@@ -13,8 +13,8 @@
 
     private class NotAnAbstractClass {}
 
-    private java.util.Hashtable table1() { return null; } //WARNING
-    private Hashtable table2() { return null; } //WARNING
+    private java.util.TreeSet table1() { return null; } //WARNING
+    private TreeSet table2() { return null; } //WARNING
     static class SomeStaticClass {
 
     }

src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalTypeMemberModifiers.java

@@ -0,0 +1,34 @@
+package com.puppycrawl.tools.checkstyle.coding;
+import java.util.TreeSet;
+import java.util.Hashtable;
+//configuration: default
+public class InputIllegalTypeMemberModifiers {
+    private AbstractClass a = null; //WARNING
+    private NotAnAbstractClass b = null; /*another comment*/
+
+    private com.puppycrawl.tools.checkstyle.coding.InputIllegalTypeMemberModifiers.AbstractClass c = null; //WARNING
+    private com.puppycrawl.tools.checkstyle.coding.InputIllegalTypeMemberModifiers.NotAnAbstractClass d = null;
+
+    private abstract class AbstractClass {/*one more comment*/}
+
+    private class NotAnAbstractClass {}
+
+    private java.util.TreeSet table1() { return null; } //WARNING
+    private TreeSet table2() { return null; } //WARNING
+    static class SomeStaticClass {
+
+    }
+
+    //WARNING if memberModifiers is set and contains TokenTypes.LITERAL_PROTECTED
+    protected com.puppycrawl.tools.checkstyle.coding.InputIllegalTypeMemberModifiers.AbstractClass c1 = null;
+    //NO WARNING if memberModifiers is set and does not contain TokenTypes.LITERAL_PUBLIC
+    public final static java.util.TreeSet table3() { return null; }
+
+    java.util.TreeSet table4() { java.util.TreeSet treeSet = null; return null; }
+
+    private class Some {
+        java.util.TreeSet treeSet = null;
+    }
+    //WARNING if memberModifiers is set and contains TokenTypes.LITERAL_PROTECTED
+    protected AbstractClass a1 = null;
+}

src/xdocs/config_coding.xml

@@ -1750,11 +1750,9 @@ if ("something".equals(x))
             <td>Classes that should not be used as types in variable
             declarations, return values or parameters</td>
             <td><a href="property_types.html#stringSet">String Set</a></td>
-            <td>&quot;java.util.GregorianCalendar, java.util.Hashtable,
-            java.util.HashSet, java.util.HashMap, java.util.ArrayList,
-            java.util.LinkedList, java.util.LinkedHashMap,
+            <td>&quot;java.util.HashSet, java.util.HashMap, java.util.LinkedHashMap,
             java.util.LinkedHashSet, java.util.TreeSet,
-            java.util.TreeMap, java.util.Vector&quot;</td>
+            java.util.TreeMap&quot;</td>
           </tr>
           <tr>
           <td>legalAbstractClassNames</td>
@@ -1774,6 +1772,12 @@ if (&quot;something&quot;.equals(x))
             <td><a href="property_types.html#regexp">regular expression</a></td>
             <td><code>^(.*[\\.])?Abstract.*$</code></td>
           </tr>
+          <tr>
+            <td>memberModifiers</td>
+            <td>Check methods and fields with only corresponding modifiers.</td>
+            <td><a href="property_types.html#intSet">List of integers</a></td>
+            <td><code>null</code></td>
+          </tr>
         </table>
       </subsection>
 
@@ -1784,6 +1788,16 @@ if (&quot;something&quot;.equals(x))
         <source>
 &lt;module name=&quot;IllegalType&quot;&gt;
     &lt;property name=&quot;ignoredMethodNames&quot; value=&quot;getInstance&quot;/&gt;
+&lt;/module&gt;
+        </source>
+        <p>
+          To configure the Check so that it verifies only public, protected and static
+           methods and fields:
+        </p>
+        <source>
+&lt;module name=&quot;IllegalType&quot;&gt;
+    &lt;property name=&quot;memberModifiers&quot; value=&quot;LITERAL_PUBLIC,
+     LITERAL_PROTECTED, LITERAL_STATIC&quot;/&gt;
 &lt;/module&gt;
         </source>
       </subsection>