-
Notifications
You must be signed in to change notification settings - Fork 443
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
Fetching a optional/required field doesn't return the default value #24
Comments
Do you have a repro case where this isn't happening today? There is logic in the generator to do exactly what you're describing, so it sounds like you've found a bug. |
See https://github.com/apple/swift-protobuf/blob/master/Reference/google/protobuf/descriptor.pb.swift#L283, for example. Since the type of Have you implemented other behavior if a different explicit value is provided for the default, rather than the zero/empty value? If not, there's currently no way to get at that default if you return In either case, these properties should not really be Swift optionals, because they always return something; the protocol buffer spec defines that querying a field that is not set returns the default value, not |
https://github.com/apple/swift-protobuf/blob/master/Reference/google/protobuf/descriptor.pb.swift#L598-L600 is another example for non strings. |
Descriptor.proto defines that field as:
So it does not have a default and is exposed as a Swift Optional. If there's an explicit default, the generated code is slightly different. For example, further down in descriptor.proto, you can see:
which generates https://github.com/apple/swift-protobuf/blob/master/Reference/google/protobuf/descriptor.pb.swift#L3055
You can see some of the test cases for this behavior (including verifying nil-resettability) at https://github.com/apple/swift-protobuf/blob/master/Tests/SwiftProtobufTests/Test_AllTypes.swift#L1342 |
It's not very intuitive to have the type, and thus the call site usage, of the property change depending on whether there is a default value or not. Worse, it also means you can't change the default value of a field (from one with to one without, or vice versa) without compile-time breakage. Plus, all fields in protobuf have default values; it's just that if they're not explicitly defined, they are the zero/empty value for that type. |
As @allevato mentions there always is a default, the difference is proto2 syntax allows a custom default (https://developers.google.com/protocol-buffers/docs/proto#optional). It actually get more complex with proto2 syntax enums, as the "default" is the first value listed, so it doesn't have to be zero, and if you checked the C++ version of descriptor, the has bit isn't set, but fetching the value does properly return the non zero value. This is one of those cases where the subject at hand (protos) has a concept of "optional" (and required), but it doesn't really line up with the concepts that Swift have that share the words. |
I should also add, I've seen a lot of proto files where folks do: optional string name = 1 [default = ""];
optional int32 age = 2 [default = 0]; Even though it was already the default. So keying off that is going to confuse the developers that realize there always is a default. |
A nit:
You can't change it without wire breakage anyway (*), so that might even be a good thing :) (*) (Except for adding or removing redundant |
@jcanizales let's not derail this issue, but changing the default doesn't break the wire format. yes, it isn't always the best thing to do, but it can be done. If you want to chat more, hit me up outside of here. |
Then we'd still have a situation where two functionally equivalent protos (one with I'd still argue for generated API consistency here—users expect to be able to query properties and get a default value back, even if it's zero/empty/first-enum-value. If and when Swift introduces some notion of "resettable properties" or "property behaviors" that have been discussed on swift-evolution, there will be a much more elegant way to model this, but I'm not convinced that the current design is the right one in Swift as it is implemented today. |
Compile-time breakage isn't entirely a bad thing if it points the developer to reconsider something that may have affected their code. |
I've spent a good portion of today working on this. It's been a challenge, since it touched some deep logic in the generator w.r.t. whether properties are generated as optionals or not, in the storage class or not, and so forth. Then changes to the generator cause the descriptor proto to change when I generated it, so I have to update the generator to use the updated proto, and so forth 😱 I'm finally back to a place where the code generates what I want, which is:
That's all working and all the old tests are (finally!) passing, with one exception: From what I can tell, it would be a fairly significant burden to get these remaining tests to pass under the new design, and since they're verifying incorrect behavior anyway, I'm inclined to just disable them so that I can clean up this branch and create a PR. Thoughts? |
You don't mentioned it, but I'm hoping maps are treated the same as repeated fields from the pov of As long as we've got issues open for the things needing fixing, I'm fine commenting out tests with pointers to the issues so they get followed up on asap. |
Correct, maps are treated the same as repeated fields (no |
Likewise, individual I filed #98 to track the required field issues. |
Increase test coverage
Like #22, but for fields not in a oneof, fetching an optional/required field should return the default value is it hasn't been set yet. In proto2 syntax, the default isn't the zero, so this becomes even more important.
The text was updated successfully, but these errors were encountered: