Issue #14502: Handling of whitespace between generic and record header (GenericWhitespaceCheck) #14529

sktpy · 2024-02-23T06:08:03Z

Resolves #14502

Reasoning for the changes are mentioned in the comments.

Old Doc(site), New Doc(site)

Regression: (Report)

Differences found (expected)

Added Violations (4): Example (rest occurrences are similar) (link)
This is intended behaviour of the fix, whitespace between generic and record header is counted as violation.

Diff Regression config: https://gist.githubusercontent.com/sktpy/44a31ea4e946a0755fa3e331fd65de98/raw/bc9e57764a6ef5b9f4aa27bcb6f55115460423bb/check.xml

sktpy · 2024-02-23T06:10:45Z

GitHub, generate report

github-actions · 2024-02-23T08:15:14Z

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

sktpy · 2024-02-23T08:22:17Z

GitHub, generate website

github-actions · 2024-02-23T08:28:14Z

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/e3fa404_2024082532/checks/whitespace/genericwhitespace.html#Description

sktpy · 2024-02-23T09:05:46Z

...s/resources/com/puppycrawl/tools/checkstyle/checks/whitespace/genericwhitespace/Example1.txt

@@ -23,4 +23,6 @@ List<T> list = ImmutableList.Builder<T>::new;
 sort(list, Comparable::<String>compareTo);
 // Constructor call
 MyClass obj = new <String>MyClass();
+// Record header
+record License<T>() {}


Have kept the examples in the non-enabled way as GenericWhitespaceExample is being updated in the PR #1464, if that happens to get merged before this one does, I will enable the examples in this PR else it can be updated in that PR.

sktpy · 2024-02-23T09:09:02Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java

@@ -51,7 +51,8 @@
 * <ul>
 * <li> should not be preceded with whitespace in all cases.</li>
 * <li> should be followed with whitespace in almost all cases,
- *   except diamond operators and when preceding method name or constructor.</li></ul>
+ *   except diamond operators and when preceding method name, constructor or record header.</li>


All similar changes reflect the updated behaviour.

sktpy · 2024-02-23T10:13:33Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java

-            || charAfter == ',' || charAfter == '['
-            || charAfter == '.' || charAfter == ':'
-            || charAfter == ';'
+        return charAfter == ')' || charAfter == ','


Removal of charAfter == '(':

(As determined in #14497 (comment) [last paragraph])

Purpose of removal:
To dispose mutant (replacing charAfter == '(' with false) which was able to survive due to the changes in this fix in the following line:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java

Line 373 in 84eaed7

return charAfter == '(' || charAfter == ')'

Why the mutant survived:
All the valid occurrences where Generic-End(>) can be followed by LPAREN(() are the cases where it needs to be checked that such generic is not followed by whitespace, and as of this fix we have accounted for all such occurrences, making every possible input where > can be followed by ( filter through if block of processSingleGeneric():

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java

Lines 223 to 225 in e3fa404

if (isGenericBeforeMethod(ast)

|| isGenericBeforeCtorInvocation(ast)

|| isGenericBeforeRecordHeader(ast)) {

Hence never reaching the else if block which calls isCharacterValidAfterGenericEnd(), which makes it possible for the mutant i.e. (replacing charAfter == '(' with false) survive all the tests.

Why the mutant didn't survived before:
Because previously Check didn't counted whitespace between generic and record header as a violation, making this the killer input where > could be preceded by ( which wasn't checked for succeeding whitespace, which made it produce illegal follow violation, hence killing the mutant.

Why this mutant cannot be killed:
To kill this mutant we would need an input where > is followed by ( but isn't from ones that we have already accounted for(method, ctor, record). Regression is unable to find such input, and to the best of my knowledge, such input does not exist, but if anyone is aware of such input please do tell.

In conclusion, it is safe to remove that operand (also provides us with nano performance gain) unless anyone is aware of the killer input or if there is any plan to use this exact method in the future where that operand would be required.
The only usage of the method with mutation is in the GenericWhitespaceCheck. (1 Usage)
GenericWhitespaceCheck does not affects any other class.

sktpy · 2024-02-23T10:41:09Z

Failing checks in CI are unrelated. (Cirrus, YAML Validation for site.xml)

nrmancuso

Excellent work! Thanks for report and website generation ahead of time to ease review.

One minor item:

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java

…cord header (GenericWhitespaceCheck)

sktpy · 2024-02-24T14:54:18Z

GitHub, generate website

nrmancuso

I am good as CI passes

github-actions · 2024-02-24T16:01:56Z

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/05dd30d_2024155912/checks/whitespace/genericwhitespace.html#Description

sktpy · 2024-03-02T16:17:55Z

@romani 🏓 ping pong
pinging as I believe this might have been missed.

sktpy · 2024-03-14T07:04:41Z

🌵🏜️🐪

romani

Ok to merge, thanks a lot

sktpy marked this pull request as draft February 23, 2024 06:10

sktpy force-pushed the issue-14502-generic-record-header branch from 32d2bfa to e3fa404 Compare February 23, 2024 06:17

sktpy commented Feb 23, 2024

View reviewed changes

sktpy marked this pull request as ready for review February 23, 2024 10:41

nrmancuso suggested changes Feb 24, 2024

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.java Outdated Show resolved Hide resolved

nrmancuso self-assigned this Feb 24, 2024

Issue checkstyle#14502: Handling of whitespace between generic and re…

05dd30d

…cord header (GenericWhitespaceCheck)

sktpy force-pushed the issue-14502-generic-record-header branch from e3fa404 to 05dd30d Compare February 24, 2024 14:51

nrmancuso approved these changes Feb 24, 2024

View reviewed changes

nrmancuso assigned romani and unassigned nrmancuso Feb 24, 2024

sktpy mentioned this pull request Mar 5, 2024

JavadocType: False negative for unknown tag with no description #14573

Closed

romani approved these changes Mar 15, 2024

View reviewed changes

romani merged commit 25cd447 into checkstyle:master Mar 15, 2024

romani mentioned this pull request Oct 18, 2024

Issue #13345: Enable TestCases for GenericWhitespaceCheckExamplesTest #14364

Closed

	if (isGenericBeforeMethod(ast)
	\|\| isGenericBeforeCtorInvocation(ast)
	\|\| isGenericBeforeRecordHeader(ast)) {

Uh oh!

Issue #14502: Handling of whitespace between generic and record header (GenericWhitespaceCheck) #14529

Issue #14502: Handling of whitespace between generic and record header (GenericWhitespaceCheck) #14529

Uh oh!

Conversation

sktpy commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Regression: (Report)

Uh oh!

sktpy commented Feb 23, 2024

Uh oh!

github-actions bot commented Feb 23, 2024

Uh oh!

sktpy commented Feb 23, 2024

Uh oh!

github-actions bot commented Feb 23, 2024

Uh oh!

sktpy Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sktpy Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sktpy Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Removal of charAfter == '(':

Uh oh!

sktpy commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sktpy commented Feb 24, 2024

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 24, 2024

Uh oh!

sktpy commented Mar 2, 2024

Uh oh!

sktpy commented Mar 14, 2024

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sktpy commented Feb 23, 2024 •

edited

Loading

sktpy Feb 23, 2024 •

edited

Loading

sktpy Feb 23, 2024 •

edited

Loading

sktpy Feb 23, 2024 •

edited

Loading

Removal of `charAfter == '('`:

sktpy commented Feb 23, 2024 •

edited

Loading