SA4017 doesn't flag unused return value of *net/http.Request.WithContext() #69

Closed
bubaflub opened this Issue Jan 4, 2017 · 2 comments

Projects

None yet

2 participants

@bubaflub
bubaflub commented Jan 4, 2017

According to https://golang.org/pkg/net/http/#Request.WithContext:

WithContext returns a shallow copy of r with its context changed to ctx.

I've accidentally used WithContext() thinking it would update the request in-place rather than return a copy. When I found my mistake I was hoping staticcheck would catch this issue, perhaps with SA4017, but it doesn't.

Minimal test case:

package foo

import (
	"bytes"
	"net/http"
)

func bad() {
	url := "http://example.com"
	payload := "this should be flagged by the context checker"
	req, _ := http.NewRequest("POST", url, bytes.NewBufferString(payload))
	req.WithContext(nil)
}
bobs-mac% staticcheck foo
/foo/something.go:12:18: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (SA1012)

I expected to see a SA4017 warning about an unused result from a pure function. Is staticcheck marking *net/http.Request.WithContext() as a pure function? Or am I misunderstanding the nature of the check.

@bubaflub
bubaflub commented Jan 4, 2017

For what it's worth, I have a very rough checker at https://github.com/bubaflub/go-withcontext-checker that finds this issue. But I'd rather not have one-off tools like this.

@dominikh
Owner
dominikh commented Jan 5, 2017

SA4017 works in two separate ways. It has a hard-coded list of pure stdlib functions, and it detects very simple pure functions that consist of a very restricted subset of the Go language.

The list is missing an entry for WithContext, and the automatic detection won't match that method.

I'll add an entry to the list.

@dominikh dominikh added the enhancement label Jan 5, 2017
@dominikh dominikh self-assigned this Jan 5, 2017
@dominikh dominikh closed this in 34cb9e1 Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment