From ae07914dc4fc1af89bed44649db1627cb7b0ccb4 Mon Sep 17 00:00:00 2001 From: Miles Delahunty <4904544+mdelah@users.noreply.github.com> Date: Tue, 23 May 2023 18:10:09 +1000 Subject: [PATCH] ifelse: option to preserve variable scope (#832) * ifelse: option to preserve variable scope --- README.md | 6 +-- RULES_DESCRIPTIONS.md | 35 ++++++++++++++-- internal/ifelse/args.go | 11 +++++ internal/ifelse/branch.go | 43 +++++++++++++++---- internal/ifelse/branch_kind.go | 3 ++ internal/ifelse/chain.go | 1 + internal/ifelse/rule.go | 23 ++++++++--- rule/early-return.go | 11 +++-- rule/indent-error-flow.go | 11 +++-- rule/superfluous-else.go | 11 +++-- test/early-return_test.go | 3 ++ test/superfluous-else_test.go | 3 ++ testdata/early-return-scope.go | 66 ++++++++++++++++++++++++++++++ testdata/superfluous-else-scope.go | 64 +++++++++++++++++++++++++++++ 14 files changed, 261 insertions(+), 30 deletions(-) create mode 100644 internal/ifelse/args.go create mode 100644 testdata/early-return-scope.go create mode 100644 testdata/superfluous-else-scope.go diff --git a/README.md b/README.md index a4e852f..d402717 100644 --- a/README.md +++ b/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 | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index c52719b..02ad606 100644 --- a/RULES_DESCRIPTIONS.md +++ b/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 diff --git a/internal/ifelse/args.go b/internal/ifelse/args.go new file mode 100644 index 0000000..c6e647e --- /dev/null +++ b/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 +} diff --git a/internal/ifelse/branch.go b/internal/ifelse/branch.go index 0d51a0a..6e6036b 100644 --- a/internal/ifelse/branch.go +++ b/internal/ifelse/branch.go @@ -9,29 +9,37 @@ import ( // Branch contains information about a branch within an if-else chain. type Branch struct { BranchKind - Call // The function called at the end for kind Panic or Exit. + 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 +} diff --git a/internal/ifelse/branch_kind.go b/internal/ifelse/branch_kind.go index 7df276f..41601d1 100644 --- a/internal/ifelse/branch_kind.go +++ b/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 { diff --git a/internal/ifelse/chain.go b/internal/ifelse/chain.go index d6afebf..9891635 100644 --- a/internal/ifelse/chain.go +++ b/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 } diff --git a/internal/ifelse/rule.go b/internal/ifelse/rule.go index ec0db01..07ad456 100644 --- a/internal/ifelse/rule.go +++ b/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 diff --git a/rule/early-return.go b/rule/early-return.go index f5f8ff5..90a0acc 100644 --- a/rule/early-return.go +++ b/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) } diff --git a/rule/indent-error-flow.go b/rule/indent-error-flow.go index 4d62404..b80e648 100644 --- a/rule/indent-error-flow.go +++ b/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" } diff --git a/rule/superfluous-else.go b/rule/superfluous-else.go index 9dce59a..1ef67bf 100644 --- a/rule/superfluous-else.go +++ b/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()) } diff --git a/test/early-return_test.go b/test/early-return_test.go index 0f1b63b..a8f54f3 100644 --- a/test/early-return_test.go +++ b/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}}) } diff --git a/test/superfluous-else_test.go b/test/superfluous-else_test.go index ab68b30..0737777 100644 --- a/test/superfluous-else_test.go +++ b/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}}) } diff --git a/testdata/early-return-scope.go b/testdata/early-return-scope.go new file mode 100644 index 0000000..a9e8118 --- /dev/null +++ b/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 +} diff --git a/testdata/superfluous-else-scope.go b/testdata/superfluous-else-scope.go new file mode 100644 index 0000000..d1d6797 --- /dev/null +++ b/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 + } +}