msoucy / dproto

D Protocol Buffer mixins to create structures at compile time
Boost Software License 1.0
37 stars 16 forks source link

Optional members of message type should be nullable #63

Closed cpconduce closed 8 years ago

cpconduce commented 8 years ago

Per the language spec for both v2 and v3, the default value for a message type is null. The new attribute-based code uses T.init as the default value for these types.

msoucy commented 8 years ago

I've started working on this in the nullable branch - the tests don't currently pass, I'm trying to figure out what I'll need to change to make it work sanely.

msoucy commented 8 years ago

@cpconduce - if you don't mind, there is a LOT required to change in order to handle the expected way. If there's any changes you can contribute so that I can get this taken care of sooner, it would be rather helpful.

cpconduce commented 8 years ago

As near as I can tell, the outstanding TODOs are 1) not wrapping builtin types in Nullable and 2) not wrapping enum types in Nullable, and 3) make sure that all types have the correct default values based either on the PB spec -- zeros, first enum member, empty list, etc.. Correct? The first is straightforward. The second two less so, especially without knowledge of other modules. I'm happy to dig in, but please let me know if I have the TODO list right.

msoucy commented 8 years ago

Sounds pretty accurate, though I did (most of) the first two on my branch already.

cpconduce commented 8 years ago

Ok. Looks like the most straightforward way would be to template default values by type and just include them everywhere where an existing default isn't specified. Is the nullable branch currently in your repo the most recent?

denizzzka commented 8 years ago

1) not wrapping builtin types in Nullable

Why?

cpconduce commented 8 years ago

The PB spec says that the default value for optional message types is null, but for builtin or enum types it's either specified by the user or empty string/zero/false/first enum member.

cpconduce commented 8 years ago

(Note that this is definitely a breaking change for dproto. Semver appropriately.)

msoucy commented 8 years ago

Closed via #66 and included in the v2.0.0 release that I'm currently writing release notes for.

Marenz commented 8 years ago

The PB spec says that the default value for optional message types is null, but for builtin or enum types it's either specified by the user or empty string/zero/false/first enum member.

just because the default value should be something doesn't mean the ability to check whether it was explicitly set or not should disappear. Even the official c++ library supports that for every type (using the has_foo() syntax)

What this change did for me is basically breaking everything.

cpconduce commented 8 years ago

You can use something like

bool exists(T)(auto ref T t) if(is(T == Nullable!U, U)) {
    return ! t.isNull();
}

for optional message types. For optional primitives you'd need to amend dproto to do per-struct accounting of each field set during deserialization, etc.

Marenz commented 8 years ago

I don't mind using isNull() if it would work.

For optional primitives you'd need to amend dproto to do per-struct accounting of each field set during deserialization, etc.

The fact that dproto doesn't support this anymore is the main problem here.

The question is also how that would look, syntactically. You can't really use Nullable anymore if you want to check for existence and want to have a default value when you access an unset field.

cpconduce commented 8 years ago

Protobuf 3.0 is solving this problem with somewhat-awkward Well Known Types. (Basically, if you want to check for the existence of an int, you need to use an int wrapped in a message. Otherwise it's all default values, all the time, and in 3.0 they won't be sent over the wire if they'r the default value.) Rolling something like that on your own is an (admittedly tedious) option.

An option for dproto is to add some compile-time optional accounting and auto-generate has_ methods for all optional types. That would probably also mean wrapping all of the fields in @property stanzas such that you could update the aforementioned accounting information at set time as well as at deserialize time. This is what the C++ implementation does at pretty minimal runtime cost. (One array of unt32 bitfields to track setness.)

Likely related: dproto 2.0 doesn't send optional value types over the wire if they're currently set to their default values. The 2.6.1 C++ implementation does. At quick glance I can't find anything particularly definitive in either of the 2.0 or 3.0 specification about whether or not set optional fields should or should not be sent over the wire when they're the default value. (Again, the C++ implementation does this.) It'd be worth making sure that dproto is consistent with the spec in this regard.

Marenz commented 8 years ago

Here is my suggestion to solve it: https://github.com/msoucy/dproto/pull/68