From 425fd5a27cd567d5784bd246729c55cf8cda3dbd Mon Sep 17 00:00:00 2001 From: Vladislav Lisetskiy Date: Sat, 26 Dec 2015 17:23:40 +0300 Subject: [PATCH] Issue #2539: Fix RequireThis check false-positive on methods --- .../checks/coding/RequireThisCheck.java | 107 +++++++++++++++--- .../checks/coding/RequireThisCheckTest.java | 2 + .../checks/coding/InputRequireThis.java | 11 ++ src/xdocs/config_coding.xml | 19 ++++ 4 files changed, 122 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java index 3da0db307..06323e9ab 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java @@ -38,7 +38,9 @@ import com.puppycrawl.tools.checkstyle.utils.ScopeUtils; * That is references to instance variables and methods of the present * object are explicitly of the form "this.varName" or * "this.methodName(args)". - *

+ *

+ * + *

Warning: the Check is very controversial and not that actual nowadays.

* *

Examples of use: * 

@@ -53,6 +55,19 @@ import com.puppycrawl.tools.checkstyle.utils.ScopeUtils;
  * </module>
  *
* + *

Rationale:

+ *
    + *
  1. + * The same notation/habit for C++ and Java (C++ have global methods, so having + * "this." do make sense in it to distinguish call of method of class + * instead of global). + *
    2. + *
  2. + * Non-IDE development (ease of refactoring, some clearness to distinguish + * static and non-static methods). + *
    3. + *
+ * *

Limitations: Nothing is currently done about static variables * or catch-blocks. Static methods invoked on a class name seem to be OK; * both the class name and the method name have a DOT parent. @@ -374,7 +389,8 @@ public class RequireThisCheck extends Check { private AbstractFrame checkMethod(DetailAST ast) { final AbstractFrame frame = findFrame(ast, true); if (frame != null - && ((ClassFrame) frame).hasInstanceMethod(ast)) { + && ((ClassFrame) frame).hasInstanceMethod(ast) + && !((ClassFrame) frame).hasStaticMethod(ast)) { return frame; } return null; @@ -478,12 +494,12 @@ public class RequireThisCheck extends Check { return frameName; } - /** Check whether the frame contains a given name. + /** Check whether the frame contains a field or a variable with the given name. * @param nameToFind the IDENT ast of the name we're looking for * @return whether it was found */ - boolean contains(DetailAST nameToFind) { - return containsName(varIdents, nameToFind); + boolean containsFieldOrVariable(DetailAST nameToFind) { + return containsFieldOrVariableDef(varIdents, nameToFind); } /** Check whether the frame contains a given name. @@ -495,7 +511,7 @@ public class RequireThisCheck extends Check { AbstractFrame frame; if (!lookForMethod - && contains(nameToFind)) { + && containsFieldOrVariable(nameToFind)) { frame = this; } else { @@ -512,7 +528,7 @@ public class RequireThisCheck extends Check { * @return true if the set contains a declaration with the text of the specified * IDENT ast and it is declared in a proper position. */ - protected boolean containsName(Set set, DetailAST ident) { + protected boolean containsFieldOrVariableDef(Set set, DetailAST ident) { boolean result = false; for (DetailAST ast: set) { if (isProperDefinition(ident, ast)) { @@ -642,25 +658,33 @@ public class RequireThisCheck extends Check { * instance member of the class */ public boolean hasInstanceMember(final DetailAST ident) { - return containsName(instanceMembers, ident); + return containsFieldOrVariableDef(instanceMembers, ident); } /** * Checks if a given name is a known instance method of the class. - * @param ident the IDENT ast of the name to check - * @return true is the given name is a name of a known + * @param ident the IDENT ast of the method call to check + * @return true if the given ast is correspondent to a known * instance method of the class */ public boolean hasInstanceMethod(final DetailAST ident) { - return containsName(instanceMethods, ident); + return containsMethodDef(instanceMethods, ident); + } + + /** + * Checks if a given name is a known static method of the class. + * @param ident the IDENT ast of the method call to check + * @return true is the given ast is correspondent to a known + * instance method of the class + */ + public boolean hasStaticMethod(final DetailAST ident) { + return containsMethodDef(staticMethods, ident); } @Override - boolean contains(DetailAST nameToFind) { - return containsName(instanceMembers, nameToFind) - || containsName(instanceMethods, nameToFind) - || containsName(staticMembers, nameToFind) - || containsName(staticMethods, nameToFind); + boolean containsFieldOrVariable(DetailAST nameToFind) { + return containsFieldOrVariableDef(instanceMembers, nameToFind) + || containsFieldOrVariableDef(staticMembers, nameToFind); } @Override @@ -673,7 +697,8 @@ public class RequireThisCheck extends Check { protected AbstractFrame getIfContains(DetailAST nameToFind, boolean lookForMethod) { AbstractFrame frame = null; - if (contains(nameToFind)) { + if (lookForMethod && containsMethod(nameToFind) + || containsFieldOrVariable(nameToFind)) { frame = this; } else if (getParent() != null) { @@ -681,6 +706,54 @@ public class RequireThisCheck extends Check { } return frame; } + + /** + * Check whether the frame contains a given method. + * @param methodToFind the AST of the method to find. + * @return true, if a method with the same name and number of parameters is found. + */ + private boolean containsMethod(DetailAST methodToFind) { + return containsMethodDef(instanceMethods, methodToFind) + || containsMethodDef(staticMethods, methodToFind); + } + + /** + * Whether the set contains a method definition with the + * same name and number of parameters. + * @param set the set of definitions. + * @param ident the specified method call IDENT ast. + * @return true if the set contains a definition with the + * same name and number of parameters. + */ + private boolean containsMethodDef(Set set, DetailAST ident) { + boolean result = false; + for (DetailAST ast: set) { + if (isSimilarSignature(ident, ast)) { + result = true; + break; + } + } + return result; + } + + /** + * Whether the method definition has the same name and number of parameters. + * @param ident the specified method call IDENT ast. + * @param ast the ast of a method definition to compare with. + * @return true if a method definition has the same name and number of parameters + * as the method call. + */ + private boolean isSimilarSignature(DetailAST ident, DetailAST ast) { + boolean result = false; + if (ident.getText().equals(ast.getText())) { + final int paramsNumber = ast.getParent().findFirstToken(TokenTypes.PARAMETERS) + .getChildCount(); + final int argsNumber = ident.getParent().findFirstToken(TokenTypes.ELIST) + .getChildCount(); + result = paramsNumber == argsNumber; + } + return result; + } } /** diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java index c27d4b414..4576fb0fc 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java @@ -58,6 +58,7 @@ public class RequireThisCheckTest extends BaseCheckTestSupport { "115:9: " + getCheckMessage(MSG_METHOD, "instanceMethod", ""), "121:13: " + getCheckMessage(MSG_METHOD, "instanceMethod", "Issue2240."), "122:13: " + getCheckMessage(MSG_VARIABLE, "i", "Issue2240."), + "134:9: " + getCheckMessage(MSG_METHOD, "foo", ""), }; verify(checkConfig, getPath("InputRequireThis.java"), @@ -73,6 +74,7 @@ public class RequireThisCheckTest extends BaseCheckTestSupport { "17:9: " + getCheckMessage(MSG_METHOD, "method1", ""), "115:9: " + getCheckMessage(MSG_METHOD, "instanceMethod", ""), "121:13: " + getCheckMessage(MSG_METHOD, "instanceMethod", "Issue2240."), + "134:9: " + getCheckMessage(MSG_METHOD, "foo", ""), }; verify(checkConfig, getPath("InputRequireThis.java"), diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputRequireThis.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputRequireThis.java index 0fa0b8ad4..0c6d45597 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputRequireThis.java +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputRequireThis.java @@ -123,3 +123,14 @@ class Issue2240 { } } } + +class Issue2539{ + void foo(int i) {} + static void foo(double i) {} + void foo() {} + + void bar() { + foo(1); + foo(); + } +} diff --git a/src/xdocs/config_coding.xml b/src/xdocs/config_coding.xml index df4254a3b..d04c76c47 100644 --- a/src/xdocs/config_coding.xml +++ b/src/xdocs/config_coding.xml @@ -2773,6 +2773,25 @@ public void foo(int i, String s) {} "this.methodName(args)" and that those references don't rely on the default behavior when "this." is absent.

+ +

+ Warning: the Check is very controversial and not that actual nowadays. +

+ +

+ Rationale: +

+
    +
  1. + The same notation/habit for C++ and Java (C++ have global methods, so having + "this." do make sense in it to distinguish call of method of class + instead of global). +
    2. +
  2. + Non-IDE development (ease of refactoring, some clearness to distinguish + static and non-static methods). +
    3. +