Fixed parsing for string->double maps. (#243)

Map parsing/serializing relies on map entries always
having a predictable order.  The code that generates
layout was not respecting this in the case of string
keys and primitive values.

BUILD

@@ -354,11 +354,24 @@ cc_test(
     ],
 )
 
+proto_library(
+    name = "test_proto",
+    testonly = 1,
+    srcs = ["tests/test.proto"],
+)
+
+upb_proto_library(
+    name = "test_upbproto",
+    testonly = 1,
+    deps = [":test_proto"],
+)
+
 cc_test(
     name = "test_generated_code",
     srcs = ["tests/test_generated_code.c"],
     deps = [
         ":test_messages_proto3_proto_upb",
+        ":test_upbproto",
         ":upb_test",
     ],
 )
@@ -657,6 +670,7 @@ cc_test(
         "@com_google_protobuf//:descriptor_proto",
         ":descriptor_proto_lua",
         ":test_messages_proto3_proto_lua",
+        ":test_proto_lua",
         "tests/bindings/lua/test_upb.lua",
         "third_party/lunit/console.lua",
         "third_party/lunit/lunit.lua",
@@ -682,9 +696,14 @@ cc_binary(
     ],
 )
 
+lua_proto_library(
+    name = "test_proto_lua",
+    testonly = 1,
+    deps = [":test_proto"],
+)
+
 lua_proto_library(
     name = "descriptor_proto_lua",
-    visibility = ["//visibility:public"],
     deps = ["@com_google_protobuf//:descriptor_proto"],
 )
 

tests/bindings/lua/main.c

@@ -27,8 +27,10 @@ const char *init =
     "./third_party/lunit/?.lua;"
     "external/com_google_protobuf/?.lua;"
     "external/com_google_protobuf/src/?.lua;"
+    "bazel-bin/?.lua;"
     "bazel-bin/external/com_google_protobuf/src/?.lua;"
     "bazel-bin/external/com_google_protobuf/?.lua;"
+    "bazel-bin/external/com_google_protobuf/?.lua;"
     "upb/bindings/lua/?.lua"
   "'";
 

tests/bindings/lua/test.proto

@@ -0,0 +1,7 @@
+syntax = "proto2";
+
+package lua_test;
+
+message TestLua {
+  map<string, double> map_string_double = 1;
+}

tests/bindings/lua/test_upb.lua

@@ -1,6 +1,7 @@
 
 local upb = require "lupb"
 local lunit = require "lunit"
+local upb_test = require "tests.test_pb"
 local test_messages_proto3 = require "google.protobuf.test_messages_proto3_pb"
 local descriptor = require "google.protobuf.descriptor_pb"
 
@@ -68,6 +69,32 @@ function test_msg_map()
   assert_equal(12, msg2.map_int32_int32[6])
 end
 
