Repository: bombsimon/wsl
Pull request #66 (2020-02-03 02:20:41)

Enforce cuddling err checks
For #65

This is the first time I've worked with an AST, it was pretty awesome... Sometimes frustrating.. but awesome :P 

I set the new feature to Off by default.

coveralls: ## Pull Request Test Coverage Report for [Build 156](https://coveralls.io/builds/28486892)

* **27** of **27**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.3%**) to **89.433%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/28486892/badge)](https://coveralls.io/builds/28486892) |
| :-- | --: |
| Change from base [Build 155](https://coveralls.io/builds/27594077): |  0.3% |
| Covered Lines: | 584 |
| Relevant Lines: | 653 |

---
##### 💛  - [Coveralls](https://coveralls.io)

bombsimon: Nice work, thanks!

I agree that AST can be a love-hate thing. But I hope my way of working with it doesn't make your first experience bad. 😃 You really have to appreciate the ast and token package from Go standard library though, very powerfull!

The next step for this package will be to work with fixers using the [analysis](https://godoc.org/golang.org/x/tools/go/analysis) package. I also need to find a better way to group comments and keep track of more than two statements at the same time.
JDiPierro: Thanks so much for the quick review!! :D I've addressed the feedback and it really did cut down on the amount of code!! I thought I'd tried some of these suggestions before and they broke other tests, but in my continual refactoring and fixing I must have corrected whatever issue resulted in that.

Since I broke the assumption that "If we made it to the case statement we're not cuddled" I had to add a few extra `if cuddledWithLastStmt` checks. Breaking that assumption feels kind of bad, since it complicates how you have to think about the code. My first attempt at this did the whole thing outside the case statement, but I wanted to re-use the LHS/RHS vars you already had and not have to gather those twice.

I should have time to write up some docs today :)
JDiPierro: Docs are done :D 

Unrelated: I was thinking about pulling out all of the error strings into constants at the top of `wsl.go`, how would you feel about that?
JDiPierro: Sorry for the force-pushes, just cleaning up some extra whitespace I noticed, didn't think they deserved their own commit :P I'm all finished with this PR if you have no further feedback :D I'll open a separate one for the error constants.
bombsimon: > Sorry for the force-pushes, just cleaning up some extra whitespace I noticed, didn't think they deserved their own commit :P I'm all finished with this PR if you have no further feedback :D I'll open a separate one for the error constants.

No worries, force push all you want in your own branch, I don't like a messy git history either. Altough I'll probably squash when merging anyway so don't think too much about that :)
bombsimon: Everything seems to work as intended now!

* Tests with `MustCuddleErrCheckAndAssign` defualting to true
* Tests with `MustCuddleErrCheckAndAssign` defaulting to false
* `wsl` on the project with `MustCuddleErrCheckAndAssign` set to false - no errors
* `wsl` on the project with `MustCuddleErrCheckAndAssign` set to true - warns on line 171 (as expected)

I'll merge this but I'm not sure when I'll tag a new release. This isn't breaking since default is false but I think I want to test this out a bit since it's bundled with `golangci-lint` and if someone installs it with `go get` a patch och minor release would update the wsl dependency.

If you're using `golangci-lint` I hope you're OK bulding it manually to include this new feature. Eitehr way it must be configured to be set to enabled.

Patch:
diff --git a/README.md b/README.md
index 1d4aa14..c5068a8 100644
--- a/README.md
+++ b/README.md
@@ -109,6 +109,7 @@ feel free to raise an [issue](https://github.com/bombsimon/wsl/issues/new).
 * [If statements should only be cuddled with assignments used in the if
   statement
   itself](doc/rules.md#if-statements-should-only-be-cuddled-with-assignments-used-in-the-if-statement-itself)
+* [If statements that check an error must be cuddled with the statement that assigned the error](doc/rules.md#if-statements-that-check-an-error-must-be-cuddled-with-the-statement-that-assigned-the-error)
 * [Only cuddled expressions if assigning variable or using from line
   above](doc/rules.md#only-cuddled-expressions-if-assigning-variable-or-using-from-line-above)
 * [Only one cuddle assignment allowed before defer statement](doc/rules.md#only-one-cuddle-assignment-allowed-before-defer-statement)
diff --git a/cmd/wsl/main.go b/cmd/wsl/main.go
index bcf900a..8f48458 100644
--- a/cmd/wsl/main.go
+++ b/cmd/wsl/main.go
@@ -35,6 +35,7 @@ func main() {
 	flag.BoolVar(&config.AllowMultiLineAssignCuddle, "allow-multi-line-assign", true, "Allow cuddling with multi line assignments")
 	flag.BoolVar(&config.AllowCuddleDeclaration, "allow-cuddle-declarations", false, "Allow declarations to be cuddled")
 	flag.BoolVar(&config.AllowTrailingComment, "allow-trailing-comment", false, "Allow blocks to end with a comment")
+	flag.BoolVar(&config.MustCuddleErrCheckAndAssign, "enforce-err-cuddling", false, "Enforce cuddling of error checks with error var assignment")
 	flag.IntVar(&config.CaseForceTrailingWhitespaceLimit, "force-case-trailing-whitespace", 0, "Force newlines for case blocks > this number.")
 
 	flag.Parse()
diff --git a/doc/configuration.md b/doc/configuration.md
index 34e3f5b..cc18f2e 100644
--- a/doc/configuration.md
+++ b/doc/configuration.md
@@ -238,3 +238,28 @@ if 1 == 1 {
     */
 }
 ```
+
+### [enforce-err-cuddling](rules.md#if-statements-that-check-an-error-must-be-cuddled-with-the-statement-that-assigned-the-error)
+
+Enforces that an `if` statement checking an error variable is cuddled with the line that assigned that error variable.
+
+> Default value: false
+
+Supported when false:
+
+```go
+err := ErrorProducingFunc()
+
+if err != nil {
+    return err
+}
+```
+
+Required when true:
+
+```go
+err := ErrorProducingFunc()
+if err != nil {
+    return err
+}
+```
diff --git a/doc/rules.md b/doc/rules.md
index c9de0e0..cae6213 100644
--- a/doc/rules.md
+++ b/doc/rules.md
@@ -983,3 +983,31 @@ switch inSwitch.(type) {
 // cases
 }
 ```
+
+---
+
+### If statements that check an error must be cuddled with the statement that assigned the error
+
+> Can be configured, see [configuration
+documentation](configuration.md#enforce-err-cuddling)
+
+When an `if` statement checks a variable named `err`, which was assigned on the line above, it should be cuddled with that assignment. This makes clear the relationship between the error check and the call that may have resulted in an error.
+
+```go
+err := ErrorProducingFunc()
+
+if err != nil {
+    return err
+}
+```
+
+#### Recommended amendment
+
+Remove the empty line between the assignment and the error check.
+
+```go
+err := ErrorProducingFunc()
+if err != nil {
+    return err
+}
+```
diff --git a/wsl.go b/wsl.go
index ea8d4eb..d270b7d 100644
--- a/wsl.go
+++ b/wsl.go
@@ -79,6 +79,23 @@ type Configuration struct {
 	//  allow := thisAssignment()
 	//  mu.Unlock()
 	AllowCuddleWithRHS []string
+
+	// MustCuddleErrCheckAndAssign will cause an error when an If statement that
+	// checks an error variable doesn't cuddle with the assignment of that variable.
+	// This defaults to false but setting it to true will cause the following
+	// to generate an error:
+	//
+	// err := ProduceError()
+	//
+	// if err != nil {
+	//     return err
+	// }
+	MustCuddleErrCheckAndAssign bool
+
+	// When MustCuddleErrCheckAndAssign is enabled this is a list of names
+	// used for error variables to check for in the conditional.
+	// Defaults to just "err"
+	ErrorVariableNames []string
 }
 
 // DefaultConfig returns default configuration
@@ -91,6 +108,8 @@ func DefaultConfig() Configuration {
 		CaseForceTrailingWhitespaceLimit: 0,
 		AllowCuddleWithCalls:             []string{"Lock", "RLock"},
 		AllowCuddleWithRHS:               []string{"Unlock", "RUnlock"},
+		MustCuddleErrCheckAndAssign:      false,
+		ErrorVariableNames:               []string{"err"},
 	}
 }
 
@@ -211,14 +230,15 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) {
 		}
 
 		previousStatement := statements[i-1]
+		cuddledWithLastStmt := p.nodeEnd(previousStatement) == p.nodeStart(stmt)-1
 
-		// If the last statement didn't end one line above the current statement
-		// we know we're not cuddled so just move on.
-		if p.nodeEnd(previousStatement) != p.nodeStart(stmt)-1 {
+		// If we're not cuddled and we don't need to enforce err-check cuddling
+		// then we can bail out here
+		if !cuddledWithLastStmt && !p.config.MustCuddleErrCheckAndAssign {
 			continue
 		}
 
-		// We know we're cuddled, extract assigned variables on the line above
+		// Extract assigned variables on the line above
 		// which is the only thing we allow cuddling with. If the assignment is
 		// made over multiple lines we should not allow cuddling.
 		var assignedOnLineAbove []string
@@ -228,18 +248,18 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) {
 		var calledOnLineAbove []string
 
 		// Check if the previous statement spans over multiple lines.
-		var isMultiLineAssignment = p.nodeStart(previousStatement) != p.nodeStart(stmt)-1
+		var cuddledWithMultiLineAssignment = cuddledWithLastStmt && p.nodeStart(previousStatement) != p.nodeStart(stmt)-1
 
 		// Ensure previous line is not a multi line assignment and if not get
 		// rightAndLeftHandSide assigned variables.
-		if !isMultiLineAssignment {
+		if !cuddledWithMultiLineAssignment {
 			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 cuddledWithMultiLineAssignment && p.config.AllowMultiLineAssignCuddle {
 			if _, ok := previousStatement.(*ast.AssignStmt); ok {
 				assignedOnLineAbove = p.findLHS(previousStatement)
 			}
@@ -300,8 +320,28 @@ func (p *Processor) parseBlockStatements(statements []ast.Stmt) {
 			return false
 		}
 
+		// If it's not an if statement and we're not cuddled move on. The only
+		// reason we need to keep going for if statements is to check if we
+		// should be cuddled with an error check.
+		if _, ok := stmt.(*ast.IfStmt); !ok {
+			if !cuddledWithLastStmt {
+				continue
+			}
+		}
+
 		switch t := stmt.(type) {
 		case *ast.IfStmt:
+			if !cuddledWithLastStmt {
+				checkingErr := atLeastOneInListsMatch(rightAndLeftHandSide, p.config.ErrorVariableNames)
+				if checkingErr {
+					if atLeastOneInListsMatch(assignedOnLineAbove, p.config.ErrorVariableNames) {
+						p.addError(t.Pos(), "if statements that check an error must be cuddled with the statement that assigned the error")
+					}
+				}
+
+				continue
+			}
+
 			if len(assignedOnLineAbove) == 0 {
 				p.addError(t.Pos(), "if statements should only be cuddled with assignments")
 				continue


Test patch:
diff --git a/wsl_test.go b/wsl_test.go
index e4c5bc8..bcac1ec 100644
--- a/wsl_test.go
+++ b/wsl_test.go
@@ -1480,6 +1480,143 @@ func TestWithConfig(t *testing.T) {
 				"case block should end with newline at this size",
 			},
 		},
+		{
+			description:         "must cuddle error checks with the error assignment",
+			customConfig: &Configuration{
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				err := errors.New("bad thing")
+
+				if err != nil {
+					fmt.Println("I'm OK")
+				}
+			}`),
+			expectedErrorStrings: []string{"if statements that check an error must be cuddled with the statement that assigned the error"},
+		},
+		{
+			description:         "must cuddle error checks with the error assignment multivalue",
+			customConfig: &Configuration{
+				AllowMultiLineAssignCuddle:       true,
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				b, err := FSByte(useLocal, name)
+
+				if err != nil {
+					panic(err)
+				}
+			}`),
+			expectedErrorStrings: []string{"if statements that check an error must be cuddled with the statement that assigned the error"},
+		},
+		{
+			description:         "must cuddle error checks with the error assignment only on assignment",
+			customConfig: &Configuration{
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				var (
+					err error
+					once sync.Once
+				)
+
+				once.Do(func() {
+					err = ProduceError()
+				})
+			
+				if err != nil {
+					return nil, err
+				}
+			}`),
+			expectedErrorStrings: []string{},
+		},
+		{
+			description:         "must cuddle error checks with the error assignment multivalue NoError",
+			customConfig: &Configuration{
+				AllowMultiLineAssignCuddle:       true,
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				b, err := FSByte(useLocal, name)
+				if err != nil {
+					panic(err)
+				}
+			}`),
+			expectedErrorStrings: []string{},
+		},
+		{
+			description:         "must cuddle error checks with the error assignment known err",
+			customConfig: &Configuration{
+				AllowMultiLineAssignCuddle:       true,
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				result, err := FetchSomething()
+
+				if err == sql.ErrNoRows {
+					return []Model{}
+				}
+			}`),
+			expectedErrorStrings: []string{"if statements that check an error must be cuddled with the statement that assigned the error"},
+		},
+		{
+			description:         "err-check cuddle enforcement doesn't generate false-positives.",
+			customConfig: &Configuration{
+				AllowMultiLineAssignCuddle:       true,
+				MustCuddleErrCheckAndAssign:      true,
+				ErrorVariableNames:               []string{"err"},
+			},
+			code: []byte(`package main
+
+			import "errors"
+
+			func main() {
+				result, err := FetchSomething()
+				if err == sql.ErrNoRows {
+					return []Model{}
+				}
+
+				foo := generateFoo()
+				if foo == "bar" {
+ 				    handleBar()
+                }
+
+				var baz []string
+
+				err = loadStuff(&baz)
+				if err != nil{
+					return nil
+				}
+
+				return baz
+			}`),
+			expectedErrorStrings: []string{},
+		},
 	}
 
 	for _, tc := range cases {