x/tools/go/analysis/passes/modernize: minmax: suggest deletion of redundant min and max functions

[cuiweixie]

Proposal Details

I proposal to add a checker to check whether user has impl a max/min instead of builtin max/min.
And suggest user to replace with builtin max/min.
search

language:go "func min"

in github with get about 142k result in code.

[cuiweixie]

cometbft/cometbft#5290

[seankhliao]

Afaik this is partly already part of the modernizer analyzers, which may be included in the future as part of go fix.
this wouldn't be part of vet which centers on correctness.

[zigo101]

Afaik this is partly already part of the modernizer analyzers, which may be included in the future as part of go fix.

@seankhliao Could you elaborate more on this? I never heard of such a thing.

@aclements @ianlancetaylor Did you know what @seankhliao talked about?

[ianlancetaylor]

From @adonovan, see https://www.gophercon.com/agenda/session/1557387 . But I don't know if the talk is online yet.

[adonovan]

As @seankhliao said, we have a modernizer for it (any minute now https://go.dev/cl/706918 will land, which makes it an official public API, so you can embed it in your own analysis tools). If you need to run it in the next few seconds, you can use this command:

$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -category=minmax -fix -diff packages...

Longer term, our plan is for this to appear in go fix for the go1.26 release.

[zigo101]

modernize@master fails to build. modernize@latest does nothing for the following code:

package main

func min(a, b int) int {
    if a < b {
        return a
    }
    return b
}

func max(a, b int) int {
    if a < b {
        return b
    }
    return a
}

func main() {
    println(min(1, 2), max(1, 2))
}

I'm wondering that how confidently the tool will think a custom min or max function should be replaced by the builtin ones.

[adonovan]

modernize@master fails to build.
The gopls module has a replace directive, so can only build at tags on the release branch suc as latest.

modernize@latest does nothing for the following code: [...]

Good point. Because these functions are named min and max, shadowing the corresponding predeclared names, the modernizer cannot transform their bodies. If you rename the function, then it will work; of course, you might as well just delete it.

This is indeed an unfortunate limitation of the current minmax modernizer. It would be nice to make it recognize when you can simply delete the whole declaration, and it wouldn't be a difficult change.

func min(a, b int) int { // minmax: "min function is redundant with go1.21 predeclared min function"
    if a < b {
        return a
    }
    return b
}

Reopening this issue to address that.