proposal: embed: give error in 1.14.x and 1.15.x if a go:embed directive is seen

[ianlancetaylor]

This is an idea from @marwan-at-work described at #43217 (comment).

We seem likely to accept proposal #43217 which will permit writing code like

//go:embed data.txt
var data string

In particular this file will not need to import the "embed" package. It follows that this code will compile without errors when using a release of Go from before 1.16, but the code will not work as expected.

To reduce the possibility of problems during this transition, this proposal is that for the next Go 1.14.x and 1.15.x releases, any line comment at package scope that starts with //go:embed should be treated as a compilation error.

CC @Merovius @rsc @robpike

[Merovius]

I've been, based on this case, wondering whether that should apply to any unrecognized //go:* comment. i.e. instead of building a list of directives to reject (seeded as ["embed"]), build a list of directives to allow (consisting of all currently used directives and any that were removed in the past). Arguably, that's not backwards compatible, but neither is this proposal. And it would safe-guard against the same case when adding new directives in the future.

[ianlancetaylor]

My first thought is that we don't want to do this for any unrecognized "go:" directive. For example, ignoring "go:noinline", seems unlikely to cause real harm. It's only important to give an error for directives for which ignoring the directive causes the program behavior to break.

[Merovius]

I think the tradeoff is, that with a rejection-list, people will still need to keep up with the latest point-release to prevent themselves from running into this problem. i.e. people who don't upgrade to go1.15.7 (or whatever would be the specific version) will still have this problem. That might be fine. As usual, I'm very fine letting other people make this tradeoff, I don't have super strong opinions :)

[hherman1]

Wasn’t part of the intent of making //go: directives comments that they’re not a part of the language, don’t have to be supported by other tool chains, and so shouldn’t be considered breaking if they’re ignored? I think pragmatically this is a good change, but is it violating the meaning of //go:?

[mvdan]

people will still need to keep up with the latest point-release to prevent themselves from running into this problem

Users will run into plenty of problems if they don't stay up to date with stable releases. I personally think that's fine.

[mvdan]

@hherman1 I think it's a bit more nuanced than that. An example is formatting tools like gofmt and goimports; if the Go language is extended, they'd have to be updated to keep working normally. Whereas normal comment directives are just comments.

Other tool chains or implementations of Go can choose what to do with //go: directives they do not support. I think what @ianlancetaylor says seems fair; by default the majority of directives shouldn't break programs, but this one likely would.

#43217 could do a better job at this, I think. If an external Go implementation does not support the embed package, then that should fail by definition, without the external implementation having to learn to reject //go:embed.

[bcmills]

I think this is a superficial workaround for a much deeper design issue in #43217 and #41191.

Backporting a compile error to 1.14 and 1.15 does not help users who are still on 1.13 (even though it is unsupported at this point), or who are using one of the myriad alternate Go implementations (such as tinygo or gccgo) that would not necessarily receive such a backport.

It's better than nothing, but I would prefer that we instead defined go:embed in a way that does not require retroactive language changes in order to produce errors with unsupported toolchains. (The approach described in #43217 (comment) is one such alternative.)

[ianlancetaylor]

I don't strongly object to your suggestion, but I also don't think we should optimize for the transition period. This proposal is an aid to the transition, it's not essential.

[rsc]

The most we should do in a Go 1.14.x and 1.15.x point release would be to reject //go:embed.

We certainly should not reject all other unrecognized //go comments in a point release. I'm pretty sure we shouldn't do it at all, but if we were going to, that's the kind of exciting (might break your code) change you'd do in a new major release. Point releases must not be that kind of exciting.

[rsc]

An example of why we would not want to reject all unknown //go: comments is the //go:fix comment in https://research.swtch.com/vgo-import#automatic_api_updates. That's not implemented yet, but if we added it, we wouldn't want people to be hesitant to adopt it for fear of breaking old picky Go versions.

[Merovius]

@rsc Yeah, sorry, I didn't actually wanted to suggest to do that in a point-release anyway, I meant more medium-term :) And if you don't think it's a good idea, that's fine.

[rsc]

No worries, I just wanted to make sure the discussion for the point release was narrowly scoped.