Only assume named returns can be read via a panic if there was a defer.#61
Only assume named returns can be read via a panic if there was a defer.#61kyegupov wants to merge 1 commit intogordonklaus:masterfrom
Conversation
This tightens the analysis which has become too lenient after fixing gordonklaus#22. A possibility of panic can only lead to named returns being read if there is a "defer" statement which could call recover().
gordonklaus
left a comment
gordonklaus
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left some comments that need to be addressed before I can merge.
| @@ -1,5 +1,7 @@ | |||
| package ineffassign | |||
|
|
|||
| // adapted from https://github.com/gordonklaus/ineffassign/blob/2e10b26642541670df2672e035b2de19fcb04cab/pkg/ineffassign/ineffassign.go | |||
| roots []*block | ||
| block *block | ||
| vars map[*ast.Object]*variable | ||
| returnsStack []funcReturnSpec |
| for _, c0 := range n.Body.List { | ||
| c := c0.(*ast.CommClause).Comm |
| v.fundept++ | ||
| } | ||
| bld.results = append(bld.results, typ.Results) | ||
| bld.returnsStack = append(bld.returnsStack, funcReturnSpec{results: typ.Results, hasDefer: false}) |
There was a problem hiding this comment.
omit hasDefer (zero value is false)
| lastIdx := len(bld.returnsStack) - 1 | ||
| bld.returnsStack[lastIdx] = funcReturnSpec{ | ||
| results: bld.returnsStack[lastIdx].results, | ||
| hasDefer: true, | ||
| } |
There was a problem hiding this comment.
rewrite these lines as just bld.returns[len(bld.returns)-1].hasDefer = true
| case *ast.TypeAssertExpr: | ||
| bld.maybePanic() | ||
| return bld | ||
| case *ast.DeferStmt: |
There was a problem hiding this comment.
A DeferStmt should have a little more special treatment. It has a CallExpr, but our handling of CallExpr (above) calls bld.maybePanic. But a defer statement doesn't actually call the function and cannot thusly panic, so this treatment is incorrect. We should call:
bld.walk(n.Call.Fun)
for _, x := range n.Call.Args {
bld.walk(x)
}
before setting hasDefer = true and then do not call return bld after.
Please also add a couple of tests to ensure that named results are not marked as used just because there are defer statements. One test with one defer statement and one with two defers (and no maybe-panicking statements) should suffice.
This isn't a regression so it's not strictly necessary, in case you want to skip it. But since you're tightening the analysis I thought you might want to include this.
There was a problem hiding this comment.
Also, please move this case block up after case *ast.BranchStmt: to follow my rough code organization ;)
|
|
||
| // An operation that might panic marks named function results as used. | ||
| // An operation that might panic marks named function results as used, if the function has a "defer" statement | ||
| // (and thus can potentially recover); see https://github.com/gordonklaus/ineffassign/issues/22 |
There was a problem hiding this comment.
no need to mention this issue since we're going to resolve it :)
| defer func() { | ||
| recover() | ||
| }() |
There was a problem hiding this comment.
you can just write defer recover() if you're into the whole brevity thing
3 participants
This tightens the analysis which has become too lenient after fixing #22.
A possibility of panic can only lead to named returns being read if there is a "defer" statement which could call recover().