diff --git a/.golangci.example.yml b/.golangci.example.yml index 1c4aff5a7f5d..ea295dcc6092 100644 --- a/.golangci.example.yml +++ b/.golangci.example.yml @@ -220,6 +220,10 @@ linters-settings: allow-assign-and-call: true # Allow multiline assignments to be cuddled. Default is true. allow-multiline-assign: true + # Allow case blocks to end with a whitespace. + allow-case-traling-whitespace: true + # Allow declarations (var) to be cuddled. + allow-cuddle-declarations: false linters: enable: diff --git a/README.md b/README.md index 8744cb36f2c6..c50e9890c5cd 100644 --- a/README.md +++ b/README.md @@ -819,6 +819,10 @@ linters-settings: allow-assign-and-call: true # Allow multiline assignments to be cuddled. Default is true. allow-multiline-assign: true + # Allow case blocks to end with a whitespace. + allow-case-traling-whitespace: true + # Allow declarations (var) to be cuddled. + allow-cuddle-declarations: false linters: enable: diff --git a/go.mod b/go.mod index 9e5b2f670750..263d2c880699 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.2.1 + github.com/bombsimon/wsl v1.2.5 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 293ced352653..01daee958033 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.2.1 h1:DcLf3V66dJi4a+KHt+F1FdOeBZ05adHqTMYFvjgv06k= -github.com/bombsimon/wsl v1.2.1/go.mod h1:43lEF/i0kpXbLCeDXL9LMT8c92HyBywXb0AsgMHYngM= +github.com/bombsimon/wsl v1.2.5 h1:9gTOkIwVtoDZywvX802SDHokeX4kW1cKnV8ZTVAPkRs= +github.com/bombsimon/wsl v1.2.5/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 90b0669683fe..1b8de67585fb 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -255,9 +255,11 @@ type GocognitSettings struct { } type WSLSettings struct { - StrictAppend bool `mapstructure:"strict-append"` - AllowAssignAndCallCuddle bool `mapstructure:"allow-assign-and-call"` - AllowMultiLineAssignCuddle bool `mapstructure:"allow-multiline-assign"` + StrictAppend bool `mapstructure:"strict-append"` + AllowAssignAndCallCuddle bool `mapstructure:"allow-assign-and-call"` + AllowMultiLineAssignCuddle bool `mapstructure:"allow-multiline-assign"` + AllowCaseTrailingWhitespace bool `mapstructure:"allow-case-trailing-whitespace"` + AllowCuddleDeclaration bool `mapstructure:"allow-cuddle-declarations"` } var defaultLintersSettings = LintersSettings{ @@ -289,9 +291,11 @@ var defaultLintersSettings = LintersSettings{ MinComplexity: 30, }, WSL: WSLSettings{ - StrictAppend: true, - AllowAssignAndCallCuddle: true, - AllowMultiLineAssignCuddle: true, + StrictAppend: true, + AllowAssignAndCallCuddle: true, + AllowMultiLineAssignCuddle: true, + AllowCaseTrailingWhitespace: true, + AllowCuddleDeclaration: false, }, } diff --git a/pkg/golinters/wsl.go b/pkg/golinters/wsl.go index ae575831f28e..174a673baecf 100644 --- a/pkg/golinters/wsl.go +++ b/pkg/golinters/wsl.go @@ -37,11 +37,13 @@ func NewWSL() *goanalysis.Linter { 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"}, + StrictAppend: linterCfg.StrictAppend, + AllowAssignAndCallCuddle: linterCfg.AllowAssignAndCallCuddle, + AllowMultiLineAssignCuddle: linterCfg.AllowMultiLineAssignCuddle, + AllowCaseTrailingWhitespace: linterCfg.AllowCaseTrailingWhitespace, + AllowCuddleDeclaration: linterCfg.AllowCuddleDeclaration, + AllowCuddleWithCalls: []string{"Lock", "RLock"}, + AllowCuddleWithRHS: []string{"Unlock", "RUnlock"}, } ) diff --git a/test/linters_test.go b/test/linters_test.go index 14ca95acc09e..1f01814af25e 100644 --- a/test/linters_test.go +++ b/test/linters_test.go @@ -105,6 +105,7 @@ func testOneSource(t *testing.T, sourcePath string) { "--print-issued-lines=false", "--print-linter-name=false", "--out-format=line-number", + "--max-same-issues=10", } rc := extractRunContextFromComments(t, sourcePath) diff --git a/test/testdata/wsl.go b/test/testdata/wsl.go index a5ad2762a11d..81fc71c8c38f 100644 --- a/test/testdata/wsl.go +++ b/test/testdata/wsl.go @@ -100,3 +100,75 @@ func f3() int { } func onelineShouldNotError() error { return nil } + +func multilineCase() { + // Multiline cases + switch { + case true, + false: + fmt.Println("ok") + case false || + true: + fmt.Println("ok") + case true, + false: + fmt.Println("ok") + } +} + +func sliceExpr() { + // Index- and slice expressions. + var aSlice = []int{1, 2, 3} + + start := 2 + if v := aSlice[start]; v == 1 { + fmt.Println("ok") + } + + notOk := 1 + if v := aSlice[start]; v == 1 { // ERROR "if statements should only be cuddled with assignments used in the if statement itself" + fmt.Println("notOk") + fmt.Println(notOk) + } + + end := 2 + if len(aSlice[start:end]) > 2 { + fmt.Println("ok") + } +} + +func indexExpr() { + var aMap = map[string]struct{}{"key": {}} + + key := "key" + if k, ok := aMap[key]; ok { + fmt.Println(k) + } + + xxx := "xxx" + if _, ok := aMap[key]; ok { // ERROR "if statements should only be cuddled with assignments used in the if statement itself" + fmt.Println("not ok") + fmt.Println(xxx) + } +} + +func allowTrailing(i int) { + switch i { + case 1: + fmt.Println("one") + + case 2: + fmt.Println("two") + + case 3: + fmt.Println("three") + } +} + +// ExampleSomeOutput simulates an example function. +func ExampleSomeOutput() { + fmt.Println("Hello, world") + + // Output: + // Hello, world +} diff --git a/vendor/github.com/bombsimon/wsl/wsl.go b/vendor/github.com/bombsimon/wsl/wsl.go index 3dcc32b196de..fe62b62c602f 100644 --- a/vendor/github.com/bombsimon/wsl/wsl.go +++ b/vendor/github.com/bombsimon/wsl/wsl.go @@ -7,6 +7,7 @@ import ( "go/token" "io/ioutil" "reflect" + "strings" ) type Configuration struct { @@ -49,6 +50,29 @@ type Configuration struct { // } AllowMultiLineAssignCuddle bool + // AllowCaseTrailingWhitespace will allow case blocks to end with a + // whitespace. Sometimes this might actually improve readability. This + // defaults to false but setting it to true will enable the following + // example: + // switch { + // case 1: + // fmt.Println(1) + // + // case 2: + // fmt.Println(2) + // + // case 3: + // fmt:println(3) + // } + AllowCaseTrailingWhitespace bool + + // AllowCuddleDeclaration will allow multiple var/declaration statements to + // be cuddled. This defaults to false but setting it to true will enable the + // following example: + // var foo bool + // var err error + AllowCuddleDeclaration 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: @@ -69,11 +93,12 @@ type Configuration struct { // DefaultConfig returns default configuration func DefaultConfig() Configuration { return Configuration{ - StrictAppend: true, - AllowAssignAndCallCuddle: true, - AllowMultiLineAssignCuddle: true, - AllowCuddleWithCalls: []string{"Lock", "RLock"}, - AllowCuddleWithRHS: []string{"Unlock", "RUnlock"}, + StrictAppend: true, + AllowAssignAndCallCuddle: true, + AllowMultiLineAssignCuddle: true, + AllowCaseTrailingWhitespace: false, + AllowCuddleWithCalls: []string{"Lock", "RLock"}, + AllowCuddleWithRHS: []string{"Unlock", "RUnlock"}, } } @@ -113,6 +138,7 @@ func NewProcessor() *Processor { // ProcessFiles takes a string slice with file names (full paths) and lints // them. +// nolint: gocritic func (p *Processor) ProcessFiles(filenames []string) ([]Result, []string) { for _, filename := range filenames { data, err := ioutil.ReadFile(filename) @@ -147,7 +173,7 @@ func (p *Processor) process(filename string, data []byte) { for _, d := range p.file.Decls { switch v := d.(type) { case *ast.FuncDecl: - p.parseBlockBody(v.Body) + p.parseBlockBody(v.Name, v.Body) case *ast.GenDecl: // `go fmt` will handle proper spacing for GenDecl such as imports, // constants etc. @@ -159,14 +185,14 @@ 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(ident *ast.Ident, block *ast.BlockStmt) { // Nothing to do if there's no value. if reflect.ValueOf(block).IsNil() { return } // Start by finding leading and trailing whitespaces. - p.findLeadingAndTrailingWhitespaces(block, nil) + p.findLeadingAndTrailingWhitespaces(ident, block, nil) // Parse the block body contents. p.parseBlockStatements(block.List) @@ -182,7 +208,7 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) { if as, isAssignStmt := stmt.(*ast.AssignStmt); isAssignStmt { for _, rhs := range as.Rhs { if fl, isFuncLit := rhs.(*ast.FuncLit); isFuncLit { - p.parseBlockBody(fl.Body) + p.parseBlockBody(nil, fl.Body) } } } @@ -244,14 +270,6 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) { calledOrAssignedOnLineAbove = append(calledOnLineAbove, assignedOnLineAbove...) ) - /* - DEBUG: - 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) { @@ -282,6 +300,7 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) { // t.X = true // return t // } + // nolint: gocritic if i == len(statements)-1 && i == 1 { if p.nodeEnd(stmt)-p.nodeStart(previousStatement) <= 2 { return true @@ -351,7 +370,9 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) { p.addError(t.Pos(), "assignments should only be cuddled with other assignments") case *ast.DeclStmt: - p.addError(t.Pos(), "declarations should never be cuddled") + if !p.config.AllowCuddleDeclaration { + p.addError(t.Pos(), "declarations should never be cuddled") + } case *ast.ExprStmt: switch previousStatement.(type) { case *ast.DeclStmt, *ast.ReturnStmt: @@ -394,6 +415,25 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) { continue } + // Special treatment of deferring body closes after error checking + // according to best practices. See + // https://github.com/bombsimon/wsl/issues/31 which links to + // discussion about error handling after HTTP requests. This is hard + // coded and very specific but for now this is to be seen as a + // special case. What this does is that it *only* allows a defer + // statement with `Close` on the right hand side to be cuddled with + // an if-statement to support this: + // resp, err := client.Do(req) + // if err != nil { + // return err + // } + // defer resp.Body.Close() + if _, ok := previousStatement.(*ast.IfStmt); ok { + if atLeastOneInListsMatch(rightHandSide, []string{"Close"}) { + continue + } + } + if moreThanOneStatementAbove() { p.addError(t.Pos(), "only one cuddle assignment allowed before defer statement") @@ -517,7 +557,7 @@ func (p *Processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { } } - p.parseBlockBody(statementBodyContent) + p.parseBlockBody(nil, statementBodyContent) case []ast.Stmt: // The Body field for an *ast.CaseClause or *ast.CommClause is of type // []ast.Stmt. We must check leading and trailing whitespaces and then @@ -530,7 +570,7 @@ func (p *Processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { nextStatement = allStmt[i+1] } - p.findLeadingAndTrailingWhitespaces(stmt, nextStatement) + p.findLeadingAndTrailingWhitespaces(nil, stmt, nextStatement) p.parseBlockStatements(statementBodyContent) default: p.addWarning( @@ -664,6 +704,13 @@ func (p *Processor) findRHS(node ast.Node) []string { return p.findRHS(t.Call) case *ast.SendStmt: return p.findLHS(t.Value) + case *ast.IndexExpr: + rhs = append(rhs, p.findRHS(t.Index)...) + rhs = append(rhs, p.findRHS(t.X)...) + case *ast.SliceExpr: + rhs = append(rhs, p.findRHS(t.X)...) + rhs = append(rhs, p.findRHS(t.Low)...) + rhs = append(rhs, p.findRHS(t.High)...) default: if x, ok := maybeX(t); ok { return p.findRHS(x) @@ -679,7 +726,7 @@ func (p *Processor) findRHS(node ast.Node) []string { // if it exists. If the node doesn't have an X field nil and false is returned. // Known fields with X that are handled: // IndexExpr, ExprStmt, SelectorExpr, StarExpr, ParentExpr, TypeAssertExpr, -// RangeStmt, UnaryExpr, ParenExpr, SLiceExpr, IncDecStmt. +// RangeStmt, UnaryExpr, ParenExpr, SliceExpr, IncDecStmt. func maybeX(node interface{}) (ast.Node, bool) { maybeHasX := reflect.Indirect(reflect.ValueOf(node)).FieldByName("X") if !maybeHasX.IsValid() { @@ -726,7 +773,8 @@ 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) { +// nolint: gocognit +func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, nextStatement ast.Node) { var ( allowedLinesBeforeFirstStatement = 1 commentMap = ast.NewCommentMap(p.fileSet, stmt, p.file.Comments) @@ -811,11 +859,16 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No return } + // If we allow case to end white whitespace just return. + if p.config.AllowCaseTrailingWhitespace { + return + } + switch n := nextStatement.(type) { case *ast.CaseClause: - blockEndPos = n.Colon + blockEndPos = n.Case case *ast.CommClause: - blockEndPos = n.Colon + blockEndPos = n.Case default: // We're not at the end of the case? return @@ -824,7 +877,7 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No blockEndLine = p.fileSet.Position(blockEndPos).Line } - if p.nodeEnd(lastStatement) != blockEndLine-1 { + if p.nodeEnd(lastStatement) != blockEndLine-1 && !isExampleFunc(ident) { p.addError( blockEndPos, "block should not end with a whitespace (or comment)", @@ -832,6 +885,10 @@ func (p *Processor) findLeadingAndTrailingWhitespaces(stmt, nextStatement ast.No } } +func isExampleFunc(ident *ast.Ident) bool { + return ident != nil && strings.HasPrefix(ident.Name, "Example") +} + func (p *Processor) nodeStart(node ast.Node) int { return p.fileSet.Position(node.Pos()).Line } diff --git a/vendor/modules.txt b/vendor/modules.txt index 9ed8871617e2..93b4abdc5cbc 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.2.1 +# github.com/bombsimon/wsl v1.2.5 github.com/bombsimon/wsl # github.com/davecgh/go-spew v1.1.1 github.com/davecgh/go-spew/spew