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/modfile, cmd/go: lines in exclude block don't use semver for sorting #60028

Closed
dmitshur opened this issue May 6, 2023 · 3 comments
Closed
Assignees
Labels
GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented May 6, 2023

I noticed that go mod tidy (in Go 1.20.4 and at tip) sorts lines in exclude blocks lexicographically. For some sequences of module versions it will cause a diff like this:

 exclude (
 	// ... some explanation for why this exclude is needed ...
 	example.com/module v0.1.0
+	example.com/module v1.10.0
+	example.com/module v1.11.0
+	example.com/module v1.12.0
 	example.com/module v1.3.0
 	example.com/module v1.5.0
 	example.com/module v1.6.0
 	example.com/module v1.6.1
 	example.com/module v1.7.0
-	example.com/module v1.10.0
-	example.com/module v1.11.0
-	example.com/module v1.12.0
 )

It's certainly not at all a big deal especially since exclude directives are rare, but it might still be slightly unexpected for it not to maintain the consistent semantic version sorting.

I took a quick look at what it might take to make it not do that. It turned out to be working as intended: all blocks (other than the newer retract one) sort lines via a simple lexicographic sort. It's just that exclude blocks are a rare situation where multiple versions of the same module may happen in practice and run into this edge case.

The go command currently gates some accepted changes to go mod tidy behavior on the Go language version specified in go.mod (e.g., tidyGoModSumVersionV = "v1.21") to avoid introducing unnecessary churn. (I think the concern of invalidating checksums doesn't apply to tidy behavior changes?) It'd also be possible to check what percentage of go.mod files in a corpus of public modules would be affected, though that misses private modules where exclude might be used more.

So fixing this or any other potential formatting change deemed worthwhile can also begin to apply upon reaching a future Go version. It probably belongs in package modfile since there's no intended change to meaning.

A formatting change that is gated behind Go version (current + 2) means by the time it happens all supported Go toolchains would handle it consistently. But since go mod tidy in Go 1.N refuses to tidy a go.mod with Go 1.(N+1), maybe just current + 1 would be sufficient?

I've sketched a possible implementation of this to see what it might look like, and perhaps it's not worth the implementation complexity (that needs to stay around) for such a small improvement to one block type. Maybe it's better to wait and solve this in bulk with some other minor changes, or when some bigger change happens and makes this obsolete. I'll let package owners chime in on if you think it's worth doing anything about this or just leaving it.

CC @bcmills, @rsc.

@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels May 6, 2023
@dmitshur dmitshur added this to the Backlog milestone May 6, 2023
@gopherbot
Copy link

Change https://go.dev/cl/492990 mentions this issue: modfile: use semantic sort for exclude blocks

@bcmills bcmills added the modules label May 8, 2023
@bcmills
Copy link
Contributor

bcmills commented May 10, 2023

Talked with @rsc this afternoon, and we agreed to treat this as a bug-fix, with the fix gated on the go version in the file.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label May 10, 2023
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 10, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 May 10, 2023
@dmitshur dmitshur self-assigned this May 10, 2023
@gopherbot
Copy link

Change https://go.dev/cl/494578 mentions this issue: cmd: update vendored golang.org/x/mod

gopherbot pushed a commit to golang/mod that referenced this issue May 17, 2023
For golang/go#60028.

Change-Id: I4c7a726a900fc7c4b34816eba5cfd0361c45315f
Reviewed-on: https://go-review.googlesource.com/c/mod/+/492990
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@dmitshur dmitshur changed the title x/mod/modfile: lines in exclude block don't use semver for sorting x/mod/modfile, cmd/go: lines in exclude block don't use semver for sorting May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants