Issue #11087: New check ChainedMethodCallWrap #11231

Vyom-Yadav · 2022-01-24T07:56:33Z

Resolves #11087: New check chained method call wrap

Diff Regression config: https://gist.githubusercontent.com/Vyom-Yadav/98dceb63a79f4833e85fff9b2e1464a6/raw/e5d475dd0a62d241e4aa20e58f607672133859fb/my_checks.xml
Diff Regression patch config: https://gist.githubusercontent.com/Vyom-Yadav/273327b9c6cd6963b7dade792bc2c9c2/raw/722734822d2766fded891ddf25819194ba6cdedf/patch_config.xml
Report label: optionalMethodCallsAllProjects

Default config of check:

<module name="ChainedMethodCallWrap">
 <property name="identifierPattern" value="^$"/>
 <property name="maxCallsInSingleLine" value="1" />
 <property name="maxCallsPerLine" value="1" />
</module>

Config used by checkstyle internally:

<module name="ChainedMethodCallWrap">
  <property name="identifierPattern" value="^assert.*"/>
  <property name="maxCallsInSingleLine" value="2" />
</module>

Report for the following config (Match all method calls, report is huge, was used to check for exceptions):

Updated

<module name="ChainedMethodCallWrap">
  <property name="identifierPattern" value="^.*$" />
</module>

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

Report for the config used by checkstyle:

Updated

<module name="ChainedMethodCallWrap">
  <property name="identifierPattern" value="^assert.*"/>
  <property name="maxCallsInSingleLine" value="2" />
</module>

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

Report for FullIdent:

Updated

