cmd/go/internal/modfetch: do not short-circuit canonical versions

Since at least CL 121857, the conversion logic in
(*modfetch).codeRepo.Stat has had a short-circuit to use the version
requested by the caller if it successfully resolves and is already
canonical.

However, we should not use that version if it refers to a branch
instead of a tag, because branches (unlike tags) usually do not refer
to a single, stable release: a branch named "v1.0.0" may be for the
development of the v1.0.0 release, or for the development of patches
based on v1.0.0, but only one commit (perhaps at the end of that
branch — but possibly not even written yet!) can be that specific
version.

We already have some logic to prefer tags that are semver-equivalent
to the version requested by the caller. That more general case
suffices for exact equality too — so we can eliminate the
special-case, fixing the bug and (happily!) also somewhat simplifying
the code.

Fixes #35671
Updates #41512

Change-Id: I2fd290190b8a99a580deec7e26d15659b58a50b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/378400
Trust: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: Bryan C. Mills

src/cmd/go/internal/modfetch/coderepo.go

@@ -298,16 +298,13 @@ func (r *codeRepo) Latest() (*RevInfo, error) {
 // If statVers is a valid module version, it is used for the Version field.
 // Otherwise, the Version is derived from the passed-in info and recent tags.
 func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, error) {
-	info2 := &RevInfo{
-		Name:  info.Name,
-		Short: info.Short,
-		Time:  info.Time,
-	}
-
 	// If this is a plain tag (no dir/ prefix)
 	// and the module path is unversioned,
 	// and if the underlying file tree has no go.mod,
 	// then allow using the tag with a +incompatible suffix.
+	//
+	// (If the version is +incompatible, then the go.mod file must not exist:
+	// +incompatible is not an ongoing opt-out from semantic import versioning.)
 	var canUseIncompatible func() bool
 	canUseIncompatible = func() bool {
 		var ok bool
@@ -321,19 +318,12 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
 		return ok
 	}
 
-	invalidf := func(format string, args ...any) error {
-		return &module.ModuleError{
-			Path: r.modPath,
-			Err: &module.InvalidVersionError{
-				Version: info2.Version,
-				Err:     fmt.Errorf(format, args...),
-			},
-		}
-	}
-
-	// checkGoMod verifies that the go.mod file for the module exists or does not
-	// exist as required by info2.Version and the module path represented by r.
-	checkGoMod := func() (*RevInfo, error) {
+	// checkCanonical verifies that the canonical version v is compatible with the
+	// module path represented by r, adding a "+incompatible" suffix if needed.
+	//
+	// If statVers is also canonical, checkCanonical also verifies that v is
+	// either statVers or statVers with the added "+incompatible" suffix.
+	checkCanonical := func(v string) (*RevInfo, error) {
 		// If r.codeDir is non-empty, then the go.mod file must exist: the module
 		// author — not the module consumer, — gets to decide how to carve up the repo
 		// into modules.
@@ -344,73 +334,91 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
 		// r.findDir verifies both of these conditions. Execute it now so that
 		// r.Stat will correctly return a notExistError if the go.mod location or
 		// declared module path doesn't match.
-		_, _, _, err := r.findDir(info2.Version)
+		_, _, _, err := r.findDir(v)
 		if err != nil {
 			// TODO: It would be nice to return an error like "not a module".
 			// Right now we return "missing go.mod", which is a little confusing.
 			return nil, &module.ModuleError{
 				Path: r.modPath,
 				Err: &module.InvalidVersionError{
-					Version: info2.Version,
+					Version: v,
 					Err:     notExistError{err: err},
 				},
 			}
 		}
 
-		// If the version is +incompatible, then the go.mod file must not exist:
-		// +incompatible is not an ongoing opt-out from semantic import versioning.
-		if strings.HasSuffix(info2.Version, "+incompatible") {
-			if !canUseIncompatible() {
+		invalidf := func(format string, args ...any) error {
+			return &module.ModuleError{
+				Path: r.modPath,
+				Err: &module.InvalidVersionError{
+					Version: v,
+					Err:     fmt.Errorf(format, args...),
+				},
+			}
+		}
+
+		// Add the +incompatible suffix if needed or requested explicitly, and
+		// verify that its presence or absence is appropriate for this version
+		// (which depends on whether it has an explicit go.mod file).
+
+		if v == strings.TrimSuffix(statVers, "+incompatible") {
+			v = statVers
+		}
+		base := strings.TrimSuffix(v, "+incompatible")
+		var errIncompatible error
+		if !module.MatchPathMajor(base, r.pathMajor) {
+			if canUseIncompatible() {
+				v = base + "+incompatible"
+			} else {
 				if r.pathMajor != "" {
-					return nil, invalidf("+incompatible suffix not allowed: module path includes a major version suffix, so major version must match")
+					errIncompatible = invalidf("module path includes a major version suffix, so major version must match")
 				} else {
-					return nil, invalidf("+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required")
+					errIncompatible = invalidf("module contains a go.mod file, so module path must match major version (%q)", path.Join(r.pathPrefix, semver.Major(v)))
 				}
 			}
+		} else if strings.HasSuffix(v, "+incompatible") {
+			errIncompatible = invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(v))
+		}
 
-			if err := module.CheckPathMajor(strings.TrimSuffix(info2.Version, "+incompatible"), r.pathMajor); err == nil {
-				return nil, invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(info2.Version))
+		if statVers != "" && statVers == module.CanonicalVersion(statVers) {
+			// Since the caller-requested version is canonical, it would be very
+			// confusing to resolve it to anything but itself, possibly with a
+			// "+incompatible" suffix. Error out explicitly.
+			if statBase := strings.TrimSuffix(statVers, "+incompatible"); statBase != base {
+				return nil, &module.ModuleError{
+					Path: r.modPath,
+					Err: &module.InvalidVersionError{
+						Version: statVers,
+						Err:     fmt.Errorf("resolves to version %v (%s is not a tag)", v, statBase),
+					},
+				}
 			}
 		}
 
-		return info2, nil
+		if errIncompatible != nil {
+			return nil, errIncompatible
+		}
+
+		return &RevInfo{
+			Name:    info.Name,
+			Short:   info.Short,
+			Time:    info.Time,
+			Version: v,
+		}, nil
 	}
 
 	// Determine version.
-	//
-	// If statVers is canonical, then the original call was repo.Stat(statVers).
-	// Since the version is canonical, we must not resolve it to anything but
-	// itself, possibly with a '+incompatible' annotation: we do not need to do
-	// the work required to look for an arbitrary pseudo-version.
-	if statVers != "" && statVers == module.CanonicalVersion(statVers) {
-		info2.Version = statVers
-
-		if module.IsPseudoVersion(info2.Version) {
-			if err := r.validatePseudoVersion(info, info2.Version); err != nil {
-				return nil, err
-			}
-			return checkGoMod()
-		}
 
-		if err := module.CheckPathMajor(info2.Version, r.pathMajor); err != nil {
-			if canUseIncompatible() {
-				info2.Version += "+incompatible"
-				return checkGoMod()
-			} else {
-				if vErr, ok := err.(*module.InvalidVersionError); ok {
-					// We're going to describe why the version is invalid in more detail,
-					// so strip out the existing “invalid version” wrapper.
-					err = vErr.Err
-				}
-				return nil, invalidf("module contains a go.mod file, so major version must be compatible: %v", err)
-			}
+	if module.IsPseudoVersion(statVers) {
+		if err := r.validatePseudoVersion(info, statVers); err != nil {
+			return nil, err
 		}
-
-		return checkGoMod()
+		return checkCanonical(statVers)
 	}
 
-	// statVers is empty or non-canonical, so we need to resolve it to a canonical
-	// version or pseudo-version.
+	// statVers is not a pseudo-version, so we need to either resolve it to a
+	// canonical version or verify that it is already a canonical tag
+	// (not a branch).
 
 	// Derive or verify a version from a code repo tag.
 	// Tag must have a prefix matching codeDir.
@@ -441,71 +449,62 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
 		if v == "" || !strings.HasPrefix(trimmed, v) {
 			return "", false // Invalid or incomplete version (just vX or vX.Y).
 		}
-		if isRetracted(v) {
-			return "", false
-		}
 		if v == trimmed {
 			tagIsCanonical = true
 		}
-
-		if err := module.CheckPathMajor(v, r.pathMajor); err != nil {
-			if canUseIncompatible() {
-				return v + "+incompatible", tagIsCanonical
-			}
-			return "", false
-		}
-
 		return v, tagIsCanonical
 	}
 
 	// If the VCS gave us a valid version, use that.
 	if v, tagIsCanonical := tagToVersion(info.Version); tagIsCanonical {
-		info2.Version = v
-		return checkGoMod()
+		if info, err := checkCanonical(v); err == nil {
+			return info, err
+		}
 	}
 
 	// Look through the tags on the revision for either a usable canonical version
 	// or an appropriate base for a pseudo-version.
-	var pseudoBase string
+	var (
+		highestCanonical string
+		pseudoBase       string
+	)
 	for _, pathTag := range info.Tags {
 		v, tagIsCanonical := tagToVersion(pathTag)
-		if tagIsCanonical {
-			if statVers != "" && semver.Compare(v, statVers) == 0 {
-				// The user requested a non-canonical version, but the tag for the
-				// canonical equivalent refers to the same revision. Use it.
-				info2.Version = v
-				return checkGoMod()
+		if statVers != "" && semver.Compare(v, statVers) == 0 {
+			// The tag is equivalent to the version requested by the user.
+			if tagIsCanonical {
+				// This tag is the canonical form of the requested version,
+				// not some other form with extra build metadata.
+				// Use this tag so that the resolved version will match exactly.
+				// (If it isn't actually allowed, we'll error out in checkCanonical.)
+				return checkCanonical(v)
 			} else {
-				// Save the highest canonical tag for the revision. If we don't find a
-				// better match, we'll use it as the canonical version.
+				// The user explicitly requested something equivalent to this tag. We
+				// can't use the version from the tag directly: since the tag is not
+				// canonical, it could be ambiguous. For example, tags v0.0.1+a and
+				// v0.0.1+b might both exist and refer to different revisions.
 				//
-				// NOTE: Do not replace this with semver.Max. Despite the name,
-				// semver.Max *also* canonicalizes its arguments, which uses
-				// semver.Canonical instead of module.CanonicalVersion and thereby
-				// strips our "+incompatible" suffix.
-				if semver.Compare(info2.Version, v) < 0 {
-					info2.Version = v
-				}
+				// The tag is otherwise valid for the module, so we can at least use it as
+				// the base of an unambiguous pseudo-version.
+				//
+				// If multiple tags match, tagToVersion will canonicalize them to the same
+				// base version.
+				pseudoBase = v
+			}
+		}
+		// Save the highest non-retracted canonical tag for the revision.
+		// If we don't find a better match, we'll use it as the canonical version.
+		if tagIsCanonical && semver.Compare(highestCanonical, v) < 0 && !isRetracted(v) {
+			if module.MatchPathMajor(v, r.pathMajor) || canUseIncompatible() {
+				highestCanonical = v
 			}
-		} else if v != "" && semver.Compare(v, statVers) == 0 {
-			// The user explicitly requested something equivalent to this tag. We
-			// can't use the version from the tag directly: since the tag is not
-			// canonical, it could be ambiguous. For example, tags v0.0.1+a and
-			// v0.0.1+b might both exist and refer to different revisions.
-			//
-			// The tag is otherwise valid for the module, so we can at least use it as
-			// the base of an unambiguous pseudo-version.
-			//
-			// If multiple tags match, tagToVersion will canonicalize them to the same
-			// base version.
-			pseudoBase = v
 		}
 	}
 
-	// If we found any canonical tag for the revision, return it.
+	// If we found a valid canonical tag for the revision, return it.
 	// Even if we found a good pseudo-version base, a canonical version is better.
-	if info2.Version != "" {
-		return checkGoMod()
+	if highestCanonical != "" {
+		return checkCanonical(highestCanonical)
 	}
 
 	// Find the highest tagged version in the revision's history, subject to
@@ -528,11 +527,10 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
 				tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor("v0"))
 			}
 		}
-		pseudoBase, _ = tagToVersion(tag) // empty if the tag is invalid
+		pseudoBase, _ = tagToVersion(tag)
 	}
 
-	info2.Version = module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short)
-	return checkGoMod()
+	return checkCanonical(module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short))
 }
 
 // validatePseudoVersion checks that version has a major version compatible with
@@ -556,10 +554,6 @@ func (r *codeRepo) validatePseudoVersion(info *codehost.RevInfo, version string)
 		}
 	}()
 
-	if err := module.CheckPathMajor(version, r.pathMajor); err != nil {
-		return err
-	}
-
 	rev, err := module.PseudoVersionRev(version)
 	if err != nil {
 		return err