cmd/fix: automate migrations for simple deprecations

[bcmills]

(I've mentioned this in passing on #32014 and #27248, but I don't see an actual proposal filed yet — so here it is!)

Over time, the author of a package may notice certain patterns in the usage of their API that they want to make simpler or clearer for users. When that happens, they may suggest that users update their own code to use the new API.

In many cases, the process of updating is nearly trivial, so it would be nice if some tool in the standard distribution could apply those trivial updates mechanically. The go fix subcommand was used for similar updates to the language itself, so perhaps it could be adapted to work for at least the simple cases.

The very simplest sort of update rewrites a call to some function or use of some variable or constant to instead call some other function or use some other variable, constant, or expression.

To draw some examples from the standard library:

• The comment for archive/tar.TypeRegA says to use TypeReg instead.
• The comments for archive/zip.FileHeader.{Compressed,Uncompressed}Size say to use the corresponding *64 fields instead.
• The comments for image.ZR and image.ZP say to use zero struct literals instead.
• The comment for go/importer.For says to use ForCompiler, and is implemented as a trivial call to ForCompiler with an additional argument.
• The comment for the io.SEEK_* constants says to use corresponding io.Seek* constants instead.
• The comments for syscall.StringByte{Slice,Ptr} say to use the corresponding Byte*FromString functions, and are implemented as short wrappers around those functions.

Outside the standard library, this pattern can also occur when a package makes a breaking change: if the v3 API can express all of the same capabilities as v2, then v2 can be implemented as a wrapper around v3 — and references to the v2 identifiers can be replaced with direct references to the underlying v3 identifiers.

---

See the current proposal details in #32816 (comment) below.

