From 112c60a449288bf0e14378d73297ddbd3bc517d5 Mon Sep 17 00:00:00 2001 From: Nirupama Date: Thu, 12 Jan 2023 18:23:21 -0800 Subject: [PATCH 1/9] feat: add linter for error/trace/log messages --- cmd/lintroller/lintroller.go | 3 + internal/config/config.go | 13 +++ internal/config/tiers.go | 20 ++++- internal/errorlint/errorlint.go | 140 ++++++++++++++++++++++++++++++++ internal/reporter/reporter.go | 2 +- 5 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 internal/errorlint/errorlint.go diff --git a/cmd/lintroller/lintroller.go b/cmd/lintroller/lintroller.go index 382dcea..7fab257 100644 --- a/cmd/lintroller/lintroller.go +++ b/cmd/lintroller/lintroller.go @@ -14,6 +14,7 @@ import ( "github.com/getoutreach/lintroller/internal/config" "github.com/getoutreach/lintroller/internal/copyright" "github.com/getoutreach/lintroller/internal/doculint" + "github.com/getoutreach/lintroller/internal/errorlint" "github.com/getoutreach/lintroller/internal/header" "github.com/getoutreach/lintroller/internal/todo" "github.com/getoutreach/lintroller/internal/why" @@ -67,6 +68,7 @@ func main() { cfg.Doculint.ValidateConstants, cfg.Doculint.ValidateTypes)}, {cfg.Todo.Enabled, &todo.Analyzer}, {cfg.Why.Enabled, &why.Analyzer}, + {cfg.Errorlint.Enabled, &errorlint.Analyzer}, } var analyzers []*analysis.Analyzer @@ -88,5 +90,6 @@ func main() { ©right.Analyzer, &todo.Analyzer, &why.Analyzer, + &errorlint.Analyzer, ) } diff --git a/internal/config/config.go b/internal/config/config.go index 7fb652d..34317bb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -59,6 +59,7 @@ type Lintroller struct { Doculint Doculint `yaml:"doculint"` Todo Todo `yaml:"todo"` Why Why `yaml:"why"` + Errorlint Errorlint `yaml:"errorlint"` } // MarshalLog implements the log.Marshaler interface. @@ -68,6 +69,7 @@ func (lr *Lintroller) MarshalLog(addField func(key string, value interface{})) { addField("doculint", lr.Doculint) addField("todo", lr.Todo) addField("why", lr.Why) + addField("errorlint", lr.Errorlint) } // Header is the configuration type that matches the flags exposed by the header @@ -174,3 +176,14 @@ type Why struct { func (w *Why) MarshalLog(addField func(key string, value interface{})) { addField("enabled", w.Enabled) } + +// Errorlint is the configuration type that matches the flags exposed by the errorlint linter. +type Errorlint struct { + // Enabled denotes whether or not this linter is enabled. Defaults to true. + Enabled bool `yaml:"enabled"` +} + +// MarshalLog implements the log.Marshaler interface. +func (w *Errorlint) MarshalLog(addField func(key string, value interface{})) { + addField("enabled", w.Enabled) +} diff --git a/internal/config/tiers.go b/internal/config/tiers.go index 360c197..2dee952 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -49,19 +49,19 @@ func (l *Lintroller) ValidateTier() error { switch strings.ToLower(*l.Tier) { case TierBronze: if err := l.EnsureMinimums(&TierBronzeConfiguration); err != nil { - return errors.Wrap(err, "ensure given configuration meets minimum requirments for bronze tier") + return errors.Wrap(err, "ensure given configuration meets minimum requirements for bronze tier") } case TierSilver: if err := l.EnsureMinimums(&TierSilverConfiguration); err != nil { - return errors.Wrap(err, "ensure given configuration meets minimum requirments for silver tier") + return errors.Wrap(err, "ensure given configuration meets minimum requirements for silver tier") } case TierGold: if err := l.EnsureMinimums(&TierGoldConfiguration); err != nil { - return errors.Wrap(err, "ensure given configuration meets minimum requirments for gold tier") + return errors.Wrap(err, "ensure given configuration meets minimum requirements for gold tier") } case TierPlatinum: if err := l.EnsureMinimums(&TierPlatinumConfiguration); err != nil { - return errors.Wrap(err, "ensure given configuration meets minimum requirments for platinum tier") + return errors.Wrap(err, "ensure given configuration meets minimum requirements for platinum tier") } default: log.Warn(context.Background(), @@ -205,6 +205,9 @@ var TierBronzeConfiguration = Lintroller{ Why: Why{ Enabled: false, }, + Errorlint: Errorlint{ + Enabled: false, + }, } // TierSilverConfiguration is the Lintroller configuration minumums that correspond @@ -234,6 +237,9 @@ var TierSilverConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + Enabled: true, + }, } // TierGoldConfiguration is the Lintroller configuration minumums that correspond @@ -263,6 +269,9 @@ var TierGoldConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + Enabled: true, + }, } // TierPlatinumConfiguration is the Lintroller configuration minumums that correspond @@ -292,4 +301,7 @@ var TierPlatinumConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + Enabled: true, + }, } diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go new file mode 100644 index 0000000..1ae630e --- /dev/null +++ b/internal/errorlint/errorlint.go @@ -0,0 +1,140 @@ +// Copyright 2023 Outreach Corporation. All Rights Reserved. + +// Description: See package comment for this one file package. + +// Package errorlint contains the necessary logic for the error/log/trace linter. +package errorlint + +import ( + "go/ast" + "go/token" + "strconv" + "unicode" + "unicode/utf8" + + "github.com/getoutreach/lintroller/internal/common" + "github.com/getoutreach/lintroller/internal/reporter" + "golang.org/x/tools/go/analysis" +) + +// name defines the name for the errorlint linter. +const name = "errorlint" + +// doc defines the help text for the error linter. +const doc = `Ensures that each static message in error/trace/log/ is lowercase. + +A valid example is the following: + + func foo() { errors.New("org not found in launchdarkly rule")}` + +// Analyzer exports the errorlint analyzer (linter). +var Analyzer = analysis.Analyzer{ + Name: name, + Doc: doc, + Run: errorlint, +} + +// errorlint defines linter for error/trace/log messages +func errorlint(_pass *analysis.Pass) (interface{}, error) { + // Ignore test packages. + if common.IsTestPackage(_pass) { + return nil, nil + } + + // Wrap _pass with reporter.Pass to take nolint directives into account. + pass := reporter.NewPass(name, _pass, reporter.Warn()) + for _, file := range pass.Files { + // Ignore generated files and test files. + if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { + continue + } + } + + for expr := range pass.TypesInfo.Types { + pkgName, isCleanString := lintMessageStrings(expr) + if !isCleanString { + pass.Reportf(expr.Pos(), "%s message should be lowercase and last char should not be one of \". : ! \\n\"", pkgName) + } + } + + return nil, nil +} + +// lintMessageStrings examines error/trace/log strings for capitalization and valid ending +func lintMessageStrings(expr ast.Expr) (string, bool) { + call, ok := expr.(*ast.CallExpr) + if !ok { + return "", true + } + + if !isDotInPkg(call.Fun, "errors", "New") && !isDotInPkg(call.Fun, "errors", "Wrap") && + !isDotInPkg(call.Fun, "errors", "Wrapf") && !isDotInPkg(call.Fun, "log", "Warn") && + !isDotInPkg(call.Fun, "log", "Info") && !isDotInPkg(call.Fun, "log", "Error") && + !isDotInPkg(call.Fun, "trace", "StartSpan") && !isDotInPkg(call.Fun, "trace", "StartCall") { + return "", true + } + + if len(call.Args) < 1 { + return "", true + } + + msgIndex := 1 + if isDotInPkg(call.Fun, "errors", "New") { + msgIndex = 0 + } + + msg, ok := call.Args[msgIndex].(*ast.BasicLit) + if !ok || msg.Kind != token.STRING { + return "", true + } + + msgString, err := strconv.Unquote(msg.Value) + if err != nil { + return "", false + } + + if msgString == "" { + return "", true + } + isClean := isStringFormatted(msgString) + return getPkgName(call.Fun), isClean +} + +// isIdent checks if ident string is equal to ast.Ident name +func isIdent(expr ast.Expr, ident string) bool { + id, ok := expr.(*ast.Ident) + return ok && id.Name == ident +} + +// hasDotInPkg checks if pkg.function format is followed +func isDotInPkg(expr ast.Expr, pkg, name string) bool { + sel, ok := expr.(*ast.SelectorExpr) + return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) +} + +// getPkgName returns package name +func getPkgName(expr ast.Expr) string { + sel, ok := expr.(*ast.SelectorExpr) + for _, pkg := range []string{"errors", "log", "trace"} { + if ok && isIdent(sel.X, pkg) { + return pkg + } + } + + return "" +} + +// isStringFormatted examines error/trace/log strings for incorrect ending and capitalization +func isStringFormatted(msg string) bool { + last, _ := utf8.DecodeLastRuneInString(msg) + if last == '.' || last == ':' || last == '!' || last == '\n' { + return false + } + for _, ch := range msg { + if unicode.IsUpper(ch) { + return false + } + } + + return true +} diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go index 90d4580..f154f79 100644 --- a/internal/reporter/reporter.go +++ b/internal/reporter/reporter.go @@ -122,7 +122,7 @@ func (p *Pass) Reportf(pos token.Pos, format string, args ...interface{}) { } if p.warn { - fmt.Printf("%s: %s (%s) [WARNING]", p.Fset.PositionFor(pos, false).String(), fmt.Sprintf(format, args...), p.linter) + fmt.Printf("%s: %s (%s) [WARNING]\n", p.Fset.PositionFor(pos, false).String(), fmt.Sprintf(format, args...), p.linter) return } p.Pass.Reportf(pos, format+" (%s)", append(args, p.linter)...) From b2f01ef3d6754dba067cd7278810693e04539b2f Mon Sep 17 00:00:00 2001 From: Nirupama Date: Fri, 13 Jan 2023 16:34:10 -0800 Subject: [PATCH 2/9] fix: added changes as per comments --- internal/config/tiers.go | 4 +- internal/errorlint/errorlint.go | 148 +++++++++++++++++++++----------- 2 files changed, 100 insertions(+), 52 deletions(-) diff --git a/internal/config/tiers.go b/internal/config/tiers.go index 2dee952..e056fea 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -53,7 +53,7 @@ func (l *Lintroller) ValidateTier() error { } case TierSilver: if err := l.EnsureMinimums(&TierSilverConfiguration); err != nil { - return errors.Wrap(err, "ensure given configuration meets minimum requirements for silver tier") + return errors.Wrap(err, "Ensure given configuration meets minimum requirements for silver tier") } case TierGold: if err := l.EnsureMinimums(&TierGoldConfiguration); err != nil { @@ -164,7 +164,7 @@ func (l *Lintroller) EnsureMinimums(desired *Lintroller) error { //nolint:funlen l.Doculint.MinFunLen = desired.Doculint.MinFunLen } else if l.Doculint.MinFunLen > desired.Doculint.MinFunLen || l.Doculint.MinFunLen < 0 { return fmt.Errorf( - "deviation detected from tier minimum defaults in lintroller.doculint.minFunLen, minFunLen must be set within (0, %d]", + "deviation detected from tier minimum defaults in lintroller.doculint.minfunlen, minfunlen must be set within (0, %d]", desired.Doculint.MinFunLen) } } diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 1ae630e..98af6bb 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -2,7 +2,10 @@ // Description: See package comment for this one file package. -// Package errorlint contains the necessary logic for the error/log/trace linter. +// Package errorlint contains the necessary logic for the error/log/trace linter. The error +// errors.New, errors.WithMessage, errors.WithMessagef, trace.StartCall, trace.StartSpan, log.Warn, +// log.Error, log.Info, errors.Wrap, errors.Wrapf, fmt.Errorf, errors.Errorf static messages for +// capitalization. It also checks messages ending in punctuation ". : ! and \n" package errorlint import ( @@ -25,7 +28,10 @@ const doc = `Ensures that each static message in error/trace/log/ is lowercase. A valid example is the following: - func foo() { errors.New("org not found in launchdarkly rule")}` + errors.New("org not found in launchdarkly rule") +An invalid example is the following: + + errors.New("Org not found in LAUNCHDARKLY rule")` // Analyzer exports the errorlint analyzer (linter). var Analyzer = analysis.Analyzer{ @@ -34,6 +40,27 @@ var Analyzer = analysis.Analyzer{ Run: errorlint, } +// file represents a file being linted. +type file struct { + f *ast.File +} + +func (file *file) walk(fn func(ast.Node) bool) { + ast.Walk(walker(fn), file.f) +} + +// walker adapts a function to satisfy the ast.Visitor interface. +// The function returns whether the walk should proceed into the node's children. +type walker func(ast.Node) bool + +func (w walker) Visit(node ast.Node) ast.Visitor { + if w(node) { + return w + } + + return nil +} + // errorlint defines linter for error/trace/log messages func errorlint(_pass *analysis.Pass) (interface{}, error) { // Ignore test packages. @@ -43,61 +70,84 @@ func errorlint(_pass *analysis.Pass) (interface{}, error) { // Wrap _pass with reporter.Pass to take nolint directives into account. pass := reporter.NewPass(name, _pass, reporter.Warn()) - for _, file := range pass.Files { + for _, astFile := range pass.Files { // Ignore generated files and test files. - if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { + if common.IsGenerated(astFile) || common.IsTestFile(pass.Pass, astFile) { continue } - } - - for expr := range pass.TypesInfo.Types { - pkgName, isCleanString := lintMessageStrings(expr) - if !isCleanString { - pass.Reportf(expr.Pos(), "%s message should be lowercase and last char should not be one of \". : ! \\n\"", pkgName) - } + newFile := &file{f: astFile} + lintMessageStrings(newFile, pass) } return nil, nil } -// lintMessageStrings examines error/trace/log strings for capitalization and valid ending -func lintMessageStrings(expr ast.Expr) (string, bool) { - call, ok := expr.(*ast.CallExpr) - if !ok { - return "", true - } +// lintMessageStrings examines error/trace/log message strings for capitalization and valid ending +func lintMessageStrings(file *file, pass *reporter.Pass) { + file.walk(func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } - if !isDotInPkg(call.Fun, "errors", "New") && !isDotInPkg(call.Fun, "errors", "Wrap") && - !isDotInPkg(call.Fun, "errors", "Wrapf") && !isDotInPkg(call.Fun, "log", "Warn") && - !isDotInPkg(call.Fun, "log", "Info") && !isDotInPkg(call.Fun, "log", "Error") && - !isDotInPkg(call.Fun, "trace", "StartSpan") && !isDotInPkg(call.Fun, "trace", "StartCall") { - return "", true - } + if isErrorPackage(call.Fun) || len(call.Args) < 1 { + return true + } - if len(call.Args) < 1 { - return "", true - } + msgIndex := 1 + if isDotInPkg(call.Fun, "errors", "New") || isDotInPkg(call.Fun, "fmt", "Errorf") || + isDotInPkg(call.Fun, "errors", "Errorf") { + msgIndex = 0 + } - msgIndex := 1 - if isDotInPkg(call.Fun, "errors", "New") { - msgIndex = 0 - } + msg, ok := call.Args[msgIndex].(*ast.BasicLit) + if !ok || msg.Kind != token.STRING { + return true + } - msg, ok := call.Args[msgIndex].(*ast.BasicLit) - if !ok || msg.Kind != token.STRING { - return "", true - } + msgString, err := strconv.Unquote(msg.Value) + if err != nil { + return false + } - msgString, err := strconv.Unquote(msg.Value) - if err != nil { - return "", false - } + if msgString == "" { + return true + } + errormsg := getErrorMessage(msgString) + if errormsg != "" { + pkgName := getPkgName(call.Fun) + pass.Reportf(node.Pos(), "%s "+errormsg, pkgName) + } + return true + }) +} + +// isErrorPackage checks if the ast.Expr package matches the error/fmt/trace/log packages for linter +func isErrorPackage(expr ast.Expr) bool { + return !isDotInPkg(expr, "errors", "New") && !isDotInPkg(expr, "errors", "Wrap") && + !isDotInPkg(expr, "errors", "Wrapf") && !isDotInPkg(expr, "log", "Warn") && + !isDotInPkg(expr, "log", "Info") && !isDotInPkg(expr, "log", "Error") && + !isDotInPkg(expr, "trace", "StartSpan") && !isDotInPkg(expr, "trace", "StartCall") && + !isDotInPkg(expr, "fmt", "Errorf") && !isDotInPkg(expr, "errors", "Errorf") && + !isDotInPkg(expr, "errors", "WithMessage") && !isDotInPkg(expr, "errors", "WithMessagef") +} - if msgString == "" { - return "", true +// getErrorMessage returns message based on whether it has capitalization, punctuation or not +func getErrorMessage(msg string) string { + isCap, isPunct := isStringFormatted(msg) + var errormsg string + switch { + case isCap && isPunct: + errormsg = "message should not be capitalized and should not end with punctuation" + case isCap: + errormsg = "message should not be capitalized" + case isPunct: + errormsg = "message should not end with punctuation" + default: + errormsg = "" } - isClean := isStringFormatted(msgString) - return getPkgName(call.Fun), isClean + + return errormsg } // isIdent checks if ident string is equal to ast.Ident name @@ -112,10 +162,10 @@ func isDotInPkg(expr ast.Expr, pkg, name string) bool { return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) } -// getPkgName returns package name +// getPkgName returns package name errors/fmt/log/trace func getPkgName(expr ast.Expr) string { sel, ok := expr.(*ast.SelectorExpr) - for _, pkg := range []string{"errors", "log", "trace"} { + for _, pkg := range []string{"errors", "log", "fmt", "trace"} { if ok && isIdent(sel.X, pkg) { return pkg } @@ -125,16 +175,14 @@ func getPkgName(expr ast.Expr) string { } // isStringFormatted examines error/trace/log strings for incorrect ending and capitalization -func isStringFormatted(msg string) bool { +func isStringFormatted(msg string) (isCap, isPunct bool) { last, _ := utf8.DecodeLastRuneInString(msg) - if last == '.' || last == ':' || last == '!' || last == '\n' { - return false - } + isPunct = last == '.' || last == ':' || last == '!' || last == '\n' for _, ch := range msg { if unicode.IsUpper(ch) { - return false + isCap = true } } - return true + return } From be9c517d5dea6271a09f789c73ec6c05bf0d148c Mon Sep 17 00:00:00 2001 From: Nirupama Date: Fri, 13 Jan 2023 16:38:21 -0800 Subject: [PATCH 3/9] fix: added changes as per comments --- internal/errorlint/errorlint.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 98af6bb..861a872 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -31,7 +31,10 @@ A valid example is the following: errors.New("org not found in launchdarkly rule") An invalid example is the following: - errors.New("Org not found in LAUNCHDARKLY rule")` + errors.New("Org not found in LAUNCHDARKLY rule") +An invalid example is the following: + + errors.New("Org not found in launchdarkly rule:")` // Analyzer exports the errorlint analyzer (linter). var Analyzer = analysis.Analyzer{ From 040069ba79ccbb466e091279fd1a1d557b980c49 Mon Sep 17 00:00:00 2001 From: Nirupama Date: Tue, 17 Jan 2023 14:38:34 -0800 Subject: [PATCH 4/9] feat: add some fixes --- internal/config/tiers.go | 2 +- internal/errorlint/errorlint.go | 35 +++++++++++++++------------------ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/internal/config/tiers.go b/internal/config/tiers.go index e056fea..4a72bd9 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -53,7 +53,7 @@ func (l *Lintroller) ValidateTier() error { } case TierSilver: if err := l.EnsureMinimums(&TierSilverConfiguration); err != nil { - return errors.Wrap(err, "Ensure given configuration meets minimum requirements for silver tier") + return errors.Wrap(err, "ensure given configuration meets minimum requirements for silver tier") } case TierGold: if err := l.EnsureMinimums(&TierGoldConfiguration); err != nil { diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 861a872..111ad3a 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -2,10 +2,15 @@ // Description: See package comment for this one file package. -// Package errorlint contains the necessary logic for the error/log/trace linter. The error +// Package errorlint contains the necessary logic for the error/log/trace linter. This validates that error +// messages follow Google's go error guidelines +// (https://google.github.io/styleguide/go/decisions.html#error-strings). +// Specifically, this requires that error messages start with a lower-case letter, and do not end in +// punctuation. +// +// This validates the following functions: // errors.New, errors.WithMessage, errors.WithMessagef, trace.StartCall, trace.StartSpan, log.Warn, -// log.Error, log.Info, errors.Wrap, errors.Wrapf, fmt.Errorf, errors.Errorf static messages for -// capitalization. It also checks messages ending in punctuation ". : ! and \n" +// log.Error, log.Info, errors.Wrap, errors.Wrapf, fmt.Errorf, errors.Errorf package errorlint import ( @@ -24,17 +29,11 @@ import ( const name = "errorlint" // doc defines the help text for the error linter. -const doc = `Ensures that each static message in error/trace/log/ is lowercase. - -A valid example is the following: - - errors.New("org not found in launchdarkly rule") -An invalid example is the following: - - errors.New("Org not found in LAUNCHDARKLY rule") -An invalid example is the following: - - errors.New("Org not found in launchdarkly rule:")` +const doc = `Ensures that each error message starts with a lower-case letter and does not end in puctuation. +// Bad: +err := fmt.Errorf("Something bad happened.") +// Good: +err := fmt.Errorf("something bad happened")` // Analyzer exports the errorlint analyzer (linter). var Analyzer = analysis.Analyzer{ @@ -93,7 +92,7 @@ func lintMessageStrings(file *file, pass *reporter.Pass) { return true } - if isErrorPackage(call.Fun) || len(call.Args) < 1 { + if isNotErrorPackage(call.Fun) || len(call.Args) < 1 { return true } @@ -125,8 +124,8 @@ func lintMessageStrings(file *file, pass *reporter.Pass) { }) } -// isErrorPackage checks if the ast.Expr package matches the error/fmt/trace/log packages for linter -func isErrorPackage(expr ast.Expr) bool { +// isNotErrorPackage checks if the ast.Expr package matches the error/fmt/trace/log packages for linter +func isNotErrorPackage(expr ast.Expr) bool { return !isDotInPkg(expr, "errors", "New") && !isDotInPkg(expr, "errors", "Wrap") && !isDotInPkg(expr, "errors", "Wrapf") && !isDotInPkg(expr, "log", "Warn") && !isDotInPkg(expr, "log", "Info") && !isDotInPkg(expr, "log", "Error") && @@ -146,8 +145,6 @@ func getErrorMessage(msg string) string { errormsg = "message should not be capitalized" case isPunct: errormsg = "message should not end with punctuation" - default: - errormsg = "" } return errormsg From c239b77de7bef2f5c3eb663cf53de2a73bf33074 Mon Sep 17 00:00:00 2001 From: Nirupama Date: Tue, 17 Jan 2023 14:53:05 -0800 Subject: [PATCH 5/9] feat: add some fixes --- internal/errorlint/errorlint.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 111ad3a..4e56b6d 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -177,7 +177,7 @@ func getPkgName(expr ast.Expr) string { // isStringFormatted examines error/trace/log strings for incorrect ending and capitalization func isStringFormatted(msg string) (isCap, isPunct bool) { last, _ := utf8.DecodeLastRuneInString(msg) - isPunct = last == '.' || last == ':' || last == '!' || last == '\n' + isPunct = unicode.IsPunct(last) || last == '\n' for _, ch := range msg { if unicode.IsUpper(ch) { isCap = true From 3f79b9dbb99535988e49da3ee69c523762cab0c0 Mon Sep 17 00:00:00 2001 From: Nirupama Date: Tue, 17 Jan 2023 16:55:53 -0800 Subject: [PATCH 6/9] feat: fixing ast.Inspect changes and opslevel config --- cmd/lintroller/lintroller.go | 14 +++++++++++- internal/config/config.go | 8 +++---- internal/config/tiers.go | 8 +++---- internal/errorlint/errorlint.go | 39 +++++++++------------------------ 4 files changed, 31 insertions(+), 38 deletions(-) diff --git a/cmd/lintroller/lintroller.go b/cmd/lintroller/lintroller.go index 7fab257..adcdceb 100644 --- a/cmd/lintroller/lintroller.go +++ b/cmd/lintroller/lintroller.go @@ -68,7 +68,6 @@ func main() { cfg.Doculint.ValidateConstants, cfg.Doculint.ValidateTypes)}, {cfg.Todo.Enabled, &todo.Analyzer}, {cfg.Why.Enabled, &why.Analyzer}, - {cfg.Errorlint.Enabled, &errorlint.Analyzer}, } var analyzers []*analysis.Analyzer @@ -78,6 +77,19 @@ func main() { } } + warnTable := []struct { + Warn bool + Analyzer *analysis.Analyzer + }{ + {cfg.Errorlint.Warn, &errorlint.Analyzer}, + } + + for i := range warnTable { + if warnTable[i].Warn { + analyzers = append(analyzers, table[i].Analyzer) + } + } + if len(analyzers) > 0 { multichecker.Main(analyzers...) } diff --git a/internal/config/config.go b/internal/config/config.go index 34317bb..5be9f61 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -179,11 +179,11 @@ func (w *Why) MarshalLog(addField func(key string, value interface{})) { // Errorlint is the configuration type that matches the flags exposed by the errorlint linter. type Errorlint struct { - // Enabled denotes whether or not this linter is enabled. Defaults to true. - Enabled bool `yaml:"enabled"` + // Warn denotes whether or not this linter is enabled as a warning. Defaults to true. + Warn bool `yaml:"warn"` } // MarshalLog implements the log.Marshaler interface. -func (w *Errorlint) MarshalLog(addField func(key string, value interface{})) { - addField("enabled", w.Enabled) +func (e *Errorlint) MarshalLog(addField func(key string, value interface{})) { + addField("warn", e.Warn) } diff --git a/internal/config/tiers.go b/internal/config/tiers.go index 4a72bd9..800c227 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -206,7 +206,7 @@ var TierBronzeConfiguration = Lintroller{ Enabled: false, }, Errorlint: Errorlint{ - Enabled: false, + Warn: false, }, } @@ -238,7 +238,7 @@ var TierSilverConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Enabled: true, + Warn: false, }, } @@ -270,7 +270,7 @@ var TierGoldConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Enabled: true, + Warn: true, }, } @@ -302,6 +302,6 @@ var TierPlatinumConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Enabled: true, + Warn: true, }, } diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 4e56b6d..5c7b2fd 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -42,27 +42,6 @@ var Analyzer = analysis.Analyzer{ Run: errorlint, } -// file represents a file being linted. -type file struct { - f *ast.File -} - -func (file *file) walk(fn func(ast.Node) bool) { - ast.Walk(walker(fn), file.f) -} - -// walker adapts a function to satisfy the ast.Visitor interface. -// The function returns whether the walk should proceed into the node's children. -type walker func(ast.Node) bool - -func (w walker) Visit(node ast.Node) ast.Visitor { - if w(node) { - return w - } - - return nil -} - // errorlint defines linter for error/trace/log messages func errorlint(_pass *analysis.Pass) (interface{}, error) { // Ignore test packages. @@ -72,21 +51,20 @@ func errorlint(_pass *analysis.Pass) (interface{}, error) { // Wrap _pass with reporter.Pass to take nolint directives into account. pass := reporter.NewPass(name, _pass, reporter.Warn()) - for _, astFile := range pass.Files { + for _, file := range pass.Files { // Ignore generated files and test files. - if common.IsGenerated(astFile) || common.IsTestFile(pass.Pass, astFile) { + if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { continue } - newFile := &file{f: astFile} - lintMessageStrings(newFile, pass) + lintMessageStrings(file, pass) } return nil, nil } // lintMessageStrings examines error/trace/log message strings for capitalization and valid ending -func lintMessageStrings(file *file, pass *reporter.Pass) { - file.walk(func(node ast.Node) bool { +func lintMessageStrings(file *ast.File, pass *reporter.Pass) { + ast.Inspect(file, func(node ast.Node) bool { call, ok := node.(*ast.CallExpr) if !ok { return true @@ -165,8 +143,11 @@ func isDotInPkg(expr ast.Expr, pkg, name string) bool { // getPkgName returns package name errors/fmt/log/trace func getPkgName(expr ast.Expr) string { sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return "" + } for _, pkg := range []string{"errors", "log", "fmt", "trace"} { - if ok && isIdent(sel.X, pkg) { + if isIdent(sel.X, pkg) { return pkg } } @@ -177,7 +158,7 @@ func getPkgName(expr ast.Expr) string { // isStringFormatted examines error/trace/log strings for incorrect ending and capitalization func isStringFormatted(msg string) (isCap, isPunct bool) { last, _ := utf8.DecodeLastRuneInString(msg) - isPunct = unicode.IsPunct(last) || last == '\n' + isPunct = last == '.' || last == ':' || last == '!' || last == '\n' for _, ch := range msg { if unicode.IsUpper(ch) { isCap = true From 0289a9c1d6474ce3a2003ba3fcf2f62d0ba67310 Mon Sep 17 00:00:00 2001 From: Nirupama Date: Tue, 17 Jan 2023 18:49:57 -0800 Subject: [PATCH 7/9] feat: separating linters for caps, punct, empty string --- internal/errorlint/errorlint.go | 51 +++++++++++++++++---------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 5c7b2fd..5968bfe 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -89,14 +89,10 @@ func lintMessageStrings(file *ast.File, pass *reporter.Pass) { if err != nil { return false } - - if msgString == "" { - return true - } - errormsg := getErrorMessage(msgString) - if errormsg != "" { - pkgName := getPkgName(call.Fun) - pass.Reportf(node.Pos(), "%s "+errormsg, pkgName) + pkgName := getPkgName(call.Fun) + errormsg := getErrorMessages(msgString) + for _, msg := range errormsg { + pass.Reportf(node.Pos(), "%s "+msg, pkgName) } return true }) @@ -112,17 +108,18 @@ func isNotErrorPackage(expr ast.Expr) bool { !isDotInPkg(expr, "errors", "WithMessage") && !isDotInPkg(expr, "errors", "WithMessagef") } -// getErrorMessage returns message based on whether it has capitalization, punctuation or not -func getErrorMessage(msg string) string { - isCap, isPunct := isStringFormatted(msg) - var errormsg string - switch { - case isCap && isPunct: - errormsg = "message should not be capitalized and should not end with punctuation" - case isCap: - errormsg = "message should not be capitalized" - case isPunct: - errormsg = "message should not end with punctuation" +// getErrorMessages returns messages based on whether error message is empty, is capitalized, or punctuated +func getErrorMessages(msg string) []string { + var errormsg []string + if msg == "" { + errormsg = append(errormsg, "message should not be empty") + } else { + if isUpperCase(msg) { + errormsg = append(errormsg, "message should not be capitalized") + } + if hasPunctuation(msg) { + errormsg = append(errormsg, "message should not end with punctuation") + } } return errormsg @@ -155,15 +152,19 @@ func getPkgName(expr ast.Expr) string { return "" } -// isStringFormatted examines error/trace/log strings for incorrect ending and capitalization -func isStringFormatted(msg string) (isCap, isPunct bool) { - last, _ := utf8.DecodeLastRuneInString(msg) - isPunct = last == '.' || last == ':' || last == '!' || last == '\n' +// isUpperCase examines error/trace/log strings for capitalization +func isUpperCase(msg string) bool { for _, ch := range msg { if unicode.IsUpper(ch) { - isCap = true + return true } } - return + return false +} + +// hasPunctuation examines error/trace/log strings for ending in punctuation +func hasPunctuation(msg string) bool { + last, _ := utf8.DecodeLastRuneInString(msg) + return last == '.' || last == ':' || last == '!' || last == '\n' } From 3264e847def9c13ad5d10bc73386f655e246b1e8 Mon Sep 17 00:00:00 2001 From: George Shaw Date: Tue, 17 Jan 2023 19:14:35 -0800 Subject: [PATCH 8/9] enable every linter to warn instead of error based off of config --- cmd/lintroller/lintroller.go | 23 +++++----------------- internal/config/config.go | 26 +++++++++++++++++++++++- internal/config/tiers.go | 11 +++++++---- internal/copyright/copyright.go | 33 +++++++++++++++++++++++-------- internal/doculint/doculint.go | 20 ++++++++++++++++--- internal/errorlint/errorlint.go | 35 ++++++++++++++++++++++++++++++--- internal/header/header.go | 28 +++++++++++++++++++++----- internal/todo/todo.go | 32 ++++++++++++++++++++++++++++-- internal/why/why.go | 32 ++++++++++++++++++++++++++++-- 9 files changed, 194 insertions(+), 46 deletions(-) diff --git a/cmd/lintroller/lintroller.go b/cmd/lintroller/lintroller.go index adcdceb..379b181 100644 --- a/cmd/lintroller/lintroller.go +++ b/cmd/lintroller/lintroller.go @@ -61,13 +61,13 @@ func main() { Enabled bool Analyzer *analysis.Analyzer }{ - {cfg.Header.Enabled, header.NewAnalyzerWithOptions(strings.Join(cfg.Header.Fields, ","))}, - {cfg.Copyright.Enabled, copyright.NewAnalyzerWithOptions(cfg.Copyright.Text, cfg.Copyright.Pattern)}, + {cfg.Header.Enabled, header.NewAnalyzerWithOptions(strings.Join(cfg.Header.Fields, ","), cfg.Header.Warn)}, + {cfg.Copyright.Enabled, copyright.NewAnalyzerWithOptions(cfg.Copyright.Text, cfg.Copyright.Pattern, cfg.Copyright.Warn)}, {cfg.Doculint.Enabled, doculint.NewAnalyzerWithOptions(cfg.Doculint.MinFunLen, cfg.Doculint.ValidatePackages, cfg.Doculint.ValidateFunctions, cfg.Doculint.ValidateVariables, - cfg.Doculint.ValidateConstants, cfg.Doculint.ValidateTypes)}, - {cfg.Todo.Enabled, &todo.Analyzer}, - {cfg.Why.Enabled, &why.Analyzer}, + cfg.Doculint.ValidateConstants, cfg.Doculint.ValidateTypes, cfg.Doculint.Warn)}, + {cfg.Todo.Enabled, todo.NewAnalyzerWithOptions(cfg.Todo.Warn)}, + {cfg.Why.Enabled, why.NewAnalyzerWithOptions(cfg.Why.Warn)}, } var analyzers []*analysis.Analyzer @@ -77,19 +77,6 @@ func main() { } } - warnTable := []struct { - Warn bool - Analyzer *analysis.Analyzer - }{ - {cfg.Errorlint.Warn, &errorlint.Analyzer}, - } - - for i := range warnTable { - if warnTable[i].Warn { - analyzers = append(analyzers, table[i].Analyzer) - } - } - if len(analyzers) > 0 { multichecker.Main(analyzers...) } diff --git a/internal/config/config.go b/internal/config/config.go index 5be9f61..b159b46 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -78,6 +78,10 @@ type Header struct { // Enabled denotes whether or not this linter is enabled. Defaults to true. Enabled bool `yaml:"enabled"` + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. + Warn bool `yaml:"warn"` + // Fields is a list of fields required to be filled out in the header. Defaults // to []string{"Description"}. Fields []string `yaml:"fields"` @@ -95,6 +99,10 @@ type Copyright struct { // Enabled denotes whether or not this linter is enabled. Defaults to true. Enabled bool `yaml:"enabled"` + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. + Warn bool `yaml:"warn"` + // Text is the copyright literal string required at the top of each .go file. If this // and pattern are empty this linter is a no-op. Pattern will always take precedence // over text if both are provided. Defaults to an empty string. @@ -119,6 +127,10 @@ type Doculint struct { // Enabled denotes whether or not this linter is enabled. Defaults to true. Enabled bool `yaml:"enabled"` + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. + Warn bool `yaml:"warn"` + // MinFunLen is the minimum function length that doculint will report on if said // function has no related documentation. Defaults to 10. MinFunLen int `yaml:"minFunLen"` @@ -159,6 +171,10 @@ func (d *Doculint) MarshalLog(addField func(key string, value interface{})) { type Todo struct { // Enabled denotes whether or not this linter is enabled. Defaults to true. Enabled bool `yaml:"enabled"` + + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. + Warn bool `yaml:"warn"` } // MarshalLog implements the log.Marshaler interface. @@ -170,6 +186,10 @@ func (t *Todo) MarshalLog(addField func(key string, value interface{})) { type Why struct { // Enabled denotes whether or not this linter is enabled. Defaults to true. Enabled bool `yaml:"enabled"` + + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. + Warn bool `yaml:"warn"` } // MarshalLog implements the log.Marshaler interface. @@ -179,7 +199,11 @@ func (w *Why) MarshalLog(addField func(key string, value interface{})) { // Errorlint is the configuration type that matches the flags exposed by the errorlint linter. type Errorlint struct { - // Warn denotes whether or not this linter is enabled as a warning. Defaults to true. + // Enabled denotes whether or not this linter is enabled. Defaults to true. + Enabled bool `yaml:"enabled"` + + // Warn denotes whether or not hits from this linter will result in warnings or + // errors. Defaults to false. Warn bool `yaml:"warn"` } diff --git a/internal/config/tiers.go b/internal/config/tiers.go index 800c227..7fdafc1 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -206,7 +206,7 @@ var TierBronzeConfiguration = Lintroller{ Enabled: false, }, Errorlint: Errorlint{ - Warn: false, + Enabled: false, }, } @@ -238,7 +238,8 @@ var TierSilverConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Warn: false, + Enabled: true, + Warn: true, }, } @@ -270,7 +271,8 @@ var TierGoldConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Warn: true, + Enabled: true, + Warn: true, }, } @@ -302,6 +304,7 @@ var TierPlatinumConfiguration = Lintroller{ Enabled: true, }, Errorlint: Errorlint{ - Warn: true, + Enabled: true, + Warn: true, }, } diff --git a/internal/copyright/copyright.go b/internal/copyright/copyright.go index 10c925f..9bab1aa 100644 --- a/internal/copyright/copyright.go +++ b/internal/copyright/copyright.go @@ -12,6 +12,7 @@ import ( "sync" "github.com/getoutreach/lintroller/internal/common" + "github.com/getoutreach/lintroller/internal/reporter" "golang.org/x/tools/go/analysis" ) @@ -36,9 +37,10 @@ var Analyzer = analysis.Analyzer{ // that would have been defined via flags if this was ran as a vet tool. This is so the // analyzers can be ran outside of the context of a vet tool and config can be gathered // from elsewhere. -func NewAnalyzerWithOptions(_text, _pattern string) *analysis.Analyzer { +func NewAnalyzerWithOptions(_text, _pattern string, _warn bool) *analysis.Analyzer { text = strings.TrimSpace(_text) pattern = strings.TrimSpace(_pattern) + warn = _warn return &Analyzer } @@ -52,6 +54,10 @@ var ( // pattern is a variable that gets collected via flags. This variable contains the copyright // string as a regular expression pattern that is required to be at the top of each .go file. pattern string + + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool ) // comparer is a convience type used to conditionally compare using either a string or a @@ -126,10 +132,12 @@ func (c *comparer) trackUniqueness(copyrightString string) { func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. // Setup flags. - //nolint:lll // Why: usage long - Analyzer.Flags.StringVar(&text, "text", "", "the copyright string required at the top of each .go file. if this and pattern are empty the linter is a no-op") - //nolint:lll // Why: usage long - Analyzer.Flags.StringVar(&pattern, "pattern", "", "the copyright pattern (as a regular expression) required at the top of each .go file. if this and pattern are empty the linter is a no-op. pattern takes precedence over text if both are supplied") + Analyzer.Flags.StringVar(&text, "text", "", + "the copyright string required at the top of each .go file. if this and pattern are empty the linter is a no-op") + Analyzer.Flags.StringVar(&pattern, "pattern", "", + "the copyright pattern (as a regular expression) required at the top of each .go file. if this and pattern are empty the linter is a no-op. pattern takes precedence over text if both are supplied") //nolint:lll // Why: usage long + Analyzer.Flags.BoolVar(&warn, "warn", false, + "controls whether or not reports from this linter will result in errors or warnings") // Trim space around the passed in variables just in case. text = strings.TrimSpace(text) @@ -138,9 +146,9 @@ func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. // copyright is the function that gets passed to the Analyzer which runs the actual // analysis for the copyright linter on a set of files. -func copyright(pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Why: Doesn't make sense to break this function up anymore. +func copyright(_pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Why: Doesn't make sense to break this function up anymore. // Ignore test packages. - if common.IsTestPackage(pass) { + if common.IsTestPackage(_pass) { return nil, nil } @@ -148,12 +156,21 @@ func copyright(pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Wh return nil, nil } + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) + // comparer to use on this pass. var c comparer for _, file := range pass.Files { // Ignore generated files and test files. - if common.IsGenerated(file) || common.IsTestFile(pass, file) { + if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { continue } diff --git a/internal/doculint/doculint.go b/internal/doculint/doculint.go index 96d2011..ae1fcc4 100644 --- a/internal/doculint/doculint.go +++ b/internal/doculint/doculint.go @@ -38,13 +38,15 @@ var Analyzer = analysis.Analyzer{ // analyzers can be ran outside of the context of a vet tool and config can be gathered // from elsewhere. func NewAnalyzerWithOptions( - _minFunLen int, _validatePackages, _validateFunctions, _validateVariables, _validateConstants, _validateTypes bool) *analysis.Analyzer { + _minFunLen int, _validatePackages, _validateFunctions, + _validateVariables, _validateConstants, _validateTypes, _warn bool) *analysis.Analyzer { minFunLen = _minFunLen validatePackages = _validatePackages validateFunctions = _validateFunctions validateVariables = _validateVariables validateConstants = _validateConstants validateTypes = _validateTypes + warn = _warn return &Analyzer } @@ -81,6 +83,10 @@ var ( // flag that denotes whether or not the linter should validate that types have // satisfactory comments. validateTypes bool + + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool ) func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. @@ -96,6 +102,8 @@ func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. &validateConstants, "validateConstants", true, "a boolean flag that denotes whether or not to validate constant comments") Analyzer.Flags.BoolVar( &validateTypes, "validateTypes", true, "a boolean flag that denotes whether or not to validate type comments") + Analyzer.Flags.BoolVar( + &warn, "warn", false, "controls whether or not reports from this linter will result in errors or warnings") if minFunLen == 0 { minFunLen = 10 @@ -110,8 +118,14 @@ func doculint(_pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Wh return nil, nil } - // Wrap _pass with reporter.Pass to take nolint directives into account. - pass := reporter.NewPass(name, _pass) + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) // Variable to keep track of whether or not this current package has a file with // the same name as the package. This is where the package comment should exist. diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index 5968bfe..dd5d984 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -29,7 +29,7 @@ import ( const name = "errorlint" // doc defines the help text for the error linter. -const doc = `Ensures that each error message starts with a lower-case letter and does not end in puctuation. +const doc = `Ensures that each error message starts with a lower-case letter and does not end in punctuation. // Bad: err := fmt.Errorf("Something bad happened.") // Good: @@ -42,6 +42,28 @@ var Analyzer = analysis.Analyzer{ Run: errorlint, } +// NewAnalyzerWithOptions returns the Analyzer package-level variable, with the options +// that would have been defined via flags if this was ran as a vet tool. This is so the +// analyzers can be ran outside of the context of a vet tool and config can be gathered +// from elsewhere. +func NewAnalyzerWithOptions(_warn bool) *analysis.Analyzer { + warn = _warn + return &Analyzer +} + +// Variable block to keep track of flags whose values are collected at runtime. See the +// init function that immediately proceeds this block to see more. +var ( + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool +) + +func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. + Analyzer.Flags.BoolVar(&warn, + "warn", false, "controls whether or not reports from this linter will result in errors or warnings") +} + // errorlint defines linter for error/trace/log messages func errorlint(_pass *analysis.Pass) (interface{}, error) { // Ignore test packages. @@ -49,8 +71,15 @@ func errorlint(_pass *analysis.Pass) (interface{}, error) { return nil, nil } - // Wrap _pass with reporter.Pass to take nolint directives into account. - pass := reporter.NewPass(name, _pass, reporter.Warn()) + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) + for _, file := range pass.Files { // Ignore generated files and test files. if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { diff --git a/internal/header/header.go b/internal/header/header.go index b61debc..bf5a9e6 100644 --- a/internal/header/header.go +++ b/internal/header/header.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/getoutreach/lintroller/internal/common" + "github.com/getoutreach/lintroller/internal/reporter" "golang.org/x/tools/go/analysis" ) @@ -58,8 +59,9 @@ var Analyzer = analysis.Analyzer{ // that would have been defined via flags if this was ran as a vet tool. This is so the // analyzers can be ran outside of the context of a vet tool and config can be gathered // from elsewhere. -func NewAnalyzerWithOptions(_rawFields string) *analysis.Analyzer { +func NewAnalyzerWithOptions(_rawFields string, _warn bool) *analysis.Analyzer { rawFields = _rawFields + warn = _warn return &Analyzer } @@ -70,26 +72,42 @@ var ( // comma-separated list of fields required to be filled out within the header of a // file. rawFields string + + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool ) func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. - Analyzer.Flags.StringVar(&rawFields, "fields", "Description", "comma-separated list of fields required to be filled out in the header") + Analyzer.Flags.StringVar(&rawFields, + "fields", "Description", "comma-separated list of fields required to be filled out in the header") + Analyzer.Flags.BoolVar(&warn, + "warn", false, "controls whether or not reports from this linter will result in errors or warnings") } // header is the function that gets passed to the Analyzer which runs the actual // analysis for the header linter on a set of files. -func header(pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Why: Doesn't make sense to break this up. +func header(_pass *analysis.Pass) (interface{}, error) { //nolint:funlen // Why: Doesn't make sense to break this up. // Ignore test packages. - if common.IsTestPackage(pass) { + if common.IsTestPackage(_pass) { return nil, nil } + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) + fields := strings.Split(rawFields, ",") validFields := make(map[string]bool, len(fields)) for _, file := range pass.Files { // Ignore generated files and test files. - if common.IsGenerated(file) || common.IsTestFile(pass, file) { + if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { continue } diff --git a/internal/todo/todo.go b/internal/todo/todo.go index e18e21c..d2c09e8 100644 --- a/internal/todo/todo.go +++ b/internal/todo/todo.go @@ -30,6 +30,28 @@ var Analyzer = analysis.Analyzer{ Run: todo, } +// NewAnalyzerWithOptions returns the Analyzer package-level variable, with the options +// that would have been defined via flags if this was ran as a vet tool. This is so the +// analyzers can be ran outside of the context of a vet tool and config can be gathered +// from elsewhere. +func NewAnalyzerWithOptions(_warn bool) *analysis.Analyzer { + warn = _warn + return &Analyzer +} + +// Variable block to keep track of flags whose values are collected at runtime. See the +// init function that immediately proceeds this block to see more. +var ( + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool +) + +func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. + Analyzer.Flags.BoolVar(&warn, + "warn", false, "controls whether or not reports from this linter will result in errors or warnings") +} + // reTodo is the regular expression that matches the required TODO format by this // linter. This is a TODO followed by one or more of a username in parens and a // Jira ticket ID in brackets. @@ -43,8 +65,14 @@ func todo(_pass *analysis.Pass) (interface{}, error) { return nil, nil } - // Wrap _pass with reporter.Pass to take nolint directives into account. - pass := reporter.NewPass(name, _pass) + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) for _, file := range pass.Files { // Ignore generated files and test files. diff --git a/internal/why/why.go b/internal/why/why.go index dd1c811..ffcc29c 100644 --- a/internal/why/why.go +++ b/internal/why/why.go @@ -33,6 +33,28 @@ var Analyzer = analysis.Analyzer{ Run: why, } +// NewAnalyzerWithOptions returns the Analyzer package-level variable, with the options +// that would have been defined via flags if this was ran as a vet tool. This is so the +// analyzers can be ran outside of the context of a vet tool and config can be gathered +// from elsewhere. +func NewAnalyzerWithOptions(_warn bool) *analysis.Analyzer { + warn = _warn + return &Analyzer +} + +// Variable block to keep track of flags whose values are collected at runtime. See the +// init function that immediately proceeds this block to see more. +var ( + // warn denotes whether or not lint reports from this linter will result in warnings or + // errors. + warn bool +) + +func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. + Analyzer.Flags.BoolVar(&warn, + "warn", false, "controls whether or not reports from this linter will result in errors or warnings") +} + // whyPattern is a regular expression fragment that matches just a "Why" // comment. const whyPattern = `//\s?Why:.+` @@ -54,8 +76,14 @@ func why(_pass *analysis.Pass) (interface{}, error) { return nil, nil } - // Wrap _pass with reporter.Pass to take nolint directives into account. - pass := reporter.NewPass(name, _pass) + var opts []reporter.PassOption + if warn { + opts = append(opts, reporter.Warn()) + } + + // Wrap _pass with reporter.Pass to take nolint directives into account and potentially + // warn instead of error. + pass := reporter.NewPass(name, _pass, opts...) for _, file := range pass.Files { // Ignore generated files and test files. From b0608aa7ea150ce2c02e91d6c11d6a0f26069cef Mon Sep 17 00:00:00 2001 From: Nirupama Date: Thu, 26 Jan 2023 16:39:52 -0800 Subject: [PATCH 9/9] feat: addressing comments --- internal/common/common.go | 2 +- internal/config/tiers.go | 9 ++- internal/errorlint/errorlint.go | 107 ++++++++++++++++++++------- internal/errorlint/errorlint_test.go | 95 ++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 30 deletions(-) create mode 100644 internal/errorlint/errorlint_test.go diff --git a/internal/common/common.go b/internal/common/common.go index e47cce5..8984539 100644 --- a/internal/common/common.go +++ b/internal/common/common.go @@ -74,7 +74,7 @@ func LoadOtherFilesIntoFset(pass *analysis.Pass) error { for _, fn := range pass.OtherFiles { content, err := os.ReadFile(fn) if err != nil { - return errors.Wrapf(err, "return file content for \"%s\"", fn) + return errors.Wrapf(err, "return file content for %s", fn) } tf := pass.Fset.AddFile(fn, -1, len(content)) diff --git a/internal/config/tiers.go b/internal/config/tiers.go index 7fdafc1..75d4099 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -65,7 +65,7 @@ func (l *Lintroller) ValidateTier() error { } default: log.Warn(context.Background(), - fmt.Sprintf("provided does not match any of the following: %q, %q, %q, %q (sans-quotes)", + fmt.Sprintf("Provided does not match any of the following: %q, %q, %q, %q (sans-quotes)", TierBronze, TierSilver, TierGold, TierPlatinum), log.F{ "tier": *l.Tier, @@ -116,7 +116,7 @@ func (l *Lintroller) EnsureMinimums(desired *Lintroller) error { //nolint:funlen if !found { return fmt.Errorf( - "deviation detected from tier minimum defaults in lintroller.header.fields, fields must contain \"%s\"", + "deviation detected from tier minimum defaults in lintroller.header.fields, fields must contain %s", desired.Header.Fields[i]) } } @@ -164,7 +164,7 @@ func (l *Lintroller) EnsureMinimums(desired *Lintroller) error { //nolint:funlen l.Doculint.MinFunLen = desired.Doculint.MinFunLen } else if l.Doculint.MinFunLen > desired.Doculint.MinFunLen || l.Doculint.MinFunLen < 0 { return fmt.Errorf( - "deviation detected from tier minimum defaults in lintroller.doculint.minfunlen, minfunlen must be set within (0, %d]", + "deviation detected from tier minimum defaults in lintroller.doculint.minfunlen, minfunlen must be set within 0 and %d", desired.Doculint.MinFunLen) } } @@ -206,7 +206,8 @@ var TierBronzeConfiguration = Lintroller{ Enabled: false, }, Errorlint: Errorlint{ - Enabled: false, + Enabled: true, + Warn: true, }, } diff --git a/internal/errorlint/errorlint.go b/internal/errorlint/errorlint.go index dd5d984..7c7c5e0 100644 --- a/internal/errorlint/errorlint.go +++ b/internal/errorlint/errorlint.go @@ -17,6 +17,7 @@ import ( "go/ast" "go/token" "strconv" + "strings" "unicode" "unicode/utf8" @@ -64,6 +65,42 @@ func init() { //nolint:gochecknoinits // Why: This is necessary to grab flags. "warn", false, "controls whether or not reports from this linter will result in errors or warnings") } +// file represents a file being linted. +type file struct { + f *ast.File + importSpecs map[*ast.File]map[*ast.Ident]string +} + +// ImportDecls creates map of import specs for a *ast.File +func (file *file) ImportDecls() { + for _, is := range file.f.Imports { + if is.Name == nil { + continue + } + arr := strings.Split(strings.Trim(is.Path.Value, "\""), "/") + pkgName := arr[len(arr)-1] + file.importSpecs = make(map[*ast.File]map[*ast.Ident]string) + file.importSpecs[file.f] = make(map[*ast.Ident]string) + file.importSpecs[file.f][is.Name] = pkgName + } +} + +func (file *file) walk(fn func(ast.Node) bool) { + ast.Walk(walker(fn), file.f) +} + +// walker adapts a function to satisfy the ast.Visitor interface. +// The function returns whether the walk should proceed into the node's children. +type walker func(ast.Node) bool + +func (w walker) Visit(node ast.Node) ast.Visitor { + if w(node) { + return w + } + + return nil +} + // errorlint defines linter for error/trace/log messages func errorlint(_pass *analysis.Pass) (interface{}, error) { // Ignore test packages. @@ -80,32 +117,33 @@ func errorlint(_pass *analysis.Pass) (interface{}, error) { // warn instead of error. pass := reporter.NewPass(name, _pass, opts...) - for _, file := range pass.Files { + for _, astFile := range pass.Files { // Ignore generated files and test files. - if common.IsGenerated(file) || common.IsTestFile(pass.Pass, file) { + if common.IsGenerated(astFile) || common.IsTestFile(pass.Pass, astFile) { continue } - lintMessageStrings(file, pass) + newFile := &file{f: astFile} + newFile.ImportDecls() + newFile.lintMessageStrings(pass) } return nil, nil } // lintMessageStrings examines error/trace/log message strings for capitalization and valid ending -func lintMessageStrings(file *ast.File, pass *reporter.Pass) { - ast.Inspect(file, func(node ast.Node) bool { +func (file *file) lintMessageStrings(pass *reporter.Pass) { + file.walk(func(node ast.Node) bool { call, ok := node.(*ast.CallExpr) if !ok { return true } - - if isNotErrorPackage(call.Fun) || len(call.Args) < 1 { + if file.isNotErrorPackage(call.Fun) || len(call.Args) < 1 { return true } msgIndex := 1 - if isDotInPkg(call.Fun, "errors", "New") || isDotInPkg(call.Fun, "fmt", "Errorf") || - isDotInPkg(call.Fun, "errors", "Errorf") { + if file.isDotInPkg(call.Fun, "errors", "New") || file.isDotInPkg(call.Fun, "fmt", "Errorf") || + file.isDotInPkg(call.Fun, "errors", "Errorf") { msgIndex = 0 } @@ -118,7 +156,7 @@ func lintMessageStrings(file *ast.File, pass *reporter.Pass) { if err != nil { return false } - pkgName := getPkgName(call.Fun) + pkgName := file.getPkgName(call.Fun) errormsg := getErrorMessages(msgString) for _, msg := range errormsg { pass.Reportf(node.Pos(), "%s "+msg, pkgName) @@ -128,13 +166,13 @@ func lintMessageStrings(file *ast.File, pass *reporter.Pass) { } // isNotErrorPackage checks if the ast.Expr package matches the error/fmt/trace/log packages for linter -func isNotErrorPackage(expr ast.Expr) bool { - return !isDotInPkg(expr, "errors", "New") && !isDotInPkg(expr, "errors", "Wrap") && - !isDotInPkg(expr, "errors", "Wrapf") && !isDotInPkg(expr, "log", "Warn") && - !isDotInPkg(expr, "log", "Info") && !isDotInPkg(expr, "log", "Error") && - !isDotInPkg(expr, "trace", "StartSpan") && !isDotInPkg(expr, "trace", "StartCall") && - !isDotInPkg(expr, "fmt", "Errorf") && !isDotInPkg(expr, "errors", "Errorf") && - !isDotInPkg(expr, "errors", "WithMessage") && !isDotInPkg(expr, "errors", "WithMessagef") +func (file *file) isNotErrorPackage(expr ast.Expr) bool { + return !file.isDotInPkg(expr, "errors", "New") && !file.isDotInPkg(expr, "errors", "Wrap") && + !file.isDotInPkg(expr, "errors", "Wrapf") && !file.isDotInPkg(expr, "log", "Warn") && + !file.isDotInPkg(expr, "log", "Info") && !file.isDotInPkg(expr, "log", "Error") && + !file.isDotInPkg(expr, "trace", "StartSpan") && !file.isDotInPkg(expr, "trace", "StartCall") && + !file.isDotInPkg(expr, "fmt", "Errorf") && !file.isDotInPkg(expr, "errors", "Errorf") && + !file.isDotInPkg(expr, "errors", "WithMessage") && !file.isDotInPkg(expr, "errors", "WithMessagef") } // getErrorMessages returns messages based on whether error message is empty, is capitalized, or punctuated @@ -155,25 +193,44 @@ func getErrorMessages(msg string) []string { } // isIdent checks if ident string is equal to ast.Ident name -func isIdent(expr ast.Expr, ident string) bool { +func (file *file) isIdent(expr ast.Expr, ident, identIdentifier string) (string, bool) { id, ok := expr.(*ast.Ident) - return ok && id.Name == ident + if !ok { + return "", false + } + originalPackage := "" + if identIdentifier == "pkg" { + pkgInfo := file.importSpecs[file.f] + for ident, pkg := range pkgInfo { + if id.Name == ident.Name { + originalPackage = pkg + break + } + } + } + return originalPackage, id.Name == ident } -// hasDotInPkg checks if pkg.function format is followed -func isDotInPkg(expr ast.Expr, pkg, name string) bool { +// isDotInPkg checks if pkg.function format is followed +func (file *file) isDotInPkg(expr ast.Expr, pkg, name string) bool { sel, ok := expr.(*ast.SelectorExpr) - return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) + if !ok { + return false + } + originalPackage, isPackageMatching := file.isIdent(sel.X, pkg, "pkg") + _, isFunctionMatching := file.isIdent(sel.Sel, name, "func") + return (originalPackage != "" || isPackageMatching) && isFunctionMatching } // getPkgName returns package name errors/fmt/log/trace -func getPkgName(expr ast.Expr) string { +func (file *file) getPkgName(expr ast.Expr) string { sel, ok := expr.(*ast.SelectorExpr) if !ok { return "" } for _, pkg := range []string{"errors", "log", "fmt", "trace"} { - if isIdent(sel.X, pkg) { + originalPackage, isPackageMatching := file.isIdent(sel.X, pkg, "pkg") + if originalPackage == pkg || isPackageMatching { return pkg } } @@ -195,5 +252,5 @@ func isUpperCase(msg string) bool { // hasPunctuation examines error/trace/log strings for ending in punctuation func hasPunctuation(msg string) bool { last, _ := utf8.DecodeLastRuneInString(msg) - return last == '.' || last == ':' || last == '!' || last == '\n' + return unicode.IsPunct(last) || last == '\n' } diff --git a/internal/errorlint/errorlint_test.go b/internal/errorlint/errorlint_test.go new file mode 100644 index 0000000..69789a6 --- /dev/null +++ b/internal/errorlint/errorlint_test.go @@ -0,0 +1,95 @@ +// Copyright 2023 Outreach Corporation. All Rights Reserved. + +package errorlint + +import ( + "go/ast" + "go/token" + "testing" + + "gotest.tools/v3/assert" +) + +func TestValidateErrorMessage(t *testing.T) { + msgTest := []struct { + name string + msg string + // The expected format string for the Reportf function argument. + expectedError []string + }{ + { + name: "empty message string", + msg: "", + expectedError: []string{"message should not be empty"}, + }, + { + name: "valid message string", + msg: "something good happened", + expectedError: nil, + }, + { + name: "uppercase message string", + msg: "Something bad happened", + expectedError: []string{"message should not be capitalized"}, + }, + { + name: "punctuated message string", + msg: "something bad happened.", + expectedError: []string{"message should not end with punctuation"}, + }, + { + name: "uppercase and punctuated message string", + msg: "Something bad happened.", + expectedError: []string{"message should not be capitalized", "message should not end with punctuation"}, + }, + } + + for _, test := range msgTest { + t.Run(test.name, func(t *testing.T) { + assert.DeepEqual(t, getErrorMessages(test.msg), test.expectedError) + }) + } +} + +type MockReporter struct { + lastFormat string +} + +func (r *MockReporter) Reportf(pos token.Pos, format string, args ...interface{}) { + r.lastFormat = format +} + +func TestImportDecl(t *testing.T) { + msgTest := []struct { + name string + path string + // The expected format string for the Reportf function argument. + expectedOriginalPackage string + }{ + { + name: "_errors", + path: "github.com/pkg/errors", + expectedOriginalPackage: "errors", + }, + { + name: "log", + path: "github.com/getoutreach/gobox/pkg/log", + expectedOriginalPackage: "log", + }, + } + + for _, test := range msgTest { + t.Run(test.name, func(t *testing.T) { + testFile := &ast.File{ + Imports: []*ast.ImportSpec{{ + Name: &ast.Ident{Name: test.name}, + Path: &ast.BasicLit{Value: test.path}, + }, + }, + } + file := &file{f: testFile} + file.ImportDecls() + assert.Equal(t, file.importSpecs[file.f][file.f.Imports[0].Name], test.expectedOriginalPackage) + }) + } +}