Issue checkstyle#3423: Allow local vars without any unnecessary viola…

…tions from RequireThisCheck. Added UT. Fixed related UT

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheck.java

@@ -437,6 +437,7 @@ private static boolean isAnonymousClassDef(DetailAST ast) {
      * or null otherwise.
      * @param ast IDENT ast to check.
      * @return the class frame where violation is found or null otherwise.
+     * @noinspection IfStatementWithIdenticalBranches
      */
     // -@cs[CyclomaticComplexity] Method already invokes too many methods that fully explain
     // a logic, additional abstraction will not make logic/algorithm more readable.
@@ -475,22 +476,13 @@ && isOverlappingByArgument(ast)
                  && !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)) {
             frameWhereViolationIsFound = findFrame(ast, true);
         }
-        else if (variableDeclarationFrameType == FrameType.BLOCK_FRAME) {
-            if (isOverlappingByLocalVariable(ast)) {
-                if (canAssignValueToClassField(ast)
-                        && !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
-                        && !isReturnedVariable(variableDeclarationFrame, ast)
-                        && canBeReferencedFromStaticContext(ast)) {
-                    frameWhereViolationIsFound = findFrame(ast, true);
-                }
-            }
-            else if (!validateOnlyOverlapping
-                     && prevSibling == null
-                     && isAssignToken(ast.getParent().getType())
-                     && !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
-                     && canBeReferencedFromStaticContext(ast)) {
-                frameWhereViolationIsFound = findFrame(ast, true);
-            }
+        else if (variableDeclarationFrameType == FrameType.BLOCK_FRAME
+                    && isOverlappingByLocalVariable(ast)
+                    && canAssignValueToClassField(ast)
+                    && !isUserDefinedArrangementOfThis(variableDeclarationFrame, ast)
+                    && !isReturnedVariable(variableDeclarationFrame, ast)
+                    && canBeReferencedFromStaticContext(ast)) {
+            frameWhereViolationIsFound = findFrame(ast, true);
         }
         return frameWhereViolationIsFound;
     }

src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/RequireThisCheckTest.java

@@ -191,8 +191,6 @@ public void testValidateOnlyOverlappingFalse() throws Exception {
             "185:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "189:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "210:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
-            "215:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
-            "225:21: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "228:21: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "238:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "253:9: " + getCheckMessage(MSG_VARIABLE, "booleanField", ""),
@@ -204,7 +202,6 @@ public void testValidateOnlyOverlappingFalse() throws Exception {
             "340:9: " + getCheckMessage(MSG_VARIABLE, "field1", ""),
             "374:40: " + getCheckMessage(MSG_METHOD, "getServletRelativeAction", ""),
             "376:20: " + getCheckMessage(MSG_METHOD, "processAction", ""),
-            "383:9: " + getCheckMessage(MSG_VARIABLE, "servletRelativeAction", ""),
             "384:16: " + getCheckMessage(MSG_METHOD, "processAction", ""),
         };
         verify(checkConfig, getPath("InputValidateOnlyOverlappingFalse.java"), expected);
@@ -261,4 +258,20 @@ public void testMethodReferences() throws Exception {
         };
         verify(checkConfig, getPath("InputRequireThisMetodReferences.java"), expected);
     }
+
+    @Test
+    public void testAllowLocalVars() throws Exception {
+        final DefaultConfiguration checkConfig = createCheckConfig(RequireThisCheck.class);
+        checkConfig.addAttribute("validateOnlyOverlapping", "false");
+        checkConfig.addAttribute("checkMethods", "false");
+        final String[] expected = {
+            "14:9: " + getCheckMessage(MSG_VARIABLE, "s1", ""),
+            "22:9: " + getCheckMessage(MSG_VARIABLE, "s1", ""),
+            "35:9: " + getCheckMessage(MSG_VARIABLE, "s2", ""),
+            "40:9: " + getCheckMessage(MSG_VARIABLE, "s2", ""),
+            "46:9: " + getCheckMessage(MSG_VARIABLE, "s2", ""),
+            "47:16: " + getCheckMessage(MSG_VARIABLE, "s1", ""),
+        };
+        verify(checkConfig, getPath("InputAllowLocalVars.java"), expected);
+    }
 }

src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputAllowLocalVars.java