Previous draft (prior to #32816 (comment)):

I propose that we establish a consistent convention to mark these kinds of mechanical replacements, and improve cmd/fix to be able to apply them.

I'm not particularly tied to any given convention, but in the spirit of having something concrete to discuss, I propose two tokens, //go:fix-to and //go:forward, as follows:

• //go:fix-to EXPR on a constant or variable indicates that go fix should replace occurrences of that constant or variable with the expression EXPR. Free variables in the expression are interpreted as package names, and go fix may rewrite them to avoid collisions with non-packages.
• //go:fix-to .EXPR on struct field indicates that go fix should replace references to that field with the given selector expression (which may be a field access OR a method call) on the same value. Free variables are again interpreted as package names. If the expression is a method call, it replaces only reads of the field.
• //go:forward on an individual constant or variable indicates that go fix should replace the constant or variable with the expression from which it is initialized. If a constant is declared with a type but initialized from an untyped expression, the replacement is wrapped in a conversion to that type.
• //go:forward on a const or var block indicates that every identifier within the block should be replaced by the expression from which it is initialized.
• //go:forward on a method or function indicates that go fix should inline its body at the call site, renaming any local variables and labels as needed to avoid collisions. The function or method must have at most one return statement and no defer statements.
• //go:forward on a type alias indicates that go fix should replace references to the alias name with the (immediate) type it aliases.

All of these rewrites must be acyclic: the result of a (transitively applied) rewrite must not refer to the thing being rewritten.

Note that there is a prototype example-based refactoring tool for Go in golang.org/x/tools/cmd/eg that supported a broader set of refactoring rules. It might be useful as a starting point.

---

For the above examples, the resulting declarations might look like:

package image

// Deprecated: Use a literal image.Point{} instead.
var ZP = Point{} //go:forward

// Deprecated: Use a literal image.Rectangle{} instead.
var ZR = Rectangle{} //go:forward

package importer

// Deprecated: Use ForCompiler, which populates a FileSet
// with the positions of objects created by the importer.
//
//go:forward
func For(compiler string, lookup Lookup) types.Importer {
	return ForCompiler(token.NewFileSet(), compiler, lookup)
}

package io

// Deprecated: Use io.SeekStart, io.SeekCurrent, and io.SeekEnd.
const (
	SEEK_SET int = 0 //go:fix-to io.SeekStart
	SEEK_CUR int = 1 //go:fix-to io.SeekCur
	SEEK_END int = 2 //go:fix-to io.SeekEnd
)

package syscall

// Deprecated: Use ByteSliceFromString instead.
//
//go:forward
func StringByteSlice(s string) []byte {
	a, err := ByteSliceFromString(s)
	if err != nil {
		panic("string with NUL passed to syscall.ByteSliceFromString")
	}
	return a
}

[…]

---

This proposal does not address the archive/tar use-case: callers producing archives might be able to transparently migrate from TypeRegA to TypeReg, but it isn't obvious that callers consuming archives can safely do so.

Nor does this proposal does not address the archive/zip.FileHeader use-case: the deprecation there indicates a loss of precision, and the caller should probably be prompted to make related fixes in the surrounding code.

Both of those cases might be addressable with a deeper example-based refactoring tool, but (I claim) are too subtle to handle directly in go fix.

---

CC @marwan-at-work @thepudds @jayconrod @ianthehat @myitcv @cep21

[mvdan]

How would this deal with Go versions? For example, importer.ForCompiler was added in 1.12, so go fix shouldn't make the change if a module still supports Go 1.11.

Perhaps it can grab the oldest supported Go version from the optional line in go.mod, defaulting to a version extremely conservative (such as 1.0) if it's missing.

[bcmills]

@mvdan, good question. I'd say that it should not attempt to do anything special with Go versions. go fix is an explicit operation, and folks should look at the diffs (and run tests) after applying it to ensure that they're happy with the results.

For stuff in the standard library in particular, perhaps that implies that we shouldn't add //go:fix or //go:forward comments until two releases after the target of the fix, since that's our recommended support window.

[mvdan]

until two releases after the target of the fix, since that's our recommended support window.

I personally think this is a losing battle. Certain libraries support older Go versions, for their own reasons. You can say "just don't use go fix", but that seems unnecessary if go.mod can give us that information.

An alternative is to use "two Go releases ago" as the fallback version, instead of 1.0. Then a module can specify an older or newer version as needed.

[thepudds]

The "Automatic API Updates" section of the initial vgo blog series sketched out an alternative. It might be interesting to contrast this proposal with that earlier sketch.

One difference between that earlier sketch and the proposal here is that the earlier sketch I think had fewer comment variations, but I think relied on the v1 code being implemented as a shim around v2 (and perhaps it could in theory also support the reverse if v2 is implemented as a wrapper around v1).

Example snippet from that older sketch:

For example, suppose v1 of an API has functions EnableFoo and DisableFoo and v2 replaces 
the pair with a single SetFoo(enabled bool). After v2 is released, v1 can be implemented 
as a wrapper around v2:

package p // v1

import v2 "p/v2"

func EnableFoo() {
	//go:fix
	v2.SetFoo(true)
}

func DisableFoo() {
	//go:fix
	v2.SetFoo(false)
}

The fact that actual Go code is used and the earlier sketch might "use symbolic execution" might mean the earlier sketch is more expressive than this newer proposal. (edit: not sure if this is true; need to digest the new proposal more).

In contrast, this newer proposal also addresses deprecation use cases (without a need for a major version change).

[bcmills]

@mvdan, I suppose we could use the annotations in api/go*.txt to figure out when various identifiers were added, but note that the go directive is more of a maximum than a minimum. (For example, you can write a go 1.13 module to get access to the features added in 1.13, but use build tags to notch out those source files when building with 1.12 and earlier.)

[bcmills]

@thepudds, note that @rsc's sketch there is substantially more powerful than what I'm proposing here.

In particular, I'm not assuming that go fix can do any sort of control-flow analysis.

One of the examples there is:

func SetFoo(enabled bool) {
	if enabled {
		//go:fix
		v2.EnableFoo()
	} else {
		//go:fix
		v2.DisableFoo()
	}
}

but under this proposal that would be expressed as:

//go:forward
func SetFoo(enabled bool) {
	if enabled {
		v2.EnableFoo()
	} else {
		v2.DisableFoo()
	}
}

Of course, once go fix does the inlining, you can always apply other (more powerful!) analyzers to prune out the inlined paths that are provably unreachable.

[mvdan]

note that the go directive is more of a maximum than a minimum

Ah, noted.

[cep21]

A few questions:

• Does go:forward imply deprecated? Is the intent to place both comments in my code?
• Can you give an example of go:fix-to .EXPR
• The go:forward inline case seems the most magical of the bunch. Probably worth extra consideration.

Trying to get a scope of what you're proposing.

• Fixing net.Dialer.Cancel is way out of scope (?)
• Fixing net.Dialer.DualStack ... does that work somehow with .Expr?
• Can net.Dialer.Dial be replaced with net.Dialer.DialContext(context.TODO(), ...) ?
• There's probably a whole class of current and future context API migrations

[bcmills]

Does go:forward imply deprecated? Is the intent to place both comments in my code?

I would generally expect so, yes: go:forward implies that there is a simpler, more direct, or more robust alternative.

However, I think we still want both comments in general. //go:forward tells go fix that the fix should be automated, and // Deprecated: tells humans why the alternative is preferred.

Can you give an example of go:fix-to .EXPR

Sure: you could envision using it for (*net.URL).RawPath, since “In general, code should call EscapedPath instead of reading u.RawPath directly.”

package url

type URL struct {
	[…]
	// encoded path hint
	RawPath string //go:fix-to .EscapedPath()
	[…]
}

The go:forward inline case seems the most magical of the bunch. Probably worth extra consideration.

Agreed. It's the most magical, but also arguably the most useful. 🙂

I think it will require particular care around how we map := and return, since we have to make the return statement mesh with the caller code. Perhaps as a first past we should restrict it to functions whose single return statement is at the end of the function.

Fixing net.Dialer.Cancel is way out of scope (?)

Correct. I could envision some ways to use example-based refactoring to convert uses of the Cancel field to use DialContext, but they would require a much more sophisticated algorithm to match the call site against the refactoring pattern.

Fixing net.Dialer.DualStack ... does that work somehow with .Expr?

Sadly, no: .EXPR works for reads, but the thing we want to match there is the write operation.

I could imagine a //go:fix-write-to extension of this proposal that would handle it, though. Let _ stand for the field being fixed, and let //go:fix-write-to .EXPR be the rewrite rule for writes to a field. Then we can combine a //go:fix-write-to with a //go:forward to produce the intended refactoring:

package net

type Dialer {
	[…]
	DualStack bool //go:fix-write-to .setDualStack(_)
	[…]
}

//go:forward
func (d *Dialer) setDualStack(dualStack bool) {
	if !dualStack {
		d.FallbackDelay = -1
	}
}

Can net.Dialer.Dial be replaced with net.Dialer.DialContext(context.TODO(), ...) ?

Yes:

package net

//go:forward
func (d *Dialer) Dial(network, address string) (Conn, error) {
	return d.DialContext(context.TODO(), network, address)
}

There's probably a whole class of current and future context API migrations

Yep. Note that it's easy to introduce context.TODO() using this proposal, but converting those context.TODO() calls to use the actual context in the current scope is beyond its capability.

[cep21]

Sure: you could envision using it for (*net.URL).RawPath, since “In general, code should call EscapedPath instead of reading u.RawPath directly.”

Is go-fix intended for changes that aren't strictly backwards compatible?

[bcmills]

I think that's an open question. This proposal leaves that decision up to package authors: nothing stops them from applying a //go:fix-to or //go:forward that isn't strictly backward compatible, although I would expect its use for incompatible changes to be rare in practice.

[bcmills]

(And do note that a //go:forward inlining a function call, constant, or type alias is very unlikely to result in an incompatible change.)

[cep21]

• Should there be two classes of go-fix runs? The 100% backwards compatible ones and the "probably" compatible ones?
• Will there be any warning when fixes do things like point to a constant with a changed value?
• If I had reassigned image.ZR, does this show up when I eventually build my code or during the fix?

[bcmills]

Should there be two classes of go-fix runs? The 100% backwards compatible ones and the "probably" compatible ones?

Maybe, but since go fix is an explicit action I'm not sure it's necessary to distinguish them. (Both should fall under the category of “changes that the package author believes to be appropriate 100% of the time”.)

Note that the proposed //go:forward inlining should ~always be backward-compatible (it works unless either the caller has a very subtle dependency on the runtime.Callers depth, or some package modifies a package-level variable that is meant not to be modified), whereas //go:fix-to may or may not be (depending on the chosen expression).

But that's an interesting idea, and I would not object to adding a variant of fix-to for the “maybe incompatible” case — perhaps //go:suggest?

Will there be any warning when fixes do things like point to a constant with a changed value?

That seems like a reasonable thing for apidiff and go release (#26420) to warn about.

[bcmills]

If I had reassigned image.ZR, does this show up when I eventually build my code or during the fix?

In general I'm working on the assumption that folks don't go around monkey-patching variables in other packages. (For example, you should never swap out io.EOF either!)

I don't have a solution to that in general, but there are complementary proposals (such as #21130) that could help to detect those sorts of errors.

Note that you can also use the race detector to detect these sorts of modifications:

var ZR = Rectangle{}
func init() {
	go func() { _ = ZR }()  // Data race if anybody mutates ZR.
}()

[rsc]

I would like to see this simpified down to a single comment that means "this is 100% guaranteed semantically OK, because it is literally a definitional substitution", and because the tool is go fix the comment should also be //go:fix (not //go:forward).

For the io constants, os can do

//go:fix

const SEEK_SET = int(io.SeekSet)

Also the //go:fix comments should not be in the doc comment; they should be above them instead.

You want to be able to tell people "it is always OK to run go fix". A suggestion mechanism is fine as part of Deprecated comments but we should consider that in a separate proposal and separate tool.

[cep21]

Is there any worry about name squatting on //go:fix? Could there be many ways to fix things? Are there use cases where go:fix does not match with deprecated? An alternative may be to stick with deprecated formatting as the tag and use another tool that understands 1:1 mapping of deprecated APIs. For example, if a deprecated API is a type alias, it would replace the type.

[bcmills]

Is there any worry about name squatting on //go:fix?

@cep21, did you have anything else in mind for it?
(After all, there are infinitely many other words we can suffix onto //go: if we discover other use-cases.)

Could there be many ways to fix things?

Yes. //go:fix specifically should be read as “fix it to this”.

Are there use cases where go:fix does not match with deprecated? An alternative may be to stick with deprecated formatting as the tag and use another tool that understands 1:1 mapping of deprecated APIs.

The intent of this proposal is specifically to mark that the mapping is 1:1, and furthermore that the mapping is straightforward enough that a human doesn't need to think about how to apply it.

Without such an annotation, I don't think it's feasible to build a tool that can decide when a deprecated comment should result in automatic code transformations.

[cep21]

Without such an annotation, I don't think it's feasible to build a tool that can decide when a deprecated comment should result in automatic code transformations.

Can you give an example that fits with rcs's simplified proposal? He mentions

"this is 100% guaranteed semantically OK, because it is literally a definitional substitution"

In those cases, I can't see how it wouldn't be expected and safe for a // deprecated flag on a type alias or a variable assignment, like const SEEK_SET = int(io.SeekSet), wouldn't contain the same information.

[cep21]

More concretely, if the proposal is limited to just type aliases (as an example), in which situation would I not want to fix this?

// deprecated: blah blah
type Circuit = newpackage.Circuit

[cep21]

Are there any surprising consequences of the following proposal:

type T1 = T2:
    go fix will replace all instances of T1 with T2 if T1 is marked deprecated.

const X = Y
    go fix will replace all instances of X with Y, if:
      * X is marked deprecated
      * Y is not marked deprecated
      * Y is not a literal (is literal the right word?  I don't want to replace things like const X = 3)

[bcmills]

@cep21, the interesting use-cases for go fix are for function and method calls. Restricting the proposal to only constants and type-aliases doesn't help for things like “automatically migrating users of a v1 module to v2”.

[bcmills]

I don't want to replace things like const X = 3

Note that one of the examples — image.ZR — explicitly does recommend that users switch from an identifier to a literal expression.

[cep21]

Note that one of the examples — image.ZR — explicitly does recommend that users switch from an identifier to a literal expression.

That's not allowed by rcs's simplification since it's not guaranteed to be backwards compatible: It's possible to reassign var types. I may be misunderstanding what "definitional substitution" and "suggestion mechanism" means. Can those terms be defined? Either way, it's not a big concern and happy this issue is getting noticed!

[bcmills]

@cep21, I've been thinking some more about the interaction with deprecation. From discussions with @ianthehat, it seems that he would prefer something similar.

At the very least, I think a heuristic based solely on Deprecated comments would need to be transitive. Consider

// Deprecated: use T2.
type T1 = T2

// Deprecated: use T3
type T2 = T3

The arguably “correct” behavior is to inline uses of T1 all the way down to T3, and it would not be appropriate to stop that process just because T2 is itself deprecated.

[bcmills]

The interaction with literal values is interesting, though. In the declaration

// Deprecated: Use a literal image.Rectangle{} instead.
var ZR = Rectangle{}

it would certainly appropriate to replace the value with a literal.

However, sometimes a value with an existing Deprecated: comment is initialized from something other than its intended substitute expression:

// Deprecated: Use io.SeekStart, io.SeekCurrent, and io.SeekEnd.
const (
	SEEK_SET int = 0 // seek relative to the origin of the file
	SEEK_CUR int = 1 // seek relative to the current offset
	SEEK_END int = 2 // seek relative to the end
)

Moreover, the problem is even worse if the initializer is not idempotent:

// Deprecated: use os.ErrTemporary directly.
var ErrFlaky = fmt.Errorf("%w: operation flaky", os.ErrTemporary)

// Deprecated: use a caller-allocated slice.
var Buffer = make([]byte, 1<<10)

The non-idempotent case, at least, seems easier to detect, although I'm not sure whether it's easy enough to be effective in general.

[gopherbot]

Change https://go.dev/cl/649055 mentions this issue: internal/analysis/gofix: simple type aliases

[gopherbot]

Change https://go.dev/cl/649057 mentions this issue: x/tools: run gopls/internal/analysis/gofix/main.go -fix

[gopherbot]

Change https://go.dev/cl/649056 mentions this issue: x/tools: add //go:fix inline

[gopherbot]

Change https://go.dev/cl/648976 mentions this issue: x/tools: eliminate various unparen (et al) helpers

[jba]

It would be nice to replace the variable ioutil.Discard with io.Discard.

How do we feel about extending //go:fix inline to vars?

[adonovan]

How do we feel about extending //go:fix inline to vars?

I'm in favor. There are two cases:

1. The (shallow) content of the variable is never modified once initialized. This applies to forwarding declarations such as package ioutil; var Discard = io.Discard.
2. The identity of the variable is important, as it may be mutated or compared by address. In this case, the old and the new variable must be declared identical using a //go:linkname directive.

We could address both of them, but I suspect the second is sufficiently rare (and tricky) that it is not worth bothering with, at least for now.

[gopherbot]

Change https://go.dev/cl/649476 mentions this issue: gopls/internal/analysis/gofix: one function per pass

[gopherbot]

Change https://go.dev/cl/649456 mentions this issue: gopls/internal/analysis/gofix: one function per kind

[gopherbot]

Change https://go.dev/cl/650755 mentions this issue: gopls/internal/analysis/gofix: inline most aliases

[jba]

How do we feel about extending //go:fix inline to vars?

I'm in favor. There are two cases:

1. The (shallow) content of the variable is never modified once initialized. This applies to forwarding declarations such as package ioutil; var Discard = io.Discard.
2. The identity of the variable is important, as it may be mutated or compared by address. In this case, the old and the new variable must be declared identical using a //go:linkname directive.

We could address both of them, but I suspect the second is sufficiently rare (and tricky) that it is not worth bothering with, at least for now.

@adonovan, I take the opposite view: if we inline vars at all, we should only inline those that have both //go:fix inline and //go:linkname directives. But we may not want to encourage the use of //go:linkname.

Back in 2019 @rsc wrote "[//go:fix inline should mean] "this is 100% guaranteed semantically OK, because it is literally a definitional substitution." Nobody can make that guarantee for an exported variable without a //go:linkname.

Thoughts, @rsc @aclements?

[jba]

Much of the early discussion on this issue talked about modifying go fix to perform these inlines in bulk. Should we do that? Is it worth its own proposal?

[aclements]

I'm pretty squeamish about anything that encourages people to use go:linkname, especially on variables. That isn't even guaranteed to work (there are optimizations that would be nice to do that would make this fragile and, IIRC, it never worked in gccgo).

My inclination is that we just shouldn't do this for variables. Go does not have a mechanism for forwarding variables, therefore //go:fix inline does not make sense on variables.

[gopherbot]

Change https://go.dev/cl/651615 mentions this issue: gopls/internal/analysis/gofix: use 1.24 iterators

[gopherbot]

Change https://go.dev/cl/651617 mentions this issue: gopls/internal/analysis/gofix: generic aliases

[gopherbot]

Change https://go.dev/cl/651655 mentions this issue: gopls/internal/analysis/gofix: allow literal array lengths

[znkr]

My inclination is that we just shouldn't do this for variables. Go does not have a mechanism for forwarding variables, therefore //go:fix inline does not make sense on variables.

I don't understand why Go support for forwarding variables needs to be a prerequisite for this kind of refactoring. It would be great if we could restrict global variables in a way that makes the inline variable refactoring 100% safe to refactor, but I disagree that it's necessary. I think that some amount of risk is acceptable for automated refactorings because we have unit tests and human reviewers to catch mistakes.

[gopherbot]

Change https://go.dev/cl/653436 mentions this issue: maps,slices: add //go:fix inline annotations for std namesakes