-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
[C-Sharp] Inconsistent generated code for wrapper primitive types and new proto3 optional
keyword
#9352
Comments
CC @jskeet @haberman @moserware @jskeet and @haberman will probably know more about why we chose the approach we did for proto3 optional. |
... which we do, to avoid breaking everyone who already uses it, including the Google Cloud client libraries. Note that support for optional in proto3 isn't really "very new" - it's been in the protoc for C# since April 2020. It's worth noting that in the current design, adding I would be strongly against changing the default behavior. I would be moderately against introducing a plugin option for this, as I expect it will make the plugin code significantly more complex, and have repercussions on the library support as well. As I'm not on the protobuf team it's not my decision though. |
I agree with @jskeet on all points. I would add that a primary goal of protobuf is to have consistent semantics across languages. One of those semantics is that For protos with A lot of users request |
So why was the And yes agree there is a difference in the on the wire representation of the wrapper object vs the nullable field, but technically making a field go from nullable to not nullable or visa versa IS a breaking change or at least should be considered that from the point of view of your code base. I would want it if I changed that field that everywhere it used to compile would now need review with a compiler warning which is exactly what would happen in C# if you did that and the field actually became a nullable field. It doesn't break your code, but it will warn you that there might be an issue now. However because you don't change anything about the property when you change it from non-optional, to optional in the current implementation, you negate that whole benefit. You also argued my case in the last point.
The C# implementation of Protobuf DOES use nullable semantics because C# is a language that supports it. I'm just saying that the intent of an "optional" field is exactly that, a nullable type. So C# should use nullable types there. You're already doing it for the wrapper types! While I might agree that this could be considered a breaking change and should be gated behind some option, it should still be available. It's logically the correct answer even if it does cause differentiation between the proto2 and proto3 representations. There is already differences between them. If google recommends you don't use the well known wrapper types any more in exchange for using the new "optional" keyword, then if I convert my proto files from using wrapper types to optional keywords, they should generally function similarly in the generated code. |
Unfortunately that ship has long sailed. Protobuf.js for instance went with
This is the page where google recommends getting rid of wrapper types, and when it says that most libraries can't even tell the difference, it's clear that factors into their motivation for their recommendation. The fact that C# can tell the difference makes it unusual, but worse C# doesn't allow migrating from one to the other. If your API is using wrappers and you want to switch to optional, you not only have to worry about serialization changes (which are to be expected and can be handled by renaming proto fields) but you have to worry about rewriting all your code, removing any situation where you use an expression to create a protobuf. It's incredibly odd to see the library massively changed it's mind on how to handle semantically equivalent things, especially when they new way is more awkward to use, doesn't play well with core language ideas, and comes at a time where the downsides of using I understand not wanting to introduce breaking changes, but that happened when |
As documented in:
https://developers.google.com/protocol-buffers/docs/reference/csharp-generated#wrapper_types
Fields that use the well known wrapper types (i.e. Int32Value, StringValue etc) generate as the primitive type but as a nullable field. I love this behavior. Most other languages did not choose this implementation which in my opinion is the simplest, most maintainable, and easy to use.
The new "optional" keyword introduced in 3.14 behaves differently where the generated property is the primitive type but not nullable and has generated "HasX" property and "ClearX" method.
So for example
Generates the following class (simplified for brevity and clarity):
There is no documentation in the C# generated code reference about the difference in behaviors (or really anything about the behavior of the "optional" keyword).
I would highly prefer that the optional keyword generated identical code to the wrapper types as their intent is identical.
Google API design guidelines even says that you should not use wrapper types anymore in favor of optional keyword: https://cloud.google.com/apis/design/design_patterns.md#optional-primitive-fields
I propose changing the behavior of the protoc plugin for dotnet to generate the same code for both methods of specifying optional primitive fields.
If for some reason we need to avoid the breaking change for the default behavior, there should be a plugin option to change the behavior so they do generate identical code (or at least code that is identical in its interface) however as the optional support in proto3 is very new, it's unlikely to break existing code.
The text was updated successfully, but these errors were encountered: