From dba6c944cdbf9acfbb40b9a2d8e6315fb14d87f0 Mon Sep 17 00:00:00 2001 From: Andrei Selkin Date: Fri, 11 Sep 2015 19:22:58 +0300 Subject: [PATCH] Issue #382: Fix HiddenField false positive violations for anonymous classes --- .../checks/coding/HiddenFieldCheck.java | 37 ++++++++++++--- .../tools/checkstyle/utils/ScopeUtils.java | 11 +++++ .../checks/coding/HiddenFieldCheckTest.java | 15 ++++++ .../InputHidenFieldStaticVisibility.java | 47 +++++++++++++++++++ 4 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/InputHidenFieldStaticVisibility.java diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java index aaa979a78..f94191598 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheck.java @@ -27,6 +27,7 @@ import java.util.regex.Pattern; import com.google.common.collect.Sets; import com.puppycrawl.tools.checkstyle.api.Check; import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.Scope; import com.puppycrawl.tools.checkstyle.api.TokenTypes; import com.puppycrawl.tools.checkstyle.utils.CommonUtils; import com.puppycrawl.tools.checkstyle.utils.ScopeUtils; @@ -280,16 +281,38 @@ public class HiddenFieldCheck final DetailAST nameAST = ast.findFirstToken(TokenTypes.IDENT); final String name = nameAST.getText(); - if (isStaticOrInstanceField(ast, name) - && !isMatchingRegexp(name) - && !isIgnoredSetterParam(ast, name) - && !isIgnoredConstructorParam(ast) - && !isIgnoredParamOfAbstractMethod(ast)) { + if ((isStaticFieldHiddenFromAnonymousClass(ast, name) + || isStaticOrInstanceField(ast, name)) + && !isMatchingRegexp(name) + && !isIgnoredParam(ast, name)) { log(nameAST, MSG_KEY, name); } } } + /** + * Checks whether a static field is hidden from closure. + * @param nameAST local variable or parameter. + * @param name field name. + * @return true if static field is hidden from closure. + */ + private boolean isStaticFieldHiddenFromAnonymousClass(DetailAST nameAST, String name) { + return isInStatic(nameAST) + && frame.containsStaticField(name); + } + + /** + * Checks whether method or constructor parameter is ignored. + * @param ast the parameter token. + * @param name the parameter name. + * @return true if parameter is ignored. + */ + private boolean isIgnoredParam(DetailAST ast, String name) { + return isIgnoredSetterParam(ast, name) + || isIgnoredConstructorParam(ast) + || isIgnoredParamOfAbstractMethod(ast); + } + /** * Check for static or instance field. * @param ast token @@ -324,7 +347,9 @@ public class HiddenFieldCheck if (parent.getType() == TokenTypes.STATIC_INIT) { inStatic = true; } - else if (parent.getType() == TokenTypes.METHOD_DEF) { + else if (parent.getType() == TokenTypes.METHOD_DEF + && !ScopeUtils.isInScope(parent, Scope.ANONINNER) + || parent.getType() == TokenTypes.VARIABLE_DEF) { final DetailAST mods = parent.findFirstToken(TokenTypes.MODIFIERS); inStatic = mods.branchContains(TokenTypes.LITERAL_STATIC); diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtils.java b/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtils.java index 74f28ed9d..d2d9203c7 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtils.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtils.java @@ -265,4 +265,15 @@ public final class ScopeUtils { } return localVariableDef; } + + /** + * Checks whether ast node is in a specific scope. + * @param ast the node to check. + * @param scope a {@code Scope} value. + * @return true if the ast node is in the scope. + */ + public static boolean isInScope(DetailAST ast, Scope scope) { + final Scope surroundingScopeOfAstToken = getSurroundingScope(ast); + return surroundingScopeOfAstToken == scope; + } } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheckTest.java index d74db8e70..ee563284b 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/HiddenFieldCheckTest.java @@ -29,6 +29,21 @@ import com.puppycrawl.tools.checkstyle.DefaultConfiguration; public class HiddenFieldCheckTest extends BaseCheckTestSupport { + + @Test + public void testStaticVisibilityFromAnonymousClasses() throws Exception { + final DefaultConfiguration checkConfig = + createCheckConfig(HiddenFieldCheck.class); + final String[] expected = { + "10:95: " + getCheckMessage(MSG_KEY, "other"), + "16:92: " + getCheckMessage(MSG_KEY, "other"), + "24:99: " + getCheckMessage(MSG_KEY, "other"), + "34:103: " + getCheckMessage(MSG_KEY, "other"), + "46:26: " + getCheckMessage(MSG_KEY, "someField"), + }; + verify(checkConfig, getPath("InputHidenFieldStaticVisibility.java"), expected); + } + @Test public void testNoParameters() throws Exception { diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputHidenFieldStaticVisibility.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputHidenFieldStaticVisibility.java new file mode 100644 index 000000000..693cd9250 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputHidenFieldStaticVisibility.java @@ -0,0 +1,47 @@ +package com.puppycrawl.tools.checkstyle; + +import java.util.Comparator; + +public class InputHidenFieldStaticVisibility { + static int someField; + static InputHidenFieldStaticVisibility other = null; + InputHidenFieldStaticVisibility field = null; + + static void method(InputHidenFieldStaticVisibility field, InputHidenFieldStaticVisibility other) { + // field 'field' can not be referenced form a static context + // static field 'other' can be referenced from a static context + } + + static class B { + void method(InputHidenFieldStaticVisibility field, InputHidenFieldStaticVisibility other) { + // field 'field' can not be referenced form a static context + // static field 'other' can be referenced from a static context + } + } + + static Comparator COMP = new Comparator() { + @Override + public int compare(InputHidenFieldStaticVisibility field, InputHidenFieldStaticVisibility other) { + // field 'field' can not be referenced form a static context + // static field 'other' can be referenced from a static context + return 0; + } + }; + + static Comparator createComp() { + return new Comparator() { + @Override + public int compare(InputHidenFieldStaticVisibility field, InputHidenFieldStaticVisibility other) { + // field 'field' can not be referenced form a static context + // static field 'other' can be referenced from a static context + return 0; + } + }; + } + + static void foo1(int a) {} + + void foo2(int a) {} + + static void foo3(int someField) {} +}