+function test_string_double_map()
+  msg = upb_test.MapTest()
+  msg.map_string_double["one"] = 1.0
+  msg.map_string_double["two point five"] = 2.5
+  assert_equal(1, msg.map_string_double["one"])
+  assert_equal(2.5, msg.map_string_double["two point five"])
+
+  -- Test overwrite.
+  msg.map_string_double["one"] = 2
+  assert_equal(2, msg.map_string_double["one"])
+  assert_equal(2.5, msg.map_string_double["two point five"])
+  msg.map_string_double["one"] = 1.0
+
+  -- Test delete.
+  msg.map_string_double["one"] = nil
+  assert_nil(msg.map_string_double["one"])
+  assert_equal(2.5, msg.map_string_double["two point five"])
+  msg.map_string_double["one"] = 1
+
+  local serialized = upb.encode(msg)
+  assert_true(#serialized > 0)
+  local msg2 = upb.decode(upb_test.MapTest, serialized)
+  assert_equal(1, msg2.map_string_double["one"])
+  assert_equal(2.5, msg2.map_string_double["two point five"])
+end
+
 function test_msg_string_map()
   msg = test_messages_proto3.TestAllTypesProto3()
   msg.map_string_string["foo"] = "bar"

tests/test.proto

@@ -1,68 +1,8 @@
 
-// A series of messages with various kinds of cycles in them.
-//      +-+---+    +---+    +---+
-//      V |   |    V   |    V   |
-// A -> B-+-> C -> D---+--->E---+
-// ^          |`---|--------^
-// +----------+----+        F
-
 syntax = "proto2";
 
-message A {
-  optional B b = 1;
-}
-
-message B {
-  optional B b = 1;
-  optional C c = 2;
-}
-
-message C {
-  optional A a = 1;
-  optional B b = 2;
-  optional D d = 3;
-  optional E e = 4;
-}
-
-message D {
-  optional A a = 1;
-  optional D d = 2;
-  optional E e = 3;
-}
-
-message E {
-  optional E e = 1;
-}
-
-message F {
-  optional E e = 1;
-}
-
-// A proto with a bunch of simple primitives.
-message SimplePrimitives {
-  optional fixed64 u64 = 1;
-  optional fixed32 u32 = 2;
-  optional double dbl = 3;
-  optional float flt = 5;
-  optional sint64 i64 = 6;
-  optional sint32 i32 = 7;
-  optional bool b = 8;
-  optional string str = 9;
-
-  oneof foo {
-    int32 oneof_int32 = 10;
-    string oneof_string = 11;
-  }
-
-  oneof bar {
-    int64 oneof_int64 = 13;
-    bytes oneof_bytes = 14;
-  }
+package upb_test;
 
-  message Nested {
-    oneof foo {
-      int32 oneof_int32 = 10;
-      string b = 11;
-    }
-  }
+message MapTest {
+  map<string, double> map_string_double = 1;
 }

tests/test_generated_code.c

@@ -5,6 +5,7 @@
 
 #include "src/google/protobuf/test_messages_proto3.upb.h"
 #include "tests/upb_test.h"
+#include "tests/test.upb.h"
 
 const char test_str[] = "abcdefg";
 const char test_str2[] = "12345678910";
@@ -114,6 +115,25 @@ static void check_string_map_one_entry(
   ASSERT(!const_ent);
 }
 
+static void test_string_double_map() {
+  upb_arena *arena = upb_arena_new();
+  upb_strview serialized;
+  upb_test_MapTest *msg = upb_test_MapTest_new(arena);
+  upb_test_MapTest *msg2;
+  double val;
+
+  upb_test_MapTest_map_string_double_set(msg, test_str_view, 1.5, arena);
+  serialized.data = upb_test_MapTest_serialize(msg, arena, &serialized.size);
+  ASSERT(serialized.data);
+
+  msg2 = upb_test_MapTest_parse(serialized.data, serialized.size, arena);
+  ASSERT(msg2);
+  ASSERT(upb_test_MapTest_map_string_double_get(msg2, test_str_view, &val));
+  ASSERT(val == 1.5);
+
+  upb_arena_free(arena);
+}
+
 static void test_string_map() {
   upb_arena *arena = upb_arena_new();
   protobuf_test_messages_proto3_TestAllTypesProto3 *msg =
@@ -323,6 +343,7 @@ void test_repeated() {
 int run_tests(int argc, char *argv[]) {
   test_scalars();
   test_string_map();
+  test_string_double_map();
   test_int32_map();
   test_repeated();
   return 0;

upb/def.c

@@ -921,6 +921,32 @@ static bool make_layout(const upb_symtab *symtab, const upb_msgdef *m) {
   l->fields = fields;
   l->submsgs = submsgs;
 
+  if (upb_msgdef_mapentry(m)) {
+    /* TODO(haberman): refactor this method so this special case is more
+     * elegant. */
+    const upb_fielddef *key = upb_msgdef_itof(m, 1);
+    const upb_fielddef *val = upb_msgdef_itof(m, 2);
+    fields[0].number = 1;
+    fields[1].number = 2;
+    fields[0].label = UPB_LABEL_OPTIONAL;
+    fields[1].label = UPB_LABEL_OPTIONAL;
+    fields[0].presence = 0;
+    fields[1].presence = 0;
+    fields[0].descriptortype = upb_fielddef_descriptortype(key);
+    fields[1].descriptortype = upb_fielddef_descriptortype(val);
+    fields[0].offset = 0;
+    fields[1].offset = sizeof(upb_strview);
+    fields[1].submsg_index = 0;
+
+    if (upb_fielddef_type(val) == UPB_TYPE_MESSAGE) {
+      submsgs[0] = upb_fielddef_msgsubdef(val)->layout;
+    }
+
+    l->field_count = 2;
+    l->size = 2 * sizeof(upb_strview);align_up(l->size, 8);
+    return true;
+  }
+
   /* Allocate data offsets in three stages:
    *
    * 1. hasbits.

upbc/message_layout.cc

@@ -32,12 +32,7 @@ bool MessageLayout::HasHasbit(const protobuf::FieldDescriptor* field) {
 
 MessageLayout::SizeAndAlign MessageLayout::SizeOf(
     const protobuf::FieldDescriptor* field) {
-  if (field->containing_type()->options().map_entry()) {
-    // Map entries aren't actually stored, they are only used during parsing.
-    // For parsing, it helps a lot if all map entry messages have the same
-    // layout.
-    return {{8, 16}, {4, 8}};  // upb_stringview
-  } else if (field->is_repeated()) {
+  if (field->is_repeated()) {
     return {{4, 8}, {4, 8}};  // Pointer to array object.
   } else {
     return SizeOfUnwrapped(field);
@@ -50,7 +45,7 @@ MessageLayout::SizeAndAlign MessageLayout::SizeOfUnwrapped(
     case protobuf::FieldDescriptor::CPPTYPE_MESSAGE:
       return {{4, 8}, {4, 8}};  // Pointer to message.
     case protobuf::FieldDescriptor::CPPTYPE_STRING:
-      return {{8, 16}, {4, 8}};  // upb_stringview
+      return {{8, 16}, {4, 8}};  // upb_strview
     case protobuf::FieldDescriptor::CPPTYPE_BOOL:
       return {{1, 1}, {1, 1}};
     case protobuf::FieldDescriptor::CPPTYPE_FLOAT:
@@ -111,8 +106,18 @@ int64_t MessageLayout::FieldLayoutRank(const protobuf::FieldDescriptor* field) {
 void MessageLayout::ComputeLayout(const protobuf::Descriptor* descriptor) {
   size_ = Size{0, 0};
   maxalign_ = Size{0, 0};
-  PlaceNonOneofFields(descriptor);
-  PlaceOneofFields(descriptor);
+
+  if (descriptor->options().map_entry()) {
+    // Map entries aren't actually stored, they are only used during parsing.
+    // For parsing, it helps a lot if all map entry messages have the same
+    // layout.
+    SizeAndAlign size{{8, 16}, {4, 8}};  // upb_strview
+    field_offsets_[descriptor->FindFieldByNumber(1)] = Place(size);
+    field_offsets_[descriptor->FindFieldByNumber(2)] = Place(size);
+  } else {
+    PlaceNonOneofFields(descriptor);
+    PlaceOneofFields(descriptor);
+  }
 
   // Align overall size up to max size.
   size_.AlignUp(maxalign_);