(Rewritten by @adonovan after prototyping.)

This proposal is the second of a sequence of two; #73605 is the first.

Background: gopls includes a number of analyzers in the Go analysis framework whose purpose is to modernize Go code to take advantage of newer features of the language and library. For example, the modernize suite, and the //go:fix inline analyzer. We would like to make these available through go fix, in much the same way that analyzers for reporting diagnostics are available through go vet.

Proposal: We propose to eliminate cmd/fix, and to restructure go fix to follow the approach of go vet, executing the same "vet tool", cmd/vet by default. The only differences between them would be (a) that go vet reports diagnostics whereas go fix applies their fixes and (b) that they invoke different subsets of analyzers.

The proposed new CLI is shown below. Here is a brief summary of changes:

  1. go fix now behaves almost exactly like go vet, and uses the same -vettool extension mechanism based on the Go analysis framework. The only difference is that it runs the vet tool (default cmd/vet) with the -fix flag, to apply fixes, and with GOVET=fix, to allow the vet tool to select a different analyzer suite. (This happens before the call to unitchecker.Main, which does flag parsing, hence the unfortunate need for an environment variable.) The go vet and go fix both dynamically query the vet tool for the set of analyzers and thus the dynamic set of feature flags.
  2. go fix and go vet both accept a -diff flag, which they pass through to the vet tool. The go command reports an error on go vet -diff without -fix.
  3. The obsolete go fix -fix=name,... flag is now a no-op, and results in a warning.
  4. The unitchecker CLI now accepts the -fix and -diff flags.
  5. We will remove from go fix the set of legacy cmd/fix passes, and their flags each of the form -name. Though this is technically a breaking change, it seems unlikely that anyone would be explicitly relying on specific passes that have no value at all. (Alternatively, we could add a specific check for these names and report a specific error.)
  6. The modernizer suite and the //go:fix inline analyzer will be added to the go fix suite. See these related proposals for those subtasks:
  7. https://github.com/golang/go/issues/75266
  8. https://github.com/golang/go/issues/75267
  9. https://github.com/golang/go/issues/75227

See https://go.dev/cl/700795 for the go fix implementation, which can be submitted once this is approved.

$ go help fix
usage: go fix [build flags] [-vettool prog] [fix flags] [packages]

Fix runs the Go vet command (cmd/vet) on the named packages
and applies suggested fixes.

It supports these flags:

  -json
    instead of applying each fix, emit JSON output
  -diff
    instead of applying each fix, print the patch as a unified diff

Use the -vettool to specify an alternative checking tool.
See 'go doc vet' for details.

For more about specifying packages, see 'go help packages'.

For a list of checkers and their flags, see 'GOVET=fix go tool vet help'.
(The set of checkers may differ from those run by "go vet".)

For details of a specific checker such as 'hostport',
see 'GOVET=fix go tool vet help hostport'.

The build flags supported by go vet are those that control package resolution
and execution, such as -C, -n, -x, -v, -tags, and -toolexec.
For more about these flags, see 'go help build'.

See also: go fmt, go vet.

$ go help vet
usage: go vet [build flags] [-vettool prog] [vet flags] [packages]

Vet runs the Go vet command (cmd/vet) on the named packages
and reports diagnostics.

It supports these flags:

  -c int
        display offending line with this many lines of context (default -1)
  -json
        emit JSON output
  -fix
    instead of printing each diagnostic, apply its first fix (if any)
  -diff
    instead of applying each fix, print the patch as a unified diff

The -vettool=prog flag selects a different analysis tool with
alternative or additional checks. For example, the 'shadow' analyzer
can be built and run using these commands:

  go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow@latest
  go vet -vettool=$(which shadow)

Alternative vet tools should be built atop golang.org/x/tools/go/analysis/unitchecker,
which handles the interaction with go vet.

For more about specifying packages, see 'go help packages'.
For a list of checkers and their flags, see 'go tool vet help'.
For details of a specific checker such as 'printf', see 'go tool vet help printf'.

The build flags supported by go vet are those that control package resolution
and execution, such as -C, -n, -x, -v, -tags, and -toolexec.
For more about these flags, see 'go help build'.

See also: go fmt, go fix.

Comment From: adonovan

