Message fields should use nullable types (eg ref object), not object

[timotheecour]

Currently message fields use object:

cf https://github.com/PMunch/protobuf-nim/issues/7#issuecomment-377592911

Since all of these are added as objects and not ref object all object fields are initialised to their Nim default value

This is not correct, and besides space concerns (which are very real in larger examples), it makes it impossible to use self referential protos, eg:

when false:
  const protoSpec = """
  syntax = "proto3";
  message Node {
    int32 payload = 1;
    Node node1 = 2;
    Node node2 = 3;
  }
  """
  parseProto(protoSpec)
  var msg: Node

nim c -o:tmptest -p:$git_clone_D/nim/combparser/src -r example3.nim gives: Error: unhandled exception: Type not recognized: Node.Node

Note, this is a valid proto as you can see:

-- foo.proto
syntax = "proto3";
message Node {
  int32 payload = 1;
  Node node1 = 2;
  Node node2 = 3;
}

-- main.cpp
#include <stdio.h>
#include "foo.pb.h"
void test(){
  Node node;
  node.mutable_node1()->mutable_node2()->set_payload(100);
  printf("res=%d\n", node.node1().node2().payload());
}
int main (int argc, char *argv[]) {
  test();
  return 0;
}

--- build.sh
set -u
mkdir -p build
protoc --cpp_out=build foo.proto
clang++ -o build/test2 -Ibuild -I$homebrew_D/include -L$homebrew_D/lib -lprotobuf build/foo.pb.cc test2.cpp
./build/test2

proposed fix

use reference types (or maybe Nullable[T] if there's such a thing in nim) as a 2nd priority, making sure https://github.com/PMunch/protobuf-nim/issues/7 still works (ie allow msg.nested.aField = 100 without initializing msg.nested)

[PMunch]

Yeah, I realised after responding to your previous issue that self referential or circularly referential types would not work. This could be fixed but it would break the setter and getter things we discussed earlier. But I guess that's worth it for compliance. And could possibly be fixed later.

[timotheecour]

yes, supporting getter and setter is definitely not as high priority as supporting valid protos (with referential msgs), and can be fixed later. happy to discuss details :)

[PMunch]

As of https://github.com/PMunch/protobuf-nim/commit/c90566eb697b5ee5d51214b3fe54911fdd0f50f5 all messages are now ref objects. But missing a field causes an error on write, since all fields are actually optional I guess they should rather be wrapped in some Optional type. Oh well, something to do for tomorrow..

[PMunch]

I've looking into this extensively now. The optional type from Options makes the entire thing quite clumsy to use, so the current approach I'm pursuing is to use the experimental . and .= methods in order to get and set fields while marking in a bitmap if they are set or not. This almost works, but it's still a bit fragile. I kinda wanted to stay away from too much special syntax like having to do:

myProtoObject.setField(someField, 100)

You can see my test file for what I want to create here: https://github.com/PMunch/protobuf-nim/blob/optionmessages/newopts.nim#L170. There are still some issues, mostly from being able to return both mutable and immutable values. I hope I can sort it out though.

[PMunch]

Finally got something that works pretty well now and merged it into the master branch. All fields now track if they are set or not, and only those that are set are written. If you try to read an unset field you get a runtime ValueError. You can use has to check if a field is set in a type, and reset to remove a previously set field. See example 2 and example 3 for more on this.