Issue checkstyle#9280: replace Scope with AccessModifierOption in…

… JavadocVariableCheck
martin-mfg · Jul 30, 2021 · ab3c591 · ab3c591
1 parent 2e453a5
commit ab3c591
Show file tree

Hide file tree

Showing 6 changed files with 120 additions and 138 deletions.
diff --git a/.ci/validation.sh b/.ci/validation.sh
@@ -168,7 +168,7 @@ no-error-xwiki)
   CS_POM_VERSION="$(getCheckstylePomVersion)"
   echo version:$CS_POM_VERSION
   mvn -e --no-transfer-progress clean install -Pno-validations
-  checkout_from "https://github.com/xwiki/xwiki-commons.git"
+  checkout_from "-b checkstyle_9280 https://github.com/checkstyle/xwiki-commons.git"
   cd .ci-temp/xwiki-commons
   # Build custom Checkstyle rules
   mvn -e --no-transfer-progress -f \

diff --git a/.ci/wercker.sh b/.ci/wercker.sh
@@ -200,16 +200,19 @@ no-error-spring-integration)
   ;;
 
 no-error-spring-cloud-gcp)
-  set -e
-  CS_POM_VERSION="$(getCheckstylePomVersion)"
-  echo CS_version: ${CS_POM_VERSION}
-  checkout_from https://github.com/googlecloudplatform/spring-cloud-gcp
-  cd .ci-temp/spring-cloud-gcp
-  mvn -e --no-transfer-progress checkstyle:check@checkstyle-validation \
-   -Dmaven-checkstyle-plugin.version=3.1.1 \
-   -Dpuppycrawl-tools-checkstyle.version=${CS_POM_VERSION}
-  cd ..
-  removeFolderWithProtectedFiles spring-cloud-gcp
+  # disabled until https://github.com/spring-io/spring-javaformat/pull/274 is merged, because of
+  # a breaking change (https://github.com/checkstyle/checkstyle/pull/9277) in checkstyle.
+
+  #set -e
+  # CS_POM_VERSION="$(getCheckstylePomVersion)"
+  # echo CS_version: ${CS_POM_VERSION}
+  # checkout_from https://github.com/googlecloudplatform/spring-cloud-gcp
+  # cd .ci-temp/spring-cloud-gcp
+  # mvn -e --no-transfer-progress checkstyle:check@checkstyle-validation \
+  #  -Dmaven-checkstyle-plugin.version=3.1.1 \
+  #  -Dpuppycrawl-tools-checkstyle.version=${CS_POM_VERSION}
+  # cd ..
+  # removeFolderWithProtectedFiles spring-cloud-gcp
   ;;
 
 no-exception-struts)

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheck.java
@@ -19,6 +19,7 @@
 
 package com.puppycrawl.tools.checkstyle.checks.javadoc;
 
+import java.util.Arrays;
 import java.util.regex.Pattern;
 
 import com.puppycrawl.tools.checkstyle.StatelessCheck;
