Fixed false positive in check ForbidReturnInFinallyBlockChech. Fixes s…

…evntu-checkstyle#269
aulianenko · Jan 3, 2015 · c3f8e73 · c3f8e73
1 parent 69017f9
commit c3f8e73
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 17 deletions.
diff --git a/...coding/ForbidReturnInFinalBlockCheck.java → ...ding/ForbidReturnInFinallyBlockCheck.java b/...coding/ForbidReturnInFinalBlockCheck.java → ...ding/ForbidReturnInFinallyBlockCheck.java
@@ -18,6 +18,9 @@
 ////////////////////////////////////////////////////////////////////////////////
 package com.github.sevntu.checkstyle.checks.coding;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import com.puppycrawl.tools.checkstyle.api.Check;
 import com.puppycrawl.tools.checkstyle.api.DetailAST;
 import com.puppycrawl.tools.checkstyle.api.TokenTypes;
@@ -33,10 +36,12 @@
  * @author <a href="mailto:vadim.panasiuk@gmail.com">Vadim Panasiuk</a>
  */
 
-public class ForbidReturnInFinalBlockCheck extends Check
+public class ForbidReturnInFinallyBlockCheck extends Check
 {
 
-	public static final String MSG_KEY = "forbid.return.in.final.block";
+	public static final String MSG_KEY = "forbid.return.in.finally.block";
+
+
 
 	@Override
 	public final int[] getDefaultTokens()
@@ -45,14 +50,44 @@ public final int[] getDefaultTokens()
     }
 
     @Override
-    public void visitToken(DetailAST aFinally)
+    public void visitToken(DetailAST aFinallyNode)
+    {
+        final DetailAST bodySlist = aFinallyNode.findFirstToken(TokenTypes.SLIST);
+
+        List<DetailAST> listOfReturnNodes = new ArrayList<DetailAST>();
+
+        collectReturnStatement(bodySlist, listOfReturnNodes);
+
+        for (DetailAST curReturn : listOfReturnNodes) {
+            if (!isReturnInMethodDefinition(curReturn)) {
+                log(aFinallyNode.getLineNo(), MSG_KEY);
+            }
+        }
+    }
+
+    private void collectReturnStatement(DetailAST aNode, List<DetailAST> listOfReturnNodes)
     {
+        DetailAST child = aNode.getFirstChild();
+
+        while (child != null) {
+            if (child.getType() == TokenTypes.LITERAL_RETURN) {
+                listOfReturnNodes.add(child);
+            }
+            collectReturnStatement(child, listOfReturnNodes);
+            child = child.getNextSibling();
+        }
+    }
 
-        final DetailAST bodySlist = aFinally.findFirstToken(TokenTypes.SLIST);
-        final boolean isReturn = bodySlist
-                .branchContains(TokenTypes.LITERAL_RETURN);
-        if (isReturn) {
-            log(aFinally.getLineNo(), MSG_KEY);
+    private boolean isReturnInMethodDefinition(DetailAST aReturnNode)
+    {
+        DetailAST node = aReturnNode;
+
+        while (node.getType() != TokenTypes.LITERAL_FINALLY) {
+            if (node.getType() == TokenTypes.METHOD_DEF) {
+                return true;
+            }
+            node = node.getParent();
         }
+        return false;
     }
-}
+}
diff --git a/...-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties b/...-checks/src/main/resources/com/github/sevntu/checkstyle/checks/coding/messages.properties
@@ -39,7 +39,7 @@ finalize.implementation.useless = finalize() method is useless: it does nothing
 finalize.implementation.missed.super.finalize = You have to call super.finalize() right after finally opening brace.
 forbid.certain.imports=This import should not match ''{0}'' pattern, it is forbidden in {1}.
 forbid.c.comments.in.the.method.body=C-style comments (/*...*/) inside method body are not allowed.
-forbid.return.in.final.block=Finally block should not contain return statements.
+forbid.return.in.finally.block=Finally block should not contain return statements.
 forbid.throw.anonymous.exception=Avoid throwing anonymous exception.
 hidden.field=''{0}'' hides a field.
 illegal.catch=Catching ''{0}'' is not allowed.

diff --git a/...ng/ForbidReturnInFinalBlockCheckTest.java → .../ForbidReturnInFinallyBlockCheckTest.java b/...ng/ForbidReturnInFinalBlockCheckTest.java → .../ForbidReturnInFinallyBlockCheckTest.java
@@ -18,13 +18,13 @@
 ////////////////////////////////////////////////////////////////////////////////
 package com.github.sevntu.checkstyle.checks.coding;
 
-import static com.github.sevntu.checkstyle.checks.coding.ForbidReturnInFinalBlockCheck.*;
+import static com.github.sevntu.checkstyle.checks.coding.ForbidReturnInFinallyBlockCheck.*;
 
 import org.junit.Test;
 import com.github.sevntu.checkstyle.BaseCheckTestSupport;
 import com.puppycrawl.tools.checkstyle.DefaultConfiguration;
 
-public class ForbidReturnInFinalBlockCheckTest 
+public class ForbidReturnInFinallyBlockCheckTest 
         extends BaseCheckTestSupport
 {
     /**
@@ -35,13 +35,13 @@ public class ForbidReturnInFinalBlockCheckTest
     public void testDefault()
             throws Exception
     {
-        final DefaultConfiguration checkConfig = createCheckConfig(ForbidReturnInFinalBlockCheck.class);
+        final DefaultConfiguration checkConfig = createCheckConfig(ForbidReturnInFinallyBlockCheck.class);
         final String[] expected = {
                 "13: " + warningMessage,
                 "26: " + warningMessage,
                 "46: " + warningMessage,
                 "53: " + warningMessage };
-        verify(checkConfig, getPath("InputForbidReturnInFinalBlockCheck.java"),
+        verify(checkConfig, getPath("InputForbidReturnInFinallyBlockCheck.java"),
                 expected);
     }
 }
diff --git a/...g/InputForbidReturnInFinalBlockCheck.java → ...InputForbidReturnInFinallyBlockCheck.java b/...g/InputForbidReturnInFinalBlockCheck.java → ...InputForbidReturnInFinallyBlockCheck.java
@@ -1,7 +1,7 @@
 package com.github.sevntu.checkstyle.checks.coding;
 
-public class InputForbidReturnInFinalBlockCheck
-{
+public class InputForbidReturnInFinallyBlockCheck
+{    
     private class A1
     {
         boolean meth1()
@@ -84,4 +84,20 @@ private static void meth5()
         }
     }
 
-}
+    public void meth6()
+    {
+        try {
+            /// some code
+        }
+        finally {
+            Runtime.getRuntime().addShutdownHook(new Thread()
+            {
+                @Override
+                public void run()
+                {
+                    return;
+                }
+            });
+        }
+    }
+}