http related false positives after "skip import check" commit

[scop]

After commit a2c3e11, v1.0.8, I'm getting reports that I think are false positives.

There's an underlying function

func WriteProblem(w http.ResponseWriter, r *http.Request, problem *Problem, originalError error) error

...which calls r.Context(), and another function that calls it (but not r.Context() directly) like

func writeProblemResponse(w http.ResponseWriter, r *http.Request, err error)

Now, every WriteProblem and writeProblemResponse call gets flagged as "should pass the context parameter". I suppose the intent is not to do that. r.Context() is used where a Context can be used.

v1.0.7 did not have this problem.

Also, for reasons I cannot explain, I'm also seeing this problem with golangci-lint v1.49.0, which uses contextcheck v1.0.6. But I cannot replicate the problem with contextcheck v1.0.6 directly 🤔

[kkHAIKE]

plz add more code to me for test 😊

[scop]

Can do later. In the meantime, can you confirm that the intent is not to warn about the cases above, i.e. when a http.Request is passed, and its .Context() is used, no warning should be emitted if another Context is not passed?

[kkHAIKE]

this case is not false-positive in current design. 😊

cuz http.Request can't determine whether it is server side, so i focus on the func sign only: func(w http.ResponseWriter, r *http.Request).

u can change these underlying function sign that add Context paramter like this: func writeProblemResponse(ctx context.Context, w http.ResponseWriter, r *http.Request, err error)

eg:

func someHandler(w http.ResponseWriter, r *http.Request) {
  ctx := r.Context()
  if err := doAnyThine(ctx, w, r); err != nil {
    writeProblemResponse(ctx, w, r, err)
  }
}

[scop]

I considered changing the signatures already, but doing so would be possibly misleading and just to appease contextcheck, so I decided against it.

Would it not be ok to change the logic here so that if a function takes both http.Request and http.ResponseWriter, then it must be a server side one, no matter what the rest of the signature is? http.ResponseWriter should not occur in client side code.

[kkHAIKE]

i think increase the HandlerFunc sign range may get more false-positive. maybe have proxy case? (req is a client side, copy resp to W)

so i add a nolint comment 7be313c for skip false-positive case. i think it's a necessary feature. 😊

u can add // nolint: contextcheck to WriteProblem and writeProblemResponse.

[scop]

For my use case the addition of //nolint:contextcheck is unnecessary here, because the exactly same can be done already through golangci-lint.

In absence of changing the default/builtin behavior, I would find useful an option (command line one as well as one that can be set by golangci-lint internally) that would make contextcheck not issue any warnings for functions that take a http.Request. For example, -assumehttprequestcontext: assume functions taking a http.Request will use its context and do not require another one

[kkHAIKE]

new version can mark it automatically.

---

add new tag '// @contextcheck(req_has_ctx)' for this case