11 ms·

//go:fix inline and the source-level inliner

185 points by commotionfever 6mo ago

measurablefunc 6mo ago
https://en.wikipedia.org/wiki/Hygienic_macro https://en.wikipedia.org/wiki/Hygienic_macro
tapirl 6mo ago
It looks the following code will be rewritten badly, but no ways to avoid it? If this is true, maybe the blog article should mention this. package main //go:fix inline func handle() { recover() } func foo() { handle() } func main() { defer foo() panic("bye") }
- arccy 6mo ago
  Or: your buggy code is no longer buggy.
  - tapirl 6mo ago
    You claim listens right for this specified example. :D It is just a demo.
- shoo 6mo ago
  Great example, illustrating go1.26.1 go fix source inline transformation breaking program semantics. Raise it as a bug against go fix?
  - tapirl 6mo ago
    As I have mentioned, no ways to fix it. Because it is hard to know whether or not the handle function is called in a deferred call.
- tapirl 6mo ago
  Another example (fixable): package main import "unsafe" //go:fix inline func foo[T any]() { var t T _ = 1 / unsafe.Sizeof(t) } func main() { foo[struct{}]() } Go is a language full of details: https://go101.org/details-and-tips/101.html https://go101.org/details-and-tips/101.html
  - tapirl 6mo ago
    similar: package main //go:fix inline func foo[T [8]byte | [4]uint16]() { var v T var n byte = 1 << len(v) >> len(v) if n == 0 { println("T is [8]byte") } else { println("T is [4]uint16]") } } func main() { foo[[8]byte]() }
  - tapirl 6mo ago
    another: package main type T = [8]byte var a T //go:fix inline func foo() T { return T{} } func main() { if foo() == a { } } filed: https://github.com/golang/go/issues/78170 https://github.com/golang/go/issues/78170 and https://github.com/golang/go/issues/78169 https://github.com/golang/go/issues/78169
