From 628e893f24cf41ad122c5adf656832a89168e766 Mon Sep 17 00:00:00 2001 From: Bhavik Patel Date: Sun, 10 Jan 2016 10:01:06 +0530 Subject: [PATCH] Issue #2807: FinalLocalVariable doesn't report variable when condition separates 2 assignments --- .../coding/FinalLocalVariableCheck.java | 77 ++++++++- .../coding/FinalLocalVariableCheckTest.java | 9 ++ .../coding/InputFinalLocalVariable.java | 152 ++++++++++++++++++ 3 files changed, 235 insertions(+), 3 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 9e3b884dc..5049bed1f 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 @@ -115,6 +115,10 @@ public class FinalLocalVariableCheck extends Check { /** Scope Deque. */ private final Deque scopeStack = new ArrayDeque<>(); + /** Uninitialized variables of previous scope. */ + private final Deque> prevScopeUninitializedVariables = + new ArrayDeque<>(); + /** Controls whether to check enhanced for-loop variable. */ private boolean validateEnhancedForLoopVariable; @@ -180,6 +184,7 @@ public class FinalLocalVariableCheck extends Check { if (ast.getParent().getType() != TokenTypes.CASE_GROUP || ast.getParent().getParent().findFirstToken(TokenTypes.CASE_GROUP) == ast.getParent()) { + storePrevScopeUninitializedVariableData(); scopeStack.push(new ScopeData()); } break; @@ -223,10 +228,17 @@ public class FinalLocalVariableCheck extends Check { scope = scopeStack.pop().scope; break; case TokenTypes.SLIST: + final Deque prevScopeUnitializedVariableData = + prevScopeUninitializedVariables.peek(); if (ast.getParent().getType() != TokenTypes.CASE_GROUP - || findLastToken(ast.getParent().getParent(), TokenTypes.CASE_GROUP, - TokenTypes.SLIST) == ast.getParent()) { + || findLastChildWhichContainsSpecifiedToken(ast.getParent().getParent(), + TokenTypes.CASE_GROUP, TokenTypes.SLIST) == ast.getParent()) { scope = scopeStack.pop().scope; + prevScopeUninitializedVariables.pop(); + } + final DetailAST parent = ast.getParent(); + if (shouldUpdateUninitializedVariables(parent)) { + updateUninitializedVariables(prevScopeUnitializedVariableData); } break; default: @@ -239,6 +251,64 @@ public class FinalLocalVariableCheck extends Check { } } + /** + * Store un-initialized variables in a temporary stack for future use. + */ + private void storePrevScopeUninitializedVariableData() { + final ScopeData scopeData = scopeStack.peek(); + final Deque prevScopeUnitializedVariableData = + new ArrayDeque<>(); + for (DetailAST variable : scopeData.uninitializedVariables) { + prevScopeUnitializedVariableData.push(variable); + } + prevScopeUninitializedVariables.push(prevScopeUnitializedVariableData); + } + + /** + * Update current scope data uninitialized variable according to the previous scope data. + * @param prevScopeUnitializedVariableData variable for previous stack of uninitialized + * variables + */ + private void updateUninitializedVariables(Deque + prevScopeUnitializedVariableData) { + // Check for only previous scope + for (DetailAST variable : prevScopeUnitializedVariableData) { + for (ScopeData scopeData : scopeStack) { + final DetailAST storedVariable = scopeData.scope.get(variable.getText()); + if (storedVariable != null && isSameVariables(storedVariable, variable) + && !scopeData.uninitializedVariables.contains(storedVariable)) { + scopeData.uninitializedVariables.push(variable); + } + } + } + // Check for rest of the scope + for (Deque unitializedVariableData : prevScopeUninitializedVariables) { + for (DetailAST variable : unitializedVariableData) { + for (ScopeData scopeData : scopeStack) { + final DetailAST storedVariable = scopeData.scope.get(variable.getText()); + if (storedVariable != null + && isSameVariables(storedVariable, variable) + && !scopeData.uninitializedVariables.contains(storedVariable)) { + scopeData.uninitializedVariables.push(variable); + } + } + } + } + } + + /** + * If token is LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, or LITERAL_ELSE, then do not + * update the uninitialized variables. + * @param ast token to be checked + * @return true if should be updated, else false + */ + private boolean shouldUpdateUninitializedVariables(DetailAST ast) { + return ast.getType() != TokenTypes.LITERAL_TRY + && ast.getType() != TokenTypes.LITERAL_CATCH + && ast.getType() != TokenTypes.LITERAL_FINALLY + && ast.getType() != TokenTypes.LITERAL_ELSE; + } + /** * Returns the last child token that makes a specified type and contains containType in * its branch. @@ -247,7 +317,8 @@ public class FinalLocalVariableCheck extends Check { * @param containType the token type which has to be present in the branch * @return the matching token, or null if no match */ - public DetailAST findLastToken(DetailAST ast, int childType, int containType) { + public DetailAST findLastChildWhichContainsSpecifiedToken(DetailAST ast, int childType, + int containType) { DetailAST returnValue = null; for (DetailAST astIterator = ast.getFirstChild(); astIterator != null; astIterator = astIterator.getNextSibling()) { diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java index 95e42f1b6..5c541d4cc 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/FinalLocalVariableCheckTest.java @@ -81,6 +81,15 @@ public class FinalLocalVariableCheckTest "247:17: " + getCheckMessage(MSG_KEY, "n"), "259:17: " + getCheckMessage(MSG_KEY, "t"), "269:21: " + getCheckMessage(MSG_KEY, "foo"), + "288:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "300:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "344:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "357:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "360:21: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "375:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "386:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "418:13: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), + "421:21: " + getCheckMessage(MSG_KEY, "shouldBeFinal"), }; verify(checkConfig, getPath("InputFinalLocalVariable.java"), expected); } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputFinalLocalVariable.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputFinalLocalVariable.java index 5f00bbec5..2fefa2799 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputFinalLocalVariable.java +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/coding/InputFinalLocalVariable.java @@ -281,3 +281,155 @@ class classs3 { } } } + +class Class3 { + public void test1() { + final boolean b = true; + int shouldBeFinal; //Violation + + if (b) { + shouldBeFinal = 1; + } + else { + shouldBeFinal = 2; + } + } + + public void test2() { + final int b = 10; + int shouldBeFinal; //Violation + + switch (b) { + case 0: + shouldBeFinal = 1; + break; + default: + shouldBeFinal = 2; + break; + } + } + + public void test3() { + int x; // No Violation + try { + x = 0; + } catch (final Exception e) { + x = 1; + } + + int y; // No Violation + try { + y = 0; + } finally { + y = 1; + } + } + + public void test4() { + final boolean b = false; + int x; // No Violation + if (b) { + x = 1; + } else { + x = 2; + } + + if(b) { + x = 3; + } + } + + public void test5() { + final boolean b = false; + int shouldBeFinal; //Violation + if(b) { + } + if (b) { + shouldBeFinal = 1; + } else { + shouldBeFinal = 2; + } + } +} + +class class4 { + public void foo() { + int shouldBeFinal; //violation + class Bar { + void bar () { + int shouldBeFinal; //Violation + final boolean b = false; + if (b) { + shouldBeFinal = 1; + } else { + shouldBeFinal = 2; + } + } + } + } +} + +class class5 { + public void test1(){ + final boolean b = false; + int shouldBeFinal; //Violation + if(b){ + if(b){ + shouldBeFinal = 1; + } else { + shouldBeFinal = 2; + } + } + } + public void test2() { + final int b = 10; + int shouldBeFinal; //Violation + + switch (b) { + case 0: + switch (b) { + case 0: + shouldBeFinal = 1; + break; + default: + shouldBeFinal = 2; + break; + } + break; + default: + shouldBeFinal = 3; + break; + } + } + public void test3() { + int x; //No Violation + try { + x = 0; + try { + x = 0; + } catch (final Exception e) { + x = 1; + } + } catch (final Exception e) { + x = 1; + } + } + public void test4() { + int shouldBeFinal; //violation + class Bar { + void bar () { + int shouldBeFinal; //Violation + final boolean b = false; + if (b) { + if (b) { + shouldBeFinal = 1; + } else { + shouldBeFinal = 2; + } + } else { + shouldBeFinal = 2; + } + } + } + } +}