-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: remove import restriction #43217
Comments
Using a magic import I'm more okay with |
Personally I like that its just a plain string and a []byte. The type wrappers aren't the end of the world, but I don't find the "_" import offensive enough to justify the expanded api personally, esp. considering there aren't actually any side effects of this import. |
FWIW, I started writing a blog post about |
I’m sure there’s already a great explanation of why this is a bad idea that I missed, but what if you just had no import at all, and //go:embed worked on its own? I take it that’s off the table? |
That would slow down go/build (e.g., cmd/go). Currently to compute package dependencies, it only parses Go source files up through the With |
One potential expansion on this proposal: The declaration of a For example, //go;embed: file.txt
var file embed.String would cause a compiler error which will alleviate programmer confusion that they misspelled Similarly, the lack of a go directive comment could also be a compiler error, because why would a programmer declare an This would address both @mdempsky and @rsc 's comments here by keeping the embedding syntax as is while providing better developer experience. Hopefully my skimming of the comments didn't miss a similar suggestion. |
The draft proposal specifically states: "It is not an error to declare an Note also that a single variable declaration can have multiple (That said, because all of the |
I've been playing with go:embed on the beta a bit, and I noticed something I'd like to add to the mix. If I start working on a file with an embedded variable, and then I decide I want to move this embedded variable to a different file or module, its very easy to forget to remove the _ "embed" import, and because the import is explicitly For that reason, I think it would be valuable at the very least for go to complain about unused I really like the idea of embedding simple |
While I'm here, I'll add on that I found it surprising that |
I also think it's really nice that you can use the plain string/[]byte. And while the drawback is that an As a proof of concept, I wrote a quick commit on top of |
People seem overwhelmingly happy with this change, from what I'm seeing here. To reply to @marwan-at-work, you can't exactly use the plain string/[]byte. You have to adorn them with |
I have a question, why [eidt] And is it ok to move |
@go101, the explicit goal of this proposal is to define types in package embed, so that the import will be required. |
Based on the discussion above, this seems like a likely accept. |
Quick question, does the import need to be named “embed” or can it be aliased (import xembed "embed")? I assume renaming the import is okay, but I don’t recall seeing it tested in the CLs yet. |
I find the "import _ embed" more logical than an alias. You are, truly, importing embed for its side effects. Document that if you are using embed just for strings or bytes, you must use the underscore. If you use the _ import, the problem is solved, only two keystrokes (space, underscore) are required, and all is smooth now. If you use the alias, you need to type more and you need to understand that it is an alias and deal with the fact that your variable although declared as embed.String is really just a plain string, despite its declaration. That troubles me, as its effects could ripple through the file. If an underscore import is good enough for image/jpeg, it's good enough for embed. |
To reiterate: A big thumbs down for me. There is zero need for a new mechanism. |
If I think if |
The big benefit I see with the aliases is that goimports will insert (and remove) the import for you. That makes all the changes you actually need to type in one location (2 lines, the |
I understand the apparent convenience of the proposal, and how tools can use it to manage things for you, but no such argument - or tooling - is necessary if the documentation and practice say to use use an underscored import. I cannot stress enough that two keystrokes are all that are required. TWO! A third of what appear in the word "string". All the mechanism necessary already exists and is widely used. There's no need to provide anything else. No documentation and explanation of the special aliases required. (No ugly aliases required, either, but the sentiment underlying that may hold a bias.) Should embed acquire other types and other mechanisms, which is certainly possible, the underscore import will continue to solve the problem without further changes, but to go with the proposal would require expanding the space of magic names. To put it another way, if the embed type is bringing in a string, let me declare it as a regular string. We already have all we need to solve this. Please don't add more complexity. |
@ianthehat Unless I'm mistaken, modules are allowed to require modules of a higher Go version without erroring, so if you depend on a module that says |
With the transition concerns, I've become sympathetic to the suggestion someone already made (but I can't find it) which is that we just don't permit We can then add //go:embed file.txt
var fileFS embed.FS
var file = fileFS.AsBytes() // file is type []byte, if this is the only use of fileFS no copy is required Certainly adds boilerplate, but sidesteps the transition issue. We can still consider supporting simple |
That seems distastefully roundabout to me, multiple layers of special cases. Strive for easy generality rather than a unique sequence of steps that yields a single solution. |
It seems to me that we already have the easy generality: the general case is All we are talking about here is the special cases: //go:embed file.txt // required to be exactly one file
var file string |
I also appreciate the only one way to do it and explicit with |
At this point, we now know that the import serves three purposes:
We were willing to throw out (1) and (2) in favor of the simplicity of not having an import. But (3) is a show-stopper. I am now convinced that our initial instincts were correct and we should keep the import requirement. Ian suggested discarding strings and byte slices, but I am very reluctant to do that. I think they will be the primary mode of embedding for many people (same as embed.FS will be for many other people), and they were a key part of the proposal as accepted. It seems like the best path forward is what we had planned, namely keeping the import requirement. |
If the import requirement is back, what about |
@carlmjohnson those who want automatic import can use embed.FS and those who prefer raw string/[]byte can use raw import |
Worth re-mentioning that goimports can also be taught to underscore import "embed" for you by analyzing compiler directives in a file, which I have demonstrated here: marwan-at-work/tools@214d261 I believe @rsc 's comment above was that having embed.String/Bytes will alleviate the need to teach goimports and tools new rules. But in light of the discussion that followed to avoid type aliases, maybe updating such tools is a worthy trade-off? |
@carlmjohnson I've retracted the idea of embed.String and embed.Bytes. They are more awkward than requiring the import. I'm not convinced goimports needs to put the import in automatically, although it is fine for it to do that. (It's not that hard for people to write the import, and we've now worked out a compelling explanation for why it is there.) |
In that vein, it would be nice for goimports to remove the import automatically, to deal with orphaned embed lines. |
For the record, I don't mind goimports adding the blank import, but removing it seems beyond the pale. Breaking the general rule "goimports leaves blank imports alone" would require very significant justification, and I don't think this is it. Suppose it were completely justified to break the rule for "embed" (it's not - maybe the user wants to provoke a compiler error if "embed" is not available - how does that user override goimports? - but suppose it were), then you still have a special case behavior now in goimports that users can observe and misunderstand. When they see goimports remove the blank import for "embed", now they have to wonder about whether their other, perhaps more important, blank imports will also be removed. And maybe the decision is it's not worth the risk and stop using goimports. Goimports really has to be 100% safe, never changing the meaning of code that compiles. It should continue to limit itself to fixing compile errors (missing imports, unnecessary imports) and not make other changes. |
This was a "likely accept" before we understood the "silent miscompilation" problem described in #43217 (comment). |
Based on the discussion above, this proposal seems like a likely decline. |
The current implementation requires saying "string" or "[]byte" and disallows aliases, defined types, and even "[]uint8". This was not 100% intended and mostly just fell out of when the checks were being done in the implementation (too early, before typechecking). After discussion on #43217 (forked into #43602), the consensus was to allow all string and byte slice types, same as we do for string conversions in the language itself. This CL does that. It's more code than you'd expect because the decision has to be delayed until after typechecking. But it also more closely aligns with the version that's already on dev.regabi. Fixes #43602. Change-Id: Iba919cfadfbd5d7116f2bf47e2512fb1d5c36731 Reviewed-on: https://go-review.googlesource.com/c/go/+/282715 Trust: Russ Cox <rsc@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Is it enough for embed to be imported anywhere in a program, or does it need to be imported in the same package as the |
It needs to be in the same file as the |
No change in consensus, so declined. |
As every |
@mdempsky points out in #41191 (comment) that the late addition of string and []byte to the embedding proposal has some unfortunate side effects that we might want to make sure we are happy with.
There are three details of string and []byte embedding that are at least surprising. This issue is about two of them. See #43216 for the third. The two for this issue are as follows.
First, to make sure source files containing embedding directives are clearly identified (by an import), we require an
import _ "embed"
to writeeven though there is no use of the embed package. This requires a special case in goimports. (This is noted in the proposal as accepted.)
Second, the late addition says “a plain string or []byte variable”, and I really did mean those words. Only those specific types are allowed, not []uint8 and not other user-defined aliases for those types.
I see two options here. One is to do nothing and leave things as they are. That's possible and is the "this issue gets declined" outcome. The other option I can see is to introduce new type aliases embed.String and embed.Bytes and require those to be used as the types, which is what this proposal issue is meant to discuss and decide whether to accept.
Specifically:
Then instead of writing:
You have to write:
That solves both of the issues: the import is now required without any special case in goimports, and there is no question about whether []uint8 or other user-defined alias is OK. Only embed.String and embed.Bytes are OK.
What do people think? (Thumbs up / thumbs down is fine.)
[One option that's not on the table is removing string and []byte embedding entirely. Many people asked for that during the pre-proposal discussions, and I think it is important to respect that process. We can fix problems, but I think it would be improper to drop the functionality completely.]
Thank you!
The text was updated successfully, but these errors were encountered: