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

cmd/go: do not modify the 'go.mod' or 'go.sum' files if the 'go' directive indicates a newer-than-supported version? #34262

Open
bcmills opened this issue Sep 12, 2019 · 14 comments
Labels
GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2019

@jayconrod notes in #34217 (comment):

One problem with this: adding go 1.14 to go.mod does not prevent old versions of the Go command from adding +incompatible versions. So a module could be broken unintentionally in a mixed environment.

There is a more general version of this problem: older versions of the Go toolchain may not know how to correctly interpret a go.mod file intended for a newer version, and thus could make arbitrarily wrong edits.

Probably we should have the go command error out instead of making changes if it thinks a go.mod file generated by a newer toolchain is incomplete or inaccurate.

@bcmills bcmills added GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 12, 2019
@bcmills bcmills added this to the Go1.14 milestone Sep 12, 2019
@mvdan
Copy link
Member

mvdan commented Sep 12, 2019

I agree that this is generally a good idea, but - will it work well in practice? I've seen a number of issues along the lines of "a command modified my go.mod when I didn't expect it to", or "mod tidy and test add and remove deps in circles". Would those bugs not be a problem, or is the plan to fix most/all of them for 1.14 too?

@bcmills
Copy link
Contributor Author

bcmills commented Sep 12, 2019

This change would not address most of those issues, because they occur independent of the version in the go directive. (Generally they are fixes for inconsistencies between go mod tidy and other operations.)

@bcmills
Copy link
Contributor Author

bcmills commented Sep 12, 2019

But yes: in general the plan is to remove bugs that introduce unnecessary/erroneous edits to the go.mod file as soon as they are reported.

@mvdan
Copy link
Member

mvdan commented Sep 12, 2019

This change would not address most of those issues

Apologies if I was unclear; I mean that those bugs could be much more annoying with this change in place. For example:

  1. I'm on 1.14, go.mod is on 1.13 as it needs to support that version
  2. I try to do an action (such as go test ./...) which, due to a bug, tries to add a couple of lines to go.mod
  3. I get an error, which I can't solve without bumping the version in go.mod

@bcmills
Copy link
Contributor Author

bcmills commented Sep 12, 2019

If you're on 1.14 and your go.mod file says go 1.13, you would not get an error anyway: the Go 1.14 toolchain will still know how to interpret go.mod files using go 1.13 semantics, so there is no need to flag the mismatch as a potential error.

@mvdan
Copy link
Member

mvdan commented Sep 12, 2019

The error I mean is that my version would try to make changes to go.mod due to a bug (such as adding unnecessary dependency lines), and so would refuse to make those changes and error.

I'll stop trying to explain this hypothetical scenario if I'm still not making sense :)

@bcmills
Copy link
Contributor Author

bcmills commented Sep 12, 2019

The proposal is to produce an error only for versions of the go.mod file that the go command in use does not know how to interpret.

So the problem you describe could only occur for a go.mod file starting at go 1.15, and only for cases in which the file would be correctly interpreted by the go 1.14 toolchain save for some bug that causes an erroneous edit.

Ideally, we we have all of those bugs fixed before go 1.14 is released anyway, so I'm not worried about that actually happening. 🙂

@bcmills
Copy link
Contributor Author

bcmills commented Sep 19, 2019

CC @ianlancetaylor

@gopherbot
Copy link

Change https://golang.org/cl/196519 mentions this issue: cmd/go: do not modify an existing go.mod file if its 'go' version is unsupported

@jayconrod
Copy link
Contributor

We should be careful here -- I want to think about this a bit more before moving ahead.

We already assume there may be incompatible changes to the go.mod language. If we add a new kind of statement, an old version of the go command will reject it in the main module and ignore it in other modules. The assumption now is that module, require, and go statements won't change and may be interpreted safely by old versions of the go command.

This change makes that stricter: old versions of the go command can't modify a main go.mod file with a greater go version, even if the file appears syntactically correct. Maybe that's okay: a greater go version indicates newer Go language features may be used, and we shouldn't expect older toolchains to build code marked that way.

