From 2fe9f501d86b6e70aa64f4474ae0863bb0b928d5 Mon Sep 17 00:00:00 2001 From: Oleg Sukhodolsky Date: Thu, 13 Oct 2011 00:08:33 +0400 Subject: [PATCH] Simple fix for bug #1155921. RequireThis check now ignores static fields and methods. --- .../checks/DeclarationCollector.java | 114 ++++++++++++++---- .../checks/coding/RequireThisCheck.java | 8 +- .../checkstyle/coding/InputRequireThis.java | 17 +++ .../checks/coding/RequireThisCheckTest.java | 16 +-- 4 files changed, 122 insertions(+), 33 deletions(-) diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java index e9697628d..10bbf2643 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java @@ -23,11 +23,11 @@ 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.TokenTypes; -import java.util.HashSet; import java.util.LinkedList; +import java.util.Set; /** - * Abstract class for chekcs which need to collect information about + * Abstract class for checks which need to collect information about * declared members/parameters/variables. * * @author o_sukhodolsky @@ -46,11 +46,29 @@ public abstract class DeclarationCollector extends Check @Override public void visitToken(DetailAST aAST) { + final LexicalFrame frame = this.mFrames.current(); switch (aAST.getType()) { - case TokenTypes.PARAMETER_DEF : - case TokenTypes.VARIABLE_DEF : { + case TokenTypes.VARIABLE_DEF : { + final String name = + aAST.findFirstToken(TokenTypes.IDENT).getText(); + if (frame instanceof ClassFrame) { + final DetailAST mods = + aAST.findFirstToken(TokenTypes.MODIFIERS); + if (mods.branchContains(TokenTypes.LITERAL_STATIC)) { + ((ClassFrame) frame).addStaticMember(name); + } + else { + ((ClassFrame) frame).addInstanceMember(name); + } + } + else { + frame.addName(name); + } + break; + } + case TokenTypes.PARAMETER_DEF : { final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT); - this.mFrames.current().addName(nameAST.getText()); + frame.addName(nameAST.getText()); break; } case TokenTypes.CLASS_DEF : @@ -58,14 +76,26 @@ public abstract class DeclarationCollector extends Check case TokenTypes.ENUM_DEF : case TokenTypes.ANNOTATION_DEF : { final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT); - this.mFrames.current().addName(nameAST.getText()); + frame.addName(nameAST.getText()); this.mFrames.enter(new ClassFrame()); break; } case TokenTypes.SLIST : this.mFrames.enter(new BlockFrame()); break; - case TokenTypes.METHOD_DEF : + case TokenTypes.METHOD_DEF : { + final String name = aAST.findFirstToken(TokenTypes.IDENT).getText(); + if (frame instanceof ClassFrame) { + final DetailAST mods = + aAST.findFirstToken(TokenTypes.MODIFIERS); + if (mods.branchContains(TokenTypes.LITERAL_STATIC)) { + ((ClassFrame) frame).addStaticMember(name); + } + else { + ((ClassFrame) frame).addInstanceMember(name); + } + } + } case TokenTypes.CTOR_DEF : this.mFrames.enter(new MethodFrame()); break; @@ -93,17 +123,6 @@ public abstract class DeclarationCollector extends Check } } - /** - * Check if given name is a name for declafred variable/parameter/member in - * current environment. - * @param aName a name to check - * @return true is the given name is declare one. - */ - protected final boolean isDeclared(String aName) - { - return (null != mFrames.findFrame(aName)); - } - /** * Check if given name is a name for class field in current environment. * @param aName a name to check @@ -111,7 +130,9 @@ public abstract class DeclarationCollector extends Check */ protected final boolean isClassField(String aName) { - return (mFrames.findFrame(aName) instanceof ClassFrame); + final LexicalFrame frame = mFrames.findFrame(aName); + return (frame instanceof ClassFrame) + && ((ClassFrame) frame).hasInstanceMember(aName); } /** @@ -122,9 +143,9 @@ public abstract class DeclarationCollector extends Check private abstract static class LexicalFrame { /** Set of name of variables declared in this frame. */ - private final HashSet mVarNames; + private final Set mVarNames; - /** constructor -- invocable only via super() from subclasses */ + /** constructor -- invokable only via super() from subclasses */ protected LexicalFrame() { mVarNames = Sets.newHashSet(); @@ -172,6 +193,57 @@ public abstract class DeclarationCollector extends Check */ private static class ClassFrame extends LexicalFrame { + /** Set of name of instance members declared in this frame. */ + private final Set mInstanceMembers; + /** Set of name of variables declared in this frame. */ + private final Set mStaticMembers; + + /** + * Creates new instance of ClassFrame + */ + public ClassFrame() + { + super(); + mInstanceMembers = Sets.newHashSet(); + mStaticMembers = Sets.newHashSet(); + } + + /** + * Adds static member's name. + * @param aName a name of static member of the class + */ + public void addStaticMember(final String aName) + { + mStaticMembers.add(aName); + } + + /** + * Adds instance member's name. + * @param aName a name of instance member of the class + */ + public void addInstanceMember(final String aName) + { + mInstanceMembers.add(aName); + } + + /** + * Checks if a given name is a known instance member of the class. + * @param aName a name to check + * @return true is the given name is a name of a known + * instance member of the class + */ + public boolean hasInstanceMember(final String aName) + { + return mInstanceMembers.contains(aName); + } + + @Override + boolean contains(String aNameToFind) + { + return super.contains(aNameToFind) + || mInstanceMembers.contains(aNameToFind) + || mStaticMembers.contains(aNameToFind); + } } /** diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java index 59e65fb38..4f3f84362 100755 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java @@ -69,7 +69,7 @@ public class RequireThisCheck extends DeclarationCollector mCheckFields = aCheckFields; } /** - * @return true if we should check fields usage false overwise. + * @return true if we should check fields usage false otherwise. */ public boolean getCheckFields() { @@ -85,7 +85,7 @@ public class RequireThisCheck extends DeclarationCollector mCheckMethods = aCheckMethods; } /** - * @return true if we should check methods usage false overwise. + * @return true if we should check methods usage false otherwise. */ public boolean getCheckMethods() { @@ -148,7 +148,7 @@ public class RequireThisCheck extends DeclarationCollector // let's check method calls if (parentType == TokenTypes.METHOD_CALL) { - if (mCheckMethods) { + if (mCheckMethods && isClassField(aAST.getText())) { log(aAST, "require.this.method", aAST.getText()); } return; @@ -161,7 +161,7 @@ public class RequireThisCheck extends DeclarationCollector } if (ScopeUtils.getSurroundingScope(aAST) == null) { - // it is not a class or inteface it's + // it is not a class or interface it's // either import or package // we shouldn't checks this return; diff --git a/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis.java b/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis.java index 88115f850..ff53e7c23 100755 --- a/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis.java +++ b/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis.java @@ -1,3 +1,5 @@ +package com.puppycrawl.tools.checkstyle.coding; + import java.awt.Toolkit; public class InputRequireThis { @@ -69,4 +71,19 @@ class Bug2123003 { @interface Rock { String[] band() default "Metallica"; } +} + +class Bug1155921 { + private static int CONST = 1; + private static int static_method() { + return 1; + } + + private int method1() { + return CONST; + } + + private int method2() { + return static_method(); + } } \ No newline at end of file diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java index b846ead85..9dfd4aa06 100755 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java @@ -31,10 +31,10 @@ public class RequireThisCheckTest extends BaseCheckTestSupport final DefaultConfiguration checkConfig = createCheckConfig(RequireThisCheck.class); final String[] expected = { - "6:9: Reference to instance variable 'i' needs \"this.\".", - "12:9: Method call to 'method1' needs \"this.\".", - "26:9: Reference to instance variable 'i' needs \"this.\".", - "51:9: Reference to instance variable 'z' needs \"this.\".", + "8:9: Reference to instance variable 'i' needs \"this.\".", + "14:9: Method call to 'method1' needs \"this.\".", + "28:9: Reference to instance variable 'i' needs \"this.\".", + "53:9: Reference to instance variable 'z' needs \"this.\".", // "13:9: Unable find where 'j' is declared.", }; verify(checkConfig, @@ -49,7 +49,7 @@ public class RequireThisCheckTest extends BaseCheckTestSupport createCheckConfig(RequireThisCheck.class); checkConfig.addAttribute("checkFields", "false"); final String[] expected = { - "12:9: Method call to 'method1' needs \"this.\".", + "14:9: Method call to 'method1' needs \"this.\".", }; verify(checkConfig, getPath("coding" + File.separator + "InputRequireThis.java"), @@ -63,9 +63,9 @@ public class RequireThisCheckTest extends BaseCheckTestSupport createCheckConfig(RequireThisCheck.class); checkConfig.addAttribute("checkMethods", "false"); final String[] expected = { - "6:9: Reference to instance variable 'i' needs \"this.\".", - "26:9: Reference to instance variable 'i' needs \"this.\".", - "51:9: Reference to instance variable 'z' needs \"this.\".", + "8:9: Reference to instance variable 'i' needs \"this.\".", + "28:9: Reference to instance variable 'i' needs \"this.\".", + "53:9: Reference to instance variable 'z' needs \"this.\".", // "13:9: Unable find where 'j' is declared.", }; verify(checkConfig,