gopls/internal/analysis/modernize: disable minmax on floating point

The built-in min and max functions return NaN if any operand is
NaN, so the minmax transformation is not sound for certain inputs.

Since it is usually infeasible to prove that the operands are not NaN,
this CL disables minmax for floating-point operands.

Behavior-preserving translation: celebrating 75 years of being
harder than it looks.

Fixes golang/go#72829

Change-Id: Idb3454fea7ec37842e622154f66d5898703a392f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/657955
Auto-Submit: Alan Donovan <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/analysis/modernize/minmax.go

@@ -16,6 +16,7 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/astutil/cursor"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // The minmax pass replaces if/else statements with calls to min or max.
@@ -25,6 +26,10 @@ import (
 //  1. if a < b { x = a } else { x = b }        =>      x = min(a, b)
 //  2. x = a; if a < b { x = b }                =>      x = max(a, b)
 //
+// Pattern 1 requires that a is not NaN, and pattern 2 requires that b
+// is not Nan. Since this is hard to prove, we reject floating-point
+// numbers.
+//
 // Variants:
 // - all four ordered comparisons
 // - "x := a" or "x = a" or "var x = a" in pattern 2
@@ -172,15 +177,17 @@ func minmax(pass *analysis.Pass) {
 	}
 
 	// Find all "if a < b { lhs = rhs }" statements.
+	info := pass.TypesInfo
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	for curFile := range filesUsing(inspect, pass.TypesInfo, "go1.21") {
+	for curFile := range filesUsing(inspect, info, "go1.21") {
 		astFile := curFile.Node().(*ast.File)
 		for curIfStmt := range curFile.Preorder((*ast.IfStmt)(nil)) {
 			ifStmt := curIfStmt.Node().(*ast.IfStmt)
 			if compare, ok := ifStmt.Cond.(*ast.BinaryExpr); ok &&
 				ifStmt.Init == nil &&
 				isInequality(compare.Op) != 0 &&
-				isAssignBlock(ifStmt.Body) {
+				isAssignBlock(ifStmt.Body) &&
+				!maybeNaN(info.TypeOf(ifStmt.Body.List[0].(*ast.AssignStmt).Lhs[0])) { // lhs
 
 				// Have: if a < b { lhs = rhs }
 				check(astFile, curIfStmt, compare)
@@ -219,6 +226,21 @@ func isSimpleAssign(n ast.Node) bool {
 		len(assign.Rhs) == 1
 }
 
+// maybeNaN reports whether t is (or may be) a floating-point type.
+func maybeNaN(t types.Type) bool {
+	// For now, we rely on core types.
+	// TODO(adonovan): In the post-core-types future,
+	// follow the approach of types.Checker.applyTypeFunc.
+	t = typeparams.CoreType(t)
+	if t == nil {
+		return true // fail safe
+	}
+	if basic, ok := t.(*types.Basic); ok && basic.Info()&types.IsFloat != 0 {
+		return true
+	}
+	return false
+}
+
 // -- utils --
 
 func is[T any](x any) bool {

gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go

@@ -136,3 +136,16 @@ func fix72727(a, b int) {
 		o = b
 	}
 }
+
+type myfloat float64
+
+// The built-in min/max differ in their treatement of NaN,
+// so reject floating-point numbers (#72829).
+func nopeFloat(a, b myfloat) (res myfloat) {
+	if a < b {
+		res = a
+	} else {
+		res = b
+	}
+	return
+}

gopls/internal/analysis/modernize/testdata/src/minmax/minmax.go.golden

@@ -123,3 +123,16 @@ func fix72727(a, b int) {
 	// want "if statement can be modernized using max"
 	o := max(a-42, b)
 }
+
+type myfloat float64
+
+// The built-in min/max differ in their treatement of NaN,
+// so reject floating-point numbers (#72829).
+func nopeFloat(a, b myfloat) (res myfloat) {
+	if a < b {
+		res = a
+	} else {
+		res = b
+	}
+	return
+}