modifies-parameter: handle slices.Delete and slices.DeleteFunc (#1427)

RULES_DESCRIPTIONS.md

@@ -873,7 +873,8 @@ arguments = [3]
 
 _Description_: A function that modifies its parameters can be hard to understand.
 It can also be misleading if the arguments are passed by value by the caller.
-This rule warns when a function modifies one or more of its parameters.
+This rule warns when a function modifies one or more of its parameters or when
+parameters are passed to functions that modify them (e.g. `slices.Delete`).
 
 _Configuration_: N/A
 

rule/modifies_param.go

@@ -4,12 +4,22 @@ import (
 	"fmt"
 	"go/ast"
 
+	"github.com/mgechev/revive/internal/astutils"
 	"github.com/mgechev/revive/lint"
 )
 
 // ModifiesParamRule warns on assignments to function parameters.
 type ModifiesParamRule struct{}
 
+// modifyingParamPositions are parameter positions that are modified by a function.
+type modifyingParamPositions = []int
+
+// modifyingFunctions maps function names to the positions of parameters they modify.
+var modifyingFunctions = map[string]modifyingParamPositions{
+	"slices.Delete":     {0},
+	"slices.DeleteFunc": {0},
+}
+
 // Apply applies the rule to given file.
 func (*ModifiesParamRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
@@ -57,12 +67,19 @@ func (w lintModifiesParamRule) Visit(node ast.Node) ast.Visitor {
 		}
 	case *ast.AssignStmt:
 		lhs := v.Lhs
-		for _, e := range lhs {
+		for i, e := range lhs {
 			id, ok := e.(*ast.Ident)
-			if ok {
-				checkParam(id, &w)
+			if !ok {
+				continue
 			}
+
+			if i < len(v.Rhs) {
+				w.checkModifyingFunction(v.Rhs[i])
+			}
+			checkParam(id, &w)
 		}
+	case *ast.ExprStmt:
+		w.checkModifyingFunction(v.X)
 	}
 
 	return w
@@ -78,3 +95,39 @@ func checkParam(id *ast.Ident, w *lintModifiesParamRule) {
 		})
 	}
 }
+
+func (w *lintModifiesParamRule) checkModifyingFunction(callNode ast.Node) {
+	callExpr, ok := callNode.(*ast.CallExpr)
+	if !ok {
+		return
+	}
+
+	funcName := astutils.GoFmt(callExpr.Fun)
+	positions, found := modifyingFunctions[funcName]
+	if !found {
+		return
+	}
+
+	for _, pos := range positions {
+		if pos >= len(callExpr.Args) {
+			return
+		}
+
+		id, ok := callExpr.Args[pos].(*ast.Ident)
+		if !ok {
+			continue
+		}
+
+		_, match := w.params[id.Name]
+		if !match {
+			continue
+		}
+
+		w.onFailure(lint.Failure{
+			Confidence: 0.5, // confidence is low because of shadow variables
+			Node:       callExpr,
+			Category:   lint.FailureCategoryBadPractice,
+			Failure:    fmt.Sprintf("parameter '%s' seems to be modified by '%s'", id.Name, funcName),
+		})
+	}
+}

testdata/modifies_param.go

@@ -1,5 +1,7 @@
 package fixtures
 
+import "slices"
+
 func one(a int) {
 	a, b := 1, 2 // MATCH /parameter 'a' seems to be modified/
 	a++          // MATCH /parameter 'a' seems to be modified/
@@ -23,3 +25,54 @@ func three(s *foo) {
 func issue355(_ *foo) {
 	_ = "foooooo"
 }
+
+func testSlicesDeleteAssigned(s []int) {
+	s = slices.Delete(s, 0, 1)
+	// MATCH:30 /parameter 's' seems to be modified/
+	// MATCH:30 /parameter 's' seems to be modified by 'slices.Delete'/
+	s = slices.DeleteFunc(s, func(e string) bool { return true })
+	// MATCH:33 /parameter 's' seems to be modified/
+	// MATCH:33 /parameter 's' seems to be modified by 'slices.DeleteFunc'/
+	_ = slices.Delete(s, 0, 1)                     // MATCH /parameter 's' seems to be modified by 'slices.Delete'/
+	_ = slices.DeleteFunc(s, func(e string) bool { // MATCH /parameter 's' seems to be modified by 'slices.DeleteFunc'/
+		return true
+	})
+	s, b := slices.Delete(s, 0, 1), 2
+	// MATCH:40 /parameter 's' seems to be modified/
+	// MATCH:40 /parameter 's' seems to be modified by 'slices.Delete'/
+	s := slices.Clone(s)
+	// MATCH:43 /parameter 's' seems to be modified/
+}
+
+func testSlicesDeleteCloned(s []int) {
+	s2 := slices.Clone(s)
+	s2 = slices.Delete(s2, 0, 1)
+	_ = slices.Delete(slices.Clone(s), 0, 1)
+	_ = slices.DeleteFunc(slices.Clone(s), func(e string) bool {
+		return e == "test"
+	})
+}
+
+func testSlicesDeleteCopied(s []int) {
+	c := make([]int, len(s))
+	copy(c, s)
+	c = slices.Delete(c, 0, 1)
+}
+
+func testMultipleParams(a, b, s []int) {
+	a = slices.Delete(a, 0, 1)
+	// MATCH:63 /parameter 'a' seems to be modified/
+	// MATCH:63 /parameter 'a' seems to be modified by 'slices.Delete'/
+	b = slices.Delete(b, 1, 2)
+	// MATCH:66 /parameter 'b' seems to be modified/
+	// MATCH:66 /parameter 'b' seems to be modified by 'slices.Delete'/
+	s = []int{1, 2, 3} // MATCH /parameter 's' seems to be modified/
+	s = slices.Delete(s, 0, 1)
+	// MATCH:70 /parameter 's' seems to be modified/
+	// MATCH:70 /parameter 's' seems to be modified by 'slices.Delete'/
+}
+
+func testAssignToNewVar(s []int) {
+	newSlice := s
+	newSlice = slices.Delete(newSlice, 0, 1)
+}