New linter: Go responsewriter lint

[javorszky]

Issue I see sometimes is that the order in which calls made to responsewriter end up with unexpected bug for those who don't have the side effects of calling Write, WriteHeader memorized in the http ResponseWriter interface.

This linter should flag up potential places that might lead to bugs by finding calls that are in the wrong order, for example:

• WriteHeader called after Write. By that time, Write has already called WriteHeader with an http.StatusOK, so additional call to WriteHeader does nothing
• Header called after either Write or WriteHeader: headers are already sent, manipulating headers after that will take no effect
• multiple calls to Write or WriteHeader are probably not intended in the same function body, so those are also probably bugs

Linter repository

https://github.com/javorszky/go-responsewriter-lint

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[javorszky]

I'm having some issue running the tests that I can't seem to resolve, or even find the cause of:

go run ./cmd/golangci-lint/ run --no-config --disable-all --enable=responsewriterlint ./test/testdata/responsewriterlint.go
test/testdata/responsewriterlint.go:5:2: could not import errors (Config.Importer.Import(errors) returned nil but no error) (typecheck)
	"errors"
	^
test/testdata/responsewriterlint.go:6:2: could not import fmt (Config.Importer.Import(fmt) returned nil but no error) (typecheck)
	"fmt"
	^
test/testdata/responsewriterlint.go:7:2: could not import net/http (Config.Importer.Import(net/http) returned nil but no error) (typecheck)
	"net/http"
	^
exit status 1

UPDATE: this was fixed by using the WithLoadForGoAnalysis method in the loader

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), log.fatal(), os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

• The linter should not use SSA. (currently, SSA does not support generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[alexandear]

Can someone explain to me why this code generates the responsewriter-lint issue?

func serveDepsJS(rw http.ResponseWriter, req *http.Request, dir string) {
	...
	b, err := closure.GenDeps(root)
	if err != nil {
		log.Print(err)
		http.Error(rw, "Server error", 500)
		return
	}
	rw.Header().Set("Content-Type", "text/javascript; charset=utf-8")
	rw.Write([]byte("// auto-generated from perkeepd\n"))
	rw.Write(b)
}

github.com/perkeep/perkeep/pkg/server/ui.go:700:5: function serveDepsJS: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.
github.com/perkeep/perkeep/pkg/server/ui.go:701:5: function serveDepsJS: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.

[javorszky]

@alexandear I'm unsure which one is your question:

• why is the responsewriter-lint linting code it shouldn't be linting? As far as I understand the way golangci-lint and its own testing, each linter should only deal with its own testdata files, but not the others, so I can't answer that. I don't even know if my assumption here is correct or not Misunderstood, that's in a different codebase, not a different linter. You seem to be using the version of golangci-lint that's in this branch with everything enabled? This linter is not in yet, there are some issues I need to work through it. It's probably best not to use this until such time that I get this into a position where it would be accepted and merged into it, and even then only turned on if you are okay with what it's testing.

The PR itself has a link to the linter's own repository, where the readme has info on what it aims to do and why. I wrote it such that if I was in a situation where I wanted to find out what it's for, the readme would answer it.

Let me know if you have additional questions.

[alexandear]

@javorszky, I apologize if my question comes across as rude - as a non-native speaker, I want to make sure I understand how this linter can be helpful in finding bugs.

I'm checking the linter requirement It must not have false positives/negatives. from the comment above.

What I did:

• Download and install linter by running the command (this is a standard way to install Go programs, not every system has make, e.g. Windows):

go install github.com/javorszky/go-responsewriter-lint/cmd/responsewriter-lint@latest

• Run the linter on some big repo, e.g. perkeep:

responsewriter-lint ./...

perkeep was created by the former [maintainer] of Go's net/http standard library, and therefore, it's very unlikely there are bugs with http.ResponseWriter.

• Get results, a false positives:

/Users/redko.o/src/github.com/perkeep/perkeep/internal/httputil/httputil.go:85:5: function ReturnJSONCode: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.
/Users/redko.o/src/github.com/perkeep/perkeep/internal/httputil/httputil.go:86:5: function ReturnJSONCode: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.
/Users/redko.o/src/github.com/perkeep/perkeep/pkg/server/ui.go:700:5: function serveDepsJS: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.
/Users/redko.o/src/github.com/perkeep/perkeep/pkg/server/ui.go:701:5: function serveDepsJS: Multiple calls to http.ResponseWriter.Write in the same function body. This is most probably a bug.

My understanding is that when you created the PR to add responsewriter-lint to golangci-lint, the linter was considered complete and ready for production use.

I am curious to know if you have analyzed any open-source projects with responsewriter-lint and found http.ResponseWriter bugs. If so, could you provide links to the results? The examples in the testdata folder seem artificial and may not represent real-world scenarios.

[javorszky]

@alexandear that context makes a lot more sense, thank you. When I made the linter I followed this tutorial: https://disaev.me/p/writing-useful-go-analysis-linter/#run-on-the-real-and-large-repositories, and ran the linter on the Go source (1.19.2 at the time). That one did not flag up any issues.

With false positives / negatives, I agree with you. In this case I don't really count it as a false positive:

• does it flag up those two calls to Write in the same function? yes
• do I expect the linter to flag them up? also yes
• is this a bug in the code with the two Write calls? probably not

I wrote in the description that there are times when multiple calls or out of order calls to those methods are desirable. The purpose of this linter is to get developers to be purposeful in calling them out of order or multiple times.

As an example, there's the gochecknoinit linter which flags up code that has an init function that introduces side effects. A fair number of database drivers rely on there being init functions to work. I don't think that linter complaining about the init functions in those libraries would be false positive either.

[javorszky]

The tests are what I used to validate the linter so that it triggers when I want it to, and does not trigger when I do not want it to. It's copy pasted from the linter's own repository. Tests there pass. Testing is written as per the tutorial.

It seems like the way golangci-lint wraps the analysis tests is somewhat different. Does it bail on the first found issue?

I would like some help with figuring out the differences between my own testing, and golangci-lint's way of doing it.

[javorszky]

Good point in adding instructions / blurb to installation methods. I do not have access to a Windows computer, but will do my best to accommodate users of other OSs.

[nightlyone]

Maybe the flagging of multiple successive Write calls should be made optional and default to off. That would allow zero false positives and still allow to enable that check.

Multiple write calls are common in API responses that stream data and want to send out some initial data as soon as possible (e.g. a. CSV header in a CSV response). I would not like to flag all these in the future to make a linter happy.