Checkstyle Checks:
- (VariableDeclarationUsageDistance shows diff:
  False Negative: VariableDeclarationUsageDistance #11335)
  https://vyom-yadav.github.io/DiffReport/diff-full-ident-main/index.html
Sevntu Checks: https://vyom-yadav.github.io/DiffReport/diff-full-ident-sevntu/index.html

Updated

Site: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/793d000_2023110050/config_whitespace.html#ChainedMethodCallWrap

contribution PR: checkstyle/contribution#774

Some thoughts about the default configuration and check rationale: #11231 (comment)

Vyom-Yadav · 2022-01-24T07:57:14Z

src/main/java/com/puppycrawl/tools/checkstyle/api/FullIdent.java

-            final boolean isArrayTypeDeclarationStart = nextSibling != null
-                    && (nextSibling.getType() == TokenTypes.ARRAY_DECLARATOR
-                        || nextSibling.getType() == TokenTypes.ANNOTATIONS)
-                    && isArrayTypeDeclaration(nextSibling);
+            final boolean isArrayTypeDeclarationStart = isArrayTypeDeclarationStart(nextSibling);


Extracted to reduce cognitive complexity.

Vyom-Yadav · 2022-01-24T08:00:49Z

...le/checks/whitespace/chainedmethodcallwrap/InputChainedMethodCallWrapMaxSingleLineCalls.java

+        Truth.assertWithMessage("test").that(1).isNotEqualTo(2);
+                                                       // ^---- ok, specified pattern is different


Though the method being implied is the same, the check matches again the complete method chain, can be changed if asked for but then it will also match for-

SomeOtherBuilder.assertWithMessage().that.....

Please add this to documentation example

Vyom-Yadav · 2022-02-04T05:40:27Z

Github, generate report

github-actions · 2022-02-04T06:27:39Z

Checking for exceptions: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/793cde2_2022062032/reports/diff/index.html

Vyom-Yadav · 2022-02-04T08:47:10Z

Github, generate report

github-actions · 2022-02-04T09:44:17Z

Checking for exceptions: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ce667a2_2022093713/reports/diff/index.html

Vyom-Yadav · 2022-02-04T18:02:03Z

Github, generate report

github-actions · 2022-02-04T18:49:28Z

Checking for exceptions: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/00cdd7b_2022184410/reports/diff/index.html

Vyom-Yadav · 2023-04-07T17:23:17Z

Github, generate report

github-actions · 2023-04-07T17:28:21Z

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

Vyom-Yadav · 2023-04-07T17:34:19Z

<module name="ChainedMethodCallWrap">                             
  <property name="id" value="streamMethodCalls"/>                 
  <property name="identifierPattern" value=".*\.(s|S)tream.*"/>   
</module>

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

<module name="ChainedMethodCallWrap">                             
  <property name="id" value="optionalMethodCalls"/>               
  <property name="identifierPattern" value=".*\.(o|O)ptional.*"/> 
</module>

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e321d3_2023161548/reports/diff/index.html (clean)

<module name="ChainedMethodCallWrap">                             
  <property name="id" value="streamMethodCalls"/>                 
  <property name="identifierPattern" value=".*\.(s|S)tream.*"/>   
  <property name="maxCallsInSingleLine" value="2" />
</module

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

<module name="ChainedMethodCallWrap">                             
  <property name="id" value="optionalMethodCalls"/>               
  <property name="identifierPattern" value=".*\.(o|O)ptional.*"/> 
  <property name="maxCallsInSingleLine" value="2" />
</module>

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e321d3_2023172801/reports/diff/index.html (clean)

For stream method calls, I would prefer 3.

Vyom-Yadav · 2023-04-07T17:34:56Z

Github, generate report

github-actions · 2023-04-07T18:12:41Z

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

Vyom-Yadav · 2023-04-08T04:50:15Z

The optional pattern isn't that useful, stream one still has good usecase

romani · 2023-04-09T05:49:57Z

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

Ma is 2 in config, but 1 in message.
Additionally I don't this proposed change is good for this code.

I have worries on name="identifierPattern usage, users will not like it, to configure for each chains.

Did we consider in design wrapping demands for method that use lambda or method ref?
values().stream() such simple chaining ok to stay in on same line

Vyom-Yadav · 2023-04-21T12:37:41Z

Github, generate site

github-actions · 2023-04-21T12:41:47Z

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

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

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

Vyom-Yadav · 2023-04-21T12:50:53Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e321d3_2023172051/reports/diff/checkstyle/index.html#A10
Ma is 2 in config, but 1 in message.

That violation is for method calls wrapped on multiple lines, that violation is basically:

foo.stream()
.abc().abc() // violation, Wrap chained method call, max allowed on a line is 1.
.abc();

In config, we have set maxCallsInSingleLine = 2, maxCallsInMultiLine is still 1 (default).

romani · 2023-11-29T00:21:45Z

@Vyom-Yadav , please rebase, let's review it one more time

romani

As I mentioned above, if Check is not general we need extend its name with some words to mention this. What identifier is used? Can configure it for stream() (most jdk collections) ? I see diff report on it, can we apply it to our config to be enforced from the beginning.

If we have no energy to continue, I recommend to move it to https://github.com/sevntu-checkstyle/sevntu.checkstyle/tree/master/sevntu-checks as is, no extra changes will be required no questions asked.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e321d3_2023172051/reports/diff/checkstyle/index.html#A10 this looks like false positive, after stream there is no chained call on same line.

Please rebase.

Items:

romani · 2023-11-29T00:46:57Z

config/checkstyle-checks.xml

@@ -134,7 +134,7 @@
  </module>
  <module name="RegexpSingleline">
    <property name="id" value="assertThatShouldBeOnSeparateLine"/>
-    <property name="format" value="assertWithMessage\(.*\).that\("/>
+    <property name="format" value="^[^*]*assertWithMessage\(.*\).that\("/>


If new Check is powerful, why we need this assertThatShouldBeOnSeparateLine now ?
It was good hack without special Check. But if we have special Check, we do not need this instance.

Yes, this isn't required anymore, removed. The check can handle it.

<module name="ChainedMethodCallWrap"> <property name="identifierPattern" value="^assert.*"/> <property name="maxCallsInSingleLine" value="2" /> </module>

romani · 2023-11-29T00:49:36Z

config/checkstyle-checks.xml

@@ -910,6 +910,10 @@
    <module name="TypecastParenPad"/>
    <module name="WhitespaceAfter"/>
    <module name="WhitespaceAround"/>
+    <module name="ChainedMethodCallWrap">
+      <property name="identifierPattern" value="^assert.*"/>
+      <property name="maxCallsInSingleLine" value="2" />


We need to eat food we produce, if we claim that Check is general, let's apply it to other chained code.
If we can not apply it to our codebase, nobody will apply, we do not need unused Checks.

Sure, do you have any method chains in mind you don't want to be changed?

romani · 2023-11-29T00:52:24Z

src/main/java/com/puppycrawl/tools/checkstyle/api/FullIdent.java

+     * @param dotAst ast node of type {@link TokenTypes#DOT}
+     */
+    private static void extractFullIdentFromDotAst(FullIdent full, DetailAST dotAst) {
+        DetailAST firstChildToExtract = dotAst.getFirstChild();


We need to avoid extra implementations in API package.
Is there a way to hide such implementation in Check or some Utility class ?

Yes, this would be required too. All discussion at #11672 and surrounding issues indicate that changing FullIdent will break some classes even if the implementation of FullIdent is correct (which is this one; the master codebase is buggy).

I would also deprecate the usage of FullIdent as it is buggy, and fixing it would require fixing VariableDeclarationDistanceUsage.

Either VariableDeclarationDistanceUsage should have a separate implementation for FullIdent (the current buggy one), which will allow us to fix the main one. What do you suggest?

romani · 2023-11-29T00:53:58Z

.../checks/whitespace/chainedmethodcallwrap/InputChainedMethodCallWrapMaxCallsInSingleLine.java

+        assertWithMessage("test").that(1).isEqualTo(1);
+                           // violation above '3 method calls on single line, max allowed is 2'
+        assertWithMessage("test")
+                .that(1).isNotEqualTo(2); // ok


Please remove //ok we can keep ok comments with details only (extra text)

romani · 2023-11-29T00:58:39Z

src/xdocs/config_whitespace.xml

+            <tr>
+              <td>identifierPattern</td>
+              <td>
+                Specify method calls to be checked. By default, identifierPattern matches


I have problems to understand what is identifier
https://checkstyle.org/property_types.html we need some explanation in this Check on this term.

In examples I see that class name can be used, but it conflicts with Specify method calls.

The identifier comprises of the actual words in the chained method call. It comprises of method name, the object on which the method is invoked, the constructor in between. Example:
new Pizza.Builder().build();, here the methodIdentifier we obtain is Pizza.Builder.build

Vyom-Yadav · 2023-11-30T08:13:54Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9e321d3_2023172051/reports/diff/checkstyle/index.html#A10 this looks like false positive, after stream there is no chained call on same line.

@romani No, it is not, the identifierPattern is .*\.(s|S)tream.*, so it will match typeDeclAstToTypeDeclDesc.values().stream(). Where the first method call is values() and second is stream(). Hence the violation Wrap chained method call, max allowed on a line is 1. is correct.

I see where you are coming from, you want to ignore all calls before stream(), and just verify after that. If it was abc.stream(), the check would work in the way you want as there are no method calls before stream()

The current design does not allow you to ignore certain method calls before or after a method call, that can be added is an enhancement later on.

Vyom-Yadav · 2023-11-30T09:40:19Z

I will fix CI errors after we come to some conclusions on various aspects.

romani · 2023-11-30T20:13:17Z

I see where you are coming from, you want to ignore all calls before stream()

I just try to understand how it works, and I am failing, so it means that nobody will understand it, so nobody will use it.
We need to figure out how to make it easier to understand for user on how to configure it.

Hence the violation Wrap chained method call, max allowed on a line is 1. is correct.

in my mind code is good, I do not see a reason to change it. May be we should make regexp end position as definition from where to demand wrapping? So it will be like "wrap after stream()" that is how most people are doing wrapping of chained.

The current design does not allow you to ignore certain method calls before or after a method call, that can be added is an enhancement later on.

that is my point, lets move it to senvtu project and start to use and in real practice see what changes in design are required.
And have full freedom to experiement with properties and breaking changes until we come to good state of Check.

Vyom-Yadav · 2023-12-02T02:55:43Z

Alright, let's move it to sevntu and iterate over there. What do you want to do about FullIdent? A copy for just this check?

romani · 2023-12-02T03:21:23Z

Just copy required code as you need and code for full ident too.

nrmancuso · 2024-02-23T04:26:58Z

@Vyom-Yadav let's close this PR, code and comments here will stay forever in Github, and we can come back and move it to sevntu at any time.

Vyom-Yadav commented Jan 24, 2022

View reviewed changes

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch 14 times, most recently from 5f1ade3 to e279be6 Compare January 25, 2022 07:36

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch 2 times, most recently from 558ff1b to 793cde2 Compare February 4, 2022 05:33

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch 3 times, most recently from 01b6534 to ce667a2 Compare February 4, 2022 08:46

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from ce667a2 to b6d4f18 Compare February 4, 2022 09:32

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from b6d4f18 to 00cdd7b Compare February 4, 2022 17:54

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from 00cdd7b to 7cd43c9 Compare February 5, 2022 05:32

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from 9e321d3 to e30a481 Compare April 21, 2023 17:18

nrmancuso assigned romani and unassigned nrmancuso and romani Nov 13, 2023

romani requested changes Nov 29, 2023

View reviewed changes

romani assigned romani and Vyom-Yadav and unassigned romani Nov 29, 2023

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from e30a481 to 97e5ad7 Compare November 30, 2023 08:57

Issue checkstyle#11087: New check ChainedMethodCallWrap

e1b7e12

Vyom-Yadav force-pushed the newCheckChainedMethodCallWrap branch from 97e5ad7 to e1b7e12 Compare November 30, 2023 09:03

Vyom-Yadav added the awaiting changes label Jan 29, 2024

Vyom-Yadav closed this Mar 2, 2024

		Truth.assertWithMessage("test").that(1).isNotEqualTo(2);
		// ^---- ok, specified pattern is different

Issue #11087: New check ChainedMethodCallWrap #11231

Issue #11087: New check ChainedMethodCallWrap #11231

Conversation

Vyom-Yadav commented Jan 24, 2022 • edited

Choose a reason for hiding this comment

Vyom-Yadav Jan 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vyom-Yadav commented Feb 4, 2022

github-actions bot commented Feb 4, 2022

Vyom-Yadav commented Feb 4, 2022

github-actions bot commented Feb 4, 2022

Vyom-Yadav commented Feb 4, 2022

github-actions bot commented Feb 4, 2022

Vyom-Yadav commented Apr 7, 2023

github-actions bot commented Apr 7, 2023

Vyom-Yadav commented Apr 7, 2023

Vyom-Yadav commented Apr 7, 2023

github-actions bot commented Apr 7, 2023

Vyom-Yadav commented Apr 8, 2023

romani commented Apr 9, 2023

Vyom-Yadav commented Apr 21, 2023

github-actions bot commented Apr 21, 2023

Vyom-Yadav commented Apr 21, 2023

romani commented Nov 29, 2023

romani left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vyom-Yadav Nov 30, 2023 • 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

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

Vyom-Yadav commented Nov 30, 2023

Vyom-Yadav commented Nov 30, 2023

romani commented Nov 30, 2023

Vyom-Yadav commented Dec 2, 2023

romani commented Dec 2, 2023

nrmancuso commented Feb 23, 2024

Vyom-Yadav commented Jan 24, 2022 •

edited

Vyom-Yadav Jan 24, 2022 •

edited

romani left a comment •

edited

Vyom-Yadav Nov 30, 2023 •

edited