From cc3ad5f7029305a998c457669d6aa488312eb311 Mon Sep 17 00:00:00 2001
From: chavacava <salvadorcavadini+github@gmail.com>
Date: Fri, 15 Nov 2024 09:41:29 +0100
Subject: [PATCH] fix #1066 by ignoring what seems legit modification of value
 receivers

---
 rule/modifies_value_receiver.go     | 58 +++++++++++++++++++++++++++--
 testdata/modifies_value_receiver.go | 16 ++++++++
 2 files changed, 70 insertions(+), 4 deletions(-)

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
+}