diff --git a/stylecheck/analysis.go b/stylecheck/analysis.go index 7171befe2..b613e0a31 100644 --- a/stylecheck/analysis.go +++ b/stylecheck/analysis.go @@ -22,6 +22,7 @@ import ( "honnef.co/go/tools/stylecheck/st1021" "honnef.co/go/tools/stylecheck/st1022" "honnef.co/go/tools/stylecheck/st1023" + "honnef.co/go/tools/stylecheck/st1024" ) var Analyzers = []*lint.Analyzer{ @@ -43,4 +44,5 @@ var Analyzers = []*lint.Analyzer{ st1021.SCAnalyzer, st1022.SCAnalyzer, st1023.SCAnalyzer, + st1024.SCAnalyzer, } diff --git a/stylecheck/st1024/st1024.go b/stylecheck/st1024/st1024.go new file mode 100644 index 000000000..b5daf9b22 --- /dev/null +++ b/stylecheck/st1024/st1024.go @@ -0,0 +1,81 @@ +package st1024 + +import ( + "go/types" + + "golang.org/x/tools/go/analysis" + "honnef.co/go/tools/analysis/lint" + "honnef.co/go/tools/analysis/report" + "honnef.co/go/tools/internal/passes/buildir" +) + +var SCAnalyzer = lint.InitializeAnalyzer(&lint.Analyzer{ + Analyzer: &analysis.Analyzer{ + Name: "ST1024", + Run: run, + Requires: []*analysis.Analyzer{buildir.Analyzer}, + }, + Doc: &lint.RawDocumentation{ + Title: "Avoid named error return values named 'err'", + Text: `Named return values called 'err' are discouraged because they can lead +to confusion and subtle mistakes. + +In most Go code, 'err' is used for short-lived local variables. Naming a +return parameter 'err' can induce cognitive load. + +For example: + + func fn() (err error) { + if err := doSomething(); err != nil { // shadows the return + return err + } + defer func() {qqq + // This 'err' is the named return, not the one above + if err != nil { + log.Println("deferred error:", err) + } + }() + return nil + } + +Using a distinct name for the named return makes the return value’s +role explicit and avoids confusion: + + func fn() (retErr error) { + if err := doSomething(); err != nil { + return err + } + defer func() { + if retErr != nil { + log.Println("deferred error:", retErr) + } + }() + return nil + } + +This check is opt-in and not enabled by default.`, + Since: "2025.2", + NonDefault: true, + MergeIf: lint.MergeIfAll, + }, +}) + +func run(pass *analysis.Pass) (interface{}, error) { + errorType := types.Universe.Lookup("error").Type() + +fnLoop: + for _, fn := range pass.ResultOf[buildir.Analyzer].(*buildir.IR).SrcFuncs { + sig := fn.Type().(*types.Signature) + rets := sig.Results() + if rets == nil { + continue + } + for i := range rets.Len() { + if r := rets.At(i); types.Unalias(r.Type()) == errorType && r.Name() == "err" { + report.Report(pass, rets.At(i), "named error return should not be named 'err'", report.ShortRange()) + continue fnLoop + } + } + } + return nil, nil +} diff --git a/stylecheck/st1024/st1024_test.go b/stylecheck/st1024/st1024_test.go new file mode 100644 index 000000000..25948e38a --- /dev/null +++ b/stylecheck/st1024/st1024_test.go @@ -0,0 +1,13 @@ +// Code generated by generate.go. DO NOT EDIT. + +package st1024 + +import ( + "testing" + + "honnef.co/go/tools/analysis/lint/testutil" +) + +func TestTestdata(t *testing.T) { + testutil.Run(t, SCAnalyzer) +} diff --git a/stylecheck/st1024/testdata/go1.0/CheckNamedErrorReturns/CheckNamedErrorReturns.go b/stylecheck/st1024/testdata/go1.0/CheckNamedErrorReturns/CheckNamedErrorReturns.go new file mode 100644 index 000000000..567dcfd3d --- /dev/null +++ b/stylecheck/st1024/testdata/go1.0/CheckNamedErrorReturns/CheckNamedErrorReturns.go @@ -0,0 +1,67 @@ +package pkg + +// no names +func fnA0() {} +func fnA1() error { return nil } +func fnA2() (int, error) { return 0, nil } +func fnA3() (error, int) { return nil, 0 } +func fnA4() (error, error) { return nil, nil } + +// names, but not 'err' +func fnB1() (a error) { return nil } +func fnB2() (a int, b error) { return 0, nil } +func fnB3() (a error, b int) { return nil, 0 } +func fnB4() (a error, b error) { return nil, nil } + +// names, and '_' +func fnC1() (_ error) { return nil } +func fnC2() (a int, _ error) { return 0, nil } +func fnC3() (_ error, b int) { return nil, 0 } +func fnC4() (_ error, b error) { return nil, nil } +func fnC5() (a error, _ error) { return nil, nil } + +// false positives: non-errors named 'err' +func fnE1() (err int) { return 0 } +func fnE2() (err int, b error) { return 0, nil } +func fnE3() (a error, err int) { return nil, 0 } + +// bad ones: errors named 'err' +func fnD1() (err error) { return nil } //@ diag(`named error return should not be named 'err'`) +func fnD2() (a int, err error) { return 0, nil } //@ diag(`named error return should not be named 'err'`) +func fnD3() (err error, b int) { return nil, 0 } //@ diag(`named error return should not be named 'err'`) +func fnD4() (err error, b error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) +func fnD5() (a error, err error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) + +type structA struct{} + +// no names +func (structA) fnA0() {} +func (structA) fnA1() error { return nil } +func (structA) fnA2() (int, error) { return 0, nil } +func (structA) fnA3() (error, int) { return nil, 0 } +func (structA) fnA4() (error, error) { return nil, nil } + +// names, but not 'err' +func (structA) fnB1() (a error) { return nil } +func (structA) fnB2() (a int, b error) { return 0, nil } +func (structA) fnB3() (a error, b int) { return nil, 0 } +func (structA) fnB4() (a error, b error) { return nil, nil } + +// names, and '_' +func (structA) fnC1() (_ error) { return nil } +func (structA) fnC2() (a int, _ error) { return 0, nil } +func (structA) fnC3() (_ error, b int) { return nil, 0 } +func (structA) fnC4() (_ error, b error) { return nil, nil } +func (structA) fnC5() (a error, _ error) { return nil, nil } + +// false positives: non-errors named 'err' +func (structA) fnE1() (err int) { return 0 } +func (structA) fnE2() (err int, b error) { return 0, nil } +func (structA) fnE3() (a error, err int) { return nil, 0 } + +// bad ones: errors named 'err' +func (structA) fnD1() (err error) { return nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD2() (a int, err error) { return 0, nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD3() (err error, b int) { return nil, 0 } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD4() (err error, b error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD5() (a error, err error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) diff --git a/stylecheck/st1024/testdata/go1.9/CheckNamedErrorReturns/CheckNamedErrorReturns.go b/stylecheck/st1024/testdata/go1.9/CheckNamedErrorReturns/CheckNamedErrorReturns.go new file mode 100644 index 000000000..9c31ab07b --- /dev/null +++ b/stylecheck/st1024/testdata/go1.9/CheckNamedErrorReturns/CheckNamedErrorReturns.go @@ -0,0 +1,69 @@ +package pkg + +type Error = error + +// no names +func fnA0() {} +func fnA1() Error { return nil } +func fnA2() (int, Error) { return 0, nil } +func fnA3() (Error, int) { return nil, 0 } +func fnA4() (Error, Error) { return nil, nil } + +// names, but not 'err' +func fnB1() (a Error) { return nil } +func fnB2() (a int, b Error) { return 0, nil } +func fnB3() (a Error, b int) { return nil, 0 } +func fnB4() (a Error, b Error) { return nil, nil } + +// names, and '_' +func fnC1() (_ Error) { return nil } +func fnC2() (a int, _ Error) { return 0, nil } +func fnC3() (_ Error, b int) { return nil, 0 } +func fnC4() (_ Error, b Error) { return nil, nil } +func fnC5() (a Error, _ Error) { return nil, nil } + +// false positives: non-errors named 'err' +func fnE1() (err int) { return 0 } +func fnE2() (err int, b Error) { return 0, nil } +func fnE3() (a Error, err int) { return nil, 0 } + +// bad ones: errors named 'err' +func fnD1() (err Error) { return nil } //@ diag(`named error return should not be named 'err'`) +func fnD2() (a int, err Error) { return 0, nil } //@ diag(`named error return should not be named 'err'`) +func fnD3() (err Error, b int) { return nil, 0 } //@ diag(`named error return should not be named 'err'`) +func fnD4() (err Error, b Error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) +func fnD5() (a Error, err Error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) + +type structA struct{} + +// no names +func (structA) fnA0() {} +func (structA) fnA1() Error { return nil } +func (structA) fnA2() (int, Error) { return 0, nil } +func (structA) fnA3() (Error, int) { return nil, 0 } +func (structA) fnA4() (Error, Error) { return nil, nil } + +// names, but not 'err' +func (structA) fnB1() (a Error) { return nil } +func (structA) fnB2() (a int, b Error) { return 0, nil } +func (structA) fnB3() (a Error, b int) { return nil, 0 } +func (structA) fnB4() (a Error, b Error) { return nil, nil } + +// names, and '_' +func (structA) fnC1() (_ Error) { return nil } +func (structA) fnC2() (a int, _ Error) { return 0, nil } +func (structA) fnC3() (_ Error, b int) { return nil, 0 } +func (structA) fnC4() (_ Error, b Error) { return nil, nil } +func (structA) fnC5() (a Error, _ Error) { return nil, nil } + +// false positives: non-errors named 'err' +func (structA) fnE1() (err int) { return 0 } +func (structA) fnE2() (err int, b Error) { return 0, nil } +func (structA) fnE3() (a Error, err int) { return nil, 0 } + +// bad ones: errors named 'err' +func (structA) fnD1() (err Error) { return nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD2() (a int, err Error) { return 0, nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD3() (err Error, b int) { return nil, 0 } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD4() (err Error, b Error) { return nil, nil } //@ diag(`named error return should not be named 'err'`) +func (structA) fnD5() (a Error, err Error) { return nil, nil } //@ diag(`named error return should not be named 'err'`)