@@ -0,0 +1,49 @@
+/*
+Input test file for RequireThisCheck.
+Created: 2017
+ */
+
+package com.puppycrawl.tools.checkstyle.checks.coding;
+
+class InputAllowLocalVars {
+
+    private String s1 = "foo1";
+    String s2 = "foo2";
+
+    InputAllowLocalVars() {
+        s1 = "bar1"; // Violation. Requires "this".
+        String s2;
+        s2 = "bar2"; // No violation. Local var allowed.
+    }
+
+    public int getS1() {
+        String s1 = null;
+        s1 = "bar"; // No violation
+        s1 = s1;    // Violation. "this" required here to resolve any confusion due to overlapping.
+        return 1;
+    }
+
+    public String getS1(String param) {
+        String s1 = null;
+        s1 = param; // No violation
+        s1 += s1;   // No violation. s1 is being returned.
+        return s1;  // No violation
+    }
+
+    String getS2() {
+        String s2 = null;
+        s2+=s2; // Violation. "this" required here to resolve any confusion due to overlapping.
+        return "return";
+    }
+
+    String getS2(String s2) {
+        s2 = null; // Violation. Requires "this". s2 is a param not a local var.
+        return s2; // No violation. param is returned.
+    }
+
+    String getS2(int a) {
+        String s2 = " ";
+        s2 += s2;  // Violation. "this" required here to resolve any confusion due to overlapping.
+        return s1; // Violation. Requires "this".
+    }
+}

src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputValidateOnlyOverlappingFalse.java

@@ -212,7 +212,7 @@ void foo23() {
 
     void foo24() {
         String field1 = "Hello";
-        field1 = "Java"; // violation
+        field1 = "Java"; // No violation. Local var allowed
         this.booleanField = true;
         this.booleanField = booleanField;
     }
@@ -222,10 +222,10 @@ void foo25() {
             if (true) {
                 String field1 = "Hello, World!";
                 if (true) {
-                    field1 = new String(); // violation
+                    field1 = new String(); // No violation. Local var allowed
                 }
                 else {
-                    field1 = new String(); // violation
+                    field1 += field1; // violation
                 }
             }
         }
@@ -380,7 +380,7 @@ else if (servletRelativeAction.endsWith("/")) {
                 servletRelativeAction = "" + servletRelativeAction;
             }
         }
-        servletRelativeAction = "servletRelativeAction"; // violation
+        servletRelativeAction = "servletRelativeAction"; // No violation. Local var allowed
         return processAction(servletRelativeAction); // violation (Method call to 'processAction' needs "this.".)
     }
 

src/xdocs/config_coding.xml

@@ -3438,7 +3438,7 @@ public static class B {
          Please, be aware of the following logic, which is implemented in the check:
        </p>
        <p>
-         1) If you arranges 'this' in your code on your own, the check will not rise violation for
+         1) If you arrange 'this' in your code on your own, the check will not raise violation for
             variables which use 'this' to reference a class field, for example:
        </p>
        <source>
@@ -3455,7 +3455,7 @@ public class C {
 }
        </source>
        <p>
-         2) If method parameter is returned from the method, the check will not rise violation for
+         2) If method parameter is returned from the method, the check will not raise violation for
             returned variable/parameter, for example:
        </p>
        <source>
@@ -3478,13 +3478,15 @@ public static class A {
   public A(int field1) {
     field1 = field1; // violation: Reference to instance variable "field1" needs "this".
     field2 = 0; // violation: Reference to instance variable "field2" needs "this".
+    String field2;
+    field2 = "0"; // No violation. Local var allowed
   }
 
   void foo3() {
     String field1 = "values";
     field1 = field1; // violation:  Reference to instance variable "field1" needs "this".
     }
-  }
+}
 
 public static class B {
   private int field1;
@@ -3497,6 +3499,19 @@ public static class B {
     return field1 += "suffix"; // violation: Reference to instance variable "field1" needs "this".
   }
 }
+
+// If the variable is locally defined, there won't be a violation provided the variable doesn't overlap.
+class C {
+  private String s1 = "foo1";
+  String s2 = "foo2";
+
+  C() {
+      s1 = "bar1"; // Violation. Reference to instance variable 's1' needs "this.".
+      String s2;
+      s2 = "bar2"; // No violation. Local var allowed.
+      s2 += s2; // Violation. Overlapping. Reference to instance variable 's2' needs "this.".
+  }
+}
        </source>
       </subsection>