WIP.

python/map.c

@@ -114,7 +114,8 @@ void PyUpb_MapContainer_Invalidate(PyObject* obj) {
   self->version++;
 }
 
-static upb_map* PyUpb_MapContainer_AssureReified(PyUpb_MapContainer* self) {
+upb_map* PyUpb_MapContainer_EnsureReified(PyObject* _self) {
+  PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self;
   self->version++;
   upb_map* map = PyUpb_MapContainer_GetIfReified(self);
   if (map) return map;  // Already writable.
@@ -134,7 +135,7 @@ static upb_map* PyUpb_MapContainer_AssureReified(PyUpb_MapContainer* self) {
 int PyUpb_MapContainer_AssignSubscript(PyObject* _self, PyObject* key,
                                        PyObject* val) {
   PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self;
-  upb_map* map = PyUpb_MapContainer_AssureReified(self);
+  upb_map* map = PyUpb_MapContainer_EnsureReified(_self);
   const upb_fielddef* f = PyUpb_MapContainer_GetField(self);
   const upb_msgdef* entry_m = upb_fielddef_msgsubdef(f);
   const upb_fielddef* key_f = upb_msgdef_field(entry_m, 0);
@@ -166,7 +167,7 @@ PyObject* PyUpb_MapContainer_Subscript(PyObject* _self, PyObject* key) {
   upb_msgval u_key, u_val;
   if (!PyUpb_PyToUpb(key, key_f, &u_key, arena)) return NULL;
   if (!map || !upb_map_get(map, u_key, &u_val)) {
-    map = PyUpb_MapContainer_AssureReified(self);
+    map = PyUpb_MapContainer_EnsureReified(_self);
     upb_arena* arena = PyUpb_Arena_Get(self->arena);
     if (upb_fielddef_issubmsg(val_f)) {
       u_val.msg_val = upb_msg_new(upb_fielddef_msgsubdef(val_f), arena);
@@ -196,7 +197,7 @@ PyObject* PyUpb_MapContainer_Contains(PyObject* _self, PyObject* key) {
 
 PyObject* PyUpb_MapContainer_Clear(PyObject* _self, PyObject* key) {
   PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self;
-  upb_map* map = PyUpb_MapContainer_AssureReified(self);
+  upb_map* map = PyUpb_MapContainer_EnsureReified(_self);
   upb_map_clear(map);
   Py_RETURN_NONE;
 }

python/map.h

@@ -48,6 +48,9 @@ PyObject* PyUpb_MapContainer_GetOrCreateWrapper(upb_map* map,
 // Reifies a map stub to point to the concrete data in `map`.
 void PyUpb_MapContainer_Reify(PyObject* self, upb_map* map);
 
+// Reifies this map object if it is not already reified.
+upb_map* PyUpb_MapContainer_EnsureReified(PyObject* self);
+
 // Assigns `self[key] = val` for the map `self`.
 int PyUpb_MapContainer_AssignSubscript(PyObject* self, PyObject* key,
                                        PyObject* val);

python/message.c

@@ -326,7 +326,7 @@ err:
   return ret;
 }
 
-void PyUpb_CMessage_AssureReified(PyUpb_CMessage* self);
+void PyUpb_CMessage_EnsureReified(PyUpb_CMessage* self);
 
 static bool PyUpb_CMessage_InitMapAttribute(PyObject* _self, PyObject* name,
                                             const upb_fielddef* f,
@@ -396,7 +396,7 @@ int PyUpb_CMessage_InitAttributes(PyObject* _self, PyObject* args,
   Py_ssize_t pos = 0;
   PyObject* name;
   PyObject* value;
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   upb_msg* msg = PyUpb_CMessage_GetMsg(self);
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
 
@@ -493,7 +493,7 @@ static void PyUpb_CMessage_SetField(PyUpb_CMessage* parent,
 }
 
 /*
- * PyUpb_CMessage_AssureReified()
+ * PyUpb_CMessage_EnsureReified()
  *
  * This implements the "expando" behavior of Python protos:
  *   foo = FooProto()
@@ -510,7 +510,7 @@ static void PyUpb_CMessage_SetField(PyUpb_CMessage* parent,
  * Post-condition:
  *   PyUpb_CMessage_IsStub(self) is false
  */
-void PyUpb_CMessage_AssureReified(PyUpb_CMessage* self) {
+void PyUpb_CMessage_EnsureReified(PyUpb_CMessage* self) {
   if (!PyUpb_CMessage_IsStub(self)) return;
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
 
@@ -562,7 +562,7 @@ static void PyUpb_CMessage_Reify(PyUpb_CMessage* self, const upb_fielddef* f,
  *
  * This operation must be invoked whenever the underlying upb_msg has been
  * mutated directly in C.  This will attach any newly-present field data
- * to previously returned "empty" wrapper objects.
+ * to previously returned stub wrapper objects.
  *
  * For example:
  *   foo = FooMessage()
@@ -658,7 +658,7 @@ void PyUpb_CMessage_CacheDelete(PyObject* _self, const upb_fielddef* f) {
 void PyUpb_CMessage_SetConcreteSubobj(PyObject* _self, const upb_fielddef* f,
                                       upb_msgval subobj) {
   PyUpb_CMessage* self = (void*)_self;
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   PyUpb_CMessage_CacheDelete(_self, f);
   upb_msg_set(self->ptr.msg, f, subobj, PyUpb_Arena_Get(self->arena));
 }
@@ -808,7 +808,7 @@ int PyUpb_CMessage_SetFieldValue(PyObject* _self, const upb_fielddef* field,
     return -1;
   }
 
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
 
   upb_msgval val;
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
@@ -958,7 +958,7 @@ PyObject* PyUpb_CMessage_MergeFrom(PyObject* self, PyObject* arg) {
 
 static PyObject* PyUpb_CMessage_SetInParent(PyObject* _self, PyObject* arg) {
   PyUpb_CMessage* self = (void*)_self;
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   Py_RETURN_NONE;
 }
 
@@ -985,7 +985,7 @@ PyObject* PyUpb_CMessage_MergeFromString(PyObject* _self, PyObject* arg) {
     return NULL;
   }
 
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   const upb_msgdef* msgdef = _PyUpb_CMessage_GetMsgdef(self);
   const upb_filedef* file = upb_msgdef_file(msgdef);
   const upb_extreg* extreg = upb_symtab_extreg(upb_filedef_symtab(file));
@@ -1027,7 +1027,7 @@ static PyObject* PyUpb_CMessage_ByteSize(PyObject* self, PyObject* args) {
 }
 
 static PyObject* PyUpb_CMessage_Clear(PyUpb_CMessage* self, PyObject* args) {
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   const upb_msgdef* msgdef = _PyUpb_CMessage_GetMsgdef(self);
   upb_msg_clear(self->ptr.msg, msgdef);
   Py_RETURN_NONE;
@@ -1035,39 +1035,39 @@ static PyObject* PyUpb_CMessage_Clear(PyUpb_CMessage* self, PyObject* args) {
 
 void PyUpb_CMessage_DoClearField(PyObject* _self, const upb_fielddef* f) {
   PyUpb_CMessage* self = (void*)_self;
-  PyUpb_CMessage_AssureReified((PyUpb_CMessage*)self);
+  PyUpb_CMessage_EnsureReified((PyUpb_CMessage*)self);
+
+  // We must ensure that any stub object is reified so its parent no longer
+  // points to us.
+  PyObject* sub = self->unset_subobj_map
+                      ? PyUpb_WeakMap_Get(self->unset_subobj_map, f)
+                      : NULL;
 
   if (upb_fielddef_ismap(f)) {
-    // We have to invalidate any existing iterator over this map.
-    PyObject* obj = NULL;
-    if (self->unset_subobj_map) {
-      obj = PyUpb_WeakMap_Get(self->unset_subobj_map, f);
-    }
-    if (!obj) {
-      upb_msg* msg = PyUpb_CMessage_GetMsg(self);
-      upb_msgval msgval = upb_msg_get(msg, f);
-      obj = PyUpb_ObjCache_Get(msgval.map_val);
+    // For maps we additionally have to invalidate any iterators.  So we need
+    // to get an object even if it's reified.
+    if (!sub) {
+      sub = PyUpb_CMessage_GetFieldValue(_self, f);
     }
-    if (obj) {
-      PyUpb_MapContainer_Invalidate(obj);
-      Py_DECREF(obj);
+    PyUpb_MapContainer_EnsureReified(sub);
+    PyUpb_MapContainer_Invalidate(sub);
+  } else if (upb_fielddef_isseq(f)) {
+    if (sub) {
+      //PyUpb_RepeatedContainer_EnsureReified(sub);
     }
-  } else if (upb_fielddef_issubmsg(f) && !upb_fielddef_isseq(f)) {
-    if (self->unset_subobj_map) {
-      PyObject* sub = PyUpb_WeakMap_Get(self->unset_subobj_map, f);
-      if (sub) {
-        PyUpb_CMessage_AssureReified((PyUpb_CMessage*)sub);
-        Py_DECREF(sub);
-      }
+  } else if (upb_fielddef_issubmsg(f)) {
+    if (sub) {
+      PyUpb_CMessage_EnsureReified((PyUpb_CMessage*)sub);
     }
   }
 
+  Py_XDECREF(sub);
   upb_msg_clearfield(self->ptr.msg, f);
 }
 
 static PyObject* PyUpb_CMessage_ClearExtension(PyObject* _self, PyObject* arg) {
   PyUpb_CMessage* self = (void*)_self;
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   const upb_fielddef* f = PyUpb_CMessage_GetExtensionDef(_self, arg);
   if (!f) return NULL;
   PyUpb_CMessage_DoClearField(_self, f);
@@ -1077,12 +1077,12 @@ static PyObject* PyUpb_CMessage_ClearExtension(PyObject* _self, PyObject* arg) {
 static PyObject* PyUpb_CMessage_ClearField(PyObject* _self, PyObject* arg) {
   PyUpb_CMessage* self = (void*)_self;
 
-  // We always need AssureWritable() here (even for an unset message) to
+  // We always need EnsureReified() here (even for an unset message) to
   // preserve behavior like:
   //   msg = FooMessage()
   //   msg.foo.Clear()
   //   assert msg.HasField("foo")
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
 
   const upb_fielddef* f;
   const upb_oneofdef* o;
@@ -1097,7 +1097,7 @@ static PyObject* PyUpb_CMessage_ClearField(PyObject* _self, PyObject* arg) {
 
 static PyObject* PyUpb_CMessage_DiscardUnknownFields(PyUpb_CMessage* self,
                                                      PyObject* arg) {
-  PyUpb_CMessage_AssureReified(self);
+  PyUpb_CMessage_EnsureReified(self);
   const upb_msgdef* msgdef = _PyUpb_CMessage_GetMsgdef(self);
   upb_msg_discardunknown(self->ptr.msg, msgdef, 64);
   Py_RETURN_NONE;

python/minimal_test.py

@@ -31,6 +31,7 @@ import unittest
 from google.protobuf.pyext import _message
 from google.protobuf.internal import api_implementation
 from google.protobuf import unittest_pb2
+from google.protobuf import map_unittest_pb2
 from google.protobuf import descriptor_pool
 from google.protobuf import text_format
 from google.protobuf import message_factory
@@ -79,6 +80,37 @@ class TestMessageExtension(unittest.TestCase):
     def testExtensionsErrors(self):
         msg = unittest_pb2.TestAllTypes()
         self.assertRaises(AttributeError, getattr, msg, 'Extensions')
+    
+    def testClearStubMapField(self):
+        msg = map_unittest_pb2.TestMap()
+        int32_map = msg.map_int32_int32
+        msg.ClearField("map_int32_int32")
+        int32_map[123] = 456
+        self.assertEqual(0, msg.ByteSize())
+
+    def testClearReifiedMapField(self):
+        msg = map_unittest_pb2.TestMap()
+        int32_map = msg.map_int32_int32
+        int32_map[123] = 456
+        msg.ClearField("map_int32_int32")
+        int32_map[111] = 222
+        self.assertEqual(0, msg.ByteSize())
+
+    def testClearStubRepeatedField(self):
+        msg = unittest_pb2.TestAllTypes()
+        int32_array = msg.repeated_int32
+        msg.ClearField("repeated_int32")
+        int32_array.append(123)
+        self.assertEqual(0, msg.ByteSize())
+
+    def testClearReifiedRepeatdField(self):
+        msg = unittest_pb2.TestAllTypes()
+        int32_array = msg.repeated_int32
+        int32_array.append(123)
+        self.assertNotEqual(0, msg.ByteSize())
+        msg.ClearField("repeated_int32")
+        int32_array.append(123)
+        self.assertEqual(0, msg.ByteSize())
 
 #TestMessageExtension.test_descriptor_pool.__unittest_expecting_failure__ = True
 

python/repeated.c

@@ -116,7 +116,7 @@ void PyUpb_RepeatedContainer_Reify(PyObject* _self, upb_array* arr) {
   assert(!PyUpb_RepeatedContainer_IsStub(self));
 }
 
-static upb_array* PyUpb_RepeatedContainer_AssureReified(PyObject* _self) {
+upb_array* PyUpb_RepeatedContainer_EnsureReified(PyObject* _self) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
   upb_array* arr = PyUpb_RepeatedContainer_GetIfReified(self);
   if (arr) return arr;  // Already writable.
@@ -209,7 +209,7 @@ PyObject* PyUpb_RepeatedContainer_DeepCopy(PyObject* _self, PyObject* value) {
 
 PyObject* PyUpb_RepeatedContainer_Extend(PyObject* _self, PyObject* value) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   size_t start_size = upb_array_size(arr);
   PyObject* it = PyObject_GetIter(value);
   if (!it) {
@@ -435,7 +435,7 @@ static PyObject* PyUpb_RepeatedContainer_Pop(PyObject* _self, PyObject* args) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
   Py_ssize_t index = -1;
   if (!PyArg_ParseTuple(args, "|n", &index)) return NULL;
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   size_t size = upb_array_size(arr);
   if (index < 0) index += size;
   if (index >= size) index = size - 1;
@@ -447,7 +447,7 @@ static PyObject* PyUpb_RepeatedContainer_Pop(PyObject* _self, PyObject* args) {
 
 static PyObject* PyUpb_RepeatedContainer_Remove(PyObject* _self,
                                                 PyObject* value) {
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   Py_ssize_t match_index = -1;
   Py_ssize_t n = PyUpb_RepeatedContainer_Length(_self);
   for (Py_ssize_t i = 0; i < n; ++i) {
@@ -474,7 +474,7 @@ static PyObject* PyUpb_RepeatedContainer_Remove(PyObject* _self,
 static bool PyUpb_RepeatedContainer_Assign(PyObject* _self, PyObject* list) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
   const upb_fielddef* f = PyUpb_RepeatedContainer_GetField(self);
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   Py_ssize_t size = PyList_Size(list);
   bool submsg = upb_fielddef_issubmsg(f);
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
@@ -538,7 +538,7 @@ static PyObject* PyUpb_RepeatedContainer_MergeFrom(PyObject* _self,
 
 static PyObject* PyUpb_RepeatedCompositeContainer_AppendNew(PyObject* _self) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   if (!arr) return NULL;
   const upb_fielddef* f = PyUpb_RepeatedContainer_GetField(self);
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
@@ -582,7 +582,7 @@ static PyObject* PyUpb_RepeatedContainer_Insert(PyObject* _self,
   Py_ssize_t index;
   PyObject* value;
   if (!PyArg_ParseTuple(args, "nO", &index, &value)) return NULL;
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   if (!arr) return NULL;
 
   // Normalize index.
@@ -667,7 +667,7 @@ static PyType_Spec PyUpb_RepeatedCompositeContainer_Spec = {
 static PyObject* PyUpb_RepeatedScalarContainer_Append(PyObject* _self,
                                                       PyObject* value) {
   PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
-  upb_array* arr = PyUpb_RepeatedContainer_AssureReified(_self);
+  upb_array* arr = PyUpb_RepeatedContainer_EnsureReified(_self);
   upb_arena* arena = PyUpb_Arena_Get(self->arena);
   const upb_fielddef* f = PyUpb_RepeatedContainer_GetField(self);
   upb_msgval msgval;

python/repeated.h

@@ -49,6 +49,9 @@ PyObject* PyUpb_RepeatedContainer_GetOrCreateWrapper(upb_array* arr,
 // Reifies a repeated field stub to point to the concrete data in `arr`.
 void PyUpb_RepeatedContainer_Reify(PyObject* self, upb_array* arr);
 
+// Reifies this repeated object if it is not already reified.
+upb_array* PyUpb_RepeatedContainer_EnsureReified(PyObject* self);
+
 // Implements repeated_field.extend(iterable).  `_self` must be a repeated
 // field (either repeated composite or repeated scalar).
 PyObject* PyUpb_RepeatedContainer_Extend(PyObject* _self, PyObject* value);