From 907a19906f790ce8faea7aa74e12608538dca1b6 Mon Sep 17 00:00:00 2001 From: Ivan Sopov Date: Wed, 30 Oct 2013 18:12:24 +0200 Subject: [PATCH 1/2] fix for #41 RequireThisCheck doesn't check methods and fields that are declared below usage --- .../checks/DeclarationCollector.java | 173 ++++++++++++++---- .../checks/coding/RequireThisCheck.java | 3 +- .../checkstyle/coding/InputRequireThis2.java | 15 ++ .../checks/coding/RequireThisCheckTest.java | 16 ++ 4 files changed, 173 insertions(+), 34 deletions(-) create mode 100644 src/testinputs/com/puppycrawl/tools/checkstyle/coding/InputRequireThis2.java diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java index 7f18348fb..8cd49bc46 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java @@ -19,11 +19,14 @@ 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.Map; import java.util.Set; /** @@ -34,19 +37,64 @@ 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 FrameStack aFrameStack = new FrameStack(); + 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(FrameStack aFrameStack, DetailAST aAST) + { + final LexicalFrame frame = aFrameStack.current(); switch (aAST.getType()) { case TokenTypes.VARIABLE_DEF : { final String name = @@ -77,11 +125,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.enter(new ClassFrame(aFrameStack.current())); break; } case TokenTypes.SLIST : - this.mFrames.enter(new BlockFrame()); + aFrameStack.enter(new BlockFrame(aFrameStack.current())); break; case TokenTypes.METHOD_DEF : { final String name = aAST.findFirstToken(TokenTypes.IDENT).getText(); @@ -97,7 +145,7 @@ public abstract class DeclarationCollector extends Check } } case TokenTypes.CTOR_DEF : - this.mFrames.enter(new MethodFrame()); + aFrameStack.enter(new MethodFrame(aFrameStack.current())); break; default: // do nothing @@ -105,8 +153,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(FrameStack aFrameStack, + DetailAST aAST) { switch (aAST.getType()) { case TokenTypes.CLASS_DEF : @@ -116,7 +170,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.leave()); break; default : // do nothing @@ -130,11 +184,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 +213,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 +245,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 +270,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 +286,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 +310,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,6 +365,14 @@ public abstract class DeclarationCollector extends Check */ private static class BlockFrame extends LexicalFrame { + + /** + * @param aParent parent frame + */ + protected BlockFrame(LexicalFrame aParent) + { + super(aParent); + } } /** @@ -281,10 +400,13 @@ public abstract class DeclarationCollector extends Check mFrameList.addFirst(aNewFrame); } - /** Leave a scope, i.e. pop a frame from the stack. */ - void leave() + /** + * Leave a scope, i.e. pop a frame from the stack. + * @return the left frame + */ + LexicalFrame leave() { - mFrameList.removeFirst(); + return mFrameList.removeFirst(); } /** @@ -295,20 +417,5 @@ public abstract class DeclarationCollector extends Check { 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; - } } } 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); + } } From 53d1722bdb579b2e58666bd48b8015d36719996b Mon Sep 17 00:00:00 2001 From: Ivan Sopov Date: Thu, 31 Oct 2013 15:20:04 +0200 Subject: [PATCH 2/2] refactoring, using standard collection instead of custom abstraction layer on top of it --- .../checks/DeclarationCollector.java | 66 ++++--------------- 1 file changed, 13 insertions(+), 53 deletions(-) diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java index 8cd49bc46..bff37085d 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/DeclarationCollector.java @@ -25,8 +25,9 @@ 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; /** @@ -50,7 +51,9 @@ public abstract class DeclarationCollector extends Check @Override public void beginTree(DetailAST aRootAST) { - final FrameStack aFrameStack = new FrameStack(); + final Deque aFrameStack = Lists.newLinkedList(); + aFrameStack.add(new GlobalFrame()); + mFrames = Maps.newHashMap(); DetailAST curNode = aRootAST; @@ -92,9 +95,10 @@ public abstract class DeclarationCollector extends Check * @param aFrameStack Stack containing the FrameTree being built * @param aAST AST to parse */ - private void collectDeclarations(FrameStack aFrameStack, DetailAST aAST) + private void collectDeclarations(Deque aFrameStack, + DetailAST aAST) { - final LexicalFrame frame = aFrameStack.current(); + final LexicalFrame frame = aFrameStack.peek(); switch (aAST.getType()) { case TokenTypes.VARIABLE_DEF : { final String name = @@ -125,11 +129,11 @@ public abstract class DeclarationCollector extends Check case TokenTypes.ANNOTATION_DEF : { final DetailAST nameAST = aAST.findFirstToken(TokenTypes.IDENT); frame.addName(nameAST.getText()); - aFrameStack.enter(new ClassFrame(aFrameStack.current())); + aFrameStack.addFirst(new ClassFrame(frame)); break; } case TokenTypes.SLIST : - aFrameStack.enter(new BlockFrame(aFrameStack.current())); + aFrameStack.addFirst(new BlockFrame(frame)); break; case TokenTypes.METHOD_DEF : { final String name = aAST.findFirstToken(TokenTypes.IDENT).getText(); @@ -145,7 +149,7 @@ public abstract class DeclarationCollector extends Check } } case TokenTypes.CTOR_DEF : - aFrameStack.enter(new MethodFrame(aFrameStack.current())); + aFrameStack.addFirst(new MethodFrame(frame)); break; default: // do nothing @@ -159,7 +163,7 @@ public abstract class DeclarationCollector extends Check * @param aFrameStack Stack containing the FrameTree being built * @param aAST AST that was parsed */ - private void endCollectingDeclarations(FrameStack aFrameStack, + private void endCollectingDeclarations(Queue aFrameStack, DetailAST aAST) { switch (aAST.getType()) { @@ -170,7 +174,7 @@ public abstract class DeclarationCollector extends Check case TokenTypes.SLIST : case TokenTypes.METHOD_DEF : case TokenTypes.CTOR_DEF : - this.mFrames.put(aAST, aFrameStack.leave()); + this.mFrames.put(aAST, aFrameStack.poll()); break; default : // do nothing @@ -374,48 +378,4 @@ public abstract class DeclarationCollector extends Check super(aParent); } } - - /** - * 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 - */ - void enter(LexicalFrame aNewFrame) - { - mFrameList.addFirst(aNewFrame); - } - - /** - * Leave a scope, i.e. pop a frame from the stack. - * @return the left frame - */ - LexicalFrame leave() - { - return mFrameList.removeFirst(); - } - - /** - * Get current scope, i.e. top frame on the stack. - * @return the current frame - */ - LexicalFrame current() - { - return mFrameList.getFirst(); - } - } }