Generated e_msg_X functions do not correctly match optional terms (in maps mode) about gpb HOT 9 CLOSED

This is a consequence of the way map support is currently implemented: a generic mapping that will be rendered into either record syntax or map syntax. So, since optional fields are always present in records, having the value undefined if not set, this is currently inherited to maps too. But I think you're right, now with maps, we have one more option: to allow non-set optional fields to be missing altogether. It also nicely solves the issue with an enum having a symbol that is the text undefined. This has never been handled very well, but with maps, it could. So I'm definitely in favour of fixing this, and will take a look at it.

I'm also thinking about backwards compatibility, whether we should still allow non-set optional fields to be present and have the value undefined, like in records, but I think I'm against it. It will break backwards compatibility, but on the other hand, map support is still flagged as an area that is settling down, so perhaps with deprecation and a transition period.

from gpb.

Cool, thanks for the information. :) Yeah, I agree that it would be cleanest to disallow using undefined to declare non-set optional fields, as a long-term solution. Your idea to deprecate that kind of usage and set up a transition period sounds reasonable to me.

from gpb.

I thought some more about it: to keep symmetry between encoding and decoding, this would also mean that on decoding, if the field is not in the binary to decode, then it should be omitted from the decoded result. This means also decoding and merging needs to change accordingly, in the addition to encoding and verification that was already identified. The new (erlang-)internal representation of an unset optional field would be that it is omitted from the map, rather than present and with the value undefined.

At first, I thought it would be enough to let the encoder just accept both undefined and the field not being present in the map, but that is no good if one wants decoding to be symmetric, so I think the internal representation really must change. (This is also better in the long run, as discussed previously.) But this means it is a non backwards compatible change, so I think there must first be an option, and on some later announced date or version, we can change the default value of the option.

Since also decoding and merging need to change it'll take some more time, so I'll merge the #28 to master meanwhile, and this will make it into some later release.

from gpb.

Sounds good to me. :)

from gpb.

I thought maybe I should give some update on the progress of this. I have code on a branch. I think think it works, but I faced two issues that has made it slow:

First thing is that the code ended up looking just too ugly, I'll have to dig into it again to try to reduce it into some kind of maintainable state, trying to avoid a case explosion. I have ideas for improvements, that I think are promising, but haven't had much time lately to dive into it.

Second thing is that the maps are getting to a point where they are needing tests. Up until about now, what-ever works with records has surely worked with maps too, since it was only a rendering issue, whether to render a record field update or a map update at generation time, but I think now that it is time for more proper unit testing. The thing here, though, is that I want the unit tests to work out of the box also with a pre-17 Erlang, so will have to invent something to that end.

from gpb.

Sounds good, thanks for the update!

from gpb.

Finally, the code's in better shape, and I've pushed the branch to github, unset-opt-fields-in-maps-omitted, in case you would want to take it out for a spin. Sorry about the delay.

This introduces a new option {maps_unset_optional, omitted | present_undefined} (same on the command line, but with dash instead of as a tuple. Default is present_undefined, but setting it to omitted will make the generated code behave like in this issue.

In case nothing bad surfaces, I'll merge it to master and push within a few days.

from gpb.

Thanks! I quickly tested out the new branch and everything seems to work as expected. :)

from gpb.

Thanks for quick feedback! Merged into 3.18.0. Closing

from gpb.