From f9f60d3142533c3eebff775e8eb1159935faaba1 Mon Sep 17 00:00:00 2001 From: alexkravin Date: Thu, 12 Feb 2015 16:31:11 +0400 Subject: [PATCH] Visibility Modifier Check, added option allows public immutable fields, issue #61 --- .../design/VisibilityModifierCheck.java | 543 ++++++++++++++++-- .../design/VisibilityModifierCheckTest.java | 88 +++ .../tools/checkstyle/InetSocketAddress.java | 8 + .../tools/checkstyle/InputImmutable.java | 39 ++ .../InputImmutableSameTypeName.java | 12 + .../checkstyle/InputImmutableStarImport.java | 10 + .../checkstyle/InputImmutableStarImport2.java | 8 + src/xdocs/config_design.xml | 110 +++- 8 files changed, 758 insertions(+), 60 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InetSocketAddress.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutable.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableSameTypeName.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport.java create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport2.java diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java index 4aa61e5c1..40d5a8ca3 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java @@ -18,20 +18,27 @@ //////////////////////////////////////////////////////////////////////////////// package com.puppycrawl.tools.checkstyle.checks.design; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; + +import org.apache.commons.beanutils.ConversionException; + import antlr.collections.AST; -import com.google.common.collect.Sets; + +import com.google.common.collect.ImmutableList; import com.puppycrawl.tools.checkstyle.api.Check; import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.ScopeUtils; import com.puppycrawl.tools.checkstyle.api.TokenTypes; import com.puppycrawl.tools.checkstyle.api.Utils; -import java.util.Set; -import java.util.regex.Pattern; -import java.util.regex.PatternSyntaxException; -import org.apache.commons.beanutils.ConversionException; /** - * Checks visibility of class members. Only static final members may be public, + * Checks visibility of class members. Only static final or immutable members may be public, * other class members must be private unless allowProtected/Package is set. *

* Public members are not flagged if the name matches the public @@ -39,8 +46,121 @@ import org.apache.commons.beanutils.ConversionException; * default). *

* Rationale: Enforce encapsulation. + *

+ * Check also has an option making it less strict: + *

+ *

+ * allowPublicImmutableFields - which allows immutable fields be + * declared as public if defined in final class. Default value is true + *

+ *

+ * Field is known to be immutable if: + *

+ *

+ *

+ * Classes known to be immutable are listed in immutableClassCanonicalNames by their + * canonical names. List by default: + *

+ * User can override this list via adding canonical class names to + * immutableClassCanonicalNames, if user will provide short class name all + * that type will match to any named the same type without consideration of package. + *

+ *

+ * Rationale: Forcing all fields of class to have private modified by default is good + * in most cases, but in some cases it drawbacks in too much boilerplate get/set code. + * One of such cases are immutable classes. + *

+ *

+ * Restriction: Check doesn't check if class is immutable, there's no checking + * if accessory methods are missing and all fields are immutable, we only check + * if current field is immutable by matching a name to user defined list of immutable classes + * and defined in final class + *

+ *

+ * Star imports are out of scope of this Check. So if one of type imported via star import + * collides with user specified one by its short name - there won't be Check's violation. + *

+ * Examples: + *

+ * Default Check's configuration will pass the code below: + *

+ *

+ * 

+ * 
+ * public final class ImmutableClass
+ * {
+ *     public final int intValue; // No warning
+ *     public final java.lang.String notes; // No warning
+ *     public final BigDecimal value; // No warning
  *
- * @author lkuehne
+ *     public ImmutableClass(int intValue, BigDecimal value, String notes)
+ *     {
+ *         this.includes = ImmutableSet.copyOf(includes);
+ *         this.excludes = ImmutableSet.copyOf(excludes);
+ *         this.value = value;
+ *         this.notes = notes;
+ *     }
+ * }
+ * 
+ *
+ *

+ * To configure the Check passing fields of type com.google.common.collect.ImmutableSet and + * java.util.List: + *

+ *

+ * <module name="VisibilityModifier"> + * <property name="immutableClassCanonicalNames" value="java.util.List, + * com.google.common.collect.ImmutableSet"/> + * </module> + *

+ *

+ * 