@@ -28,6 +29,7 @@
 import com.puppycrawl.tools.checkstyle.api.Scope;
 import com.puppycrawl.tools.checkstyle.api.TextBlock;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+import com.puppycrawl.tools.checkstyle.checks.naming.AccessModifierOption;
 import com.puppycrawl.tools.checkstyle.utils.ScopeUtil;
 
 /**
@@ -36,15 +38,10 @@
  * </p>
  * <ul>
  * <li>
- * Property {@code scope} - Specify the visibility scope where Javadoc comments are checked.
- * Type is {@code com.puppycrawl.tools.checkstyle.api.Scope}.
- * Default value is {@code private}.
- * </li>
- * <li>
- * Property {@code excludeScope} - Specify the visibility scope where Javadoc
- * comments are not checked.
- * Type is {@code com.puppycrawl.tools.checkstyle.api.Scope}.
- * Default value is {@code null}.
+ * Property {@code accessModifiers} - Specify the access modifiers where Javadoc comments are
+ * checked.
+ * Type is {@code com.puppycrawl.tools.checkstyle.checks.naming.AccessModifierOption[]}.
+ * Default value is {@code public, protected, package, private}.
  * </li>
  * <li>
  * Property {@code ignoreNamePattern} - Specify the regexp to define variable names to ignore.
@@ -67,8 +64,8 @@
  * &lt;module name="JavadocVariable"/&gt;
  * </pre>
  * <p>
- * By default, this setting will report a violation if
- * there is no javadoc for any scope member.
+ * By default, this setting will report a violation
+ * if there is no javadoc for a member with any access modifier.
  * </p>
  * <pre>
  * public class Test {
@@ -84,40 +81,16 @@
  * }
  * </pre>
  * <p>
- * To configure the check for {@code public} scope:
- * </p>
- * <pre>
- * &lt;module name="JavadocVariable"&gt;
- *   &lt;property name="scope" value="public"/&gt;
- * &lt;/module&gt;
- * </pre>
- * <p>This setting will report a violation if there is no javadoc for {@code public} member.</p>
- * <pre>
- * public class Test {
- *   private int a; // OK
- *
- *   &#47;**
- *    * Some description here
- *    *&#47;
- *   private int b; // OK
- *   protected int c; // OK
- *   public int d; // violation, missing javadoc for public member
- *   &#47;*package*&#47; int e; // OK
- * }
- * </pre>
- * <p>
- * To configure the check for members which are in {@code private},
- * but not in {@code protected} scope:
+ * To configure the check for {@code package} and {@code private} variables:
  * </p>
  * <pre>
  * &lt;module name="JavadocVariable"&gt;
- *   &lt;property name="scope" value="private"/&gt;
- *   &lt;property name="excludeScope" value="protected"/&gt;
+ *   &lt;property name="accessModifiers" value="package,private"/&gt;
  * &lt;/module&gt;
  * </pre>
  * <p>
- * This setting will report a violation if there is no javadoc for {@code private}
- * member and ignores {@code protected} member.
+ * This setting will report a violation if there is no javadoc for {@code package}
+ * or {@code private} members.
  * </p>
  * <pre>
  * public class Test {
@@ -141,8 +114,8 @@
  * &lt;/module&gt;
  * </pre>
  * <p>
- * This setting will report a violation if there is no javadoc for any scope
- * member and ignores members with name {@code log} or {@code logger}.
+ * This setting will report a violation if there is no javadoc for a
+ * member with any scope and ignores members with name {@code log} or {@code logger}.
  * </p>
  * <pre>
  * public class Test {
@@ -183,31 +156,25 @@ public class JavadocVariableCheck
      */
     public static final String MSG_JAVADOC_MISSING = "javadoc.missing";
 
-    /** Specify the visibility scope where Javadoc comments are checked. */
-    private Scope scope = Scope.PRIVATE;
-
-    /** Specify the visibility scope where Javadoc comments are not checked. */
-    private Scope excludeScope;
+    /** Specify the access modifiers where Javadoc comments are checked. */
+    private AccessModifierOption[] accessModifiers = {
+        AccessModifierOption.PUBLIC,
+        AccessModifierOption.PROTECTED,
+        AccessModifierOption.PACKAGE,
+        AccessModifierOption.PRIVATE,
+    };
 
     /** Specify the regexp to define variable names to ignore. */
     private Pattern ignoreNamePattern;
 
     /**
-     * Setter to specify the visibility scope where Javadoc comments are checked.
-     *
-     * @param scope a scope.
-     */
-    public void setScope(Scope scope) {
-        this.scope = scope;
-    }
-
-    /**
-     * Setter to specify the visibility scope where Javadoc comments are not checked.
+     * Setter to specify the access modifiers where Javadoc comments are checked.
      *
-     * @param excludeScope a scope.
+     * @param accessModifiers access modifiers of variables which should be checked.
      */
-    public void setExcludeScope(Scope excludeScope) {
-        this.excludeScope = excludeScope;
+    public void setAccessModifiers(AccessModifierOption... accessModifiers) {
+        this.accessModifiers =
+            Arrays.copyOf(accessModifiers, accessModifiers.length);
     }
 
     /**
@@ -285,12 +252,56 @@ private boolean shouldCheck(final DetailAST ast) {
             }
 
             final Scope surroundingScope = ScopeUtil.getSurroundingScope(ast);
-            result = customScope.isIn(scope) && surroundingScope.isIn(scope)
-                && (excludeScope == null
-                    || !customScope.isIn(excludeScope)
-                    || !surroundingScope.isIn(excludeScope));
+
+            final Scope effectiveScope;
+            if (surroundingScope.isIn(customScope)) {
+                effectiveScope = customScope;
+            }
+            else {
+                effectiveScope = surroundingScope;
+            }
+            result = matchAccessModifiers(toAccessModifier(effectiveScope));
         }
         return result;
     }
 
+    /**
+     * Checks whether a variable has the correct access modifier to be checked.
+     *
+     * @param accessModifier the access modifier of the variable.
+     * @return whether the variable matches the expected access modifier.
+     */
+    private boolean matchAccessModifiers(final AccessModifierOption accessModifier) {
+        return Arrays.stream(accessModifiers)
+            .anyMatch(modifier -> modifier == accessModifier);
+    }
+
+    /**
+     * Converts a {@link Scope} to {@link AccessModifierOption}. {@code Scope.NOTHING} and {@code
+     * Scope.ANONINNER} are converted to {@code AccessModifierOption.PUBLIC}.
+     *
+     * @param scope Scope to be converted.
+     * @return the corresponding AccessModifierOption.
+     */
+    private static AccessModifierOption toAccessModifier(Scope scope) {
+        final AccessModifierOption accessModifier;
+        switch (scope) {
+            case PROTECTED:
+                accessModifier = AccessModifierOption.PROTECTED;
+                break;
+            case PACKAGE:
+                accessModifier = AccessModifierOption.PACKAGE;
+                break;
+            case PRIVATE:
+                accessModifier = AccessModifierOption.PRIVATE;
+                break;
+            case NOTHING:
+            case ANONINNER:
+            case PUBLIC:
+            default:
+                accessModifier = AccessModifierOption.PUBLIC;
+        }
+        return accessModifier;
+    }
+
 }
