Codegen: required fields are not required

[koivunej]

Testcases can be found at end of gist: https://gist.github.com/koivunej/6efeecef4f251685b0e032245c1dc7dc

It'd seem that the currently emitted code does not validate that required field are set or become serialized. I would assume this required means "must be found" in the protobuf sense? With the current implementation, validating required fields becomes users responsibility.

Preferred solution would be to generate structs that ensure that required fields are in fact there. This means dropping the use of Option<T> for required fields, also dropping deriving Default. This will make the (de)serialization code more complicated having to define all fields first as local variables as Option<T> or T depending on whether they are Default types and then finally move them into struct.

Next thing that comes to mind is: can the same tag appear multiple times while deserializing? I would expect that to be an error, but the current while loop will not fail that.

[koivunej]

Started looking root cause for this in https://github.com/koivunej/quick-protobuf/tree/required_fail where there are some codegen/parser tests and refactorings. Figured out that when rendering the from_reader function the field in gist example has Frequency::Optional but it seems to parse out ok.

[tafia]

There are several checks probably not implemented yet.
For the required fields I'd like to have these checks optional (and default to the current behavior).

My main motivation is

• the performance penalty can be quite significant
• proto3 dropped support for required fields, which means (to me) that the 'required/unique' feature is not something very important in practice

[koivunej]

proto3 dropped support for required fields

I was not aware of this, and it certainly changes things making this a really low priority nice-to-have/wishlist kind of issue.

I think this issue could be kept open in case someone else comes wondering the same thing, or migrated into the existing checklist in #12? Not sure what could be a better/more discoverable way to document this.