Issue checkstyle#14726: new implementation of the module

config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml

@@ -452,6 +452,36 @@
     </details>
   </checkerFrameworkError>
 
+  <checkerFrameworkError unstable="false">
+    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java</fileName>
+    <specifier>methodref.param</specifier>
+    <message>Incompatible parameter type for arg0</message>
+    <lineContent>.map(children::indexOf);</lineContent>
+    <details>
+      found   : @GuardSatisfied Object
+      required: @GuardedBy DetailAST
+      Consequence: method in @GuardedBy List&lt;@GuardedBy DetailAST&gt;
+      @GuardedBy int indexOf(@GuardSatisfied List&lt;@GuardedBy DetailAST&gt; this, @GuardSatisfied Object p0)
+      is not a valid method reference for method in @GuardedBy Function&lt;@GuardedBy DetailAST, @GuardedBy Integer&gt;
+      @GuardedBy Integer apply(@GuardedBy Function&lt;@GuardedBy DetailAST, @GuardedBy Integer&gt; this, @GuardedBy DetailAST p0)
+    </details>
+  </checkerFrameworkError>
+
+  <checkerFrameworkError unstable="false">
+    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java</fileName>
+    <specifier>methodref.receiver.bound</specifier>
+    <message>Incompatible receiver type</message>
+    <lineContent>.map(children::indexOf);</lineContent>
+    <details>
+      found   : @GuardedBy List&lt;@GuardedBy DetailAST&gt;
+      required: @GuardSatisfied List&lt;@GuardedBy DetailAST&gt;
+      Consequence: method
+      @GuardedBy List&lt;@GuardedBy DetailAST&gt;
+      is not a valid method reference for method in @GuardedBy List&lt;@GuardedBy DetailAST&gt;
+      @GuardedBy int indexOf(@GuardSatisfied List&lt;@GuardedBy DetailAST&gt; this, @GuardSatisfied Object p0)
+    </details>
+  </checkerFrameworkError>
+
   <checkerFrameworkError unstable="false">
     <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ModifiedControlVariableCheck.java</fileName>
     <specifier>methodref.param</specifier>

src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionConstructorsDeclarationGroupingTest.java

@@ -47,7 +47,7 @@ public void testClass() throws Exception {
 
         final String[] expectedViolation = {
             "10:5: " + getCheckMessage(clazz,
-                    ConstructorsDeclarationGroupingCheck.MSG_KEY, 6),
+                    ConstructorsDeclarationGroupingCheck.MSG_KEY, 4),
         };
 
         final List<String> expectedXpathQueries = Arrays.asList(
@@ -79,7 +79,7 @@ public void testEnum() throws Exception {
 
         final String[] expectedViolation = {
             "12:5: " + getCheckMessage(clazz,
-                    ConstructorsDeclarationGroupingCheck.MSG_KEY, 8),
+                    ConstructorsDeclarationGroupingCheck.MSG_KEY, 6),
         };
 
         final List<String> expectedXpathQueries = Arrays.asList(

src/it/resources-noncompilable/org/checkstyle/suppressionxpathfilter/constructorsdeclarationgrouping/InputXpathConstructorsDeclarationGroupingRecords.java

@@ -12,9 +12,5 @@ public MyRecord(int a) {
     void foo() {}
 
     public MyRecord {} // warn
-
-    public MyRecord(int x, int y, int z) {
-      this(x+y, z);
-    }
   }
 }

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheck.java

@@ -19,20 +19,24 @@
 
 package com.puppycrawl.tools.checkstyle.checks.coding;
 
-import java.util.HashMap;
-import java.util.Map;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
 
-import com.puppycrawl.tools.checkstyle.FileStatefulCheck;
+import com.puppycrawl.tools.checkstyle.StatelessCheck;
 import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
 import com.puppycrawl.tools.checkstyle.api.DetailAST;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
 
 /**
  * <p>
  * Checks that all constructors are grouped together.
- * If there is any code between the constructors
- * then this check will give an error.
- * Comments between constructors are ignored.
+ * If there is any other type of code (like method or variable declarations, etc)
+ * separating the constructors then this check will log a violation.
+ * The check identifies and logs any constructors that are not grouped together.
+ * The violation message will specify the line number from where the constructors got separated.
+ * Comments between constructors are allowed.
  * </p>
  * <p>
  * Parent is {@code com.puppycrawl.tools.checkstyle.TreeWalker}
@@ -46,10 +50,10 @@
  * </li>
  * </ul>
  *
- * @since 10.16.0
+ * @since 10.17.0
  */
 
-@FileStatefulCheck
+@StatelessCheck
 public class ConstructorsDeclarationGroupingCheck extends AbstractCheck {
 
     /**
@@ -58,11 +62,6 @@ public class ConstructorsDeclarationGroupingCheck extends AbstractCheck {
      */
     public static final String MSG_KEY = "constructors.declaration.grouping";
 
-    /**
-     * Specifies different Object Blocks scope.
-     */
-    private final Map<DetailAST, Integer> allObjBlocks = new HashMap<>();
-
     @Override
     public int[] getDefaultTokens() {
         return getRequiredTokens();
@@ -76,35 +75,74 @@ public int[] getAcceptableTokens() {
     @Override
     public int[] getRequiredTokens() {
         return new int[] {
-            TokenTypes.CTOR_DEF,
-            TokenTypes.COMPACT_CTOR_DEF,
+            TokenTypes.CLASS_DEF,
+            TokenTypes.ENUM_DEF,
+            TokenTypes.RECORD_DEF,
         };
     }
 
-    @Override
-    public void beginTree(DetailAST rootAst) {
-        allObjBlocks.clear();
-    }
-
     @Override
     public void visitToken(DetailAST ast) {
-        final DetailAST currObjBlock = ast.getParent();
-        final Integer previousCtorLineNo = allObjBlocks.get(currObjBlock);
+        // first make a list of all child ASTs
+        final List<DetailAST> children = getChildList(ast);
+
+        // find first constructor
+        final DetailAST firstConstructor = children.stream()
+                .filter(ConstructorsDeclarationGroupingCheck::isConstructor)
+                .findFirst()
+                .orElse(null);
+
+        if (firstConstructor != null) {
+
+            // get all children after the first constructor
+            final List<DetailAST> childrenAfterFirstConstructor =
+                    children.subList(children.indexOf(firstConstructor), children.size());
 
-        if (previousCtorLineNo != null) {
-            final DetailAST previousSibling = ast.getPreviousSibling();
-            final int siblingType = previousSibling.getType();
-            final boolean isCtor = siblingType == TokenTypes.CTOR_DEF;
-            final boolean isCompactCtor = siblingType == TokenTypes.COMPACT_CTOR_DEF;
+            // find first index of non-constructor AST after the first constructor, if present
+            final Optional<Integer> indexOfFirstNonConstructor = childrenAfterFirstConstructor
+                    .stream()
+                    .filter(currAst -> !isConstructor(currAst))
+                    .findFirst()
+                    .map(children::indexOf);
 
-            if (!isCtor && !isCompactCtor) {
-                log(ast, MSG_KEY, previousCtorLineNo);
-            }
+            // make list of all children after this
+            final List<DetailAST> childrenAfterFirstNonConstructor = indexOfFirstNonConstructor
+                    .map(index -> children.subList(index, children.size()))
+                    .orElseGet(ArrayList::new);
 
-            allObjBlocks.put(currObjBlock, ast.getLineNo());
+            // create a list of all constructors that are not grouped to log
+            final List<DetailAST> constructorsToLog = childrenAfterFirstNonConstructor.stream()
+                    .filter(ConstructorsDeclarationGroupingCheck::isConstructor)
+                    .collect(Collectors.toUnmodifiableList());
+
+            constructorsToLog.forEach(ctor -> log(ctor, MSG_KEY, firstConstructor.getLineNo()));
         }
-        else {
-            allObjBlocks.put(currObjBlock, ast.getLineNo());
+    }
+
+    /**
+     * Get a list of all children of the given AST.
+     *
+     * @param ast the AST to get children of
+     * @return a list of all children of the given AST
+     */
+    private static List<DetailAST> getChildList(DetailAST ast) {
+        final List<DetailAST> children = new ArrayList<>();
+        DetailAST child = ast.findFirstToken(TokenTypes.OBJBLOCK).getFirstChild();
+        while (child != null) {
+            children.add(child);
+            child = child.getNextSibling();
         }
+        return children;
+    }
+
+    /**
+     * Check if the given AST is a constructor.
+     *
+     * @param ast the AST to check
+     * @return true if the given AST is a constructor, false otherwise
+     */
+    private static boolean isConstructor(DetailAST ast) {
+        return ast.getType() == TokenTypes.CTOR_DEF
+                || ast.getType() == TokenTypes.COMPACT_CTOR_DEF;
     }
 }

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Inner assignments should be avoided.
 avoid.clone.method=Avoid using clone method.
 avoid.double.brace.init=Avoid double brace initialization.
 avoid.finalizer.method=Avoid using finalizer method.
-constructors.declaration.grouping=All constructors should be grouped together. Previous constructor was at line number ''{0}''.
+constructors.declaration.grouping=Constructors should be grouped together. The first constructor is declared at line ''{0}''.
 covariant.equals=covariant equals without overriding equals(java.lang.Object).
 declaration.order.access=Variable access definition in wrong order.
 declaration.order.constructor=Constructor definition in wrong order.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_fi.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Älä käytä sisäkkäisiä sijoituksia.
 avoid.clone.method=Vältä klooni menetelmää.
 avoid.double.brace.init=Vältä kaksinkertaista ahdintuen alustamista.
 avoid.finalizer.method=Vältä Finalizer menetelmää.
-constructors.declaration.grouping=Kaikki rakentajat tulee ryhmitellä yhteen. Edellinen rakentaja oli rivillä ''{0}''.
+constructors.declaration.grouping=Rakentajat tulee ryhmitellä yhteen. Ensimmäinen rakentaja ilmoitetaan rivillä ''{0}''.
 covariant.equals=covariant vastaa ilman painavaa tasavertaisten (java.lang.Object).
 declaration.order.access=Muuttuja pääsy määritelmä väärässä järjestyksessä.
 declaration.order.constructor=Rakentaja määritelmä väärässä järjestyksessä.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_fr.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Évitez d''affecter une valeur à une variable au sein d'
 avoid.clone.method=Évitez d''utiliser la méthode de clonage.
 avoid.double.brace.init=Évitez l''initialisation à double accolade.
 avoid.finalizer.method=Évitez d''utiliser la méthode de finalisation.
-constructors.declaration.grouping=Tous les constructeurs doivent être regroupés. Le constructeur précédent se trouvait au numéro de ligne ''{0}''.
+constructors.declaration.grouping=Les constructeurs doivent être regroupés. Le premier constructeur est déclaré à la ligne ''{0}''.
 covariant.equals=Votre méthode equals compare uniquement les objets de votre classe. N''oubliez pas de surcharger la méthode equals(java.lang.Object).
 declaration.order.access=La définition des variables n''est pas triée suivant leur portée.
 declaration.order.constructor=La définition des constructeurs n''apparaît pas dans le bon ordre.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_ja.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=式内部での代入は避けるべきです。
 avoid.clone.method=cloneメソッドを使用しないでください。
 avoid.double.brace.init=二重中括弧を使った初期化は使用しないでください。
 avoid.finalizer.method=ファイナライザメソッドを使用しないでください。
-constructors.declaration.grouping=すべてのコンストラクターはグループ化する必要があります。前のコンストラクターは行番号 ''{0}'' にありました。
+constructors.declaration.grouping=コンストラクターはグループ化する必要があります。最初のコンストラクターは行 ''{0}'' で宣言されます。
 covariant.equals=equals(java.lang.Object) をオーバーライドせずに共変 な equals を定義しています。
 declaration.order.access=変数アクセスの定義順序が間違っています。
 declaration.order.constructor=コンストラクタの定義順序が間違っています。

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_pt.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Atribuições aninhadas devem ser evitadas.
 avoid.clone.method=Evite o uso do método ''clone()''.
 avoid.double.brace.init=Evite a inicialização entre chaves.
 avoid.finalizer.method=Evite o uso do método ''finalize()''.
-constructors.declaration.grouping=Todos os construtores devem ser agrupados. O construtor anterior estava no número de linha ''{0}''.
+constructors.declaration.grouping=Os construtores devem ser agrupados. O primeiro construtor é declarado na linha ''{0}''.
 covariant.equals="equals" covariante sem sobrescrever ''equals(java.lang.Object)''.
 declaration.order.access=Definição de acesso a variável em ordem errada.
 declaration.order.constructor=Definição de construtor em ordem errada.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_ru.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Внутренние присвоения следует
 avoid.clone.method=Избегайте использования метода clone().
 avoid.double.brace.init=Избегайте инициализации в двойных скобках.
 avoid.finalizer.method=Избегайте использования метода finalize().
-constructors.declaration.grouping=Все конструкторы должны быть сгруппированы вместе. Предыдущий конструктор находился на строке с номером ''{0}''.
+constructors.declaration.grouping=Конструкторы должны быть сгруппированы вместе. Первый конструктор объявлен в строке ''{0}''.
 covariant.equals=Ковариантный equals() без переопределения equals(java.lang.Object).
 declaration.order.access=Объявляйте переменные в порядке расширения доступа.
 declaration.order.constructor=Определение конструктора в неправильном порядке.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_tr.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=Dahili atamalar kullanılmamalıdır.
 avoid.clone.method=''clone'' metodu kullanılmamalıdır.
 avoid.double.brace.init=Çift ayraç başlatmadan kaçının.
 avoid.finalizer.method=''finalize'' metodu kullanılmamalıdır.
-constructors.declaration.grouping=Tüm kurucular birlikte gruplandırılmalıdır. Önceki kurucu ''{0}'' satır numarasındaydı.
+constructors.declaration.grouping=Yapıcılar birlikte gruplandırılmalıdır. İlk kurucu ''{0}'' satırında bildirildi.
 covariant.equals=java.lang.Object sınıfının ''equals'' metodundan başka bir ''equals'' metodu tanımlanmış, java.lang.Object sınıfından gelen ''equals'' metodu da ezilmelidir (override).
 declaration.order.access=Değişken, erişim seviyesine göre yanlış sırada tanımlanmış.
 declaration.order.constructor=''constructor'' tanımı yanlış sırada yapılmış.

src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages_zh.properties

@@ -3,7 +3,7 @@ assignment.inner.avoid=应避免在子表达式中赋值。
 avoid.clone.method=避免重写 clone 方法。
 avoid.double.brace.init=避免双括号初始化。
 avoid.finalizer.method=避免重写finalize方法。
-constructors.declaration.grouping=所有构造函数应该组合在一起。先前的构造函数位于行号''{0}''
+constructors.declaration.grouping=构造函数应该组合在一起。第一个构造函数在''{0}''行声明。
 covariant.equals=重写''equals()''方法时，必须确保重写了''equals(java.lang.Object)''方法。
 declaration.order.access=属性访问器定义顺序错误。
 declaration.order.constructor=构造器定义顺序错误。

src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/coding/ConstructorsDeclarationGroupingCheck.xml

@@ -6,9 +6,11 @@
              parent="com.puppycrawl.tools.checkstyle.TreeWalker">
          <description>&lt;p&gt;
  Checks that all constructors are grouped together.
- If there is any code between the constructors
- then this check will give an error.
- Comments between constructors are ignored.
+ If there is any other type of code (like method or variable declarations, etc)
+ separating the constructors then this check will log a violation.
+ The check identifies and logs any constructors that are not grouped together.
+ The violation message will specify the line number from where the constructors got separated.
+ Comments between constructors are allowed.
  &lt;/p&gt;</description>
          <message-keys>
             <message-key key="constructors.declaration.grouping"/>

src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/ConstructorsDeclarationGroupingCheckTest.java

@@ -22,17 +22,9 @@
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.puppycrawl.tools.checkstyle.checks.coding.ConstructorsDeclarationGroupingCheck.MSG_KEY;
 
-import java.io.File;
-import java.util.Map;
-import java.util.Optional;
-
 import org.junit.jupiter.api.Test;
 
 import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
-import com.puppycrawl.tools.checkstyle.JavaParser;
-import com.puppycrawl.tools.checkstyle.api.DetailAST;
-import com.puppycrawl.tools.checkstyle.api.TokenTypes;
-import com.puppycrawl.tools.checkstyle.internal.utils.TestUtil;
 
 public class ConstructorsDeclarationGroupingCheckTest extends AbstractModuleTestSupport {
     @Override
@@ -43,12 +35,17 @@ protected String getPackageLocation() {
     @Test
     public void testDefault() throws Exception {
         final String[] expected = {
-            "23:5: " + getCheckMessage(MSG_KEY, 19),
-            "27:5: " + getCheckMessage(MSG_KEY, 23),
-            "41:9: " + getCheckMessage(MSG_KEY, 37),
-            "50:13: " + getCheckMessage(MSG_KEY, 44),
-            "53:9: " + getCheckMessage(MSG_KEY, 41),
-            "56:5: " + getCheckMessage(MSG_KEY, 27),
+            "23:5: " + getCheckMessage(MSG_KEY, 17),
+            "27:5: " + getCheckMessage(MSG_KEY, 17),
+            "43:9: " + getCheckMessage(MSG_KEY, 35),
+            "52:13: " + getCheckMessage(MSG_KEY, 46),
+            "55:9: " + getCheckMessage(MSG_KEY, 35),
+            "58:5: " + getCheckMessage(MSG_KEY, 17),
+            "60:5: " + getCheckMessage(MSG_KEY, 17),
+            "78:7: " + getCheckMessage(MSG_KEY, 72),
+            "82:7: " + getCheckMessage(MSG_KEY, 72),
+            "84:7: " + getCheckMessage(MSG_KEY, 72),
+            "87:5: " + getCheckMessage(MSG_KEY, 17),
         };
         verifyWithInlineConfigParser(
                 getPath("InputConstructorsDeclarationGrouping.java"), expected);
@@ -58,8 +55,10 @@ public void testDefault() throws Exception {
     public void testConstructorsDeclarationGroupingRecords() throws Exception {
 
         final String[] expected = {
-            "18:9: " + getCheckMessage(MSG_KEY, 12),
-            "34:9: " + getCheckMessage(MSG_KEY, 30),
+            "20:9: " + getCheckMessage(MSG_KEY, 12),
+            "22:9: " + getCheckMessage(MSG_KEY, 12),
+            "26:9: " + getCheckMessage(MSG_KEY, 12),
+            "42:9: " + getCheckMessage(MSG_KEY, 34),
         };
 
         verifyWithInlineConfigParser(
@@ -82,32 +81,4 @@ public void testTokensNotNull() {
                 .isNotNull();
     }
 
-    /**
-     * We cannot reproduce situation when visitToken is called and leaveToken is not.
-     * So, we have to use reflection to be sure that even in such situation
-     * state of the field will be cleared.
-     *
-     * @throws Exception when code tested throws exception
-     */
-    @Test
-    @SuppressWarnings("unchecked")
-    public void testClearState() throws Exception {
-        final ConstructorsDeclarationGroupingCheck check =
-                        new ConstructorsDeclarationGroupingCheck();
-        final Optional<DetailAST> ctorDef = TestUtil.findTokenInAstByPredicate(
-                JavaParser.parseFile(new File(getNonCompilablePath(
-                        "InputConstructorsDeclarationGroupingRecords.java")),
-                        JavaParser.Options.WITHOUT_COMMENTS),
-                ast -> ast.getType() == TokenTypes.CTOR_DEF);
-
-        assertWithMessage("Ast should contain CTOR_DEF")
-                .that(ctorDef.isPresent())
-                .isTrue();
-        assertWithMessage("State is not cleared on beginTree")
-                .that(TestUtil.isStatefulFieldClearedDuringBeginTree(check, ctorDef.get(),
-                        "allObjBlocks",
-                        allObjBlocks -> ((Map<DetailAST, Integer>) allObjBlocks).isEmpty()))
-                .isTrue();
-    }
-
 }