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: separate default GODEBUGs from go language version #65573

Closed
rsc opened this issue Feb 7, 2024 · 31 comments
Closed

cmd/go: separate default GODEBUGs from go language version #65573

rsc opened this issue Feb 7, 2024 · 31 comments

Comments

@rsc
Copy link
Contributor

rsc commented Feb 7, 2024

Go 1.21 introduced a formalization of how we handle compatible-but-breaking changes, defining configuration knobs called GODEBUG settings that let users control whether or when these changes happen in their specific programs. The current docs are https://go.dev/doc/godebug, and the proposal was #56986. The default settings are taken from the “go” version line in the go.mod file for the main package, and main package Go source files can override with //go:debug key=value lines.

Go 1.21 also introduced additional forward compatibility rules, among them that the “go” line in any module must be the max of all the “go” lines in its dependencies. Among other things, this means that when deciding whether the current toolchain is new enough to build a module at all, only the top-level “go” line needs to be consulted. This is documented at https://go.dev/doc/toolchain, and the proposal was #57001.

These two interact in an unfortunate way: a maintenance version M of a particular program wants to lock in the GODEBUG semantics from the release they started with, which they can do by setting the “go” line even as they update to newer toolchains. But when they update to newer versions of dependencies, if those dependencies have updated to newer “go” lines, that will force using a newer “go” line in the top-level module, which changes the default GODEBUG settings. If this happens for one dependency, it can be forked and replaced. For a module with a large dependency tree, it may well happen with many dependencies, at which point fork+replace does not scale.

Another possible workaround is to add //go:debug lines to every package main in the module. This is reasonable except that there is no way to predict which ones to add. The point is to say “be like Go 1.X” for some X, but when the module updates to a “go 1.(X+1)” line, the GODEBUG settings from Go 1.(X+1) become relevant, and those can't be predicted when cutting the maintenance branch.

I propose two changes to allow separation of the default GODEBUGs from the “go” line in the go.mod.

First, add a new meta-setting “default=go1.X” that means “set everything the way Go 1.X was”, the same as the “go 1.X” line means. This would let you have a module that says “go 1.23” but a main program that says //go:debug default=go1.21. In terms of this new meta-setting, the “go 1.X” line in go.mod essentially implies //go:debug default=go1.X in each main package. The meta-setting can be used in $GODEBUG as well, of course.

Second, add a new “godebug” line to go.mod, respected only in the current module (like toolchain, replace, and exclude are only respected in the current module), as well as to go.work (again like those). It takes a single k=v argument, same as a //go:debug source line, but it applies to all the package main in the module. So you can write

go 1.23
godebug default=go1.21

