Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/mod: semver.Max does more than compute the max #32700

Open
bcmills opened this issue Jun 19, 2019 · 5 comments
Open

x/mod: semver.Max does more than compute the max #32700

bcmills opened this issue Jun 19, 2019 · 5 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 19, 2019

Currently, semver.Max canonicalizes its arguments according to semver.Canonical:
https://play.golang.org/p/04IP9gNhFWH

To me, the name Max does not evoke “canonicalize”, and the action of canonicalizing is potentially fairly destructive: for example, if I have a version with a +incompatible build suffix, that version is canonical according to module.CanonicalVersion, but will not be preserved by a call to semver.Max.

@rsc points out that the canonicalization avoids ambiguity: v0.1.0+a and v0.1.0+b have the same precedence, so if we call semver.Max("v0.1.0+a", "v0.1.0+b") we would otherwise have to pick one arbitrarily, and semver.org says “Build metadata SHOULD be ignored when determining version precedence.”

Instead, I propose that Max should always return one of its arguments. Build metadata must comprise only the usual dot-separated ASCII identifiers, so if we have two versions with equal precedence it seems to me that we could break the tie using the usual rule for comparing pre-release suffixes.

CC @jayconrod @hyangah

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 20, 2019

Change https://golang.org/cl/212200 mentions this issue: cmd/go: avoid erroneous canonicalization when trying to resolve imports using replacements

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 20, 2019

Change https://golang.org/cl/212202 mentions this issue: cmd/go/internal/modfetch/codehost: replace a dubious call to semver.Max

gopherbot pushed a commit that referenced this issue Dec 20, 2019
…ts using replacements

Updates #32700
Fixes #33795

Change-Id: I16897a0a2f3aa2f0b0bf8cf8252f3f39eef2e7ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/212200
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Dec 20, 2019
The documentation for RecentTag indicates that it returns an actual
tag, not a canonicalized prefix+version blob equivalent to a tag,
so the canonicalization due to semver.Max seems like a bug here.

Fortunately, RecentTag is not currently ever actually used as a tag,
so the removal of metadata does not result in a user-facing bug.
Nonetheless, it may be a subtle source of confusion for maintainers
in the future.

Updates #32700

Change-Id: I525423c1c0c7ec7c36c09e53b180034474f74e5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/212202
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dsymonds
Copy link
Member

@dsymonds dsymonds commented May 14, 2020

Alternatively, replace Max with Less, which just returns a bool. It is trivial to wrap to get the existing Max semantics in existing code, and avoids the issue by not even trying to return one arg or the other.

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 14, 2020

@dsymonds, we already have Compare, which is even more ergonomic than Less in general. But it's true that we could just remove Max instead of providing a version of it with dubious semantics.

@dsymonds
Copy link
Member

@dsymonds dsymonds commented May 14, 2020

Oh, I overlooked Compare. Yeah, Max seems extra pointless; callers who care can use Compare, then if they actually care about canonicalisation they can use Canonical explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.