From 11ff590ba5cc717cbe00d88eb0b578e805572bc8 Mon Sep 17 00:00:00 2001 From: Bhavik Patel Date: Sun, 14 Jun 2015 20:04:25 +0530 Subject: [PATCH] Solution to wrong variable reported because of name shadowing in FinalLocalVariableCheck. solves #1142 --- .../coding/FinalLocalVariableCheck.java | 52 +++++++++++++++---- .../coding/FinalLocalVariableCheckTest.java | 14 +++++ .../InputFinalLocalVariableNameShadowing.java | 26 ++++++++++ 3 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputFinalLocalVariableNameShadowing.java 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 4dd0f4d86..ac95e7740 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 @@ -159,8 +159,9 @@ public class FinalLocalVariableCheck extends Check { } case TokenTypes.VARIABLE_DEF: if (ast.getParent().getType() != TokenTypes.OBJBLOCK - && shouldCheckEnhancedForLoopVariable(ast) - && isVariableInForInit(ast)) { + && shouldCheckEnhancedForLoopVariable(ast) + && isVariableInForInit(ast) + && !ast.branchContains(TokenTypes.FINAL)) { insertVariable(ast); } break; @@ -255,16 +256,48 @@ public class FinalLocalVariableCheck extends Check { return paramDef.getParent().getParent().getType() == TokenTypes.LAMBDA; } + /** + * Find the Class or Method in which it is defined. + * @param ast Variable for which we want to find the scope in which it is defined + * @return ast The Class or Method in which it is defined. + */ + private static DetailAST findClassOrMethodInWhichItIsDefined(DetailAST ast) { + DetailAST astTraverse = ast; + while (!(astTraverse.getType() == TokenTypes.METHOD_DEF + || astTraverse.getType() == TokenTypes.CLASS_DEF)) { + astTraverse = astTraverse.getParent(); + } + return astTraverse; + } + + /** + * Check if both the Variable are same. + * @param ast1 Variable to compare + * @param ast2 Variable to compare + * @return true if both the variable are same, otherwise false + */ + private static boolean isSameVariables(DetailAST ast1, DetailAST ast2) { + final DetailAST classOrMethodOfAst1 = + findClassOrMethodInWhichItIsDefined(ast1); + final DetailAST classOrMethodOfAst2 = + findClassOrMethodInWhichItIsDefined(ast2); + + final String identifierOfAst1 = + classOrMethodOfAst1.findFirstToken(TokenTypes.IDENT).getText(); + final String identifierOfAst2 = + classOrMethodOfAst2.findFirstToken(TokenTypes.IDENT).getText(); + + return identifierOfAst1.equals(identifierOfAst2); + } + /** * Inserts a variable at the topmost scope stack * @param ast the variable to insert */ private void insertVariable(DetailAST ast) { - if (!ast.branchContains(TokenTypes.FINAL)) { - final Map state = scopeStack.peek(); - final DetailAST astNode = ast.findFirstToken(TokenTypes.IDENT); - state.put(astNode.getText(), astNode); - } + final Map state = scopeStack.peek(); + final DetailAST astNode = ast.findFirstToken(TokenTypes.IDENT); + state.put(astNode.getText(), astNode); } /** @@ -275,8 +308,9 @@ public class FinalLocalVariableCheck extends Check { final Iterator> iterator = scopeStack.descendingIterator(); while (iterator.hasNext()) { final Map state = iterator.next(); - final Object obj = state.remove(ast.getText()); - if (obj != null) { + final DetailAST storedVariable = state.get(ast.getText()); + if (storedVariable != null && isSameVariables(storedVariable, ast)) { + state.remove(ast.getText()); break; } } 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 bc6ea6cbb..42417b398 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 @@ -131,4 +131,18 @@ public class FinalLocalVariableCheckTest + "tools/checkstyle/naming/InputFinalLocalVariableNameLambda.java") .getCanonicalPath(), expected); } + + @Test + public void testVariableNameShadowing() + throws Exception { + final DefaultConfiguration checkConfig = + createCheckConfig(FinalLocalVariableCheck.class); + checkConfig.addAttribute("tokens", "PARAMETER_DEF,VARIABLE_DEF"); + + final String[] expected = { + "4:28: " + "Variable 'text' should be declared final.", + "17:13: " + "Variable 'x' should be declared final.", + }; + verify(checkConfig, getPath("coding/InputFinalLocalVariableNameShadowing.java"), expected); + } } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputFinalLocalVariableNameShadowing.java b/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputFinalLocalVariableNameShadowing.java new file mode 100644 index 000000000..9ea6bf54b --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/coding/InputFinalLocalVariableNameShadowing.java @@ -0,0 +1,26 @@ +package com.puppycrawl.tools.checkstyle.coding; + +class Foo1 { + public void foo(String text) { + System.out.println(text); + + class Bar { + void bar (String text) { + text = "xxx"; + } + } + } +} + +class Foo2 { + public void foo() { + int x; + class Bar { + void bar () { + int x = 1; + x++; + x++; + } + } + } +}