From 00a86ae1fd79125c407534f8c96f9f4b4d6fb39f Mon Sep 17 00:00:00 2001 From: SalvadorC Date: Thu, 18 Apr 2019 19:35:51 +0200 Subject: [PATCH] new rule: unused-receiver (#119) * new rule: unused-receiver * unused-receiver: adds more test cases --- README.md | 1 + RULES_DESCRIPTIONS.md | 7 ++++ config.go | 1 + fixtures/unused-receiver.go | 43 ++++++++++++++++++++ rule/unused-receiver.go | 77 ++++++++++++++++++++++++++++++++++++ test/unused-receiver_test.go | 11 ++++++ 6 files changed, 140 insertions(+) create mode 100644 fixtures/unused-receiver.go create mode 100644 rule/unused-receiver.go create mode 100644 test/unused-receiver_test.go diff --git a/README.md b/README.md index a11f54a..dfacc0e 100644 --- a/README.md +++ b/README.md @@ -296,6 +296,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`duplicated-imports`](./RULES_DESCRIPTIONS#duplicated-imports) | n/a | Looks for packages that are imported two or more times | no | no | | [`import-shadowing`](./RULES_DESCRIPTIONS.md#import-shadowing) | n/a | Spots identifiers that shadow an import | no | no | | [`bare-return`](./RULES_DESCRIPTIONS#bare-return) | n/a | Warns on bare returns | no | no | +| [`unused-receiver`](./RULES_DESCRIPTIONS.md#unused-receiver) | n/a | Suggests to rename or remove unused method receivers | no | no | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 5e05556..e4d6225 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -54,6 +54,7 @@ List of all available rules. - [unnecessary-stmt](#unnecessary-stmt) - [unreachable-code](#unreachable-code) - [unused-parameter](#unused-parameter) + - [unused-receiver](#unused-receiver) - [waitgroup-by-value](#waitgroup-by-value) ## add-constant @@ -451,6 +452,12 @@ _Description_: This rule warns on unused parameters. Functions or methods with u _Configuration_: N/A +## 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 + ## waitgroup-by-value _Description_: Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a `sync.WaitGroup` is usually not what the developer wants to do. diff --git a/config.go b/config.go index a342189..f874326 100644 --- a/config.go +++ b/config.go @@ -76,6 +76,7 @@ var allRules = append([]lint.Rule{ &rule.DuplicatedImportsRule{}, &rule.ImportShadowingRule{}, &rule.BareReturnRule{}, + &rule.UnusedReceiverRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/unused-receiver.go b/fixtures/unused-receiver.go new file mode 100644 index 0000000..2d61db6 --- /dev/null +++ b/fixtures/unused-receiver.go @@ -0,0 +1,43 @@ +package fixtures + +import ( + "fmt" + + "github.com/mgechev/revive/lint" +) + +func (f *Unix) Name() string { // MATCH /method receiver 'f' is not referenced in method's body, consider removing or renaming it as _/ + return "unix" +} + +func (f *Unix) Format(failures <-chan lint.Failure, _ lint.RulesConfig) (string, error) { // MATCH /method receiver 'f' is not referenced in method's body, consider removing or renaming it as _/ + for failure := range failures { + fmt.Printf("%v: [%s] %s\n", failure.Position.Start, failure.RuleName, failure.Failure) + } + return "", nil +} + +func (u *Unix) Foo() (string, error) { // MATCH /method receiver 'u' is not referenced in method's body, consider removing or renaming it as _/ + for failure := range failures { + u := 1 // shadowing the receiver + fmt.Printf("%v\n", u) + } + return "", nil +} + +func (u *Unix) Foos() (string, error) { + for failure := range failures { + u := 1 // shadowing the receiver + fmt.Printf("%v\n", u) + } + + return u, nil // use of the receiver +} + +func (u *Unix) Bar() (string, error) { + for failure := range failures { + u.path = nil // modifies the receiver + fmt.Printf("%v\n", u) + } + return "", nil +} diff --git a/rule/unused-receiver.go b/rule/unused-receiver.go new file mode 100644 index 0000000..43eaf83 --- /dev/null +++ b/rule/unused-receiver.go @@ -0,0 +1,77 @@ +package rule + +import ( + "fmt" + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// UnusedReceiverRule lints unused params in functions. +type UnusedReceiverRule struct{} + +// Apply applies the rule to given file. +func (_ *UnusedReceiverRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintUnusedReceiverRule{onFailure: onFailure} + + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (_ *UnusedReceiverRule) Name() string { + return "unused-receiver" +} + +type lintUnusedReceiverRule struct { + onFailure func(lint.Failure) +} + +func (w lintUnusedReceiverRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + if n.Recv == nil { + return nil // skip this func decl, not a method + } + + rec := n.Recv.List[0] // safe to access only the first (unique) element of the list + if len(rec.Names) < 1 { + return nil // the receiver is anonymous: func (aType) Foo(...) ... + } + + recID := rec.Names[0] + if recID.Name == "_" { + return nil // the receiver is already named _ + } + + // inspect the func body looking for references to the receiver id + fselect := func(n ast.Node) bool { + ident, isAnID := n.(*ast.Ident) + + return isAnID && ident.Obj == recID.Obj + } + refs2recID := pick(n.Body, fselect, nil) + + if len(refs2recID) > 0 { + return nil // the receiver is referenced in the func body + } + + w.onFailure(lint.Failure{ + 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), + }) + + return nil // full method body already inspected + } + + return w +} diff --git a/test/unused-receiver_test.go b/test/unused-receiver_test.go new file mode 100644 index 0000000..e4bae7c --- /dev/null +++ b/test/unused-receiver_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestUnusedReceiver(t *testing.T) { + testRule(t, "unused-receiver", &rule.UnusedReceiverRule{}) +}