From 1898b1afd5c36a1fed5652933a61ef1d6911a8e9 Mon Sep 17 00:00:00 2001 From: Christian Mehlmauer Date: Wed, 20 Apr 2022 14:34:38 +0200 Subject: [PATCH 1/5] add tagging to makefile --- Makefile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index 7f30c90..0a5ea90 100644 --- a/Makefile +++ b/Makefile @@ -24,3 +24,9 @@ lint-update: .PHONY: test test: go test -race -cover ./... + +.PHONY: tag +tag: + @[ "${TAG}" ] && echo "Tagging a new version ${TAG}" || ( echo "TAG is not set"; exit 1 ) + git tag -a "${TAG}" -m "${TAG}" + git push origin "${TAG}" From a380e00e7ca20416ba5ef5f0c0651036ae8740e3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 May 2022 01:50:35 +0000 Subject: [PATCH 2/5] Bump goreleaser/goreleaser-action from 2 to 3 Bumps [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) from 2 to 3. - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](https://github.com/goreleaser/goreleaser-action/compare/v2...v3) --- updated-dependencies: - dependency-name: goreleaser/goreleaser-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b6098cf..c498cdc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -23,7 +23,7 @@ jobs: with: go-version: 1.18 - name: Run GoReleaser - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v3 with: distribution: goreleaser version: latest From a9830d2f236f4fb24e5c7ca4bd4de3b898194896 Mon Sep 17 00:00:00 2001 From: Christian Mehlmauer Date: Mon, 30 May 2022 08:17:51 +0200 Subject: [PATCH 3/5] update --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index b5aa45e..6d3245c 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,6 @@ require golang.org/x/tools v0.1.10 require ( golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3 // indirect - golang.org/x/sys v0.0.0-20220403020550-483a9cbc67c0 // indirect - golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect + golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect + golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df // indirect ) diff --git a/go.sum b/go.sum index bb16a6d..e5cb294 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,8 @@ golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3 h1:kQgndtyPBW/JIYERgdxfwMYh3AVStj88WQTlNDi2a+o= golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY= -golang.org/x/sys v0.0.0-20220403020550-483a9cbc67c0 h1:PgUUmg0gNMIPY2WafhL/oLyQGw+kdTNPlVWOjltpp3w= -golang.org/x/sys v0.0.0-20220403020550-483a9cbc67c0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a h1:dGzPydgVsqGcTRVwiLJ1jVbufYwmzD3LfVPLKsKg+0k= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/tools v0.1.10 h1:QjFRCZxdOhBJ/UNgnBZLbNV13DlbnK0quyivTnXJM20= golang.org/x/tools v0.1.10/go.mod h1:Uh6Zz+xoGYZom868N8YTex3t7RhtHDBrE8Gzo9bV56E= -golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE= -golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df h1:5Pf6pFKu98ODmgnpvkJ3kFUOQGGLIzLIkbzUHp47618= +golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8= From a16bc58df9f87a72b72ddd310961b0c787e59804 Mon Sep 17 00:00:00 2001 From: Marat Reymers Date: Thu, 9 Jun 2022 14:02:18 +0300 Subject: [PATCH 4/5] add flag allow-error-in-defer --- analyzer/analyzer.go | 71 ++++++ analyzer/analyzer_test.go | 8 +- .../allow_error_in_defer.go | 226 ++++++++++++++++++ .../p.go => default-config/default_config.go} | 7 + 4 files changed, 311 insertions(+), 1 deletion(-) create mode 100644 testdata/src/allow-error-in-defer/allow_error_in_defer.go rename testdata/src/{p/p.go => default-config/default_config.go} (92%) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 465cce2..b809a31 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -1,6 +1,7 @@ package analyzer import ( + "flag" "go/ast" "go/types" @@ -9,14 +10,25 @@ import ( "golang.org/x/tools/go/ast/inspector" ) +const FlagAllowErrorInDefer = "allow-error-in-defer" + var Analyzer = &analysis.Analyzer{ Name: "nonamedreturns", Doc: "Reports all named returns", + Flags: flags(), Run: run, Requires: []*analysis.Analyzer{inspect.Analyzer}, } +func flags() flag.FlagSet { + fs := flag.FlagSet{} + fs.Bool(FlagAllowErrorInDefer, false, "do not complain about named error, if it is assigned inside defer") + return fs +} + func run(pass *analysis.Pass) (interface{}, error) { + allowErrorInDefer := pass.Analyzer.Flags.Lookup(FlagAllowErrorInDefer).Value.String() == "true" + inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) // only filter function defintions @@ -27,12 +39,15 @@ func run(pass *analysis.Pass) (interface{}, error) { inspector.Preorder(nodeFilter, func(node ast.Node) { var funcResults *ast.FieldList + var funcBody *ast.BlockStmt switch n := node.(type) { case *ast.FuncLit: funcResults = n.Type.Results + funcBody = n.Body case *ast.FuncDecl: funcResults = n.Type.Results + funcBody = n.Body default: return } @@ -51,6 +66,14 @@ func run(pass *analysis.Pass) (interface{}, error) { } for _, n := range p.Names { + if allowErrorInDefer { + if ident, ok := p.Type.(*ast.Ident); ok { + if ident.Name == "error" && findDeferWithErrorAssignment(funcBody, n.Name) { + continue + } + } + } + pass.Reportf(node.Pos(), "named return %q with type %q found", n.Name, types.ExprString(p.Type)) } } @@ -58,3 +81,51 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } + +func findDeferWithErrorAssignment(body *ast.BlockStmt, name string) bool { + found := false + + ast.Inspect(body, func(node ast.Node) bool { + if found { + return false // stop inspection + } + + if d, ok := node.(*ast.DeferStmt); ok { + if fn, ok2 := d.Call.Fun.(*ast.FuncLit); ok2 { + if findErrorAssignment(fn.Body, name) { + found = true + return false + } + } + } + + return true + }) + + return found +} + +func findErrorAssignment(body *ast.BlockStmt, name string) bool { + found := false + + ast.Inspect(body, func(node ast.Node) bool { + if found { + return false // stop inspection + } + + if a, ok := node.(*ast.AssignStmt); ok { + for _, lh := range a.Lhs { + if i, ok2 := lh.(*ast.Ident); ok2 { + if i.Name == name { + found = true + return false + } + } + } + } + + return true + }) + + return found +} diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 5b95fb4..66e5c2c 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -15,5 +15,11 @@ func TestAll(t *testing.T) { } testdata := filepath.Join(filepath.Dir(wd), "testdata") - analysistest.Run(t, testdata, Analyzer, "p") + analysistest.Run(t, testdata, Analyzer, "default-config") + + err = Analyzer.Flags.Set(FlagAllowErrorInDefer, "true") + if err != nil { + t.Fatalf("Failed to set flag: %s", err) + } + analysistest.Run(t, testdata, Analyzer, "allow-error-in-defer") } diff --git a/testdata/src/allow-error-in-defer/allow_error_in_defer.go b/testdata/src/allow-error-in-defer/allow_error_in_defer.go new file mode 100644 index 0000000..1b592f4 --- /dev/null +++ b/testdata/src/allow-error-in-defer/allow_error_in_defer.go @@ -0,0 +1,226 @@ +package main + +func simple() (err error) { + defer func() { + err = nil + }() + return +} + +func twoReturnParams() (i int, err error) { // want `named return "i" with type "int" found` + defer func() { + i = 0 + err = nil + }() + return +} + +// TODO: enable test after https://github.com/firefart/nonamedreturns/pull/7 +//func allUnderscoresExceptError() (_ int, err error) { +// defer func() { +// err = nil +// }() +// return +//} + +func customName() (myName error) { + defer func() { + myName = nil + }() + return +} + +func errorIsNoAssigned() (err error) { // want `named return "err" with type "error" found` + defer func() { + _ = err + processError(err) + if err == nil { + } + switch err { + case nil: + default: + } + }() + return +} + +func shadowVariable() (err error) { + defer func() { + err := 123 // linter doesn't understand that this is different variable (even if different type) (yet?) + _ = err + }() + return +} + +func shadowVariable2() (err error) { + defer func() { + a, err := doSomething() // linter doesn't understand that this is different variable (yet?) + _ = a + _ = err + }() + return +} + +type myError = error // linter doesn't understand that this is the same type (yet?) + +func customType() (err myError) { // want `named return "err" with type "myError" found` + defer func() { + err = nil + }() + return +} + +// TODO: replace `i` with `_` after https://github.com/firefart/nonamedreturns/pull/7 +func notTheLast() (err error, i int) { // want `named return "i" with type "int" found` + defer func() { + i = 0 + err = nil + }() + return +} + +func twoErrorsCombined() (err1, err2 error) { + defer func() { + err1 = nil + err2 = nil + }() + return +} + +func twoErrorsSeparated() (err1 error, err2 error) { + defer func() { + err1 = nil + err2 = nil + }() + return +} + +func errorSlice() (err []error) { // want `named return "err" with type "\[\]error" found` + defer func() { + err = nil + }() + return +} + +func deferWithVariable() (err error) { // want `named return "err" with type "error" found` + f := func() { + err = nil + } + defer f() // linter can't catch closure passed via variable (yet?) + return +} + +func uberMultierr() (err error) { // want `named return "err" with type "error" found` + defer func() { + multierrAppendInto(&err, nil) // linter doesn't allow it (yet?) + }() + return +} + +func deferInDefer() (err error) { + defer func() { + defer func() { + err = nil + }() + }() + return +} + +func twoDefers() (err error) { + defer func() {}() + defer func() { + err = nil + }() + return +} + +func callFunction() (err error) { + defer func() { + _, err = doSomething() + }() + return +} + +func callFunction2() (err error) { + defer func() { + var a int + a, err = doSomething() + _ = a + }() + return +} + +func deepInside() (err error) { + if true { + switch true { + case false: + for i := 0; i < 10; i++ { + go func() { + select { + default: + defer func() { + if true { + switch true { + case false: + for j := 0; j < 10; j++ { + go func() { + select { + default: + err = nil + } + }() + } + } + } + }() + } + }() + } + } + } + return +} + +var goodFuncLiteral = func() (err error) { + defer func() { + err = nil + }() + return +} + +var badFuncLiteral = func() (err error) { // want `named return "err" with type "error" found` + defer func() { + _ = err + }() + return +} + +func funcLiteralInsideFunc() error { + do := func() (err error) { + defer func() { + err = nil + }() + return + } + return do() +} + +type x struct{} + +func (x) goodMethod() (err error) { + defer func() { + err = nil + }() + return +} + +func (x) badMethod() (err error) { // want `named return "err" with type "error" found` + defer func() { + _ = err + }() + return +} + +func processError(error) {} +func doSomething() (int, error) { return 10, nil } +func multierrAppendInto(*error, error) bool { return false } // https://pkg.go.dev/go.uber.org/multierr#AppendInto diff --git a/testdata/src/p/p.go b/testdata/src/default-config/default_config.go similarity index 92% rename from testdata/src/p/p.go rename to testdata/src/default-config/default_config.go index 4a3006a..ee88668 100644 --- a/testdata/src/p/p.go +++ b/testdata/src/default-config/default_config.go @@ -23,6 +23,13 @@ var e = func() (err error) { // want `named return "err" with type "error" found return } +func deferWithError() (err error) { // want `named return "err" with type "error" found` + defer func() { + err = nil // use flag to allow this + }() + return +} + var ( f = func() { return From f79863ad42a58c510162ecc4170a9c44da6c6a32 Mon Sep 17 00:00:00 2001 From: Marat Reymers Date: Wed, 8 Jun 2022 11:41:10 +0300 Subject: [PATCH 5/5] ignore underscore names Name `_` is the same as no name and should be ignored. --- analyzer/analyzer.go | 4 ++++ .../allow_error_in_defer.go | 17 ++++++-------- testdata/src/default-config/default_config.go | 23 +++++++++++++++++++ 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index b809a31..594a0fc 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -66,6 +66,10 @@ func run(pass *analysis.Pass) (interface{}, error) { } for _, n := range p.Names { + if n.Name == "_" { + continue + } + if allowErrorInDefer { if ident, ok := p.Type.(*ast.Ident); ok { if ident.Name == "error" && findDeferWithErrorAssignment(funcBody, n.Name) { diff --git a/testdata/src/allow-error-in-defer/allow_error_in_defer.go b/testdata/src/allow-error-in-defer/allow_error_in_defer.go index 1b592f4..31fa5ff 100644 --- a/testdata/src/allow-error-in-defer/allow_error_in_defer.go +++ b/testdata/src/allow-error-in-defer/allow_error_in_defer.go @@ -15,13 +15,12 @@ func twoReturnParams() (i int, err error) { // want `named return "i" with type return } -// TODO: enable test after https://github.com/firefart/nonamedreturns/pull/7 -//func allUnderscoresExceptError() (_ int, err error) { -// defer func() { -// err = nil -// }() -// return -//} +func allUnderscoresExceptError() (_ int, err error) { + defer func() { + err = nil + }() + return +} func customName() (myName error) { defer func() { @@ -70,10 +69,8 @@ func customType() (err myError) { // want `named return "err" with type "myError return } -// TODO: replace `i` with `_` after https://github.com/firefart/nonamedreturns/pull/7 -func notTheLast() (err error, i int) { // want `named return "i" with type "int" found` +func notTheLast() (err error, _ int) { defer func() { - i = 0 err = nil }() return diff --git a/testdata/src/default-config/default_config.go b/testdata/src/default-config/default_config.go index ee88668..406feba 100644 --- a/testdata/src/default-config/default_config.go +++ b/testdata/src/default-config/default_config.go @@ -23,6 +23,10 @@ var e = func() (err error) { // want `named return "err" with type "error" found return } +var e2 = func() (_ error) { + return +} + func deferWithError() (err error) { // want `named return "err" with type "error" found` defer func() { err = nil // use flag to allow this @@ -43,6 +47,10 @@ var ( err = nil return } + + h2 = func() (_ error) { + return + } ) // this should not match as the implementation does not need named parameters (see below) @@ -56,11 +64,24 @@ func funcDefintionImpl2(arg1, arg2 interface{}) (num int, err error) { // want ` return 0, nil } +func funcDefintionImpl3(arg1, arg2 interface{}) (num int, _ error) { // want `named return "num" with type "int" found` + return 0, nil +} + +func funcDefintionImpl4(arg1, arg2 interface{}) (_ int, _ error) { + return 0, nil +} + var funcVar = func() (msg string) { // want `named return "msg" with type "string" found` msg = "c" return msg } +var funcVar2 = func() (_ string) { + msg := "c" + return msg +} + func test() { a := funcVar() _ = a @@ -98,3 +119,5 @@ func myLog(format string, args ...interface{}) { type obj struct{} func (o *obj) func1() (err error) { return nil } // want `named return "err" with type "error" found` + +func (o *obj) func2() (_ error) { return nil }