diff --git a/...in/resources/com/puppycrawl/tools/checkstyle/meta/checks/javadoc/JavadocVariableCheck.xml b/...in/resources/com/puppycrawl/tools/checkstyle/meta/checks/javadoc/JavadocVariableCheck.xml
@@ -8,14 +8,11 @@
  Checks that a variable has a Javadoc comment. Ignores {@code serialVersionUID} fields.
  &lt;/p&gt;</description>
          <properties>
-            <property default-value="private"
-                      name="scope"
-                      type="com.puppycrawl.tools.checkstyle.api.Scope">
-               <description>Specify the visibility scope where Javadoc comments are checked.</description>
-            </property>
-            <property name="excludeScope" type="com.puppycrawl.tools.checkstyle.api.Scope">
-               <description>Specify the visibility scope where Javadoc
- comments are not checked.</description>
+            <property default-value="public, protected, package, private"
+                      name="accessModifiers"
+                      type="com.puppycrawl.tools.checkstyle.checks.naming.AccessModifierOption[]">
+               <description>Specify the access modifiers where Javadoc comments are
+ checked.</description>
             </property>
             <property name="ignoreNamePattern" type="java.util.regex.Pattern">
                <description>Specify the regexp to define variable names to ignore.</description>

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheckTest.java
@@ -22,12 +22,13 @@
 import static com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocVariableCheck.MSG_JAVADOC_MISSING;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 
+import com.puppycrawl.tools.checkstyle.api.Scope;
 import org.junit.jupiter.api.Test;
 
 import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
 import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
-import com.puppycrawl.tools.checkstyle.api.Scope;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+import com.puppycrawl.tools.checkstyle.checks.naming.AccessModifierOption;
 import com.puppycrawl.tools.checkstyle.utils.CommonUtil;
 
 public class JavadocVariableCheckTest
@@ -93,7 +94,7 @@ public void testAnother2()
             throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addProperty("scope", Scope.PUBLIC.getName());
+        checkConfig.addProperty("accessModifiers", AccessModifierOption.PUBLIC.toString());
         final String[] expected = CommonUtil.EMPTY_STRING_ARRAY;
         verify(checkConfig, getPath("InputJavadocVariableInner2.java"), expected);
     }
@@ -120,15 +121,15 @@ public void testAnother4()
             throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addProperty("scope", Scope.PUBLIC.getName());
+        checkConfig.addProperty("accessModifiers", AccessModifierOption.PUBLIC.toString());
         final String[] expected = {
             "52:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
         };
         verify(checkConfig, getPath("InputJavadocVariablePublicOnly2.java"), expected);
     }
 
     @Test
-    public void testScopes() throws Exception {
+    public void testAccessModifiersAll() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
         final String[] expected = {
@@ -176,10 +177,13 @@ public void testScopes() throws Exception {
     }
 
     @Test
-    public void testScopes2() throws Exception {
+    public void testAccessModifiersPublicProtected() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addProperty("scope", Scope.PROTECTED.getName());
+        checkConfig.addProperty(
+            "accessModifiers",
+            AccessModifierOption.PROTECTED.name() + "," + AccessModifierOption.PUBLIC.name()
+        );
         final String[] expected = {
             "15:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
             "16:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
@@ -192,11 +196,13 @@ public void testScopes2() throws Exception {
     }
 
     @Test
-    public void testExcludeScope() throws Exception {
+    public void testAccessModifiersPrivatePackage() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addProperty("scope", Scope.PRIVATE.getName());
-        checkConfig.addProperty("excludeScope", Scope.PROTECTED.getName());
+        checkConfig.addProperty(
+            "accessModifiers",
+            AccessModifierOption.PRIVATE.name() + "," + AccessModifierOption.PACKAGE.name()
+        );
         final String[] expected = {
             "17:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
             "18:5: " + getCheckMessage(MSG_JAVADOC_MISSING),