From 677acc1e8491acffd24c859e553b06362da0d471 Mon Sep 17 00:00:00 2001 From: alexkravin Date: Fri, 30 Jan 2015 15:38:49 +0400 Subject: [PATCH] Illegal Type Check, updated default illegal types, added memberModifiers option, issue #567 --- .../checks/coding/IllegalTypeCheck.java | 79 ++++++++++++++++--- .../checks/coding/IllegalTypeCheckTest.java | 37 +++++++-- .../checkstyle/coding/InputIllegalType.java | 6 +- .../InputIllegalTypeMemberModifiers.java | 34 ++++++++ src/xdocs/config_coding.xml | 22 +++++- 5 files changed, 151 insertions(+), 27 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalTypeMemberModifiers.java diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java index 7d750a11f..380dc565a 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java @@ -24,6 +24,9 @@ import com.puppycrawl.tools.checkstyle.api.FullIdent; 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 @@ import java.util.Set; * will be ok. *

*

- * ignoredMethodNames - Methods that should not be checked.. + * ignoredMethodNames - Methods that should not be checked. + *

+ *

+ * memberModifiers - To check only methods and fields with only specified modifiers. + *

+ *

+ * In most cases it's justified to put following classes to illegalClassNames: + *

+ * as methods that are differ from interface methods are rear used, so in most cases user will + * benefit from checking for them. *

* * @author Simon Harris @@ -80,28 +98,18 @@ public final class IllegalTypeCheck extends AbstractFormatCheck 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 @@ public final class IllegalTypeCheck extends AbstractFormatCheck private final Set legalAbstractClassNames = Sets.newHashSet(); /** methods which should be ignored. */ private final Set ignoredMethodNames = Sets.newHashSet(); + /** check methods and fields with only corresponding modifiers. */ + private List memberModifiers; /** Creates new instance of the check. */ public IllegalTypeCheck() @@ -142,10 +152,14 @@ public final class IllegalTypeCheck extends AbstractFormatCheck { 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 final class IllegalTypeCheck extends AbstractFormatCheck } } + /** + * Checks if current method's return type or variable's type is verifiable + * according to memberModifiers option. + * @param methodOrVariableDef METHOD_DEF or VARIABLE_DEF ast node. + * @return true if member is verifiable according to memberModifiers 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 final class IllegalTypeCheck extends AbstractFormatCheck 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 modifiersList = new ArrayList(modifiers.length()); + for (String modifier : modifiers.split(", ")) { + modifiersList.add(TokenTypes.getTokenId(modifier)); + } + this.memberModifiers = modifiersList; + } } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheckTest.java index 4e242e7b6..2c73bffee 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheckTest.java @@ -42,8 +42,8 @@ public class IllegalTypeCheckTest extends BaseCheckTestSupport "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 class IllegalTypeCheckTest extends BaseCheckTestSupport "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 class IllegalTypeCheckTest extends BaseCheckTestSupport 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 class IllegalTypeCheckTest extends BaseCheckTestSupport "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 class IllegalTypeCheckTest extends BaseCheckTestSupport 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); + } } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalType.java b/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalType.java index 87e5b4b64..10c69b618 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalType.java +++ b/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 @@ public class InputIllegalType { 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 { } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalTypeMemberModifiers.java b/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputIllegalTypeMemberModifiers.java new file mode 100644 index 000000000..21d70432b --- /dev/null +++ b/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; +} diff --git a/src/xdocs/config_coding.xml b/src/xdocs/config_coding.xml index fe062ddf0..12ab01be3 100644 --- a/src/xdocs/config_coding.xml +++ b/src/xdocs/config_coding.xml @@ -1750,11 +1750,9 @@ if ("something".equals(x)) Classes that should not be used as types in variable declarations, return values or parameters String Set - "java.util.GregorianCalendar, java.util.Hashtable, - java.util.HashSet, java.util.HashMap, java.util.ArrayList, - java.util.LinkedList, java.util.LinkedHashMap, + "java.util.HashSet, java.util.HashMap, java.util.LinkedHashMap, java.util.LinkedHashSet, java.util.TreeSet, - java.util.TreeMap, java.util.Vector" + java.util.TreeMap" legalAbstractClassNames @@ -1774,6 +1772,12 @@ if ("something".equals(x)) regular expression ^(.*[\\.])?Abstract.*$ + + memberModifiers + Check methods and fields with only corresponding modifiers. + List of integers + null + @@ -1784,6 +1788,16 @@ if ("something".equals(x)) <module name="IllegalType"> <property name="ignoredMethodNames" value="getInstance"/> +</module> + +

+ To configure the Check so that it verifies only public, protected and static + methods and fields: +

+ +<module name="IllegalType"> + <property name="memberModifiers" value="LITERAL_PUBLIC, + LITERAL_PROTECTED, LITERAL_STATIC"/> </module>