From 55aa50d8f09d28521f0d0004ce475fa09082faea Mon Sep 17 00:00:00 2001 From: Andrei Selkin Date: Sat, 26 Mar 2016 12:48:04 +0300 Subject: [PATCH] Issue #3006: Fix false positive when variable is assigned multiple times --- .../coding/FinalLocalVariableCheck.java | 181 ++++++++++++++++-- 1 file changed, 168 insertions(+), 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 index 64dfec2b3..47a7d7c2d 100644 --- 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 @@ public class FinalLocalVariableCheck extends AbstractCheck { 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 @@ public class FinalLocalVariableCheck extends AbstractCheck { } } + 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 iterator = scopeStack.descendingIterator(); + while (iterator.hasNext()) { + final ScopeData scopeData = iterator.next(); + final Map 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 iterator = scopeStack.descendingIterator(); + while (iterator.hasNext()) { + final ScopeData scopeData = iterator.next(); + final Map 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 iterator = scopeStack.descendingIterator(); + while (iterator.hasNext()) { + final ScopeData scopeData = iterator.next(); + final Map 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 scope = null; + Map scope = null; switch (ast.getType()) { case TokenTypes.OBJBLOCK: case TokenTypes.CTOR_DEF: @@ -245,8 +332,9 @@ public class FinalLocalVariableCheck extends AbstractCheck { // do nothing } if (scope != null) { - for (DetailAST node : scope.values()) { - log(node.getLineNo(), node.getColumnNo(), MSG_KEY, node.getText()); + for (FinalVariableCandidate candidate : scope.values()) { + DetailAST ident = candidate.variableIdent; + log(ident.getLineNo(), ident.getColumnNo(), MSG_KEY, ident.getText()); } } } @@ -274,7 +362,11 @@ public class FinalLocalVariableCheck extends AbstractCheck { // 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 @@ public class FinalLocalVariableCheck extends AbstractCheck { for (Deque 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,9 +440,9 @@ public class FinalLocalVariableCheck extends AbstractCheck { * @param ast the variable to insert. */ private void insertParameter(DetailAST ast) { - final Map scope = scopeStack.peek().scope; + final Map scope = scopeStack.peek().scope; final DetailAST astNode = ast.findFirstToken(TokenTypes.IDENT); - scope.put(astNode.getText(), astNode); + scope.put(astNode.getText(), new FinalVariableCandidate(astNode)); } /** @@ -354,9 +450,9 @@ public class FinalLocalVariableCheck extends AbstractCheck { * @param ast the variable to insert. */ private void insertVariable(DetailAST ast) { - final Map scope = scopeStack.peek().scope; + final Map 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 @@ public class FinalLocalVariableCheck extends AbstractCheck { final Iterator iterator = scopeStack.descendingIterator(); while (iterator.hasNext()) { final ScopeData scopeData = iterator.next(); - final Map scope = scopeData.scope; - final DetailAST storedVariable = scope.get(ast.getText()); + final Map scope = scopeData.scope; + 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,8 +516,18 @@ public class FinalLocalVariableCheck extends AbstractCheck { // 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; } @@ -425,6 +535,36 @@ public class FinalLocalVariableCheck extends AbstractCheck { 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 @@ public class FinalLocalVariableCheck extends AbstractCheck { */ private static class ScopeData { /** Contains variable definitions. */ - private final Map scope = new HashMap<>(); + private final Map scope = new HashMap<>(); /** Contains definitions of uninitialized variables. */ private final Deque 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; + } + } }