go/version: IsValid reports false on inputs like "go1.8.5rc5", "go1.9.2rc2"

246859 commented 1 month ago

Go version

go version go1.22.5 windows/amd64

Output of `go env` in your module/workspace:

$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=D:\work\libs\golang\mods\bin
set GOCACHE=D:\work\libs\golang\cache
set GOENV=D:\work\libs\golang\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=D:\work\libs\golang\mods\libs
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\work\libs\golang\mods
set GOPRIVATE=
set GOPROXY=https://goproxy.cn,direct
set GOROOT=D:\work\libs\golang\root\go1.22.5
set GOSUMDB=sum.golang.org
set GOTMPDIR=D:\work\libs\golang\temp
set GOTOOLCHAIN=auto
set GOTOOLDIR=D:\work\libs\golang\root\go1.22.5\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22.5
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=D:\work\libs\golang\temp\go-build838424048=/tmp/go-build -gno-record-gcc-switches

What did you do?

When I try to sort go version list that got from below command

git ls-remote -tags --sort=version:refname https://github.com/golang/go

I got an incredible result that the version go1.8.5rc5 is the minimum version, even less than go1. The reason is that go/version.IsValid do not consider prerelease patch as valid version, but some prerelease patch really do exist in history versions, such as go1.8.5rc5, go1.8.5rc4, go1.9.2rc2 .The version go1.9.2rc2 even could be discovered from https://go.dev/dl/?mode=json&include=all, and downloaded from https://dl.google.com/go/go1.9.2rc2.linux-amd64.tar.gz, it's very unreasonable to treat it as an invalid version.

Here is a snippset to prove this: https://go.dev/play/p/TBz7px5tMd_8

What did you see happen?

fmt.Println(version.IsValid("go1.9.2rc2"))

output

false

What did you expect to see?

fmt.Println(version.IsValid("go1.9.2rc2"))

output

true

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

seankhliao commented 1 month ago

cc @rsc seems like 1.8.5 was the only patch version to have release candidate tags? ref #22264

dmitshur commented 1 month ago

The reason go1.8.5rc5 sorts as oldest is because the go/version package doesn't consider it valid; go1 isn't very relevant as you could pick any other valid version to compare with. Compare docs include:

Invalid versions, including the empty string, compare less than valid versions and equal to each other.

And version.IsValid("go1.8.5rc5") reports false.

These case will not happen If I get versions from https://go.dev/dl/?mode=json&include=all, because go1.8.5rc5 and go1.8.5rc4 is not exist in that list.

Interestingly, go1.9.2rc2 is an old Go version that can be downloaded from https://go.dev/dl/#go1.9.2rc2 and exists in the versions list API output, but doesn't have a corresponding tag.

I asked about the intended behavior these IsValid edge cases in https://github.com/golang/go/issues/62039#issuecomment-1680697372 but I don't see an answer there.

246859 commented 1 month ago

Interestingly, go1.9.2rc2 is an old Go version that can be downloaded from https://go.dev/dl/#go1.9.2rc2 and exists in the versions list API output, but doesn't have a corresponding tag.

I found this point too when I tested it more deeply, and version.IsValid doesn't seem to be dealing with prereleases (alpha, beta, rc) for patch releases, as the comments says:

// Parse patch if present.
if x[0] == '.' {
    v.Patch, x, ok = cutInt(x[1:])
    if !ok || x != "" {
        // Note that we are disallowing prereleases (alpha, beta, rc) for patch releases here (x != "").
        // Allowing them would be a bit confusing because we already have:
        //  1.21 < 1.21rc1
        // But a prerelease of a patch would have the opposite effect:
        //  1.21.3rc1 < 1.21.3
        // We've never needed them before, so let's not start now.
        return Version{}
    }
    return v
}

I don't think this is a rational reason to disallowing prereleases for patch releases

 We've never needed them before, so let's not start now.

So I'd like to create a pull request to solve this.

gopherbot commented 1 month ago

Change https://go.dev/cl/602096 mentions this issue: go/version,internal/gover: treat prerelease patch as valid version

dmitshur commented 1 month ago

At this time it's not clear to me that the scope of this package intends to consider a number of one-off old (pre-go1.21.0) releases that followed the "goA.B.CrcN" pattern as valid. The package comment says:

Package version provides operations on Go versions in Go toolchain name syntax: strings like "go1.20", "go1.21.0", "go1.22rc2", and "go1.23.4-bigcorp".

Only "goA.BrcN" pre-release version strings come up in https://go.dev/doc/toolchain. The current release process includes only those pre-release versions, there aren't pre-releases for minor Go releases, so version.IsValid correctly reports true for all planned future Go release versions.

Maybe @rsc has more thoughts on this, otherwise I don't expect the behavior of version.IsValid on old versions strings (i.e., before go1.21.0) needs any changes.

246859 commented 1 month ago

At this time it's not clear to me that the scope of this package intends to consider a number of one-off old (pre-go1.21.0) releases that followed the "goA.B.CrcN" pattern as valid. The package comment says:

Package version provides operations on Go versions in Go toolchain name syntax: strings like "go1.20", "go1.21.0", "go1.22rc2", and "go1.23.4-bigcorp".

Only "goA.BrcN" pre-release version strings come up in https://go.dev/doc/toolchain. The current release process includes only those pre-release versions, there aren't pre-releases for minor Go releases, so version.IsValid correctly reports true for all planned future Go release versions.

Maybe @rsc has more thoughts on this, otherwise I don't expect the behavior of version.IsValid on old versions strings (i.e., before go1.21.0) needs any changes.

Thanks for your patient replying. Actually the cause of this issue is that I wrote a tiny cmd tool a week ago which could manage multiple versions for go. At the beginning of the work, the go version rules make me confused. After scanning docs about Go version, I found rules as
below:

Before 1.21, the first minor is 1.N, it became 1.N.0 after 1.21.
Before 1.21, prelease was always less than language version, due to above change, it was opposite after 1.21.
Prelease patch is less than release patch.

I think I should handle all possible situations, so I tried to depend on std pkg go/version to deal with them. Then the scene I described at the issue beginning happened.

Although I no longer rely on this pkg now, I still have some questions:

What's the reason and benefit for change first patch to 1.N.0 ?
Why is it reasonable to treat these existing old versions as invalid just simply because they are used so little? Even if future versions do not consider pre-release patches, but they also may cause some problems as below:
1. verson.IsValid("go1.9.2rc2") report false
2. version.Compare("go1.9.2rc2", "go1") = -1
3. both slices.Sort and slices.Min may produce wrong output when input contain prerelease patch versions

I'd be glad if you could answer these questions

golang / go