checkers: rewrite deferUnlambda checker as a StmtListVisitor
#1406
Conversation
- fix assigned closure function false-positive issue deferUnlambda: False positive for mutated variable #1401
|
There are failures in |
95123ff to
1f541c7
Compare
|
Addressed the issues. |
- fix assigned closure function false-positive issue go-critic#1401
|
Sorry for the delay, I will take a look tomorrow. Thanks. |
| }() | ||
| foo = func() { | ||
| println("should print") | ||
| } |
There was a problem hiding this comment.
Based on the implementation, I guess it wouldn't "see" the assignment anywhere except the same depth level.
// This assignment inside of the if statement will not be handled.
if cond {
foo = func() {
println("should print")
}
}The proper analysis is cumbersome at AST level. This is probably one of the cases where SSA would be beneficial.
There was a problem hiding this comment.
Thank you for your advice. Since this is the first time I work with AST, I'll see if I could figure out a proper solution based on what you mentioned.
| continue | ||
| } | ||
|
|
||
| if ident.Name == target.Name { |
There was a problem hiding this comment.
It's better to check the "objects" for identity, so the shadowing doesn't break anything.
But for a single-level statement scanning name check is probably enough, depending on whether you want to make it recursive or not.
|
I've read a number of other checkers since last week and yet I couldn't find any similar situation to begin with. I also inspected the context using a debugger in which lays several utilities, however none of them helped me with the case. Could you possibly provide me with some reference, doc or code to help me find my way? Talking about accessing "objects" and utilizing SSA approach. Of course there is always a way to implement something, but given that my little code is already messy and unreadable, I don't feel good about shooting in the dark. |
|
I will make a review today, thank you for the update. |
| if callFuncIdent != nil { | ||
| return callFuncIdent, callExpr, c.allCallArgsAreConst(callExpr.Args) | ||
| } | ||
| return nil, nil, false |
There was a problem hiding this comment.
slightly better in this context:
| if callFuncIdent != nil { | |
| return callFuncIdent, callExpr, c.allCallArgsAreConst(callExpr.Args) | |
| } | |
| return nil, nil, false | |
| if callFuncIdent == nil { | |
| return nil, nil, false | |
| } | |
| return callFuncIdent, callExpr, c.allCallArgsAreConst(callExpr.Args) |