But I think a few questions need to be resolved:

  1. Should the go command be even stricter and error out if it sees a go statement with a newer version in the main module regardless of whether it needs to make changes? This is probably the right thing to do if we need to change the interpretation of an existing statement. However, this should rarely be necessary, and we should avoid doing it if we can.
  2. Is this necessary for proposal: cmd/go: ignore +incompatible versions as of Go 1.14 #34217? That issues suggests that a go 1.14 statement would make +incompatible versions non-canonical, which is a change to the meaning of an existing statement. I'd like to understand whether we can solve the problems described in that issue without such a change (but let's discuss over there).
  3. Do we need this change outside of proposal: cmd/go: ignore +incompatible versions as of Go 1.14 #34217? Before I checked on how we handle go.mod syntax compatibility, I was thinking yes, but now I'm not sure. Maybe the go command can just print better error messages when it sees new go.mod syntax it doesn't recognize together with a go statement with a newer version.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 4, 2019

Should the go command be even stricter and error out if it sees a go statement with a newer version in the main module regardless of whether it needs to make changes?

No: it won't do that for the compile step, so it shouldn't do it for loading either.

Is this necessary for #34217?

Rendered moot by withdrawing #34217.

Do we need this change outside of #34217?

On CL 198319, you suggested that we avoid writing vendor/modules.txt if the go version in go.mod is unsupported, because it might not include information needed by higher Go versions.

@rsc has also brought up the possibility of adding redundant information to go.mod files in order to allow the module loader to prune out parts of the dependency graph in some situations. If that happens, the situation will be similar: older go tools may try to remove redundant information that newer versions would retain, even though both tools interpret that information in the same way.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 11, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 11, 2019
@jayconrod
Copy link
Contributor

I think there are two main issues here:

  1. We want to avoid automatic edits that an old version of the go command can't understand without an explicit opt-in.
  • So far, we haven't needed to make any backward incompatible changes to go.mod, go.sum, and vendor/modules.txt (but for example, forbidding or ignoring +incompatible versions in proposal: cmd/go: ignore +incompatible versions as of Go 1.14 #34217 would have been incompatible). We should definitely gate any automatic incompatible change on the go version.
  • We've made a few backward compatible changes (adding go statement, adding more information in vendor/modules.txt), and we have more proposed. I can't think of anything that would cause problems for automatic edits by old versions of the go command.
  1. We want to avoid two versions of the go command making conflicting changes.
  • This is came up in proposal: cmd/go: ignore +incompatible versions as of Go 1.14 #34217 (+incompatible versions being added and removed) and in cmd/go: automatically check and use vendored packages #33848 (go mod vendor will produce different vendor/modules.txt files in 1.13 and 1.14).
  • I think the general solution here is what we did with CL 201257. If we have this kind of conflict, a new version of the go command should emit either old or new syntax depending on the go statement in go.mod. An old version of the go command should still be able to edit go.mod and other files.
  • The consequence of this is that conflicts are still possible, but only if authors have opted into the new features by updating the go version in go.mod.
  • The drawback is that some new features will be less discoverable because developers need to opt in. This seems like a better tradeoff than preventing old versions of the go command from making edits though.

@bcmills
Copy link
Contributor Author

bcmills commented Mar 31, 2022

Now that -mod=readonly is the default (#40728), we no longer make nearly so many automatic edits, and as part of #46141 we made go mod tidy in particular refuse to tidy higher-than-supported versions.

It seems to me that the only remaining possible bad edits are due to explicit uses of -mod=mod (which should be rare) and explicit calls to go get (which may be somewhat more problematic).

@bcmills
Copy link
Contributor Author

bcmills commented Apr 5, 2022

Discussed with @rsc and @matloob; consensus was to reject edits from -mod=mod and go get if the version is mismatched.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 5, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 5, 2022
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

5 participants