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

proto3 'optional' keyword support #523

Open
zs-dima opened this issue Jul 15, 2021 · 14 comments
Open

proto3 'optional' keyword support #523

zs-dima opened this issue Jul 15, 2021 · 14 comments

Comments

@zs-dima
Copy link

zs-dima commented Jul 15, 2021

Currently optional Timestamp and optional Duration generate non-nullable types.
However proto3 'optional' keyword means nullable types.

Could be nice to fix this issue to be able to have nullable and non-nullable proto3 types processed correctly.

protocolbuffers/protobuf#2168
protocolbuffers/protobuf#8822

Thanks in advance

@zs-dima zs-dima changed the title Nullable google.protobuf Timestamp and Duration proto3 'optional' keyword support Jul 26, 2021
@tomasweigenast
Copy link

Any update?

@osa1 osa1 self-assigned this Apr 7, 2022
@osa1
Copy link
Member

osa1 commented Apr 7, 2022

However proto3 'optional' keyword means nullable types.

@zs-dima Do you have the source for that? I'm aware of two design documents related to optional in proto3:

As far as I can see none of these say that optional means the value needs to be nullable. You should be able to check for an optional field's presence, and in protobuf.dart we provide this with hasX methods (where X is the field name).

@osa1
Copy link
Member

osa1 commented Apr 7, 2022

Support for optional syntax in proto3 files was implemented in #444 (merged as 1319847).

@wb14123
Copy link

wb14123 commented Apr 7, 2022

@osa1 hasX is helpful for reading the fields. However it doesn't provide much type safe. I think make the type nullable would be much more handy.

I'm also wondering the writing of optional fields. If I want a structure with an optional field to be null, do I just init the structure without assign that field? And if I want to update a field from not null value to null value, how can I do it?

@zs-dima
Copy link
Author

zs-dima commented Apr 7, 2022

@osa1 thanks a lot for looking into
nullable - means that optional could has or could not has value
therefore hasX properties are added - to check if nullable field has value or not.

@ryuujo1573
Copy link

ryuujo1573 commented Apr 19, 2022

However proto3 'optional' keyword means nullable types.

@zs-dima Do you have the source for that? I'm aware of two design documents related to optional in proto3:

As far as I can see none of these say that optional means the value needs to be nullable. You should be able to check for an optional field's presence, and in protobuf.dart we provide this with hasX methods (where X is the field name).

Many thanks but sorry to be here to mention:
We should not use the hasFoo property in languages of null-safety, and instead, we use much simpler syntax which is like:

Foo? get foo => $_getIZ(2);
// ...
foo?.doSomething(); // this conditionally calls the function only if `foo` is not null.

which is in place of

if (hasFoo) {
    foo.doSomething();
}

Also, we can benefit from Foo? syntax when initializing models for response, e.g.

var msg = CameraImageMessage()
  ..width = rawImage.width
  ..height = rawImage.height;
msg.planes
  ..clear()
  ..addAll(rawImage.planes.map((e) => CameraImageMessage_Plane()
    ..height = e.height // this errors when setter of field in ⬆️Cam...Plane receives no nullable value.
    ..width = e.width
    ..data = e.bytes
    ..bytesPerPixel = e.bytesPerPixel
    ..bytesPerRow = e.bytesPerRow));

With property foo (setter/getter) being type Foo?, we can easily initialize generated models with corresponding nullable sources.

To my understandings, the default in proto3 is singular which is equivalent to nullable. (zero for null, and one for normal T)

Thank you.

@osa1
Copy link
Member

osa1 commented Jun 24, 2022

To answer my own question and elaborate, the field presence spec clearly specifies that the default value for message fields should be null:

When serializing, fields with no presence are not serialized if they contain their default value.
...
For messages, the default is the language-specific null value.

So yes, we should use nullable return types for getters in fields with message types, and return null when the field is not present. #309 is another report of this issue.

@daviddomkar
Copy link

I would also like to have the optional types nullable. So far this is the most anoying thing with this package I came across. It is so easy to make a mistake to not check if the value exist with the hasX() method. Native dart null safety would be a much better approach.

@talksik
Copy link

talksik commented Dec 6, 2022

how have people worked around it? it's super annoying because I have nested objects in a protobuf response and so I have to do a nest hasX check as far as I know

@ghost
Copy link

ghost commented Dec 29, 2022

Hello everyone! 👋

I want to try to add support for nullable types, and I would like to know if it's better :

  1. to add an option to the generator to choose between hazzers or nullable types
  2. to replace hazzers with nullable types
  3. not to add this feature in this package

Thanks a lot in advance !

@zs-dima
Copy link
Author

zs-dima commented Dec 29, 2022

@johynpapin option 1 looks nice to support (temporary) old projects.
It could be nice if nullable types will be the default generator option.
Could be nice to see some more details about solution.

@ghost
Copy link

ghost commented Dec 29, 2022

@zs-dima I'd be really happy to discuss this and get some advice, and see if it's possible to break this functionality down into steps.

@osa1 osa1 removed their assignment Dec 29, 2022
@ryuujo1573
Copy link

ryuujo1573 commented Dec 30, 2022

@johynpapin Hi there!
I prefer the 2nd option, since hasX is not hard to migrate to nullable X.

However I'm sad to say I haven't been on this for a long time, but if possible, I'd like to help out.
Thank you in advance.

@ghost
Copy link

ghost commented Jan 5, 2023

@ryuujo1573 I will start to develop, and if at some point I see a point on which it would be possible to work together, I will contact you. 😄 (But maybe it's not a big enough change to be done by several people initially.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants