From d4b4ad8dfe4a210a5d6c592e7ff3ea90a0273da4 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Tue, 8 Oct 2019 03:22:44 +0200 Subject: [PATCH] Update WSL to v1.2.1 (#794) * Update WSL to v1.2.1 * Add new tests for fixed false positives, don't derive defaults from WSL --- .golangci.example.yml | 9 + README.md | 9 + go.mod | 2 +- go.sum | 4 +- pkg/config/config.go | 12 + pkg/golinters/wsl.go | 16 +- test/testdata/wsl.go | 22 ++ vendor/github.com/bombsimon/wsl/README.md | 21 ++ vendor/github.com/bombsimon/wsl/go.mod | 2 +- vendor/github.com/bombsimon/wsl/go.sum | 2 - vendor/github.com/bombsimon/wsl/wsl.go | 369 +++++++++++++++------- vendor/modules.txt | 2 +- 12 files changed, 344 insertions(+), 126 deletions(-) diff --git a/.golangci.example.yml b/.golangci.example.yml index 24b95b96..7a19b12f 100644 --- a/.golangci.example.yml +++ b/.golangci.example.yml @@ -211,6 +211,15 @@ linters-settings: whitespace: multi-if: false # Enforces newlines (or comments) after every multi-line if statement multi-func: false # Enforces newlines (or comments) after every multi-line function signature + wsl: + # If true append is only allowed to be cuddled if appending value is + # matching variables, fields or types on line above. Default is true. + strict-append: true + # Allow calls and assignments to be cuddled as long as the lines have any + # matching variables, fields or types. Default is true. + allow-assign-and-call: true + # Allow multiline assignments to be cuddled. Default is true. + allow-multiline-assign: true linters: enable: diff --git a/README.md b/README.md index ef2aaffa..1f86213d 100644 --- a/README.md +++ b/README.md @@ -804,6 +804,15 @@ linters-settings: whitespace: multi-if: false # Enforces newlines (or comments) after every multi-line if statement multi-func: false # Enforces newlines (or comments) after every multi-line function signature + wsl: + # If true append is only allowed to be cuddled if appending value is + # matching variables, fields or types on line above. Default is true. + strict-append: true + # Allow calls and assignments to be cuddled as long as the lines have any + # matching variables, fields or types. Default is true. + allow-assign-and-call: true + # Allow multiline assignments to be cuddled. Default is true. + allow-multiline-assign: true linters: enable: diff --git a/go.mod b/go.mod index 3ccf5146..e1b1a462 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.12 require ( github.com/OpenPeeDeeP/depguard v1.0.1 - github.com/bombsimon/wsl v1.0.1 + github.com/bombsimon/wsl v1.2.1 github.com/fatih/color v1.7.0 github.com/go-critic/go-critic v0.3.5-0.20190904082202-d79a9f0c64db github.com/go-lintpack/lintpack v0.5.2 diff --git a/go.sum b/go.sum index 8959b8d6..b06e2ceb 100644 --- a/go.sum +++ b/go.sum @@ -11,8 +11,8 @@ github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRF github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8= github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q= github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+CedLV8= -github.com/bombsimon/wsl v1.0.1 h1:GkrpOj7ajds8re6EJXN+k6UtatYSktupigQ2ZfOUIOU= -github.com/bombsimon/wsl v1.0.1/go.mod h1:DSRwCD8c7NecM11/LnZcTS8nS8OND5ybj04DWM4l8mc= +github.com/bombsimon/wsl v1.2.1 h1:DcLf3V66dJi4a+KHt+F1FdOeBZ05adHqTMYFvjgv06k= +github.com/bombsimon/wsl v1.2.1/go.mod h1:43lEF/i0kpXbLCeDXL9LMT8c92HyBywXb0AsgMHYngM= github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= diff --git a/pkg/config/config.go b/pkg/config/config.go index 117720a3..206a4341 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -176,6 +176,7 @@ type LintersSettings struct { MultiFunc bool `mapstructure:"multi-func"` } + WSL WSLSettings Lll LllSettings Unparam UnparamSettings Nakedret NakedretSettings @@ -249,6 +250,12 @@ type GocognitSettings struct { MinComplexity int `mapstructure:"min-complexity"` } +type WSLSettings struct { + StrictAppend bool `mapstructure:"strict-append"` + AllowAssignAndCallCuddle bool `mapstructure:"allow-assign-and-call"` + AllowMultiLineAssignCuddle bool `mapstructure:"allow-multiline-assign"` +} + var defaultLintersSettings = LintersSettings{ Lll: LllSettings{ LineLength: 120, @@ -277,6 +284,11 @@ var defaultLintersSettings = LintersSettings{ Gocognit: GocognitSettings{ MinComplexity: 30, }, + WSL: WSLSettings{ + StrictAppend: true, + AllowAssignAndCallCuddle: true, + AllowMultiLineAssignCuddle: true, + }, } type Linters struct { diff --git a/pkg/golinters/wsl.go b/pkg/golinters/wsl.go index f037de24..4dcbf55e 100644 --- a/pkg/golinters/wsl.go +++ b/pkg/golinters/wsl.go @@ -33,13 +33,25 @@ func NewWSL() *goanalysis.Linter { nil, ).WithContextSetter(func(lintCtx *linter.Context) { analyzer.Run = func(pass *analysis.Pass) (interface{}, error) { - files := []string{} + var ( + files = []string{} + linterCfg = lintCtx.Cfg.LintersSettings.WSL + processorCfg = wsl.Configuration{ + StrictAppend: linterCfg.StrictAppend, + AllowAssignAndCallCuddle: linterCfg.AllowAssignAndCallCuddle, + AllowMultiLineAssignCuddle: linterCfg.AllowMultiLineAssignCuddle, + AllowCuddleWithCalls: []string{"Lock", "RLock"}, + AllowCuddleWithRHS: []string{"Unlock", "RUnlock"}, + } + ) for _, file := range pass.Files { files = append(files, pass.Fset.Position(file.Pos()).Filename) } - wslErrors, _ := wsl.ProcessFiles(files) + wslErrors, _ := wsl.NewProcessorWithConfig(processorCfg). + ProcessFiles(files) + if len(wslErrors) == 0 { return nil, nil } diff --git a/test/testdata/wsl.go b/test/testdata/wsl.go index 7659a5fb..a5ad2762 100644 --- a/test/testdata/wsl.go +++ b/test/testdata/wsl.go @@ -52,6 +52,26 @@ func main() { _, cf2 := context.WithCancel(context.Background()) defer cf1() // ERROR "only one cuddle assignment allowed before defer statement" defer cf2() + + err := multiline( + "spanning", + "multiple", + ) + if err != nil { + panic(err) + } + + notErr := multiline( + "spanning", + "multiple", + ) + if err != nil { // ERROR "if statements should only be cuddled with assignments used in the if statement itself" + panic(notErr) + } +} + +func multiline(s ...string) error { + return nil } func f1() int { @@ -78,3 +98,5 @@ func f3() int { return sum + notSum } + +func onelineShouldNotError() error { return nil } diff --git a/vendor/github.com/bombsimon/wsl/README.md b/vendor/github.com/bombsimon/wsl/README.md index 7b84ee51..a8fb0d1e 100644 --- a/vendor/github.com/bombsimon/wsl/README.md +++ b/vendor/github.com/bombsimon/wsl/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/vendor/github.com/bombsimon/wsl/go.mod b/vendor/github.com/bombsimon/wsl/go.mod index 20b38773..bce35c3e 100644 --- a/vendor/github.com/bombsimon/wsl/go.mod +++ b/vendor/github.com/bombsimon/wsl/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/vendor/github.com/bombsimon/wsl/go.sum b/vendor/github.com/bombsimon/wsl/go.sum index 1033a6ca..042e09bc 100644 --- a/vendor/github.com/bombsimon/wsl/go.sum +++ b/vendor/github.com/bombsimon/wsl/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/vendor/github.com/bombsimon/wsl/wsl.go b/vendor/github.com/bombsimon/wsl/wsl.go index 7b74acbb..3dcc32b1 100644 --- a/vendor/github.com/bombsimon/wsl/wsl.go +++ b/vendor/github.com/bombsimon/wsl/wsl.go @@ -9,26 +9,111 @@ import ( "reflect" ) -type result struct { +type Configuration struct { + // StrictAppend will do strict checking when assigning from append (x = + // append(x, y)). If this is set to true the append call must append either + // a variable assigned, called or used on the line above. Example on not + // allowed when this is true: + // + // x := []string{} + // y := "not going in X" + // x = append(x, "not y") // This is not allowed with StrictAppend + // z := "going in X" + // + // x = append(x, z) // This is allowed with StrictAppend + // + // m := transform(z) + // x = append(x, z) // So is this because Z is used above. + StrictAppend bool + + // AllowAssignAndCallCuddle 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() + // x = Assign() + // x.AnotherCall() + // x = AnotherAssign() + AllowAssignAndCallCuddle bool + + // AllowMultiLineAssignCuddle allows cuddling to assignments even if they + // span over multiple lines. This defaults to true which allows the + // following example: + // + // err := function( + // "multiple", "lines", + // ) + // if err != nil { + // // ... + // } + AllowMultiLineAssignCuddle 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 +} + +// DefaultConfig returns default configuration +func DefaultConfig() Configuration { + return Configuration{ + StrictAppend: true, + AllowAssignAndCallCuddle: true, + AllowMultiLineAssignCuddle: true, + AllowCuddleWithCalls: []string{"Lock", "RLock"}, + AllowCuddleWithRHS: []string{"Unlock", "RUnlock"}, + } +} + +// Result represents the result of one error. +type Result struct { FileName string LineNumber int Position token.Position Reason string } -type processor struct { - result []result +// String returns the filename, line number and reason of a 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(DefaultConfig()) +} + // 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 +126,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 +159,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 +174,8 @@ 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) { +// nolint: gocognit +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 +207,26 @@ 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 + + // Check if the previous statement spans over multiple lines. + var isMultiLineAssignment = p.nodeStart(previousStatement) != p.nodeStart(stmt)-1 + // Ensure previous line is not a multi line assignment and if not get - // all assigned variables. - if p.nodeStart(previousStatement) == p.nodeStart(stmt)-1 { - assignedOnLineAbove = p.findLhs(previousStatement) + // rightAndLeftHandSide assigned variables. + if !isMultiLineAssignment { + assignedOnLineAbove = p.findLHS(previousStatement) + calledOnLineAbove = p.findRHS(previousStatement) + } + + // If previous assignment is multi line and we allow it, fetch + // assignments (but only assignments). + if isMultiLineAssignment && p.config.AllowMultiLineAssignCuddle { + if _, ok := previousStatement.(*ast.AssignStmt); ok { + assignedOnLineAbove = p.findLHS(previousStatement) + } } // We could potentially have a block which require us to check the first @@ -139,32 +234,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 +295,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 +323,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.StrictAppend { + 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 AllowAssignAndCallCuddle to + // true. + // Example (x is used): + // x.function() + // a.Field = x.anotherFunction() + if p.config.AllowAssignAndCallCuddle { + 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 +356,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 + // AllowAssignAndCallCuddle to true. + // Example of allowed cuddled (x is used): + // a.Field = x.func() + // x.function() + if p.config.AllowAssignAndCallCuddle { + 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 +403,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 +427,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 +439,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 +449,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 +464,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 +486,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 +542,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 +562,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 +600,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 +618,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 +726,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 +742,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 +761,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 +778,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 +795,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 +813,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/vendor/modules.txt b/vendor/modules.txt index 7bb80739..4893c528 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -4,7 +4,7 @@ github.com/BurntSushi/toml github.com/OpenPeeDeeP/depguard # github.com/StackExchange/wmi v0.0.0-20180116203802-5d049714c4a6 github.com/StackExchange/wmi -# github.com/bombsimon/wsl v1.0.1 +# github.com/bombsimon/wsl v1.2.1 github.com/bombsimon/wsl # github.com/davecgh/go-spew v1.1.1 github.com/davecgh/go-spew/spew