Issue #14344: Fix GenericWhitespace Check ignoring exception to the '>' rule for succeeding whitespaces #14497

sktpy · 2024-02-17T10:21:59Z

Resolves #14344

Reasoning for the changes are mentioned in the corresponding review comments of mine.

Regression: (Report)
Differences found (expected).

Added violations(71): Example (rest occurrences are similar). (Line 123)

componentPlugins = new DualKeyHashMap<> ();
//                                    ^
//  '>' is followed by whitespace  ---+

This added violation was not detected before (a false negative, reasoning in the description of the Issue this PR resolves)

Removed violations(3): Example (rest occurrences are similar). (Line 78)

return new <@BG Inner<@BH Integer>> @BI Inner<@BJ Inner<@BK Integer>>(null);
//         ^
//         +---  '<' is preceded with whitespace.

This removed violation is a false positive which was detected as a violation before because generics enclosing Annotations that were succeeding new keyword weren't detected as the cases where they should be preceded with a whitespace.
These cases are now detected properly in the fix (See comment).

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

github-actions · 2024-02-17T10:56:04Z

Report generation failed on phase "parse_body",
step "Parsing content of PR description".
Link: https://github.com/checkstyle/checkstyle/actions/runs/7941040371

sktpy · 2024-02-17T11:09:05Z

GitHub, generate report or else... 😈🔪

github-actions · 2024-02-17T12:56:09Z

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

sktpy · 2024-02-17T13:38:15Z

...le/checkstyle/test/chapter4formatting/rule462horizontalwhitespace/GenericWhitespaceTest.java

