Open bcmills opened 5 years ago
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.
@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.
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.
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).
@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.)
@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.
note that the
go
directive is more of a maximum than a minimum
Ah, noted.
A few questions:
go:fix-to .EXPR
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.
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 withnet.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.
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?
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.
(And do note that a //go:forward
inlining a function call, constant, or type alias is very unlikely to result in an incompatible change.)
image.ZR
, does this show up when I eventually build my code or during the fix?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.
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.
}()
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.
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.
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 withdeprecated
? An alternative may be to stick withdeprecated
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.
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.
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
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)
@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
”.
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.
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!
@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.
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.
Here is my revised proposal, based on https://github.com/golang/go/issues/32816#issuecomment-507721945.
A directive of the form //go:fix
, which must start at the beginning of a line (consistent with the other //go:
directives), specifies that references to the identifier(s) introduced in the next declaration in the file should be inlined at the point of use. Tools may apply that inlining directly (go fix
), or may produce a visual cue or prompt the user to apply inlining within an IDE (gopls
).
//go:fix
on a const
or var
declaration indicates that references to the identifiers within the declaration should be replaced with their corresponding initializer expressions.
//go:fix
annotation covers any var
declaration whose initializer expression is known not to be idempotent, tools may choose not to inline it, and diagnostic tools such as go vet
and gopls
should emit a warning for it.//go:fix
on a type
declaration indicates that references to the alias name(s) should be replaced with the (immediate) aliased type(s).
//go:fix
annotation on a type declaration covers any definition other than an alias, tools should not attempt to inline it, and diagnostic tools such as go vet
and gopls
should emit a warning for it.//go:fix
on a method or function declaration indicates that calls to the method or function should be replaced with an inline copy of its body.
return
statement, or an empty block, and the arguments passed to that function are either idempotent expressions or used exactly once each within the function body, it should be inlined directly to the call site.defer
statement, the tool must ensure that the deferred call runs after the last statement that was formerly in the function body. (It may either wrap the inlined body in a function literal, or verify that the last inlined statement is also the last statement of the enclosing function at the call site.)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 examples mentioned in the initial post, the resulting declarations might look like:
package image
//go:fix
// Deprecated: Use a literal image.Point{} instead.
var ZP = Point{}
//go:fix
// Deprecated: Use a literal image.Rectangle{} instead.
var ZR = Rectangle{}
package importer
//go:fix
// Deprecated: Use ForCompiler, which populates a FileSet
// with the positions of objects created by the importer.
func For(compiler string, lookup Lookup) types.Importer {
return ForCompiler(token.NewFileSet(), compiler, lookup)
}
package io
//go:fix
// Deprecated: Use io.SeekStart, io.SeekCurrent, and io.SeekEnd.
const (
SEEK_SET int = io.SeekStart
SEEK_CUR int = io.SeekCur
SEEK_END int = io.SeekEnd
)
package syscall
//go:fix
// Deprecated: Use ByteSliceFromString instead.
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 with only a //go:fix
annotation.
I like this proposal a lot. Just a few questions.
The examples for var ZP = Point
still include //go:forward
. Was that a copy/paste typo or is that still required?
The line cadence of the var ZP
example was a bit jarring. The //go:fix
between var ZP
and var ZR
feels abandoned or reads like it's in the middle of them and I'm not sure which it belongs to (it's clear from your spec it belongs to ZR, but reads abandoned). I understand why you did it that way, but I wonder if another version would be clearer. This is one of the things I like about tying it to Deprecated.
The go:fix
for function calls makes a lot of sense, but there's probably some difficult interactions between defer statements and panics that will need to be considered when implementing this.
You previously mentioned you were thinking about the interaction with deprecated. What were your final thoughts there?
The examples for
var ZP = Point
still include//go:forward
.
The //go:forward
was indeed a copy/paste error (fixed now).
The line cadence of the
var ZP
example was a bit jarring.
The distance between the //go:fix
token and the following variable declarations is indeed a bit jarring, although I don't seen an alternative in keeping with the current rules for //go:
directives. I think anything more compact would require changes in at least go doc
, and perhaps other tools as well.
The
go:fix
for function calls makes a lot of sense, but there's probably some difficult interactions between defer statements and panics that will need to be considered when implementing this.
The interaction between inlining and defer
and panic
can be a bit subtle, but I believe that the current proposal covers them adequately: specifically, “the tool must ensure that the deferred call runs after the last statement that was formerly in the function body”.
That's admittedly not a trivial property in general, but bear in mind that the tool always has the option to inline as a function literal if it can't figure out the behavior, and I doubt that many functions or methods tagged go:fix
will include a defer
statement anyway.
You previously mentioned you were thinking about the interaction with deprecated. What were your final thoughts there?
Keying off of Deprecated
comments would require that the tool be able to determine with 100% certainty whether a given expression is idempotent. If it is too conservative, it will fail to inline things that ought to be inlined, and if it is too aggressive, it will inline things that are not actually safe (such as the ErrFlaky
and Buffer
examples in https://github.com/golang/go/issues/32816#issuecomment-516150037).
Even then, the existence of Deprecated
comments that don't quite match their intended replacements today (such as io.SEEK_SET
, which is defined as simply 0
) would potentially make the tool-based inlining confusingly aggressive even when correct.
Thanks for the clarification! I don't have any other comments and I'm interested to see where this goes!
I'm curious if anyone following this issue has any additional thoughts or comments beyond what has already been said in the discussion in this issue so far?
@bcmills Does your latest proposal in https://github.com/golang/go/issues/32816#issuecomment-517827465 still represent your latest thinking here? I think there was some feedback from Russ in July 2019 in https://github.com/golang/go/issues/32816#issuecomment-507721945 that you then incorporated into your latest proposal. From a process perspective, would it make sense to ask for additional feedback from the proposal review committee? Even if there is no bandwidth to immediately implement, it might still be useful to continue to refine the design.
Yes, that comment still reflects my current thinking, modulo the fact that we can format the comments a bit more clearly now that #37974 is fixed.
Thanks @bcmills for spelling out your requirements in https://github.com/golang/go/issues/32816#issuecomment-517827465. I've been meaning to write an inliner function for use both by interactive tools (e.g. gopls) and for batch cleanup (as described here), and it's helpful to know your soundness expectations. I think I can summarize your position as: the transformation must preserve all observable behavior (with the exception of things observed through reflection on the call stack).
To achieve soundness requires that we address some subtle semantic edge cases. For example, given these inputs,
f(x)
...
func f(x T) { x.g() }
it may appear safe for the inliner to replace the statement f(x)
by the function body x.g()
, but that would be unsound if the declaration of g were:
func (x *T) g() { global = x }
var global *T
because x.g()
causes x to escape, and the naive inlining would change which variable escapes. Obviously I'm not proposing the inliner should do a full escape analysis, but even the most trivial escape analysis ("is the variable address-taken?") can be camouflaged in Go by the implicit address-taking operation in method calls: x.g() really means (&x).g(). A sound inliner will need to deal with that.
As another example, inlining a function from one package to another may reduce the set of packages it is allowed to directly import (due to .../internal/...
directories or Bazel visibility directives); more subtly, it may change the dialect of Go in use, since the two packages may be governed by go.mod files with different go
version directives, potentially changing the semantics of loop iteration variables.
Anyway, I plan to write an inliner this summer, and I will make sure it is sound. (Do let me know soon if someone else has "licked this cookie".)
To achieve soundness requires that we address some subtle semantic edge cases.
Agreed.
it may appear safe for the inliner to replace the statement f(x) by the function body x.g(), but that would be unsound if the declaration of g were: … A sound inliner will need to deal with that.
Yep. That would presumably have to include a (renamed) local variable for the function argument:
x1 := x
x1.g()
As another example, inlining a function from one package to another may reduce the set of packages it is allowed to directly import
Indeed. If the package needed for the inlined definition cannot be imported, probably the inlining tool should refuse to inline it, and go vet
and gopls
should emit a warning for any use sites that cannot be inlined as a result. (Generally when that sort of cross-visibility fix occurs, it is because the identifier should not have been exported in the first place.)
more subtly, it may change the dialect of Go in use
That was not an issue when I posted https://github.com/golang/go/issues/32816#issuecomment-517827465, but in light of #60078 it is certainly a significant concern. We may need to be careful about how we inline for
loops, but we probably would have needed to be extra careful about those anyway because of the same variable-capture issues. 😅
Apart from loops, perhaps the fix
tool should report an error (and go vet
and gopls
should emit warnings) if the go:fix
body cannot be inlined due to a feature that requires a higher Go version?
Hi @adonovan, FWIW, I had mentioned / linked above to the "Automatic API Updates" section from the initial modules blogs, but here is a more explicit mention by Russ that he had been thinking about that (and automatic inlining) when working on https://github.com/rsc/rf.
//go:fix on a type declaration indicates that references to the alias name(s) should be replaced with the (immediate) aliased type(s).
Do folks have any thoughts about whether non-aliased named types need to be supported? If they can already use type a alias type T = q.T
, they have done most of the work to translate the type from T
to q.T
. I am quite unsure about how to support this with just a single comment that is short and the translation cannot fail.
Change https://go.dev/cl/519715 mentions this issue: refactor/internal/inline: logic for inlining Go functions
@timothy-king, if the two types have the same underlying type, it might be possible to insert explicit conversions during translation, although that seems like it would add a lot of boilerplate to callers — so maybe we shouldn't support that at all?
(If the two types do not have the same underlying type, then I agree that an automatic migration is not feasible at all.)
Change https://go.dev/cl/523995 mentions this issue: internal/refactor/inline: an inliner for Go source
The function inliner, which was the major technical obstacle to building a prototype of the proposed tool, is now ready for serious use.
"Inlining" references to annotated constants and vars could be squeezed into the existing logic by modeling them as calls to a zero-argument function:
const two = 1 + 1 // => func two() untyped int { return 1 + 1 }
@suzmue
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
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
The basic idea here is to allow a //go:fix annotation on a definition or perhaps at some point also inside a function (for selective inlining) to mean that go fix should inline the definition at the use site.
@bcmills wrote a more detailed description at https://github.com/golang/go/issues/32816#issuecomment-517827465. I expect there will be minor adjustments as we implement it, and that's fine for the purpose of this proposal.
Are there any concerns or objections with the idea, or any aspects we may be overlooking that need discussion?
Are there any concerns or objections with the idea, or any aspects we may be overlooking that need discussion?
I have two questions, one about the implementation and one about annotation syntax.
The desired behavior could be implemented entirely within the go/analysis framework: for each function with a //go:fix annotation, the analyzer would export a fact consisting of the inliner's summary of the callee. For every call, the analyzer would report an "inlinable call" diagnostic with an associated SuggestedFix that does the inlining. Running a singlechecker for this analyzer with -fix would apply all the edits to a set of packages. (We have an implementation already.)
One advantage of this approach is that we can drop the analyzer into other analysis drivers such as gopls (for real-time feedback) and nogo (for Bazel, and Google's Go code base), and it will just work.
Q1. What are the pros and cons of adding this functionality within cmd/fix? Any good reasons not to develop only the analyzer approach?
The second question is the particular spelling of the annotation: this issue proposes //go:fix
comments; the prototype implementation above uses // inlineme
as a placeholder, but raises the question of some other syntax, perhaps based on a // Deprecated
comment.
Another possibility is to use whatever general analysis annotation syntax we end up agreeing upon (if any), though we don't even have a proposal for that yet, so it will be a while before any decision is made. I imagine it might look something like a no-op call inlineannot.InlineMe()
inside the function body, or a call inlineannot.Inline(Foo) or
inlineannot.Inline((*T).Foo)` external to the function or method of interest. But perhaps I'm getting ahead of myself.
Q2. How should we spell it?
+1 to using the analyzer framework for this job. We use that framework internally at Google for large scale changes and it's been a very pleasant experience.
Using analyzers for the fix command has the other advantage that you can extend it quite easily. To illustrate a potential future scope increase for the go fix command: One deprecation I ran across a number of times is adding a function variant that takes a context:
// Deprecated: Use MyFunctionContext instead
func MyFunction(...) error {
return MyFunctionContext(context.TODO(), ...)
}
func MyFunctionContext(ctx context.Context, ...) error { ... }
Inlining MyFunction
would result in a proliferation of context.TODO(). Wouldn't it be nice if that TODO could be resolved immediately when there's a context available?
Another comment I have is about //go:fix
. I think it's too implicit. I would prefer a more explicit annotation, that is understandable without looking up how the annotation works.
Perhaps //go:fix inline
?
In the long term we might also want it to apply to non-funcs, like type aliases or constants.
I think it is fine to use go/analysis the same way go vet does.
The second question is the particular spelling of the annotation: this issue proposes
//go:fix
comments; the prototype implementation above uses// inlineme
as a placeholder, but raises the question of some other syntax, perhaps based on a// Deprecated
comment.
I'm fine with other syntactic variations, but heuristics based on // Deprecated
comments seem like a non-starter: I expect that not all deprecated functions will be suitable for inlining, even if they happen to have very short implementations in terms of some potential replacement.
(For example, it may be that the deprecation is due to a common class of bug, and migrating from the function to the thing it wraps would perpetuate that bug instead of fixing it as intended.)
(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:
archive/tar.TypeRegA
says to useTypeReg
instead.archive/zip.FileHeader.{Compressed,Uncompressed}Size
say to use the corresponding*64
fields instead.image.ZR
andimage.ZP
say to use zero struct literals instead.go/importer.For
says to useForCompiler
, and is implemented as a trivial call toForCompiler
with an additional argument.io.SEEK_*
constants says to use correspondingio.Seek*
constants instead.syscall.StringByte{Slice,Ptr}
say to use the correspondingByte*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 asv2
, thenv2
can be implemented as a wrapper aroundv3
— and references to thev2
identifiers can be replaced with direct references to the underlyingv3
identifiers.See the current proposal details in https://github.com/golang/go/issues/32816#issuecomment-517827465 below.
Previous draft (prior to https://github.com/golang/go/issues/32816#issuecomment-507721945):
CC @marwan-at-work @thepudds @jayconrod @ianthehat @myitcv @cep21