diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e7707f..69c31b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## [Unreleased] ### Added: +- new rule unclosedResource - new rule syncPoolNonPtr ### Changed: diff --git a/Makefile b/Makefile index 6f6e8bd..5457686 100644 --- a/Makefile +++ b/Makefile @@ -4,11 +4,9 @@ test: go test --count=1 -race . lint: - @echo "Running golangci-lint..." - @golangci-lint run --skip-dirs testdata --config=.golangci.yml + ruleguard -rules=rules.go ./... ci-lint: install-linter lint install-linter: - @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.44.0 - @$(GOPATH_DIR)/bin/golangci-lint run -v + @go install github.com/quasilyte/go-ruleguard/cmd/ruleguard@cb19258d2ade88dbf466420bb4585dc747bcec57 diff --git a/go.mod b/go.mod index 24e4fb2..9d6ad47 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module github.com/delivery-club/delivery-club-rules go 1.17 require ( - github.com/quasilyte/go-ruleguard v0.3.16-0.20220120183406-c57998eb544d - github.com/quasilyte/go-ruleguard/dsl v0.3.15 + github.com/quasilyte/go-ruleguard v0.3.16-0.20220202142729-cb19258d2ade + github.com/quasilyte/go-ruleguard/dsl v0.3.16 golang.org/x/tools v0.1.9-0.20211228192929-ee1ca4ffc4da ) @@ -12,6 +12,7 @@ require ( github.com/go-toolsmith/astcopy v1.0.0 // indirect github.com/go-toolsmith/astequal v1.0.1 // indirect github.com/quasilyte/gogrep v0.0.0-20220120141003-628d8b3623b5 // indirect + github.com/quasilyte/stdinfo v0.0.0-20220114132959-f7386bf02567 // indirect golang.org/x/mod v0.5.1 // indirect golang.org/x/sys v0.0.0-20211019181941-9d821ace8654 // indirect golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect diff --git a/go.sum b/go.sum index af22510..c4decd5 100644 --- a/go.sum +++ b/go.sum @@ -9,15 +9,17 @@ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/quasilyte/go-ruleguard v0.3.1-0.20210203134552-1b5a410e1cc8/go.mod h1:KsAh3x0e7Fkpgs+Q9pNLS5XpFSvYCEVl5gP9Pp1xp30= -github.com/quasilyte/go-ruleguard v0.3.16-0.20220120183406-c57998eb544d h1:S22nV+Hs92FQsvM7fL9SoGt6tsmeBUjRY5OaKMA5mzE= -github.com/quasilyte/go-ruleguard v0.3.16-0.20220120183406-c57998eb544d/go.mod h1:LeEyOmPzEUSWMOUnTmyMVQe9SOxaG0CeiletX8qcleY= +github.com/quasilyte/go-ruleguard v0.3.16-0.20220202142729-cb19258d2ade h1:G3m+kdJMAKQX7Xg30c52uuL3SF/bDKPB3vRVmEugsgg= +github.com/quasilyte/go-ruleguard v0.3.16-0.20220202142729-cb19258d2ade/go.mod h1:VMX+OnnSw4LicdiEGtRSD/1X8kW7GuEscjYNr4cOIT4= github.com/quasilyte/go-ruleguard/dsl v0.3.0/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU= -github.com/quasilyte/go-ruleguard/dsl v0.3.15 h1:rClYn6lk8wUV5kXnQG4JVsRQjZhSetaNtwml5wkFp5g= -github.com/quasilyte/go-ruleguard/dsl v0.3.15/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU= +github.com/quasilyte/go-ruleguard/dsl v0.3.16 h1:yJtIpd4oyNS+/c/gKqxNwoGO9+lPOsy1A4BzKjJRcrI= +github.com/quasilyte/go-ruleguard/dsl v0.3.16/go.mod h1:KeCP03KrjuSO0H1kTuZQCWlQPulDV6YMIXmpQss17rU= github.com/quasilyte/go-ruleguard/rules v0.0.0-20201231183845-9e62ed36efe1/go.mod h1:7JTjp89EGyU1d6XfBiXihJNG37wB2VRkd125Q1u7Plc= github.com/quasilyte/go-ruleguard/rules v0.0.0-20211022131956-028d6511ab71/go.mod h1:4cgAphtvu7Ftv7vOT2ZOYhC6CvBxZixcasr8qIOTA50= github.com/quasilyte/gogrep v0.0.0-20220120141003-628d8b3623b5 h1:PDWGei+Rf2bBiuZIbZmM20J2ftEy9IeUCHA8HbQqed8= github.com/quasilyte/gogrep v0.0.0-20220120141003-628d8b3623b5/go.mod h1:wSEyW6O61xRV6zb6My3HxrQ5/8ke7NE2OayqCHa3xRM= +github.com/quasilyte/stdinfo v0.0.0-20220114132959-f7386bf02567 h1:M8mH9eK4OUR4lu7Gd+PU1fV2/qnDNfzT635KRSObncs= +github.com/quasilyte/stdinfo v0.0.0-20220114132959-f7386bf02567/go.mod h1:DWNGW8A4Y+GyBgPuaQJuWiy0XYftx4Xm/y5Jqk9I6VQ= github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.4.1/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= diff --git a/rules.go b/rules.go index 925e275..37b91f3 100644 --- a/rules.go +++ b/rules.go @@ -310,6 +310,29 @@ func regexpCompileInLoop(m dsl.Matcher) { Report(`don't compile regex in the loop, move it outside of the loop`) } +func unclosedResource(m dsl.Matcher) { + varEscapeFunction := func(x dsl.Var) bool { + return x.Contains(`$_($*_, $res, $*_)`) || x.Contains(`$_{$*_, $res, $*_}`) || + x.Contains(`$_{$*_, $_: $res, $*_}`) || x.Contains(`$_ <- $res`) || + x.Contains(`return $*_, $res, $*_`) || x.Contains(`$_[$_] = $res`) || + x.Contains(`$_[$res] = $_`) + } + + m.Match(`$res, $err := $open($*_); $*body`, + `$res, $err = $open($*_); $*body`, + `var $res, $err = $open($*_); $*body`, + ). + Where( + m["res"].Type.Implements(`io.Closer`) && + !m["res"].Object.IsGlobal() && + m["err"].Type.Implements(`error`) && + !m["body"].Contains(`$res.Close()`) && + !varEscapeFunction(m["body"]), + ). + Report(`$res.Close() should be deferred right after the $open error check`). + At(m["res"]) +} + func simplifyErrorCheck(m dsl.Matcher) { m.Match(`$err := $f($*args); if $err != nil { $*body }`). Where(m["err"].Type.Implements("error") && @@ -336,8 +359,8 @@ func simplifyErrorCheck(m dsl.Matcher) { func syncPoolNonPtr(m dsl.Matcher) { isPointer := func(x dsl.Var) bool { return x.Type.Underlying().Is("*$_") || x.Type.Underlying().Is("chan $_") || - x.Type.Underlying().Is("map[$_]$_") || x.Type.Underlying().Is("interface{$*_}") || - x.Type.Underlying().Is(`func($*_) $*_`) || x.Type.Underlying().Is(`unsafe.Pointer`) + x.Type.Underlying().Is("map[$_]$_") || x.Type.Underlying().Is("interface{$*_}") || + x.Type.Underlying().Is(`func($*_) $*_`) || x.Type.Underlying().Is(`unsafe.Pointer`) } m.Match(`$x.Put($y)`). diff --git a/testdata/src/queryWithoutContext/sqlstd.go b/testdata/src/queryWithoutContext/sqlstd.go index 10e1b15..38534b2 100644 --- a/testdata/src/queryWithoutContext/sqlstd.go +++ b/testdata/src/queryWithoutContext/sqlstd.go @@ -15,6 +15,8 @@ type decoratorWithParams struct { func warnings() { db, _ := sql.Open("PostgreSQL", "test") + defer db.Close() + db.Query(`SELECT 1`) // want `don't send query to external storage without context` db.QueryRow(`SELECT 1`) // want `don't send query to external storage without context` db.Exec(`SELECT 1`) // want `don't send query to external storage without context` @@ -48,9 +50,11 @@ func warnings() { tx.Stmt(nil) // want `don't send query to external storage without context` stmt, _ := db.Prepare(query) // want `don't send query to external storage without context` - stmt.Query(query) // want `don't send query to external storage without context` - stmt.Exec(query) // want `don't send query to external storage without context` - stmt.QueryRow(query) // want `don't send query to external storage without context` + defer stmt.Close() + + stmt.Query(query) // want `don't send query to external storage without context` + stmt.Exec(query) // want `don't send query to external storage without context` + stmt.QueryRow(query) // want `don't send query to external storage without context` db.ExecContext(context.Background(), query) tx.StmtContext(context.Background(), nil) diff --git a/testdata/src/unclosedResource/negative.go b/testdata/src/unclosedResource/negative.go new file mode 100644 index 0000000..9738f76 --- /dev/null +++ b/testdata/src/unclosedResource/negative.go @@ -0,0 +1,127 @@ +package unclosedResource + +import ( + "database/sql" + "io/ioutil" + "os" +) + +func negative1() { + ff, err := ioutil.TempFile("/fo", "bo") + if err != nil { + print(err) + } + defer ff.Close() + + ff, err = ioutil.TempFile("/fo", "bo") + if err != nil { + print(err) + } + print(123) + ff.Close() +} + +var globalVar *os.File + +func negative2() { + globalVar, _ = ioutil.TempFile("", "") // false negative because "_" doesn't have error type + kk := globalVar.Name() + + print(kk) +} + +func negativeGlobalVar() { + var err error + globalVar, err = ioutil.TempFile("", "") // global var + kk := globalVar.Name() + + print(kk, err) +} + +func negative3() *os.File { + file, _ := ioutil.TempFile("", "") // var escape the function + + return file +} + +func negative4() { + var files []*os.File + file, _ := ioutil.TempFile("", "") // var escape the function in another var + + files = append(files, file) +} + +func negative5() { + var filesMap map[string]*os.File + file, _ := ioutil.TempFile("", "") // var escape the function in another var + + filesMap[file.Name()] = file +} + +func negative6() { + type st struct { + *os.File + } + var ( + fileDecorator1 st + fileDecorator2 st + ) + file, _ := ioutil.TempFile("", "") // var escape the function in another var + + fileDecorator1 = st{file} + fileDecorator2 = st{ + File: file, + } + + kk, kkk := fileDecorator1.Name(), fileDecorator2.Name() + + print(kk, kkk) +} + +func negative7() { + var ch chan *os.File + file, _ := ioutil.TempFile("", "") // var escape the function in another var + + ch <- file +} + +func negative8() []int { + db, _ := sql.Open("", "") + defer db.Close() + + rows, _ := db.QueryContext(nil, "") + + var ( + i int + result []int + ) + defer rows.Close() + + for rows.Next() { + rows.Scan(&i) + result = append(result, i) + } + + return result +} + +func negative9() { + closure := func() (*os.File, error) { + return nil, nil + } + + f, _ := closure() + f.Name() + defer f.Close() +} + +// test for: https://github.com/quasilyte/go-ruleguard/issues/366 +func dataRace() { + f, err := os.Open("bar") + print(f.Name()) + + f, err = os.Open("bar") + if err == nil { + defer f.Close() + } +} diff --git a/testdata/src/unclosedResource/positive.go b/testdata/src/unclosedResource/positive.go new file mode 100644 index 0000000..1f676c6 --- /dev/null +++ b/testdata/src/unclosedResource/positive.go @@ -0,0 +1,83 @@ +package unclosedResource + +import ( + "database/sql" + "io/ioutil" + "os" +) + +func warning1() { + f, err := os.Open("bar") //want `\Qf.Close() should be deferred right after the os.Open error check` + if err == nil { + print(f.Name()) + } + + f, err = os.Open("bar") //want `\Qf.Close() should be deferred right after the os.Open error check` + print(f.Name()) +} + +func warning2() { + f, err := os.Open("bar") + if err == nil { + defer f.Close() + } + + f, err = os.Open("bar") //want `\Qf.Close() should be deferred right after the os.Open error check` + print(f.Name()) +} + +func warning3() { + var ff, err = os.Open("foo.txt") //want `\Qff.Close() should be deferred right after the os.Open error check` + if err != nil { + print(ff.Fd()) + } + + ff, err = ioutil.TempFile("/kek", "foo") //want `\Qff.Close() should be deferred right after the ioutil.TempFile error check` + print(ff.Name()) +} + +func warning4() { + ff, err := os.Open("foo.txt") //want `\Qff.Close() should be deferred right after the os.Open error check` + if err != nil { + print(ff.Fd()) + } + + ff, err = ioutil.TempFile("/kek", "foo") //want `\Qff.Close() should be deferred right after the ioutil.TempFile error check` + print(ff.Name()) +} + +func warning5() { + f, err := os.Open("bar") //want `\Qf.Close() should be deferred right after the os.Open error check` + print(f.Name()) + + ff, err := os.Open("bar") + if err == nil { + defer ff.Close() + } +} + +func warning6() []int { + db, _ := sql.Open("", "") //want `\Qdb.Close() should be deferred right after the sql.Open error check` + var rows, _ = db.QueryContext(nil, "") //want `\Qrows.Close() should be deferred right after the db.QueryContext error check` + + var ( + i int + result []int + ) + + for rows.Next() { + _ = rows.Scan(&i) + result = append(result, i) + } + + return result +} + +func warning7() { + closure := func() (*os.File, error) { + return nil, nil + } + + f, _ := closure() // want `\Qf.Close() should be deferred right after the closure error check` + f.Name() +} diff --git a/todo b/todo index 6eb5dc0..c885e48 100644 --- a/todo +++ b/todo @@ -1,2 +1 @@ 1. timer.Stop check -2. not closed resources check