@@ -54,6 +54,7 @@ public void testWhitespaceAroundGenerics() throws Exception {
            "14:46: " + getCheckMessage(messages, msgPreceded, ">"),
            "15:33: " + getCheckMessage(messages, msgFollowed, "<"),
            "15:33: " + getCheckMessage(messages, msgPreceded, "<"),
+            "15:46: " + getCheckMessage(messages, msgFollowed, ">"),


All the similar additions in expected violations is due to following:
In this instance the extra violation is in the following line:

checkstyle/src/it/resources/com/google/checkstyle/test/chapter4formatting/rule462horizontalwhitespace/InputWhitespaceAroundGenerics.java

Line 15 in 9842608

new java.util.Hashtable < Integer, D > (); //warn

Before the fix, check didn't considered the whitespace between > ( as a violation, this is now correctly identified in the fix.

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

sktpy · 2024-02-17T13:57:51Z

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

+     * @param ast ast
+     * @return true if generic after {@code LITERAL_NEW}
+     */
+    private static boolean isGenericAfterNew(DetailAST ast) {


Method name change from isGenericBeforeCtor() to isGenericAfterNew(), this describes method's behaviour more accurately, as it only identified the cases where generic existed after new keyword.
The method didn't identified cases where generic preceded constructor invocation () which was the cause for the false negative.

The method now also identifies a new similar case for Annotations (case 3.)

sktpy · 2024-02-17T14:02:26Z

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

        //
+        //   new <String>Object()
+        //       ^
+        //    3. +--- ws required


Added example new <String>Object() in the comments as isGenericBeforeCtor() (now isGenericAfterNew()) was added earlier to be used in this method to detect a new type of case for which a preceding whitespace was required, but the comment was not updated in that commit to reflect this new behaviour.

sktpy · 2024-02-17T14:40:31Z

...checkstyle/checks/whitespace/genericwhitespace/InputGenericWhitespaceBeforeRecordHeader.java

+package com.puppycrawl.tools.checkstyle.checks.whitespace.genericwhitespace;
+
+public class InputGenericWhitespaceBeforeRecordHeader {
+    record Session<T>() {}


Disclaimer: Contains some off-topic observations.

The purpose of this input and the corresponding test is to kill the mutation which survived after the changes in Check.
Surviving mutation was replacing charAfter == '(' with false in the following line:

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

Line 347 in 9842608

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

Why this didn't survived before:

Before the fix, check didn't identified all the cases where generic was present before constructor invocation, which meant some CTOR cases where > was succeeded with ( weren't filtered through the following if statement:

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

Line 222 in 9842608

if (isGenericBeforeMethod(ast) || isGenericBeforeCtor(ast)) {

Such cases went through isCharacterValidAfterGenericEnd() in the following else if block, making the mutant produce illegal follow violation, hence killing the mutant.
Due to the fix, all the possible existing cases where > could be succeeded by ( were filtered through the if statement, meaning none of the inputs went through isCharacterValidAfterGenericEnd(), hence creating that surviving mutation.

So after a bit of mutant hunting to find a valid input where > could be succeeded by ( but wasn't before Method or Ctor, found out that such valid input exist for generic before record header, which was able to kill the mutant.

As of now, we don't have a clear definition in the documentation on how to handle whitespace for generic that is before a record header, current check does not count space between > and ( of record header as a violation, ~~will open an Issue shortly for discussing if it should~~(See #14502), based on the decision of the discussion, will update the Check accordingly and strengthen the test inputs for the records in a separate PR.

Funny thing is that if there is supposed to be violation for the case above, this will revive the mutant, as we will now be filtering the killing input in the if statement(this in my knowledge makes for all possible cases where generic can be followed by (), which will mean that the surviving mutated operand could be safely removed if no regressions are found and no one is aware of such input.

sktpy · 2024-02-17T15:29:39Z

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

     */
-    private static boolean isGenericBeforeCtor(DetailAST ast) {
+    private static boolean isGenericBeforeCtorInvocation(DetailAST ast) {


The method is used to identify cases where > should not be followed by whitespace, fixing the false negative.

sktpy · 2024-02-17T15:51:17Z

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

romani

Please extend inputs by a bit more cases.
We need to not rely on regression too much in future, so we collecting all different types of how code can be written.

...rawl/tools/checkstyle/checks/whitespace/genericwhitespace/InputGenericWhitespaceDefault.java

romani

items:

...eckstyle/checks/whitespace/genericwhitespace/InputGenericWhitespaceBeforeCtorInvocation.java

…n to the '>' rule for succeeding whitespaces

romani

ok to merge

sktpy marked this pull request as draft February 17, 2024 10:22

sktpy commented Feb 17, 2024

View reviewed changes

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

sktpy commented Feb 17, 2024

View reviewed changes

sktpy marked this pull request as ready for review February 17, 2024 15:51

romani requested changes Feb 17, 2024

View reviewed changes

...rawl/tools/checkstyle/checks/whitespace/genericwhitespace/InputGenericWhitespaceDefault.java Show resolved Hide resolved

sktpy force-pushed the issue-14344-genericwhitespace-false-negative branch from 28c7818 to 0eaeb24 Compare February 18, 2024 07:01

sktpy mentioned this pull request Feb 18, 2024

GenericWhitespaceCheck: Handling of whitespace between generic and record header #14502

Closed

romani requested changes Feb 18, 2024

View reviewed changes

...eckstyle/checks/whitespace/genericwhitespace/InputGenericWhitespaceBeforeCtorInvocation.java Outdated Show resolved Hide resolved

sktpy force-pushed the issue-14344-genericwhitespace-false-negative branch 3 times, most recently from 3c611a0 to c5abe70 Compare February 19, 2024 13:26

Issue checkstyle#14344: Fix GenericWhitespace Check ignoring exceptio…

d79288a

…n to the '>' rule for succeeding whitespaces

sktpy force-pushed the issue-14344-genericwhitespace-false-negative branch from c5abe70 to d79288a Compare February 21, 2024 14:27

romani approved these changes Feb 21, 2024

View reviewed changes

romani requested a review from rnveach February 21, 2024 14:34

romani assigned rnveach Feb 21, 2024

rnveach approved these changes Feb 22, 2024

View reviewed changes

rnveach merged commit b6eb4a6 into checkstyle:master Feb 22, 2024

sktpy mentioned this pull request Feb 23, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Issue #14344: Fix GenericWhitespace Check ignoring exception to the '>' rule for succeeding whitespaces #14497

Issue #14344: Fix GenericWhitespace Check ignoring exception to the '>' rule for succeeding whitespaces #14497

Uh oh!

sktpy commented Feb 17, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 17, 2024

Uh oh!

sktpy commented Feb 17, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Feb 17, 2024

Uh oh!

sktpy Feb 17, 2024 •

edited

Loading

Uh oh!

Uh oh!

sktpy Feb 17, 2024

Uh oh!

sktpy Feb 17, 2024 •

edited

Loading

Uh oh!

sktpy Feb 17, 2024 •

edited

Loading

Uh oh!

sktpy Feb 17, 2024 •

edited

Loading

Uh oh!

sktpy commented Feb 17, 2024 •

edited

Loading

Uh oh!

romani left a comment

Uh oh!

Uh oh!

romani left a comment

Uh oh!

Uh oh!

romani left a comment

Uh oh!

Uh oh!

Uh oh!

Issue #14344: Fix GenericWhitespace Check ignoring exception to the '>' rule for succeeding whitespaces #14497

Issue #14344: Fix GenericWhitespace Check ignoring exception to the '>' rule for succeeding whitespaces #14497

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Feb 17, 2024

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Feb 17, 2024

Uh oh!

sktpy Feb 17, 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!

Uh oh!

sktpy Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

sktpy Feb 17, 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 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Why this didn't survived before:

Uh oh!

sktpy Feb 17, 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 commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sktpy commented Feb 17, 2024 •

edited

Loading

sktpy commented Feb 17, 2024 •

edited

Loading

sktpy Feb 17, 2024 •

edited

Loading

sktpy Feb 17, 2024 •

edited

Loading

sktpy Feb 17, 2024 •

edited

Loading

sktpy Feb 17, 2024 •

edited

Loading

sktpy commented Feb 17, 2024 •

edited

Loading