diff --git a/fixtures/error-strings.go b/fixtures/error-strings.go new file mode 100644 index 0000000..eb060b2 --- /dev/null +++ b/fixtures/error-strings.go @@ -0,0 +1,20 @@ +// Package foo ... +package foo + +import ( + "errors" + "fmt" +) + +// Check for the error strings themselves. + +func g(x int) error { + var err error + err = fmt.Errorf("This %d is too low", x) // MATCH /error strings should not be capitalized or end with punctuation or a newline/ + err = fmt.Errorf("XML time") // ok + err = fmt.Errorf("newlines are fun\n") // MATCH /error strings should not be capitalized or end with punctuation or a newline/ + err = fmt.Errorf("Newlines are really fun\n") // MATCH /error strings should not be capitalized or end with punctuation or a newline/ + err = errors.New(`too much stuff.`) // MATCH /error strings should not be capitalized or end with punctuation or a newline/ + err = errors.New("This %d is too low", x) // MATCH /error strings should not be capitalized or end with punctuation or a newline/ + return err +} diff --git a/fixtures/errors.go b/fixtures/errors.go new file mode 100644 index 0000000..11df352 --- /dev/null +++ b/fixtures/errors.go @@ -0,0 +1,25 @@ +// Test for naming errors. + +// Package foo ... +package foo + +import ( + "errors" + "fmt" +) + +var unexp = errors.New("some unexported error") // MATCH /error var unexp should have name of the form errFoo/ + +// Exp ... +var Exp = errors.New("some exported error") // MATCH /error var Exp should have name of the form ErrFoo/ + +var ( + e1 = fmt.Errorf("blah %d", 4) // MATCH /error var e1 should have name of the form errFoo/ + // E2 ... + E2 = fmt.Errorf("blah %d", 5) // MATCH /error var E2 should have name of the form ErrFoo/ +) + +func f() { + var whatever = errors.New("ok") // ok + _ = whatever +} diff --git a/rule/error-strings.go b/rule/error-strings.go new file mode 100644 index 0000000..ae664b4 --- /dev/null +++ b/rule/error-strings.go @@ -0,0 +1,99 @@ +package rule + +import ( + "go/ast" + "go/token" + "strconv" + "unicode" + "unicode/utf8" + + "github.com/mgechev/revive/lint" +) + +// ErrorStringsRule lints given else constructs. +type ErrorStringsRule struct{} + +// Apply applies the rule to given file. +func (r *ErrorStringsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + fileAst := file.AST + walker := lintErrorStrings{ + file: file, + fileAst: fileAst, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ErrorStringsRule) Name() string { + return "errorf" +} + +type lintErrorStrings struct { + file *lint.File + fileAst *ast.File + onFailure func(lint.Failure) +} + +func (w lintErrorStrings) Visit(n ast.Node) ast.Visitor { + ce, ok := n.(*ast.CallExpr) + if !ok { + return w + } + if !isPkgDot(ce.Fun, "errors", "New") && !isPkgDot(ce.Fun, "fmt", "Errorf") { + return w + } + if len(ce.Args) < 1 { + return w + } + str, ok := ce.Args[0].(*ast.BasicLit) + if !ok || str.Kind != token.STRING { + return w + } + s, _ := strconv.Unquote(str.Value) // can assume well-formed Go + if s == "" { + return w + } + clean, conf := lintErrorString(s) + if clean { + return w + } + + w.onFailure(lint.Failure{ + Node: str, + Confidence: conf, + URL: "#error-strings", + Category: "errors", + Failure: "error strings should not be capitalized or end with punctuation or a newline", + }) + return w +} + +func lintErrorString(s string) (isClean bool, conf float64) { + const basicConfidence = 0.8 + const capConfidence = basicConfidence - 0.2 + first, firstN := utf8.DecodeRuneInString(s) + last, _ := utf8.DecodeLastRuneInString(s) + if last == '.' || last == ':' || last == '!' || last == '\n' { + return false, basicConfidence + } + if unicode.IsUpper(first) { + // People use proper nouns and exported Go identifiers in error strings, + // so decrease the confidence of warnings for capitalization. + if len(s) <= firstN { + return false, capConfidence + } + // Flag strings starting with something that doesn't look like an initialism. + if second, _ := utf8.DecodeRuneInString(s[firstN:]); !unicode.IsUpper(second) { + return false, capConfidence + } + } + return true, 0 +} diff --git a/rule/errors.go b/rule/errors.go new file mode 100644 index 0000000..1358bdd --- /dev/null +++ b/rule/errors.go @@ -0,0 +1,79 @@ +package rule + +import ( + "fmt" + "go/ast" + "go/token" + "strings" + + "github.com/mgechev/revive/lint" +) + +// ErrorsRule lints given else constructs. +type ErrorsRule struct{} + +// Apply applies the rule to given file. +func (r *ErrorsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + fileAst := file.AST + walker := lintErrors{ + file: file, + fileAst: fileAst, + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, fileAst) + + return failures +} + +// Name returns the rule name. +func (r *ErrorsRule) Name() string { + return "errors" +} + +type lintErrors struct { + file *lint.File + fileAst *ast.File + onFailure func(lint.Failure) +} + +func (w lintErrors) Visit(n ast.Node) ast.Visitor { + for _, decl := range w.fileAst.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.VAR { + continue + } + for _, spec := range gd.Specs { + spec := spec.(*ast.ValueSpec) + if len(spec.Names) != 1 || len(spec.Values) != 1 { + continue + } + ce, ok := spec.Values[0].(*ast.CallExpr) + if !ok { + continue + } + if !isPkgDot(ce.Fun, "errors", "New") && !isPkgDot(ce.Fun, "fmt", "Errorf") { + continue + } + + id := spec.Names[0] + prefix := "err" + if id.IsExported() { + prefix = "Err" + } + if !strings.HasPrefix(id.Name, prefix) { + w.onFailure(lint.Failure{ + Node: id, + Confidence: 0.9, + Category: "naming", + Failure: fmt.Sprintf("error var %s should have name of the form %sFoo", id.Name, prefix), + }) + } + } + } + return nil +} diff --git a/testutil/lint_test.go b/testutil/lint_test.go index 25ffe18..acc89ac 100644 --- a/testutil/lint_test.go +++ b/testutil/lint_test.go @@ -43,6 +43,8 @@ var rules = []lint.Rule{ &rule.IfReturnRule{}, &rule.RangeRule{}, &rule.ErrorfRule{}, + &rule.ErrorsRule{}, + &rule.ErrorStringsRule{}, } func TestAll(t *testing.T) {