Issue #13758: Add pattern array converter for auto bean #14740

Lmh-java · 2024-03-30T07:28:21Z

For Issue #13758.

Create auto configuration for Pattern[] with a fixed separator (in this PR)
Update AbstractClassCouplingCheck to use this new property type (in this PR)
Update ImportOrderCheck to replace use of Pattern[] with String[] (In this PR)

Agreement on update is archived at #14764 (comment)

Lmh-java · 2024-03-30T07:39:28Z

#14740 (comment)

In this PR, I have:

Create auto configuration for Pattern[] .....

@rnveach @romani Could you please share your thoughts on this plan? I truly appreciate your opinions.

Also, for #13758 (comment), which one do you think would be a better solution in our case?

Update: CI is failing, but I will fix them later if we've decided the plan :).

romani

Please read #13758 (comment)

item to disucuss

romani · 2024-04-05T13:39:02Z

src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

+        @Override
+        public Object convert(Class type, Object value) {
+            final String plainString = value.toString();
+            final String[] patternStrings = plainString.split(REGEX_SEPARATOR);


bad side effect of this is we should clearly mention that we do not allow , as part of regexp to match what user needs. So user can not use , in it Patterns, it is delimiter.
\, will also not work, as we simply splitting for just ,.

@romani, do you want me to close this PR for now? Since it might be confusing for others. We can re-open it later to further modify Pattern[] if it is the right time. :)

Let's keep it open until we come to agreement on how to fix issue.

romani · 2024-04-08T14:39:06Z

@Lmh-java , please put in this PR all from #14764 (comment)

config/linkcheck-suppressions.txt

config/pitest-suppressions/pitest-imports-suppressions.xml

Lmh-java · 2024-04-09T06:37:39Z

config/pmd.xml

@@ -316,7 +316,8 @@
      <!-- Main has an annotated field for each command line option. This is by design. -->
      <property name="violationSuppressXPath"
                value="//ClassOrInterfaceDeclaration[@SimpleName='Checker']
-    | //ClassOrInterfaceDeclaration[@SimpleName='Main']"/>
+    | //ClassOrInterfaceDeclaration[@SimpleName='Main']
+    | //ClassOrInterfaceDeclaration[@SimpleName='ImportOrderCheck']"/>


Suppress this because PWD complains that ImportOrderCheck has too many class fields.

I am ok with this.

Lmh-java · 2024-04-09T06:42:04Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheck.java

+    /**
+     * Complied array of patterns for property {@code groups}.
+     */
+    private Pattern[] groupsReg = EMPTY_PATTERN_ARRAY;
+
+    /**
+     * Complied array of patterns for property {@code staticGroups}.
+     */
+    private Pattern[] staticGroupsReg = EMPTY_PATTERN_ARRAY;
+


