Repository: mgechev/revive
Pull request #1157 (2024-12-03 16:56:13)

unused-param,unused-receiver: refactor configure func
The PR refactors the `configure` function in both `unused-param` and `unused-receiver`:

- change to use `blankIdentifierRegex` that is compiled once;
- reduce nested `if`s;
- add tests and benchmarks.

## Benchmarks

Benchmarks show that version in this PR has slightly fewer allocations.

```sh
$ git switch master
$ go test -benchmem -run=^$ -bench "^BenchmarkUnused.*" github.com/mgechev/revive/test -count=10 > old.txt
$ git switch refactor/unused-configure
$ go test -benchmem -run=^$ -bench "^BenchmarkUnused.*" github.com/mgechev/revive/test -count=10 > new.txt
$ benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: github.com/mgechev/revive/test
cpu: Apple M1 Pro
                 │   old.txt   │              new.txt               │
                 │   sec/op    │   sec/op     vs base               │
UnusedParam-8      671.0µ ± 1%   673.0µ ± 3%       ~ (p=0.436 n=10)
UnusedReceiver-8   211.9µ ± 8%   207.7µ ± 4%       ~ (p=0.063 n=10)
geomean            377.0µ        373.9µ       -0.83%

                 │   old.txt    │               new.txt               │
                 │     B/op     │     B/op      vs base               │
UnusedParam-8      325.5Ki ± 0%   323.7Ki ± 0%  -0.58% (p=0.000 n=10)
UnusedReceiver-8   80.22Ki ± 0%   78.09Ki ± 0%  -2.65% (p=0.000 n=10)
geomean            161.6Ki        159.0Ki       -1.62%

                 │   old.txt   │              new.txt               │
                 │  allocs/op  │  allocs/op   vs base               │
UnusedParam-8      8.322k ± 0%   8.290k ± 0%  -0.38% (p=0.000 n=10)
UnusedReceiver-8   1.896k ± 0%   1.864k ± 0%  -1.69% (p=0.000 n=10)
geomean            3.972k        3.931k       -1.04%
```



Patch:
diff --git a/rule/unused_param.go b/rule/unused_param.go
index 08945da..958cd89 100644
--- a/rule/unused_param.go
+++ b/rule/unused_param.go
@@ -9,6 +9,8 @@ import (
 	"github.com/mgechev/revive/lint"
 )
 
