diff --git a/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go b/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go new file mode 100644 index 0000000..2e5f364 --- /dev/null +++ b/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go @@ -0,0 +1,133 @@ +package testpkg + +import ( + "fmt" + "strconv" +) + +func ForCuddleAssignmentWholeBlock() { + x := 1 + y := 2 + + var numbers []int + for i := 0; i < 10; i++ { + if x == y { + numbers = append(numbers, i) + } + } + + var numbers2 []int + for i := range 10 { + if x == y { + numbers2 = append(numbers2, i) + } + } + + var numbers3 []int + for { // want "for statement without condition should never be cuddled" + if x == y { + numbers3 = append(numbers3, i) + } + } + + environment = make(map[string]string) + for _, env := range req.GetConfig().GetEnvs() { + switch env.GetKey() { + case "user-data": + cloudInitUserData = env.GetValue() + default: + environment[env.GetKey()] = env.GetValue() + } + } +} + +func IfCuddleAssignmentWholeBlock() { + x := 1 + y := 2 + + counter := 0 + if somethingTrue { + checker := getAChecker() + if !checker { + return + } + + counter++ // Cuddled variable used in block, but not as first statement + } + + var number2 []int + if x == y { + fmt.Println("a") + } else { + if x > y { + number2 = append(number2, i) + } + } + + var number3 []int + if x == y { + fmt.Println("a") + } else if x > y { + if x == y { + number3 = append(number3, i) + } + } + + var number4 []int + if x == y { + if x == y { + number4 = append(number4, i) + } + } else if x > y { + if x == y { + number4 = append(number4, i) + } + } else { + if x > y { + number4 = append(number4, i) + } + } +} + +func SwitchCuddleAssignmentWholeBlock() { + var id string + var b bool // want "declarations should never be cuddled" + switch b { // want "only one cuddle assignment allowed before switch statement" + case true: + id = "a" + case false: + id = "b" + } + + var id2 string + switch i := objectID.(type) { + case int: + if true { + id2 = strconv.Itoa(i) + } + case uint32: + if true { + id2 = strconv.Itoa(int(i)) + } + case string: + if true { + id2 = i + } + } + + var id3 string + switch { // want "anonymous switch statements should never be cuddled" + case int: + if true { + id3 = strconv.Itoa(i) + } + case uint32: + if true { + id3 = strconv.Itoa(int(i)) + } + case string: + if true { + id3 = i + } + } +} diff --git a/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go.golden b/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go.golden new file mode 100644 index 0000000..2ac4e32 --- /dev/null +++ b/testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go.golden @@ -0,0 +1,136 @@ +package testpkg + +import ( + "fmt" + "strconv" +) + +func ForCuddleAssignmentWholeBlock() { + x := 1 + y := 2 + + var numbers []int + for i := 0; i < 10; i++ { + if x == y { + numbers = append(numbers, i) + } + } + + var numbers2 []int + for i := range 10 { + if x == y { + numbers2 = append(numbers2, i) + } + } + + var numbers3 []int + + for { // want "for statement without condition should never be cuddled" + if x == y { + numbers3 = append(numbers3, i) + } + } + + environment = make(map[string]string) + for _, env := range req.GetConfig().GetEnvs() { + switch env.GetKey() { + case "user-data": + cloudInitUserData = env.GetValue() + default: + environment[env.GetKey()] = env.GetValue() + } + } +} + +func IfCuddleAssignmentWholeBlock() { + x := 1 + y := 2 + + counter := 0 + if somethingTrue { + checker := getAChecker() + if !checker { + return + } + + counter++ // Cuddled variable used in block, but not as first statement + } + + var number2 []int + if x == y { + fmt.Println("a") + } else { + if x > y { + number2 = append(number2, i) + } + } + + var number3 []int + if x == y { + fmt.Println("a") + } else if x > y { + if x == y { + number3 = append(number3, i) + } + } + + var number4 []int + if x == y { + if x == y { + number4 = append(number4, i) + } + } else if x > y { + if x == y { + number4 = append(number4, i) + } + } else { + if x > y { + number4 = append(number4, i) + } + } +} + +func SwitchCuddleAssignmentWholeBlock() { + var id string + + var b bool // want "declarations should never be cuddled" + switch b { // want "only one cuddle assignment allowed before switch statement" + case true: + id = "a" + case false: + id = "b" + } + + var id2 string + switch i := objectID.(type) { + case int: + if true { + id2 = strconv.Itoa(i) + } + case uint32: + if true { + id2 = strconv.Itoa(int(i)) + } + case string: + if true { + id2 = i + } + } + + var id3 string + + switch { // want "anonymous switch statements should never be cuddled" + case int: + if true { + id3 = strconv.Itoa(i) + } + case uint32: + if true { + id3 = strconv.Itoa(int(i)) + } + case string: + if true { + id3 = i + } + } +} diff --git a/wsl.go b/wsl.go index 44c7abe..2943642 100644 --- a/wsl.go +++ b/wsl.go @@ -304,12 +304,14 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { } } - // We could potentially have a block which require us to check the first - // argument before ruling out an allowed cuddle. - var calledOrAssignedFirstInBlock []string + // Contains the union of all variable names used anywhere + // within the block body (if applicable) and is used to check + // if a preceding statement's variables are actually used within + // the block. This helps enforce rules about allowed cuddling. + var blockUsedVars []string if firstBodyStatement != nil { - calledOrAssignedFirstInBlock = append(p.findLHS(firstBodyStatement), p.findRHS(firstBodyStatement)...) + blockUsedVars = p.findUsedVariablesInStatement(stmt) } var ( @@ -426,7 +428,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { reportNewlineTwoLinesAbove := func(n1, n2 ast.Node, reason string) { if atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) || - atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { // If both the assignment on the line above _and_ the assignment // two lines above is part of line or first in block, add the // newline as if non were. @@ -435,7 +437,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { if isAssignmentTwoLinesAbove && (atLeastOneInListsMatch(rightAndLeftHandSide, assignedTwoLinesAbove) || - atLeastOneInListsMatch(assignedTwoLinesAbove, calledOrAssignedFirstInBlock)) { + atLeastOneInListsMatch(assignedTwoLinesAbove, blockUsedVars)) { p.addWhitespaceBeforeError(n1, reason) } else { // If the variable on the line above is allowed to be @@ -507,7 +509,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + if atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { continue } @@ -611,7 +613,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { } if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { - if !atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + if !atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { p.addWhitespaceBeforeError(t, reasonRangeCuddledWithoutUse) } } @@ -679,7 +681,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { } } - if atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + if atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { continue } @@ -701,7 +703,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { // comments regarding variable usages on the line before or as the // first line in the block for details. if !atLeastOneInListsMatch(rightAndLeftHandSide, assignedOnLineAbove) { - if !atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + if !atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { p.addWhitespaceBeforeError(t, reasonForCuddledAssignWithoutUse) } } @@ -757,7 +759,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { if !atLeastOneInListsMatch(rightHandSide, assignedOnLineAbove) { // Allow type assertion on variables used in the first case // immediately. - if !atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) { + if !atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) { p.addWhitespaceBeforeError(t, reasonTypeSwitchCuddledWithoutUse) } } @@ -839,6 +841,32 @@ func (p *processor) firstBodyStatement(i int, allStmt []ast.Stmt) ast.Node { return firstBodyStatement } +// findUsedVariablesInStatement processes a statement, +// returning a union of all variables used within it. +func (p *processor) findUsedVariablesInStatement(stmt ast.Stmt) []string { + var ( + used []string + seen = map[string]struct{}{} + ) + + // ast.Inspect walks the AST of the statement. + ast.Inspect(stmt, func(n ast.Node) bool { + // We're only interested in identifiers. + if ident, ok := n.(*ast.Ident); ok { + if _, exists := seen[ident.Name]; !exists { + seen[ident.Name] = struct{}{} + + used = append(used, ident.Name) + } + } + + // Continue walking the AST. + return true + }) + + return used +} + func (p *processor) findLHS(node ast.Node) []string { var lhs []string diff --git a/wsl_test.go b/wsl_test.go index 0070d6f..758a0d8 100644 --- a/wsl_test.go +++ b/wsl_test.go @@ -25,6 +25,7 @@ func TestDefaultConfig(t *testing.T) { {dir: "else_if"}, {dir: "fix_advanced"}, {dir: "fix_cuddle_blocks"}, + {dir: "fix_cuddle_whole_blocks"}, {dir: "generated"}, {dir: "generated_file"}, {dir: "generic_handling"},