diff --git a/.golangci.yml b/.golangci.yml index 8fbfb161..a4448876 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -55,6 +55,7 @@ linters: # inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint disable-all: true enable: + # - rowserrcheck - bodyclose - deadcode - depguard diff --git a/README.md b/README.md index c3e9f669..e424f204 100644 --- a/README.md +++ b/README.md @@ -228,6 +228,7 @@ maligned: Tool to detect Go structs that would take less memory if their fields misspell: Finds commonly misspelled English words in comments [fast: true, auto-fix: true] nakedret: Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false] prealloc: Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false] +rowserrcheck: checks whether Err of rows is checked successfully [fast: true, auto-fix: false] scopelint: Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false] stylecheck: Stylecheck is a replacement for golint [fast: true, auto-fix: false] unconvert: Remove unnecessary type conversions [fast: true, auto-fix: false] @@ -456,6 +457,7 @@ golangci-lint help linters - [bodyclose](https://github.com/timakin/bodyclose) - checks whether HTTP response body is closed successfully - [golint](https://github.com/golang/lint) - Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes +- [rowserrcheck](https://github.com/jingyugao/rowserr) - checks whether Err of rows is checked successfully - [stylecheck](https://github.com/dominikh/go-tools/tree/master/stylecheck) - Stylecheck is a replacement for golint - [gosec](https://github.com/securego/gosec) - Inspects source code for security problems - [interfacer](https://github.com/mvdan/interfacer) - Linter that suggests narrower interface types @@ -983,6 +985,7 @@ linters: # inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint disable-all: true enable: + # - rowserrcheck - bodyclose - deadcode - depguard @@ -1204,6 +1207,7 @@ Thanks to developers and authors of used linters: - [timakin](https://github.com/timakin) - [kisielk](https://github.com/kisielk) - [golang](https://github.com/golang) +- [jingyugao](https://github.com/jingyugao) - [dominikh](https://github.com/dominikh) - [securego](https://github.com/securego) - [opennota](https://github.com/opennota) diff --git a/go.mod b/go.mod index 1743705f..447e062b 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( github.com/golangci/prealloc v0.0.0-20180630174525-215b22d4de21 github.com/golangci/revgrep v0.0.0-20180526074752-d9c87f5ffaf0 github.com/golangci/unconvert v0.0.0-20180507085042-28b1c447d1f4 + github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a github.com/matoous/godox v0.0.0-20190911065817-5d6d842e92eb // v1.0 github.com/mattn/go-colorable v0.1.4 github.com/mitchellh/go-homedir v1.1.0 diff --git a/go.sum b/go.sum index 5ba5f71b..74222108 100644 --- a/go.sum +++ b/go.sum @@ -42,6 +42,8 @@ github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9 github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V4qmtdjCk= github.com/go-ole/go-ole v1.2.1 h1:2lOsA72HgjxAuMlKpFiCbHTvu44PIVkZ5hqm3RSdI/E= github.com/go-ole/go-ole v1.2.1/go.mod h1:7FAglXiTm7HKlQRDeOQ6ZNUHidzCWXuZWq/1dTyBNF8= +github.com/go-sql-driver/mysql v1.4.0 h1:7LxgVwFb2hIQtMm87NdgAVfXjnt4OePseqT1tKx+opk= +github.com/go-sql-driver/mysql v1.4.0/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w= github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= github.com/go-toolsmith/astcast v1.0.0 h1:JojxlmI6STnFVG9yOImLeGREv8W2ocNUM+iOhR6jE7g= github.com/go-toolsmith/astcast v1.0.0/go.mod h1:mt2OdQTeAQcY4DQgPSArJjHCcOwlX+Wl/kwN+LbLGQ4= @@ -128,6 +130,10 @@ github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM= github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= +github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a h1:GmsqmapfzSJkm28dhRoHz2tLRbJmqhU86IPgBtN3mmk= +github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a/go.mod h1:xRskid8CManxVta/ALEhJha/pweKBaVG6fWgc0yH25s= +github.com/jmoiron/sqlx v1.2.1-0.20190826204134-d7d95172beb5 h1:lrdPtrORjGv1HbbEvKWDUAy97mPpFm4B8hp77tcCUJY= +github.com/jmoiron/sqlx v1.2.1-0.20190826204134-d7d95172beb5/go.mod h1:1FEQNm3xlJgrMD+FBdI9+xvCksHtbpVBBw5dYhBSsks= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7C0MuV77Wo= github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= @@ -148,6 +154,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= +github.com/lib/pq v1.2.0 h1:LXpIM/LZ5xGFhOpXAQUIMM1HdyqzVYM13zNdjCEEcA0= github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/logrusorgru/aurora v0.0.0-20181002194514-a7b3b318ed4e/go.mod h1:7rIyQOR62GCctdiQpZ/zOJlFyk6y+94wXzv6RNZgaR4= github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ= @@ -159,6 +167,8 @@ github.com/mattn/go-colorable v0.1.4 h1:snbPLB8fVfU9iwbbo30TPtbLRzwWu6aJS6Xh4eaa github.com/mattn/go-colorable v0.1.4/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE= github.com/mattn/go-isatty v0.0.8 h1:HLtExJ+uU2HOZ+wI0Tt5DtUDrx8yhUqDcp7fYERX4CE= github.com/mattn/go-isatty v0.0.8/go.mod h1:Iq45c/XA43vh69/j3iqttzPXn0bhXyGjM0Hdxcsrc5s= +github.com/mattn/go-sqlite3 v1.9.0 h1:pDRiWfl+++eC2FEFRy6jXmQlvp4Yh3z1MJKg4UeYM/4= +github.com/mattn/go-sqlite3 v1.9.0/go.mod h1:FPy6KqzDD04eiIsT53CuJW3U88zkxoIYsOqkbpncsNc= github.com/mattn/goveralls v0.0.2 h1:7eJB6EqsPhRVxvwEXGnqdO2sJI0PTsrWoTMXEk9/OQc= github.com/mattn/goveralls v0.0.2/go.mod h1:8d1ZMHsd7fW6IRPKQh46F2WRpyib5/X4FOpevwGNQEw= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= @@ -311,6 +321,7 @@ golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +google.golang.org/appengine v1.1.0 h1:igQkv0AAhEIvTEpD5LIpAfav2eeVO9HBTjvKHVJPRSs= google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM= google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= diff --git a/pkg/config/config.go b/pkg/config/config.go index c1db0328..38b2ca58 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -179,6 +179,9 @@ type LintersSettings struct { MultiIf bool `mapstructure:"multi-if"` MultiFunc bool `mapstructure:"multi-func"` } + RowsErrCheck struct { + Packages []string + } WSL WSLSettings Lll LllSettings diff --git a/pkg/golinters/rowerrcheck.go b/pkg/golinters/rowerrcheck.go new file mode 100644 index 00000000..d4c89d38 --- /dev/null +++ b/pkg/golinters/rowerrcheck.go @@ -0,0 +1,23 @@ +package golinters + +import ( + "github.com/jingyugao/rowserrcheck/passes/rowserr" + "golang.org/x/tools/go/analysis" + + "github.com/golangci/golangci-lint/pkg/golinters/goanalysis" + "github.com/golangci/golangci-lint/pkg/lint/linter" +) + +func NewRowsErrCheck() *goanalysis.Linter { + analyzer := rowserr.NewAnalyzer() + return goanalysis.NewLinter( + "rowserrcheck", + "checks whether Err of rows is checked successfully", + []*analysis.Analyzer{analyzer}, + nil, + ).WithLoadMode(goanalysis.LoadModeTypesInfo). + WithContextSetter(func(lintCtx *linter.Context) { + pkgs := lintCtx.Settings().RowsErrCheck.Packages + analyzer.Run = rowserr.NewRun(pkgs...) + }) +} diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 9818b9f5..38c4cbab 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -108,6 +108,10 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithLoadForGoAnalysis(). WithPresets(linter.PresetStyle). WithURL("https://github.com/golang/lint"), + linter.NewConfig(golinters.NewRowsErrCheck()). + WithLoadForGoAnalysis(). + WithPresets(linter.PresetPerformance, linter.PresetBugs). + WithURL("https://github.com/jingyugao/rowserr"), linter.NewConfig(golinters.NewStaticcheck()). WithLoadForGoAnalysis(). diff --git a/test/testdata/rowserrcheck.go b/test/testdata/rowserrcheck.go new file mode 100644 index 00000000..c45064c3 --- /dev/null +++ b/test/testdata/rowserrcheck.go @@ -0,0 +1,13 @@ +//args: -Erowserrcheck +package testdata + +import ( + "database/sql" +) + +func RowsErrNotChecked(db *sql.DB) { + rows, _ := db.Query("select id from tb") // ERROR "rows.Err must be checked" + for rows.Next() { + + } +} diff --git a/vendor/github.com/jingyugao/rowserrcheck/LICENSE b/vendor/github.com/jingyugao/rowserrcheck/LICENSE new file mode 100644 index 00000000..6957f188 --- /dev/null +++ b/vendor/github.com/jingyugao/rowserrcheck/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2019 Seiji Takahashi + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/vendor/github.com/jingyugao/rowserrcheck/passes/rowserr/rowserr.go b/vendor/github.com/jingyugao/rowserrcheck/passes/rowserr/rowserr.go new file mode 100644 index 00000000..7b8c64f4 --- /dev/null +++ b/vendor/github.com/jingyugao/rowserrcheck/passes/rowserr/rowserr.go @@ -0,0 +1,319 @@ +package rowserr + +import ( + "fmt" + "go/ast" + "go/types" + "strconv" + + "github.com/gostaticanalysis/analysisutil" + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" +) + +func NewAnalyzer(sqlPkgs ...string) *analysis.Analyzer { + return &analysis.Analyzer{ + Name: "rowserrcheck", + Doc: Doc, + Run: NewRun(sqlPkgs...), + Requires: []*analysis.Analyzer{ + buildssa.Analyzer, + }, + } +} + +const ( + Doc = "rowserrcheck checks whether Rows.Err is checked" + errMethod = "Err" + rowsName = "Rows" +) + +type runner struct { + pass *analysis.Pass + rowsTyp *types.Pointer + rowsObj types.Object + skipFile map[*ast.File]bool + sqlPkgs []string +} + +func NewRun(pkgs ...string) func(pass *analysis.Pass) (interface{}, error) { + return func(pass *analysis.Pass) (interface{}, error) { + pkgs = append(pkgs, "database/sql") + for _, pkg := range pkgs { + r := new(runner) + r.sqlPkgs = pkgs + r.run(pass, pkg) + } + return nil, nil + } +} + +// run executes an analysis for the pass. The receiver is passed +// by value because this func is called in parallel for different passes. +func (r runner) run(pass *analysis.Pass, pkgPath string) (interface{}, error) { + r.pass = pass + pssa := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) + funcs := pssa.SrcFuncs + + pkg := pssa.Pkg.Prog.ImportedPackage(pkgPath) + if pkg == nil { + // skip + return nil, nil + } + + r.rowsObj = pkg.Type(rowsName).Object() + if r.rowsObj == nil { + // skip checking + return nil, nil + } + + resNamed, ok := r.rowsObj.Type().(*types.Named) + if !ok { + return nil, nil + } + + r.rowsTyp = types.NewPointer(resNamed) + r.skipFile = map[*ast.File]bool{} + + for _, f := range funcs { + if r.noImportedDBSQL(f) { + // skip this + continue + } + + // skip if the function is just referenced + var isreffunc bool + + for i := 0; i < f.Signature.Results().Len(); i++ { + if types.Identical(f.Signature.Results().At(i).Type(), r.rowsTyp) { + isreffunc = true + } + } + + if isreffunc { + continue + } + + for _, b := range f.Blocks { + for i := range b.Instrs { + if r.notCheck(b, i) { + pass.Reportf(b.Instrs[i].Pos(), fmt.Sprintf("rows.Err must be checked")) + } + } + } + } + + return nil, nil +} + +func (r *runner) notCheck(b *ssa.BasicBlock, i int) bool { + call, ok := r.getReqCall(b.Instrs[i]) + if !ok { + return false + } + + for _, cRef := range *call.Referrers() { + val, ok := r.getResVal(cRef) + if !ok { + continue + } + + if len(*val.Referrers()) == 0 { + return true + } + + resRefs := *val.Referrers() + for _, resRef := range resRefs { + switch resRef := resRef.(type) { + case *ssa.Store: // Call in Closure function + if len(*resRef.Addr.Referrers()) == 0 { + return true + } + + for _, aref := range *resRef.Addr.Referrers() { + if c, ok := aref.(*ssa.MakeClosure); ok { + f := c.Fn.(*ssa.Function) + if r.noImportedDBSQL(f) { + // skip this + return false + } + called := r.isClosureCalled(c) + + return r.calledInFunc(f, called) + } + + } + case *ssa.Call: // Indirect function call + if r.isCloseCall(resRef) { + return false + } + if f, ok := resRef.Call.Value.(*ssa.Function); ok { + for _, b := range f.Blocks { + for i := range b.Instrs { + return r.notCheck(b, i) + } + } + } + case *ssa.FieldAddr: + for _, bRef := range *resRef.Referrers() { + bOp, ok := r.getBodyOp(bRef) + if !ok { + continue + } + + for _, ccall := range *bOp.Referrers() { + if r.isCloseCall(ccall) { + return false + } + } + } + } + } + } + + return true +} + +func (r *runner) getReqCall(instr ssa.Instruction) (*ssa.Call, bool) { + call, ok := instr.(*ssa.Call) + if !ok { + return nil, false + } + + res := call.Call.Signature().Results() + flag := false + + for i := 0; i < res.Len(); i++ { + flag = flag || types.Identical(res.At(i).Type(), r.rowsTyp) + } + + if !flag { + return nil, false + } + + return call, true +} + +func (r *runner) getResVal(instr ssa.Instruction) (ssa.Value, bool) { + switch instr := instr.(type) { + case *ssa.Call: + if len(instr.Call.Args) == 1 && types.Identical(instr.Call.Args[0].Type(), r.rowsTyp) { + return instr.Call.Args[0], true + } + case ssa.Value: + if types.Identical(instr.Type(), r.rowsTyp) { + return instr, true + } + } + + return nil, false +} + +func (r *runner) getBodyOp(instr ssa.Instruction) (*ssa.UnOp, bool) { + op, ok := instr.(*ssa.UnOp) + if !ok { + return nil, false + } + // fix: try to check type + // if op.Type() != r.rowsObj.Type() { + // return nil, false + // } + return op, true +} + +func (r *runner) isCloseCall(ccall ssa.Instruction) bool { + switch ccall := ccall.(type) { + case *ssa.Defer: + if ccall.Call.Value != nil && ccall.Call.Value.Name() == errMethod { + return true + } + case *ssa.Call: + if ccall.Call.Value != nil && ccall.Call.Value.Name() == errMethod { + return true + } + } + + return false +} + +func (r *runner) isClosureCalled(c *ssa.MakeClosure) bool { + for _, ref := range *c.Referrers() { + switch ref.(type) { + case *ssa.Call, *ssa.Defer: + return true + } + } + + return false +} + +func (r *runner) noImportedDBSQL(f *ssa.Function) (ret bool) { + obj := f.Object() + if obj == nil { + return false + } + + file := analysisutil.File(r.pass, obj.Pos()) + if file == nil { + return false + } + + if skip, has := r.skipFile[file]; has { + return skip + } + defer func() { + r.skipFile[file] = ret + }() + + for _, impt := range file.Imports { + path, err := strconv.Unquote(impt.Path.Value) + if err != nil { + continue + } + path = analysisutil.RemoveVendor(path) + for _, pkg := range r.sqlPkgs { + if pkg == path { + return false + } + } + } + + return true +} + +func (r *runner) calledInFunc(f *ssa.Function, called bool) bool { + for _, b := range f.Blocks { + for i, instr := range b.Instrs { + switch instr := instr.(type) { + case *ssa.UnOp: + for _, ref := range *instr.Referrers() { + if v, ok := ref.(ssa.Value); ok { + if vCall, ok := v.(*ssa.Call); ok { + if vCall.Call.Value != nil && vCall.Call.Value.Name() == errMethod { + if called { + return false + } + } + } + } + } + default: + if r.notCheck(b, i) || !called { + return true + } + } + } + } + return true +} + +// isNamedType reports whether t is the named type path.name. +func isNamedType(t types.Type, path, name string) bool { + n, ok := t.(*types.Named) + if !ok { + return false + } + obj := n.Obj() + return obj.Name() == name && obj.Pkg() != nil && obj.Pkg().Path() == path +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 9482448c..05d6565d 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -99,6 +99,8 @@ github.com/hashicorp/hcl/json/scanner github.com/hashicorp/hcl/json/token # github.com/inconshreveable/mousetrap v1.0.0 github.com/inconshreveable/mousetrap +# github.com/jingyugao/rowserrcheck v0.0.0-20191204022205-72ab7603b68a +github.com/jingyugao/rowserrcheck/passes/rowserr # github.com/kisielk/gotool v1.0.0 github.com/kisielk/gotool github.com/kisielk/gotool/internal/load