+var allowBlankIdentifierRegex = regexp.MustCompile("^_$")
+
 // UnusedParamRule lints unused params in functions.
 type UnusedParamRule struct {
 	// regex to check if some name is valid for unused parameter, "^_$" by default
@@ -21,30 +23,29 @@ type UnusedParamRule struct {
 func (r *UnusedParamRule) configure(args lint.Arguments) {
 	// 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 allowRegexStr string
+	r.allowRegex = allowBlankIdentifierRegex
+	r.failureMsg = "parameter '%s' seems to be unused, consider removing or renaming it as _"
 	if len(args) == 0 {
-		allowRegexStr = "^_$"
-		r.failureMsg = "parameter '%s' seems to be unused, consider removing or renaming it as _"
-	} else {
-		// Arguments = [{}]
-		options := args[0].(map[string]any)
-		// Arguments = [{allowRegex="^_"}]
-
-		if allowRegexParam, ok := options["allowRegex"]; ok {
-			allowRegexStr, ok = allowRegexParam.(string)
-			if !ok {
-				panic(fmt.Errorf("error configuring %s rule: allowRegex is not string but [%T]", r.Name(), allowRegexParam))
-			}
-		}
+		return
+	}
+	// Arguments = [{}]
+	options := args[0].(map[string]any)
+
+	allowRegexParam, ok := options["allowRegex"]
+	if !ok {
+		return
+	}
+	// Arguments = [{allowRegex="^_"}]
+	allowRegexStr, ok := allowRegexParam.(string)
+	if !ok {
+		panic(fmt.Errorf("error configuring %s rule: allowRegex is not string but [%T]", r.Name(), allowRegexParam))
 	}
 	var err error
 	r.allowRegex, err = regexp.Compile(allowRegexStr)
 	if err != nil {
 		panic(fmt.Errorf("error configuring %s rule: allowRegex is not valid regex [%s]: %v", r.Name(), allowRegexStr, err))
 	}
-	if r.failureMsg == "" {
-		r.failureMsg = "parameter '%s' seems to be unused, consider removing or renaming it to match " + r.allowRegex.String()
-	}
+	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.
diff --git a/rule/unused_receiver.go b/rule/unused_receiver.go
index c88c0e3..b3be2a7 100644
--- a/rule/unused_receiver.go
+++ b/rule/unused_receiver.go
@@ -21,30 +21,29 @@ type UnusedReceiverRule struct {
 func (r *UnusedReceiverRule) configure(args lint.Arguments) {
 	// 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 allowRegexStr string
+	r.allowRegex = allowBlankIdentifierRegex
+	r.failureMsg = "method receiver '%s' is not referenced in method's body, consider removing or renaming it as _"
 	if len(args) == 0 {
-		allowRegexStr = "^_$"
-		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]any)
-		// Arguments = [{allowRegex="^_"}]
-
-		if allowRegexParam, ok := options["allowRegex"]; ok {
-			allowRegexStr, ok = allowRegexParam.(string)
-			if !ok {
-				panic(fmt.Errorf("error configuring [unused-receiver] rule: allowRegex is not string but [%T]", allowRegexParam))
-			}
-		}
+		return
+	}
+	// Arguments = [{}]
+	options := args[0].(map[string]any)
+
+	allowRegexParam, ok := options["allowRegex"]
+	if !ok {
+		return
+	}
+	// Arguments = [{allowRegex="^_"}]
+	allowRegexStr, ok := allowRegexParam.(string)
+	if !ok {
+		panic(fmt.Errorf("error configuring [unused-receiver] rule: allowRegex is not string but [%T]", allowRegexParam))
 	}
 	var err error
 	r.allowRegex, err = regexp.Compile(allowRegexStr)
 	if err != nil {
 		panic(fmt.Errorf("error configuring [unused-receiver] rule: allowRegex is not valid regex [%s]: %v", allowRegexStr, 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()
-	}
+	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.


Test patch:
diff --git a/test/unused_param_test.go b/test/unused_param_test.go
index 6a98c9b..c67316e 100644
--- a/test/unused_param_test.go
+++ b/test/unused_param_test.go
@@ -9,6 +9,10 @@ import (
 
 func TestUnusedParam(t *testing.T) {
 	testRule(t, "unused_param", &rule.UnusedParamRule{})
+	testRule(t, "unused_param", &rule.UnusedParamRule{}, &lint.RuleConfig{Arguments: []any{}})
+	testRule(t, "unused_param", &rule.UnusedParamRule{}, &lint.RuleConfig{Arguments: []any{
+		map[string]any{"a": "^xxx"},
+	}})
 	testRule(t, "unused_param_custom_regex", &rule.UnusedParamRule{}, &lint.RuleConfig{Arguments: []any{
 		map[string]any{"allowRegex": "^xxx"},
 	}})
diff --git a/test/unused_receiver_test.go b/test/unused_receiver_test.go
index 1f7ce32..3f1bc61 100644
--- a/test/unused_receiver_test.go
+++ b/test/unused_receiver_test.go
@@ -9,7 +9,18 @@ import (
 
 func TestUnusedReceiver(t *testing.T) {
 	testRule(t, "unused_receiver", &rule.UnusedReceiverRule{})
+	testRule(t, "unused_receiver", &rule.UnusedReceiverRule{}, &lint.RuleConfig{Arguments: []any{}})
+	testRule(t, "unused_receiver", &rule.UnusedReceiverRule{}, &lint.RuleConfig{Arguments: []any{
+		map[string]any{"a": "^xxx"},
+	}})
 	testRule(t, "unused_receiver_custom_regex", &rule.UnusedReceiverRule{}, &lint.RuleConfig{Arguments: []any{
 		map[string]any{"allowRegex": "^xxx"},
 	}})
 }
+
+func BenchmarkUnusedReceiver(b *testing.B) {
+	var t *testing.T
+	for i := 0; i <= b.N; i++ {
+		testRule(t, "unused_receiver", &rule.UnusedReceiverRule{})
+	}
+}