diff --git a/README.md b/README.md index 7b84ee5..a8fb0d1 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,13 @@ good, making it harder for other people to read and understand. The linter will warn about newlines in and around blocks, in the beginning of files and other places in the code. +**I know this linter is aggressive** and a lot of projects I've tested it on +have failed miserably. For this linter to be useful at all I want to be open to +new ideas, configurations and discussions! Also note that some of the warnings +might be bugs or unintentional false positives so I would love an +[issue](https://github.com/bombsimon/wsl/issues/new) to fix, discuss, change or +make something configurable! + ## Usage Install by using `go get -u github.com/bombsimon/wsl/cmd/...`. @@ -26,6 +33,9 @@ By default, the linter will run on `./...` which means all go files in the current path and all subsequent paths, including test files. To disable linting test files, use `-n` or `--no-test`. +This linter can also be used as a part of +[golangci-lint](https://github.com/golangci/golangci-lint) + ## Rules Note that this linter doesn't take in consideration the issues that will be @@ -335,6 +345,11 @@ fmt.Println(a) foo := true someFunc(false) + +if someBool() { + fmt.Println("doing things") +} +x.Calling() ``` **Do** @@ -350,6 +365,12 @@ someFunc(foo) bar := false someFunc(true) + +if someBool() { + fmt.Println("doing things") +} + +x.Calling() ``` #### Ranges diff --git a/cmd/wsl/main.go b/cmd/wsl/main.go index 07cd456..dda3fed 100644 --- a/cmd/wsl/main.go +++ b/cmd/wsl/main.go @@ -12,27 +12,26 @@ import ( func main() { var ( - args []string - help bool - notest bool - warnings bool - cwd, _ = os.Getwd() - files = []string{} - finalFiles = []string{} + args []string + help bool + noTest bool + showWarnings bool + cwd, _ = os.Getwd() + files = []string{} + finalFiles = []string{} ) flag.BoolVar(&help, "h", false, "Show this help text") flag.BoolVar(&help, "help", false, "") - flag.BoolVar(¬est, "n", false, "Don't lint test files") - flag.BoolVar(¬est, "no-test", false, "") - flag.BoolVar(&warnings, "w", false, "Show warnings (ignored rules)") - flag.BoolVar(&warnings, "warnings", false, "") + flag.BoolVar(&noTest, "n", false, "Don't lint test files") + flag.BoolVar(&noTest, "no-test", false, "") + flag.BoolVar(&showWarnings, "w", false, "Show warnings (ignored rules)") + flag.BoolVar(&showWarnings, "warnings", false, "") flag.Parse() if help { showHelp() - return } @@ -57,7 +56,7 @@ func main() { // Use relative path to print shorter names, sort out test files if chosen. for _, f := range files { - if notest { + if noTest { if strings.HasSuffix(f, "_test.go") { continue } @@ -72,26 +71,25 @@ func main() { finalFiles = append(finalFiles, f) } - r, w := wsl.ProcessFiles(finalFiles) + processor := wsl.NewProcessor() + result, warnings := processor.ProcessFiles(finalFiles) - for _, x := range r { - fmt.Printf("%s:%d: %s\n", x.FileName, x.LineNumber, x.Reason) + for _, r := range result { + fmt.Println(r.String()) } - if warnings && len(w) > 0 { + if showWarnings && len(warnings) > 0 { fmt.Println() fmt.Println("⚠️ Warnings found") - for _, x := range w { - fmt.Println(x) + for _, w := range warnings { + fmt.Println(w) } } - if len(r) > 0 { + if len(result) > 0 { os.Exit(2) } - - return } func expandGoWildcard(root string) []string { diff --git a/go.mod b/go.mod index 20b3877..bce35c3 100644 --- a/go.mod +++ b/go.mod @@ -1,7 +1,7 @@ module github.com/bombsimon/wsl require ( - github.com/davecgh/go-spew v1.1.1 + github.com/davecgh/go-spew v1.1.1 // indirect github.com/kr/pretty v0.1.0 // indirect github.com/stretchr/testify v1.4.0 gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect diff --git a/go.sum b/go.sum index 1033a6c..042e09b 100644 --- a/go.sum +++ b/go.sum @@ -18,7 +18,5 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogR gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v2 v2.2.3 h1:fvjTMHxHEw/mxHbtzPi3JCcKXQRAnQTBRo6YCJSVHKI= -gopkg.in/yaml.v2 v2.2.3/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/wsl.go b/wsl.go index 7b74acb..89ea4d2 100644 --- a/wsl.go +++ b/wsl.go @@ -9,26 +9,78 @@ import ( "reflect" ) -type result struct { +type Configuration struct { + // CheckAppend will do special handling when assigning from append (x = + // append(x, y)). If this is set to true the append call must append either + // a variable assigned on the line above or a variable called on the line + // above. + CheckAppend bool + + // AllowAssignAndCallsCuddle allows assignments to be cuddled with variables + // used in calls on line above and calls to be cuddled with assignments of + // variables used in call on line above. + // Example supported with this set to true: + // x.Call() + // y = Assign() + // x.AnotherCall() + // y = AnotherAssign() + AllowAssignAndCallsCuddle bool + + // AllowCuddleWithCalls is a list of call idents that everything can be + // cuddled with. Defaults to calls looking like locks to support a flow like + // this: + // mu.Lock() + // allow := thisAssignment + AllowCuddleWithCalls []string + + // AllowCuddleWithRHS is a list of right hand side variables that is allowed + // to be cuddled with anything. Defaults to assignments or calls looking + // like unlocks to support a flow like this: + // allow := thisAssignment() + // mu.Unlock() + AllowCuddleWithRHS []string +} + +type Result struct { FileName string LineNumber int Position token.Position Reason string } -type processor struct { - result []result +func (r *Result) String() string { + return fmt.Sprintf("%s:%d: %s", r.FileName, r.LineNumber, r.Reason) +} + +type Processor struct { + config Configuration + result []Result warnings []string fileSet *token.FileSet file *ast.File } +// NewProcessor will create a Processor. +func NewProcessorWithConfig(cfg Configuration) *Processor { + return &Processor{ + result: []Result{}, + config: cfg, + } +} + +// NewProcessor will create a Processor. +func NewProcessor() *Processor { + return NewProcessorWithConfig(Configuration{ + CheckAppend: true, + AllowAssignAndCallsCuddle: true, + AllowCuddleWithCalls: []string{"Lock", "RLock", "RWLock"}, + AllowCuddleWithRHS: []string{"Unlock", "RUnlock", "RWUnlock"}, + }) +} + // ProcessFiles takes a string slice with file names (full paths) and lints // them. -func ProcessFiles(filenames []string) ([]result, []string) { - p := NewProcessor() - - // Iterate over all files. +func (p *Processor) ProcessFiles(filenames []string) ([]Result, []string) { for _, filename := range filenames { data, err := ioutil.ReadFile(filename) if err != nil { @@ -41,20 +93,13 @@ func ProcessFiles(filenames []string) ([]result, []string) { return p.result, p.warnings } -// NewProcessor will create a processor. -func NewProcessor() *processor { - return &processor{ - result: []result{}, - } -} - -func (p *processor) process(filename string, data []byte) { +func (p *Processor) process(filename string, data []byte) { fileSet := token.NewFileSet() file, err := parser.ParseFile(fileSet, filename, data, parser.ParseComments) // If the file is not parsable let's add a syntax error and move on. if err != nil { - p.result = append(p.result, result{ + p.result = append(p.result, Result{ FileName: filename, LineNumber: 0, Reason: fmt.Sprintf("invalid syntax, file cannot be linted (%s)", err.Error()), @@ -81,7 +126,7 @@ func (p *processor) process(filename string, data []byte) { // parseBlockBody will parse any kind of block statements such as switch cases // and if statements. A list of Result is returned. -func (p *processor) parseBlockBody(block *ast.BlockStmt) { +func (p *Processor) parseBlockBody(block *ast.BlockStmt) { // Nothing to do if there's no value. if reflect.ValueOf(block).IsNil() { return @@ -96,7 +141,7 @@ func (p *processor) parseBlockBody(block *ast.BlockStmt) { // parseBlockStatements will parse all the statements found in the body of a // node. A list of Result is returned. -func (p *processor) parseBlockStatements(statements []ast.Stmt) { +func (p *Processor) parseBlockStatements(statements []ast.Stmt) { for i, stmt := range statements { // TODO: How to tell when and where func literals may exist to enforce // linting. @@ -128,10 +173,15 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { // made over multiple lines we should not allow cuddling. var assignedOnLineAbove []string + // We want to keep track of what was called on the line above to support + // special handling of things such as mutexes. + var calledOnLineAbove []string + // Ensure previous line is not a multi line assignment and if not get - // all assigned variables. + // rightAndLeftHandSide assigned variables. if p.nodeStart(previousStatement) == p.nodeStart(stmt)-1 { - assignedOnLineAbove = p.findLhs(previousStatement) + assignedOnLineAbove = p.findLHS(previousStatement) + calledOnLineAbove = p.findRHS(previousStatement) } // We could potentially have a block which require us to check the first @@ -139,32 +189,44 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { var assignedFirstInBlock []string if firstBodyStatement != nil { - assignedFirstInBlock = p.findLhs(firstBodyStatement) + assignedFirstInBlock = p.findLHS(firstBodyStatement) } - lhs := p.findLhs(stmt) - rhs := p.findRhs(stmt) - all := append(lhs, rhs...) + var ( + leftHandSide = p.findLHS(stmt) + rightHandSide = p.findRHS(stmt) + rightAndLeftHandSide = append(leftHandSide, rightHandSide...) + calledOrAssignedOnLineAbove = append(calledOnLineAbove, assignedOnLineAbove...) + ) /* DEBUG: - fmt.Println("LHS: ", lhs) - fmt.Println("RHS: ", rhs) + fmt.Println("LHS: ", leftHandSide) + fmt.Println("RHS: ", rightHandSide) fmt.Println("Assigned above: ", assignedOnLineAbove) fmt.Println("Assigned first: ", assignedFirstInBlock) */ + // If we called some kind of lock on the line above we allow cuddling + // anything. + if atLeastOneInListsMatch(calledOnLineAbove, p.config.AllowCuddleWithCalls) { + continue + } + + // If we call some kind of unlock on this line we allow cuddling with + // anything. + if atLeastOneInListsMatch(rightHandSide, p.config.AllowCuddleWithRHS) { + continue + } + moreThanOneStatementAbove := func() bool { if i < 2 { return false } statementBeforePreviousStatement := statements[i-2] - if p.nodeStart(previousStatement)-1 == p.nodeEnd(statementBeforePreviousStatement) { - return true - } - return false + return p.nodeStart(previousStatement)-1 == p.nodeEnd(statementBeforePreviousStatement) } isLastStatementInBlockOfOnlyTwoLines := func() bool { @@ -188,17 +250,15 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { case *ast.IfStmt: if len(assignedOnLineAbove) == 0 { p.addError(t.Pos(), "if statements should only be cuddled with assignments") - continue } if moreThanOneStatementAbove() { p.addError(t.Pos(), "only one cuddle assignment allowed before if statement") - continue } - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { if !atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) { p.addError(t.Pos(), "if statements should only be cuddled with assignments used in the if statement itself") } @@ -218,16 +278,32 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { case *ast.AssignStmt: // append is usually an assignment but should not be allowed to be // cuddled with anything not appended. - if len(rhs) > 0 && rhs[len(rhs)-1] == "append" { - if !atLeastOneInListsMatch(assignedOnLineAbove, rhs) { - p.addError(t.Pos(), "append only allowed to cuddle with appended value") + if len(rightHandSide) > 0 && rightHandSide[len(rightHandSide)-1] == "append" { + if p.config.CheckAppend { + if !atLeastOneInListsMatch(calledOrAssignedOnLineAbove, rightHandSide) { + p.addError(t.Pos(), "append only allowed to cuddle with appended value") + } } + + continue } if _, ok := previousStatement.(*ast.AssignStmt); ok { continue } + // If the assignment is from a type or variable called on the line + // above we can allow it by setting AllowAssignAndCallsCuddle to + // true. + // Example (x is used): + // x.function() + // a.Field = x.anotherFunction() + if p.config.AllowAssignAndCallsCuddle { + if atLeastOneInListsMatch(calledOrAssignedOnLineAbove, rightAndLeftHandSide) { + continue + } + } + p.addError(t.Pos(), "assignments should only be cuddled with other assignments") case *ast.DeclStmt: p.addError(t.Pos(), "declarations should never be cuddled") @@ -235,22 +311,34 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { switch previousStatement.(type) { case *ast.DeclStmt, *ast.ReturnStmt: p.addError(t.Pos(), "expressions should not be cuddled with declarations or returns") + case *ast.IfStmt, *ast.RangeStmt, *ast.SwitchStmt: + p.addError(t.Pos(), "expressions should not be cuddled with blocks") + } + + // If the expression is called on a type or variable used or + // assigned on the line we can allow it by setting + // AllowAssignAndCallsCuddle to true. + // Example of allowed cuddled (x is used): + // a.Field = x.func() + // x.function() + if p.config.AllowAssignAndCallsCuddle { + if atLeastOneInListsMatch(calledOrAssignedOnLineAbove, rightAndLeftHandSide) { + continue + } } // If we assigned variables on the line above but didn't use them in - // this expression we there should probably be a newline between - // them. - if len(assignedOnLineAbove) > 0 && !atLeastOneInListsMatch(all, assignedOnLineAbove) { + // this expression there should probably be a newline between them. + if len(assignedOnLineAbove) > 0 && !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { p.addError(t.Pos(), "only cuddled expressions if assigning variable or using from line above") } case *ast.RangeStmt: if moreThanOneStatementAbove() { p.addError(t.Pos(), "only one cuddle assignment allowed before range statement") - continue } - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { if !atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) { p.addError(t.Pos(), "ranges should only be cuddled with assignments used in the iteration") } @@ -270,16 +358,16 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { // Be extra nice with RHS, it's common to use this for locks: // m.Lock() // defer m.Unlock() - previousRhs := p.findRhs(previousStatement) - if atLeastOneInListsMatch(rhs, previousRhs) { + previousRHS := p.findRHS(previousStatement) + if atLeastOneInListsMatch(rightHandSide, previousRHS) { continue } - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { p.addError(t.Pos(), "defer statements should only be cuddled with expressions on same variable") } case *ast.ForStmt: - if len(all) == 0 { + if len(rightAndLeftHandSide) == 0 { p.addError(t.Pos(), "for statement without condition should never be cuddled") continue @@ -294,7 +382,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { // The same rule applies for ranges as for if statements, see // comments regarding variable usages on the line before or as the // first line in the block for details. - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { if !atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) { p.addError(t.Pos(), "for statements should only be cuddled with assignments used in the iteration") } @@ -306,7 +394,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { p.addError(t.Pos(), "go statements can only invoke functions assigned on line above") } case *ast.SwitchStmt: @@ -316,8 +404,8 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if !atLeastOneInListsMatch(all, assignedOnLineAbove) { - if len(all) == 0 { + if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { + if len(rightAndLeftHandSide) == 0 { p.addError(t.Pos(), "anonymous switch statements should never be cuddled") } else { p.addError(t.Pos(), "switch statements should only be cuddled with variables switched") @@ -331,7 +419,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { } // Allowed to type assert on variable assigned on line above. - if !atLeastOneInListsMatch(rhs, assignedOnLineAbove) { + if !atLeastOneInListsMatch(rightHandSide, assignedOnLineAbove) { // Allow type assertion on variables used in the first case // immediately. if !atLeastOneInListsMatch(assignedOnLineAbove, assignedFirstInBlock) { @@ -353,7 +441,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { // directly as the first argument inside a body. // The body will then be parsed as a *ast.BlockStmt (regular block) or as a list // of []ast.Stmt (case block). -func (p *processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { +func (p *Processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { stmt := allStmt[i] // Start by checking if the statement has a body (probably if-statement, @@ -409,7 +497,7 @@ func (p *processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { return firstBodyStatement } -func (p *processor) findLhs(node ast.Node) []string { +func (p *Processor) findLHS(node ast.Node) []string { var lhs []string if node == nil { @@ -429,36 +517,36 @@ func (p *processor) findLhs(node ast.Node) []string { return []string{t.Name} case *ast.AssignStmt: for _, v := range t.Lhs { - lhs = append(lhs, p.findLhs(v)...) + lhs = append(lhs, p.findLHS(v)...) } case *ast.GenDecl: for _, v := range t.Specs { - lhs = append(lhs, p.findLhs(v)...) + lhs = append(lhs, p.findLHS(v)...) } case *ast.ValueSpec: for _, v := range t.Names { - lhs = append(lhs, p.findLhs(v)...) + lhs = append(lhs, p.findLHS(v)...) } case *ast.BlockStmt: for _, v := range t.List { - lhs = append(lhs, p.findLhs(v)...) + lhs = append(lhs, p.findLHS(v)...) } case *ast.BinaryExpr: return append( - p.findLhs(t.X), - p.findLhs(t.Y)..., + p.findLHS(t.X), + p.findLHS(t.Y)..., ) case *ast.DeclStmt: - return p.findLhs(t.Decl) + return p.findLHS(t.Decl) case *ast.IfStmt: - return p.findLhs(t.Cond) + return p.findLHS(t.Cond) case *ast.TypeSwitchStmt: - return p.findLhs(t.Assign) + return p.findLHS(t.Assign) case *ast.SendStmt: - return p.findLhs(t.Chan) + return p.findLHS(t.Chan) default: if x, ok := maybeX(t); ok { - return p.findLhs(x) + return p.findLHS(x) } p.addWarning("UNKNOWN LHS", t.Pos(), t) @@ -467,7 +555,7 @@ func (p *processor) findLhs(node ast.Node) []string { return lhs } -func (p *processor) findRhs(node ast.Node) []string { +func (p *Processor) findRHS(node ast.Node) []string { var rhs []string if node == nil { @@ -485,53 +573,55 @@ func (p *processor) findRhs(node ast.Node) []string { return []string{t.Name} case *ast.SelectorExpr: // TODO: Should this be RHS? - // Needed for defer as of now - return p.findRhs(t.X) + // t.X is needed for defer as of now and t.Sel needed for special + // functions such as Lock() + rhs = p.findRHS(t.X) + rhs = append(rhs, p.findRHS(t.Sel)...) case *ast.AssignStmt: for _, v := range t.Rhs { - rhs = append(rhs, p.findRhs(v)...) + rhs = append(rhs, p.findRHS(v)...) } case *ast.CallExpr: for _, v := range t.Args { - rhs = append(rhs, p.findRhs(v)...) + rhs = append(rhs, p.findRHS(v)...) } - rhs = append(rhs, p.findRhs(t.Fun)...) + rhs = append(rhs, p.findRHS(t.Fun)...) case *ast.CompositeLit: for _, v := range t.Elts { - rhs = append(rhs, p.findRhs(v)...) + rhs = append(rhs, p.findRHS(v)...) } case *ast.IfStmt: - rhs = append(rhs, p.findRhs(t.Cond)...) - rhs = append(rhs, p.findRhs(t.Init)...) + rhs = append(rhs, p.findRHS(t.Cond)...) + rhs = append(rhs, p.findRHS(t.Init)...) case *ast.BinaryExpr: return append( - p.findRhs(t.X), - p.findRhs(t.Y)..., + p.findRHS(t.X), + p.findRHS(t.Y)..., ) case *ast.TypeSwitchStmt: - return p.findRhs(t.Assign) + return p.findRHS(t.Assign) case *ast.ReturnStmt: for _, v := range t.Results { - rhs = append(rhs, p.findRhs(v)...) + rhs = append(rhs, p.findRHS(v)...) } case *ast.BlockStmt: for _, v := range t.List { - rhs = append(rhs, p.findRhs(v)...) + rhs = append(rhs, p.findRHS(v)...) } case *ast.SwitchStmt: - return p.findRhs(t.Tag) + return p.findRHS(t.Tag) case *ast.GoStmt: - return p.findRhs(t.Call) + return p.findRHS(t.Call) case *ast.ForStmt: - return p.findRhs(t.Cond) + return p.findRHS(t.Cond) case *ast.DeferStmt: - return p.findRhs(t.Call) + return p.findRHS(t.Call) case *ast.SendStmt: - return p.findLhs(t.Value) + return p.findLHS(t.Value) default: if x, ok := maybeX(t); ok { - return p.findRhs(x) + return p.findRHS(x) } p.addWarning("UNKNOWN RHS", t.Pos(), t) @@ -591,13 +681,15 @@ func atLeastOneInListsMatch(listOne, listTwo []string) bool { // findLeadingAndTrailingWhitespaces will find leading and trailing whitespaces // in a node. The method takes comments in consideration which will make the // parser more gentle. -func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.Node) { +func (p *Processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.Node) { var ( allowedLinesBeforeFirstStatement = 1 commentMap = ast.NewCommentMap(p.fileSet, stmt, p.file.Comments) blockStatements []ast.Stmt blockStartLine int blockEndLine int + blockStartPos token.Pos + blockEndPos token.Pos ) // Depending on the block type, get the statements in the block and where @@ -605,14 +697,14 @@ func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No switch t := stmt.(type) { case *ast.BlockStmt: blockStatements = t.List - blockStartLine = p.fileSet.Position(t.Lbrace).Line - blockEndLine = p.fileSet.Position(t.Rbrace).Line + blockStartPos = t.Lbrace + blockEndPos = t.Rbrace case *ast.CaseClause: blockStatements = t.Body - blockStartLine = p.fileSet.Position(t.Colon).Line + blockStartPos = t.Colon case *ast.CommClause: blockStatements = t.Body - blockStartLine = p.fileSet.Position(t.Colon).Line + blockStartPos = t.Colon default: p.addWarning("whitespace node type not implemented ", stmt.Pos(), stmt) @@ -624,6 +716,14 @@ func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No return } + blockStartLine = p.fileSet.Position(blockStartPos).Line + blockEndLine = p.fileSet.Position(blockEndPos).Line + + // No whitespace possible if LBrace and RBrace is on the same line. + if blockStartLine == blockEndLine { + return + } + var ( firstStatement = blockStatements[0] lastStatement = blockStatements[len(blockStatements)-1] @@ -633,13 +733,9 @@ func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No // the beginning of a block before the first statement. if c, ok := commentMap[firstStatement]; ok { for _, commentGroup := range c { - var ( - start = p.fileSet.Position(commentGroup.Pos()).Line - ) - - // If the comment group is on the same lince as the block start + // If the comment group is on the same line as the block start // (LBrace) we should not consider it. - if start == blockStartLine { + if p.nodeStart(commentGroup) == blockStartLine { continue } @@ -654,10 +750,9 @@ func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No } } - if p.fileSet.Position(firstStatement.Pos()).Line != blockStartLine+allowedLinesBeforeFirstStatement { - p.addErrorOffset( - firstStatement.Pos(), - -1, + if p.nodeStart(firstStatement) != blockStartLine+allowedLinesBeforeFirstStatement { + p.addError( + blockStartPos, "block should not start with a whitespace", ) } @@ -673,52 +768,47 @@ func (p *processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No switch n := nextStatement.(type) { case *ast.CaseClause: - blockEndLine = p.fileSet.Position(n.Colon).Line + blockEndPos = n.Colon case *ast.CommClause: - blockEndLine = p.fileSet.Position(n.Colon).Line + blockEndPos = n.Colon default: // We're not at the end of the case? return } + + blockEndLine = p.fileSet.Position(blockEndPos).Line } - if p.fileSet.Position(lastStatement.End()).Line != blockEndLine-1 { - p.addErrorOffset( - lastStatement.End(), - 1, + if p.nodeEnd(lastStatement) != blockEndLine-1 { + p.addError( + blockEndPos, "block should not end with a whitespace (or comment)", ) } } -func (p *processor) nodeStart(node ast.Node) int { +func (p *Processor) nodeStart(node ast.Node) int { return p.fileSet.Position(node.Pos()).Line } -func (p *processor) nodeEnd(node ast.Node) int { +func (p *Processor) nodeEnd(node ast.Node) int { return p.fileSet.Position(node.End()).Line } // Add an error for the file and line number for the current token.Pos with the // given reason. -func (p *processor) addError(pos token.Pos, reason string) { - p.addErrorOffset(pos, 0, reason) -} - -// Add an error for the file for the current token.Pos with the given offset and -// reason. The offset will be added to the token.Pos line. -func (p *processor) addErrorOffset(pos token.Pos, offset int, reason string) { +func (p *Processor) addError(pos token.Pos, reason string) { position := p.fileSet.Position(pos) - p.result = append(p.result, result{ + p.result = append(p.result, Result{ FileName: position.Filename, - LineNumber: position.Line + offset, + LineNumber: position.Line, Position: position, Reason: reason, }) } -func (p *processor) addWarning(w string, pos token.Pos, t interface{}) { +func (p *Processor) addWarning(w string, pos token.Pos, t interface{}) { position := p.fileSet.Position(pos) p.warnings = append(p.warnings, diff --git a/wsl_test.go b/wsl_test.go index b5e6443..27aca7f 100644 --- a/wsl_test.go +++ b/wsl_test.go @@ -1,6 +1,7 @@ package wsl import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -56,6 +57,14 @@ func TestGenericHandling(t *testing.T) { fmt.Println("hello, world") }`), }, + { + description: "no false positives for one line functions", + code: []byte(`package main + + func main() { + s := func() error { return nil } + }`), + }, } for _, tc := range cases { @@ -901,7 +910,7 @@ func TestShouldAddEmptyLines(t *testing.T) { f := func() { // This violates whitespaces - + // Violates cuddle notThis := false if isThisTested { @@ -918,6 +927,57 @@ func TestShouldAddEmptyLines(t *testing.T) { "if statements should only be cuddled with assignments used in the if statement itself", }, }, + { + description: "locks", + code: []byte(`package main + + func main() { + hashFileCache.Lock() + out, ok := hashFileCache.m[file] + hashFileCache.Unlock() + + mu := &sync.Mutex{} + mu.X(y).Z.RLock() + x, y := someMap[someKey] + mu.RUnlock() + }`), + }, + { + description: "append type", + code: []byte(`package main + + func main() { + s := []string{} + + // Multiple append should be OK + s = append(s, "one") + s = append(s, "two") + s = append(s, "three") + + // Both assigned and called should be allowed to be cuddled. + // It's not nice, but they belong' + p.defs = append(p.defs, x) + def.parseFrom(p) + p.defs = append(p.defs, def) + def.parseFrom(p) + def.parseFrom(p) + p.defs = append(p.defs, x) + }`), + }, + { + description: "AllowAssignAndCallsCuddle", + code: []byte(`package main + + func main() { + for i := range make([]int, 10) { + fmt.Println("x") + } + x.Calling() + }`), + expectedErrorStrings: []string{ + "expressions should not be cuddled with blocks", + }, + }, } for _, tc := range cases { @@ -938,6 +998,48 @@ func TestShouldAddEmptyLines(t *testing.T) { } } +func TestWithConfig(t *testing.T) { + // This test is used to simulate code to perform TDD. This part should never + // be committed with any test. + cases := []struct { + description string + code []byte + expectedErrorStrings []string + customConfig *Configuration + }{ + { + description: "AllowAssignAndCallsCuddle", + code: []byte(`package main + + func main() { + p.token(':') + d.StartBit = p.uint() + p.token('|') + d.Size = p.uint() + }`), + customConfig: &Configuration{ + AllowAssignAndCallsCuddle: true, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + p := NewProcessor() + if tc.customConfig != nil { + p = NewProcessorWithConfig(*tc.customConfig) + } + + p.process("unit-test", tc.code) + require.Len(t, p.result, len(tc.expectedErrorStrings), "correct amount of errors found") + + for i := range tc.expectedErrorStrings { + assert.Contains(t, p.result[i].Reason, tc.expectedErrorStrings[i], "expected error found") + } + }) + } +} + func TestTODO(t *testing.T) { // This test is used to simulate code to perform TDD. This part should never // be committed with any test. @@ -945,12 +1047,16 @@ func TestTODO(t *testing.T) { description string code []byte expectedErrorStrings []string + customConfig *Configuration }{ { description: "boilerplate", code: []byte(`package main func main() { + for i := range make([]int, 10) { + fmt.Println("x") + } }`), }, } @@ -958,10 +1064,18 @@ func TestTODO(t *testing.T) { for _, tc := range cases { t.Run(tc.description, func(t *testing.T) { p := NewProcessor() + if tc.customConfig != nil { + p = NewProcessorWithConfig(*tc.customConfig) + } + p.process("unit-test", tc.code) t.Logf("WARNINGS: %s", p.warnings) + for _, r := range p.result { + fmt.Println(r.String()) + } + require.Len(t, p.result, len(tc.expectedErrorStrings), "correct amount of errors found") for i := range tc.expectedErrorStrings {