Add these two more fields to store the compiled regex expressions. I have to expose String[] to users, but we also need to store the complied regex (we don't want to compile every time we want to use the regex). I am curious whether there are better solutions to this, since this change brings us more suppressions of all kinds.

To next reviewers:
to avoid dealing with long discussion on what is actually a type that user should care, I am ok with this update.
I hope in some future we will agree on type system and simplify this Check

Lmh-java · 2024-04-10T09:47:48Z

...ols/checkstyle/checks/metrics/classfanoutcomplexity/InputClassFanOutComplexityLambdaNew.java

+        String string = "";
+        entry.values().stream()
+                .flatMap(List::stream)
+                .map(Constraint::new)


Constraint::new will not be recognized as a reference and will not be recorded. I am adding this test file to pass CI right now. I think it would be better if we could fix this false negative in a separated PR to make everything easier.

Please use here File class, as you did in issue description

Lmh-java · 2024-04-10T09:49:29Z

...test/java/com/puppycrawl/tools/checkstyle/checks/metrics/ClassFanOutComplexityCheckTest.java

+        final String[] expected = {};
+
+        verifyWithInlineConfigParser(
+                getPath("InputClassFanOutComplexityLambdaNew.java"), expected);


This test case is not correct, but I have to add it to pass the current CI. InputClassFanOutComplexityLambdaNew.java should NOT have 0 complexity.
See #14740 (comment)

Please put comment, in code, no violations until #14787

Lmh-java · 2024-04-10T10:04:39Z

ClassFanOut produces violations when running no-exception-xwiki if I remove

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

Line 121 in 7d5d4d2

excludeClassesRegexps.add(CommonUtil.createPattern("^$"));

But if I keep it, Pitest will complain about it. If this function call is removed by Pietist, nothing in our unit test will complain.

After investigating it, I found that if this line is removed, sometimes there will be a few more class references counted. This is because when handling lambda expressions with a new keyword (such as Constrant::new), the following code will get "" (an blank string)

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

Line 398 in 7d5d4d2

final String fullIdentName = FullIdent.createFullIdent(ast).getText();

Therefore, ^$ will match the blank string and ignore that. However, if we set the default value of excludeClassesRegexps to an empty array. This case will not be ignored, and therefore more class references will be counted.

Ignoring such lambda expressions with ^$ can be problematic. I think the test example I added is a false negative of this case, which no reference classes are counted.
https://github.com/checkstyle/checkstyle/blob/7d5d4d21e3a139ec80120f02e08def8d347f9911/src/test/resources/com/puppycrawl/tools/checkstyle/checks/metrics/classfanoutcomplexity/InputClassFanOutComplexityLambdaNew.java
I will keep investigating this and send a new PR to fix this. For now, I have come up with a workaround to support us to finish this PR first before we move on.

PS: this is an issue with AbstractClassCouplingCheck, so it also affects ClassFanOutComplexityCheck and ClassDataAbstractionCouplingCheck

romani

items:

config/pitest-suppressions/pitest-imports-suppressions.xml

romani

please apply review items to all cases:

src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java

src/test/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBeanTest.java

romani

Items

src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

romani

Ok to merge

rnveach

Start Issue #13758.

How does this start the issue but doesn't completely resolve it? What is planned next?

src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

src/xdocs/property_types.xml

rnveach · 2024-04-16T00:22:16Z

...test/java/com/puppycrawl/tools/checkstyle/checks/metrics/ClassFanOutComplexityCheckTest.java

@@ -371,4 +371,18 @@ public void testClearStatePackageName() throws Exception {
                .isTrue();
    }

+    /**
+     * Test [ClassName]::new expression. Such expression will be processed as a


What does this have to do with the pattern array converter? All the properties in the file are default which means it doesn't go against the converter, right?

This has nothing to do with the converter. Please see #14740 (comment).

This is a part of the workaround for ClassFanOutCheck until #14787

TLDR;
I have to keep the following line, otherwise the CI will fail.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/metrics/AbstractClassCouplingCheck.java

Line 121 in 7d5d4d2

excludeClassesRegexps.add(CommonUtil.createPattern("^$"));

Added this test case to kill Pitest mutation for this line. After #14787 is solved, this test case will report violations.

rnveach · 2024-04-16T00:23:55Z

src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/imports/ImportOrderCheck.xml

@@ -40,7 +40,7 @@
 &lt;a href="https://en.wikipedia.org/wiki/ASCII#Order"&gt;ASCII sort order&lt;/a&gt;.
 It affects both type imports and static imports.</description>
            </property>
-            <property default-value="" name="groups" type="java.util.regex.Pattern[]">
+            <property default-value="" name="groups" type="java.lang.String[]">


Has it been discussed what impact this has on 3rd party tools like eclipse-cs or sonar-cs?

It will be declared breaking compatibility, I hope eclipscs will not be actually affected.

So we are not going to do any analysis or warning?

yes, let move forward, and just declare breaking changes.

rnveach · 2024-04-20T16:09:36Z

src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

+            final String plainString = value.toString();
+            Pattern[] patterns = new Pattern[0];
+            if (!CommonUtil.isBlank(plainString)) {
+                final String[] patternStrings = plainString.split(COMMA_SEPARATOR);


Sorry I missed this before.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

Line 393 in 8dc065a

value.toString().trim(), COMMA_SEPARATOR);

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java

Line 398 in 8dc065a

result.add(token.trim());

This is similar to RelaxedStringArrayConverter and it trims the input during the split. You are not.

You can see in testBeanConverterPatternArray that the space remains in the regular expression. This violates your example you added in property_types.xml as it shows the space removed.

Originally we were using RelaxedStringArrayConverter for these type of instances, so we should continue the trimming it does. We need to implement trim in this method. I would expect spaces to be trimmed.

Sorry I missed this before.

Totally no worries. This is a great catch. I was about to ask about this, but as time went by, I forgot...

Added trim. I have also updated the new converter to have the same structure with RelaxedStringArrayConverter, so we can reduce one import to Arrays.

rnveach · 2024-04-20T16:10:51Z

Start Issue #13758.

How does this start the issue but doesn't completely resolve it? What is planned next?

I did not get a response, but I see text was updated in first post. I am more curious now. Is this PR plan to finish the issue?

Lmh-java · 2024-04-20T19:41:15Z

Start Issue #13758.

How does this start the issue but doesn't completely resolve it? What is planned next?

I did not get a response, but I see text was updated in first post. I am more curious now. Is this PR plan to finish the issue?

@rnveach
Yes, I had updated the old PR description. Sorry for forgetting to reply the message. All these 3 steps listed now should be completed in this PR, and it should finish the issue.

The only thing needs to be done after this PR is #14787, but it is not relevant to #13758 (not relevant to unify pattern array.)

nrmancuso · 2024-05-01T11:51:45Z

Github, generate site

github-actions · 2024-05-01T11:57:46Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9f55855_2024115508/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9f55855_2024115508/checks/imports/importorder.html#Properties

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9f55855_2024115508/property_types.html#

rnveach · 2024-05-02T00:06:29Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9f55855_2024115508/property_types.html#

Weird it failed to generated the fragment Pattern[].

Edit: From just glancing, it looks like the code is looking at the source for subsection with an id and not section with a name.

Lmh-java · 2024-05-03T03:26:14Z

@rnveach
Could you please explain a little more on "fragment Pattern[]". I am not very sure what it means.
I can see Pattern[] on the site.
Pattern[] section:

Content section.

rnveach · 2024-05-03T03:55:16Z

https://en.wikipedia.org/wiki/URI_fragment
In the URL, it is represented by the # and what comes after it.

#14740 (comment)
The links generated by the GHA did not provide the correct fragment for the last one. My edit provided why it didn't.

Lmh-java · 2024-05-03T04:03:13Z

@rnveach I get it right now.

So we expect the last link to be https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9f55855_2024115508/property_types.html#Pattern.5B.5D
(with an anchor in the end)

I will open an issue for this to the main checkstyle repo.

nrmancuso · 2024-05-03T11:27:28Z

@Lmh-java i think we might have an issue open for it, please try to search and see if it turns up

Lmh-java · 2024-05-04T11:53:39Z

I didn't see an issue related to this specifically, I will try to open an issue.

Edit: please see issue #14857. Not sure about my wording, please feel free to change.

nrmancuso · 2024-05-07T13:16:00Z

Thanks @Lmh-java !

Lmh-java marked this pull request as ready for review March 30, 2024 07:34

romani requested changes Apr 5, 2024

View reviewed changes

rnveach mentioned this pull request Apr 6, 2024

Issue #13758: Fix documentation of pattern array #14764

Closed

Lmh-java force-pushed the minghao/unify-pattern-array branch 5 times, most recently from 6869cde to 5b49c08 Compare April 9, 2024 05:57

Lmh-java commented Apr 9, 2024

View reviewed changes

config/linkcheck-suppressions.txt Show resolved Hide resolved

Lmh-java commented Apr 9, 2024

View reviewed changes

config/pitest-suppressions/pitest-imports-suppressions.xml Outdated Show resolved Hide resolved

Lmh-java commented Apr 9, 2024

View reviewed changes

Lmh-java force-pushed the minghao/unify-pattern-array branch 2 times, most recently from 3080d29 to 7d5d4d2 Compare April 10, 2024 09:44

Lmh-java commented Apr 10, 2024

View reviewed changes

Lmh-java requested a review from romani April 10, 2024 10:07

romani requested changes Apr 11, 2024

View reviewed changes

config/pitest-suppressions/pitest-imports-suppressions.xml Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/unify-pattern-array branch from 7d5d4d2 to fdf77ce Compare April 12, 2024 00:28

Lmh-java mentioned this pull request Apr 12, 2024

False Negative of ClassFanOutCheck with "new" Keyword #14787

Open

Lmh-java force-pushed the minghao/unify-pattern-array branch from fdf77ce to 1a4fb0f Compare April 12, 2024 06:19

Lmh-java requested a review from romani April 12, 2024 06:48

romani requested changes Apr 13, 2024

View reviewed changes

Lmh-java force-pushed the minghao/unify-pattern-array branch 2 times, most recently from a70942d to 9d67d78 Compare April 13, 2024 22:21

romani requested changes Apr 13, 2024

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/AbstractAutomaticBean.java Outdated Show resolved Hide resolved

Lmh-java force-pushed the minghao/unify-pattern-array branch from 9d67d78 to a0fb82f Compare April 14, 2024 01:29

romani approved these changes Apr 14, 2024

View reviewed changes

romani assigned rnveach Apr 14, 2024

romani requested a review from rnveach April 14, 2024 03:00

rnveach requested changes Apr 16, 2024

View reviewed changes

Lmh-java force-pushed the minghao/unify-pattern-array branch from 9c35f3e to 8dc065a Compare April 16, 2024 06:18

Lmh-java requested a review from rnveach April 16, 2024 19:52

rnveach requested changes Apr 20, 2024

View reviewed changes

Lmh-java force-pushed the minghao/unify-pattern-array branch 2 times, most recently from aa80438 to 6bdcb72 Compare April 20, 2024 20:36

Lmh-java requested a review from rnveach April 20, 2024 20:55

Issue checkstyle#13758: Add pattern array converter for auto bean

9f55855

Lmh-java force-pushed the minghao/unify-pattern-array branch from 6bdcb72 to 9f55855 Compare April 20, 2024 21:03

rnveach approved these changes Apr 24, 2024

View reviewed changes

rnveach assigned nrmancuso and unassigned rnveach Apr 24, 2024

rnveach requested a review from nrmancuso April 24, 2024 16:51

nrmancuso approved these changes May 2, 2024

View reviewed changes

nrmancuso merged commit 67dd5b4 into checkstyle:master May 2, 2024
113 checks passed

Lmh-java mentioned this pull request May 4, 2024

Github generate site fails to generate links with anchors. #14857

Open

Issue #13758: Add pattern array converter for auto bean #14740

Issue #13758: Add pattern array converter for auto bean #14740

Conversation

Lmh-java commented Mar 30, 2024 • edited

Lmh-java commented Mar 30, 2024 • edited

romani left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Apr 8, 2024

Lmh-java Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 10, 2024 • edited

Choose a reason for hiding this comment

romani Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java commented Apr 10, 2024 • edited

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 16, 2024 • edited

Choose a reason for hiding this comment

rnveach Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Apr 20, 2024 • edited

Choose a reason for hiding this comment

rnveach commented Apr 20, 2024

Lmh-java commented Apr 20, 2024 • edited

nrmancuso commented May 1, 2024

github-actions bot commented May 1, 2024

rnveach commented May 2, 2024 • edited

Lmh-java commented May 3, 2024

rnveach commented May 3, 2024

Lmh-java commented May 3, 2024 • edited

nrmancuso commented May 3, 2024

Lmh-java commented May 4, 2024 • edited

nrmancuso commented May 7, 2024

Lmh-java commented Mar 30, 2024 •

edited

Lmh-java commented Mar 30, 2024 •

edited

romani left a comment •

edited

Lmh-java Apr 9, 2024 •

edited

romani Apr 13, 2024 •

edited

Lmh-java Apr 10, 2024 •

edited

romani Apr 13, 2024 •

edited

Lmh-java commented Apr 10, 2024 •

edited

Lmh-java Apr 16, 2024 •

edited

rnveach Apr 16, 2024 •

edited

Lmh-java Apr 20, 2024 •

edited

Lmh-java commented Apr 20, 2024 •

edited

rnveach commented May 2, 2024 •

edited

Lmh-java commented May 3, 2024 •

edited

Lmh-java commented May 4, 2024 •

edited