Bug: semantically incorrect elision of empty messages

[sharrap]

(Originally posted here)

According to the proto3 spec, the default value of a message is 'not set'. However, this code elides empty (but set) maps, essentially interpreting nil and {} as identical. On top of being generally incorrect (the receiver can't distinguish between the two despite the two being semantically different according to the spec), this has a nasty interaction with oneof:

syntax = "proto3";

package test;

message A {
  int32 i = 1;
}

message E {}

message M {
  oneof oo {
    A a = 1;
    E b = 2;
  }
  int32 c = 3;
}

user=> (require '[protojure.protobuf :as pb] '[test :as t])
nil
user=> (test/pb->M (pb/->pb (test/new-M {:oo {:a {:i 1}} :c 1})))
#test.M-record{:oo {:a #test.A-record{:i 1}}, :c 1}
user=> (test/pb->M (pb/->pb (test/new-M {:oo {:b {}} :c 1})))
#test.M-record{:oo nil, :c 1}

Dropping the length check in the linked code fixes the issue. I'm happy to create a PR, but I figured I'd be a little more cautious here since it seems like this check was put there intentionally and it would be good to know if there's a reason it's done that way or if it's just an oversight. (I checked the git history but this dates back to "initial public release")

[ghaskins]

@sharrap I suspect that the issue was introduced in an overzealous attempt to add the :optimize feature, not something specifically targeting the described behavior. Unfortunately, I no longer have access to the original git commits from the private repository prior to the public release, so I can't dig further. But its ok: i think we can just move forward. If you are willing to make a proposal/PR, I'd be happy to review.