diff --git a/README.md b/README.md index 6b1532d..b40f58f 100644 --- a/README.md +++ b/README.md @@ -262,6 +262,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | `flag-parameter` | n/a | Warns on boolean parameters that create a control coupling | no | no | | `unnecessary-stmt` | n/a | Suggests removing or simplifying unnecessary statements | no | no | | `struct-tag` | n/a | Checks common struct tags like `json`,`xml`,`yaml` | no | no | +| `modifies-value-receiver` | n/a | Warns on assignments to value-passed method receivers | no | yes | ## Available Formatters diff --git a/config.go b/config.go index d3fe994..e3d044e 100644 --- a/config.go +++ b/config.go @@ -60,6 +60,7 @@ var allRules = append([]lint.Rule{ &rule.FlagParamRule{}, &rule.UnnecessaryStmtRule{}, &rule.StructTagRule{}, + &rule.ModifiesValRecRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/fixtures/modifies-value-receiver.go b/fixtures/modifies-value-receiver.go new file mode 100644 index 0000000..e7355a4 --- /dev/null +++ b/fixtures/modifies-value-receiver.go @@ -0,0 +1,13 @@ +package fixtures + +type data struct { + num int + key *string + items map[string]bool +} + +func (this data) vmethod() { + this.num = 8 // MATCH /suspicious assignment to a by-value method receiver/ + *this.key = "v.key" + this.items["vmethod"] = true +} diff --git a/rule/modifies-value-receiver.go b/rule/modifies-value-receiver.go new file mode 100644 index 0000000..a27334e --- /dev/null +++ b/rule/modifies-value-receiver.go @@ -0,0 +1,134 @@ +package rule + +import ( + "go/ast" + "strings" + + "github.com/mgechev/revive/lint" +) + +// ModifiesValRecRule lints assignments to value method-receivers. +type ModifiesValRecRule struct{} + +// Apply applies the rule to given file. +func (r *ModifiesValRecRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := lintModifiesValRecRule{file: file, onFailure: onFailure} + file.Pkg.TypeCheck() + ast.Walk(w, file.AST) + + return failures +} + +// Name returns the rule name. +func (r *ModifiesValRecRule) Name() string { + return "modifies-value-receiver" +} + +type lintModifiesValRecRule struct { + file *lint.File + onFailure func(lint.Failure) +} + +func (w lintModifiesValRecRule) Visit(node ast.Node) ast.Visitor { + switch n := node.(type) { + case *ast.FuncDecl: + if n.Recv == nil { + return nil // skip, not a method + } + + receiver := n.Recv.List[0] + if _, ok := receiver.Type.(*ast.StarExpr); ok { + return nil // skip, method with pointer receiver + } + + if w.skipType(receiver.Type) { + return nil // skip, receiver is a map or array + } + + if len(receiver.Names) < 1 { + return nil // skip, anonymous receiver + } + + receiverName := receiver.Names[0].Name + if receiverName == "_" { + return nil // skip, anonymous receiver + } + + fselect := func(n ast.Node) bool { + // look for assignments with the receiver in the right hand + asgmt, ok := n.(*ast.AssignStmt) + if !ok { + return false + } + + for _, exp := range asgmt.Lhs { + switch e := exp.(type) { + case *ast.IndexExpr: // receiver...[] = ... + continue + case *ast.StarExpr: // *receiver = ... + continue + case *ast.SelectorExpr: // receiver.field = ... + name := w.getNameFromExpr(e.X) + if name == "" || name != receiverName { + continue + } + + if w.skipType(ast.Expr(e.Sel)) { + continue + } + + case *ast.Ident: // receiver := ... + if e.Name != receiverName { + continue + } + default: + continue + } + + return true + } + + return false + } + + assignmentsToReceiver := pick(n.Body, fselect, nil) + + for _, assignment := range assignmentsToReceiver { + w.onFailure(lint.Failure{ + Node: assignment, + Confidence: 1, + Failure: "suspicious assignment to a by-value method receiver", + }) + } + } + + return w +} + +func (w lintModifiesValRecRule) skipType(t ast.Expr) bool { + rt := w.file.Pkg.TypeOf(t) + if rt == nil { + return false + } + + rt = rt.Underlying() + rtName := rt.String() + + // skip when receiver is a map or array + return strings.HasPrefix(rtName, "[]") || strings.HasPrefix(rtName, "map[") +} + +func (_ lintModifiesValRecRule) getNameFromExpr(ie ast.Expr) string { + ident, ok := ie.(*ast.Ident) + if !ok { + return "" + } + + return ident.Name +} diff --git a/test/modifies-value-receiver_test.go b/test/modifies-value-receiver_test.go new file mode 100644 index 0000000..b16f535 --- /dev/null +++ b/test/modifies-value-receiver_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestModifiesValRec(t *testing.T) { + testRule(t, "modifies-value-receiver", &rule.ModifiesValRecRule{}) +}