Setting multiple GODEBUGs is done with multiple debug lines (just as it is done with multiple //go:debug source lines), and like everything else in the go.mod or go.work file, godebug lines can be factored:

go 1.23

godebug (
	default=go1.21
	panicnil=0
)

The precedence order would be: run-time $GODEBUG wins, then //go:debug lines in package main, then go.work or go.mod (go.work when using a workspace, go.mod otherwise).

If this proposal is accepted for Go 1.23, we may want to consider whether to backport either just the first step or both steps to Go 1.22 and possibly Go 1.21. Go 1.22 in particular has a lot of new GODEBUG settings. That's good: we are paying close attention to compatibility. But it's also a bit difficult to cope with in modules that need a “go 1.22” line due to updated dependencies but also want Go 1.21 semantics from their GODEBUG settings. Being able to write //go:debug default=go1.21 could be worth backporting there. Go 1.21 has fewer relevant settings: basically just panicnil. But our typical rule is that if a fix is deemed critical to backport, we backport it to all supported releases.

Note that backporting the godebug directive in go.mod does not cause any compatibility concerns for older go commands seeing these in dependencies: unknown directives are always ignored in dependency go.mods, for precisely this reason.

@rsc rsc added this to the Proposal milestone Feb 7, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Feb 7, 2024
@bcmills bcmills added the GoCommand cmd/go label Feb 7, 2024
@liggitt
Copy link
Contributor

liggitt commented Feb 7, 2024

Thanks for opening this. Giving the main module the final say over compatibility switches in a usable way makes a lot of sense to me (just like it has the final say over dependency versions via replace, toolchain version via toolchain, etc).

From my perspective (maintaining multiple Kubernetes release branches and patch releases with a large dependency tree), this proposal would restore the usability of the Go 1.21 backwards-compatibility improvements, and would remove a lot of uncertainty and friction from the approach we're recommending (for components to update toolchain minor versions and do security-related dependency bumps as needed on maintenance branches).

@rittneje
Copy link

rittneje commented Feb 7, 2024

What about unit tests? Will they also apply the godebug settings from their go.mod? Also, I'm not sure what "//go:debug lines in package main" means in the context of a unit test.

Also, I think the merging rules can get tricky here. For example I say godebug panicnil=0 in go.mod, and then I do GODEBUG=default=go1.21 at runtime, which panicnil setting wins?

Also, should the default setting include the patch version? Or is the assumption a patch release will never introduce a new godebug setting?

@liggitt
Copy link
Contributor

liggitt commented Feb 8, 2024

What about unit tests?

My read is that they would behave the same as they do for the existing defaulting based on the go.mod go directive (or main package go:debug directives for main package unit tests).

Also, I think the merging rules can get tricky here. For example I say godebug panicnil=0 in go.mod, and then I do GODEBUG=default=go1.21 at runtime, which panicnil setting wins?

I think this would be equivalent to expanding the default meta-setting to all the specific settings at that location, so in your example, the $GODEBUG env would take precedence over go.mod, just as it would if it explicitly set $GODEBUG=panicnil=1. That ensures the current precedence order continues to be respected. Documenting that is important, but seems straightforward.

@prattmic
Copy link
Member

prattmic commented Feb 8, 2024

I agree that this is a pain point and don’t have objections to this proposal. That said, it seems to me that the premise of this issue generalizes to any change tied to the go version in go.mod.

For example, suppose I am keeping go 1.21 because my program depends on old for loop semantics (or perhaps I just want to be really careful and closely vet my loops before upgrading). Now one of my dependencies decides that they want to use range over integer loops and so they bump to go 1.22.

Unless I am forgetting something, my options are:

  • Don’t upgrade the dependency
  • Complete my for loop semantic migration/vetting immediately
  • Or, add //go:build go1.21 to every file in my project.

That last option is technically a workaround, but it is even more painful than the “//go:debug in every package main” workaround listed in the proposal.

I don’t think that for loop semantics specifically have enough pain to justify a specific fix, I’m just wondering if we should think about approaches to address the more general problem.

@rsc
Copy link
Contributor Author

rsc commented Feb 8, 2024

@prattmic raises an important point. The question is whether the more general problem of backdating the language semantics is or will be an actual problem. We have made many public commitments about the loop semantics change being an one-time exception justified by a severe cost-benefit imbalance in favor of benefits, not a prelude to more invasive language changes. If we introduced a line that controlled the language semantics separate from the go line, that would make the more invasive language changes that much more tempting. I'm inclined to focus this on godebug specifically to avoid that temptation, like Odysseus tying himself to the mast.

@rsc rsc moved this from Incoming to Active in Proposals Feb 9, 2024
@rsc
Copy link
Contributor Author

rsc commented Feb 9, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@hherman1
Copy link

If a library was written against go 1.23, so it does not clone loop variables, but go debug is set to go 1.21, will it introduce bugs in calls to the library code?

@thepudds
Copy link
Contributor

Hi @hherman1, I think part of the answer might be that there is no GODEBUG setting for the new loop variable semantics, and those semantics are instead controlled for example by the Go language version specified on the go line of the go.mod of the library code (or for example by //go:build lines on a per-file basis if an author needed something more fine grained)?

So as far as I understand this proposal, I think the answer would be no, it would not introduce bugs in your scenario, and something like godebug default=go1.21 in a go.mod influences final GODEBUG settings, but does not change the Go language version.

@rsc
Copy link
Contributor Author

rsc commented Feb 14, 2024

Yes, @thepudds is right (as usual 😄) about the loopvars: that's not godebug-controlled.

@rsc
Copy link
Contributor Author

rsc commented Feb 14, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is in the top comment: #65573 (comment).

@hherman1
Copy link

🤔 I guess it’s expected that libraries written against go 1.N should work with godebug settings from 1.N+1 then? If so my concerns are addressed

@jingxu97
Copy link

With the new knobs in place, I am wondering whether there would be a general policy/guidance on how to set the versions in different places what could be applied to most of the projects so that owners can easily follow, or it has to be analyzed case by case by project owners in order to set it correctly?

@rittneje
Copy link

I think adding godebug to go.mod is a great change. When you have many applications in a module together, making sure they all have the same //go:debug directives simply isn't feasible, so that makes life much simpler.

However, I don't think default is necessary, and could cause more harm than benefit. In particular, it makes it easy for people to simply park themselves at older configurations without fully understanding what that means. And it is unclear how that would work as certain godebug settings become obsolete. For example, when x509sha1 is removed, what does default=go1.18 mean? Maybe the right approach is for default=go1.X to be a kind of macro that gets expanded/rewritten in the go.mod file, similar to how @latest gets rewritten.

@liggitt
Copy link
Contributor

liggitt commented Feb 15, 2024

it makes it easy for people to simply park themselves at older configurations without fully understanding what that means

Right now, we cannot park a maintenance branch at an older configuration even if we fully understand what it means. That's a problem.

And it is unclear how that would work as certain godebug settings become obsolete.

This is a good thing to decide and clarify. I see three possibilities if a godebug setting is removed (after the promised 2+ years) and is still requested via default=1.x:

  1. Ignore. This is similar to what the go directive does... go-directive-based defaulting of compatibility switches no longer has any effect on removed options.
  2. Error. This is similar to what //go:debug directives do with directives it cannot honor.
  3. Warn. This is sort of in the middle... making it visible that one or more of the godebugs associates with the requested minor are no longer supported seems good, but continuing to default the ones that still exist also seems good.

Maybe the right approach is for default=go1.X to be a kind of macro that gets expanded/rewritten in the go.mod file, similar to how @latest gets rewritten.

Expansion doesn't seem like what we want... it loses intent and the expanded list becomes incomplete/incorrect in the future when a new minor adds new compatibility switches.

@ChrisHines
Copy link
Contributor

And it is unclear how that would work as certain godebug settings become obsolete.

I would vote for a build error in order to force a maintenance activity to update the godebug setting to be compatible with the new version of Go.

@rittneje
Copy link

Right now, we cannot park a maintenance branch at an older configuration even if we fully understand what it means. That's a problem.

Why can't you just add the requisite //go:debug settings to main?

Expansion doesn't seem like what we want... it loses intent and the expanded list becomes incomplete/incorrect in the future when a new minor adds new compatibility switches.

I don't think it would be incomplete or incorrect. For example, let's say you are upgrading from 1.21 to 1.22. I'm thinking you could just run some command of the form "inject all godebug directives into go.mod to emulate 1.21 in 1.22". Then later if your upgrade from 1.22 to 1.23, you'd run the command "inject all godebug directives into go.mod to emulate 1.22 in 1.23". The key here would be that if a particular godebug setting already appears in the go.mod, then the injection should leave it as-is. In other words, the go.mod file just explicitly says what the end result of this proposal would have been.

@liggitt
Copy link
Contributor

liggitt commented Feb 16, 2024

Why can't you just add the requisite //go:debug settings to main?

Doing that for multiple (sometimes dozens-to-hundreds) of main packages on multiple release branches and revisiting/updating that on every go minor version bump on the release branch is a lot of churn for a branch that should be seeing minimal changes.

Expansion doesn't seem like what we want... it loses intent and the expanded list becomes incomplete/incorrect in the future when a new minor adds new compatibility switches.

I don't think it would be incomplete or incorrect.

  1. My intent is "use compatibility settings consistent with go1.20", so I would set godebug default=1.20
  2. I build with go 1.21, and that gets expanded/replaced to use compatibility settings go 1.21 knows about.

The intent is lost and future go versions would not honor it unless I took action to reassert that intent. That seems problematic and accident-prone.

@rittneje
Copy link

rittneje commented Feb 16, 2024

Doing that for multiple (sometimes dozens-to-hundreds) of main packages on multiple release branches and revisiting/updating that on every go minor version bump on the release branch is a lot of churn for a branch that should be seeing minimal changes.

Sure, I agree it's not a pleasant process, as I mentioned above. But I think that is addressed by moving the godebug directive to go.mod, regardless of whether default=go1.X is a thing.

My intent is "use compatibility settings consistent with go1.20", so I would set godebug default=1.20
I build with go 1.21, and that gets expanded/replaced to use compatibility settings go 1.21 knows about.

"The compatibility settings for 1.20 under 1.22" ought to be equivalent to "the compatibility settings for 1.20 under 1.21" + "the compatibility settings for 1.21 under 1.22", with the former taking precedence. So I don't understand what the concern here is, or how it would be error prone. Can you elaborate?

@liggitt
Copy link
Contributor

liggitt commented Feb 16, 2024

So I don't understand what the concern here is, or how it would be error prone. Can you elaborate?

There's multiple steps required:

  1. Needing to remember out of band what go version a particular release branch in a particular module meant to retain compatibility with (since there would no longer be a record in the go.mod file)
  2. Needing to reassert the desired compatibility on every go minor bump
  3. Needing to ensure the expansion precedence order does what you describe in cmd/go: separate default GODEBUGs from go language version #65573 (comment)

If any of those are forgotten or done incorrectly, the system defaults to not doing what I want. Requiring ongoing active modifications to keep compatibility seems burdensome.

It's also much harder to review a changing set of godebug lines to make sure they correctly match the desired defaults for the current builder version, than to establish a single default=go1.x line and make sure that doesn't change for the life of the release branch.

@rittneje
Copy link

It's also much harder to review a changing set of godebug lines to make sure they correctly match the desired defaults for the current builder version, than to establish a single default=go1.x line and make sure that doesn't change for the life of the release branch.

Fair, but it also seems undesirable that either I have to include both go 1.X and godebug default=go1.X whenever I create go.mod, or I have to remember to do it the first time I update the go directive. And I don't like that I can't just look at the source code to know what godebug settings it is actually using, especially because some may be security-related.

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 8, 2024
@rsc
Copy link
Contributor Author

rsc commented Mar 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is in the top comment: #65573 (comment).

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 15, 2024
@rsc rsc changed the title proposal: cmd/go: separate default GODEBUGs from go language version cmd/go: separate default GODEBUGs from go language version Mar 15, 2024
@rsc rsc modified the milestones: Proposal, Backlog Mar 15, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584300 mentions this issue: modfile: add API for godebug lines

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584476 mentions this issue: cmd/go: add support for godebug lines in go.mod and go.work

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584475 mentions this issue: vendor: pull in x/mod/modfile godebug changes

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/584218 mentions this issue: _content/ref/mod: document godebug directive

gopherbot pushed a commit to golang/mod that referenced this issue May 14, 2024
For golang/go#65573

Change-Id: I5c1be8833f70b0b5a7257bd5216fa6a89bd2665f
Reviewed-on: https://go-review.googlesource.com/c/mod/+/584300
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
gopherbot pushed a commit that referenced this issue May 14, 2024
	go get golang.org/x/mod@c0bdc7bd
	go mod tidy
	go mod vendor

Pulls in CL 584300.

For #65573.

Change-Id: Ia8ec86e2ee049b911fcf09d57f83972786b0470d
Reviewed-on: https://go-review.googlesource.com/c/go/+/584475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@liggitt
Copy link
Contributor

liggitt commented May 15, 2024

Excellent to see this land for go1.23, this will help a lot 🎉

Now that the implementation size / scope is well understood, I wondered if any thought had been given to this bit:

If this proposal is accepted for Go 1.23, we may want to consider whether to backport either just the first step or both steps to Go 1.22 and possibly Go 1.21. Go 1.22 in particular has a lot of new GODEBUG settings. That's good: we are paying close attention to compatibility. But it's also a bit difficult to cope with in modules that need a “go 1.22” line due to updated dependencies but also want Go 1.21 semantics from their GODEBUG settings. Being able to write //go:debug default=go1.21 could be worth backporting there. Go 1.21 has fewer relevant settings: basically just panicnil. But our typical rule is that if a fix is deemed critical to backport, we backport it to all supported releases.

We're seeing significant numbers of modules bumping go.mod to go 1.21+, many of which only release security fixes at HEAD. We're already building with Go 1.21 and 1.22, but it's becoming difficult to hold our maintenance branches go.mod directives to Go 1.20 or 1.21 for GODEBUG defaults. Adoption of go1.23 will likely not reach all of our maintenance branches until the end of 2024.

gopherbot pushed a commit to golang/website that referenced this issue May 15, 2024
For golang/go#65573.

Change-Id: Ie6b92586d203dd921d86084da84edc4706143fd8
Reviewed-on: https://go-review.googlesource.com/c/website/+/584218
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Sam Thanawalla <samthanawalla@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jun 5, 2024
@nathanperkins
Copy link

@liggitt @rsc should we open a separate issue to request backporting this to 1.21 and 1.22?

@nathanperkins
Copy link

nathanperkins commented Jun 17, 2024

Answer back from @rsc

It is too large a change to backport. Our backport policy is critical fixes only, and this is a major new feature with many parts. If it helped significantly, I might consider backporting just the default=go1.22 keywords, but they'd only be allowed in //go:build lines in Go source files, so you'd still have to add a godebug_default.go file to every main package that contained "//go:debug default=go1.22\n\npackage main\n". And at that point the script that emits that file can emit the full list instead, so it doesn't seem like an important enough win to make a minor exception to our backport policy.

Introducing new syntax in go.mod and go.work files in a point release is just too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests