diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java index 7f18348fb..bff37085d 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java @@ -19,11 +19,15 @@ package com.puppycrawl.tools.checkstyle.checks; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; 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.LinkedList; + +import java.util.Deque; +import java.util.Map; +import java.util.Queue; import java.util.Set; /** @@ -34,19 +38,67 @@ import java.util.Set; */ public abstract class DeclarationCollector extends Check { - /** Stack of variable declaration frames. */ - private FrameStack mFrames; + /** + * Tree of all the parsed frames + */ + private Map mFrames; + + /** + * Frame for the currently processed AST + */ + private LexicalFrame mCurrent; @Override public void beginTree(DetailAST aRootAST) { - mFrames = new FrameStack(); + final Deque aFrameStack = Lists.newLinkedList(); + aFrameStack.add(new GlobalFrame()); + + mFrames = Maps.newHashMap(); + + DetailAST curNode = aRootAST; + while (curNode != null) { + collectDeclarations(aFrameStack, curNode); + DetailAST toVisit = curNode.getFirstChild(); + while (curNode != null && toVisit == null) { + endCollectingDeclarations(aFrameStack, curNode); + toVisit = curNode.getNextSibling(); + if (toVisit == null) { + curNode = curNode.getParent(); + } + } + curNode = toVisit; + } } @Override public void visitToken(DetailAST aAST) { - final LexicalFrame frame = this.mFrames.current(); + switch (aAST.getType()) { + case TokenTypes.CLASS_DEF : + case TokenTypes.INTERFACE_DEF : + case TokenTypes.ENUM_DEF : + case TokenTypes.ANNOTATION_DEF : + case TokenTypes.SLIST : + case TokenTypes.METHOD_DEF : + case TokenTypes.CTOR_DEF : + this.mCurrent = this.mFrames.get(aAST); + break; + default : + // do nothing + } + } // end visitToken + + /** + * Parse the next AST for declarations + * + * @param aFrameStack Stack containing the FrameTree being built + * @param aAST AST to parse + */ + private void collectDeclarations(Deque aFrameStack, + DetailAST aAST) + { + final LexicalFrame frame = aFrameStack.peek(); switch (aAST.getType()) { case TokenTypes.VARIABLE_DEF : { final String name = @@ -77,11 +129,11 @@ public abstract class DeclarationCollector extends Check case TokenTypes.ANNOTATION_DEF : { final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT); frame.addName(nameAST.getText()); - this.mFrames.enter(new ClassFrame()); + aFrameStack.addFirst(new ClassFrame(frame)); break; } case TokenTypes.SLIST : - this.mFrames.enter(new BlockFrame()); + aFrameStack.addFirst(new BlockFrame(frame)); break; case TokenTypes.METHOD_DEF : { final String name = aAST.findFirstToken(TokenTypes.IDENT).getText(); @@ -97,7 +149,7 @@ public abstract class DeclarationCollector extends Check } } case TokenTypes.CTOR_DEF : - this.mFrames.enter(new MethodFrame()); + aFrameStack.addFirst(new MethodFrame(frame)); break; default: // do nothing @@ -105,8 +157,14 @@ public abstract class DeclarationCollector extends Check } - @Override - public void leaveToken(DetailAST aAST) + /** + * End parsing of the AST for declarations. + * + * @param aFrameStack Stack containing the FrameTree being built + * @param aAST AST that was parsed + */ + private void endCollectingDeclarations(Queue aFrameStack, + DetailAST aAST) { switch (aAST.getType()) { case TokenTypes.CLASS_DEF : @@ -116,7 +174,7 @@ public abstract class DeclarationCollector extends Check case TokenTypes.SLIST : case TokenTypes.METHOD_DEF : case TokenTypes.CTOR_DEF : - this.mFrames.leave(); + this.mFrames.put(aAST, aFrameStack.poll()); break; default : // do nothing @@ -130,11 +188,26 @@ public abstract class DeclarationCollector extends Check */ protected final boolean isClassField(String aName) { - final LexicalFrame frame = mFrames.findFrame(aName); + final LexicalFrame frame = findFrame(aName); return (frame instanceof ClassFrame) && ((ClassFrame) frame).hasInstanceMember(aName); } + /** + * Find frame containing declaration + * @param aName name of the declaration to find + * @return LexicalFrame containing declaration or null + */ + private LexicalFrame findFrame(String aName) + { + if (mCurrent != null) { + return mCurrent.getIfContains(aName); + } + else { + return null; + } + } + /** * A declaration frame. * @author Stephen Bloch @@ -144,10 +217,19 @@ public abstract class DeclarationCollector extends Check { /** Set of name of variables declared in this frame. */ private final Set mVarNames; + /** + * Parent frame. + */ + private final LexicalFrame mParent; - /** constructor -- invokable only via super() from subclasses */ - protected LexicalFrame() + /** + * constructor -- invokable only via super() from subclasses + * + * @param aParent parent frame + */ + protected LexicalFrame(LexicalFrame aParent) { + mParent = aParent; mVarNames = Sets.newHashSet(); } @@ -167,6 +249,23 @@ public abstract class DeclarationCollector extends Check { return mVarNames.contains(aNameToFind); } + + /** check whether the frame contains a given name. + * @param aNameToFind the name we're looking for + * @return whether it was found + */ + LexicalFrame getIfContains(String aNameToFind) + { + if (contains(aNameToFind)) { + return this; + } + else if (mParent != null) { + return mParent.getIfContains(aNameToFind); + } + else { + return null; + } + } } /** @@ -175,6 +274,14 @@ public abstract class DeclarationCollector extends Check */ private static class GlobalFrame extends LexicalFrame { + + /** + * Constructor for the root of the FrameTree + */ + protected GlobalFrame() + { + super(null); + } } /** @@ -183,6 +290,13 @@ public abstract class DeclarationCollector extends Check */ private static class MethodFrame extends LexicalFrame { + /** + * @param aParent parent frame + */ + protected MethodFrame(LexicalFrame aParent) + { + super(aParent); + } } /** @@ -200,10 +314,11 @@ public abstract class DeclarationCollector extends Check /** * Creates new instance of ClassFrame + * @param aParent parent frame */ - public ClassFrame() + public ClassFrame(LexicalFrame aParent) { - super(); + super(aParent); mInstanceMembers = Sets.newHashSet(); mStaticMembers = Sets.newHashSet(); } @@ -254,61 +369,13 @@ public abstract class DeclarationCollector extends Check */ private static class BlockFrame extends LexicalFrame { - } - - /** - * A stack of LexicalFrames. Standard issue.... - * @author Stephen Bloch - */ - private static class FrameStack - { - /** List of lexical frames. */ - private final LinkedList mFrameList; - - /** Creates an empty FrameStack. */ - FrameStack() - { - mFrameList = Lists.newLinkedList(); - this.enter(new GlobalFrame()); - } /** - * Enter a scope, i.e. push a frame on the stack. - * @param aNewFrame the already-created frame to push + * @param aParent parent frame */ - void enter(LexicalFrame aNewFrame) + protected BlockFrame(LexicalFrame aParent) { - mFrameList.addFirst(aNewFrame); - } - - /** Leave a scope, i.e. pop a frame from the stack. */ - void leave() - { - mFrameList.removeFirst(); - } - - /** - * Get current scope, i.e. top frame on the stack. - * @return the current frame - */ - LexicalFrame current() - { - return mFrameList.getFirst(); - } - - /** - * Search this and containing frames for a given name. - * @param aNameToFind the name we're looking for - * @return the frame in which it was found, or null if not found - */ - LexicalFrame findFrame(String aNameToFind) - { - for (LexicalFrame thisFrame : mFrameList) { - if (thisFrame.contains(aNameToFind)) { - return thisFrame; - } - } - return null; + super(aParent); } } } 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 1278bc58a..dc42835f3 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java @@ -185,7 +185,8 @@ public class RequireThisCheck extends DeclarationCollector || (parentType == TokenTypes.CLASS_DEF) || (parentType == TokenTypes.ENUM_DEF) || (parentType == TokenTypes.INTERFACE_DEF) - || (parentType == TokenTypes.PARAMETER_DEF)) + || (parentType == TokenTypes.PARAMETER_DEF) + || (parentType == TokenTypes.TYPE_ARGUMENT)) { // it's being declared; no problem return; diff --git a/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis2.java b/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis2.java new file mode 100644 index 000000000..8de704356 --- /dev/null +++ b/src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis2.java @@ -0,0 +1,15 @@ +package com.puppycrawl.tools.checkstyle.coding; + +public class InputRequireThis2 { + private final int number = 1; + + public int check() { + int sum = number; + sum += other(); + return sum; + } + + private int other() { + return 0; + } +} \ 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 24c6328ee..dab7b0e8a 100644 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java @@ -34,6 +34,7 @@ public class RequireThisCheckTest extends BaseCheckTestSupport "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.\".", + "46:13: Reference to instance variable 'z' needs \"this.\".", "53:9: Reference to instance variable 'z' needs \"this.\".", // "13:9: Unable find where 'j' is declared.", }; @@ -65,6 +66,7 @@ public class RequireThisCheckTest extends BaseCheckTestSupport final String[] expected = { "8:9: Reference to instance variable 'i' needs \"this.\".", "28:9: Reference to instance variable 'i' needs \"this.\".", + "46:13: Reference to instance variable 'z' needs \"this.\".", "53:9: Reference to instance variable 'z' needs \"this.\".", // "13:9: Unable find where 'j' is declared.", }; @@ -81,4 +83,18 @@ public class RequireThisCheckTest extends BaseCheckTestSupport final String[] expected = {}; verify(checkConfig, getPath("Input15Extensions.java"), expected); } + + @Test + public void testGithubIssue41() throws Exception + { + final DefaultConfiguration checkConfig = + createCheckConfig(RequireThisCheck.class); + final String[] expected = { + "7:19: Reference to instance variable 'number' needs \"this.\".", + "8:16: Method call to 'other' needs \"this.\".", + }; + verify(checkConfig, + getPath("coding" + File.separator + "InputRequireThis2.java"), + expected); + } }