+ * 
+ * public final class ImmutableClass
+ * {
+ *     public final ImmutableSet<String> includes; // No warning
+ *     public final ImmutableSet<String> excludes; // No warning
+ *     public final BigDecimal value; // Warning here, type BigDecimal isn't specified as immutable
+ *
+ *     public ImmutableClass(Collection<String> includes, Collection<String> excludes,
+ *                  BigDecimal value)
+ *     {
+ *         this.includes = ImmutableSet.copyOf(includes);
+ *         this.excludes = ImmutableSet.copyOf(excludes);
+ *         this.value = value;
+ *         this.notes = notes;
+ *     }
+ * }
+ * 
+ *
+ *

+ * + * @author Aleksey Nesterenko */ public class VisibilityModifierCheck extends Check @@ -68,13 +188,44 @@ public class VisibilityModifierCheck private String publicMemberFormat = "^serialVersionUID$"; /** regexp for public members that should be ignored */ - private Pattern publicMemberPattern; + private Pattern publicMemberPattern = Pattern.compile(publicMemberFormat); - /** Create an instance. */ - public VisibilityModifierCheck() - { - setPublicMemberPattern(publicMemberFormat); - } + /** Allows immutable fields to be declared as public. */ + private boolean allowPublicImmutableFields = true; + + /** List of immutable classes canonical names. */ + private List immutableClassCanonicalNames = new ArrayList<>(DEFAULT_IMMUTABLE_TYPES); + + /** List of immutable classes short names. */ + private final List immutableClassShortNames = + getClassShortNames(DEFAULT_IMMUTABLE_TYPES); + + /** Default immutable types canonical names. */ + private static final List DEFAULT_IMMUTABLE_TYPES = ImmutableList.of( + "java.lang.String", + "java.lang.Integer", + "java.lang.Byte", + "java.lang.Character", + "java.lang.Short", + "java.lang.Boolean", + "java.lang.Long", + "java.lang.Double", + "java.lang.Float", + "java.lang.StackTraceElement", + "java.math.BigInteger", + "java.math.BigDecimal", + "java.io.File", + "java.util.Locale", + "java.util.UUID", + "java.net.URL", + "java.net.URI", + "java.net.Inet4Address", + "java.net.Inet6Address", + "java.net.InetSocketAddress" + ); + + /** contains explicit access modifiers. */ + private static final String[] EXPLICIT_MODS = {"public", "private", "protected"}; /** @return whether protected members are allowed */ public boolean isProtectedAllowed() @@ -129,85 +280,214 @@ public class VisibilityModifierCheck return publicMemberPattern; } + /** + * Sets whether public immutable are allowed. + * @param allow user's value. + */ + public void setAllowPublicImmutableFields(boolean allow) + { + this.allowPublicImmutableFields = allow; + } + + /** + * Set the list of immutable classes types names. + * @param classNames array of immutable types short names. + */ + public void setImmutableClassNames(String[] classNames) + { + immutableClassCanonicalNames = Arrays.asList(classNames); + } + @Override public int[] getDefaultTokens() { - return new int[] {TokenTypes.VARIABLE_DEF}; + return new int[] { + TokenTypes.VARIABLE_DEF, + TokenTypes.IMPORT, + }; } @Override public int[] getAcceptableTokens() { - return new int[] {TokenTypes.VARIABLE_DEF, TokenTypes.OBJBLOCK}; + return new int[] { + TokenTypes.VARIABLE_DEF, + TokenTypes.OBJBLOCK, + TokenTypes.IMPORT, + }; + } + + @Override + public void beginTree(DetailAST rootAst) + { + immutableClassShortNames.clear(); + final List shortNames = getClassShortNames(immutableClassCanonicalNames); + immutableClassShortNames.addAll(shortNames); } @Override public void visitToken(DetailAST ast) { - if ((ast.getType() != TokenTypes.VARIABLE_DEF) - || (ast.getParent().getType() != TokenTypes.OBJBLOCK)) - { - return; + switch (ast.getType()) { + case TokenTypes.VARIABLE_DEF: + if (!isAnonymousClassVariable(ast)) { + visitVariableDef(ast); + } + break; + case TokenTypes.IMPORT: + visitImport(ast); + break; + default: + final String exceptionMsg = "Unexpected token type: " + ast.getText(); + throw new IllegalArgumentException(exceptionMsg); } + } - final DetailAST varNameAST = getVarNameAST(ast); - final String varName = varNameAST.getText(); + /** + * Checks if current variable definition is definition of an anonymous class. + * @param variableDef {@link TokenTypes#VARIABLE_DEF VARIABLE_DEF} + * @return true if current variable definition is definition of an anonymous class. + */ + private static boolean isAnonymousClassVariable(DetailAST variableDef) + { + return variableDef.getParent().getType() != TokenTypes.OBJBLOCK; + } + + /** + * Checks access modifier of given variable. + * If it is not proper according to Check - puts violation on it. + * @param variableDef variable to check. + */ + private void visitVariableDef(DetailAST variableDef) + { final boolean inInterfaceOrAnnotationBlock = - ScopeUtils.inInterfaceOrAnnotationBlock(ast); - final Set mods = getModifiers(ast); - final String declaredScope = getVisibilityScope(mods); - final String variableScope = - inInterfaceOrAnnotationBlock ? "public" : declaredScope; + ScopeUtils.inInterfaceOrAnnotationBlock(variableDef); - if (!("private".equals(variableScope) - || inInterfaceOrAnnotationBlock // implicitly static and final - || (mods.contains("static") && mods.contains("final")) - || ("package".equals(variableScope) && isPackageAllowed()) - || ("protected".equals(variableScope) && isProtectedAllowed()) - || ("public".equals(variableScope) - && getPublicMemberRegexp().matcher(varName).find()))) - { - log(varNameAST.getLineNo(), varNameAST.getColumnNo(), - MSG_KEY, varName); + if (!inInterfaceOrAnnotationBlock) { + final DetailAST varNameAST = getVarNameAST(variableDef); + final String varName = varNameAST.getText(); + if (!hasProperAccessModifier(variableDef, varName)) { + log(varNameAST.getLineNo(), varNameAST.getColumnNo(), + MSG_KEY, varName); + } } } + /** + * Checks imported type. If type's canonical name was not specified in + * immutableClassCanonicalNames, but it's short name collides with one from + * immutableClassShortNames - removes it from the last one. + * @param importAst {@link TokenTypes#IMPORT Import} + */ + private void visitImport(DetailAST importAst) + { + if (!isStarImport(importAst)) { + final DetailAST type = importAst.getFirstChild(); + final String canonicalName = getCanonicalName(type); + final String shortName = getClassShortName(canonicalName); + + // If imported canonical class name is not specified as allowed immutable class, + // but its short name collides with one of specified class - removes the short name + // from list to avoid names collision + if (!immutableClassCanonicalNames.contains(canonicalName) + && immutableClassShortNames.contains(shortName)) + { + immutableClassShortNames.remove(shortName); + } + } + } + + /** + * Checks if current import is star import. E.g.: + *

+ * + * import java.util.*; + * + *

+ * @param importAst {@link TokenTypes#IMPORT Import} + * @return true if it is star import + */ + private static boolean isStarImport(DetailAST importAst) + { + boolean result = false; + DetailAST toVisit = importAst; + while (toVisit != null) { + toVisit = getNextSubTreeNode(toVisit, importAst); + if (toVisit != null && toVisit.getType() == TokenTypes.STAR) { + result = true; + break; + } + } + return result; + } + + /** + * Checks if current variable has proper access modifier according to Check's options. + * @param variableDef Variable definition node. + * @param variableName Variable's name. + * @return true if variable has proper access modifier. + */ + private boolean hasProperAccessModifier(DetailAST variableDef, String variableName) + { + boolean result = true; + + final Set mods = getModifiers(variableDef); + final String variableScope = getVisibilityScope(mods); + + if (!"private".equals(variableScope)) { + final DetailAST classDef = variableDef.getParent().getParent(); + final Set classModifiers = getModifiers(classDef); + + result = + (mods.contains("static") && mods.contains("final")) + || (isPackageAllowed() && "package".equals(variableScope)) + || (isProtectedAllowed() && "protected".equals(variableScope)) + || ("public".equals(variableScope) + && getPublicMemberRegexp().matcher(variableName).find() + || (allowPublicImmutableFields + && classModifiers.contains("final") && isImmutableField(variableDef))); + } + + return result; + } + /** * Returns the variable name in a VARIABLE_DEF AST. * @param variableDefAST an AST where type == VARIABLE_DEF AST. * @return the variable name in variableDefAST */ - private DetailAST getVarNameAST(DetailAST variableDefAST) + private static DetailAST getVarNameAST(DetailAST variableDefAST) { DetailAST ast = variableDefAST.getFirstChild(); + DetailAST varNameAst = null; while (ast != null) { final DetailAST nextSibling = ast.getNextSibling(); if (ast.getType() == TokenTypes.TYPE) { - return nextSibling; + varNameAst = nextSibling; + break; } ast = nextSibling; } - return null; + return varNameAst; } /** - * Returns the set of modifier Strings for a VARIABLE_DEF AST. - * @param variableDefAST AST for a vraiable definition - * @return the set of modifier Strings for variableDefAST + * Returns the set of modifier Strings for a VARIABLE_DEF or CLASS_DEF AST. + * @param defAST AST for a variable or class definition. + * @return the set of modifier Strings for defAST. */ - private Set getModifiers(DetailAST variableDefAST) + private static Set getModifiers(DetailAST defAST) { - final AST modifiersAST = variableDefAST.getFirstChild(); - if (modifiersAST.getType() != TokenTypes.MODIFIERS) { - throw new IllegalStateException("Strange parse tree"); + final AST modifiersAST = defAST.findFirstToken(TokenTypes.MODIFIERS); + final Set modifiersSet = new HashSet<>(); + if (modifiersAST != null) { + AST modifier = modifiersAST.getFirstChild(); + while (modifier != null) { + modifiersSet.add(modifier.getText()); + modifier = modifier.getNextSibling(); + } } - final Set retVal = Sets.newHashSet(); - AST modifier = modifiersAST.getFirstChild(); - while (modifier != null) { - retVal.add(modifier.getText()); - modifier = modifier.getNextSibling(); - } - return retVal; + return modifiersSet; } @@ -216,14 +496,159 @@ public class VisibilityModifierCheck * @param modifiers the set of modifier Strings * @return one of "public", "private", "protected", "package" */ - private String getVisibilityScope(Set modifiers) + private static String getVisibilityScope(Set modifiers) { - final String[] explicitModifiers = {"public", "private", "protected"}; - for (final String candidate : explicitModifiers) { - if (modifiers.contains(candidate)) { - return candidate; + String accessModifier = "package"; + for (final String modifier : EXPLICIT_MODS) { + if (modifiers.contains(modifier)) { + accessModifier = modifier; + break; } } - return "package"; + return accessModifier; } + + /** + * Checks if current field is immutable: + * has final modifier and either a primitive type or instance of class + * known to be immutable (such as String, ImmutableCollection from Guava and etc). + * Classes known to be immutable are listed in + * {@link VisibilityModifierCheck#immutableClassCanonicalNames} + * @param variableDef Field in consideration. + * @return true if field is immutable. + */ + private boolean isImmutableField(DetailAST variableDef) + { + boolean result = false; + + final DetailAST modifiers = variableDef.findFirstToken(TokenTypes.MODIFIERS); + final boolean isFinal = modifiers.branchContains(TokenTypes.FINAL); + if (isFinal) { + final DetailAST type = variableDef.findFirstToken(TokenTypes.TYPE); + final boolean isCanonicalName = type.getFirstChild().getType() == TokenTypes.DOT; + final String typeName = getTypeName(type, isCanonicalName); + + result = (!isCanonicalName && isPrimitive(type)) + || immutableClassShortNames.contains(typeName) + || (isCanonicalName && immutableClassCanonicalNames.contains(typeName)); + } + return result; + } + + /** + * Gets the name of type from given ast {@link TokenTypes#TYPE TYPE} node. + * If type is specified via its canonical name - canonical name will be returned, + * else - short type's name. + * @param type {@link TokenTypes#TYPE TYPE} node. + * @param isCanonicalName is given name canonical. + * @return String representation of given type's name. + */ + private static String getTypeName(DetailAST type, boolean isCanonicalName) + { + String typeName = ""; + if (isCanonicalName) { + typeName = getCanonicalName(type); + } + else { + typeName = type.getFirstChild().getText(); + } + return typeName; + } + + /** + * Checks if current type is primitive type (int, short, float, boolean, double, etc.). + * As primitive types have special tokens for each one, such as: + * LITERAL_INT, LITERAL_BOOLEAN, etc. + * So, if type's identifier differs from {@link TokenTypes#IDENT IDENT} token - it's a + * primitive type. + * @param type Ast {@link TokenTypes#TYPE TYPE} node. + * @return true if current type is primitive type. + */ + private static boolean isPrimitive(DetailAST type) + { + return type.getFirstChild().getType() != TokenTypes.IDENT; + } + + /** + * Gets canonical type's name from given {@link TokenTypes#TYPE TYPE} node. + * @param type DetailAST {@link TokenTypes#TYPE TYPE} node. + * @return canonical type's name + */ + private static String getCanonicalName(DetailAST type) + { + final StringBuilder canonicalNameBuilder = new StringBuilder(); + DetailAST toVisit = type.getFirstChild(); + while (toVisit != null) { + toVisit = getNextSubTreeNode(toVisit, type); + if (toVisit != null && toVisit.getType() == TokenTypes.IDENT) { + canonicalNameBuilder.append(toVisit.getText()); + final DetailAST nextSubTreeNode = getNextSubTreeNode(toVisit, + type); + if (nextSubTreeNode != null && nextSubTreeNode.getType() != TokenTypes.SEMI) { + canonicalNameBuilder.append('.'); + } + } + } + return canonicalNameBuilder.toString(); + } + + /** + * Gets the next node of a syntactical tree (child of a current node or + * sibling of a current node, or sibling of a parent of a current node) + * @param currentNodeAst Current node in considering + * @param subTreeRootAst SubTree root + * @return Current node after bypassing, if current node reached the root of a subtree + * method returns null + */ + private static DetailAST + getNextSubTreeNode(DetailAST currentNodeAst, DetailAST subTreeRootAst) + { + DetailAST currentNode = currentNodeAst; + DetailAST toVisitAst = currentNode.getFirstChild(); + while (toVisitAst == null) { + toVisitAst = currentNode.getNextSibling(); + if (toVisitAst == null) { + if (currentNode.getParent().equals(subTreeRootAst) + && currentNode.getParent().getColumnNo() == subTreeRootAst.getColumnNo()) + { + break; + } + currentNode = currentNode.getParent(); + } + } + currentNode = toVisitAst; + return currentNode; + } + + /** + * Gets the list with short names classes. + * These names are taken from array of classes canonical names. + * @param canonicalClassNames canonical class names. + * @return the list of short names of classes. + */ + private static List getClassShortNames(List canonicalClassNames) + { + final List shortNames = new ArrayList<>(); + for (String canonicalClassName : canonicalClassNames) { + final String shortClassName = canonicalClassName + .substring(canonicalClassName.lastIndexOf(".") + 1, + canonicalClassName.length()); + shortNames.add(shortClassName); + } + return shortNames; + } + + /** + * Gets the short class name from given canonical name. + * @param canonicalClassName canonical class name. + * @return short name of class. + */ + private static String getClassShortName(String canonicalClassName) + { + final String shortClassName = canonicalClassName + .substring(canonicalClassName.lastIndexOf(".") + 1, + canonicalClassName.length()); + return shortClassName; + } + } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java index 0b9c69f0d..0f08d292f 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java @@ -91,4 +91,92 @@ public class VisibilityModifierCheckTest }; verify(getChecker(), getPath("InputPublicOnly.java"), expected); } + + @Test + public void testAllowPublicFinalFieldsInImmutableClass() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + final String[] expected = { + "12:39: " + getCheckMessage(MSG_KEY, "includes"), + "13:39: " + getCheckMessage(MSG_KEY, "excludes"), + "16:23: " + getCheckMessage(MSG_KEY, "list"), + "34:20: " + getCheckMessage(MSG_KEY, "value"), + "36:24: " + getCheckMessage(MSG_KEY, "bValue"), + "37:31: " + getCheckMessage(MSG_KEY, "longValue"), + }; + verify(checkConfig, getPath("InputImmutable.java"), expected); + } + + @Test + public void testUserSpecifiedImmutableClassesList() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("immutableClassNames", "java.util.List," + + "com.google.common.collect.ImmutableSet"); + final String[] expected = { + "14:35: " + getCheckMessage(MSG_KEY, "notes"), + "15:29: " + getCheckMessage(MSG_KEY, "value"), + "32:35: " + getCheckMessage(MSG_KEY, "uri"), + "33:35: " + getCheckMessage(MSG_KEY, "file"), + "34:20: " + getCheckMessage(MSG_KEY, "value"), + "35:35: " + getCheckMessage(MSG_KEY, "url"), + "36:24: " + getCheckMessage(MSG_KEY, "bValue"), + "37:31: " + getCheckMessage(MSG_KEY, "longValue"), + }; + verify(checkConfig, getPath("InputImmutable.java"), expected); + } + + @Test + public void testImmutableSpecifiedSameTypeName() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("immutableClassNames", + "com.puppycrawl.tools.checkstyle.coding.GregorianCalendar," + + "com.puppycrawl.tools.checkstyle.InetSocketAddress"); + final String[] expected = { + "7:46: " + getCheckMessage(MSG_KEY, "calendar"), + "11:45: " + getCheckMessage(MSG_KEY, "adr"), + }; + verify(checkConfig, getPath("InputImmutableSameTypeName.java"), expected); + } + + @Test + public void testImmutableDefaultValueSameTypeName() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + final String[] expected = { + "7:46: " + getCheckMessage(MSG_KEY, "calendar"), + "8:36: " + getCheckMessage(MSG_KEY, "calendar2"), + "9:75: " + getCheckMessage(MSG_KEY, "calendar3"), + "10:36: " + getCheckMessage(MSG_KEY, "address"), + }; + verify(checkConfig, getPath("InputImmutableSameTypeName.java"), expected); + } + + @Test + public void testImmutableStarImportFalseNegative() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("immutableClassNames", "java.util.Arrays"); + final String[] expected = { + }; + verify(checkConfig, getPath("InputImmutableStarImport.java"), expected); + } + + @Test + public void testImmutableStarImportNoWarn() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(VisibilityModifierCheck.class); + checkConfig.addAttribute("immutableClassNames", "com.google.common.collect.ImmutableSet"); + final String[] expected = { + }; + verify(checkConfig, getPath("InputImmutableStarImport2.java"), expected); + } + } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InetSocketAddress.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InetSocketAddress.java new file mode 100644 index 000000000..177f9cd9a --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InetSocketAddress.java @@ -0,0 +1,8 @@ +package com.puppycrawl.tools.checkstyle; + +public class InetSocketAddress +{ + class Arrays { + + } +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutable.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutable.java new file mode 100644 index 000000000..38f782321 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutable.java @@ -0,0 +1,39 @@ +package com.puppycrawl.tools.checkstyle; + +import java.math.BigDecimal; +import java.util.Collection; +import java.util.List; + +import com.google.common.collect.ImmutableSet; + +public final class InputImmutable +{ + public final int someIntValue; + public final ImmutableSet includes; + public final ImmutableSet excludes; + public final java.lang.String notes; + public final BigDecimal value; + public final List list; + + public InputImmutable(Collection includes, Collection excludes, + BigDecimal value, String notes, int someValue, List l) { + this.includes = ImmutableSet.copyOf(includes); + this.excludes = ImmutableSet.copyOf(excludes); + this.value = value; + this.notes = notes; + this.someIntValue = someValue; + this.list = l; + } + + final class Immutable + { + public final float f = 4; + public final boolean bool = false; + public final java.net.URI uri = null; + public final java.io.File file = null; + public int value = 42; + public final java.net.URL url = null; + public boolean bValue = false; + public java.lang.Long longValue = 1L; + } +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableSameTypeName.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableSameTypeName.java new file mode 100644 index 000000000..2f1da876b --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableSameTypeName.java @@ -0,0 +1,12 @@ +package com.puppycrawl.tools.checkstyle; + +import com.puppycrawl.tools.checkstyle.coding.GregorianCalendar; +import com.puppycrawl.tools.checkstyle.InetSocketAddress; +public final class InputImmutableSameTypeName +{ + public final java.util.GregorianCalendar calendar = null; + public final GregorianCalendar calendar2 = null; + public final com.puppycrawl.tools.checkstyle.coding.GregorianCalendar calendar3 = null; + public final InetSocketAddress address = null; + public final java.net.InetSocketAddress adr = null; +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport.java new file mode 100644 index 000000000..40a442438 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport.java @@ -0,0 +1,10 @@ +package com.puppycrawl.tools.checkstyle; + +import com.puppycrawl.tools.checkstyle.InputImmutable; +import com.puppycrawl.tools.checkstyle.InetSocketAddress.*; + +public final class InputImmutableStarImport +{ + public final Arrays f = null; // If Arrays is specified as immutable class, no matter of canonical name + // no warning will be here, star imports are out of consideration +} diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport2.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport2.java new file mode 100644 index 000000000..aecc639b0 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputImmutableStarImport2.java @@ -0,0 +1,8 @@ +package com.puppycrawl.tools.checkstyle; + +import com.google.common.collect.*; +//config.immutableClassName=com.google.google.common.ImmutableSet +public final class InputImmutableStarImport2 +{ + public final ImmutableSet set = null; // No warning here +} diff --git a/src/xdocs/config_design.xml b/src/xdocs/config_design.xml index 3e009ea5e..88559cd9a 100644 --- a/src/xdocs/config_design.xml +++ b/src/xdocs/config_design.xml @@ -13,7 +13,7 @@

- Checks visibility of class members. Only static final members + Checks visibility of class members. Only static final or immutable members may be public; other class members must be private unless the property protectedAllowed or packageAllowed is set.

@@ -32,6 +32,38 @@

Rationale: Enforce encapsulation.

+

+ Check also has an option making it less strict: +

+

+ allowPublicImmutableFields - which allows immutable fields be declared as + public if defined in final class. Default value is true +

+

+ Field is known to be immutable if: + - It's declared as final + - Has either a primitive type or instance of class user defined to be immutable + (such as String, ImmutableCollection from Guava and etc) +

+

+ Classes known to be immutable are listed in immutableClassCanonicalNames by their + canonical names. +

+

+ Rationale: Forcing all fields of class to have private modified by default is good + in most cases, but in some cases it drawbacks in too much boilerplate get/set code. + One of such cases are immutable classes. +

+

+ Restriction: Check doesn't check if class is immutable, there's no + checking if accessory methods are missing and all fields are immutable, we only check + if current field is immutable and defined in final class +

+

+ Star imports are out of scope of this Check. So if one of type imported via + star import collides with user specified one by its short name - + there won't be Check's violation. +

@@ -60,6 +92,22 @@ regular expression ^serialVersionUID$ + + allowPublicFinalFields + allows field with only final modifier be public + boolean + true + + + immutableClassCanonicalNames + immutable classes canonical names + String Set + java.lang.String, java.lang.Integer, java.lang.Byte, java.lang.Character, + java.lang.Short, java.lang.Boolean, java.lang.Long, java.lang.Double, java.lang.Float, + java.lang.StackTraceElement, java.math.BigInteger, java.math.BigDecimal, java.io.File, + java.util.Locale, java.util.UUID, java.net.URL, java.net.URI, + java.net.Inet4Address, java.net.Inet6Address, java.net.InetSocketAddress, + @@ -88,6 +136,66 @@ <property name="publicMemberPattern" value="^$"/> </module> +

+ To configure the Check so that it allows public immutable fields + (mostly for immutable classes): +

+ +<module name="VisibilityModifier"/> + +

+ Example of allowed public immutable fields: +

+ +public class ImmutableClass +{ + public final ImmutableSet<String> includes; // No warning + public final ImmutableSet<String> excludes; // No warning + public final java.lang.String notes; // No warning + public final BigDecimal value; // No warning + + public ImmutableClass(Collection<String> includes, Collection<String> excludes, + BigDecimal value, String notes) + { + this.includes = ImmutableSet.copyOf(includes); + this.excludes = ImmutableSet.copyOf(excludes); + this.value = value; + this.notes = notes; + } +} + +

+ To configure the Check which allows user specified immutable class names: +

+ +<module name="VisibilityModifier"> + <property name="immutableClassCanonicalNames" value=" + com.google.common.collect.ImmutableSet"/> +</module> + +

+ Example of allowed public immutable fields: +

+ +public class ImmutableClass +{ + public final ImmutableSet<String> includes; // No warning + public final ImmutableSet<String> excludes; // No warning + public final java.lang.String notes; // Warning here because + //'java.lang.String' wasn't specified as allowed class + public final int someValue; // No warning + + public ImmutableClass(Collection<String> includes, Collection<String> excludes, + String notes, int someValue) + { + this.includes = ImmutableSet.copyOf(includes); + this.excludes = ImmutableSet.copyOf(excludes); + this.value = value; + this.notes = notes; + this.someValue = someValue; + } +} +