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.

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

proposal: cmd/vet: warn about recover inside non-deferred function #64116

Closed
ghost opened this issue Nov 14, 2023 · 9 comments
Closed

proposal: cmd/vet: warn about recover inside non-deferred function #64116

ghost opened this issue Nov 14, 2023 · 9 comments
Labels
Milestone

Comments

@ghost
Copy link

ghost commented Nov 14, 2023

func outer() {
    go func() {
        if r := recover(); r != nil { ... }
    }()
}

Here, the go keyword is mistakenly used instead of the defer keyword. It would be lovely if vet warned about this case.

@ghost ghost added the Proposal label Nov 14, 2023
@gopherbot gopherbot added this to the Proposal milestone Nov 14, 2023
@cherrymui
Copy link
Member

This may be hard in general. It is true that recover is only useful in a deferred function, but that deferred function doesn't need to be a function literal. It can be a top-level function, like https://go.dev/play/p/YjT5mHT0wLl . The function definition and the defer statement can be far from each other, even in different packages. Or it could be a dynamic func value.

@timothy-king
Copy link
Contributor

@cherrymui's point is well taken. Solving this is roughly the same as a pointer/may-escape analysis in the limit. This example will be much harder:

func harder() {
    f := func() {
        if r := recover(); r != nil { ... }
    }
    go f()
}

func evenHarderA(){
    f := func() {
        if r := recover(); r != nil { ... }
    }
    evenHarderB(f)
}
func evenHarderB(f func()) {
    go f()
}

The reason is that a warning should probably only be given if a recover() cannot be directly called by some defer statement. So off the bat, exported functions cannot be warned on in vet. Some cases should not be too hard though, like #64116 (comment) , or unexported functions that are never stored. FWIW something useful would be simpler in x/tools/go/ssa.

I do not have a good sense for how frequent these issues are. Getting a good grasp might be hard without an Analyzer implementation to test.

@findleyr
Copy link
Contributor

I also don't have any intuition for whether this would meet the "frequency" requirement for cmd/vet. @justpretending have you seen this frequently in practice?

@adonovan
Copy link
Member

In the absence of data supporting that this is a significant problem I think we should decline this.

@rsc
Copy link
Contributor

rsc commented Jan 10, 2024

In the absence of evidence that this is a common mistake it seems like this is headed for likely decline.

@rsc rsc changed the title proposal: cmd/vet: warn about recover() being used inside a non-deferred function proposal: cmd/vet: warn about recover inside non-deferred function Jan 10, 2024
@rsc
Copy link
Contributor

rsc commented Jan 10, 2024

In general literally the only time we could issue a definitive positive would be 'go func() { ... }()'. There's no evidence people accidentally write defer instead of go, and even if they did, we'd only catch it when they happened to call recover.

@rsc
Copy link
Contributor

rsc commented Jan 10, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jan 19, 2024

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jan 26, 2024

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

6 participants