-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
compiler/protogen: Go name conflicts can occur for non-style compliant protobuf names #1206
Comments
The derived Go names that I should note that naming styles in most programming languages is typically an aesthetic concern. However, for protocol buffers, it's actually a semantic concern since the naming style used makes a difference in whether generated code is even valid. Therefore adherence to the style is of much greater concern beyond just personal preferences. If the protobuf style guide is properly followed, conflicts should not occur. Otherwise, all bets are off, and we can't guarantee conflict-free name generation. If you want guaranteed conflict-free name derivation, then the following naming styles must be used for the corresponding descriptor types:
In particular:
|
Explicit failure is better than a silent one in case there are no plans to support non-style compliant proto files. Centralised proto-> multiple language automated builds pass right now, only to get broken on import. Can we add an explicit failure with an error message for cases where conflicts will occur? If we want to support the behaviour, we can follow the convention proto2 codegen does and add an |
There's no way we can enforce this today without breaking the world. The situation is unfortunate, but it is what it is.
We already do this for some small set of conflicts. We consider it a grave mistake to have ever done this since it means that generated names are dependent on the order that fields are specified in a |
The world already breaks when those failures happen. It just breaks when the program compiles instead of code generation. Making it break at build time will not break any successfully running program, will help catch problems early.
Oh yes, you're correct! For fields I assumed you'd have ordered it based on the field numbering system. No way to translate that into methods.
|
The code breaks if you add another non-compliant name that results in a conflict. That's very different from breaking the code the moment if any non-style compliant name is used. I should also note that style enforcement is not the responsibility of |
I meant the examples I posted in original issue, sorry for the confusion. We can fail explicit when we know generated code will not compile.
Given that one officially supported major language implementation stops working in some cases, this might be a good candidate issue for discussion in v4.
Someone trying to migrate from non-compliant code to compliant one will also face this. If there is a way to solve for that case, I can help dedicate time on that. |
What's the point of building conflict detection code into
What is v4? |
I believe it might not be that difficult to achieve this by leveraging public API of the go compiler. We already do something similar for formatting go code, checking for package errors will not be a lot of additional effort for better UX.
proto4, whenever that comes. Is there any official place to request features/fixes for the next major version or to file things for consideration when that conversation begins? |
That's 100LOC to maintain for slightly better UX; I'm not convinced this is worth it. If there's anything to do here, it should be in
In my opinion, the introduction of proto3 itself, given the pre-existing proto2 has been technically expensive to maintain and rollout. I'm doubtful there will be a proto4. Regardless, any discussion about proto4 should be at github.com/protocolbuffers/protobuf |
This issue is more about how to handle conflicting cases that break go generated code. It is not about addressing all non-style-compliant code. Case sensitivity is pretty unique to go. Almost every other language can generate service and method names as is and call it a day. They might not like the style of generated code, but it will work nevertheless.
Thank you 😊 |
I figured a better 2 LOC check. conf := &types.Config{Importer: importer.Default()}
_, err = conf.Check("", fset, []*ast.File{fileAST}, nil) Is it worth it now? |
That snippet tells us that the code doesn't compile. Can you explain how that's better UX than the Go toolchain not compiling the code? To my knowledge, the |
Codebases with multiple languages usually have a centralised step to generate code in multiple languages from proto files. Right now, the code gen succeeds and breaks during import. Failing at build time will prevent such code from being released at all. As an example, think of places where services are in other languages but also have generated go clients etc. They would release a new service before ever realising that the generated code will break in go. The most common case I can think of is when people are switching from non-compliant code to compliant one.
Failing at build time and printing the error with a documentation link which explains this gotcha is much better than leaving people in the dark to discover on their own. The crux is that a build time error would avoid a lot of work later. |
I agree with @ofpiyush here. The earlier a break occurs in a complex build system, the better it is for everyone. We've been fighting with: enum MyType {
MY_TYPE_UNSPECIFIED = 0;
name = 1;
address = 2;
} causing a name conflict between the generated const definition of: const (
MyType_MY_TYPE_UNSPECIFIED MyType = 0
MyType_name MyType = 1
MyType_address MyType = 2
) and the later generated definition of: // Enum value maps for MyType.
var (
MyType_name = map[int32]string{
0: "MY_TYPE_UNSPECIFIED",
1: "name",
2: "address",
}
) All of our .proto files are in a central repo that produces packages for several different languages, including go. The repo has pretty tight linting, but in this particular case the author of the above had a good reason for turning off UPPER_SNAKE_CASE enforcement on the If you're worried about "breaking the world", why not make the generation time conflict check @ofpiyush proposed a flag, perhaps |
I think a central issue of dealing with non-style-compliant protobuf names and these sorts of name conflicts is that for the most part, either we would need to track all these package-global names ourselves, or import the whole Go toolchain to reparse the code we output to check for these issues. More ideally, anyone generating Golang protobuf code could just add a |
We already do this to format the code.
That is not ideal, it is a very non-obvious requirement to put on one's user. It would still somewhat be palatable it if we had precedence with saying "you need to run The following is not meant to be a dig, just a reflection on the hilarious situation we're all in. More to lighten the atmosphere on this thread than anything else: Today
|
Closing as "working as intended". It's appropriate for us to the rely on the Go toolchain to reject generated code that has conflicts. In working in the |
What version of protobuf and what language are you using?
// protoc-gen-go v1.25.0-devel
// protoc v3.12.4
What did you do?
Tried to generate this file.
What did you expect to see?
Some solution on how protoc-gen-go resolves/avoids conflict in name conversion.
What did you see instead?
Anything else we should know about your project / environment?
Twirp is trying to resolve naming convention issues in go.
Specifically, how to handle service and method names that begin with a lower case letter.
What should they do if it happens to conflict with another method or service. twitchtv/twirp#269
I figured this project would've probably solved this issue with message names. Sadly. it does not.
The text was updated successfully, but these errors were encountered: