[tools] go/analysis/passes/modernize: directly remove user-defined min/max functions
shuang cui (Gerrit)
shuang cui has uploaded the change for review![]( "Open in Gerrit")
Commit message
go/analysis/passes/modernize: directly remove user-defined min/max functions
If the parameters, return values, and logic of the user-defined min/max are
identical to those of the standard library min/max, then remove the
user-defined functions.
Change diff
diff --git a/go/analysis/passes/modernize/minmax.go b/go/analysis/passes/modernize/minmax.go
index bd815c6..bb0486e 100644
--- a/go/analysis/passes/modernize/minmax.go
+++ b/go/analysis/passes/modernize/minmax.go
@@ -31,9 +31,10 @@
URL: "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/modernize#minmax",
}
-// The minmax pass replaces if/else statements with calls to min or max.
+// The minmax pass replaces if/else statements with calls to min or max,
+// and removes user-defined min/max functions that are equivalent to built-ins.
//
-// Patterns:
+// If/else replacement patterns:
//
// 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)
@@ -42,6 +43,11 @@
// is not Nan. Since this is hard to prove, we reject floating-point
// numbers.
//
+// Function removal:
+// User-defined min/max functions are suggested for removal if they have
+// the same signature and logic as built-in min/max, except when they
+// accept floating-point parameters (due to different NaN handling).
+//
// Variants:
// - all four ordered comparisons
// - "x := a" or "x = a" or "var x = a" in pattern 2
@@ -49,6 +55,9 @@
func minmax(pass *analysis.Pass) (any, error) {
skipGenerated(pass)
+ // Check for user-defined min/max functions that can be removed
+ checkUserDefinedMinMax(pass)
+
// check is called for all statements of this form:
// if a < b { lhs = rhs }
check := func(file *ast.File, curIfStmt inspector.Cursor, compare *ast.BinaryExpr) {
@@ -275,6 +284,279 @@
return false
}
+// checkUserDefinedMinMax looks for user-defined min/max functions that are
+// equivalent to the built-in functions and suggests removing them.
+func checkUserDefinedMinMax(pass *analysis.Pass) {
+ inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+ inspect.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {
+ funcDecl := n.(*ast.FuncDecl)
+
+ // Skip methods and functions without names
+ if funcDecl.Recv != nil || funcDecl.Name == nil {
+ return
+ }
+
+ funcName := funcDecl.Name.Name
+ if funcName != "min" && funcName != "max" {
+ return
+ }
+
+ // Check if this function matches the built-in min/max signature and behavior
+ if isEquivalentToBuiltinMinMax(pass, funcDecl, funcName) {
+ pass.Report(analysis.Diagnostic{
+ Pos: funcDecl.Pos(),
+ End: funcDecl.End(),
+ Message: fmt.Sprintf("user-defined %s function is equivalent to built-in %s and can be removed", funcName, funcName),
+ SuggestedFixes: []analysis.SuggestedFix{{
+ Message: fmt.Sprintf("Remove user-defined %s function", funcName),
+ TextEdits: []analysis.TextEdit{{
+ Pos: funcDecl.Pos(),
+ End: funcDecl.End(),
+ NewText: []byte(""),
+ }},
+ }},
+ })
+ }
+ })
+}
+
+// isEquivalentToBuiltinMinMax checks if a user-defined function is equivalent
+// to the built-in min/max function. Returns false if the function accepts
+// floating-point parameters due to NaN handling differences.
+func isEquivalentToBuiltinMinMax(pass *analysis.Pass, funcDecl *ast.FuncDecl, funcName string) bool {
+ // Must have a function type
+ if funcDecl.Type == nil {
+ return false
+ }
+
+ // Must have at least 2 parameters
+ if funcDecl.Type.Params == nil {
+ return false
+ }
+
+ // Count total parameter names
+ paramCount := 0
+ for _, param := range funcDecl.Type.Params.List {
+ paramCount += len(param.Names)
+ }
+
+ if paramCount < 2 {
+ return false
+ }
+
+ // Check if any parameter might be floating-point
+ for _, param := range funcDecl.Type.Params.List {
+ if paramType := pass.TypesInfo.TypeOf(param.Type); paramType != nil && maybeNaN(paramType) {
+ return false // Don't suggest removal for float types due to NaN handling
+ }
+ }
+
+ // Must have exactly one return value of the same type as parameters
+ if funcDecl.Type.Results == nil || len(funcDecl.Type.Results.List) != 1 {
+ return false
+ }
+
+ // Check that the function body implements the expected min/max logic
+ if funcDecl.Body == nil {
+ return false
+ }
+
+ return hasMinMaxLogic(funcDecl.Body, funcName)
+}
+
+// hasMinMaxLogic checks if the function body implements typical min/max logic.
+// This is a heuristic check for common patterns like:
+// - if a < b { return a } else { return b } (for min)
+// - if a > b { return a } else { return b } (for max)
+// - if a < b { return a }; return b (for min)
+// - if a > b { return a }; return b (for max)
+// - for loops with comparisons for variadic versions
+func hasMinMaxLogic(body *ast.BlockStmt, funcName string) bool {
+ if len(body.List) == 0 {
+ return false
+ }
+
+ // Look for simple if/else return pattern
+ for _, stmt := range body.List {
+ if ifStmt, ok := stmt.(*ast.IfStmt); ok {
+ if hasSimpleMinMaxIfElse(ifStmt, funcName) {
+ return true
+ }
+ }
+
+ // Look for for-loop with min/max logic (variadic case)
+ if forStmt, ok := stmt.(*ast.ForStmt); ok {
+ if hasMinMaxForLoop(forStmt, funcName) {
+ return true
+ }
+ }
+
+ // Look for range loop with min/max logic
+ if rangeStmt, ok := stmt.(*ast.RangeStmt); ok {
+ if hasMinMaxRangeLoop(rangeStmt, funcName) {
+ return true
+ }
+ }
+ }
+
+ // Check for pattern: if a < b { return a }; return b
+ if len(body.List) == 2 {
+ if ifStmt, ok := body.List[0].(*ast.IfStmt); ok {
+ if retStmt, ok := body.List[1].(*ast.ReturnStmt); ok {
+ return hasMinMaxIfReturnPattern(ifStmt, retStmt, funcName)
+ }
+ }
+ }
+
+ return false
+}
+
+// hasSimpleMinMaxIfElse checks for patterns like:
+// if a < b { return a } else { return b }
+// or: if a < b { return a }; return b
+func hasSimpleMinMaxIfElse(ifStmt *ast.IfStmt, funcName string) bool {
+ // Must have condition with comparison
+ binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr)
+ if !ok {
+ return false
+ }
+
+ // Check comparison operator matches function name
+ expectedOp := token.LSS // for min: if a < b
+ if funcName == "max" {
+ expectedOp = token.GTR // for max: if a > b
+ }
+
+ if binExpr.Op != expectedOp && binExpr.Op != token.LEQ && binExpr.Op != token.GEQ {
+ return false
+ }
+
+ // Check if then branch returns one of the compared values
+ if ifStmt.Body == nil || len(ifStmt.Body.List) != 1 {
+ return false
+ }
+
+ thenRet, ok := ifStmt.Body.List[0].(*ast.ReturnStmt)
+ if !ok || len(thenRet.Results) != 1 {
+ return false
+ }
+
+ // Check for explicit else block
+ if elseBlock, ok := ifStmt.Else.(*ast.BlockStmt); ok {
+ if len(elseBlock.List) != 1 {
+ return false
+ }
+
+ elseRet, ok := elseBlock.List[0].(*ast.ReturnStmt)
+ if !ok || len(elseRet.Results) != 1 {
+ return false
+ }
+
+ // Check that the returns match the comparison operands
+ // For min: if a < b { return a } else { return b }
+ // For max: if a > b { return a } else { return b }
+ return (equalSyntax(thenRet.Results[0], binExpr.X) && equalSyntax(elseRet.Results[0], binExpr.Y)) ||
+ (equalSyntax(thenRet.Results[0], binExpr.Y) && equalSyntax(elseRet.Results[0], binExpr.X))
+ }
+
+ // If no explicit else, the pattern should be checked at the function level
+ // This will be handled by hasMinMaxLogic
+ return false
+}
+
+// hasMinMaxForLoop checks for for-loop patterns that implement min/max logic
+func hasMinMaxForLoop(forStmt *ast.ForStmt, funcName string) bool {
+ // This is a simplified check - in practice, you'd want more sophisticated
+ // pattern matching for variadic min/max implementations
+ if forStmt.Body == nil {
+ return false
+ }
+
+ // Look for assignment statements with comparisons in the loop body
+ for _, stmt := range forStmt.Body.List {
+ if ifStmt, ok := stmt.(*ast.IfStmt); ok {
+ if binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr); ok {
+ expectedOp := token.LSS
+ if funcName == "max" {
+ expectedOp = token.GTR
+ }
+ if binExpr.Op == expectedOp || binExpr.Op == token.LEQ || binExpr.Op == token.GEQ {
+ return true
+ }
+ }
+ }
+ }
+
+ return false
+}
+
+// hasMinMaxRangeLoop checks for range-loop patterns that implement min/max logic
+func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {
+ // Similar to hasMinMaxForLoop but for range statements
+ if rangeStmt.Body == nil {
+ return false
+ }
+
+ for _, stmt := range rangeStmt.Body.List {
+ if ifStmt, ok := stmt.(*ast.IfStmt); ok {
+ if binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr); ok {
+ expectedOp := token.LSS
+ if funcName == "max" {
+ expectedOp = token.GTR
+ }
+ if binExpr.Op == expectedOp || binExpr.Op == token.LEQ || binExpr.Op == token.GEQ {
+ return true
+ }
+ }
+ }
+ }
+
+ return false
+}
+
+// hasMinMaxIfReturnPattern checks for patterns like:
+// if a < b { return a }; return b (for min)
+// if a > b { return a }; return b (for max)
+func hasMinMaxIfReturnPattern(ifStmt *ast.IfStmt, retStmt *ast.ReturnStmt, funcName string) bool {
+ // Must have condition with comparison
+ binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr)
+ if !ok {
+ return false
+ }
+
+ // Check comparison operator matches function name
+ expectedOp := token.LSS // for min: if a < b
+ if funcName == "max" {
+ expectedOp = token.GTR // for max: if a > b
+ }
+
+ if binExpr.Op != expectedOp && binExpr.Op != token.LEQ && binExpr.Op != token.GEQ {
+ return false
+ }
+
+ // Check if then branch returns one of the compared values
+ if ifStmt.Body == nil || len(ifStmt.Body.List) != 1 {
+ return false
+ }
+
+ thenRet, ok := ifStmt.Body.List[0].(*ast.ReturnStmt)
+ if !ok || len(thenRet.Results) != 1 {
+ return false
+ }
+
+ // Check final return statement
+ if len(retStmt.Results) != 1 {
+ return false
+ }
+
+ // Check that the returns match the comparison operands
+ // For min: if a < b { return a }; return b
+ // For max: if a > b { return a }; return b
+ return (equalSyntax(thenRet.Results[0], binExpr.X) && equalSyntax(retStmt.Results[0], binExpr.Y)) ||
+ (equalSyntax(thenRet.Results[0], binExpr.Y) && equalSyntax(retStmt.Results[0], binExpr.X))
+}
+
// -- utils --
func is[T any](x any) bool {
diff --git a/go/analysis/passes/modernize/testdata/src/minmax/minmax.go b/go/analysis/passes/modernize/testdata/src/minmax/minmax.go
index 74d84b2..49e670f 100644
--- a/go/analysis/passes/modernize/testdata/src/minmax/minmax.go
+++ b/go/analysis/passes/modernize/testdata/src/minmax/minmax.go
@@ -169,3 +169,77 @@
}
return x
}
+
+func min(a, b int) int { // want "user-defined min function is equivalent to built-in min and can be removed"
+ if a < b {
+ return a
+ } else {
+ return b
+ }
+}
+
+func max(a, b int) int { // want "user-defined max function is equivalent to built-in max and can be removed"
+ if a > b {
+ return a
+ } else {
+ return b
+ }
+}
+
+// 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 {
+ return a
+ } else {
+ return b
+ }
+}
+
+// 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{}
+
+func (m MyType) min(a, b int) int {
+ if a < b {
+ return a
+ } else {
+ return b
+ }
+}
+
+// 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 {
+ print("choosing min")
+ if a < b {
+ return a
+ } else {
+ return b
+ }
+}
diff --git a/go/analysis/passes/modernize/testdata/src/minmax/minmax.go.golden b/go/analysis/passes/modernize/testdata/src/minmax/minmax.go.golden
index f8dc94b..dcf169c 100644
--- a/go/analysis/passes/modernize/testdata/src/minmax/minmax.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/minmax/minmax.go.golden
@@ -168,3 +168,61 @@
}
return x
}
+
+// 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 {
+ return a
+ } else {
+ return b
+ }
+}
+
+// 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{}
+
+func (m MyType) min(a, b int) int {
+ if a < b {
+ return a
+ } else {
+ return b
+ }
+}
+
+// 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 {
+ print("choosing min")
+ if a < b {
+ return a
+ } else {
+ return b
+ }
+}
Change information
- M go/analysis/passes/modernize/minmax.go
- M go/analysis/passes/modernize/testdata/src/minmax/minmax.go
- M go/analysis/passes/modernize/testdata/src/minmax/minmax.go.golden
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Alan Donovan
- shuang cui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui added 1 comment![]( "Open in Gerrit")
Thanks @adon...@google.com for the help and advice.
The functionality is complete, and I have added the corresponding test cases.
However, I'm not sure if there is a simpler way to detect this situation with the current tools.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 6 comments![]( "Open in Gerrit")
inspect.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {Rather than making a pass over all the declarations, since we're only looking for two functions, why not look them up by name? Use `pass.Pkg.Scope().Lookup("min")` check that it's a `*types.Func`. You can use typeindexanalyzer (`Index.Def(obj)`) to find its FuncDecl in constant time.
Pos: funcDecl.Pos(),This should expand to include any preceding doc comment. You might want to steal this code from gopls' `unusedfunc` analyzer (code that probably one day belongs in a library):
```
// Expand to include leading doc comment.
pos := node.Pos()
if doc := docComment(node); doc != nil {
pos = doc.Pos()
}
func docComment(n ast.Node) *ast.CommentGroup {
switch n := n.(type) {
case *ast.FuncDecl:
return n.Doc
case *ast.GenDecl:
return n.Doc
case *ast.ValueSpec:
return n.Doc
case *ast.TypeSpec:
return n.Doc
}
return nil // includes File, ImportSpec, Field
}
```
NewText: []byte(""),You can delete this line.
// Must have a function type
if funcDecl.Type == nil {
return false
}
// Must have at least 2 parameters
if funcDecl.Type.Params == nil {
return false
}
// Count total parameter names
paramCount := 0
for _, param := range funcDecl.Type.Params.List {
paramCount += len(param.Names)
}
if paramCount < 2 {
return false
}
// Check if any parameter might be floating-point
for _, param := range funcDecl.Type.Params.List {
if paramType := pass.TypesInfo.TypeOf(param.Type); paramType != nil && maybeNaN(paramType) {
return false // Don't suggest removal for float types due to NaN handling
}
}
// Must have exactly one return value of the same type as parameters
if funcDecl.Type.Results == nil || len(funcDecl.Type.Results.List) != 1 {
return false
}
This operation is more conveniently expressed on the types.Signature instead of the FuncDecl syntax.
if len(body.List) == 0 {
return false
}You can delete this; it doesn't save any work.
func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {I don't understand the relationship of this function to the rest of the change. Ditto the hasMinMaxForLoop. Can you explain?
Might I ask: was this code primarily written by an LLM? If so, I think we might need to establish some clear ground rules for their use in contributions.
Related details
- shuang cui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui added 2 comments![]( "Open in Gerrit")
inspect.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {Rather than making a pass over all the declarations, since we're only looking for two functions, why not look them up by name? Use `pass.Pkg.Scope().Lookup("min")` check that it's a `*types.Func`. You can use typeindexanalyzer (`Index.Def(obj)`) to find its FuncDecl in constant time.
Thanks for your suggestion, I'll make the changes later.
func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {I don't understand the relationship of this function to the rest of the change. Ditto the hasMinMaxForLoop. Can you explain?
Might I ask: was this code primarily written by an LLM? If so, I think we might need to establish some clear ground rules for their use in contributions.
Yes, I used Claude to generate some logic here. But the problem with hasMinMaxLogic and hasMinMaxRangeLoop is actually more my fault: Before making this suggestion, my goal was to eliminate all use of min/max in Go libraries, and I had already "implemented" this locally—but this is unrealistic. Some min/max functions aren't part of the standard library and shouldn't be replaced.
For example, https://github.com/golang/crypto/blob/master/ssh/channel.go#L134 and https://github.com/golang/go/blob/master/test/235.go#L26 (and there are many more).
My misunderstanding and unrealistic expectations led to Claude overdoing it. However, the test case is designed for situations like the one in the title, which pass perfectly.
Sorry for any inconvenience caused. I agree with your point of view. I will review it again and again after submitting it, not just through the test cases.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui added 1 comment![]( "Open in Gerrit")
func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {shuang cuiI don't understand the relationship of this function to the rest of the change. Ditto the hasMinMaxForLoop. Can you explain?
Might I ask: was this code primarily written by an LLM? If so, I think we might need to establish some clear ground rules for their use in contributions.
Yes, I used Claude to generate some logic here. But the problem with hasMinMaxLogic and hasMinMaxRangeLoop is actually more my fault: Before making this suggestion, my goal was to eliminate all use of min/max in Go libraries, and I had already "implemented" this locally—but this is unrealistic. Some min/max functions aren't part of the standard library and shouldn't be replaced.
For example, https://github.com/golang/crypto/blob/master/ssh/channel.go#L134 and https://github.com/golang/go/blob/master/test/235.go#L26 (and there are many more).
My misunderstanding and unrealistic expectations led to Claude overdoing it. However, the test case is designed for situations like the one in the title, which pass perfectly.
Sorry for any inconvenience caused. I agree with your point of view. I will review it again and again after submitting it, not just through the test cases.
It seems that I may have ignored the case of https://github.com/golang/crypto/blob/master/ssh/channel.go#L134, but did not rule out https://github.com/golang/go/blob/master/test/235.go#L26 and other cases
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- shuang cui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- shuang cui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui added 1 comment![]( "Open in Gerrit")
In fact, according to the standard library definition of min/max, the number of arguments can be in the range [1, +∞). For example, both min(1) and min(1, 2, 3, 4, -1) are valid. Here we should only consider the most common case, where the number of arguments is 2.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
shuang cui (Gerrit)
shuang cui voted and added 6 comments![]( "Open in Gerrit")
Votes added by shuang cui
| Commit-Queue | +1 |
6 comments
inspect.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {shuang cuiRather than making a pass over all the declarations, since we're only looking for two functions, why not look them up by name? Use `pass.Pkg.Scope().Lookup("min")` check that it's a `*types.Func`. You can use typeindexanalyzer (`Index.Def(obj)`) to find its FuncDecl in constant time.
Thanks for your suggestion, I'll make the changes later.
Using pass.Pkg.Scope().Lookup is indeed a better solution; I didn’t know it could be done this way before.
Some parts were assisted by Claude, but I reviewed them again myself. I also made slight adjustments to the test cases to cover more scenarios.
This should expand to include any preceding doc comment. You might want to steal this code from gopls' `unusedfunc` analyzer (code that probably one day belongs in a library):
```
// Expand to include leading doc comment.
pos := node.Pos()
if doc := docComment(node); doc != nil {
pos = doc.Pos()
}
func docComment(n ast.Node) *ast.CommentGroup {
switch n := n.(type) {
case *ast.FuncDecl:
return n.Doc
case *ast.GenDecl:
return n.Doc
case *ast.ValueSpec:
return n.Doc
case *ast.TypeSpec:
return n.Doc
}
return nil // includes File, ImportSpec, Field
}
```
Done
You can delete this line.
Done
// Must have a function type
if funcDecl.Type == nil {
return false
}
// Must have at least 2 parameters
if funcDecl.Type.Params == nil {
return false
}
// Count total parameter names
paramCount := 0
for _, param := range funcDecl.Type.Params.List {
paramCount += len(param.Names)
}
if paramCount < 2 {
return false
}
// Check if any parameter might be floating-point
for _, param := range funcDecl.Type.Params.List {
if paramType := pass.TypesInfo.TypeOf(param.Type); paramType != nil && maybeNaN(paramType) {
return false // Don't suggest removal for float types due to NaN handling
}
}
// Must have exactly one return value of the same type as parameters
if funcDecl.Type.Results == nil || len(funcDecl.Type.Results.List) != 1 {
return false
}
This operation is more conveniently expressed on the types.Signature instead of the FuncDecl syntax.
Done
You can delete this; it doesn't save any work.
Done
func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {shuang cuiI don't understand the relationship of this function to the rest of the change. Ditto the hasMinMaxForLoop. Can you explain?
Might I ask: was this code primarily written by an LLM? If so, I think we might need to establish some clear ground rules for their use in contributions.
shuang cuiYes, I used Claude to generate some logic here. But the problem with hasMinMaxLogic and hasMinMaxRangeLoop is actually more my fault: Before making this suggestion, my goal was to eliminate all use of min/max in Go libraries, and I had already "implemented" this locally—but this is unrealistic. Some min/max functions aren't part of the standard library and shouldn't be replaced.
For example, https://github.com/golang/crypto/blob/master/ssh/channel.go#L134 and https://github.com/golang/go/blob/master/test/235.go#L26 (and there are many more).
My misunderstanding and unrealistic expectations led to Claude overdoing it. However, the test case is designed for situations like the one in the title, which pass perfectly.
Sorry for any inconvenience caused. I agree with your point of view. I will review it again and again after submitting it, not just through the test cases.
It seems that I may have ignored the case of https://github.com/golang/crypto/blob/master/ssh/channel.go#L134, but did not rule out https://github.com/golang/go/blob/master/test/235.go#L26 and other cases
Done
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 17 comments![]( "Open in Gerrit")
Thanks, this all looks sound (but for one question about token.LEQ), but I think we can better factor the code to avoid repetition.
// the same signature and logic as built-in min/max, except when theyThe logic of built-in min/max is not something we can easily know (it's a compiler intrinsic), and in fact it is more complex due to NaNs. So let's say simply:
```
// User-defined min/max functions are suggested for removal if they may
// be safely replaced by their built-in namesake.
```
and leave the definition of safety to the logic below.
if obj := pass.Pkg.Scope().Lookup(funcName); obj != nil {
if fn, ok := obj.(*types.Func); ok {Combine:
```
if fn, ok := pass.Pkg.Scope().Lookup(funcName).(*types.Func); ok {
```
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
inspect.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {
decl := n.(*ast.FuncDecl)
if decl.Name != nil && decl.Name.Name == funcName {
// Verify this is the same function by checking position
if pass.TypesInfo.Defs[decl.Name] == fn {This could be replaced by
```
decl := index.Def(fn).Node().(*ast.FuncDecl)
```
where
```
index = pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index)
```
if isEquivalentToBuiltinMinMax(pass, fn, decl, funcName) {strength-reduce to just decl.Body
// isEquivalentToBuiltinMinMax checks if a user-defined function is equivalentFunction equivalence is an undecidable problem. Let's go with `canUseBuiltinMinMax`, which does not promise the impossible.
```
// canUseBuiltinMinMax reports whether it is safe to replace a call
// to this min or max function by its built-in namesake.
```
We needn't mention floats here; it's an implementation detail covered by the logic.
func isEquivalentToBuiltinMinMax(pass *analysis.Pass, fn *types.Func, funcDecl *ast.FuncDecl, funcName string) bool {Redundant; use `fn.Name()`.
func isEquivalentToBuiltinMinMax(pass *analysis.Pass, fn *types.Func, funcDecl *ast.FuncDecl, funcName string) bool {unused
for i := 0; i < sig.Params().Len(); i++ {for param := range sig.Params().Variables() {
binExpr, ok := ifStmt.Cond.(*ast.BinaryExpr)cmp (since it's a comparison)
if binExpr.Op != expectedOp && binExpr.Op != token.LEQ && binExpr.Op != token.GEQ {What's going on here? Could you add a test case that exercises LEQ?
if binExpr.Op != expectedOp && binExpr.Op != token.LEQ && binExpr.Op != token.GEQ {`cond(name == "max", token.GTR, token.LSS)`
if ifStmt.Body == nil || len(ifStmt.Body.List) != 1 {Always true for an IfStmt; delete.
func hasMinMaxIfReturnPattern(ifStmt *ast.IfStmt, retStmt *ast.ReturnStmt, funcName string) bool {This function is substantially similar to the previous one. Please factor the common parts. The only difference is how to find the "false" result expression.
Once that's done, I think the merged logic could all go in hasMinMaxLogic, again factoring the handling of IfStmt. Once you have the two expressions, everything else is common to both forms.
if binExpr.Op != expectedOp && binExpr.Op != token.LEQ && binExpr.Op != token.GEQ {ditto
// docComment returns the doc comment for a node, if any.Move this below the `-- utils --` fold.
(We should move many of these helpers into a library at some point.)
func hasMinMaxRangeLoop(rangeStmt *ast.RangeStmt, funcName string) bool {shuang cuiI don't understand the relationship of this function to the rest of the change. Ditto the hasMinMaxForLoop. Can you explain?
Might I ask: was this code primarily written by an LLM? If so, I think we might need to establish some clear ground rules for their use in contributions.
shuang cuiYes, I used Claude to generate some logic here. But the problem with hasMinMaxLogic and hasMinMaxRangeLoop is actually more my fault: Before making this suggestion, my goal was to eliminate all use of min/max in Go libraries, and I had already "implemented" this locally—but this is unrealistic. Some min/max functions aren't part of the standard library and shouldn't be replaced.
For example, https://github.com/golang/crypto/blob/master/ssh/channel.go#L134 and https://github.com/golang/go/blob/master/test/235.go#L26 (and there are many more).
My misunderstanding and unrealistic expectations led to Claude overdoing it. However, the test case is designed for situations like the one in the title, which pass perfectly.
Sorry for any inconvenience caused. I agree with your point of view. I will review it again and again after submitting it, not just through the test cases.
shuang cuiIt seems that I may have ignored the case of https://github.com/golang/crypto/blob/master/ssh/channel.go#L134, but did not rule out https://github.com/golang/go/blob/master/test/235.go#L26 and other cases
Done
I see; thanks for the explanation, that makes sense. I suspect the number of variadic min/max functions is rather smaller than the binary ones, and probably not worth handling as the combinatorics of syntax explode pretty quickly.
Related details
- shuang cui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |