From 5f388777a2351a3c8f2533b3dfce63249e026e65 Mon Sep 17 00:00:00 2001 From: Ruslan Sorokin <88016602+ruslanSorokin@users.noreply.github.com> Date: Thu, 23 May 2024 09:36:14 +0300 Subject: [PATCH] feat(analyzer): recognize custom error values in return (#102) --------- Co-authored-by: xobotyi --- README.md | 37 +++++++++++++++++++++++++ analyzer/analyzer.go | 45 ++++++++++++++++++++++++------- analyzer/analyzer_test.go | 4 +-- analyzer/testdata/src/i/i.go | 14 ---------- analyzer/testdata/src/j/j.go | 52 ++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 25 deletions(-) create mode 100644 analyzer/testdata/src/j/j.go diff --git a/README.md b/README.md index 0abeadc3..049b1c20 100644 --- a/README.md +++ b/README.md @@ -99,3 +99,40 @@ var b Shape = a.Shape{ Length: 5, } ``` + +### Errors handling + +In order to avoid unnecessary noise, when dealing with non-pointer types returned along with errors - `exhaustruct` will +ignore non-error types, checking only structures satisfying `error` interface. + +```go +package main + +import "errors" + +type Shape struct { + Length int + Width int +} + +func NewShape() (Shape, error) { + return Shape{}, errors.New("error") // will not raise an error +} + +type MyError struct { + Err error +} + +func (e MyError) Error() string { + return e.Err.Error() +} + +func NewSquare() (Shape, error) { + return Shape{}, MyError{Err: errors.New("error")} // will not raise an error +} + +func NewCircle() (Shape, error) { + return Shape{}, MyError{} // will raise "main.MyError is missing field Err" +} + +``` diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index b490f1c6..ec75fd40 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -100,12 +100,9 @@ func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, s if len(lit.Elts) == 0 { if ret, ok := stackParentIsReturn(stack); ok { - if returnContainsNonNilError(pass, ret) { + if returnContainsNonNilError(pass, ret, n) { // it is okay to return uninitialized structure in case struct's direct parent is // a return statement containing non-nil error - // - // we're unable to check if returned error is custom, but at least we're able to - // cover str [error] type. return true } } @@ -184,17 +181,47 @@ func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, * func stackParentIsReturn(stack []ast.Node) (*ast.ReturnStmt, bool) { // it is safe to skip boundary check, since stack always has at least one element - // - whole file. - ret, ok := stack[len(stack)-2].(*ast.ReturnStmt) + // we also have no reason to check the first element, since it is always a file + for i := len(stack) - 2; i > 0; i-- { + switch st := stack[i].(type) { + case *ast.ReturnStmt: + return st, true - return ret, ok + case *ast.UnaryExpr: + // in case we're dealing with pointers - it is still viable to check pointer's + // parent for return statement + continue + + default: + return nil, false + } + } + + return nil, false } -func returnContainsNonNilError(pass *analysis.Pass, ret *ast.ReturnStmt) bool { +// errorIface is a type that represents [error] interface and all types will be +// compared against. +var errorIface = types.Universe.Lookup("error").Type().Underlying().(*types.Interface) + +func returnContainsNonNilError(pass *analysis.Pass, ret *ast.ReturnStmt, except ast.Node) bool { // errors are mostly located at the end of return statement, so we're starting // from the end. for i := len(ret.Results) - 1; i >= 0; i-- { - if pass.TypesInfo.TypeOf(ret.Results[i]).String() == "error" { + ri := ret.Results[i] + + // skip current node + if ri == except { + continue + } + + if un, ok := ri.(*ast.UnaryExpr); ok { + if un.X == except { + continue + } + } + + if types.Implements(pass.TypesInfo.TypeOf(ri), errorIface) { return true } } diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index e56366f7..3d99c02b 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -33,10 +33,10 @@ func TestAnalyzer(t *testing.T) { assert.Error(t, err) a, err = analyzer.NewAnalyzer( - []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`}, + []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`, `j\..*Error`}, []string{`.*Excluded$`, `e\.`}, ) require.NoError(t, err) - analysistest.Run(t, testdataPath, a, "i", "e") + analysistest.Run(t, testdataPath, a, "i", "e", "j") } diff --git a/analyzer/testdata/src/i/i.go b/analyzer/testdata/src/i/i.go index b159c274..93d27c57 100644 --- a/analyzer/testdata/src/i/i.go +++ b/analyzer/testdata/src/i/i.go @@ -2,8 +2,6 @@ package i import ( - "errors" - "e" ) @@ -64,18 +62,6 @@ func shouldFailRequiredOmitted() { } } -func shouldPassEmptyStructWithNonNilErr() (Test, error) { - return Test{}, errors.New("some error") -} - -func shouldFailEmptyStructWithNilErr() (Test, error) { - return Test{}, nil // want "i.Test is missing fields A, B, C, D" -} - -func shouldFailEmptyNestedStructWithNonNilErr() ([]Test, error) { - return []Test{{}}, nil // want "i.Test is missing fields A, B, C, D" -} - func shouldPassUnnamed() { _ = []Test{{"", 0, 0.0, false, ""}} } diff --git a/analyzer/testdata/src/j/j.go b/analyzer/testdata/src/j/j.go new file mode 100644 index 00000000..e7207933 --- /dev/null +++ b/analyzer/testdata/src/j/j.go @@ -0,0 +1,52 @@ +package j + +import ( + "fmt" + "os" +) + +type Test struct { + A string +} + +type AError struct{} + +func (AError) Error() string { return "error message" } + +type BError struct{ msg string } + +func (e BError) Error() string { return e.msg } + +func shouldPassEmptyStructWithConcreteAError() (Test, *AError) { + return Test{}, &AError{} +} + +func shouldFailEmptyStructWithEmptyBError() (Test, error) { + return Test{}, &BError{} // want "j.BError is missing field msg" +} + +func shouldFailEmptyStructWithNilConcreteError() (Test, *BError) { + return Test{}, nil // want "j.Test is missing field A" +} + +func shouldPassEmptyStructWithFmtError() (Test, error) { + return Test{}, fmt.Errorf("error message") +} + +func shouldPassStaticError() (Test, error) { + return Test{}, os.ErrNotExist +} + +func shouldPassAnonymousFunctionReturningError() (Test, error) { + return Test{}, func() error { return nil }() +} + +func shouldFailAnonymousFunctionReturningEmptyError() (Test, error) { + fn := func() error { return &BError{} } // want "j.BError is missing field msg" + + return Test{}, fn() +} + +func shouldFailEmptyNestedStructWithNonNilErr() ([]Test, error) { + return []Test{{}}, os.ErrNotExist // want "j.Test is missing field A" +}