go/analysis/passes/modernize: minmax: only remove exact userdefined

Make sure that the min/max logic in a user defined min max functions use the two function parameters.

Also refactored the userdefined tests so that they actually test what they intend to (all of the moved tests would also fail the userdefined test just because of the name of their functions, not the reasons stated in the comments). Because of the rules of min max though, there can only be two tests per directory since the functions have to have the names "min" or "max".

Fixes golang/go#79210

Change-Id: Id06fd15c54862270e86545e5c562588b91985283
Reviewed-on: https://go-review.googlesource.com/c/tools/+/774760
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alex Putman <aputman@google.com>

Author: aputman

go/analysis/passes/modernize/minmax.go

@@ -363,18 +363,18 @@ func canUseBuiltinMinMax(fn *types.Func, body *ast.BlockStmt) bool {
 		return false
 	}
 
-	return hasMinMaxLogic(body, fn.Name())
+	return hasMinMaxLogic(body, fn.Name(), sig.Params().At(0).Name(), sig.Params().At(1).Name())
 }
 
 // hasMinMaxLogic checks if the function body implements simple min/max logic.
-func hasMinMaxLogic(body *ast.BlockStmt, funcName string) bool {
+func hasMinMaxLogic(body *ast.BlockStmt, funcName, param0, param1 string) bool {
 	// Pattern 1: Single if/else statement
 	if len(body.List) == 1 {
 		if ifStmt, ok := body.List[0].(*ast.IfStmt); ok {
 			// Get the "false" result from the else block
 			if elseBlock, ok := ifStmt.Else.(*ast.BlockStmt); ok && len(elseBlock.List) == 1 {
 				if elseRet, ok := elseBlock.List[0].(*ast.ReturnStmt); ok && len(elseRet.Results) == 1 {
-					return checkMinMaxPattern(ifStmt, elseRet.Results[0], funcName)
+					return checkMinMaxPattern(ifStmt, elseRet.Results[0], funcName, param0, param1)
 				}
 			}
 		}
@@ -384,7 +384,7 @@ func hasMinMaxLogic(body *ast.BlockStmt, funcName string) bool {
 	if len(body.List) == 2 {
 		if ifStmt, ok := body.List[0].(*ast.IfStmt); ok && ifStmt.Else == nil {
 			if retStmt, ok := body.List[1].(*ast.ReturnStmt); ok && len(retStmt.Results) == 1 {
-				return checkMinMaxPattern(ifStmt, retStmt.Results[0], funcName)
+				return checkMinMaxPattern(ifStmt, retStmt.Results[0], funcName, param0, param1)
 			}
 		}
 	}
@@ -396,7 +396,8 @@ func hasMinMaxLogic(body *ast.BlockStmt, funcName string) bool {
 // ifStmt: the if statement to check
 // falseResult: the expression returned when the condition is false
 // funcName: "min" or "max"
-func checkMinMaxPattern(ifStmt *ast.IfStmt, falseResult ast.Expr, funcName string) bool {
+// param0, param1: the two parameter names for the function.
+func checkMinMaxPattern(ifStmt *ast.IfStmt, falseResult ast.Expr, funcName, param0, param1 string) bool {
 	// Must have condition with comparison
 	cmp, ok := ifStmt.Cond.(*ast.BinaryExpr)
 	if !ok {
@@ -419,10 +420,24 @@ func checkMinMaxPattern(ifStmt *ast.IfStmt, falseResult ast.Expr, funcName strin
 		return false // Not a comparison operator
 	}
 
-	t := thenRet.Results[0] // "true" result
-	f := falseResult        // "false" result
-	x := cmp.X              // left operand
-	y := cmp.Y              // right operand
+	t := thenRet.Results[0]     // "true" result
+	f := falseResult            // "false" result
+	x, ok := cmp.X.(*ast.Ident) // left operand
+	if !ok {
+		return false // Not a basic min/max comparison
+	}
+	y, ok := cmp.Y.(*ast.Ident) // right operand
+	if !ok {
+		return false // Not a basic min/max comparison
+	}
+
+	// Check that the min max algorithm uses the function's params
+	// Which param corresponds to which part of the operation doesn't matter,
+	// so we have to try both.
+	if !(param0 == x.Name && param1 == y.Name ||
+		param0 == y.Name && param1 == x.Name) {
+		return false
+	}
 
 	// Check operand order and adjust sign accordingly
 	if astutil.EqualSyntax(t, x) && astutil.EqualSyntax(f, y) {

go/analysis/passes/modernize/testdata/src/minmax/userdefined/differentlogic/differentlogic.go

@@ -0,0 +1,16 @@
+package differentlogic
+
+// User-defined min with different logic - should NOT be removed
+func min(a, b int) int {
+	return a + b // Completely different logic
+}
+
+// Function with complex body that doesn't match pattern - should NOT be removed
+func max(a, b int) int {
+	println("choosing min")
+	if a < b {
+		return a
+	} else {
+		return b
+	}
+}

go/analysis/passes/modernize/testdata/src/minmax/userdefined/nanhandling/nanhandling.go

@@ -0,0 +1,19 @@
+package nanhandling
+
+// User-defined min with float parameters - should NOT be removed due to NaN handling
+func min(a, b float64) float64 {
+	if a < b {
+		return a
+	} else {
+		return b
+	}
+}
+
+// User-defined max with float parameters - should NOT be removed due to NaN handling
+func max(a, b float64) float64 {
+	if a > b {
+		return a
+	} else {
+		return b
+	}
+}

go/analysis/passes/modernize/testdata/src/minmax/userdefined/notuserparams/notuserparams.go

@@ -0,0 +1,19 @@
+package notuserparams
+
+// User-defined max with correct logic but wrong vars - should NOT be removed
+func max(a, b int) int {
+	if a < 5 {
+		return a
+	} else {
+		return 5
+	}
+}
+
+// User-defined min with correct logic but wrong vars - should NOT be removed
+func min(a, b int) int {
+	if 5 < b {
+		return 5
+	} else {
+		return b
+	}
+}

go/analysis/passes/modernize/testdata/src/minmax/userdefined/userdefined.go

@@ -1,23 +1,5 @@
 package userdefined
 
-// User-defined min with float parameters - should NOT be removed due to NaN handling
-func minFloat(a, b float64) float64 {
-	if a < b {
-		return a
-	} else {
-		return b
-	}
-}
-
-// User-defined max with float parameters - should NOT be removed due to NaN handling
-func maxFloat(a, b float64) float64 {
-	if a > b {
-		return a
-	} else {
-		return b
-	}
-}
-
 // User-defined function with different name - should NOT be removed
 func minimum(a, b int) int {
 	if a < b {
@@ -27,11 +9,6 @@ func minimum(a, b int) int {
 	}
 }
 
-// User-defined min with different logic - should NOT be removed
-func minDifferent(a, b int) int {
-	return a + b // Completely different logic
-}
-
 // Method on a type - should NOT be removed
 type MyType struct{}
 
@@ -43,21 +20,6 @@ func (m MyType) min(a, b int) int {
 	}
 }
 
-// Function with wrong signature - should NOT be removed
-func minWrongSig(a int) int {
-	return a
-}
-
-// Function with complex body that doesn't match pattern - should NOT be removed
-func minComplex(a, b int) int {
-	println("choosing min")
-	if a < b {
-		return a
-	} else {
-		return b
-	}
-}
-
 // min returns the smaller of two values.
 func min(a, b int) int { // want "user-defined min function is equivalent to built-in min and can be removed"
 	if a < b {
@@ -74,5 +36,3 @@ func max(a, b int) int { // want "user-defined max function is equivalent to bui
 	}
 	return b
 }
-
-

go/analysis/passes/modernize/testdata/src/minmax/userdefined/userdefined.go.golden

@@ -1,23 +1,5 @@
 package userdefined
 
-// User-defined min with float parameters - should NOT be removed due to NaN handling
-func minFloat(a, b float64) float64 {
-	if a < b {
-		return a
-	} else {
-		return b
-	}
-}
-
-// User-defined max with float parameters - should NOT be removed due to NaN handling
-func maxFloat(a, b float64) float64 {
-	if a > b {
-		return a
-	} else {
-		return b
-	}
-}
-
 // User-defined function with different name - should NOT be removed
 func minimum(a, b int) int {
 	if a < b {
@@ -27,11 +9,6 @@ func minimum(a, b int) int {
 	}
 }
 
-// User-defined min with different logic - should NOT be removed
-func minDifferent(a, b int) int {
-	return a + b // Completely different logic
-}
-
 // Method on a type - should NOT be removed
 type MyType struct{}
 
@@ -43,19 +20,4 @@ func (m MyType) min(a, b int) int {
 	}
 }
 
-// Function with wrong signature - should NOT be removed
-func minWrongSig(a int) int {
-	return a
-}
-
-// Function with complex body that doesn't match pattern - should NOT be removed
-func minComplex(a, b int) int {
-	println("choosing min")
-	if a < b {
-		return a
-	} else {
-		return b
-	}
-}
-
 

go/analysis/passes/modernize/testdata/src/minmax/userdefined/wrongsignature/wrongsignature.go

@@ -0,0 +1,6 @@
+package wrongsignature
+
+// Function with wrong signature - should NOT be removed
+func min(a int) int {
+	return a
+}