Issue #14502: Handling of whitespace between generic and record header (GenericWhitespaceCheck)#14529
Conversation
|
GitHub, generate report |
32d2bfa to
e3fa404
Compare
|
GitHub, generate website |
| // Constructor call | ||
| MyClass obj = new <String>MyClass(); | ||
| // Record header | ||
| record License<T>() {} |
There was a problem hiding this comment.
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.
| * <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> |
There was a problem hiding this comment.
All similar changes reflect the updated behaviour.
| || charAfter == ',' || charAfter == '[' | ||
| || charAfter == '.' || charAfter == ':' | ||
| || charAfter == ';' | ||
| return charAfter == ')' || charAfter == ',' |
There was a problem hiding this comment.
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:
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():
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.
|
Failing checks in CI are unrelated. (Cirrus, YAML Validation for site.xml) |
nrmancuso
left a comment
There was a problem hiding this comment.
Excellent work! Thanks for report and website generation ahead of time to ease review.
One minor item:
…cord header (GenericWhitespaceCheck)
e3fa404 to
05dd30d
Compare
|
GitHub, generate website |
nrmancuso
left a comment
There was a problem hiding this comment.
I am good as CI passes
|
@romani 🏓 ping pong |
|
🌵🏜️🐪 |
Resolves #14502
Reasoning for the changes are mentioned in the comments.
Old Doc(site), New Doc(site)
Regression: (Report)
Differences found (expected)
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