diff --git a/rule/modifies_value_receiver.go b/rule/modifies_value_receiver.go index e9e64b9a6..2f92991f5 100644 --- a/rule/modifies_value_receiver.go +++ b/rule/modifies_value_receiver.go @@ -2,6 +2,7 @@ package rule import ( "go/ast" + "go/token" "strings" "github.com/mgechev/revive/lint" @@ -60,14 +61,14 @@ func (w lintModifiesValRecRule) Visit(node ast.Node) ast.Visitor { return nil // skip, anonymous receiver } - fselect := func(n ast.Node) bool { + receiverAssignmentFinder := func(n ast.Node) bool { // look for assignments with the receiver in the right hand - asgmt, ok := n.(*ast.AssignStmt) + assignment, ok := n.(*ast.AssignStmt) if !ok { return false } - for _, exp := range asgmt.Lhs { + for _, exp := range assignment.Lhs { switch e := exp.(type) { case *ast.IndexExpr: // receiver...[] = ... continue @@ -92,7 +93,15 @@ func (w lintModifiesValRecRule) Visit(node ast.Node) ast.Visitor { return false } - assignmentsToReceiver := pick(n.Body, fselect) + assignmentsToReceiver := pick(n.Body, receiverAssignmentFinder) + if len(assignmentsToReceiver) == 0 { + return nil // receiver is not modified + } + + methodReturnsReceiver := len(w.findReturnReceiverStatements(receiverName, n.Body)) > 0 + if methodReturnsReceiver { + return nil // modification seems legit (see issue #1066) + } for _, assignment := range assignmentsToReceiver { w.onFailure(lint.Failure{ @@ -127,3 +136,44 @@ func (lintModifiesValRecRule) getNameFromExpr(ie ast.Expr) string { return ident.Name } + +func (w lintModifiesValRecRule) findReturnReceiverStatements(receiverName string, target ast.Node) []ast.Node { + finder := func(n ast.Node) bool { + // look for returns with the receiver as value + returnStatement, ok := n.(*ast.ReturnStmt) + if !ok { + return false + } + + for _, exp := range returnStatement.Results { + switch e := exp.(type) { + case *ast.SelectorExpr: // receiver.field = ... + name := w.getNameFromExpr(e.X) + if name == "" || name != receiverName { + continue + } + case *ast.Ident: // receiver := ... + if e.Name != receiverName { + continue + } + case *ast.UnaryExpr: + if e.Op != token.AND { + continue + } + name := w.getNameFromExpr(e.X) + if name == "" || name != receiverName { + continue + } + + default: + continue + } + + return true + } + + return false + } + + return pick(target, finder) +} diff --git a/testdata/modifies_value_receiver.go b/testdata/modifies_value_receiver.go index b5ae87741..e6a716606 100644 --- a/testdata/modifies_value_receiver.go +++ b/testdata/modifies_value_receiver.go @@ -12,3 +12,19 @@ func (this data) vmethod() { this.items = make(map[string]bool) // MATCH /suspicious assignment to a by-value method receiver/ this.items["vmethod"] = true } + +func (a A) Foo() *A { + a.whatever = true + return &a +} + +func (a A) Clone() (*A, error) { + a.whatever = true + return &a, nil +} + +// WithBin will set the specific bin path to the builder. +func (b JailerCommandBuilder) WithBin(bin string) JailerCommandBuilder { + b.bin = bin + return b +}