From 98dce265c6b388f39efd00e3746de5b1f5eb0e77 Mon Sep 17 00:00:00 2001 From: mgechev Date: Sun, 21 Jan 2018 18:04:41 -0800 Subject: [PATCH] Refactoring --- defaultrule/else-error.go | 116 ------------------ formatter/cli_formatter.go | 12 +- formatter/json_formatter.go | 6 +- {file => linter}/file.go | 21 ++-- {formatter => linter}/formatter.go | 6 +- linter/linter.go | 72 ++++++++--- linter/package.go | 77 ++++++++++++ {rule => linter}/rule.go | 8 +- main.go | 11 +- {defaultrule => rule}/argument-limit.go | 17 ++- {defaultrule => rule}/argument-limit_test.go | 2 +- {defaultrule => rule}/blank-imports.go | 17 ++- {defaultrule => rule}/blank-imports_test.go | 2 +- rule/else-error.go | 107 ++++++++++++++++ {defaultrule => rule}/exported.go | 31 +++-- {defaultrule => rule}/imports.go | 17 ++- {defaultrule => rule}/imports_test.go | 2 +- {defaultrule => rule}/names.go | 23 ++-- {defaultrule => rule}/no-else-return-rule.go | 17 ++- {defaultrule => rule}/package-comments.go | 25 ++-- .../package-comments_test.go | 2 +- {defaultrule => rule}/ranges.go | 15 ++- {defaultrule => rule}/utils.go | 6 +- {defaultrule => rule}/var-declarations.go | 17 ++- 24 files changed, 363 insertions(+), 266 deletions(-) delete mode 100644 defaultrule/else-error.go rename {file => linter}/file.go (57%) rename {formatter => linter}/formatter.go (72%) create mode 100644 linter/package.go rename {rule => linter}/rule.go (89%) rename {defaultrule => rule}/argument-limit.go (74%) rename {defaultrule => rule}/argument-limit_test.go (96%) rename {defaultrule => rule}/blank-imports.go (81%) rename {defaultrule => rule}/blank-imports_test.go (97%) create mode 100644 rule/else-error.go rename {defaultrule => rule}/exported.go (91%) rename {defaultrule => rule}/imports.go (69%) rename {defaultrule => rule}/imports_test.go (97%) rename {defaultrule => rule}/names.go (93%) rename {defaultrule => rule}/no-else-return-rule.go (80%) rename {defaultrule => rule}/package-comments.go (85%) rename {defaultrule => rule}/package-comments_test.go (96%) rename {defaultrule => rule}/ranges.go (75%) rename {defaultrule => rule}/utils.go (96%) rename {defaultrule => rule}/var-declarations.go (83%) diff --git a/defaultrule/else-error.go b/defaultrule/else-error.go deleted file mode 100644 index 0a44300..0000000 --- a/defaultrule/else-error.go +++ /dev/null @@ -1,116 +0,0 @@ -package defaultrule - -import ( - "go/ast" - "go/token" - "strings" - - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" -) - -// LintElseRule lints given else constructs. -type LintElseRule struct{} - -// Apply applies the rule to given file. -func (r *LintElseRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure - - onFailure := func(failure rule.Failure) { - failures = append(failures, failure) - } - - astFile := file.GetAST() - w := &lintElseError{astFile, onFailure} - ast.Walk(w, astFile) - return failures -} - -// Name returns the rule name. -func (r *LintElseRule) Name() string { - return "no-else-return" -} - -type lintElseError struct { - file *ast.File - onFailure func(rule.Failure) -} - -func (w *lintElseError) Visit(node ast.Node) ast.Visitor { - switch v := node.(type) { - case *ast.BlockStmt: - for i := 0; i < len(v.List)-1; i++ { - // if var := whatever; var != nil { return var } - s, ok := v.List[i].(*ast.IfStmt) - if !ok || s.Body == nil || len(s.Body.List) != 1 || s.Else != nil { - continue - } - assign, ok := s.Init.(*ast.AssignStmt) - if !ok || len(assign.Lhs) != 1 || !(assign.Tok == token.DEFINE || assign.Tok == token.ASSIGN) { - continue - } - id, ok := assign.Lhs[0].(*ast.Ident) - if !ok { - continue - } - expr, ok := s.Cond.(*ast.BinaryExpr) - if !ok || expr.Op != token.NEQ { - continue - } - if lhs, ok := expr.X.(*ast.Ident); !ok || lhs.Name != id.Name { - continue - } - if rhs, ok := expr.Y.(*ast.Ident); !ok || rhs.Name != "nil" { - continue - } - r, ok := s.Body.List[0].(*ast.ReturnStmt) - if !ok || len(r.Results) != 1 { - continue - } - if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != id.Name { - continue - } - - // return nil - r, ok = v.List[i+1].(*ast.ReturnStmt) - if !ok || len(r.Results) != 1 { - continue - } - if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != "nil" { - continue - } - - // check if there are any comments explaining the construct, don't emit an error if there are some. - if containsComments(w.file, s.Pos(), r.Pos()) { - continue - } - - w.onFailure(rule.Failure{ - Confidence: 0.9, - Failure: "redundant if ...; err != nil check, just return error instead.", - Node: v.List[i], - }) - } - } - return w -} - -func containsComments(f *ast.File, start, end token.Pos) bool { - for _, cgroup := range f.Comments { - comments := cgroup.List - if comments[0].Slash >= end { - // All comments starting with this group are after end pos. - return false - } - if comments[len(comments)-1].Slash < start { - // Comments group ends before start pos. - continue - } - for _, c := range comments { - if start <= c.Slash && c.Slash < end && !strings.HasPrefix(c.Text, "// MATCH ") { - return true - } - } - } - return false -} diff --git a/formatter/cli_formatter.go b/formatter/cli_formatter.go index 308ef0f..1ad8cf7 100644 --- a/formatter/cli_formatter.go +++ b/formatter/cli_formatter.go @@ -5,7 +5,7 @@ import ( "fmt" "github.com/fatih/color" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" "github.com/olekukonko/tablewriter" ) @@ -17,28 +17,28 @@ const ( // CLIFormatter is an implementation of the Formatter interface // which formats the errors to JSON. type CLIFormatter struct { - Metadata FormatterMetadata + Metadata linter.FormatterMetadata } -func formatFailure(failure rule.Failure) []string { +func formatFailure(failure linter.Failure) []string { fString := color.BlueString(failure.Failure) fTypeStr := string(failure.Type) fType := color.RedString(fTypeStr) lineColumn := failure.Position pos := fmt.Sprintf("(%d, %d)", lineColumn.Start.Line, lineColumn.Start.Column) - if failure.Type == rule.FailureTypeWarning { + if failure.Type == linter.FailureTypeWarning { fType = color.YellowString(fTypeStr) } return []string{failure.GetFilename(), pos, fType, fString} } // Format formats the failures gotten from the linter. -func (f *CLIFormatter) Format(failures []rule.Failure) (string, error) { +func (f *CLIFormatter) Format(failures []linter.Failure) (string, error) { var result [][]string var totalErrors = 0 for _, f := range failures { result = append(result, formatFailure(f)) - if f.Type == rule.FailureTypeError { + if f.Type == linter.FailureTypeError { totalErrors++ } } diff --git a/formatter/json_formatter.go b/formatter/json_formatter.go index ffc65dd..a1a3a03 100644 --- a/formatter/json_formatter.go +++ b/formatter/json_formatter.go @@ -3,17 +3,17 @@ package formatter import ( "encoding/json" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // JSONFormatter is an implementation of the Formatter interface // which formats the errors to JSON. type JSONFormatter struct { - Metadata FormatterMetadata + Metadata linter.FormatterMetadata } // Format formats the failures gotten from the linter. -func (f *JSONFormatter) Format(failures []rule.Failure) (string, error) { +func (f *JSONFormatter) Format(failures []linter.Failure) (string, error) { result, error := json.Marshal(failures) if error != nil { return "", error diff --git a/file/file.go b/linter/file.go similarity index 57% rename from file/file.go rename to linter/file.go index 0a8dfb0..ede6da9 100644 --- a/file/file.go +++ b/linter/file.go @@ -1,4 +1,4 @@ -package file +package linter import ( "go/ast" @@ -9,31 +9,38 @@ import ( // File abstraction used for representing files. type File struct { Name string - files *token.FileSet + pkg *Package Content []byte ast *ast.File } -// New creates a new file -func New(name string, content []byte, files *token.FileSet) (*File, error) { - f, err := parser.ParseFile(files, name, content, parser.ParseComments) +// NewFile creates a new file +func NewFile(name string, content []byte, pkg *Package) (*File, error) { + f, err := parser.ParseFile(pkg.Fset, name, content, parser.ParseComments) if err != nil { return nil, err } return &File{ Name: name, Content: content, - files: files, + pkg: pkg, ast: f, }, nil } // ToPosition returns line and column for given position. func (f *File) ToPosition(pos token.Pos) token.Position { - return f.files.Position(pos) + return f.pkg.Fset.Position(pos) } // GetAST returns the AST of the file func (f *File) GetAST() *ast.File { return f.ast } + +func (f *File) isMain() bool { + if f.GetAST().Name.Name == "main" { + return true + } + return false +} diff --git a/formatter/formatter.go b/linter/formatter.go similarity index 72% rename from formatter/formatter.go rename to linter/formatter.go index 4eb7773..209febe 100644 --- a/formatter/formatter.go +++ b/linter/formatter.go @@ -1,6 +1,4 @@ -package formatter - -import "github.com/mgechev/revive/rule" +package linter // FormatterMetadata configuration of a formatter type FormatterMetadata struct { @@ -11,5 +9,5 @@ type FormatterMetadata struct { // Formatter defines an interface for failure formatters type Formatter interface { - Format([]rule.Failure) string + Format([]Failure) string } diff --git a/linter/linter.go b/linter/linter.go index 59dd7ab..a4c7f93 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -1,20 +1,20 @@ package linter import ( + "bufio" + "bytes" + "fmt" "go/ast" "go/token" "math" "regexp" "strings" - - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" ) // ReadFile defines an abstraction for reading files. type ReadFile func(path string) (result []byte, err error) -type disabledIntervalsMap = map[string][]rule.DisabledInterval +type disabledIntervalsMap = map[string][]DisabledInterval // Linter is used for linting set of files. type Linter struct { @@ -26,26 +26,62 @@ func New(reader ReadFile) Linter { return Linter{reader: reader} } +var ( + genHdr = []byte("// Code generated ") + genFtr = []byte(" DO NOT EDIT.") +) + +// isGenerated reports whether the source file is generated code +// according the rules from https://golang.org/s/generatedcode. +// This is inherited from the original linter. +func isGenerated(src []byte) bool { + sc := bufio.NewScanner(bytes.NewReader(src)) + for sc.Scan() { + b := sc.Bytes() + if bytes.HasPrefix(b, genHdr) && bytes.HasSuffix(b, genFtr) && len(b) >= len(genHdr)+len(genFtr) { + return true + } + } + return false +} + // Lint lints a set of files with the specified rule. -func (l *Linter) Lint(filenames []string, ruleSet []rule.Rule, rulesConfig rule.RulesConfig) ([]rule.Failure, error) { - var fileSet token.FileSet - var failures []rule.Failure - var ruleNames = []string{} +func (l *Linter) Lint(filenames []string, ruleSet []Rule, rulesConfig RulesConfig) ([]Failure, error) { + var failures []Failure + ruleNames := []string{} for _, r := range ruleSet { ruleNames = append(ruleNames, r.Name()) } + pkg := &Package{ + Fset: token.NewFileSet(), + Files: map[string]*File{}, + } + var pkgName string for _, filename := range filenames { content, err := l.reader(filename) if err != nil { return nil, err } - file, err := file.New(filename, content, &fileSet) - disabledIntervals := l.disabledIntervals(file, ruleNames) + if isGenerated(content) { + continue + } + file, err := NewFile(filename, content, pkg) if err != nil { return nil, err } + if pkgName == "" { + pkgName = file.GetAST().Name.Name + } else if file.GetAST().Name.Name != pkgName { + return nil, fmt.Errorf("%s is in package %s, not %s", filename, file.GetAST().Name.Name, pkgName) + } + + pkg.Files[filename] = file + disabledIntervals := l.disabledIntervals(file, ruleNames) + + pkg.TypeCheck() + for _, currentRule := range ruleSet { config := rulesConfig[currentRule.Name()] currentFailures := currentRule.Apply(file, config) @@ -54,7 +90,7 @@ func (l *Linter) Lint(filenames []string, ruleSet []rule.Rule, rulesConfig rule. failure.RuleName = currentRule.Name() } if failure.Node != nil { - failure.Position = rule.ToFailurePosition(failure.Node.Pos(), failure.Node.End(), file) + failure.Position = ToFailurePosition(failure.Node.Pos(), failure.Node.End(), file) } currentFailures[idx] = failure } @@ -71,7 +107,7 @@ type enableDisableConfig struct { position int } -func (l *Linter) disabledIntervals(file *file.File, allRuleNames []string) disabledIntervalsMap { +func (l *Linter) disabledIntervals(file *File, allRuleNames []string) disabledIntervalsMap { re := regexp.MustCompile(`^\s*revive:(enable|disable)(?:-(line|next-line))?(:|\s|$)`) enabledDisabledRulesMap := make(map[string][]enableDisableConfig) @@ -80,9 +116,9 @@ func (l *Linter) disabledIntervals(file *file.File, allRuleNames []string) disab result := make(disabledIntervalsMap) for ruleName, disabledArr := range enabledDisabledRulesMap { - ruleResult := []rule.DisabledInterval{} + ruleResult := []DisabledInterval{} for i := 0; i < len(disabledArr); i++ { - interval := rule.DisabledInterval{ + interval := DisabledInterval{ RuleName: ruleName, From: token.Position{ Filename: file.Name, @@ -122,8 +158,8 @@ func (l *Linter) disabledIntervals(file *file.File, allRuleNames []string) disab enabledDisabledRulesMap[name] = existing } - handleRules := func(filename, modifier string, isEnabled bool, line int, ruleNames []string) []rule.DisabledInterval { - var result []rule.DisabledInterval + handleRules := func(filename, modifier string, isEnabled bool, line int, ruleNames []string) []DisabledInterval { + var result []DisabledInterval for _, name := range ruleNames { if modifier == "line" { handleConfig(isEnabled, line, name) @@ -172,8 +208,8 @@ func (l *Linter) disabledIntervals(file *file.File, allRuleNames []string) disab return getEnabledDisabledIntervals() } -func (l *Linter) filterFailures(failures []rule.Failure, disabledIntervals disabledIntervalsMap) []rule.Failure { - result := []rule.Failure{} +func (l *Linter) filterFailures(failures []Failure, disabledIntervals disabledIntervalsMap) []Failure { + result := []Failure{} for _, failure := range failures { fStart := failure.Position.Start.Line fEnd := failure.Position.End.Line diff --git a/linter/package.go b/linter/package.go new file mode 100644 index 0000000..9de032f --- /dev/null +++ b/linter/package.go @@ -0,0 +1,77 @@ +package linter + +import ( + "go/ast" + "go/token" + "go/types" + + "golang.org/x/tools/go/gcexportdata" +) + +// Package represents a package in the project. +type Package struct { + Fset *token.FileSet + Files map[string]*File + + TypesPkg *types.Package + TypesInfo *types.Info + + // sortable is the set of types in the package that implement sort.Interface. + Sortable map[string]bool + // main is whether this is a "main" package. + main int +} + +var newImporter = func(fset *token.FileSet) types.ImporterFrom { + return gcexportdata.NewImporter(fset, make(map[string]*types.Package)) +} + +var ( + trueValue = 1 + falseValue = 2 + notSet = 3 +) + +// IsMain returns if that's the main package. +func (p *Package) IsMain() bool { + if p.main == trueValue { + return true + } else if p.main == falseValue { + return false + } + for _, f := range p.Files { + if f.isMain() { + p.main = trueValue + return true + } + } + p.main = falseValue + return false +} + +// TypeCheck performs type checking for given package. +func (p *Package) TypeCheck() error { + config := &types.Config{ + // By setting a no-op error reporter, the type checker does as much work as possible. + Error: func(error) {}, + Importer: newImporter(p.Fset), + } + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Scopes: make(map[ast.Node]*types.Scope), + } + var anyFile *File + var astFiles []*ast.File + for _, f := range p.Files { + anyFile = f + astFiles = append(astFiles, f.GetAST()) + } + typesPkg, err := config.Check(anyFile.GetAST().Name.Name, p.Fset, astFiles, info) + // Remember the typechecking info, even if config.Check failed, + // since we will get partial information. + p.TypesPkg = typesPkg + p.TypesInfo = info + return err +} diff --git a/rule/rule.go b/linter/rule.go similarity index 89% rename from rule/rule.go rename to linter/rule.go index 66597d0..38141f9 100644 --- a/rule/rule.go +++ b/linter/rule.go @@ -1,10 +1,8 @@ -package rule +package linter import ( "go/ast" "go/token" - - "github.com/mgechev/revive/file" ) const ( @@ -60,7 +58,7 @@ type RulesConfig = map[string]Arguments // Rule defines an abstract rule interaface type Rule interface { Name() string - Apply(*file.File, Arguments) []Failure + Apply(*File, Arguments) []Failure } // AbstractRule defines an abstract rule. @@ -69,7 +67,7 @@ type AbstractRule struct { } // ToFailurePosition returns the failure position. -func ToFailurePosition(start token.Pos, end token.Pos, file *file.File) FailurePosition { +func ToFailurePosition(start token.Pos, end token.Pos, file *File) FailurePosition { return FailurePosition{ Start: file.ToPosition(start), End: file.ToPosition(end), diff --git a/main.go b/main.go index 9c66e75..7ae7c22 100644 --- a/main.go +++ b/main.go @@ -3,7 +3,6 @@ package main import ( "fmt" - "github.com/mgechev/revive/defaultrule" "github.com/mgechev/revive/formatter" "github.com/mgechev/revive/linter" "github.com/mgechev/revive/rule" @@ -26,17 +25,17 @@ func main() { } ` - linter := linter.New(func(file string) ([]byte, error) { + revive := linter.New(func(file string) ([]byte, error) { return []byte(src), nil }) - var result []rule.Rule - result = append(result, &defaultrule.LintElseRule{}, &defaultrule.ArgumentsLimitRule{}) + var result []linter.Rule + result = append(result, &rule.LintElseRule{}, &rule.ArgumentsLimitRule{}) - var config = rule.RulesConfig{ + var config = linter.RulesConfig{ "argument-limit": []string{"3"}, } - failures, err := linter.Lint([]string{"foo.go", "bar.go", "baz.go"}, result, config) + failures, err := revive.Lint([]string{"foo.go", "bar.go", "baz.go"}, result, config) if err != nil { panic(err) } diff --git a/defaultrule/argument-limit.go b/rule/argument-limit.go similarity index 74% rename from defaultrule/argument-limit.go rename to rule/argument-limit.go index ebd10a7..d607845 100644 --- a/defaultrule/argument-limit.go +++ b/rule/argument-limit.go @@ -1,19 +1,18 @@ -package defaultrule +package rule import ( "fmt" "go/ast" "strconv" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // ArgumentsLimitRule lints given else constructs. type ArgumentsLimitRule struct{} // Apply applies the rule to given file. -func (r *ArgumentsLimitRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { +func (r *ArgumentsLimitRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { if len(arguments) != 1 { panic(`invalid configuration for "argument-limit"`) } @@ -22,11 +21,11 @@ func (r *ArgumentsLimitRule) Apply(file *file.File, arguments rule.Arguments) [] panic(`invalid configuration for "argument-limit"`) } - var failures []rule.Failure + var failures []linter.Failure walker := lintArgsNum{ total: total, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, } @@ -43,7 +42,7 @@ func (r *ArgumentsLimitRule) Name() string { type lintArgsNum struct { total int64 - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w lintArgsNum) Visit(n ast.Node) ast.Visitor { @@ -51,9 +50,9 @@ func (w lintArgsNum) Visit(n ast.Node) ast.Visitor { if ok { num := int64(len(node.Type.Params.List)) if num > w.total { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("maximum number of arguments per function exceeded; max %d but got %d", w.total, num), - Type: rule.FailureTypeWarning, + Type: linter.FailureTypeWarning, Node: node.Type, }) return w diff --git a/defaultrule/argument-limit_test.go b/rule/argument-limit_test.go similarity index 96% rename from defaultrule/argument-limit_test.go rename to rule/argument-limit_test.go index bfbf688..e0b980e 100644 --- a/defaultrule/argument-limit_test.go +++ b/rule/argument-limit_test.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "testing" diff --git a/defaultrule/blank-imports.go b/rule/blank-imports.go similarity index 81% rename from defaultrule/blank-imports.go rename to rule/blank-imports.go index 62a98f9..dd0af35 100644 --- a/defaultrule/blank-imports.go +++ b/rule/blank-imports.go @@ -1,24 +1,23 @@ -package defaultrule +package rule import ( "go/ast" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // BlankImportsRule lints given else constructs. type BlankImportsRule struct{} // Apply applies the rule to given file. -func (r *BlankImportsRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *BlankImportsRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure fileAst := file.GetAST() walker := lintBlankImports{ file: file, fileAst: fileAst, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, } @@ -35,8 +34,8 @@ func (r *BlankImportsRule) Name() string { type lintBlankImports struct { fileAst *ast.File - file *file.File - onFailure func(rule.Failure) + file *linter.File + onFailure func(linter.Failure) } func (w lintBlankImports) Visit(n ast.Node) ast.Visitor { @@ -63,7 +62,7 @@ func (w lintBlankImports) Visit(n ast.Node) ast.Visitor { // This is the first blank import of a group. if imp.Doc == nil && imp.Comment == nil { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: imp, Failure: "a blank import should be only in a main or test package, or have a comment justifying it", Confidence: 1, diff --git a/defaultrule/blank-imports_test.go b/rule/blank-imports_test.go similarity index 97% rename from defaultrule/blank-imports_test.go rename to rule/blank-imports_test.go index 0191d19..29d44ae 100644 --- a/defaultrule/blank-imports_test.go +++ b/rule/blank-imports_test.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "testing" diff --git a/rule/else-error.go b/rule/else-error.go new file mode 100644 index 0000000..d062ee2 --- /dev/null +++ b/rule/else-error.go @@ -0,0 +1,107 @@ +package rule + +// // LintElseRule lints given else constructs. +// type LintElseRule struct{} + +// // Apply applies the rule to given file. +// func (r *LintElseRule) Apply(file *project.File, arguments rule.Arguments) []rule.Failure { +// var failures []rule.Failure + +// onFailure := func(failure rule.Failure) { +// failures = append(failures, failure) +// } + +// astFile := file.GetAST() +// w := &lintElseError{astFile, onFailure} +// ast.Walk(w, astFile) +// return failures +// } + +// // Name returns the rule name. +// func (r *LintElseRule) Name() string { +// return "no-else-return" +// } + +// type lintElseError struct { +// file *ast.File +// onFailure func(rule.Failure) +// } + +// func (w *lintElseError) Visit(node ast.Node) ast.Visitor { +// switch v := node.(type) { +// case *ast.BlockStmt: +// for i := 0; i < len(v.List)-1; i++ { +// // if var := whatever; var != nil { return var } +// s, ok := v.List[i].(*ast.IfStmt) +// if !ok || s.Body == nil || len(s.Body.List) != 1 || s.Else != nil { +// continue +// } +// assign, ok := s.Init.(*ast.AssignStmt) +// if !ok || len(assign.Lhs) != 1 || !(assign.Tok == token.DEFINE || assign.Tok == token.ASSIGN) { +// continue +// } +// id, ok := assign.Lhs[0].(*ast.Ident) +// if !ok { +// continue +// } +// expr, ok := s.Cond.(*ast.BinaryExpr) +// if !ok || expr.Op != token.NEQ { +// continue +// } +// if lhs, ok := expr.X.(*ast.Ident); !ok || lhs.Name != id.Name { +// continue +// } +// if rhs, ok := expr.Y.(*ast.Ident); !ok || rhs.Name != "nil" { +// continue +// } +// r, ok := s.Body.List[0].(*ast.ReturnStmt) +// if !ok || len(r.Results) != 1 { +// continue +// } +// if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != id.Name { +// continue +// } + +// // return nil +// r, ok = v.List[i+1].(*ast.ReturnStmt) +// if !ok || len(r.Results) != 1 { +// continue +// } +// if r, ok := r.Results[0].(*ast.Ident); !ok || r.Name != "nil" { +// continue +// } + +// // check if there are any comments explaining the construct, don't emit an error if there are some. +// if containsComments(w.file, s.Pos(), r.Pos()) { +// continue +// } + +// w.onFailure(rule.Failure{ +// Confidence: 0.9, +// Failure: "redundant if ...; err != nil check, just return error instead.", +// Node: v.List[i], +// }) +// } +// } +// return w +// } + +// func containsComments(f *ast.File, start, end token.Pos) bool { +// for _, cgroup := range f.Comments { +// comments := cgroup.List +// if comments[0].Slash >= end { +// // All comments starting with this group are after end pos. +// return false +// } +// if comments[len(comments)-1].Slash < start { +// // Comments group ends before start pos. +// continue +// } +// for _, c := range comments { +// if start <= c.Slash && c.Slash < end && !strings.HasPrefix(c.Text, "// MATCH ") { +// return true +// } +// } +// } +// return false +// } diff --git a/defaultrule/exported.go b/rule/exported.go similarity index 91% rename from defaultrule/exported.go rename to rule/exported.go index 01dbb82..c78e18a 100644 --- a/defaultrule/exported.go +++ b/rule/exported.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "fmt" @@ -8,16 +8,15 @@ import ( "unicode" "unicode/utf8" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // ExportedRule lints given else constructs. type ExportedRule struct{} // Apply applies the rule to given file. -func (r *ExportedRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *ExportedRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure if isTest(file) { return failures @@ -27,7 +26,7 @@ func (r *ExportedRule) Apply(file *file.File, arguments rule.Arguments) []rule.F walker := lintExported{ file: file, fileAst: fileAst, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, genDeclMissingComments: make(map[*ast.GenDecl]bool), @@ -44,11 +43,11 @@ func (r *ExportedRule) Name() string { } type lintExported struct { - file *file.File + file *linter.File fileAst *ast.File lastGen *ast.GenDecl genDeclMissingComments map[*ast.GenDecl]bool - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { @@ -79,7 +78,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { name = recv + "." + name } if fn.Doc == nil { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: fn, Failure: fmt.Sprintf("exported %s %s should have comment or be unexported", kind, name), Confidence: 1, @@ -89,7 +88,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { s := fn.Doc.Text() prefix := fn.Name.Name + " " if !strings.HasPrefix(s, prefix) { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: fn.Doc, Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), Confidence: 1, @@ -118,7 +117,7 @@ func (w *lintExported) checkStutter(id *ast.Ident, thing string) { // the it's starting a new word and thus this name stutters. rem := name[len(pkg):] if next, _ := utf8.DecodeRuneInString(rem); next == '_' || unicode.IsUpper(next) { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: id, Failure: fmt.Sprintf("%s name will be used as %s.%s by other packages, and that stutters; consider calling this %s", thing, pkg, name, rem), Confidence: 0.8, @@ -131,7 +130,7 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { return } if doc == nil { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: t, Failure: fmt.Sprintf("exported type %v should have comment or be unexported", t.Name), Confidence: 1, @@ -148,7 +147,7 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { } } if !strings.HasPrefix(s, t.Name.Name+" ") { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: doc, Failure: fmt.Sprintf(`comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name), Confidence: 1, @@ -166,7 +165,7 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD // Check that none are exported except for the first. for _, n := range vs.Names[1:] { if ast.IsExported(n.Name) { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: vs, Failure: fmt.Sprintf("exported %s %s should have its own declaration", kind, n.Name), Confidence: 1, @@ -190,7 +189,7 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD if kind == "const" && gd.Lparen.IsValid() { block = " (or a comment on this block)" } - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: vs, Failure: fmt.Sprintf("exported %s %s should have comment%s or be unexported", kind, name, block), Confidence: 1, @@ -210,7 +209,7 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD } prefix := name + " " if !strings.HasPrefix(doc.Text(), prefix) { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Node: doc, Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), Confidence: 1, diff --git a/defaultrule/imports.go b/rule/imports.go similarity index 69% rename from defaultrule/imports.go rename to rule/imports.go index 66e8649..5ce411b 100644 --- a/defaultrule/imports.go +++ b/rule/imports.go @@ -1,24 +1,23 @@ -package defaultrule +package rule import ( "go/ast" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // ImportsRule lints given else constructs. type ImportsRule struct{} // Apply applies the rule to given file. -func (r *ImportsRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *ImportsRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure fileAst := file.GetAST() walker := lintImports{ file: file, fileAst: fileAst, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, } @@ -34,15 +33,15 @@ func (r *ImportsRule) Name() string { } type lintImports struct { - file *file.File + file *linter.File fileAst *ast.File - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w lintImports) Visit(n ast.Node) ast.Visitor { for _, is := range w.fileAst.Imports { if is.Name != nil && is.Name.Name == "." && !isTest(w.file) { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Confidence: 1, Failure: "should not use dot imports", Node: is, diff --git a/defaultrule/imports_test.go b/rule/imports_test.go similarity index 97% rename from defaultrule/imports_test.go rename to rule/imports_test.go index 1ee8df8..112a296 100644 --- a/defaultrule/imports_test.go +++ b/rule/imports_test.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "testing" diff --git a/defaultrule/names.go b/rule/names.go similarity index 93% rename from defaultrule/names.go rename to rule/names.go index eb02cb6..c579dd6 100644 --- a/defaultrule/names.go +++ b/rule/names.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "fmt" @@ -7,16 +7,15 @@ import ( "strings" "unicode" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // NamesRule lints given else constructs. type NamesRule struct{} // Apply applies the rule to given file. -func (r *NamesRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *NamesRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure if isTest(file) { return failures @@ -26,7 +25,7 @@ func (r *NamesRule) Apply(file *file.File, arguments rule.Arguments) []rule.Fail walker := lintNames{ file: file, fileAst: fileAst, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, } @@ -42,11 +41,11 @@ func (r *NamesRule) Name() string { } type lintNames struct { - file *file.File + file *linter.File fileAst *ast.File lastGen *ast.GenDecl genDeclMissingComments map[*ast.GenDecl]bool - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w *lintNames) Visit(n ast.Node) ast.Visitor { @@ -149,7 +148,7 @@ func (w *lintNames) check(id *ast.Ident, thing string) { // Handle two common styles from other languages that don't belong in Go. if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("don't use ALL_CAPS in Go names; use CamelCase"), Confidence: 0.8, Node: id, @@ -158,7 +157,7 @@ func (w *lintNames) check(id *ast.Ident, thing string) { } if len(id.Name) > 2 && id.Name[0] == 'k' && id.Name[1] >= 'A' && id.Name[1] <= 'Z' { should := string(id.Name[1]+'a'-'A') + id.Name[2:] - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("don't use leading k in Go names; %s %s should be %s", thing, id.Name, should), Confidence: 0.8, Node: id, @@ -171,14 +170,14 @@ func (w *lintNames) check(id *ast.Ident, thing string) { } if len(id.Name) > 2 && strings.Contains(id.Name[1:], "_") { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("don't use underscores in Go names; %s %s should be %s", thing, id.Name, should), Confidence: 0.9, Node: id, }) return } - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("%s %s should be %s", thing, id.Name, should), Confidence: 0.8, Node: id, diff --git a/defaultrule/no-else-return-rule.go b/rule/no-else-return-rule.go similarity index 80% rename from defaultrule/no-else-return-rule.go rename to rule/no-else-return-rule.go index 1377082..f789863 100644 --- a/defaultrule/no-else-return-rule.go +++ b/rule/no-else-return-rule.go @@ -1,21 +1,20 @@ -package defaultrule +package rule import ( "go/ast" "go/token" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // LintElseRule lints given else constructs. type LintElseRule struct{} // Apply applies the rule to given file. -func (r *LintElseRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *LintElseRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure - onFailure := func(failure rule.Failure) { + onFailure := func(failure linter.Failure) { failures = append(failures, failure) } @@ -31,7 +30,7 @@ func (r *LintElseRule) Name() string { type lintElse struct { ignore map[*ast.IfStmt]bool - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w lintElse) Visit(node ast.Node) ast.Visitor { @@ -65,9 +64,9 @@ func (w lintElse) Visit(node ast.Node) ast.Visitor { if shortDecl { extra = " (move short variable declaration to its own line if necessary)" } - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: "if block ends with a return statement, so drop this else and outdent its block" + extra, - Type: rule.FailureTypeWarning, + Type: linter.FailureTypeWarning, Node: ifStmt.Else, }) } diff --git a/defaultrule/package-comments.go b/rule/package-comments.go similarity index 85% rename from defaultrule/package-comments.go rename to rule/package-comments.go index 821bde4..abbbc0c 100644 --- a/defaultrule/package-comments.go +++ b/rule/package-comments.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "fmt" @@ -6,8 +6,7 @@ import ( "go/token" "strings" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // PackageCommentsRule lints the package comments. It complains if @@ -18,14 +17,14 @@ import ( type PackageCommentsRule struct{} // Apply applies the rule to given file. -func (r *PackageCommentsRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *PackageCommentsRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure if isTest(file) { return failures } - onFailure := func(failure rule.Failure) { + onFailure := func(failure linter.Failure) { failures = append(failures, failure) } @@ -42,8 +41,8 @@ func (r *PackageCommentsRule) Name() string { type lintPackageComments struct { fileAst *ast.File - file *file.File - onFailure func(rule.Failure) + file *linter.File + onFailure func(linter.Failure) } func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { @@ -73,17 +72,17 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { Line: endPos.Line + 1, Column: 1, } - l.onFailure(rule.Failure{ + l.onFailure(linter.Failure{ Failure: "package comment is detached; there should be no blank lines between it and the package statement", Confidence: 0.9, - Position: rule.FailurePosition{Start: pos}, + Position: linter.FailurePosition{Start: pos}, }) return nil } } if l.fileAst.Doc == nil { - l.onFailure(rule.Failure{ + l.onFailure(linter.Failure{ Failure: "should have a package comment, unless it's in another file for this package", Confidence: 0.2, Node: l.fileAst.Name, @@ -92,7 +91,7 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { } s := l.fileAst.Doc.Text() if ts := strings.TrimLeft(s, " \t"); ts != s { - l.onFailure(rule.Failure{ + l.onFailure(linter.Failure{ Failure: "package comment should not have leading space", Confidence: 1, Node: l.fileAst.Doc, @@ -101,7 +100,7 @@ func (l *lintPackageComments) Visit(n ast.Node) ast.Visitor { } // Only non-main packages need to keep to this form. if l.fileAst.Name.Name != "main" && !strings.HasPrefix(s, prefix) { - l.onFailure(rule.Failure{ + l.onFailure(linter.Failure{ Failure: fmt.Sprintf(`package comment should be of the form "%s..."`, prefix), Confidence: 1, Node: l.fileAst.Doc, diff --git a/defaultrule/package-comments_test.go b/rule/package-comments_test.go similarity index 96% rename from defaultrule/package-comments_test.go rename to rule/package-comments_test.go index 868e098..fccfaa6 100644 --- a/defaultrule/package-comments_test.go +++ b/rule/package-comments_test.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "testing" diff --git a/defaultrule/ranges.go b/rule/ranges.go similarity index 75% rename from defaultrule/ranges.go rename to rule/ranges.go index 9ba29ad..b3960f9 100644 --- a/defaultrule/ranges.go +++ b/rule/ranges.go @@ -1,21 +1,20 @@ -package defaultrule +package rule import ( "fmt" "go/ast" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // LintRangesRule lints given else constructs. type LintRangesRule struct{} // Apply applies the rule to given file. -func (r *LintRangesRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *LintRangesRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure - onFailure := func(failure rule.Failure) { + onFailure := func(failure linter.Failure) { failures = append(failures, failure) } @@ -32,7 +31,7 @@ func (r *LintRangesRule) Name() string { type lintRanges struct { file *ast.File - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w *lintRanges) Visit(node ast.Node) ast.Visitor { @@ -49,7 +48,7 @@ func (w *lintRanges) Visit(node ast.Node) ast.Visitor { return w } - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Failure: fmt.Sprintf("should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", render(rs.Key), rs.Tok), Confidence: 1, Node: rs.Value, diff --git a/defaultrule/utils.go b/rule/utils.go similarity index 96% rename from defaultrule/utils.go rename to rule/utils.go index 29508f2..42309a6 100644 --- a/defaultrule/utils.go +++ b/rule/utils.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "fmt" @@ -7,7 +7,7 @@ import ( "regexp" "strings" - "github.com/mgechev/revive/file" + "github.com/mgechev/revive/linter" ) const styleGuideBase = "https://golang.org/wiki/CodeReviewComments" @@ -16,7 +16,7 @@ const styleGuideBase = "https://golang.org/wiki/CodeReviewComments" // If id == nil, the answer is false. func isBlank(id *ast.Ident) bool { return id != nil && id.Name == "_" } -func isTest(f *file.File) bool { +func isTest(f *linter.File) bool { return strings.HasSuffix(f.Name, "_test.go") } diff --git a/defaultrule/var-declarations.go b/rule/var-declarations.go similarity index 83% rename from defaultrule/var-declarations.go rename to rule/var-declarations.go index 6217ddc..1570c19 100644 --- a/defaultrule/var-declarations.go +++ b/rule/var-declarations.go @@ -1,4 +1,4 @@ -package defaultrule +package rule import ( "bytes" @@ -7,22 +7,21 @@ import ( "go/printer" "go/token" - "github.com/mgechev/revive/file" - "github.com/mgechev/revive/rule" + "github.com/mgechev/revive/linter" ) // VarDeclarationsRule lints given else constructs. type VarDeclarationsRule struct{} // Apply applies the rule to given file. -func (r *VarDeclarationsRule) Apply(file *file.File, arguments rule.Arguments) []rule.Failure { - var failures []rule.Failure +func (r *VarDeclarationsRule) Apply(file *linter.File, arguments linter.Arguments) []linter.Failure { + var failures []linter.Failure fileAst := file.GetAST() walker := &lintVarDeclarations{ file: file, fileAst: fileAst, - onFailure: func(failure rule.Failure) { + onFailure: func(failure linter.Failure) { failures = append(failures, failure) }, } @@ -39,9 +38,9 @@ func (r *VarDeclarationsRule) Name() string { type lintVarDeclarations struct { fileAst *ast.File - file *file.File + file *linter.File lastGen *ast.GenDecl - onFailure func(rule.Failure) + onFailure func(linter.Failure) } func (w *lintVarDeclarations) Visit(node ast.Node) ast.Visitor { @@ -73,7 +72,7 @@ func (w *lintVarDeclarations) Visit(node ast.Node) ast.Visitor { zero = true } if zero { - w.onFailure(rule.Failure{ + w.onFailure(linter.Failure{ Confidence: 0.9, Node: rhs, Failure: fmt.Sprintf("should drop = %s from declaration of var %s; it is the zero value", render(rhs), v.Names[0]),