Skip to content
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.

Sign up for GitHub

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

Jump to bottom

proposal: cmd/vet: warn on defer as the last statement before return if the arguments exists prior to the penultimate statement to the return #64987

Closed
Jorropo opened this issue Jan 6, 2024 · 7 comments
Closed

proposal: cmd/vet: warn on defer as the last statement before return if the arguments exists prior to the penultimate statement to the return #64987

Jorropo opened this issue Jan 6, 2024 · 7 comments
Labels
Proposal
Milestone
Proposal

Comments

@Jorropo
Copy link
Member

Jorropo commented Jan 6, 2024 •
edited

Proposal Details

I see regularly new comers using defer incorrectly.
Some write things like this:

func doSomething() {
 resource := Grab()
 err := use(resource)
 if err != nil {
  return
 }
 defer resource.Close()
}

This code is tricky because it works, or at least looks to do so if you don't have a test for the failure case which also checks that resource is cleaned up properly.
So I propose that defers should be a go vet error if they are the last statement before a return as

defer thing()
return

is identical to

thing()
return

Unless one or more of their arguments (this include receivers, function arguments and variables captured by closures) are coming from the penultimate statement, this is so code like this remains ok:

thing := Grab()
defer thing.Close()

return

Ideally this would be counted as per SSA rules so:

thing, err := Grab()
if err != nil {
 return err
}
defer thing.Close()

return nil

would remain ok, we could also replace this with more edge cases.

I see the acquire (+ if err != nil check) + defer close as it's own logical unit, and sometime you open a connection or file just to check that it is reachable or you have correct permissions, but then don't use it (yet).
However it tries to exclude acquire + use + defer close pattern which is a bug.
@gopherbot gopherbot added this to the Proposal milestone Jan 6, 2024
@Jorropo
Copy link
Member Author

Jorropo commented Jan 6, 2024

Falling through control flow should also be counted.
In other words I think this should also be warned on:

if x {
 r := Grab()
 use(r)
 defer r.Close()
}
return

@Jorropo Jorropo changed the title proposal: go/vet: warn on defer as the last statement before return if the arguments exists prior to the penultimate statement to the return proposal: cmd/vet: warn on defer as the last statement before return if the arguments exists prior to the penultimate statement to the return Jan 6, 2024
@thediveo
Copy link

thediveo commented Jan 6, 2024

I'm afraid that this at the same time causes friction especially when writing tests and the tests get fleshed out step by step. In those cases govet will block running the test, even if it is fully correct.

rsc, err := openrsc()
if err ...
defer rsc.Close()
// test test so far, more to come later

@Jorropo
Copy link
Member Author

Jorropo commented Jan 6, 2024

@thediveo I tried covering this with:

Ideally this would be counted as per SSA rules so

It's very unclear (and incorrect).
I guess a more correct analysis could be to do this analysis on the CFG while ignoring control flow.
So if err != nil and it's content would be ignored because your error checking block would finish with a panic, return or other terminating block.
defer rsc.Close() would be the final statement, if would be skipped, rsc, err := openrsc() would be the penultimate statement and this wouldn't match since one of the return values from the penultimate value is an argument to the defer.

@seankhliao
Copy link
Member

how frequent is this that it needs to be a vet check?

@timothy-king
Copy link
Contributor

How does this satisfy vet's 'Correctness' criteria? cmd/vet/README:

Correctness:

Vet's checks are about correctness, not style. A vet check must identify real or
potential bugs that could cause incorrect compilation or execution. A check that
only identifies stylistic points or alternative correct approaches to a situation
is not acceptable.

@go101
Copy link

go101 commented Jan 7, 2024 •
edited

Unless one or more of their arguments (this include receivers, function arguments and variables captured by closures) are coming from the penultimate statement, this is so code like this remains ok:

I don't understand this line. Do you mean whether or not arguments exist will affect the logic? (It certainly doesn't).

@Jorropo
Copy link
Member Author

Jorropo commented Jan 13, 2024

I searched for matches in opensource code and there were a couple too much false positive, I thought it would be pretty rare, but yes this probably falls under styling.

@Jorropo Jorropo closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
6 participants
@timothy-king @thediveo @gopherbot @seankhliao @go101 @Jorropo