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

by commotionfever • 4 months ago

185 76 4 months ago

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

tapirl7• 4 months 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• 4 months ago

Or: your buggy code is no longer buggy.

tapirl7• 4 months ago

You claim listens right for this specified example. :D

It is just a demo.

shoo3• 4 months ago

Great example, illustrating go1.26.1 go fix source inline transformation breaking program semantics. Raise it as a bug against go fix?

tapirl7• 4 months 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.

tapirl7• 4 months 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

tapirl7• 4 months 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]()
    }

tapirl7• 4 months 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 and https://github.com/golang/go/issues/78169

arjvik• 4 months 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.

tapirl7• 4 months 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.

adonovan• 4 months ago

Thanks, that's a bug. We should never inline a function that directly calls recover. I've filed https://go.dev/issue/78193.

shoo3• 4 months 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• 4 months 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

shoo3• 4 months 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• 4 months 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".

bmitch3020• 4 months ago

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.

It makes those breakages less painful. A project can eventually remove a deprecated API after notifying other projects to run `go fix`. And when projects ignore that advice (some always will), they can revert to a previous working version, run `go fix`, and then upgrade, without spending time in the code identifying how to replace each removed API.

And for those projects that routinely update and run `go fix`, they'll never notice the removal of deprecated code. Given the other benefits of `go fix`, switching to easier to read methods, and leveraging more efficient methods, in addition to security fixes that come with regular updates, this should be the workflow for most maintained projects.

omoikane2• 4 months 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

0x696C6961• 4 months ago

The //go:xyz comments are an established pattern in the Go tooling.

Mond_3• 4 months 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.)

kjksf• 4 months ago

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.

bheadmaster• 4 months ago

That's such an elegant solution.

I keep being impressed at subtle but meaningful things that Go does right.

dwattttt5• 4 months ago

If the comments impact correctness (which inlining doesn't, but I believe there are other directives that do), saying it's "an implementation detail" waves away "it's an implementation detail that everyone needs" aka part of the spec.

The reason it feels like a kludge is that "comments" are normally understood to be non-impactful. Is a source transformation that removes all comments valid? If comments have no impact per the spec, yes. But that's not the case here.

In practice comments in go are defined to be able to carry semantic meaning extensibly. Whether they're safe to ignore depends on what meaning is given to the directives, e.g. conditional compilation directives.

Groxx• 4 months ago

Because these are instructions for users for making tool-assisted changes to their source code, not a behavior that exists at runtime (or even compile time). A new syntax wouldn't make sense for it.

For other things, like `//go:noinline`, this is fair criticism. `//go:fix inline` is quite different in every way.

piekvorst2• 4 months ago

I suppose, to minimize its use. If annotations have the same syntactic weight as normal statements, such as “if” or “for” statements, there’s a temptation to use them liberally, which is clearly not a good fit for Go.

By making them comments, Go subtly signals that these are exceptional, making them less prominent and harder to abuse.

ansgri• 4 months 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• 4 months ago

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

gnabgib• 4 months 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 #scp

freakynit6• 4 months 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)?

alecthomas2• 4 months ago

These are called directives [1], and are treated as metadata by the compiler.

[1] https://pkg.go.dev/go/ast#Directive

freakynit6• 4 months ago

Understood... but why in comments?

piekvorst2• 4 months ago

I think the core reasoning is about minimizing its use. I have answered [1] the same question in another thread.

https://news.ycombinator.com/item?id=47395574

pjmlp• 4 months ago

This is typical Go design, other languages do it worse, it isn't really needed, and then it gets added half way as it was supposed to be if done early on, and everyone cheers how Go is a "simple" language.

Just like some other famous languages of the authors.

frou_dh• 4 months ago

It's abject hackery because such "comment" directives don't even necessarily have to be a comment to be active:

    const x = `This whole thing is
    a
    //go:generate worse-is-better
    multiline
    string literal`

A lot of people in this discussion are beating up Go for using syntactical comments for directives, but in reality the implementation is even less principled than that!

fghorow• 4 months 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

"""

(Why yes, that is IBM System 360 JCL from circa 1975. Why do you ask?)

rishabhjajoriya• 4 months ago

That was quite insightful read

divan• 4 months 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.

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

Related Stories