Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to cuddle assignments for a whole block #163

Merged
merged 3 commits into from
Feb 24, 2025
Merged

Add support to cuddle assignments for a whole block #163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a4ee0e
Support to cuddle assignments for a whole block
jacobdrury Feb 21, 2025
68bb1ab
Implement PR suggestions
jacobdrury Feb 24, 2025
dd00deb
Update doc/configuration.md
jacobdrury Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
}
}
136 changes: 136 additions & 0 deletions testdata/src/default_config/fix_cuddle_whole_blocks/fix_cuddle_whole_blocks.go.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}
}
}
50 changes: 39 additions & 11 deletions wsl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to add this as an opt-in and keep the old behavior. Ideally we could use the same code/walk but just return after the first variable is found if we didn't opt in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to use the old code if the new flag isn't enabled that way we don't introduce any unintended new behavior on accident by using the new ast.Inspect approach. Lmk if you want to re-visit this.

if firstBodyStatement != nil {
	if p.config.CuddleUsedInBlock {
		identifiersUsedInBlock = p.findUsedVariablesInStatement(stmt)
	} else {
		identifiersUsedInBlock = append(p.findLHS(firstBodyStatement), p.findRHS(firstBodyStatement)...)
	}
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the old behaviour is not what the rule describes: "must be used in the iteration". 🤔

If you wanted to keep the old behaviour it might be better to re-introduce it as a new rule with a more accurate error message so that the different messages can be interpreted more easily, rather than a message which may be misleading/inaccurate depending on the configuration of the rule producing that message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats why I originally introduced this as a fix rather than a new flag. There was no mention of the "first line only" rule. All the rules just said that it "must be used in the iteration"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr - there's a bunch of bugs and bad behavior here, no need to adress that for now. I think it should be addressed, but I'm fine isolating this PR to only address the new feature flag and don't try to fix this. The flag kan be added in a minor release directly and we can fix the old stuff in a future breaking change.

Yeah this is a bug and bad documentation, it stems from way back when each different kind of block was seen as separate. Code changed and never really got adapted (which is pretty clear from the whole code base tbh).

The only mention of it is here for range specifically. It's just a comment on the second example. It's also documented here as code docs, but that's not very clear from a user perspective.

I had a quick look now and it seems to be missing from switch statements but exist for type switch.

There are more bugs with defer and go statements passing the whole Body causing false negatives. These two passes but shouldn't according to the other logc.

y := 1
go func() {
	_ = 1
	y = 1
}()
y := 1
defer func() {
	_ = 1
	y = 1
}()


if firstBodyStatement != nil {
calledOrAssignedFirstInBlock = append(p.findLHS(firstBodyStatement), p.findRHS(firstBodyStatement)...)
blockUsedVars = p.findUsedVariablesInStatement(stmt)
}

var (
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -507,7 +509,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) {
continue
}

if atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) {
if atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) {
continue
}

Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -679,7 +681,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) {
}
}

if atLeastOneInListsMatch(assignedOnLineAbove, calledOrAssignedFirstInBlock) {
if atLeastOneInListsMatch(assignedOnLineAbove, blockUsedVars) {
continue
}

Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions wsl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down