Pull #5392: fixed RequireThisCheck and for loop variable handling

4 files changed, 83 insertions, 5 deletions

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java
index 1e34daf37..58e5f7b3d 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java
@@ -91,6 +91,7 @@ import com.puppycrawl.tools.checkstyle.utils.TokenUtils;
  * @author o_sukhodolsky
  * @author Andrei Selkin
  */
+// -@cs[ClassDataAbstractionCoupling] This check requires to work with and identify many frames.
 @FileStatefulCheck
 public class RequireThisCheck extends AbstractCheck {
 
@@ -197,6 +198,7 @@ public class RequireThisCheck extends AbstractCheck {
             TokenTypes.ANNOTATION_DEF,
             TokenTypes.CTOR_DEF,
             TokenTypes.METHOD_DEF,
+            TokenTypes.LITERAL_FOR,
             TokenTypes.SLIST,
             TokenTypes.IDENT,
         };
@@ -241,6 +243,7 @@ public class RequireThisCheck extends AbstractCheck {
             case TokenTypes.SLIST :
             case TokenTypes.METHOD_DEF :
             case TokenTypes.CTOR_DEF :
+            case TokenTypes.LITERAL_FOR :
                 current.push(frames.get(ast));
                 break;
             default :
@@ -258,6 +261,7 @@ public class RequireThisCheck extends AbstractCheck {
             case TokenTypes.SLIST :
             case TokenTypes.METHOD_DEF :
             case TokenTypes.CTOR_DEF :
+            case TokenTypes.LITERAL_FOR:
                 current.pop();
                 break;
             default :
@@ -352,6 +356,7 @@ public class RequireThisCheck extends AbstractCheck {
      * @param frameStack stack containing the FrameTree being built.
      * @param ast AST to parse.
      */
+    // -@cs[JavaNCSS] This method is a big switch and is too hard to remove.
     private static void collectDeclarations(Deque<AbstractFrame> frameStack, DetailAST ast) {
         final AbstractFrame frame = frameStack.peek();
         switch (ast.getType()) {
@@ -401,6 +406,10 @@ public class RequireThisCheck extends AbstractCheck {
                         TokenTypes.IDENT));
                 frameStack.addFirst(catchFrame);
                 break;
+            case TokenTypes.LITERAL_FOR:
+                final AbstractFrame forFrame = new ForFrame(frame, ast);
+                frameStack.addFirst(forFrame);
+                break;
             case TokenTypes.LITERAL_NEW:
                 if (isAnonymousClassDef(ast)) {
                     frameStack.addFirst(new AnonymousClassFrame(frame,
@@ -450,6 +459,7 @@ public class RequireThisCheck extends AbstractCheck {
             case TokenTypes.METHOD_DEF :
             case TokenTypes.CTOR_DEF :
             case TokenTypes.LITERAL_CATCH :
+            case TokenTypes.LITERAL_FOR :
                 frames.put(ast, frameStack.poll());
                 break;
             case TokenTypes.LITERAL_NEW :
@@ -617,7 +627,8 @@ public class RequireThisCheck extends AbstractCheck {
     private boolean canBeReferencedFromStaticContext(DetailAST ident) {
         AbstractFrame variableDeclarationFrame = findFrame(ident, false);
         boolean staticInitializationBlock = false;
-        while (variableDeclarationFrame.getType() == FrameType.BLOCK_FRAME) {
+        while (variableDeclarationFrame.getType() == FrameType.BLOCK_FRAME
+                || variableDeclarationFrame.getType() == FrameType.FOR_FRAME) {
             final DetailAST blockFrameNameIdent = variableDeclarationFrame.getFrameNameIdent();
             final DetailAST definitionToken = blockFrameNameIdent.getParent();
             if (definitionToken.getType() == TokenTypes.STATIC_INIT) {
@@ -983,6 +994,8 @@ public class RequireThisCheck extends AbstractCheck {
         BLOCK_FRAME,
         /** Catch frame type. */
         CATCH_FRAME,
+        /** Lambda frame type. */
+        FOR_FRAME,
     }
 
     /**
@@ -1408,4 +1421,24 @@ public class RequireThisCheck extends AbstractCheck {
             return FrameType.CATCH_FRAME;
         }
     }
+
+    /**
+     * A frame initiated on entering a for block; holds local for variable names.
+     * @author Richard Veach
+     */
+    public static class ForFrame extends AbstractFrame {
+        /**
+         * Creates for frame.
+         * @param parent parent frame.
+         * @param ident ident frame name ident.
+         */
+        protected ForFrame(AbstractFrame parent, DetailAST ident) {
+            super(parent, ident);
+        }
+
+        @Override
+        public FrameType getType() {
+            return FrameType.FOR_FRAME;
+        }
+    }
 }
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java
index 512aaa884..83b7dac17 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java
@@ -61,8 +61,10 @@ public class RequireThisCheckTest extends AbstractModuleTestSupport {
             "122:13: " + getCheckMessage(MSG_VARIABLE, "i", "Issue2240."),
             "134:9: " + getCheckMessage(MSG_METHOD, "foo", ""),
             "142:9: " + getCheckMessage(MSG_VARIABLE, "s", ""),
-            "167:16: " + getCheckMessage(MSG_VARIABLE, "a", ""),
-            "167:20: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "168:16: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "168:20: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "174:16: " + getCheckMessage(MSG_VARIABLE, "b", ""),
+            "174:20: " + getCheckMessage(MSG_VARIABLE, "b", ""),
         };
         verify(checkConfig,
                getPath("InputRequireThisEnumInnerClassesAndBugs.java"),
@@ -101,8 +103,10 @@ public class RequireThisCheckTest extends AbstractModuleTestSupport {
             "114:9: " + getCheckMessage(MSG_VARIABLE, "i", ""),
             "122:13: " + getCheckMessage(MSG_VARIABLE, "i", "Issue2240."),
             "142:9: " + getCheckMessage(MSG_VARIABLE, "s", ""),
-            "167:16: " + getCheckMessage(MSG_VARIABLE, "a", ""),
-            "167:20: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "168:16: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "168:20: " + getCheckMessage(MSG_VARIABLE, "a", ""),
+            "174:16: " + getCheckMessage(MSG_VARIABLE, "b", ""),
+            "174:20: " + getCheckMessage(MSG_VARIABLE, "b", ""),
         };
         verify(checkConfig,
                getPath("InputRequireThisEnumInnerClassesAndBugs.java"),
@@ -328,6 +332,17 @@ public class RequireThisCheckTest extends AbstractModuleTestSupport {
     }
 
     @Test
+    public void testFor() throws Exception {
+        final DefaultConfiguration checkConfig = createModuleConfig(RequireThisCheck.class);
+        checkConfig.addAttribute("validateOnlyOverlapping", "false");
+        final String[] expected = {
+            "13:13: " + getCheckMessage(MSG_VARIABLE, "bottom", ""),
+            "21:34: " + getCheckMessage(MSG_VARIABLE, "name", ""),
+        };
+        verify(checkConfig, getPath("InputRequireThisFor.java"), expected);
+    }
+
+    @Test
     public void test() throws Exception {
         final DefaultConfiguration checkConfig = createModuleConfig(RequireThisCheck.class);
         final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisEnumInnerClassesAndBugs.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisEnumInnerClassesAndBugs.java
index 872996d6f..77beca3a7 100644
--- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisEnumInnerClassesAndBugs.java
+++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisEnumInnerClassesAndBugs.java
@@ -152,6 +152,7 @@ class NestedRechange {
 }
 class NestedFrames {
     int a = 0;
+    int b = 0;
 
     public int oneReturnInMethod2() {
         for (int i = 0; i < 10; i++) {
@@ -166,4 +167,10 @@ class NestedFrames {
         }
         return a + a * a;
     }
+
+    public int oneReturnInMethod3() {
+        for (int b = 0; b < 10; b++) {
+        }
+        return b + b * b;
+    }
 }
diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisFor.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisFor.java
new file mode 100644
index 000000000..9ad0e9f5c
--- /dev/null
+++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/requirethis/InputRequireThisFor.java
@@ -0,0 +1,23 @@
+package com.puppycrawl.tools.checkstyle.checks.coding.requirethis;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+public class InputRequireThisFor {
+    private String name;
+    int bottom;
+
+    public void method1() {
+        for (int i = 0; i < 10; i++) {
+            int bottom = i - 4;
+            bottom = bottom > 0 ? bottom - 1 : bottom;
+        }
+    }
+
+    public void method2() {
+        for (String name : new String[]{}) {
+        }
+
+        Path jarfile = Paths.get(name + ".jar");
+    }
+}