Skip to content

Commit

Permalink
Issue checkstyle#2971: Add allowPublicFinalFields option for Visibili…
Browse files Browse the repository at this point in the history 
…tyModifier
  • Loading branch information
@MEZk
MEZk committed May 23, 2016
1 parent 91dd77d commit 4c8690a
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 8 deletions.
33 changes: 28 additions & 5 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java
View file
Expand Up @@ -72,6 +72,10 @@
* </pre>
*
* <p>
* <b>allowPublicFinalFields</b> - which allows immutable fields be
* declared as public. Default value is <b>true</b>
* </p>
* <p>
* <b>allowPublicImmutableFields</b> - which allows immutable fields be
* declared as public if defined in final class. Default value is <b>true</b>
* </p>
Expand Down Expand Up @@ -328,9 +332,12 @@ public class VisibilityModifierCheck
/** Whether package visible members are allowed. */
private boolean packageAllowed;

/** Allows immutable fields to be declared as public. */
/** Allows immutable fields of final classes to be declared as public. */
private boolean allowPublicImmutableFields = true;

/** Allows final fields to be declared as public. */
private boolean allowPublicFinalFields = true;

/** List of immutable classes canonical names. */
private List<String> immutableClassCanonicalNames = new ArrayList<>(DEFAULT_IMMUTABLE_TYPES);

Expand Down Expand Up @@ -371,13 +378,21 @@ public void setPublicMemberPattern(String pattern) {
}

/**
* Sets whether public immutable are allowed.
* Sets whether public immutable fields are allowed.
* @param allow user's value.
*/
public void setAllowPublicImmutableFields(boolean allow) {
allowPublicImmutableFields = allow;
}

/**
* Sets whether public final fields are allowed.
* @param allow user's value.
*/
public void setAllowPublicFinalFields(boolean allow) {
allowPublicFinalFields = allow;
}

/**
* Set the list of immutable classes types names.
* @param classNames array of immutable types canonical names.
Expand Down Expand Up @@ -540,8 +555,7 @@ private boolean hasProperAccessModifier(DetailAST variableDef, String variableNa
|| packageAllowed && PACKAGE_ACCESS_MODIFIER.equals(variableScope)
|| protectedAllowed && PROTECTED_ACCESS_MODIFIER.equals(variableScope)
|| isIgnoredPublicMember(variableName, variableScope)
|| allowPublicImmutableFields
&& isImmutableFieldDefinedInFinalClass(variableDef);
|| isAllowedPublicField(variableDef);
}

return result;
Expand Down Expand Up @@ -569,6 +583,16 @@ private boolean isIgnoredPublicMember(String variableName, String variableScope)
&& publicMemberPattern.matcher(variableName).find();
}

/**
* Checks whether the variable satisfies the public field check.
* @param variableDef Variable definition node.
* @return true if allowed
*/
private boolean isAllowedPublicField(DetailAST variableDef) {
return allowPublicFinalFields && isImmutableField(variableDef)
|| allowPublicImmutableFields && isImmutableFieldDefinedInFinalClass(variableDef);
}

