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

Jump to bottom

cmd/go: refactor mvsReqs.Max for better readability #39042

Closed
robpike opened this issue May 13, 2020 · 4 comments
Closed

cmd/go: refactor mvsReqs.Max for better readability #39042

robpike opened this issue May 13, 2020 · 4 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@robpike
Copy link
Contributor

robpike commented May 13, 2020

At head (go version devel +e90b0ce68b Sat May 2 20:22:19 2020 +0000 darwin/amd64) I see this function in src/go/cmd/go/internal/modload/mvs.go:

func (*mvsReqs) Max(v1, v2 string) string {
	if v1 != "" && semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}

I believe that "return v1" is wrong if v1 is empty, and the code should be something like:

func (*mvsReqs) Max(v1, v2 string) string {
	if v1 == "" || semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}

but I am not confident of the semantics this function requires. Moreover, since Compare protects against bad parses, it's possible no checking is required:

func (*mvsReqs) Max(v1, v2 string) string {
	if semver.Compare(v1, v2) == -1 {
		return v2
	}
	return v1
}
@myitcv myitcv added GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 13, 2020
@myitcv
Copy link
Member

myitcv commented May 13, 2020

cc @bcmills @jayconrod

@jayconrod
Copy link
Contributor

I think this is working as intended, but the code could be refactored for better readability.

mvsReqs.Max is run as part of MVS. At the point it's called, all go.mod files have been read, and all versions have been canonicalized. The only module with the version "" should be the main module, which must be chosen over any other version required by its transitive dependencies. For this reason, "" compares higher than any other string.

We should refactor this for better readability. We should explain why semver.Max is not used instead.

@jayconrod jayconrod changed the title cmd/go: bug in mvsReqs.Max? cmd/go: refactor mvsReqs.Max for better readability May 13, 2020
@jayconrod jayconrod added this to the Backlog milestone May 13, 2020
@bcmills
Copy link
Contributor

bcmills commented May 13, 2020 •
edited

We should explain why semver.Max is not used instead.

#32700 is another reason we avoid semver.Max.

@gopherbot
Copy link

Change https://golang.org/cl/234003 mentions this issue: cmd/go/internal/modload: document mvsReqs.Max

@golang golang locked and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@jayconrod @myitcv @robpike @bcmills @gopherbot