Issue #3006: Fix false positive when variable is assigned multiple times

checkstyle · Mar 29, 2016 · 55aa50d · 55aa50d
1 parent 1121035
commit 55aa50d
Showing 1 changed file with 168 additions and 13 deletions.
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheck.java
@@ -209,6 +209,18 @@ && shouldCheckEnhancedForLoopVariable(ast)) {
                 final int parentType = ast.getParent().getType();
                 if (isAssignOperator(parentType)
                         && isFirstChild(ast)) {
+                    if (isInIfBlock(ast))
+                    {
+                        markFinalVariableCandidateAsAssignInIfBlock(ast);
+                    }
+                    else if (isInElseBlock(ast))
+                    {
+                        markFinalVariableCandidateAsAssignInElseBlock(ast);
+                    }
+                    else
+                    {
+                        markFinalVariableCandidateAsAssignOutsideIfOrElseBlock(ast);
+                    }
                     removeVariable(ast);
                 }
                 break;
@@ -218,9 +230,84 @@ && isFirstChild(ast)) {
         }
     }
 
+    private boolean isInIfBlock(DetailAST node) {
+        boolean returnValue = false;
+        for (DetailAST token = node.getParent(); token != null; token = token.getParent()) {
+            final int type = token.getType();
+            if (type == TokenTypes.LITERAL_IF) {
+                returnValue = true;
+                break;
+            }
+        }
+        return returnValue;
+    }
+
+    private boolean isInElseBlock(DetailAST node) {
+        boolean returnValue = false;
+        for (DetailAST token = node.getParent(); token != null; token = token.getParent()) {
+            final int type = token.getType();
+            if (type == TokenTypes.LITERAL_ELSE) {
+                returnValue = true;
+                break;
+            }
+        }
+        return returnValue;
+    }
+
+    private void markFinalVariableCandidateAsAssignInIfBlock(DetailAST ast) {
+        final Iterator<ScopeData> iterator = scopeStack.descendingIterator();
+        while (iterator.hasNext()) {
+            final ScopeData scopeData = iterator.next();
+            final Map<String, FinalVariableCandidate> scope = scopeData.scope;
+            DetailAST storedVariable = null;
+            final FinalVariableCandidate candidate = scope.get(ast.getText());
+            if (candidate != null) {
+                storedVariable = candidate.variableIdent;
+            }
+            if (storedVariable != null && isSameVariables(storedVariable, ast)) {
+                candidate.assignInIfBlock = true;
+                break;
+            }
+        }
+    }
+
+    private void markFinalVariableCandidateAsAssignInElseBlock(DetailAST ast) {
+        final Iterator<ScopeData> iterator = scopeStack.descendingIterator();
+        while (iterator.hasNext()) {
+            final ScopeData scopeData = iterator.next();
+            final Map<String, FinalVariableCandidate> scope = scopeData.scope;
+            DetailAST storedVariable = null;
+            final FinalVariableCandidate candidate = scope.get(ast.getText());
+            if (candidate != null) {
+                storedVariable = candidate.variableIdent;
+            }
+            if (storedVariable != null && isSameVariables(storedVariable, ast)) {
+                candidate.assignInElseBlock = true;
+                break;
+            }
+        }
+    }
+
+    private void markFinalVariableCandidateAsAssignOutsideIfOrElseBlock(DetailAST ast) {
+        final Iterator<ScopeData> iterator = scopeStack.descendingIterator();
+        while (iterator.hasNext()) {
+            final ScopeData scopeData = iterator.next();
+            final Map<String, FinalVariableCandidate> scope = scopeData.scope;
+            DetailAST storedVariable = null;
+            final FinalVariableCandidate candidate = scope.get(ast.getText());
+            if (candidate != null) {
+                storedVariable = candidate.variableIdent;
+            }
+            if (storedVariable != null && isSameVariables(storedVariable, ast)) {
+                candidate.assignOutsideIfOrElseBlock = true;
+                break;
+            }
+        }
+    }
+
     @Override
     public void leaveToken(DetailAST ast) {
-        Map<String, DetailAST> scope = null;
+        Map<String, FinalVariableCandidate> scope = null;
         switch (ast.getType()) {
             case TokenTypes.OBJBLOCK:
             case TokenTypes.CTOR_DEF:
@@ -245,8 +332,9 @@ public void leaveToken(DetailAST ast) {
                 // do nothing
         }
         if (scope != null) {
-            for (DetailAST node : scope.values()) {
+            for (FinalVariableCandidate candidate : scope.values()) {
-                log(node.getLineNo(), node.getColumnNo(), MSG_KEY, node.getText());
+                DetailAST ident = candidate.variableIdent;
+                log(ident.getLineNo(), ident.getColumnNo(), MSG_KEY, ident.getText());
             }
         }
     }
@@ -274,7 +362,11 @@ private void updateUninitializedVariables(Deque<DetailAST>
         // Check for only previous scope
         for (DetailAST variable : prevScopeUnitializedVariableData) {
             for (ScopeData scopeData : scopeStack) {
-                final DetailAST storedVariable = scopeData.scope.get(variable.getText());
+                final FinalVariableCandidate candidate = scopeData.scope.get(variable.getText());
+                DetailAST storedVariable = null;
+                if (candidate != null) {
+                    storedVariable = candidate.variableIdent;
+                }
                 if (storedVariable != null && isSameVariables(storedVariable, variable)
                         && !scopeData.uninitializedVariables.contains(storedVariable)) {
                     scopeData.uninitializedVariables.push(variable);
@@ -285,7 +377,11 @@ private void updateUninitializedVariables(Deque<DetailAST>
         for (Deque<DetailAST> unitializedVariableData : prevScopeUninitializedVariables) {
             for (DetailAST variable : unitializedVariableData) {
                 for (ScopeData scopeData : scopeStack) {
-                    final DetailAST storedVariable = scopeData.scope.get(variable.getText());
+                    final FinalVariableCandidate candidate = scopeData.scope.get(variable.getText());
+                    DetailAST storedVariable = null;
+                    if (candidate != null) {
+                        storedVariable = candidate.variableIdent;
+                    }
                     if (storedVariable != null
                             && isSameVariables(storedVariable, variable)
                             && !scopeData.uninitializedVariables.contains(storedVariable)) {
@@ -344,19 +440,19 @@ private boolean shouldCheckEnhancedForLoopVariable(DetailAST ast) {
      * @param ast the variable to insert.
      */
     private void insertParameter(DetailAST ast) {
-        final Map<String, DetailAST> scope = scopeStack.peek().scope;
+        final Map<String, FinalVariableCandidate> scope = scopeStack.peek().scope;
         final DetailAST astNode = ast.findFirstToken(TokenTypes.IDENT);
-        scope.put(astNode.getText(), astNode);
+        scope.put(astNode.getText(), new FinalVariableCandidate(astNode));
     }
 
     /**
      * Insert a variable at the topmost scope stack.
      * @param ast the variable to insert.
      */
     private void insertVariable(DetailAST ast) {
-        final Map<String, DetailAST> scope = scopeStack.peek().scope;
+        final Map<String, FinalVariableCandidate> scope = scopeStack.peek().scope;
         final DetailAST astNode = ast.findFirstToken(TokenTypes.IDENT);
-        scope.put(astNode.getText(), astNode);
+        scope.put(astNode.getText(), new FinalVariableCandidate(astNode));
         if (!isInitialized(astNode)) {
             scopeStack.peek().uninitializedVariables.add(astNode);
         }
@@ -388,8 +484,12 @@ private void removeVariable(DetailAST ast) {
         final Iterator<ScopeData> iterator = scopeStack.descendingIterator();
         while (iterator.hasNext()) {
             final ScopeData scopeData = iterator.next();
-            final Map<String, DetailAST> scope = scopeData.scope;
+            final Map<String, FinalVariableCandidate> scope = scopeData.scope;
-            final DetailAST storedVariable = scope.get(ast.getText());
+            final FinalVariableCandidate candidate = scope.get(ast.getText());
+            DetailAST storedVariable = null;
+            if (candidate != null) {
+                storedVariable = candidate.variableIdent;
+            }
             if (storedVariable != null && isSameVariables(storedVariable, ast)) {
                 if (shouldRemoveVariable(scopeData, ast)) {
                     scope.remove(ast.getText());
@@ -416,15 +516,55 @@ private static boolean shouldRemoveVariable(ScopeData scopeData, DetailAST ast)
                 // more than once in this case
                 if (isInTheSameLoop(variable, ast)
                         || !isUseOfExternalVariableInsideLoop(ast)) {
-                    shouldRemove = false;
+                    if (isAssignInIfBlock(scopeData, ast) && isAssignInElseBlock(scopeData, ast)) {
+                        shouldRemove = true;
+                    }
+                    else if (isAssignInIfBlock(scopeData, ast)
+                        && isAssignOutsideIfOrElseBlock(scopeData, ast)) {
+                        shouldRemove = true;
+                    }
+                    else {
+                        shouldRemove = false;
+                    }
                 }
+
                 scopeData.uninitializedVariables.remove(variable);
                 break;
             }
         }
         return shouldRemove;
     }
 
+    private static boolean isAssignInIfBlock(ScopeData scopeData, DetailAST ast) {
+        boolean assignInIfElseBlock = false;
+        FinalVariableCandidate candidate = scopeData.scope.get(ast.getText());
+        if (candidate != null)
+        {
+            assignInIfElseBlock = candidate.assignInIfBlock;
+        }
+        return assignInIfElseBlock;
+    }
+
+    private static boolean isAssignInElseBlock(ScopeData scopeData, DetailAST ast) {
+        boolean assignInIfElseBlock = false;
+        FinalVariableCandidate candidate = scopeData.scope.get(ast.getText());
+        if (candidate != null)
+        {
+            assignInIfElseBlock = candidate.assignInElseBlock;
+        }
+        return assignInIfElseBlock;
+    }
+
+    private static boolean isAssignOutsideIfOrElseBlock(ScopeData scopeData, DetailAST ast) {
+        boolean assignInIfElseBlock = false;
+        FinalVariableCandidate candidate = scopeData.scope.get(ast.getText());
+        if (candidate != null)
+        {
+            assignInIfElseBlock = candidate.assignOutsideIfOrElseBlock;
+        }
+        return assignInIfElseBlock;
+    }
+
     /**
      * Checks whether a variable which is declared ouside loop is used inside loop.
      * For example:
@@ -565,9 +705,24 @@ private static boolean isLoopAst(int ast) {
      */
     private static class ScopeData {
         /** Contains variable definitions. */
-        private final Map<String, DetailAST> scope = new HashMap<>();
+        private final Map<String, FinalVariableCandidate> scope = new HashMap<>();
 
         /** Contains definitions of uninitialized variables. */
         private final Deque<DetailAST> uninitializedVariables = new ArrayDeque<>();
     }
+
+    private static class FinalVariableCandidate {
+
+        private DetailAST variableIdent;
+
+        private boolean assignInIfBlock;
+
+        private boolean assignInElseBlock;
+
+        private boolean assignOutsideIfOrElseBlock;
+
+        public FinalVariableCandidate(DetailAST variableIdent) {
+            this.variableIdent = variableIdent;
+        }
+    }
 }