(This proposal is the first of a sequence of two; https://github.com/golang/go/issues/71859 is the second.)

Background: The go fix subcommand was an important tool during the early days of Go to help early adopters deal with rapid changes, not always compatible, in the evolving language. It currently contains these features:

  • buildtag: remove old-style +build tags.
  • cftype: Fixes initializers and casts of C.*Ref and JNI types
  • context: Change imports of golang.org/x/net/context to context
  • egl: Fixes initializers of EGLDisplay
  • eglconf Fixes initializers of EGLConfig
  • gotypes: Change imports of golang.org/x/tools/go/{exact,types} to go/{constant,types}
  • jni: Fixes initializers of JNI's jobject and subtypes
  • netipv6zone: Adapt element key to IPAddr, UDPAddr or TCPAddr composite literals
  • printerconfigFix: Add element keys to Config composite literals

I think all of these have outlived their usefulness, with the possible exceptions of the first, which could be converted to the analysis framework.

This paves the way for a future proposal to add new functionality to go fix to make it invoke the analysis framework (as vet does), but instead of reporting diagnostics, it would batch-apply safe fixes such as those produced by these analyzers:

  • gofix (which probably wants to be renamed to inline)
  • assign
  • hostport
  • timeformat
  • maprange
  • modernize
  • simplifycompositelit
  • simplifyrange
  • simplifyslice
  • unusedparams

Proposal: We propose to remove all functionality from go fix, so that it simply prints an error message.

Related: - https://github.com/golang/go/issues/70815 - https://github.com/golang/go/issues/32816 - https://github.com/golang/go/issues/71859

Comment From: gabyhelp

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: seankhliao

I'm not sure it's a good idea to make it error? It seems to be part of people's workflows, especially when combined with other commands like go mod tidy.

Makefiles: https://github.com/search?q=language%3AMakefile+%2F%5E%5Ctgo+fix%2F&type=code Shell: https://github.com/search?q=language%3Ashell+%2F%5E%5Cs*go+fix%2F&type=code

Comment From: rsc

Indeed, even an empty "go fix" should not error out. The buildtag fix should be kept in some form, and the context and gotypes fixes should be kept in the form of //go:fix annotations in the old packages.

Comment From: aclements

It seems like it would be easy to clear out fix without breaking compatibility and still set ourselves on a path to rejuvenate it. The go fix command just takes a list of packages and, optionally, a -fix list flag. We want to keep at least buildtag and printerconfigFix. It's probably easy to keep the -fix flag working and just make the other fixers no-ops. (I'm not worried about keeping cmd/fix itself compatible, which is good because it takes a list of file paths, not packages, which would be far more annoying to keep working.)

Comment From: aclements

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

Comment From: cherrymui

It seems okay to remove features that are obsolete, namely, everything except buildtag and printerconfigFix.

It would be nice if we can keep the command line interface's compatibility, by ignoring the obsolete flags. @adonovan will try to implement it and see if it is doable.

Comment From: meling

  • gofix (which probably wants to be renamed to inline)

I read the //go:fix inline docs (link from #71859 (comment)).

I'm a bit confused about naming it inline. The examples in the docs mainly promote it as a way to mark functions and constants as deprecated. Seems to me that a better name would be //go:fix replace. At least I found it confusing to conflate inlining and deprecation.

Moreover, I later discovered a more up-to-date version of the above link, and just wanted to point out that the preceding comment about fixes en masse doesn't match what the command does (it has been like this for a while).

You can use this (officially unsupported) command to apply gofix fixes en masse:

$ go run golang.org/x/tools/internal/gofix/cmd/gofix@latest -test ./...

What may be adding to the confusion is whether or not the above command will actually fix things based on analysers (I think it did so before) or just fix //go:fix labeled functions/commands in some way, which the linked doc seems to indicate.

(I apologize if I missed some previous discussion that would make things clearer, and I realize that this comment may be premature, as this is a work in progress.)

Comment From: aclements

@meling, could you file a new issue about the confusing go fix docs? In particular, //go:fix inline is not a way to mark functions as deprecated, so it sounds like the docs need improving.

Comment From: aclements

Based on the discussion above, this proposal seems like a likely accept. — aclements for the proposal review group

converging. => check? [adonovan, austin]

Comment From: aclements

Looking back, I'm not sure we totally agreed on which fixes need to be kept working. buildtag seems pretty clear. @cherrymui mentioned also keeping printerconfigFix, though I'm not sure why. @rsc mentioned context and gotypes, but, if I understood correctly, that we should replicate the functionality of those fixers as //go:fix inline annotations and then it's okay to make them no-ops in go fix.

Comment From: aclements

@adonovan looked into printerconfigFix and it's incredibly specific; it only operates on printer.Config. I don't think we need to keep that.

It also sounds like we don't need to do anything about gotypes because the old x package hasn't been updated in many versions and is entirely obsolete at this point.

golang.org/x/net/context is still widely imported, so we should do the transition to //go:fix for that.

Comment From: gopherbot

Change https://go.dev/cl/691075 mentions this issue: context: add //go:fix inline annotation to Context

Comment From: aclements

I see I messed up posting the proposal details. Here's what I meant to post:

The proposal is to make all of the current go fix fixes no-ops except for buildtag. go fix will continue to accept them in the -fix list, but will simply ignore them. As part of this, the functionality of the existing context fix will be replicated as //go:fix inline annotations in [golang.org/x/net/context](http://golang.org/x/net/context).

This means the following fixes will become no-ops: cftype, context, egl, eglconf, gotypes, jni, netipv6zone, and printerconfigFix.

Comment From: aclements

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

The proposal is to make all of the current go fix fixes no-ops except for buildtag. go fix will continue to accept them in the -fix list, but will simply ignore them. As part of this, the functionality of the existing context fix will be replicated as //go:fix inline annotations in golang.org/x/net/context.

This means the following fixes will become no-ops: cftype, context, egl, eglconf, gotypes, jni, netipv6zone, and printerconfigFix.

Comment From: gopherbot

Change https://go.dev/cl/694415 mentions this issue: go/analysis/passes/buildtag: suggest fix to remove +build lines

Comment From: adonovan

  • https://go.dev/cl/691075 annotates net/context, obviating the context fixer.
  • https://go.dev/cl/694415 adds removal of redundant +build tags into go/analysis

After that, we can remove all legacy fixers without exception.

Comment From: gopherbot

Change https://go.dev/cl/695855 mentions this issue: cmd/fix: remove all functionality except for buildtag

Comment From: aclements

After that, we can remove all legacy fixers without exception.

Just to clarify, you mean that go fix -fix buildtag will continue to work, but it will be backed by go/analysis/passes/buildtag?

Comment From: adonovan

Just to clarify, you mean that go fix -fix buildtag will continue to work, but it will be backed by go/analysis/passes/buildtag?

The -fix=... flag will have no effect other than to print a warning: the -fix=... flag is obsolete and has no effect. If the argument includes buildtag, then go fix will print an additional warning: to enable the buildtag check, use -buildtag.

The default suite of go fix will include go/analysis/passes/buildtag, so regardless of the value of the -fix flag, buildtag will be run (unless -buildtag=false is specified).

Comment From: gopherbot

Change https://go.dev/cl/706758 mentions this issue: cmd/fix: remove all functionality