-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #14506: False positive for UnusedLocalVariable #14672
Conversation
Github, generate report |
src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedLocalVariableCheckTest.java
Outdated
Show resolved
Hide resolved
3c60d4e
to
15f5e6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor item:
src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedLocalVariableCheck.java
Outdated
Show resolved
Hide resolved
Good to merge, @Kevin222004 please make CI happy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/UnusedLocalVariableCheck.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to merge
...ls/checkstyle/checks/coding/unusedlocalvariable/InputUnusedLocalVariableSwitchStatement.java
Show resolved
Hide resolved
TokenTypes.ELIST, | ||
TokenTypes.INDEX_OP, | ||
TokenTypes.ASSIGN, | ||
TokenTypes.LITERAL_SWITCH, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why LITERAL_SWITCH
and why not something that makes more sense like EXPR
? A switch is not technically anything used with increments/decrements. At least the other 3 make somewhat more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever the token is of type TokenTypes.ELIST, TokenTypes.INDEX_OP, TokenTypes.ASSIGN,
in that cases the Variable is been used for example
int h = ++m;
the last will be
| | |--VARIABLE_DEF -> VARIABLE_DEF [66:8]
| | | |--MODIFIERS -> MODIFIERS [66:8]
| | | |--TYPE -> TYPE [66:8]
| | | | `--LITERAL_INT -> int [66:8]
| | | |--IDENT -> h [66:12]
| | | `--ASSIGN -> = [66:14]
| | | `--EXPR -> EXPR [66:16]
| | | `--INC -> ++ [66:16]
| | | `--IDENT -> m [66:18]
this is the one case where expr parent is TokenTypes.ASSIGN
know particular in switch
switch (i++) {
the ast will be
| |--LITERAL_SWITCH -> switch [12:8]
| | |--LPAREN -> ( [12:15]
| | |--EXPR -> EXPR [12:17]
| | | `--POST_INC -> ++ [12:17]
| | | `--IDENT -> i [12:16]
| | |--RPAREN -> ) [12:19]
the way to declare the i++
as used I have to put their TokenTypes.LiteralSwitch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and why not something that makes more sense like EXPR?
I am not seeing this specifically answered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically in this function we are trying to find out whether the increment or decrement variable is used or not and in case of TokenTypes.ELIST, TokenTypes.INDEX_OP, TokenTypes.ASSIGN
if this tokens are the parent of expr
then it is used and they are understandable for example again TokenTypes.ASSIGN
int h = ++m;
this case of LiteralSwitch
is slightly unique in general as per the check logic if the increment decrement variable is alone then the variable seems to be unused but in this case, it is not true that's why the TokenTypes.LiteralSwitch
is used in the sense if it is parent then the variable is used as I have explained in above comment.
and I also don't see any way some how only EXPR can be used and we can declare this as used variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fully understanding and this isn't making sense.
Either way, the javadoc needs to be updated for this method for the new token being examined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevin222004 ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnveach sorry for the delay. I will update the pr as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevin222004, ping, please find time to fix this PR. We are almost done. Passing of last reviewer is left only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for such long delay will fix with in 2 days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Issue #14506: False positive for UnusedLocalVariable
Diff Regression config: https://gist.githubusercontent.com/Kevin222004/0dbdfb487f8e9a050ed0e6356f1a35b4/raw/31f7927f400e11e4285e86fd086b373095227848/unusedLocal.xml