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

staticcheck: flag defer foo() where foo returns a closure #466

Open
dominikh opened this issue May 4, 2019 · 4 comments · May be fixed by #1543
Open

staticcheck: flag defer foo() where foo returns a closure #466

dominikh opened this issue May 4, 2019 · 4 comments · May be fixed by #1543
Labels
aggressive A set of checks that is more prone to false positives but is helpful during code review new-check

Comments

@dominikh
Copy link
Owner

dominikh commented May 4, 2019

Inspired by golang/lint#451

Obviously concerns about false positives exist. This check will need a thorough test run on the Go corpus.

@dgryski
Copy link

dgryski commented May 4, 2019

In what situation would you create a closure and not call it. Note that with no assignment to the closure you've lost access to the return value, so it would need to have modified state somewhere. In that case, the code is horrible and should be flagged anyway :)

@dominikh
Copy link
Owner Author

dominikh commented May 4, 2019

In what situation would you create a closure and not call it

Maybe a function does some work but also returns a closure that is optional to call to do some other work.

I'm not saying that it's great code or that it should exist, but given enough programmers with typewriters, all code exists :)

@larytet
Copy link

larytet commented May 5, 2019

A function changing a global state and returning a "cancel" has a right to exist. I would still like to see a warning. If the static analyses can deduct that the function does not modify global variables I would probably mark the code as an error.

@fortytw2
Copy link
Contributor

fortytw2 commented May 6, 2019

https://godoc.org/github.com/fortytw2/leaktest#Check is a good example of a very easy to make error that this check would catch.

This form of code is generally useful when you want to do something at the start and end of a function without worrying about it... like defer timeThisFunction()(), the first call is immediately executed and can run some code, then still have access to those values at the end of it.

@dominikh dominikh added the aggressive A set of checks that is more prone to false positives but is helpful during code review label Nov 6, 2021
@dominikh dominikh added this to the Staticcheck 2022.2 milestone Jan 12, 2022
arp242 added a commit to arp242/go-tools that referenced this issue May 25, 2024
Add a new checker to flag cases where a deferred function returns
exactly one function, and that function isn't called. For example:

	func f() func() {
		// Do stuff.
		return func() {
			// Do stuff
		}
	}

	func main() {
		defer f() // Error: should be f()()
	}

As mentioned in dominikh#466, there's a change for false positives. I ran the
check on Go stdlib, all my own code, and about 200 popular Go packages,
and haven't found any false positives (and one match:
grpc/grpc-go#7270)

Fixes dominikh#466
arp242 added a commit to arp242/go-tools that referenced this issue May 25, 2024
Add a new checker to flag cases where a deferred function returns
exactly one function, and that function isn't called. For example:

	func f() func() {
		// Do stuff.
		return func() {
			// Do stuff
		}
	}

	func main() {
		defer f() // Error: should be f()()
	}

As mentioned in dominikh#466, there's a change for false positives. I ran the
check on Go stdlib, all my own code, and about 200 popular Go packages,
and haven't found any false positives (and one match:
grpc/grpc-go#7270)

Fixes dominikh#466
@arp242 arp242 linked a pull request May 25, 2024 that will close this issue
arp242 added a commit to arp242/go-tools that referenced this issue Jun 1, 2024
Add a new checker to flag cases where a deferred function returns
exactly one function, and that function isn't called. For example:

	func f() func() {
		// Do stuff.
		return func() {
			// Do stuff
		}
	}

	func main() {
		defer f() // Error: should be f()()
	}

As mentioned in dominikh#466, there's a change for false positives. I ran the
check on Go stdlib, all my own code, and about 200 popular Go packages,
and haven't found any false positives (and one match:
grpc/grpc-go#7270)

Fixes dominikh#466
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aggressive A set of checks that is more prone to false positives but is helpful during code review new-check
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants