Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extended [allowRegex] parameter for unused-parameter and unused-receiver rules #858

Merged
merged 3 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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/
}
Loading