Issue #2971: Add allowPublicFinalFields option for VisibilityModifier (…

…#3207)

src/main/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheck.java

@@ -72,8 +72,11 @@
  * </pre>
  *
  * <p>
- * <b>allowPublicImmutableFields</b> - which allows immutable fields be
- * declared as public if defined in final class. Default value is <b>true</b>
+ * <b>allowPublicFinalFields</b> - which allows public final fields. Default value is <b>false</b>.
+ * </p>
+ * <p>
+ * <b>allowPublicImmutableFields</b> - which allows immutable fields to be
+ * declared as public if defined in final class. Default value is <b>false</b>
  * </p>
  * <p>
  * Field is known to be immutable if:
@@ -131,21 +134,21 @@
  * </p>
  * Examples:
  * <p>
- * Default Check's configuration will pass the code below:
+ * The check will rise 3 violations if it is run with default configuration against the following
+ * code example:
  * </p>
  *
  * <pre>
  * {@code
- * public final class ImmutableClass
+ * public class ImmutableClass
  * {
- *     public final int intValue; // No warning
- *     public final java.lang.String notes; // No warning
- *     public final BigDecimal value; // No warning
+ *     public int intValue; // violation
+ *     public java.lang.String notes; // violation
+ *     public BigDecimal value; // violation
  *
  *     public ImmutableClass(int intValue, BigDecimal value, String notes)
  *     {
- *         this.includes = ImmutableSet.copyOf(includes);
- *         this.excludes = ImmutableSet.copyOf(excludes);
+ *         this.intValue = intValue;
  *         this.value = value;
  *         this.notes = notes;
  *     }
@@ -159,6 +162,7 @@
  * </p>
  * <p>
  * &lt;module name=&quot;VisibilityModifier&quot;&gt;
+ *   &lt;property name=&quot;allowPublicImmutableFields&quot; value=&quot;true&quot;/&gt;
  *   &lt;property name=&quot;immutableClassCanonicalNames&quot; value=&quot;java.util.List,
  *   com.google.common.collect.ImmutableSet&quot;/&gt;
  * &lt;/module&gt;
@@ -328,8 +332,11 @@ public class VisibilityModifierCheck
     /** Whether package visible members are allowed. */
     private boolean packageAllowed;
 
-    /** Allows immutable fields to be declared as public. */
-    private boolean allowPublicImmutableFields = true;
+    /** Allows immutable fields of final classes to be declared as public. */
+    private boolean allowPublicImmutableFields;
+
+    /** Allows final fields to be declared as public. */
+    private boolean allowPublicFinalFields;
 
     /** List of immutable classes canonical names. */
     private List<String> immutableClassCanonicalNames = new ArrayList<>(DEFAULT_IMMUTABLE_TYPES);
@@ -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.
@@ -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;
@@ -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 && isFinalField(variableDef)
+            || allowPublicImmutableFields && isImmutableFieldDefinedInFinalClass(variableDef);
+    }
+
     /**
      * Checks whether immutable field is defined in final class.
      * @param variableDef Variable definition node.
@@ -597,7 +621,6 @@ private static Set<String> getModifiers(DetailAST defAST) {
             }
         }
         return modifiersSet;
-
     }
 
     /**
@@ -628,10 +651,7 @@ private static String getVisibilityScope(DetailAST variableDef) {
      */
     private boolean isImmutableField(DetailAST variableDef) {
         boolean result = false;
-
-        final DetailAST modifiers = variableDef.findFirstToken(TokenTypes.MODIFIERS);
-        final boolean isFinal = modifiers.branchContains(TokenTypes.FINAL);
-        if (isFinal) {
+        if (isFinalField(variableDef)) {
             final DetailAST type = variableDef.findFirstToken(TokenTypes.TYPE);
             final boolean isCanonicalName = type.getFirstChild().getType() == TokenTypes.DOT;
             final String typeName = getTypeName(type, isCanonicalName);
@@ -643,6 +663,16 @@ private boolean isImmutableField(DetailAST variableDef) {
         return result;
     }
 
+    /**
+     * Checks whether current field is final.
+     * @param variableDef field in consideration.
+     * @return true if current field is final.
+     */
+    private boolean isFinalField(DetailAST variableDef) {
+        final DetailAST modifiers = variableDef.findFirstToken(TokenTypes.MODIFIERS);
+        return modifiers.branchContains(TokenTypes.FINAL);
+    }
+
     /**
      * 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,

src/test/java/com/puppycrawl/tools/checkstyle/checks/design/VisibilityModifierCheckTest.java

@@ -117,6 +117,7 @@ public void testStrictJavadoc() throws Exception {
     public void testAllowPublicFinalFieldsInImmutableClass() throws Exception {
         final DefaultConfiguration checkConfig =
                 createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         final String[] expected = {
             "12:39: " + getCheckMessage(MSG_KEY, "includes"),
             "13:39: " + getCheckMessage(MSG_KEY, "excludes"),
@@ -128,15 +129,52 @@ public void testAllowPublicFinalFieldsInImmutableClass() throws Exception {
         verify(checkConfig, getPath("InputImmutable.java"), expected);
     }
 
+    @Test
+    public void testDisAllowPublicFinalAndImmutableFieldsInImmutableClass() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createCheckConfig(VisibilityModifierCheck.class);
+        final String[] expected = {
+            "11:22: " + getCheckMessage(MSG_KEY, "someIntValue"),
+            "12:39: " + getCheckMessage(MSG_KEY, "includes"),
+            "13:39: " + getCheckMessage(MSG_KEY, "excludes"),
+            "14:35: " + getCheckMessage(MSG_KEY, "notes"),
+            "15:29: " + getCheckMessage(MSG_KEY, "money"),
+            "16:23: " + getCheckMessage(MSG_KEY, "list"),
+            "30:28: " + getCheckMessage(MSG_KEY, "f"),
+            "31:30: " + getCheckMessage(MSG_KEY, "bool"),
+            "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 testAllowPublicFinalFieldsInNonFinalClass() throws Exception {
+        final DefaultConfiguration checkConfig =
+                createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicFinalFields", "true");
+        final String[] expected = {
+            "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("allowPublicImmutableFields", "true");
         checkConfig.addAttribute("immutableClassCanonicalNames", "java.util.List,"
                 + "com.google.common.collect.ImmutableSet");
         final String[] expected = {
             "14:35: " + getCheckMessage(MSG_KEY, "notes"),
-            "15:29: " + getCheckMessage(MSG_KEY, "value"),
+            "15:29: " + getCheckMessage(MSG_KEY, "money"),
             "32:35: " + getCheckMessage(MSG_KEY, "uri"),
             "33:35: " + getCheckMessage(MSG_KEY, "file"),
             "34:20: " + getCheckMessage(MSG_KEY, "value"),
@@ -151,6 +189,7 @@ public void testUserSpecifiedImmutableClassesList() throws Exception {
     public void testImmutableSpecifiedSameTypeName() throws Exception {
         final DefaultConfiguration checkConfig =
                 createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         checkConfig.addAttribute("immutableClassCanonicalNames",
                  "com.puppycrawl.tools.checkstyle.checks.coding.InputGregorianCalendar,"
                  + "com.puppycrawl.tools.checkstyle.checks.design.InetSocketAddress");
@@ -162,9 +201,10 @@ public void testImmutableSpecifiedSameTypeName() throws Exception {
     }
 
     @Test
-    public void testImmutableDefaultValueSameTypeName() throws Exception {
+    public void testImmutableValueSameTypeName() throws Exception {
         final DefaultConfiguration checkConfig =
                 createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         final String[] expected = {
             "7:46: " + getCheckMessage(MSG_KEY, "calendar"),
             "8:41: " + getCheckMessage(MSG_KEY, "calendar2"),
@@ -178,6 +218,7 @@ public void testImmutableDefaultValueSameTypeName() throws Exception {
     public void testImmutableStarImportFalseNegative() throws Exception {
         final DefaultConfiguration checkConfig =
                 createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         checkConfig.addAttribute("immutableClassCanonicalNames", "java.util.Arrays");
         final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
         verify(checkConfig, getPath("InputImmutableStarImport.java"), expected);
@@ -187,6 +228,7 @@ public void testImmutableStarImportFalseNegative() throws Exception {
     public void testImmutableStarImportNoWarn() throws Exception {
         final DefaultConfiguration checkConfig =
                 createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         checkConfig.addAttribute("immutableClassCanonicalNames",
                  "com.google.common.collect.ImmutableSet");
         final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
@@ -284,7 +326,6 @@ public void testGetAcceptableTokens() {
     public void testPublicImmutableFieldsNotAllowed() throws Exception {
         final DefaultConfiguration checkConfig =
             createCheckConfig(VisibilityModifierCheck.class);
-        checkConfig.addAttribute("allowPublicImmutableFields", "false");
         final String[] expected = {
             "10:22: " + getCheckMessage(MSG_KEY, "someIntValue"),
             "11:39: " + getCheckMessage(MSG_KEY, "includes"),
@@ -295,6 +336,42 @@ public void testPublicImmutableFieldsNotAllowed() throws Exception {
         verify(checkConfig, getPath("InputPublicImmutable.java"), expected);
     }
 
+    @Test
+    public void testPublicFinalFieldsNotAllowed() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createCheckConfig(VisibilityModifierCheck.class);
+        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 testPublicFinalFieldsAllowed() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicFinalFields", "true");
+        checkConfig.addAttribute("immutableClassCanonicalNames",
+            "com.google.common.collect.ImmutableSet");
+        final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
+        verify(checkConfig, getPath("InputPublicImmutable.java"), expected);
+    }
+
+    @Test
+    public void testPublicFinalFieldInEnum() throws Exception {
+        final DefaultConfiguration checkConfig =
+            createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
+        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();
@@ -307,6 +384,7 @@ public void testWrongTokenType() {
     public void testNullModifiers() throws Exception {
         final DefaultConfiguration checkConfig =
             createCheckConfig(VisibilityModifierCheck.class);
+        checkConfig.addAttribute("allowPublicImmutableFields", "true");
         final String[] expected = {
             "11:50: " + getCheckMessage(MSG_KEY, "i"),
         };

src/test/resources/com/puppycrawl/tools/checkstyle/checks/design/InputEnumIsSealed.java

@@ -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;
+}

src/test/resources/com/puppycrawl/tools/checkstyle/checks/design/InputImmutable.java

@@ -12,14 +12,14 @@ public final class InputImmutable
     public final ImmutableSet<String> includes;
     public final ImmutableSet<String> excludes;
     public final java.lang.String notes;
-    public final BigDecimal value;
+    public final BigDecimal money;
     public final List list;
 
     public InputImmutable(Collection<String> includes, Collection<String> excludes,
              BigDecimal value, String notes, int someValue, List l) {
         this.includes = ImmutableSet.copyOf(includes);
         this.excludes = ImmutableSet.copyOf(excludes);
-        this.value = value;
+        this.money = value;
         this.notes = notes;
         this.someIntValue = someValue;
         this.list = l;