- arjvik 6mo ago
  recover()'s semantics make it so that "pointless" use like this can be inlined in a way that changes its semantics, but "correct" use remains unchanged. Yes, maybe some code uses recover() to check if its being called as a panic handler, and perhaps `go fix` should add a check for this ("error: function to be inlined calls recover()"), but this isn't a particularly common footgun.
  - tapirl 6mo ago
    > ... and perhaps `go fix` should add a check for this ( This is an impossible task. For a library function, you can't know whether or not the function is defer called. Maybe this is not an important problem. But it would be better if the blog article mentions this.
  - hrmtst93837 6mo ago
    [flagged]
- adonovan 6mo ago
  Thanks, that's a bug. We should never inline a function that directly calls recover. I've filed https://go.dev/issue/78193 https://go.dev/issue/78193.
shoo 6mo ago
If I follow, this isn't a compile time inline directive, it's a `go fix` time source transformation of client code calling the annotated function. Per the post, it sounds like this is most effective in closed-ecosystem internal monorepo-like contexts where an organisation has control over every instance of client code & can `go fix` all of the call sites to completely eradicate all usage of a deprecated APIs: > For many years now, our Google colleagues on the teams supporting Java, Kotlin, and C++ have been using source-level inliner tools like this. To date, these tools have eliminated millions of calls to deprecated functions in Google’s code base. Users simply add the directives, and wait. During the night, robots quietly prepare, test, and submit batches of code changes across a monorepo of billions of lines of code. If all goes well, by the morning the old code is no longer in use and can be safely deleted. Go’s inliner is a relative newcomer, but it has already been used to prepare more than 18,000 changelists to Google’s monorepo. It could still have some incremental benefit for public APIs where client code is not under centralised control, but would not allow deprecated APIs to be removed without breakage.
- avabuildsdata 6mo ago
  yeah this is the part that got me excited honestly. we're not google-scale by any stretch but we have ~8 internal Go modules and deprecating old helper functions is always this awkward dance of "please update your imports" in slack for weeks. even if it doesn't let you delete the function immediately for external consumers, having the tooling nudge internal callers toward the replacement automatically is huge. way better than grep + manual PRs
  - shoo 6mo ago
    it could be better than a nudge -- if you could get a mandatory `go fix` call into internal teams' CI pipelines that either fixes in place (perhaps risky) or fails the build if code isn't already identical to fixed code.
- RossBencina 6mo ago
  I'm not sure what all of the hazards are, but I could imagine a language (or a policy) where public APIs ship with all of the inline fix directives packaged as robust transactions (some kind of "API-version usage diffs"). When the client pulls the new API version they are required to run the update transaction against their usage as part of the validation process. The catch being that this will only work if the fix is entirely semantically equivalent, which is sometimes hard to guarantee. The benefits would be huge in terms of allowing projects to refine APIs and fix bad design decisions early rather than waiting or never fixing things "because too many people already depend on the current interface".
omoikane 6mo ago
I wonder why they chose to add these directives as comments as opposed to adding new syntax for them. It feels like a kludge. https://wiki.c2.com/?HotComments https://wiki.c2.com/?HotComments
- 0x696C6961 6mo ago
  The //go:xyz comments are an established pattern in the Go tooling.
  - Mond_ 6mo ago
    This is begging the question. Yes, but why did they do that over dedicated syntax? (My personal theory is that early go had a somewhat misguided idea of simplicity, and preferred overloading existing concepts with special cases over introducing new keywords. Capitalization for visibility is another example of that.)
    - thwarted 6mo ago
      //go:xyz is dedicated syntax that is compatible with both the language spec and other toolchains that don't know about it.
      - Mond_ 6mo ago
        It's an overloaded comment. I am personally quite fine with it, I don't think it's bad. but it is an overloaded comment.
        thwarted 6mo ago
        I'm no longer sure what you're saying. You asked why they didn't go with dedicated syntax, I listed two advantageous aspects of the chosen syntax. We know it's an overloaded comment: that's literally one of the advantages.
        6mo ago
ansgri 6mo ago
Good illustration that a seemingly simple feature could require a ton of functionality under the hood. Would be nice to have this in Python.
vismit2000 6mo ago
Earlier submission: https://news.ycombinator.com/item?id=47385766 https://news.ycombinator.com/item?id=47385766
- gnabgib 6mo ago
  Far later submission. Check the ID again.. you were 2 days later. There was even a more upvoted post between your triple dupe and this https://news.ycombinator.com/item?id=47347322 https://news.ycombinator.com/item?id=47347322 #scp
freakynit 6mo ago
Can't golang devs prioritize something like annotations or other attribute/metadata system instead of writing these in comments? I'm pretty sure this must have been raised a lot of times before, so just wanted to ask if there is/are any specific reason(s)?
- alecthomas 6mo ago
  These are called directives [1], and are treated as metadata by the compiler. [1] https://pkg.go.dev/go/ast#Directive https://pkg.go.dev/go/ast#Directive
  - freakynit 6mo ago
    Understood... but why in comments?
    - alecthomas 6mo ago
      Someone else said this below... > Go designers distinguish between Go language as defined by Go spec and implementation details. > //go:fix is something understood by a particular implementation of Go. Another implementation could implement Go without implementing support for //go:fix and it would be a fully compliant implementation of Go, the language. > > If they made it part of the syntax, that would require other implementations to implement it. ...I'm not sure I buy that argument TBH.
      - freakynit 6mo ago
        hmm... thanks... And yes, I don't buy it either. "If they made it part of the syntax, that would require other implementations to implement it." ... I mean, so what? Has golang stopped ading new features to the spec? If not (which I guess so), then how is this any different? Unless you have freezed the language, this reasoning doesn't make sense to me.
        9rx 6mo ago
fghorow 6mo ago
I know this is off-topic, and likely to get me down-voted, but hey! I'll live dangerously for the sake of repeating a hilarious (to me) .sig in a now ancient Usenet post. These //go.* commands always remind me of this: """ //GO.SYSIN DD * DOO DAH DOO DAH """ (Why yes, that is IBM System 360 JCL from circa 1975. Why do you ask?)
useftmly 6mo ago
[dead]
rishabhjajoriya 6mo ago
That was quite insightful read
divan 6mo ago
As usual, great writeup and problem solving from Go team. One nitpick: wording "call to oldmath.Sub should be inlined" might be a bit confusing due to existing meaning of word "inlining" for functions (i.e. compiler inlining optimization). Without this article I would not be able to guess that this diagnostic message refer to something else.

185 points by commotionfever 6mo ago

https://en.wikipedia.org/wiki/Hygienic_macro https://en.wikipedia.org/wiki/Hygienic_macro

It looks the following code will be rewritten badly, but no ways to avoid it? If this is true, maybe the blog article should mention this. package main //go:fix inline func handle() { recover() } func foo() { handle() } func main() { defer foo() panic("bye") }

arccy 6mo ago
Or: your buggy code is no longer buggy.
- tapirl 6mo ago
  You claim listens right for this specified example. :D It is just a demo.
shoo 6mo ago
Great example, illustrating go1.26.1 go fix source inline transformation breaking program semantics. Raise it as a bug against go fix?
- tapirl 6mo ago
  As I have mentioned, no ways to fix it. Because it is hard to know whether or not the handle function is called in a deferred call.
tapirl 6mo ago
Another example (fixable): package main import "unsafe" //go:fix inline func foo[T any]() { var t T _ = 1 / unsafe.Sizeof(t) } func main() { foo[struct{}]() } Go is a language full of details: https://go101.org/details-and-tips/101.html https://go101.org/details-and-tips/101.html
- tapirl 6mo ago
  similar: package main //go:fix inline func foo[T [8]byte | [4]uint16]() { var v T var n byte = 1 << len(v) >> len(v) if n == 0 { println("T is [8]byte") } else { println("T is [4]uint16]") } } func main() { foo[[8]byte]() }
- tapirl 6mo ago
  another: package main type T = [8]byte var a T //go:fix inline func foo() T { return T{} } func main() { if foo() == a { } } filed: https://github.com/golang/go/issues/78170 https://github.com/golang/go/issues/78170 and https://github.com/golang/go/issues/78169 https://github.com/golang/go/issues/78169
arjvik 6mo ago
recover()'s semantics make it so that "pointless" use like this can be inlined in a way that changes its semantics, but "correct" use remains unchanged. Yes, maybe some code uses recover() to check if its being called as a panic handler, and perhaps `go fix` should add a check for this ("error: function to be inlined calls recover()"), but this isn't a particularly common footgun.
- tapirl 6mo ago
  > ... and perhaps `go fix` should add a check for this ( This is an impossible task. For a library function, you can't know whether or not the function is defer called. Maybe this is not an important problem. But it would be better if the blog article mentions this.
- hrmtst93837 6mo ago
  [flagged]
adonovan 6mo ago
Thanks, that's a bug. We should never inline a function that directly calls recover. I've filed https://go.dev/issue/78193 https://go.dev/issue/78193.

shoo 6mo ago

If I follow, this isn't a compile time inline directive, it's a `go fix` time source transformation of client code calling the annotated function. Per the post, it sounds like this is most effective in closed-ecosystem internal monorepo-like contexts where an organisation has control over every instance of client code & can `go fix` all of the call sites to completely eradicate all usage of a deprecated APIs: > For many years now, our Google colleagues on the teams supporting Java, Kotlin, and C++ have been using source-level inliner tools like this. To date, these tools have eliminated millions of calls to deprecated functions in Google’s code base. Users simply add the directives, and wait. During the night, robots quietly prepare, test, and submit batches of code changes across a monorepo of billions of lines of code. If all goes well, by the morning the old code is no longer in use and can be safely deleted. Go’s inliner is a relative newcomer, but it has already been used to prepare more than 18,000 changelists to Google’s monorepo. It could still have some incremental benefit for public APIs where client code is not under centralised control, but would not allow deprecated APIs to be removed without breakage.

avabuildsdata 6mo ago
yeah this is the part that got me excited honestly. we're not google-scale by any stretch but we have ~8 internal Go modules and deprecating old helper functions is always this awkward dance of "please update your imports" in slack for weeks. even if it doesn't let you delete the function immediately for external consumers, having the tooling nudge internal callers toward the replacement automatically is huge. way better than grep + manual PRs
- shoo 6mo ago
  it could be better than a nudge -- if you could get a mandatory `go fix` call into internal teams' CI pipelines that either fixes in place (perhaps risky) or fails the build if code isn't already identical to fixed code.
RossBencina 6mo ago
I'm not sure what all of the hazards are, but I could imagine a language (or a policy) where public APIs ship with all of the inline fix directives packaged as robust transactions (some kind of "API-version usage diffs"). When the client pulls the new API version they are required to run the update transaction against their usage as part of the validation process. The catch being that this will only work if the fix is entirely semantically equivalent, which is sometimes hard to guarantee. The benefits would be huge in terms of allowing projects to refine APIs and fix bad design decisions early rather than waiting or never fixing things "because too many people already depend on the current interface".

omoikane 6mo ago

I wonder why they chose to add these directives as comments as opposed to adding new syntax for them. It feels like a kludge. https://wiki.c2.com/?HotComments https://wiki.c2.com/?HotComments

0x696C6961 6mo ago
The //go:xyz comments are an established pattern in the Go tooling.
- Mond_ 6mo ago
  This is begging the question. Yes, but why did they do that over dedicated syntax? (My personal theory is that early go had a somewhat misguided idea of simplicity, and preferred overloading existing concepts with special cases over introducing new keywords. Capitalization for visibility is another example of that.)
  - thwarted 6mo ago
    //go:xyz is dedicated syntax that is compatible with both the language spec and other toolchains that don't know about it.
    - Mond_ 6mo ago
      It's an overloaded comment. I am personally quite fine with it, I don't think it's bad. but it is an overloaded comment.
      - thwarted 6mo ago
        I'm no longer sure what you're saying. You asked why they didn't go with dedicated syntax, I listed two advantageous aspects of the chosen syntax. We know it's an overloaded comment: that's literally one of the advantages.
        6mo ago

ansgri 6mo ago

Good illustration that a seemingly simple feature could require a ton of functionality under the hood. Would be nice to have this in Python.

vismit2000 6mo ago

Earlier submission: https://news.ycombinator.com/item?id=47385766 https://news.ycombinator.com/item?id=47385766

gnabgib 6mo ago
Far later submission. Check the ID again.. you were 2 days later. There was even a more upvoted post between your triple dupe and this https://news.ycombinator.com/item?id=47347322 https://news.ycombinator.com/item?id=47347322 #scp

freakynit 6mo ago

Can't golang devs prioritize something like annotations or other attribute/metadata system instead of writing these in comments? I'm pretty sure this must have been raised a lot of times before, so just wanted to ask if there is/are any specific reason(s)?

alecthomas 6mo ago
These are called directives [1], and are treated as metadata by the compiler. [1] https://pkg.go.dev/go/ast#Directive https://pkg.go.dev/go/ast#Directive
- freakynit 6mo ago
  Understood... but why in comments?
  - alecthomas 6mo ago
    Someone else said this below... > Go designers distinguish between Go language as defined by Go spec and implementation details. > //go:fix is something understood by a particular implementation of Go. Another implementation could implement Go without implementing support for //go:fix and it would be a fully compliant implementation of Go, the language. > > If they made it part of the syntax, that would require other implementations to implement it. ...I'm not sure I buy that argument TBH.
    - freakynit 6mo ago
      hmm... thanks... And yes, I don't buy it either. "If they made it part of the syntax, that would require other implementations to implement it." ... I mean, so what? Has golang stopped ading new features to the spec? If not (which I guess so), then how is this any different? Unless you have freezed the language, this reasoning doesn't make sense to me.
      - 9rx 6mo ago

fghorow 6mo ago

I know this is off-topic, and likely to get me down-voted, but hey! I'll live dangerously for the sake of repeating a hilarious (to me) .sig in a now ancient Usenet post. These //go.* commands always remind me of this: """ //GO.SYSIN DD * DOO DAH DOO DAH """ (Why yes, that is IBM System 360 JCL from circa 1975. Why do you ask?)

useftmly 6mo ago

[dead]

rishabhjajoriya 6mo ago

That was quite insightful read

divan 6mo ago

As usual, great writeup and problem solving from Go team. One nitpick: wording "call to oldmath.Sub should be inlined" might be a bit confusing due to existing meaning of word "inlining" for functions (i.e. compiler inlining optimization). Without this article I would not be able to guess that this diagnostic message refer to something else.