From f262b1c13b0dfe1a17ccf9b54a20cc30d7657b30 Mon Sep 17 00:00:00 2001
From: martin-mfg <2026226+martin-mfg@users.noreply.github.com>
Date: Fri, 12 Feb 2021 15:23:07 +0100
Subject: [PATCH] Issue #9280: replace `Scope` with `AccessModifierOption` in
 JavadocVariableCheck

---
 .ci/validation.sh                             |   2 +-
 .ci/wercker.sh                                |  23 +--
 .../checks/javadoc/JavadocVariableCheck.java  | 137 ++++++++++--------
 .../checks/javadoc/JavadocVariableCheck.xml   |  13 +-
 .../javadoc/JavadocVariableCheckTest.java     |  23 +--
 src/xdocs/config_javadoc.xml                  |  59 ++------
 6 files changed, 119 insertions(+), 138 deletions(-)

diff --git a/.ci/validation.sh b/.ci/validation.sh
index baa997308a7..1d1f20ddeee 100755
--- 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
index d90472e3775..9557e7499ee 100755
--- 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
index 487efa58bf0..0c05339593b 100644
--- 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/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/javadoc/JavadocVariableCheck.xml b/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/javadoc/JavadocVariableCheck.xml
index aea982226cd..a5e7ea0806f 100644
--- a/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/javadoc/JavadocVariableCheck.xml
+++ b/src/main/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
index fb7c0510cab..8f45d7f1ca4 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocVariableCheckTest.java
@@ -26,8 +26,8 @@
 
 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 +93,7 @@ public void testAnother2()
             throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addAttribute("scope", Scope.PUBLIC.getName());
+        checkConfig.addAttribute("accessModifiers", AccessModifierOption.PUBLIC.toString());
         final String[] expected = CommonUtil.EMPTY_STRING_ARRAY;
         verify(checkConfig, getPath("InputJavadocVariableInner2.java"), expected);
     }
@@ -120,7 +120,7 @@ public void testAnother4()
             throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addAttribute("scope", Scope.PUBLIC.getName());
+        checkConfig.addAttribute("accessModifiers", AccessModifierOption.PUBLIC.toString());
         final String[] expected = {
             "52:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
         };
@@ -128,7 +128,7 @@ public void testAnother4()
     }
 
     @Test
-    public void testScopes() throws Exception {
+    public void testAccessModifiersAll() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
         final String[] expected = {
@@ -176,10 +176,13 @@ public void testScopes() throws Exception {
     }
 
     @Test
-    public void testScopes2() throws Exception {
+    public void testAccessModifiersPublicProtected() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addAttribute("scope", Scope.PROTECTED.getName());
+        checkConfig.addAttribute(
+            "accessModifiers",
+            AccessModifierOption.PROTECTED.name() + "," + AccessModifierOption.PUBLIC.name()
+        );
         final String[] expected = {
             "15:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
             "16:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
@@ -192,11 +195,13 @@ public void testScopes2() throws Exception {
     }
 
     @Test
-    public void testExcludeScope() throws Exception {
+    public void testAccessModifiersPrivatePackage() throws Exception {
         final DefaultConfiguration checkConfig =
             createModuleConfig(JavadocVariableCheck.class);
-        checkConfig.addAttribute("scope", Scope.PRIVATE.getName());
-        checkConfig.addAttribute("excludeScope", Scope.PROTECTED.getName());
+        checkConfig.addAttribute(
+            "accessModifiers",
+            AccessModifierOption.PRIVATE.name() + "," + AccessModifierOption.PACKAGE.name()
+        );
         final String[] expected = {
             "17:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
             "18:5: " + getCheckMessage(MSG_JAVADOC_MISSING),
diff --git a/src/xdocs/config_javadoc.xml b/src/xdocs/config_javadoc.xml
index 1d753f28a10..e50bcc49a2c 100644
--- a/src/xdocs/config_javadoc.xml
+++ b/src/xdocs/config_javadoc.xml
@@ -2280,19 +2280,14 @@ class DatabaseConfiguration {}
               <th>since</th>
             </tr>
             <tr>
-              <td>scope</td>
-              <td>Specify the visibility scope where Javadoc comments are checked.</td>
-              <td><a href="property_types.html#Scope">Scope</a></td>
-              <td><code>private</code></td>
+              <td>accessModifiers</td>
+              <td>Specify the access modifiers where Javadoc comments are checked.</td>
+              <td><a href="property_types.html#AccessModifierOption.5B.5D">AccessModifierOption[]
+              </a>
+              </td>
+              <td><code>public, protected, package, private</code></td>
               <td>3.0</td>
             </tr>
-            <tr>
-              <td>excludeScope</td>
-              <td>Specify the visibility scope where Javadoc comments are not checked.</td>
-              <td><a href="property_types.html#Scope">Scope</a></td>
-              <td><code>null</code></td>
-              <td>3.4</td>
-            </tr>
             <tr>
               <td>ignoreNamePattern</td>
               <td>Specify the regexp to define variable names to ignore.</td>
@@ -2326,7 +2321,7 @@ class DatabaseConfiguration {}
         </source>
         <p>
           By default, this setting will report a violation if
-          there is no javadoc for any scope member.
+          there is no javadoc for a member with any access modifier.
         </p>
         <source>
 public class Test {
@@ -2343,48 +2338,18 @@ public int d; // violation, missing javadoc for public member
         </source>
 
         <p>
-          To configure the check for <code>public</code>
-          scope:
+          To configure the check for <code>package</code> and <code>private</code>
+          variables:
         </p>
 
         <source>
 &lt;module name="JavadocVariable"&gt;
-  &lt;property name="scope" value="public"/&gt;
-&lt;/module&gt;
-        </source>
-        <p>
-          This setting will report a violation if there
-          is no javadoc for <code>public</code> member.
-        </p>
-        <source>
-public class Test {
-private int a; // OK
-
-/**
- * Some description here
- */
-private int b; // OK
-protected int c; // OK
-public int d; // violation, missing javadoc for public member
-/*package*/ int e; // OK
-}
-        </source>
-
-        <p>
-          To configure the check for members which are in <code>private</code>, but not in
-          <code>protected</code> scope:
-        </p>
-
-        <source>
-&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;
         </source>
         <p>
           This setting will report a violation if there is no
-          javadoc for <code>private</code> member and
-          ignores <code>protected</code> member.
+          javadoc for <code>package</code> or <code>private</code> members.
         </p>
         <source>
 public class Test {
@@ -2411,7 +2376,7 @@ public int d; // OK
         </source>
         <p>
           This setting will report a violation if there is no
-          javadoc for any scope member and ignores members with
+          javadoc for a member with any scope and ignores members with
           name <code>log</code> or <code>logger</code>.
         </p>
         <source>