diff --git a/cmd/lintroller/lintroller.go b/cmd/lintroller/lintroller.go index 382dcea..379b181 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" @@ -60,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 @@ -88,5 +89,6 @@ func main() { ©right.Analyzer, &todo.Analyzer, &why.Analyzer, + &errorlint.Analyzer, ) } 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/config.go b/internal/config/config.go index 7fb652d..b159b46 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 @@ -76,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"` @@ -93,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. @@ -117,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"` @@ -157,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. @@ -168,9 +186,28 @@ 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. 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"` + + // 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. +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 360c197..75d4099 100644 --- a/internal/config/tiers.go +++ b/internal/config/tiers.go @@ -49,23 +49,23 @@ 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(), - 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) } } @@ -205,6 +205,10 @@ var TierBronzeConfiguration = Lintroller{ Why: Why{ Enabled: false, }, + Errorlint: Errorlint{ + Enabled: true, + Warn: true, + }, } // TierSilverConfiguration is the Lintroller configuration minumums that correspond @@ -234,6 +238,10 @@ var TierSilverConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + Enabled: true, + Warn: true, + }, } // TierGoldConfiguration is the Lintroller configuration minumums that correspond @@ -263,6 +271,10 @@ var TierGoldConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + Enabled: true, + Warn: true, + }, } // TierPlatinumConfiguration is the Lintroller configuration minumums that correspond @@ -292,4 +304,8 @@ var TierPlatinumConfiguration = Lintroller{ Why: Why{ Enabled: true, }, + Errorlint: Errorlint{ + 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 new file mode 100644 index 0000000..7c7c5e0 --- /dev/null +++ b/internal/errorlint/errorlint.go @@ -0,0 +1,256 @@ +// 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. 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 +package errorlint + +import ( + "go/ast" + "go/token" + "strconv" + "strings" + "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 error message starts with a lower-case letter and does not end in punctuation. +// Bad: +err := fmt.Errorf("Something bad happened.") +// Good: +err := fmt.Errorf("something bad happened")` + +// Analyzer exports the errorlint analyzer (linter). +var Analyzer = analysis.Analyzer{ + Name: name, + Doc: doc, + 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") +} + +// 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. + 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...) + + for _, astFile := range pass.Files { + // Ignore generated files and test files. + if common.IsGenerated(astFile) || common.IsTestFile(pass.Pass, astFile) { + continue + } + 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 (file *file) lintMessageStrings(pass *reporter.Pass) { + file.walk(func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + if file.isNotErrorPackage(call.Fun) || len(call.Args) < 1 { + return true + } + + msgIndex := 1 + if file.isDotInPkg(call.Fun, "errors", "New") || file.isDotInPkg(call.Fun, "fmt", "Errorf") || + file.isDotInPkg(call.Fun, "errors", "Errorf") { + 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 + } + pkgName := file.getPkgName(call.Fun) + errormsg := getErrorMessages(msgString) + for _, msg := range errormsg { + pass.Reportf(node.Pos(), "%s "+msg, pkgName) + } + return true + }) +} + +// isNotErrorPackage checks if the ast.Expr package matches the error/fmt/trace/log packages for linter +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 +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 +} + +// isIdent checks if ident string is equal to ast.Ident name +func (file *file) isIdent(expr ast.Expr, ident, identIdentifier string) (string, bool) { + id, ok := expr.(*ast.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 +} + +// isDotInPkg checks if pkg.function format is followed +func (file *file) isDotInPkg(expr ast.Expr, pkg, name string) bool { + sel, ok := expr.(*ast.SelectorExpr) + 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 (file *file) getPkgName(expr ast.Expr) string { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + return "" + } + for _, pkg := range []string{"errors", "log", "fmt", "trace"} { + originalPackage, isPackageMatching := file.isIdent(sel.X, pkg, "pkg") + if originalPackage == pkg || isPackageMatching { + return pkg + } + } + + return "" +} + +// isUpperCase examines error/trace/log strings for capitalization +func isUpperCase(msg string) bool { + for _, ch := range msg { + if unicode.IsUpper(ch) { + return true + } + } + + return false +} + +// hasPunctuation examines error/trace/log strings for ending in punctuation +func hasPunctuation(msg string) bool { + last, _ := utf8.DecodeLastRuneInString(msg) + 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) + }) + } +} 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/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)...) 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.