Skip to content

Commit

Permalink
adds [allowRegex] parameter for unused-parameter and `unused-receiv…
Browse files Browse the repository at this point in the history
…er` rules (#858)
  • Loading branch information
comdiv authored Aug 11, 2023
1 parent 4c84a17 commit b31eb18
Show file tree
Hide file tree
Showing 7 changed files with 179 additions and 13 deletions.
27 changes: 25 additions & 2 deletions RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -735,13 +735,36 @@ _Configuration_: N/A

_Description_: This rule warns on unused parameters. Functions or methods with unused parameters can be a symptom of an unfinished refactoring or a bug.

_Configuration_: N/A
_Configuration_: Supports arguments with single of `map[string]any` with option `allowRegex` to provide additional to `_` mask to allowed unused parameter names, for example:

```toml
[rule.unused-parameter]
Arguments = [ { allowRegex = "^_" } ]
```

allows any names started with `_`, not just `_` itself:

```go
func SomeFunc(_someObj *MyStruct) {} // matches rule
```

## unused-receiver

_Description_: This rule warns on unused method receivers. Methods with unused receivers can be a symptom of an unfinished refactoring or a bug.

_Configuration_: N/A
_Configuration_: Supports arguments with single of `map[string]any` with option `allowRegex` to provide additional to `_` mask to allowed unused receiver names, for example:

```toml
[rule.unused-receiver]
Arguments = [ { allowRegex = "^_" } ]
```

allows any names started with `_`, not just `_` itself:

```go
func (_my *MyStruct) SomeMethod() {} // matches rule
```


## use-any

Expand Down
67 changes: 61 additions & 6 deletions rule/unused-param.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,72 @@ package rule
import (
"fmt"
"go/ast"
"regexp"
"sync"

"github.com/mgechev/revive/lint"
)

// UnusedParamRule lints unused params in functions.
type UnusedParamRule struct{}
type UnusedParamRule struct {
configured bool
// regex to check if some name is valid for unused parameter, "^_$" by default
allowRegex *regexp.Regexp
failureMsg string
sync.Mutex
}

func (r *UnusedParamRule) configure(args lint.Arguments) {
r.Lock()
defer r.Unlock()

if r.configured {
return
}
r.configured = true

// while by default args is an array, i think it's good to provide structures inside it by default, not arrays or primitives
// it's more compatible to JSON nature of configurations
var allowedRegexStr string
if len(args) == 0 {
allowedRegexStr = "^_$"
r.failureMsg = "parameter '%s' seems to be unused, consider removing or renaming it as _"
} else {
// Arguments = [{}]
options := args[0].(map[string]interface{})
// Arguments = [{allowedRegex="^_"}]

if allowedRegexParam, ok := options["allowRegex"]; ok {
allowedRegexStr, ok = allowedRegexParam.(string)
if !ok {
panic(fmt.Errorf("error configuring %s rule: allowedRegex is not string but [%T]", r.Name(), allowedRegexParam))
}
}
}
var err error
r.allowRegex, err = regexp.Compile(allowedRegexStr)
if err != nil {
panic(fmt.Errorf("error configuring %s rule: allowedRegex is not valid regex [%s]: %v", r.Name(), allowedRegexStr, err))
}

if r.failureMsg == "" {
r.failureMsg = "parameter '%s' seems to be unused, consider removing or renaming it to match " + r.allowRegex.String()
}
}

// Apply applies the rule to given file.
func (*UnusedParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (r *UnusedParamRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
r.configure(args)
var failures []lint.Failure

onFailure := func(failure lint.Failure) {
failures = append(failures, failure)
}

w := lintUnusedParamRule{onFailure: onFailure}
w := lintUnusedParamRule{
onFailure: onFailure,
allowRegex: r.allowRegex,
failureMsg: r.failureMsg,
}

ast.Walk(w, file.AST)

Expand All @@ -31,7 +81,9 @@ func (*UnusedParamRule) Name() string {
}

type lintUnusedParamRule struct {
onFailure func(lint.Failure)
onFailure func(lint.Failure)
allowRegex *regexp.Regexp
failureMsg string
}

func (w lintUnusedParamRule) Visit(node ast.Node) ast.Visitor {
Expand Down Expand Up @@ -65,12 +117,15 @@ func (w lintUnusedParamRule) Visit(node ast.Node) ast.Visitor {

for _, p := range n.Type.Params.List {
for _, n := range p.Names {
if w.allowRegex.FindStringIndex(n.Name) != nil {
continue
}
if params[n.Obj] {
w.onFailure(lint.Failure{
Confidence: 1,
Node: n,
Category: "bad practice",
Failure: fmt.Sprintf("parameter '%s' seems to be unused, consider removing or renaming it as _", n.Name),
Failure: fmt.Sprintf(w.failureMsg, n.Name),
})
}
}
Expand Down
66 changes: 61 additions & 5 deletions rule/unused-receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,72 @@ package rule
import (
"fmt"
"go/ast"
"regexp"
"sync"

"github.com/mgechev/revive/lint"
)

// UnusedReceiverRule lints unused params in functions.
type UnusedReceiverRule struct{}
type UnusedReceiverRule struct {
configured bool
// regex to check if some name is valid for unused parameter, "^_$" by default
allowRegex *regexp.Regexp
failureMsg string
sync.Mutex
}

func (r *UnusedReceiverRule) configure(args lint.Arguments) {
r.Lock()
defer r.Unlock()

if r.configured {
return
}
r.configured = true

// while by default args is an array, i think it's good to provide structures inside it by default, not arrays or primitives
// it's more compatible to JSON nature of configurations
var allowedRegexStr string
if len(args) == 0 {
allowedRegexStr = "^_$"
r.failureMsg = "method receiver '%s' is not referenced in method's body, consider removing or renaming it as _"
} else {
// Arguments = [{}]
options := args[0].(map[string]interface{})
// Arguments = [{allowedRegex="^_"}]

if allowedRegexParam, ok := options["allowRegex"]; ok {
allowedRegexStr, ok = allowedRegexParam.(string)
if !ok {
panic(fmt.Errorf("error configuring [unused-receiver] rule: allowedRegex is not string but [%T]", allowedRegexParam))
}
}
}
var err error
r.allowRegex, err = regexp.Compile(allowedRegexStr)
if err != nil {
panic(fmt.Errorf("error configuring [unused-receiver] rule: allowedRegex is not valid regex [%s]: %v", allowedRegexStr, err))
}
if r.failureMsg == "" {
r.failureMsg = "method receiver '%s' is not referenced in method's body, consider removing or renaming it to match " + r.allowRegex.String()
}
}

// Apply applies the rule to given file.
func (*UnusedReceiverRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (r *UnusedReceiverRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
r.configure(args)
var failures []lint.Failure

onFailure := func(failure lint.Failure) {
failures = append(failures, failure)
}

w := lintUnusedReceiverRule{onFailure: onFailure}
w := lintUnusedReceiverRule{
onFailure: onFailure,
allowRegex: r.allowRegex,
failureMsg: r.failureMsg,
}

ast.Walk(w, file.AST)

Expand All @@ -31,7 +81,9 @@ func (*UnusedReceiverRule) Name() string {
}

type lintUnusedReceiverRule struct {
onFailure func(lint.Failure)
onFailure func(lint.Failure)
allowRegex *regexp.Regexp
failureMsg string
}

func (w lintUnusedReceiverRule) Visit(node ast.Node) ast.Visitor {
Expand All @@ -51,6 +103,10 @@ func (w lintUnusedReceiverRule) Visit(node ast.Node) ast.Visitor {
return nil // the receiver is already named _
}

if w.allowRegex != nil && w.allowRegex.FindStringIndex(recID.Name) != nil {
return nil
}

// inspect the func body looking for references to the receiver id
fselect := func(n ast.Node) bool {
ident, isAnID := n.(*ast.Ident)
Expand All @@ -67,7 +123,7 @@ func (w lintUnusedReceiverRule) Visit(node ast.Node) ast.Visitor {
Confidence: 1,
Node: recID,
Category: "bad practice",
Failure: fmt.Sprintf("method receiver '%s' is not referenced in method's body, consider removing or renaming it as _", recID.Name),
Failure: fmt.Sprintf(w.failureMsg, recID.Name),
})

return nil // full method body already inspected
Expand Down
4 changes: 4 additions & 0 deletions test/unused-param_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ package test
import (
"testing"

"github.com/mgechev/revive/lint"
"github.com/mgechev/revive/rule"
)

func TestUnusedParam(t *testing.T) {
testRule(t, "unused-param", &rule.UnusedParamRule{})
testRule(t, "unused-param-custom-regex", &rule.UnusedParamRule{}, &lint.RuleConfig{Arguments: []interface{}{
map[string]interface{}{"allowRegex": "^xxx"},
}})
}

func BenchmarkUnusedParam(b *testing.B) {
Expand Down
4 changes: 4 additions & 0 deletions test/unused-receiver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ package test
import (
"testing"

"github.com/mgechev/revive/lint"
"github.com/mgechev/revive/rule"
)

func TestUnusedReceiver(t *testing.T) {
testRule(t, "unused-receiver", &rule.UnusedReceiverRule{})
testRule(t, "unused-receiver-custom-regex", &rule.UnusedReceiverRule{}, &lint.RuleConfig{Arguments: []interface{}{
map[string]interface{}{"allowRegex": "^xxx"},
}})
}
12 changes: 12 additions & 0 deletions testdata/unused-param-custom-regex.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package fixtures

// all will ok with xxxParam if Arguments = [{allowRegex="^xxx"}]

func f0(xxxParam int) {}

// still works with _

func f1(_ int) {}

func f2(yyyParam int) { // MATCH /parameter 'yyyParam' seems to be unused, consider removing or renaming it to match ^xxx/
}
12 changes: 12 additions & 0 deletions testdata/unused-receiver-custom-regex.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package fixtures

// all will ok with xxxParam if Arguments = [{allowRegex="^xxx"}]

func (xxxParam *SomeObj) f0() {}

// still works with _

func (_ *SomeObj) f1() {}

func (yyyParam *SomeObj) f2() { // MATCH /method receiver 'yyyParam' is not referenced in method's body, consider removing or renaming it to match ^xxx/
}

0 comments on commit b31eb18

Please sign in to comment.