From 3dc86d87c85160083a934842aa220ebbacbabd22 Mon Sep 17 00:00:00 2001 From: chavacava Date: Thu, 28 Jun 2018 11:06:43 +0200 Subject: [PATCH 1/2] new ADS rule: newerr --- config.go | 1 + fixtures/ads-newerror.go | 7 +++ rule/ads-newerr.go | 97 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 fixtures/ads-newerror.go create mode 100644 rule/ads-newerr.go diff --git a/config.go b/config.go index ba45edb..ad6c7ae 100644 --- a/config.go +++ b/config.go @@ -52,6 +52,7 @@ var allRules = append([]lint.Rule{ &rule.GetReturnRule{}, &rule.ModifiesParamRule{}, &rule.DeepExitRule{}, + &rule.ADSNewErrRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/ads-newerror.go b/fixtures/ads-newerror.go new file mode 100644 index 0000000..b92e6d8 --- /dev/null +++ b/fixtures/ads-newerror.go @@ -0,0 +1,7 @@ +package fixtures + +import "errors" + +func foo(a, b, c, d int) { + errors.New("aaa") +} diff --git a/rule/ads-newerr.go b/rule/ads-newerr.go new file mode 100644 index 0000000..c130aa9 --- /dev/null +++ b/rule/ads-newerr.go @@ -0,0 +1,97 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// ADSNewErrRule lints program exit at functions other than main or init. +type ADSNewErrRule struct{} + +// Apply applies the rule to given file. +func (r *ADSNewErrRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintNewErr{onFailure, "errors", "New"} + ast.Walk(w, file.AST) + return failures +} + +// Name returns the rule name. +func (r *ADSNewErrRule) Name() string { + return "ads-newerr" +} + +type lintNewErr struct { + onFailure func(lint.Failure) + targetPkg string + targetFunc string +} + +func (w lintNewErr) Visit(node ast.Node) ast.Visitor { + ce, ok := node.(*ast.CallExpr) + if !ok { + return w + } + + pkg, fn := getPkgFunc(ce) + + if pkg == w.targetPkg && fn == w.targetFunc { + if pkg == "errors" && fn == "New" { + w.targetFunc = "MessageOption" + return w + } + + s := searchErr{&w, ce} + for _, exp := range ce.Args { + ast.Walk(s, exp) + } + + w.targetFunc = "New" + } + + return w +} + +type searchErr struct { + w *lintNewErr + fc *ast.CallExpr +} + +func (s searchErr) Visit(node ast.Node) ast.Visitor { + + id, ok := node.(*ast.Ident) + if !ok { + return s + } + if id.Name == "err" { + s.w.onFailure(lint.Failure{ + Confidence: 0.8, + Node: s.fc, + Category: "bad practice", + URL: "#ads-newerr", + Failure: "consider errors.Wrap instead of errors.New", + }) + + } + + return s +} + +func getPkgFunc(ce *ast.CallExpr) (string, string) { + fc, ok := ce.Fun.(*ast.SelectorExpr) + if !ok { + return "", "" + } + id, ok := fc.X.(*ast.Ident) + if !ok { + return "", "" + } + + return id.Name, fc.Sel.Name + +} From fa8bea3d3b9b9bf98574d9a455bc4d7cee4d411d Mon Sep 17 00:00:00 2001 From: chavacava Date: Fri, 29 Jun 2018 13:41:18 +0200 Subject: [PATCH 2/2] ads-lost-err working version --- config.go | 2 +- fixtures/ads-lost-err.go | 9 +++++++++ fixtures/ads-newerror.go | 7 ------- rule/{ads-newerr.go => ads-lost-err.go} | 20 +++++++++----------- test/ads-lost-err_test.go | 11 +++++++++++ 5 files changed, 30 insertions(+), 19 deletions(-) create mode 100644 fixtures/ads-lost-err.go delete mode 100644 fixtures/ads-newerror.go rename rule/{ads-newerr.go => ads-lost-err.go} (72%) create mode 100644 test/ads-lost-err_test.go diff --git a/config.go b/config.go index ad6c7ae..3bf5903 100644 --- a/config.go +++ b/config.go @@ -52,7 +52,7 @@ var allRules = append([]lint.Rule{ &rule.GetReturnRule{}, &rule.ModifiesParamRule{}, &rule.DeepExitRule{}, - &rule.ADSNewErrRule{}, + &rule.ADSLostErrRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/ads-lost-err.go b/fixtures/ads-lost-err.go new file mode 100644 index 0000000..8d31192 --- /dev/null +++ b/fixtures/ads-lost-err.go @@ -0,0 +1,9 @@ +package fixtures + +import "errors" + +func foo(a, b, c, d int) { + errors.New("aaa") + errors.New(errors.InternalError, errors.MessageOption("nice error message "+err.Error())) // MATCH /original error is lost, consider using errors.NewFromError/ + errors.MessageOption("nice error message " + err.Error()) +} diff --git a/fixtures/ads-newerror.go b/fixtures/ads-newerror.go deleted file mode 100644 index b92e6d8..0000000 --- a/fixtures/ads-newerror.go +++ /dev/null @@ -1,7 +0,0 @@ -package fixtures - -import "errors" - -func foo(a, b, c, d int) { - errors.New("aaa") -} diff --git a/rule/ads-newerr.go b/rule/ads-lost-err.go similarity index 72% rename from rule/ads-newerr.go rename to rule/ads-lost-err.go index c130aa9..a8eeb9d 100644 --- a/rule/ads-newerr.go +++ b/rule/ads-lost-err.go @@ -6,33 +6,33 @@ import ( "github.com/mgechev/revive/lint" ) -// ADSNewErrRule lints program exit at functions other than main or init. -type ADSNewErrRule struct{} +// ADSLostErrRule lints program exit at functions other than main or init. +type ADSLostErrRule struct{} // Apply applies the rule to given file. -func (r *ADSNewErrRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *ADSLostErrRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { failures = append(failures, failure) } - w := lintNewErr{onFailure, "errors", "New"} + w := lintLostErr{onFailure, "errors", "New"} ast.Walk(w, file.AST) return failures } // Name returns the rule name. -func (r *ADSNewErrRule) Name() string { +func (r *ADSLostErrRule) Name() string { return "ads-newerr" } -type lintNewErr struct { +type lintLostErr struct { onFailure func(lint.Failure) targetPkg string targetFunc string } -func (w lintNewErr) Visit(node ast.Node) ast.Visitor { +func (w lintLostErr) Visit(node ast.Node) ast.Visitor { ce, ok := node.(*ast.CallExpr) if !ok { return w @@ -58,7 +58,7 @@ func (w lintNewErr) Visit(node ast.Node) ast.Visitor { } type searchErr struct { - w *lintNewErr + w *lintLostErr fc *ast.CallExpr } @@ -73,8 +73,7 @@ func (s searchErr) Visit(node ast.Node) ast.Visitor { Confidence: 0.8, Node: s.fc, Category: "bad practice", - URL: "#ads-newerr", - Failure: "consider errors.Wrap instead of errors.New", + Failure: "original error is lost, consider using errors.NewFromError", }) } @@ -93,5 +92,4 @@ func getPkgFunc(ce *ast.CallExpr) (string, string) { } return id.Name, fc.Sel.Name - } diff --git a/test/ads-lost-err_test.go b/test/ads-lost-err_test.go new file mode 100644 index 0000000..47cf6c1 --- /dev/null +++ b/test/ads-lost-err_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestADSLostErr(t *testing.T) { + testRule(t, "ads-lost-err", &rule.ADSLostErrRule{}) +}