/**
* Checks whether immutable field is defined in final class.
* @param variableDef Variable definition node.
Expand Down Expand Up @@ -597,7 +621,6 @@ private static Set<String> getModifiers(DetailAST defAST) {
}
}
return modifiersSet;

}

/**
Expand Down
67 changes: 67 additions & 0 deletions src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java
View file
Expand Up @@ -117,6 +117,25 @@ public void testStrictJavadoc() throws Exception {
public void testAllowPublicFinalFieldsInImmutableClass() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicImmutableFields", "true");
checkConfig.addAttribute("allowPublicFinalFields", "false");
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 testAllowPublicFinalFieldsInNonFinalClass() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicImmutableFields", "false");
checkConfig.addAttribute("allowPublicFinalFields", "true");
final String[] expected = {
"12:39: " + getCheckMessage(MSG_KEY, "includes"),
"13:39: " + getCheckMessage(MSG_KEY, "excludes"),
Expand All @@ -134,6 +153,8 @@ public void testUserSpecifiedImmutableClassesList() throws Exception {
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("immutableClassCanonicalNames", "java.util.List,"
+ "com.google.common.collect.ImmutableSet");
checkConfig.addAttribute("allowPublicImmutableFields", "true");
checkConfig.addAttribute("allowPublicFinalFields", "false");
final String[] expected = {
"14:35: " + getCheckMessage(MSG_KEY, "notes"),
"15:29: " + getCheckMessage(MSG_KEY, "value"),
Expand Down Expand Up @@ -285,6 +306,23 @@ public void testPublicImmutableFieldsNotAllowed() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicImmutableFields", "false");
checkConfig.addAttribute("allowPublicFinalFields", "false");
final String[] expected = {
"10:22: " + getCheckMessage(MSG_KEY, "someIntValue"),
"11:39: " + getCheckMessage(MSG_KEY, "includes"),
"12:35: " + getCheckMessage(MSG_KEY, "notes"),
"13:29: " + getCheckMessage(MSG_KEY, "value"),
"14:23: " + getCheckMessage(MSG_KEY, "list"),
};
verify(checkConfig, getPath("InputPublicImmutable.java"), expected);
}

@Test
public void testPublicFinalFieldsNotAllowed() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicImmutableFields", "true");
checkConfig.addAttribute("allowPublicFinalFields", "false");
final String[] expected = {
"10:22: " + getCheckMessage(MSG_KEY, "someIntValue"),
"11:39: " + getCheckMessage(MSG_KEY, "includes"),
Expand All @@ -295,6 +333,34 @@ public void testPublicImmutableFieldsNotAllowed() throws Exception {
verify(checkConfig, getPath("InputPublicImmutable.java"), expected);
}

@Test
public void testPublicFinalFieldsAllowed() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("immutableClassCanonicalNames",
"com.google.common.collect.ImmutableSet");
checkConfig.addAttribute("allowPublicImmutableFields", "false");
checkConfig.addAttribute("allowPublicFinalFields", "true");
final String[] expected = {
"12:35: " + getCheckMessage(MSG_KEY, "notes"),
"13:29: " + getCheckMessage(MSG_KEY, "value"),
"14:23: " + getCheckMessage(MSG_KEY, "list"),
};
verify(checkConfig, getPath("InputPublicImmutable.java"), expected);
}

@Test
public void testPublicFinalFieldInEnum() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicImmutableFields", "true");
checkConfig.addAttribute("allowPublicFinalFields", "false");
final String[] expected = {
"15:23: " + getCheckMessage(MSG_KEY, "hole"),
};
verify(checkConfig, getPath("InputEnumIsSealed.java"), expected);
}

@Test(expected = IllegalArgumentException.class)
public void testWrongTokenType() {
final VisibilityModifierCheck obj = new VisibilityModifierCheck();
Expand All @@ -307,6 +373,7 @@ public void testWrongTokenType() {
public void testNullModifiers() throws Exception {
final DefaultConfiguration checkConfig =
createCheckConfig(VisibilityModifierCheck.class);
checkConfig.addAttribute("allowPublicFinalFields", "false");
final String[] expected = {
"11:50: " + getCheckMessage(MSG_KEY, "i"),
};
Expand Down
16 changes: 16 additions & 0 deletions src/test/resources/com/puppycrawl/tools/checkstyle/checks/design/InputEnumIsSealed.java
View file
@@ -0,0 +1,16 @@

package com.puppycrawl.tools.checkstyle.checks.design;

/** Shows that sealed enum is good as final. */
public enum InputEnumIsSealed {
SOME_VALUE;

static class Hole {
}

/** Normally disallowed if final enclosing class is required. */
public final int someField = Integer.MAX_VALUE;

/** Disallowed because mutable. */
public final Hole hole = null;
}
81 changes: 78 additions & 3 deletions src/xdocs/config_design.xml
View file
Expand Up @@ -792,6 +792,10 @@ public class Foo{
in consideration. If user will provide short annotation name that type will match to any
named the same type without consideration of package
</p>
<p>
<b>allowPublicFinalFields</b> - which allows immutable fields be declared as
public. Default value is <b>true</b>
</p>
<p>
<b>allowPublicImmutableFields</b> - which allows immutable fields be declared as
public if defined in final class. Default value is <b>true</b>
Expand All @@ -814,7 +818,8 @@ public class Foo{
<p>
<b>Restriction</b>: 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
<b>if current field is immutable and defined in final class</b>
<b>if current field is immutable</b>. Under the flag <b>allowPublicImmutableFields</b>,
the enclosing class must also be final, to encourage immutability.
</p>
<p>
Star imports are out of scope of this Check. So if one of type imported via
Expand Down Expand Up @@ -849,6 +854,12 @@ public class Foo{
<td><a href="property_types.html#regexp">regular expression</a></td>
<td><code>^serialVersionUID$</code></td>
</tr>
<tr>
<td>allowPublicFinalFields</td>
<td>allows immutable fields be declared as public</td>
<td><a href="property_types.html#boolean">Boolean</a></td>
<td><code>true</code></td>
</tr>
<tr>
<td>allowPublicImmutableFields</td>
<td>allows immutable fields be declared as public if defined in final class</td>
Expand Down Expand Up @@ -1009,8 +1020,7 @@ class SomeClass
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;&gt;
&lt;property name=&quot;ignoreAnnotationCanonicalNames&quot;
value=&quot;CustomAnnotation&quot;/&gt;
&lt;property name=&quot;ignoreAnnotationCanonicalNames&quot; value=&quot;CustomAnnotation&quot;/&gt;
&lt;/module&gt;
</source>
<p>
Expand All @@ -1026,6 +1036,71 @@ class SomeClass
@mypackage.annotation.CustomAnnotation
String customAnnotatedAnotherPackage; // another package but short name matches
// so no violation
}
</source>

<p>
To understand the difference between allowPublicImmutableFields and
allowPublicFinalFields options, please, study the following examples.
</p>
<p>
1) To configure the check to use only 'allowPublicImmutableFields' option:
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;&gt;
&lt;property name=&quot;allowPublicImmutableFields&quot; value=&quot;true&quot;/&gt;
&lt;property name=&quot;allowPublicFinalFields&quot; value=&quot;false&quot;/&gt;
&lt;/module&gt;
</source>
<p>
Examples of violations:
</p>
<source>
public class InputPublicImmutable {
public final int someIntValue; // violation
public final ImmutableSet&lt;String&gt; includes; // violation
public final java.lang.String notes; // violation
public final BigDecimal value; // violation
public final List list; // violation

public InputPublicImmutable(Collection&lt;String&gt; includes,
BigDecimal value, String notes, int someValue, List l) {
this.includes = ImmutableSet.copyOf(includes);
this.value = value;
this.notes = notes;
this.someIntValue = someValue;
this.list = l;
}
}
</source>
<p>
2) To configure the check to use only 'allowPublicFinalFields' option:
</p>
<source>
&lt;module name=&quot;VisibilityModifier&quot;&gt;
&lt;property name=&quot;allowPublicImmutableFields&quot; value=&quot;false&quot;/&gt;
&lt;property name=&quot;allowPublicFinalFields&quot; value=&quot;true&quot;/&gt;
&lt;/module&gt;
</source>
<p>
Examples of violations:
</p>
<source>
public class InputPublicImmutable {
public final int someIntValue;
public final ImmutableSet&lt;String&gt; includes; // violation
public final java.lang.String notes;
public final BigDecimal value;
public final List list; // violation

public InputPublicImmutable(Collection&lt;String&gt; includes,
BigDecimal value, String notes, int someValue, List l) {
this.includes = ImmutableSet.copyOf(includes);
this.value = value;
this.notes = notes;
this.someIntValue = someValue;
this.list = l;
}
}
</source>
</subsection>
Expand Down

0 comments on commit 4c8690a

Please sign in to comment.