Merge pull request #8 from maratori/allow-error-in-defer

Add flag `allow-error-in-defer`

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,10 +66,66 @@ 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))
 			}
 		}
 	})
 
 	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
+}

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")
 }

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

testdata/src/p/p.go → 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