ifelse: option to preserve variable scope (#832)

* ifelse: option to preserve variable scope

README.md

@@ -448,13 +448,13 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`package-comments`](./RULES_DESCRIPTIONS.md#package-comments)    |  n/a   | Package commenting conventions.                                  |   yes    |  no   |
 | [`range`](./RULES_DESCRIPTIONS.md#range)               |  n/a   | Prevents redundant variables when iterating over a collection.   |   yes    |  no   |
 | [`receiver-naming`](./RULES_DESCRIPTIONS.md#receiver-naming)     |  n/a   | Conventions around the naming of receivers.                      |   yes    |  no   |
-| [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)   |  n/a   | Prevents redundant else statements.                              |   yes    |  no   |
+| [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)   |  []string   | Prevents redundant else statements.                              |   yes    |  no   |
 | [`argument-limit`](./RULES_DESCRIPTIONS.md#argument-limit)      |  int (defaults to 8)  | Specifies the maximum number of arguments a function can receive |    no    |  no   |
 | [`cyclomatic`](./RULES_DESCRIPTIONS.md#cyclomatic)          |  int (defaults to 10)   | Sets restriction for maximum Cyclomatic complexity.              |    no    |  no   |
 | [`max-public-structs`](./RULES_DESCRIPTIONS.md#max-public-structs)  |  int (defaults to 5)  | The maximum number of public structs in a file.                  |    no    |  no   |
 | [`file-header`](./RULES_DESCRIPTIONS.md#file-header)         | string (defaults to none)| Header which each file should have.                              |    no    |  no   |
 | [`empty-block`](./RULES_DESCRIPTIONS.md#empty-block)         |  n/a   | Warns on empty code blocks                                       |    no    |  yes   |
-| [`superfluous-else`](./RULES_DESCRIPTIONS.md#superfluous-else)    |  n/a   | Prevents redundant else statements (extends [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)) |    no    |  no   |
+| [`superfluous-else`](./RULES_DESCRIPTIONS.md#superfluous-else)    |  []string   | Prevents redundant else statements (extends [`indent-error-flow`](./RULES_DESCRIPTIONS.md#indent-error-flow)) |    no    |  no   |
 | [`confusing-naming`](./RULES_DESCRIPTIONS.md#confusing-naming)    |  n/a   | Warns on methods with names that differ only by capitalization   |    no    |  no   |
 | [`get-return`](./RULES_DESCRIPTIONS.md#get-return)          |  n/a   | Warns on getters that do not yield any result                    |    no    |  no   |
 | [`modifies-parameter`](./RULES_DESCRIPTIONS.md#modifies-parameter)  |  n/a   | Warns on assignments to function parameters                      |    no    |  no   |
@@ -487,7 +487,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`cognitive-complexity`](./RULES_DESCRIPTIONS.md#cognitive-complexity)          |  int (defaults to 7) | Sets restriction for maximum Cognitive complexity.              |    no    |  no   |
 | [`string-of-int`](./RULES_DESCRIPTIONS.md#string-of-int)          |  n/a   | Warns on suspicious casts from int to string            |    no    |  yes   |
 | [`string-format`](./RULES_DESCRIPTIONS.md#string-format)          |  map   | Warns on specific string literals that fail one or more user-configured regular expressions            |    no    |  no   |
-| [`early-return`](./RULES_DESCRIPTIONS.md#early-return)          |  n/a   | Spots if-then-else statements where the predicate may be inverted to reduce nesting |    no    |  no   |
+| [`early-return`](./RULES_DESCRIPTIONS.md#early-return)          | []string   | Spots if-then-else statements where the predicate may be inverted to reduce nesting |    no    |  no   |
 | [`unconditional-recursion`](./RULES_DESCRIPTIONS.md#unconditional-recursion)          |  n/a   | Warns on function calls that will lead to (direct) infinite recursion |    no    |  no   |
 | [`identical-branches`](./RULES_DESCRIPTIONS.md#identical-branches)          |  n/a   | Spots if-then-else statements with identical `then` and `else` branches       |    no    |  no   |
 | [`defer`](./RULES_DESCRIPTIONS.md#defer)          |  map   |  Warns on some [defer gotchas](https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1)       |    no    |  no   |

RULES_DESCRIPTIONS.md

@@ -299,7 +299,7 @@ if cond {
 ```
 where the `if` condition may be inverted in order to reduce nesting:
 ```go
-if ! cond {
+if !cond {
   // do other thing
   return ...
 }
@@ -307,7 +307,16 @@ if ! cond {
 // do something
 ```
 
-_Configuration_: N/A
+_Configuration_: ([]string) rule flags. Available flags are:
+
+* _preserveScope_: do not suggest refactorings that would increase variable scope
+
+Example:
+
+```toml
+[rule.exported]
+  arguments =["preserveScope"]
+```
 
 ## empty-block
 
@@ -448,7 +457,16 @@ This rule highlights redundant _else-blocks_ that can be eliminated from the cod
 
 More information [here](https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow)
 
-_Configuration_: N/A
+_Configuration_: ([]string) rule flags. Available flags are:
+
+* _preserveScope_: do not suggest refactorings that would increase variable scope
+
+Example:
+
+```toml
+[rule.exported]
+  arguments =["preserveScope"]
+```
 
 ## imports-blacklist
 
@@ -624,7 +642,16 @@ To accept the `inline` option in JSON tags (and `outline` and `gnu` in BSON tags
 _Description_: To improve the readability of code, it is recommended to reduce the indentation as much as possible.
 This rule highlights redundant _else-blocks_ that can be eliminated from the code.
 
-_Configuration_: N/A
+_Configuration_: ([]string) rule flags. Available flags are:
+
+* _preserveScope_: do not suggest refactorings that would increase variable scope
+
+Example:
+
+```toml
+[rule.exported]
+  arguments =["preserveScope"]
+```
 
 ## time-equal
 

internal/ifelse/args.go

@@ -0,0 +1,11 @@
+package ifelse
+
+// PreserveScope is a configuration argument that prevents suggestions
+// that would enlarge variable scope
+const PreserveScope = "preserveScope"
+
+// Args contains arguments common to the early-return, indent-error-flow
+// and superfluous-else rules (currently just preserveScope)
+type Args struct {
+	PreserveScope bool
+}

internal/ifelse/branch.go

@@ -10,28 +10,36 @@ import (
 type Branch struct {
 	BranchKind
 	Call          // The function called at the end for kind Panic or Exit.
+	HasDecls bool // The branch has one or more declarations (at the top level block)
 }
 
 // BlockBranch gets the Branch of an ast.BlockStmt.
 func BlockBranch(block *ast.BlockStmt) Branch {
 	blockLen := len(block.List)
 	if blockLen == 0 {
-		return Branch{BranchKind: Empty}
+		return Empty.Branch()
 	}
 
-	switch stmt := block.List[blockLen-1].(type) {
+	branch := StmtBranch(block.List[blockLen-1])
+	branch.HasDecls = hasDecls(block)
+	return branch
+}
+
+// StmtBranch gets the Branch of an ast.Stmt.
+func StmtBranch(stmt ast.Stmt) Branch {
+	switch stmt := stmt.(type) {
 	case *ast.ReturnStmt:
-		return Branch{BranchKind: Return}
+		return Return.Branch()
 	case *ast.BlockStmt:
 		return BlockBranch(stmt)
 	case *ast.BranchStmt:
 		switch stmt.Tok {
 		case token.BREAK:
-			return Branch{BranchKind: Break}
+			return Break.Branch()
 		case token.CONTINUE:
-			return Branch{BranchKind: Continue}
+			return Continue.Branch()
 		case token.GOTO:
-			return Branch{BranchKind: Goto}
+			return Goto.Branch()
 		}
 	case *ast.ExprStmt:
 		fn, ok := ExprCall(stmt)
@@ -42,9 +50,12 @@ func BlockBranch(block *ast.BlockStmt) Branch {
 		if ok {
 			return Branch{BranchKind: kind, Call: fn}
 		}
+	case *ast.EmptyStmt:
+		return Empty.Branch()
+	case *ast.LabeledStmt:
+		return StmtBranch(stmt.Stmt)
 	}
-
-	return Branch{BranchKind: Regular}
+	return Regular.Branch()
 }
 
 // String returns a brief string representation
@@ -66,3 +77,17 @@ func (b Branch) LongString() string {
 		return b.BranchKind.LongString()
 	}
 }
+
+func hasDecls(block *ast.BlockStmt) bool {
+	for _, stmt := range block.List {
+		switch stmt := stmt.(type) {
+		case *ast.DeclStmt:
+			return true
+		case *ast.AssignStmt:
+			if stmt.Tok == token.DEFINE {
+				return true
+			}
+		}
+	}
+	return false
+}

internal/ifelse/branch_kind.go

@@ -49,6 +49,9 @@ func (k BranchKind) Deviates() bool {
 	}
 }
 
+// Branch returns a Branch with the given kind
+func (k BranchKind) Branch() Branch { return Branch{BranchKind: k} }
+
 // String returns a brief string representation
 func (k BranchKind) String() string {
 	switch k {

internal/ifelse/chain.go

@@ -6,4 +6,5 @@ type Chain struct {
 	Else                 Branch // what happens at the end of the "else" block
 	HasInitializer       bool   // is there an "if"-initializer somewhere in the chain?
 	HasPriorNonDeviating bool   // is there a prior "if" block that does NOT deviate control flow?
+	AtBlockEnd           bool   // whether the chain is placed at the end of the surrounding block
 }

internal/ifelse/rule.go

@@ -9,7 +9,7 @@ import (
 
 // Rule is an interface for linters operating on if-else chains
 type Rule interface {
-	CheckIfElse(chain Chain) (failMsg string)
+	CheckIfElse(chain Chain, args Args) (failMsg string)
 }
 
 // Apply evaluates the given Rule on if-else chains found within the given AST,
@@ -28,8 +28,13 @@ type Rule interface {
 //
 // Only the block following "bar" is linted. This is because the rules that use this function
 // do not presently have anything to say about earlier blocks in the chain.
-func Apply(rule Rule, node ast.Node, target Target) []lint.Failure {
+func Apply(rule Rule, node ast.Node, target Target, args lint.Arguments) []lint.Failure {
 	v := &visitor{rule: rule, target: target}
+	for _, arg := range args {
+		if arg == PreserveScope {
+			v.args.PreserveScope = true
+		}
+	}
 	ast.Walk(v, node)
 	return v.failures
 }
@@ -38,15 +43,22 @@ type visitor struct {
 	failures []lint.Failure
 	target   Target
 	rule     Rule
+	args     Args
 }
 
 func (v *visitor) Visit(node ast.Node) ast.Visitor {
-	ifStmt, ok := node.(*ast.IfStmt)
+	block, ok := node.(*ast.BlockStmt)
 	if !ok {
 		return v
 	}
 
-	v.visitChain(ifStmt, Chain{})
+	for i, stmt := range block.List {
+		if ifStmt, ok := stmt.(*ast.IfStmt); ok {
+			v.visitChain(ifStmt, Chain{AtBlockEnd: i == len(block.List)-1})
+			continue
+		}
+		ast.Walk(v, stmt)
+	}
 	return nil
 }
 
@@ -73,8 +85,9 @@ func (v *visitor) visitChain(ifStmt *ast.IfStmt, chain Chain) {
 	case *ast.BlockStmt:
 		// look for other if-else chains nested inside this else { } block
 		ast.Walk(v, elseBlock)
+
 		chain.Else = BlockBranch(elseBlock)
-		if failMsg := v.rule.CheckIfElse(chain); failMsg != "" {
+		if failMsg := v.rule.CheckIfElse(chain, v.args); failMsg != "" {
 			if chain.HasInitializer {
 				// if statement has a := initializer, so we might need to move the assignment
 				// onto its own line in case the body references it

rule/early-return.go

@@ -12,8 +12,8 @@ import (
 type EarlyReturnRule struct{}
 
 // Apply applies the rule to given file.
-func (e *EarlyReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
-	return ifelse.Apply(e, file.AST, ifelse.TargetIf)
+func (e *EarlyReturnRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
+	return ifelse.Apply(e, file.AST, ifelse.TargetIf, args)
 }
 
 // Name returns the rule name.
@@ -22,7 +22,7 @@ func (*EarlyReturnRule) Name() string {
 }
 
 // CheckIfElse evaluates the rule against an ifelse.Chain.
-func (e *EarlyReturnRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
+func (e *EarlyReturnRule) CheckIfElse(chain ifelse.Chain, args ifelse.Args) (failMsg string) {
 	if !chain.Else.Deviates() {
 		// this rule only applies if the else-block deviates control flow
 		return
@@ -39,6 +39,11 @@ func (e *EarlyReturnRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
 		return
 	}
 
+	if args.PreserveScope && !chain.AtBlockEnd && (chain.HasInitializer || chain.If.HasDecls) {
+		// avoid increasing variable scope
+		return
+	}
+
 	if chain.If.IsEmpty() {
 		return fmt.Sprintf("if c { } else { %[1]v } can be simplified to if !c { %[1]v }", chain.Else)
 	}

rule/indent-error-flow.go

@@ -9,8 +9,8 @@ import (
 type IndentErrorFlowRule struct{}
 
 // Apply applies the rule to given file.
-func (e *IndentErrorFlowRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
-	return ifelse.Apply(e, file.AST, ifelse.TargetElse)
+func (e *IndentErrorFlowRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
+	return ifelse.Apply(e, file.AST, ifelse.TargetElse, args)
 }
 
 // Name returns the rule name.
@@ -19,7 +19,7 @@ func (*IndentErrorFlowRule) Name() string {
 }
 
 // CheckIfElse evaluates the rule against an ifelse.Chain.
-func (e *IndentErrorFlowRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
+func (e *IndentErrorFlowRule) CheckIfElse(chain ifelse.Chain, args ifelse.Args) (failMsg string) {
 	if !chain.If.Deviates() {
 		// this rule only applies if the if-block deviates control flow
 		return
@@ -36,5 +36,10 @@ func (e *IndentErrorFlowRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
 		return
 	}
 
+	if args.PreserveScope && !chain.AtBlockEnd && (chain.HasInitializer || chain.Else.HasDecls) {
+		// avoid increasing variable scope
+		return
+	}
+
 	return "if block ends with a return statement, so drop this else and outdent its block"
 }

rule/superfluous-else.go

@@ -10,8 +10,8 @@ import (
 type SuperfluousElseRule struct{}
 
 // Apply applies the rule to given file.
-func (e *SuperfluousElseRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
-	return ifelse.Apply(e, file.AST, ifelse.TargetElse)
+func (e *SuperfluousElseRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
+	return ifelse.Apply(e, file.AST, ifelse.TargetElse, args)
 }
 
 // Name returns the rule name.
@@ -20,7 +20,7 @@ func (*SuperfluousElseRule) Name() string {
 }
 
 // CheckIfElse evaluates the rule against an ifelse.Chain.
-func (e *SuperfluousElseRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
+func (e *SuperfluousElseRule) CheckIfElse(chain ifelse.Chain, args ifelse.Args) (failMsg string) {
 	if !chain.If.Deviates() {
 		// this rule only applies if the if-block deviates control flow
 		return
@@ -37,5 +37,10 @@ func (e *SuperfluousElseRule) CheckIfElse(chain ifelse.Chain) (failMsg string) {
 		return
 	}
 
+	if args.PreserveScope && !chain.AtBlockEnd && (chain.HasInitializer || chain.Else.HasDecls) {
+		// avoid increasing variable scope
+		return
+	}
+
 	return fmt.Sprintf("if block ends with %v, so drop this else and outdent its block", chain.If.LongString())
 }

test/early-return_test.go

@@ -3,10 +3,13 @@ package test
 import (
 	"testing"
 
+	"github.com/mgechev/revive/internal/ifelse"
+	"github.com/mgechev/revive/lint"
 	"github.com/mgechev/revive/rule"
 )
 
 // TestEarlyReturn tests early-return rule.
 func TestEarlyReturn(t *testing.T) {
 	testRule(t, "early-return", &rule.EarlyReturnRule{})
+	testRule(t, "early-return-scope", &rule.EarlyReturnRule{}, &lint.RuleConfig{Arguments: []interface{}{ifelse.PreserveScope}})
 }

test/superfluous-else_test.go

@@ -3,10 +3,13 @@ package test
 import (
 	"testing"
 
+	"github.com/mgechev/revive/internal/ifelse"
+	"github.com/mgechev/revive/lint"
 	"github.com/mgechev/revive/rule"
 )
 
 // TestSuperfluousElse rule.
 func TestSuperfluousElse(t *testing.T) {
 	testRule(t, "superfluous-else", &rule.SuperfluousElseRule{})
+	testRule(t, "superfluous-else-scope", &rule.SuperfluousElseRule{}, &lint.RuleConfig{Arguments: []interface{}{ifelse.PreserveScope}})
 }

testdata/early-return-scope.go

@@ -0,0 +1,66 @@
+// Test data for the early-return rule with preserveScope option enabled
+
+package fixtures
+
+func fn1() {
+	// No initializer, match as normal
+	if cond { //   MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
+		fn2()
+	} else {
+		return
+	}
+}
+
+func fn2() {
+	// Moving the declaration of x here is fine since it goes out of scope either way
+	if x := fn1(); x != nil { //   MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } ... (move short variable declaration to its own line if necessary)/
+		fn2()
+	} else {
+		return
+	}
+}
+
+func fn3() {
+	// Don't want to move the declaration of x here since it stays in scope afterward
+	if x := fn1(); x != nil {
+		fn2()
+	} else {
+		return
+	}
+	x := fn2()
+	fn3(x)
+}
+
+func fn4() {
+	if cond {
+		var x = fn2()
+		fn3(x)
+	} else {
+		return
+	}
+	// Don't want to move the declaration of x here since it stays in scope afterward
+	y := fn2()
+	fn3(y)
+}
+
+func fn5() {
+	if cond {
+		x := fn2()
+		fn3(x)
+	} else {
+		return
+	}
+	// Don't want to move the declaration of x here since it stays in scope afterward
+	y := fn2()
+	fn3(y)
+}
+
+func fn6() {
+	if cond { //   MATCH /if c { ... } else { ... return } can be simplified to if !c { ... return } .../
+		x := fn2()
+		fn3(x)
+	} else {
+		return
+	}
+	// Moving x here is fine since it goes out of scope anyway
+}

testdata/superfluous-else-scope.go

@@ -0,0 +1,64 @@
+// Test data for the superfluous-else rule with preserveScope option enabled
+
+package fixtures
+
+func fn1() {
+	for {
+		// No initializer, match as normal
+		if cond {
+			continue
+		} else { // MATCH /if block ends with a continue statement, so drop this else and outdent its block/
+			fn2()
+		}
+	}
+}
+
+func fn2() {
+	for {
+		// Moving the declaration of x here is fine since it goes out of scope either way
+		if x := fn1(); x != nil {
+			continue
+		} else { // MATCH /if block ends with a continue statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)/
+			fn2()
+		}
+	}
+}
+
+func fn3() {
+	for {
+		// Don't want to move the declaration of x here since it stays in scope afterward
+		if x := fn1(); x != nil {
+			continue
+		} else {
+			fn2()
+		}
+		x := fn2()
+		fn3(x)
+	}
+}
+
+func fn4() {
+	for {
+		if cond {
+			continue
+		} else {
+			x := fn1()
+			fn2(x)
+		}
+		// Don't want to move the declaration of x here since it stays in scope afterward
+		y := fn2()
+		fn3(y)
+	}
+}
+
+func fn4() {
+	for {
+		if cond {
+			continue
+		} else { // MATCH /if block ends with a continue statement, so drop this else and outdent its block/
+			x := fn1()
+			fn2(x)
+		}
+		// Moving x here is fine since it goes out of scope anyway
+	}
+}