Currently the go fix ~comment~ command is completely unrelated to the //go:fix inline annotation; it's just a grab-bag of syntax-only rewrites for very ancient features. I'm not sure whether anyone really uses it at all. At some point it might be worth rethinking it as a front-end for the go/analysis framework and the multichecker's -fix functionality.

Comment From: mvdan

(I assume you meant "go fix command" above). I have yet to encounter a recent Go user (say, who started using Go after 2015 or so) who is aware of go fix or what it does. I suspect at this point the command could be redesigned to do whatever you feel like is most useful, as long as it can be safely run on large codebases without having to worry about any subtle breakages.

Comment From: adonovan

I agree. A good step would be to add a telemetry counter to Go 1.25 for invocations of go fix so we can tell whether anyone is in fact still using it. @matloob @samthanawalla @jitsu-net

Comment From: adonovan

I propose that the go fix command-line interface should resemble go vet as much as possible, and the two tools should be thereafter documented as a pair, where vet is responsible for emitting diagnostics and fix is responsible for quietly applying suggested fixes. The two tools would be implemented by the same structure both implemented by the unitchecker driver, but effectively invoked in different modes.

The suites of analyzers in the two tools would of course differ (with some overlap). Whereas vet prizes accurate diagnosis of significant problems, fix would aim for fixes that never hurt the quality of the code, where quality is defined across many axes:

  • syntactic and semantic well-formedness (fixes don't break the build)
  • behavior preservation (including in edge cases such as nilness, NaNs, aliasing, cardinality of effects)
  • performance (fixes mustn't make code slower)
  • style (doesn't make the code ungainly, or lose commentary)

Our goal is that users should be able to run go fix on a large code base and merge the resulting CL with only cursory review, confident that the tools have not introduced bugs. I plan to write up a checklist for authors of fixers wanting to meet this bar.

Comment From: deefdragon

Would fix only be limited to issues that vet detects? or could/would/should its bounds extend beyond that?

Relatedly, if the latter, would that extension be for just common things determine worth fixing by the go team, or would it be extendable by 3rd parties IE - Tooling (lets say staticcheck/golangci-lint for common issues) - The user (Ie. replacing a deprecated code pattern across an org with a newer pattern) - A library developer to assist in migrating (IE. github.com/pkg/errors could extend fix to help users migrate to modern error conventions)

I suspect the ability to extend in general is not the best idea as it may increase complexity beyond what its worth, but it still feels worth bringing up to see if its a reasonable option.

Comment From: adonovan

Would fix only be limited to issues that vet detects? or could/would/should its bounds extend beyond that?

No, fix and vet would run an extended subset of vet's analyzers. Vet's analyzers would be chosen for diagnostic accuracy; not all diagnostics carry a fix. Fix's analyzers would be chosen for the safety and positive benefit of their fixes; not all fixes represent a mistake in the code that vet would be concerned with.

Relatedly, if the latter, would that extension be for just common things determine worth fixing by the go team, or would it be extendable by 3rd parties IE

  • Tooling (lets say staticcheck/golangci-lint for common issues)
  • The user (Ie. replacing a deprecated code pattern across an org with a newer pattern)
  • A library developer to assist in migrating (IE. github.com/pkg/errors could extend fix to help users migrate to modern error conventions)

I suspect the ability to extend in general is not the best idea as it may increase complexity beyond what its worth, but it still feels worth bringing up to see if its a reasonable option.

Good question. Earlier in the year we had been discussing the design of a framework for third-party analyzers that allowed them to be securely run within gopls. The promise of such a design is that merely by using the Foo module, you could benefit from all the analyzers written by the maintainers of the Foo module on how best to use their APIs, just as today users of fmt.Printf get algorithmic guidance from the authors of the fmt package. Also, you would be able to write "housekeeping" rules specific to your module that would automatically report problems in that module.

However, even ignoring the security challenge (which sadly seems to be very much the fashion in this new era of agents!), we have learned from our "modernizer" efforts that: (a) if analyzers sprinkle diagnostics over the code, users want a way to batch-apply them because they are distracting; and (b) if fixes are to be batch-applied, they had better be safe by the standards of a compiler writer---no half-measures.

Unfortunately, even in the modest case of our modernizers, it is very hard--even for people with a compiler background and experience in analysis refactoring tools--to write fixes that are truly safe. It is too easy to introduce subtle behavior changes that slip past the type checker and test coverage, and it seems inevitable that one of these innocent-looking fixes will one day cause a serious problem. So we are less enthusiastic than we were about opening up the floodgates so that any one of your dependencies' maintainers can inject fixes into your workflow--or merely sprinkle nuisance diagnostics into your editor.

The //go:fix inline workflow is inherently much safer, and given that module upgrades in Go should--unlike the norms in some other languages--never break compatibility (and thus demand a "repair" tool), I am more optimistic about it as a way to replace old code with new.

Comment From: adonovan

I think we should rename things as part of the process of adding to go fix: - The gofix Analyzer should become inline (to match the //go:fix inline directives). - The modernize Analyzer should be split into a bunch of separate Analyzers to follow the usual pattern. (They can remain in the existing modernize package and share helper functions.) There's then no need to use the -category flag;

Also, we should: - move gopls/internal/util/astutil.Equal into x/tools, and perhaps moreiters too. - move the memoization of which ast.Files have // generated comments (currently in the main modernize.run function) into the typeindex helper analyzer.

The biggest part of the work is still wrangling unitchecker to apply fixes.

Comment From: gopherbot

Change https://go.dev/cl/699956 mentions this issue: gopls/internal/analysis/modernize: split up

Comment From: gopherbot

Change https://go.dev/cl/699955 mentions this issue: gopls/internal/analysis/generated: amortize ast.IsGenerated

Comment From: gopherbot

Change https://go.dev/cl/700795 mentions this issue: cmd/go: unify "go fix" and "go vet"

Comment From: gopherbot

Change https://go.dev/cl/700896 mentions this issue: go/analysis/unitchecker: support -fix

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: gopherbot

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

Comment From: gopherbot

Change https://go.dev/cl/703995 mentions this issue: go/analysis/unitchecker: add cfg.WarnDiagnostics

Comment From: gopherbot

Change https://go.dev/cl/703896 mentions this issue: cmd: vendor x/tools@9fccddc

Comment From: aclements

go fix now behaves almost exactly like go vet, and uses the same -vettool extension mechanism based on the Go analysis framework. The only difference is that it runs the vet tool (default cmd/vet) with the -fix flag, to apply fixes, and with GOVET=fix, to allow the vet tool to select a different analyzer suite.

We discussed this in proposal review for a while and came to the conclusion that we should just have separate binaries for vet and fix. While there is overlap in the analyzers between vet and fix, there seem to be more drawbacks to combining them into a single binary than upsides.

The main downside of a combined binary is the need for a new out-of-band protocol to communicate whether the tool should act as "vet" or "fix". Any such protocol, whether it's an environment variable as proposed or something else, is going to interfere with custom vettools or any unitchecker that doesn't have special code to check the mode.

Also, using a combined will actually result in a larger Go distribution than two separate binaries. We need to ship the vet binary in the distribution, but if fix is a separate binary, we can build that on demand (#71867). Thus, it's actually an advantage to have fewer analyzers baked in to the vet binary.

We will remove from go fix the set of legacy cmd/fix passes, and their flags each of the form -name. Though this is technically a breaking change, it seems unlikely that anyone would be explicitly relying on specific passes that have no value at all.

I'm a bit confused by this. As far as I can tell, neither go fix nor go tool fix accepts flags of the form -name today. 

$ go fix -buildtag
flag provided but not defined: -buildtag
usage: go fix [-fix list] [packages]
Run 'go help fix' for details.
$ go tool fix -buildtag
flag provided but not defined: -buildtag
usage: go tool fix [-diff] [-r fixname,...] [-force fixname,...] [path ...]
  -diff
        display diffs instead of rewriting files
  -force string
        force these fixes to run even if the code looks updated
  -go string
        go language version for files
  -r string
        restrict the rewrites to this comma-separated list

Available rewrites are:

buildtag
    Remove +build comments from modules using Go 1.18 or later

...

You mentioned that the go fix -fix=... flag would print a warning and otherwise do nothing.

I think for go tool fix we would drop -force, -r, and presumably -go. I think that's fine. The bigger change is that I think go tool fix would no longer accept source paths. Is that right?

Comment From: gopherbot

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