From 65f0d196bfd4b9cad8825e2233dc10770f6fca88 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Sun, 28 May 2023 22:51:20 +0200 Subject: [PATCH 1/2] Allow defer statements to be cuddled if used two lines above This is an extension of allowing defer calls to `Close` to now be valid for any name and usage. This new rule will only be applied if there's exactly three statements grouped. Resolves #85 --- testdata/src/default_config/defer.go | 58 +++++++++++++ testdata/src/default_config/defer.go.golden | 62 ++++++++++++++ wsl.go | 92 ++++++++++++++------- 3 files changed, 184 insertions(+), 28 deletions(-) create mode 100644 testdata/src/default_config/defer.go create mode 100644 testdata/src/default_config/defer.go.golden diff --git a/testdata/src/default_config/defer.go b/testdata/src/default_config/defer.go new file mode 100644 index 0000000..27f424d --- /dev/null +++ b/testdata/src/default_config/defer.go @@ -0,0 +1,58 @@ +package testpkg + +import "fmt" + +func fn1() { + undoMaxProcs, err := maxprocsSet() + if err != nil { + return fmt.Errorf("failed to set GOMAXPROCS, err: %w", err) + } + defer undoMaxProcs() + + callback, x := getCb() + if x != b { + return + } + defer callback() + + resp, err := client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + db, err := OpenDB() + requireNoError(t, err) + defer db.Close() + + tx := BeginTx(db) + defer func() { + EndTx(tx, err) + }() + + thingOne := getOne() + thingTwo := getTwo() + defer thingOne.Close() +} + +func fn2() { + thingOne := getOne() + thingTwo := getTwo() + defer thingTwo.Close() // want "only one cuddle assignment allowed before defer statement" + + someF, err := getF() + if err != nil { + return err + } + someF() // want "expressions should not be cuddled with block" + + thingOne := getOne() + defer thingOne.Close() + thingTwo := getTwo() // want "assignments should only be cuddled with other assignments" + defer thingTwo.Close() // want "only one cuddle assignment allowed before defer statement" + + thingOne := getOne() + thingTwo := getTwo() + defer thingOne.Close() // want "only one cuddle assignment allowed before defer statement" + defer thingTwo.Close() +} diff --git a/testdata/src/default_config/defer.go.golden b/testdata/src/default_config/defer.go.golden new file mode 100644 index 0000000..aff6e96 --- /dev/null +++ b/testdata/src/default_config/defer.go.golden @@ -0,0 +1,62 @@ +package testpkg + +import "fmt" + +func fn1() { + undoMaxProcs, err := maxprocsSet() + if err != nil { + return fmt.Errorf("failed to set GOMAXPROCS, err: %w", err) + } + defer undoMaxProcs() + + callback, x := getCb() + if x != b { + return + } + defer callback() + + resp, err := client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + db, err := OpenDB() + requireNoError(t, err) + defer db.Close() + + tx := BeginTx(db) + defer func() { + EndTx(tx, err) + }() + + thingOne := getOne() + thingTwo := getTwo() + defer thingOne.Close() +} + +func fn2() { + thingOne := getOne() + + thingTwo := getTwo() + defer thingTwo.Close() // want "only one cuddle assignment allowed before defer statement" + + someF, err := getF() + if err != nil { + return err + } + + someF() // want "expressions should not be cuddled with block" + + thingOne := getOne() + defer thingOne.Close() + + thingTwo := getTwo() // want "assignments should only be cuddled with other assignments" + defer thingTwo.Close() // want "only one cuddle assignment allowed before defer statement" + + thingOne := getOne() + thingTwo := getTwo() + + defer thingOne.Close() // want "only one cuddle assignment allowed before defer statement" + defer thingTwo.Close() +} diff --git a/wsl.go b/wsl.go index 4700002..6e4bf8a 100644 --- a/wsl.go +++ b/wsl.go @@ -326,14 +326,38 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - moreThanOneStatementAbove := func() bool { - if i < 2 { + nStatementsBefore := func(n int) bool { + if i < n { return false } - statementBeforePreviousStatement := statements[i-2] + for j := 1; j < n; j++ { + s1 := statements[i-j] + s2 := statements[i-(j+1)] - return p.nodeStart(previousStatement)-1 == p.nodeEnd(statementBeforePreviousStatement) + if p.nodeStart(s1)-1 != p.nodeEnd(s2) { + return false + } + } + + return true + } + + nStatementsAfter := func(n int) bool { + if len(statements)-1 < i+n { + return false + } + + for j := 0; j < n; j++ { + s1 := statements[i+j] + s2 := statements[i+j+1] + + if p.nodeEnd(s1)+1 != p.nodeStart(s2) { + return false + } + } + + return true } isLastStatementInBlockOfOnlyTwoLines := func() bool { @@ -469,7 +493,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeIf) continue } @@ -576,7 +600,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { p.addWhitespaceBeforeError(t, reasonExprCuddlingNonAssignedVar) } case *ast.RangeStmt: - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeRange) continue } @@ -592,27 +616,39 @@ 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 nStatementsBefore(2) { + // We allow cuddling defer if the defer references something + // used two lines above. + // There are several reasons to why we do this. + // Originally there was a special use case only for "Close" + // + // https://github.com/bombsimon/wsl/issues/31 which links to + // resp, err := client.Do(req) + // if err != nil { + // return err + // } + // defer resp.Body.Close() + // + // After a discussion in a followup issue it makes sense to not + // only hard code `Close` but for anything that's referenced two + // statements above. + // + // https://github.com/bombsimon/wsl/issues/85 + // db, err := OpenDB() + // require.NoError(t, err) + // defer db.Close() + // + // All of this is only allowed if there's exactly three cuddled + // statements, otherwise the regular rules apply. + if !nStatementsBefore(3) && !nStatementsAfter(1) { + variablesTwoLinesAbove := append(p.findLHS(statements[i-2]), p.findRHS(statements[i-2])...) + if atLeastOneInListsMatch(rightHandSide, variablesTwoLinesAbove) { + continue + } } - } - if moreThanOneStatementAbove() { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeDefer) + continue } @@ -651,7 +687,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeFor) continue } @@ -669,7 +705,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { continue } - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeGo) continue } @@ -694,7 +730,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { p.addWhitespaceBeforeError(t, reasonGoFuncWithoutAssign) } case *ast.SwitchStmt: - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeSwitch) continue } @@ -707,7 +743,7 @@ func (p *processor) parseBlockStatements(statements []ast.Stmt) { } } case *ast.TypeSwitchStmt: - if moreThanOneStatementAbove() { + if nStatementsBefore(2) { reportNewlineTwoLinesAbove(t, statements[i-1], reasonOnlyOneCuddleBeforeTypeSwitch) continue } From 7edb3ef55d05f0ea02c478db68da59bef5d9325a Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Sun, 28 May 2023 23:01:44 +0200 Subject: [PATCH 2/2] Disable deprecated linters --- .golangci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index 336ad4b..986ccf3 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -62,11 +62,13 @@ linters: - nosnakecase - paralleltest - prealloc + - rowserrcheck - scopelint - structcheck - testpackage - varcheck - varnamelen + - wastedassign fast: false