Issue #4058: RightCurly: false negative in ALONE and anonymous classes#4173
Issue #4058: RightCurly: false negative in ALONE and anonymous classes#4173therootusr wants to merge 1 commit intocheckstyle:masterfrom
Conversation
| // -@cs[JavaNCSS] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| // -@cs[ExecutableStatementCount] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| // -@cs[NPathComplexity] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| private static Details getDetails(DetailAST ast) { |
There was a problem hiding this comment.
This function was exactly at the thresholds for the complexity checks and statement counts. So, had to restructure it. Also restructuring might prove to be useful in the future when more tokens might need to be added to the check
There was a problem hiding this comment.
This change is definitely making the new code hard to review.
There was a problem hiding this comment.
@rnveach Should I make two commits then ? One for refactoring one for adding LITERAL_NEW ?
Codecov Report
@@ Coverage Diff @@
## master #4173 +/- ##
========================================
Coverage ? 100%
Complexity ? 7386
========================================
Files ? 296
Lines ? 16157
Branches ? 3260
========================================
Hits ? 16157
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
|
@rnveach please review |
| details = getDetailsForLoops(ast); | ||
| break; | ||
| default: | ||
| details = getDetailsForDefs(ast); |
There was a problem hiding this comment.
not all of these are DEFs. Let's just call this 'Other' or 'Misc'.
| lcurly = ast.getFirstChild(); | ||
| rcurly = lcurly.getLastChild(); | ||
| } | ||
| final Details details = new Details(); |
There was a problem hiding this comment.
I like the separation the new way gives for each method, but I dislike some of the code it has to copy around.
I am not sure about which of 2 ideas to change this to:
- How about we drop the default constructor and make a new one that takes all fields as arguments, then we could make the fields of the class
finaland renamegetDetailstogetInstance. - Why don't we transform
getDetailsinto a constructor, and have all these methods populate the fields directly.
Problem with point 2 is assignments could be overlooked since they are split into so many methods.
Problem with point 1 is we are stuck with getDetails as a wrapper for the constructor.
What do you all think?
There was a problem hiding this comment.
Approach 1. seems better. In my humble opinion, approach 2. would make things more complex than they are.
| final DetailAST lcurly; | ||
| final DetailAST nextToken; | ||
| final int tokenType = ast.getType(); | ||
| if (tokenType == TokenTypes.CLASS_DEF) { |
There was a problem hiding this comment.
Should this be a switch since this is probably the most likely spot where we will add missing tokens?
There was a problem hiding this comment.
Yeah, but wouldn't the need for switch also depend on how the details get collected. Perhaps we might not need many branches for different tokens that get added ? I feel this can be managed in issue #3547 ? What do you think ?
| * @param ast the given node. | ||
| * @return the token which represents next lexical item. | ||
| */ | ||
| private static DetailAST getNextToken(DetailAST ast) { |
There was a problem hiding this comment.
Shouldn't this be a method of Details? It is only used there.
If so, move any other methods that only Details uses to inside it.
| // -@cs[JavaNCSS] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| // -@cs[ExecutableStatementCount] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| // -@cs[NPathComplexity] getDetails() method is a huge SWITCH, it has to be monolithic | ||
| private static Details getDetails(DetailAST ast) { |
There was a problem hiding this comment.
This change is definitely making the new code hard to review.
|
@PS-SP |
f167b8c to
13e08b6
Compare
| public String toString() { | ||
| return "name:"+s; | ||
| } | ||
| }; // no violation |
There was a problem hiding this comment.
This is ok to me too, lambda didn't require it separated.
| start(); | ||
| super.run(); | ||
| } | ||
| }, // no violation |
| run(); | ||
| } | ||
| } | ||
| }), // no violation |
There was a problem hiding this comment.
As for me, violation should be there for alone and alone-or-singleline, but it should not be there at same http://checkstyle.sourceforge.net/property_types.html#rcurly
Thread t = new Thread(new Runnable() {
}); // this is OK, allowed for better code readability
There was a problem hiding this comment.
@romani
Done for same and alone. As far as alone_or_singleline goes, isAnonInnerClassInit prevents an error. This method was added in this commit
There was a problem hiding this comment.
As far as alone_or_singleline goes, isAnonInnerClassInit prevents an error.
make sense to have no error.
| protected Object clone() { | ||
| return new Object() { @Override protected void finalize() { "".toString(); } }; // violation | ||
| } | ||
| }); // no violation |
There was a problem hiding this comment.
Done. There is a violation for this now.
|
@PS-SP |
|
@romani Please have an look and let me know if you like the refactoring |
romani
left a comment
There was a problem hiding this comment.
@PS-SP , you provided big refactoring to Check it is hard to catch changes that are related to new token support.
Please split refactoring to separate PR , let us merge it, and this PR (after rebase) will be easy to read and validate.
NO issue is required, in commit message you can use "minor: " prefix.
|
this PR can be continued after GSoC |
|
we released 8.2 version, please rebase all your PRs to our latest master to avoid CI problems |
|
@PS-SP , sorry for delay in reply, please rebase one more time to let me review, this PR is hanging too long. |
|
@romani yeah it has been hanging for so long. I also couldn't follow it, sorry. Will update. |
d724fa6 to
da8ba1f
Compare
|
@romani updated PR. Please have a look once. |
|
@PS-SP , |
da8ba1f to
e9dc085
Compare
|
@romani So very sorry for delay. |
|
@PS-SP , I would appreciate if you rebase your PR on latest master. Reports "all the old tokens" have no changes that is good.
looks good.
looks good.
https://4058-rightcurly-new-same.netlify.com/apache-jsecurity/#A1 this is false positive. but in Please explain this change in behavior. |
0a71ee0 to
3e2da61
Compare
|
@PS-SP there is a PMD failure. |
|
@rnveach @romani |
Unless you can very easily merge tokens together, I would just exclude it. It is just one big switch case is all. |
There was a problem hiding this comment.
@PS-SP ,
please add your PMD case to exclude at https://github.com/checkstyle/checkstyle/blob/master/config/pmd.xml#L261
items to confirm:
| details = new Details(lcurly, rcurly, nextToken, false); | ||
| } | ||
| else { | ||
| details = new Details(); |
There was a problem hiding this comment.
@PS-SP , please explain for what case you need this ?
Details withtout details, looks strange.
There was a problem hiding this comment.
LITERAL_NEW not only represents anonymous classes but also object(OBJECT1 = new Object();) and array instantiations(new String[] {"br", } or ar = new int[4];). Hence we first check if a LITERAL_NEW type of AST actually represents an anonymous class and if it does then we return a proper Details object.
If it doesn't then we return a dummy Details object without any details to avoid NullPointerException here
if-else branch can be removed by making the variable details non-final.
There was a problem hiding this comment.
Please remove default c-tor and lets use existing c-tor will all arguments explicitly defined.
not only represents anonymous classes but also object and array
So here is point, please name method exactly what it does getDetailsForAnonymousClassOrObjectOrArray
ar = new int[4];
no details should be in this case only, please add it to UT input file that you created.
new String[] {"br", }
this case should do have right curly, and we need to find it and put in Details. This is should be separate processing branch in IF-ELSE.
code should be like:
if (isAnonymous()) {
} else if (isArrayInitialization()) {
} else {
// it is array creation without curly braces
details = new Details(null, null, null, false);
}
If it doesn't then we return a dummy Details object without any details to avoid NullPointerException here
All other methods getDetailsXXXXX also can return Detail object full of nulls inside, but they use the same c-tor. Please reuse it.
There was a problem hiding this comment.
Please remove default c-tor and lets use existing c-tor will all arguments explicitly defined.
Done.
So here is point, please name method exactly what it does
Done.
no details should be in this case only, please add it to UT input file that you created.
Added another test, testBracePolicyAloneForArrayInit.
this case should do have right curly, and we need to find it and put in Details. This is should be separate processing branch in IF-ELSE.
Done.
ca14566 to
4574711
Compare
…ns for RightCurlyCheck. RightCurlyCheck with option 'ALONE' for anonymous classes. Added UT
|
@romani
|
completely ok to make them as violations.
I am pretty ok with following mode for NEW. Patch below show problematic cases |
If we switch to this then we will have to re-evaluate other tokens. I am fine with |
Please provide example (and cli result probably) to let understand clearly what you mean.
We do not need to. I my patch only new token is under new option. I do not see a reason to review and think about other tokens. If you see a reason please share it, but it definitely separate issue. |
I'm not sure what I used for those posts, but here is one based off current discussions. |
thanks a lot, this is what I expected. =========================== Looks like we did not pay much attention to Issue discussion and how to make it during GSoC hurry and rushed to implementation. I am calling back my approval for changes in ALONE_OR_SINGLELINE. |
|
@PS-SP , I terribly sorry but I do not see future in this PR, sorry for wasting your time on it. New issue is created - #5945 , some part of code might be reused but .... looks like it is better to make new code from scratch. Please do last confirmation that you are ok to close PR. |
Issue #4058 : Added LITERAL_NEW to the list of accepted tokens for RightCurlyCheck. RightCurlyCheck with option 'ALONE' for anonymous classes. Added UT