minor: update checks(related to checkstyle issue#10100) #851

nrmancuso · 2021-06-15T19:38:45Z

Original reports from Checkstyle PR showing regression:

https://nmancus1.github.io/issue-10100_check_diff_reports_2021_06_11/diff_sevntu-check-regression_part_1/index.html

NoMainMethodInAbstractClassCheck- new NPE
ForbidWildcardAsReturnType - lots of new legit violations, we were missing violations on methods like public Class<?>[] getClasses() {}
InnerClass - symmetrical column changes
StaticMethodCandidate - new violations, we were missing violations previously on methods with String return type

https://nmancus1.github.io/issue-10100_check_diff_reports_2021_06_11/diff_sevntu-check-regression_part_2/index.html

MoveVariableInsideIf - symmetrical column number changes
SimpleAccessorNameNotation - symmetrical column number changes
CustomDeclarationOrder - symmetrical column number changes

New reports showing no regression:

https://nmancus1.github.io/issue-10100_check_diff_reports_2021_06_16/diff_sevntu-check-regression_part_1/index.html

Symmetric column changes and new legitimate violations
NoMainMethodInAbstractClassCheck- NPE is gone

https://nmancus1.github.io/issue-10100_check_diff_reports_2021_06_16/diff_sevntu-check-regression_part_2/index.html

Symmetric column changes only

All other changes were because of failing existing unit tests (found via mvn clean verify).

I will let the build fail to show that the changes in this PR are dependent on the changes at checkstyle/checkstyle#10104, then I will change the Checkstyle repo to my PR to show that all pass.

Failed build (I had to add mvn clean verify to CI):

Results :

Failed tests: 
  NoMainMethodInAbstractClassCheckTest.testDefault:87->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 0 expected:<...actClassCheck.java:1[00]:5: Main method insi...> but was:<...actClassCheck.java:1[16]:5: Main method insi...>
  ForbidWildcardAsReturnTypeCheckTest.testNewArrayStructure:461->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 0 expected:<[/pipeline/source/sevntu-checks/src/test/resources/com/github/sevntu/checkstyle/checks/design/InputForbidWildcardAsReturnTypeCheckNewArrayStructure.java:4:5: Wildcard as return type should be avoided].> but was:<[Audit done].>
  PreferMethodReferenceCheckTest.testObjectCreation:146->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 2 expected:<...ferenceCheck4.java:1[7]:39: Lambda can be r...> but was:<...ferenceCheck4.java:1[8]:39: Lambda can be r...>
  PreferMethodReferenceCheckTest.testStatementsListsWithExpressions:128->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 11 expected:<...eferenceCheck3.java:[46]:40: Lambda can be r...> but was:<...eferenceCheck3.java:[50]:40: Lambda can be r...>
  Jsr305AnnotationsCheckTest.testArrays:161->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 6 expected:<...kWithArrays.java:90:[5]: Return value must ...> but was:<...kWithArrays.java:90:[8]: Return value must ...>
  CustomDeclarationOrderCheckTest.mainMethod:189->AbstractModuleTestSupport.verify:229->AbstractModuleTestSupport.verify:267->AbstractModuleTestSupport.verify:304 error message 0 expected:<...d(private )' then 'M[ainM]ethod(.*)'.> but was:<...d(private )' then 'M[]ethod(.*)'.>

Tests run: 411, Failures: 6, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 32.994 s
[INFO] Finished at: 2021-06-16T11:42:15Z
[INFO] Final Memory: 56M/980M
[INFO] ---------------------------------------------------------------------

Full log at https://app.wercker.com/checkstyle/sevntu.checkstyle/runs/build/60c9e32c237ec900086455fa?step=60c9e3678ea84100087595c9

mvn clean verify is passing with Checkstyle version from checkstyle/checkstyle#10104

rnveach · 2021-06-16T02:05:41Z

the changes in this PR are dependent on the changes at checkstyle/checkstyle#10104

This means this PR is dependent on 4 things before it can be merged:

Issue #10100: Extract parameter type from nested array declarator brackets checkstyle/checkstyle#10104 must be merged (UPDATE: DONE)
checkstyle must release those changes (DONE - merged and released as 8.44)
eclipse-cs must release with that new version of checkstyle
The version of checkstyle in sevntu must update to the new eclipse-cs version (see https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L18-L19 )

nrmancuso · 2021-06-16T11:56:49Z

...ks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java

                final String parameterName =
-                        getIdentifier(arrayDeclaratorAST);
+                        parameterTypeAST.getFirstChild().getText();


Parameter type is always first child now, it is no longer nested.

nrmancuso · 2021-06-16T11:58:07Z

...-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/Jsr305AnnotationsCheck.java

@@ -585,22 +586,17 @@ private static boolean isPrimitiveType(DetailAST ast) {
    /**
     * Checks whether token is array or elipsis.
     *
-     * @param identToken
+     * @param typeToken


Previously named identToken, this DetailAST was actually never an IDENT. In every UT we have, this method is passed a TYPE ast.

nrmancuso · 2021-06-16T12:00:04Z

...-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/Jsr305AnnotationsCheck.java

+    private static boolean isArrayOrElipsis(final DetailAST typeToken) {
+        final DetailAST next = typeToken.getNextSibling();
+        final boolean isArrayDeclarator =
+                typeToken.findFirstToken(TokenTypes.ARRAY_DECLARATOR) != null;
+        final boolean hasArrayOrEllipses =
+                TokenUtil.isOfType(next, TokenTypes.ARRAY_DECLARATOR, TokenTypes.ELLIPSIS);
+        return hasArrayOrEllipses || isArrayDeclarator;


Switch was overkill for two branches, refactored this and modified logic a bit for new type ast.

nrmancuso · 2021-06-16T12:02:26Z

...va/com/github/sevntu/checkstyle/checks/coding/MultipleVariableDeclarationsExtendedCheck.java

@@ -167,9 +167,7 @@ private static DetailAST getLastNode(final DetailAST node) {
        while (child != null) {
            final DetailAST newNode = getLastNode(child);
            if (newNode.getLineNo() > currentNode.getLineNo()
-                    || newNode.getLineNo()
-                        == currentNode.getLineNo() && newNode


After main repo PR, this line was not covered, and no cases to cover it were discovered in regression testing.

nrmancuso · 2021-06-16T12:04:58Z

...cks/src/main/java/com/github/sevntu/checkstyle/checks/coding/PreferMethodReferenceCheck.java

-            result = ident != null && name.equals(ident.getText());
-        }
-        else if (nestedDecl != null && expr == null) {
-            result = isArgUsedInArrayInit(nestedDecl, name);


No need for recursion here anymore, we can just iterate through the sequential ARRAY_DECLARATOR brackets and check if they have an EXPR child (i.e. [2]), then simply check the first [a] to make sure that the lambda parameter and expression match.

nrmancuso · 2021-06-16T12:05:19Z

...c/main/java/com/github/sevntu/checkstyle/checks/design/NoMainMethodInAbstractClassCheck.java

                final String parameterName =
-                        getIdentifier(arrayDeclaratorAST);
+                        parameterTypeAST.getFirstChild().getText();


Same as above, type is always the first child.

nrmancuso · 2021-06-16T12:05:50Z

...est/java/com/github/sevntu/checkstyle/checks/design/ForbidWildcardAsReturnTypeCheckTest.java

+                createModuleConfig(ForbidWildcardAsReturnTypeCheck.class);
+
+        final String[] expected = {
+            "4:5: " + getCheckMessage(MSG_KEY),


New violation found in regression testing.

wercker.yml

rnveach · 2021-06-16T18:21:51Z

Added blocked label because of #851 (comment) to prevent accidental merging. Label can be removed when this PR can be safely merged.

ghost · 2021-07-19T10:29:49Z

Hi, I am a beginner at this issue can you help me. How to start it?

coveralls · 2021-08-09T15:56:12Z

Coverage decreased (-0.0009%) to 98.796% when pulling 850a091 on nmancus1:checkstyle-10100 into b7c8c72 on sevntu-checkstyle:master.

nrmancuso marked this pull request as draft June 15, 2021 19:38

nrmancuso force-pushed the checkstyle-10100 branch 2 times, most recently from 0f60cbe to ab6c95e Compare June 16, 2021 11:54

nrmancuso commented Jun 16, 2021

View reviewed changes

nrmancuso force-pushed the checkstyle-10100 branch from ab6c95e to 8d4fd06 Compare June 16, 2021 12:07

nrmancuso marked this pull request as ready for review June 16, 2021 12:16

nrmancuso commented Jun 16, 2021

View reviewed changes

wercker.yml Outdated Show resolved Hide resolved

nrmancuso mentioned this pull request Jun 16, 2021

infra: add 'mvn clean verify' to CI #853

Closed

rnveach added the blocked label Jun 16, 2021

nrmancuso mentioned this pull request Jun 17, 2021

Issue #10100: Extract parameter type from nested array declarator brackets checkstyle/checkstyle#10104

Merged

nrmancuso mentioned this pull request Aug 8, 2021

minor: update checks (related to checkstyle#3095) #858

Merged

nrmancuso added 2 commits August 9, 2021 11:46

minor: update checks(related to checkstyle issue#10100)

4e92d7b

config: bump eclipse-cs version

850a091

nrmancuso force-pushed the checkstyle-10100 branch from 8d4fd06 to 850a091 Compare August 9, 2021 15:50

romani merged commit 9b12f7c into sevntu-checkstyle:master Aug 9, 2021

nrmancuso deleted the checkstyle-10100 branch August 10, 2021 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor: update checks(related to checkstyle issue#10100) #851

minor: update checks(related to checkstyle issue#10100) #851

nrmancuso commented Jun 15, 2021 •

edited

Loading

rnveach commented Jun 16, 2021 •

edited by romani

Loading

nrmancuso Jun 16, 2021

nrmancuso Jun 16, 2021 •

edited

Loading

nrmancuso Jun 16, 2021

nrmancuso Jun 16, 2021

nrmancuso Jun 16, 2021

nrmancuso Jun 16, 2021

nrmancuso Jun 16, 2021

rnveach commented Jun 16, 2021 •

edited

Loading

ghost commented Jul 19, 2021

coveralls commented Aug 9, 2021

minor: update checks(related to checkstyle issue#10100) #851

minor: update checks(related to checkstyle issue#10100) #851

Conversation

nrmancuso commented Jun 15, 2021 • edited Loading

Original reports from Checkstyle PR showing regression:

New reports showing no regression:

rnveach commented Jun 16, 2021 • edited by romani Loading

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

nrmancuso Jun 16, 2021

Choose a reason for hiding this comment

rnveach commented Jun 16, 2021 • edited Loading

ghost commented Jul 19, 2021

coveralls commented Aug 9, 2021

nrmancuso commented Jun 15, 2021 •

edited

Loading

rnveach commented Jun 16, 2021 •

edited by romani

Loading

nrmancuso Jun 16, 2021 •

edited

Loading

rnveach commented Jun 16, 2021 •

edited

Loading