search

sevntu-checkstyle / sevntu.checkstyle

Additional Checkstyle checks, that could be added as extension to EclipseCS plugin and maven-checkstyle-plugin, Sonar checkstyle plugin, extension for CheckStyle IDEA plugin.
http://sevntu-checkstyle.github.io/sevntu.checkstyle/
190 stars 146 forks source link

StaticMethodCandidateCheck: False Negatives on recursion #421

Open rnveach opened 8 years ago

rnveach commented 8 years ago

StaticMethodCandidateCheck does not pick up the following cases from checkstyle's source.

Config: https://github.com/checkstyle/checkstyle/blob/617ebf2bf27d284dcc7b866f85439fcf6dab3402/config/checkstyle_sevntu_checks.xml#L15

Examples:

EqualsAvoidNullCheck.containsAllSafeTokens: private method, uses parameters, local variables, and calls one private-static method (skipVariableAssign) defined in the same class. https://github.com/checkstyle/checkstyle/blob/617ebf2bf27d284dcc7b866f85439fcf6dab3402/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/EqualsAvoidNullCheck.java#L340

NeedBracesCheck.isEmptyLoopBody: private method, uses parameters and local variables https://github.com/checkstyle/checkstyle/blob/617ebf2bf27d284dcc7b866f85439fcf6dab3402/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java#L238

RequireThisCheck.ClassFrame.isSimilarSignature: private method in inner static class, uses parameters and local variables https://github.com/checkstyle/checkstyle/blob/617ebf2bf27d284dcc7b866f85439fcf6dab3402/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java#L746

Found with Eclipse setting "Method can be static" to "Error" inside "Java Compiler > Errors/Warnings". Found no conflicts when the identified methods were temporarily made static.

romani commented 8 years ago

EqualsAvoidNullCheck.containsAllSafeTokens, NeedBracesCheck.isEmptyLoopBody

yes, we should do violation there, looks like simple case. they are not in scope of our limitations.

@Vladlis , can you confirm this issue ?

romani commented 8 years ago

https://github.com/attatrol/checkstyle/commit/c0ce36da9c9c0b4d59b89865bed61413b160692a

3 missed cases: private int countLogicalOperators(DetailAST ast) private DetailAST getLastToken(DetailAST ast) private int countCaseTokens(DetailAST ast)

Vladlis commented 8 years ago

In all cases except from getLastToken the problem is in TokenTypes - we cannot be sure that it is not an instance variable from a parent class.

In getLastToken the reason of failure is recursion and it looks like an issue.

romani commented 8 years ago

I changed name of issue to address "recursion".

Out of context: For TokenTypes ..... we might need some option to let user help to treat some types as full of static content (Util methods and constants). So responsibility for false-positives will be on user side. It will help to avoid some false-negavites.

@rnveach , @Vladlis , what do think about it ? if you are ok - we could create separate issue.

Vladlis commented 8 years ago

@romani +1 I've just had the same idea :) I'm only not sure, whether giving users an ability to produce false-positives is OK.

romani commented 8 years ago

I'm only not sure, whether giving users an ability to produce false-positives is OK.

By default we will NOT classify any Class as static. False-negatives are better than false-positives. But if smb want to play a game with fully static classes - he will has ability. We give users a tool, without proper adjustment most of Checks can do damage.