Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ianlancetaylor opened this issue Jan 6, 2021 · 13 comments

Comments

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

Merovius commented Jan 6, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

Merovius commented Jan 7, 2021

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
Copy link

hherman1 commented Jan 7, 2021

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
Copy link
Member

mvdan commented Jan 7, 2021

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
Copy link
Member

mvdan commented Jan 7, 2021

@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
Copy link
Contributor

bcmills commented Jan 7, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

rsc commented Jan 8, 2021

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
Copy link
Contributor

rsc commented Jan 8, 2021

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
Copy link
Contributor

Merovius commented Jan 8, 2021

@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
Copy link
Contributor

rsc commented Jan 8, 2021

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

@ianlancetaylor
Copy link
Contributor Author

As we are now likely to decline #43217, I'm retracting this proposal.

@golang golang locked and limited conversation to collaborators Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants