diff --git a/python/google/protobuf/internal/containers.py b/python/google/protobuf/internal/containers.py index ba4b7a1299994..34a1908a6c6fd 100755 --- a/python/google/protobuf/internal/containers.py +++ b/python/google/protobuf/internal/containers.py @@ -101,10 +101,20 @@ def sort(self, *args, **kwargs) -> None: if 'sort_function' in kwargs: kwargs['cmp'] = kwargs.pop('sort_function') self._values.sort(*args, **kwargs) + if not self._message_listener.dirty: + self._message_listener.Modified() def reverse(self) -> None: self._AssureWritable() self._values.reverse() + if not self._message_listener.dirty: + self._message_listener.Modified() + + def clear(self) -> None: + self._AssureWritable() + self._values.clear() + if not self._message_listener.dirty: + self._message_listener.Modified() # TODO: Remove this. BaseContainer does *not* conform to diff --git a/python/google/protobuf/internal/message_test.py b/python/google/protobuf/internal/message_test.py index 8532d499e8b01..a275d0f8dbbff 100755 --- a/python/google/protobuf/internal/message_test.py +++ b/python/google/protobuf/internal/message_test.py @@ -531,16 +531,16 @@ def testAssignRepeatedField(self, message_module): self.assertEqual([1, 2, 3, 4], msg.payload.repeated_int32) def testRepeatedFieldSelfSliceAssignment(self, message_module): - msg = message_module.NestedTestAllTypes() - msg.payload.repeated_int32[:] = [1, 2, 3, 4] - msg.payload.repeated_int32[:] = msg.payload.repeated_int32 - self.assertEqual([1, 2, 3, 4], msg.payload.repeated_int32) + msg = message_module.NestedTestAllTypes() + msg.payload.repeated_int32[:] = [1, 2, 3, 4] + msg.payload.repeated_int32[:] = msg.payload.repeated_int32 + self.assertEqual([1, 2, 3, 4], msg.payload.repeated_int32) def testRepeatedFieldSelfExtend(self, message_module): - msg = message_module.NestedTestAllTypes() - msg.payload.repeated_int32[:] = [1, 2, 3, 4] - msg.payload.repeated_int32.extend(msg.payload.repeated_int32) - self.assertEqual([1, 2, 3, 4] * 2, msg.payload.repeated_int32) + msg = message_module.NestedTestAllTypes() + msg.payload.repeated_int32[:] = [1, 2, 3, 4] + msg.payload.repeated_int32.extend(msg.payload.repeated_int32) + self.assertEqual([1, 2, 3, 4] * 2, msg.payload.repeated_int32) def testAssignOutOfRange(self, message_module): msg = message_module.NestedTestAllTypes() @@ -781,13 +781,72 @@ def testRepeatedNestedFieldIteration(self, message_module): ) def testSortEmptyRepeated(self, message_module): - message = message_module.NestedTestAllTypes() - self.assertFalse(message.HasField('child')) - self.assertFalse(message.HasField('payload')) - message.child.repeated_child.sort() - message.payload.repeated_int32.sort() - self.assertFalse(message.HasField('child')) - self.assertFalse(message.HasField('payload')) + if api_implementation.Type() == 'python': + return + msg = message_module.NestedTestAllTypes() + self.assertFalse(msg.HasField('child')) + self.assertFalse(msg.HasField('payload')) + msg.child.repeated_child.sort() + msg.payload.repeated_int32.sort() + self.assertTrue(msg.HasField('child')) + self.assertTrue(msg.HasField('payload')) + + def testReverseEmptyRepeated(self, message_module): + if api_implementation.Type() == 'python': + return + msg = message_module.NestedTestAllTypes() + self.assertFalse(msg.HasField('child')) + self.assertFalse(msg.HasField('payload')) + msg.child.repeated_child.reverse() + msg.payload.repeated_int32.reverse() + self.assertTrue(msg.HasField('child')) + self.assertTrue(msg.HasField('payload')) + + def testClearEmptyRepeated(self, message_module): + if api_implementation.Type() == 'python': + return + msg = message_module.NestedTestAllTypes() + self.assertFalse(msg.HasField('child')) + self.assertFalse(msg.HasField('payload')) + msg.child.repeated_child.clear() + msg.payload.repeated_int32.clear() + self.assertTrue(msg.HasField('child')) + self.assertTrue(msg.HasField('payload')) + + def testDelEmptyRepeated(self, message_module): + if api_implementation.Type() == 'python': + return + msg = message_module.NestedTestAllTypes() + self.assertFalse(msg.HasField('child')) + self.assertFalse(msg.HasField('payload')) + del msg.child.repeated_child[:] + del msg.payload.repeated_int32[:] + self.assertTrue(msg.HasField('child')) + self.assertTrue(msg.HasField('payload')) + + def testImmutabilityCheckComesFirstEmptyRepeated(self, message_module): + if api_implementation.Type() == 'python': + return + msg = message_module.TestAllTypes() + options = msg.DESCRIPTOR.GetOptions() + self.assertEqual(0, len(options.uninterpreted_option)) + + with self.assertRaises( + (TypeError, AttributeError, message.FrozenInstanceError) + ): + options.uninterpreted_option.sort() + with self.assertRaises( + (TypeError, AttributeError, message.FrozenInstanceError) + ): + options.uninterpreted_option.reverse() + with self.assertRaises( + (TypeError, AttributeError, message.FrozenInstanceError) + ): + options.uninterpreted_option.clear() + with self.assertRaises( + (TypeError, AttributeError, message.FrozenInstanceError) + ): + del options.uninterpreted_option[:] def testSortingRepeatedScalarFieldsDefaultComparator(self, message_module): """Check some different types with the default comparator.""" diff --git a/python/google/protobuf/pyext/repeated_composite_container.cc b/python/google/protobuf/pyext/repeated_composite_container.cc index aef12e003759f..9b6c84d80e7c7 100644 --- a/python/google/protobuf/pyext/repeated_composite_container.cc +++ b/python/google/protobuf/pyext/repeated_composite_container.cc @@ -263,12 +263,6 @@ int AssignSubscript(RepeatedCompositeContainer* self, PyObject* slice, return -1; } - // TODO: b/517235198 - Reify even for empty sequences. - int status = cmessage::CheckRepeatedFieldDeletion( - self->parent, self->parent_field_descriptor, slice); - if (status < 0) return -1; - if (status > 0) return 0; - if (cmessage::AssureWritable(self->parent) == nullptr) return -1; return cmessage::DeleteRepeatedField(self->parent, @@ -361,13 +355,12 @@ static PyObject* ToStr(PyObject* pself) { // --------------------------------------------------------------------- // sort() -static void ReorderAttached(RepeatedCompositeContainer* self, +static bool ReorderAttached(RepeatedCompositeContainer* self, PyObject* child_list) { - const Py_ssize_t length = Length(reinterpret_cast(self)); - if (length == 0) return; - Message* message = cmessage::AssureWritable(self->parent); - if (message == nullptr) return; + if (message == nullptr) return false; + const Py_ssize_t length = Length(reinterpret_cast(self)); + if (length == 0) return true; const Reflection* reflection = message->GetReflection(); const FieldDescriptor* descriptor = self->parent_field_descriptor; @@ -379,10 +372,11 @@ static void ReorderAttached(RepeatedCompositeContainer* self, CMessage* child_cmsg = reinterpret_cast(PyList_GET_ITEM(child_list, i)); Message* child_message = cmessage::AssureWritable(child_cmsg); - if (child_message == nullptr) return; + if (child_message == nullptr) return false; reflection->UnsafeArenaAddAllocatedMessage(message, descriptor, child_message); } + return true; } // Returns 0 if successful; returns -1 and sets an exception if @@ -398,7 +392,9 @@ static int SortPythonMessages(RepeatedCompositeContainer* self, PyObject* args, if (m == nullptr) return -1; if (ScopedPyObjectPtr(PyObject_Call(m.get(), args, kwds)) == nullptr) return -1; - ReorderAttached(self, child_list.get()); + if (!ReorderAttached(self, child_list.get())) { + return -1; + } return 0; } @@ -406,9 +402,6 @@ static PyObject* Sort(PyObject* pself, PyObject* args, PyObject* kwds) { RepeatedCompositeContainer* self = reinterpret_cast(pself); - if (self->parent->state == python::MESSAGE_FROZEN) { - return SetContainerFrozenError(); - } // Support the old sort_function argument for backwards // compatibility. @@ -422,10 +415,6 @@ static PyObject* Sort(PyObject* pself, PyObject* args, PyObject* kwds) { } } - // TODO: b/517235198 - Reify even for empty sequences. - if (Length(pself) == 0) { - Py_RETURN_NONE; - } if (SortPythonMessages(self, args, kwds) < 0) { return nullptr; @@ -447,7 +436,9 @@ static int ReversePythonMessages(RepeatedCompositeContainer* self) { if (ScopedPyObjectPtr( PyObject_CallMethod(child_list.get(), "reverse", nullptr)) == nullptr) return -1; - ReorderAttached(self, child_list.get()); + if (!ReorderAttached(self, child_list.get())) { + return -1; + } return 0; } @@ -455,14 +446,7 @@ static PyObject* Reverse(PyObject* pself) { RepeatedCompositeContainer* self = reinterpret_cast(pself); - if (self->parent->state == python::MESSAGE_FROZEN) { - return SetContainerFrozenError(); - } - // TODO: b/517235198 - Reify even for empty sequences. - if (Length(pself) == 0) { - Py_RETURN_NONE; - } if (ReversePythonMessages(self) < 0) { return nullptr; @@ -474,10 +458,6 @@ static PyObject* Reverse(PyObject* pself) { static PyObject* Clear(PyObject* pself) { RepeatedCompositeContainer* self = reinterpret_cast(pself); - // TODO: b/517235198 - Reify even for empty sequences. - if (Length(pself) == 0) { - Py_RETURN_NONE; - } CMessage* cmessage = self->parent; Message* message = cmessage::AssureWritable(cmessage); diff --git a/python/google/protobuf/pyext/repeated_scalar_container.cc b/python/google/protobuf/pyext/repeated_scalar_container.cc index db75699b7a562..d46456066e684 100644 --- a/python/google/protobuf/pyext/repeated_scalar_container.cc +++ b/python/google/protobuf/pyext/repeated_scalar_container.cc @@ -1081,9 +1081,6 @@ static PyObject* Sort(PyObject* pself, PyObject* args, PyObject* kwds) { RepeatedScalarContainer* self = reinterpret_cast(pself); - if (self->parent->state == python::MESSAGE_FROZEN) { - return SetContainerFrozenError(); - } // Support the old sort_function argument for backwards // compatibility. @@ -1105,9 +1102,6 @@ static PyObject* Sort(PyObject* pself, PyObject* args, PyObject* kwds) { if (list == nullptr) { return nullptr; } - if (PyList_GET_SIZE(list.get()) == 0) { - Py_RETURN_NONE; - } ScopedPyObjectPtr m(PyObject_GetAttrString(list.get(), "sort")); if (m == nullptr) { return nullptr; @@ -1128,14 +1122,7 @@ static PyObject* Reverse(PyObject* pself) { RepeatedScalarContainer* self = reinterpret_cast(pself); - if (self->parent->state == python::MESSAGE_FROZEN) { - return SetContainerFrozenError(); - } - // TODO: b/517235198 - Reify even for empty sequences. - if (Len(pself) == 0) { - Py_RETURN_NONE; - } ScopedPyObjectPtr full_slice(PySlice_New(nullptr, nullptr, nullptr)); if (full_slice == nullptr) { @@ -1161,10 +1148,6 @@ static PyObject* Clear(PyObject* pself) { RepeatedScalarContainer* self = reinterpret_cast(pself); - // TODO: b/517235198 - Reify even for empty sequences. - if (Len(pself) == 0) { - Py_RETURN_NONE; - } CMessage* cmessage = self->parent; Message* message = cmessage::AssureWritable(cmessage); diff --git a/python/repeated.c b/python/repeated.c index 8303dd4effa78..b8299ea2df2f7 100644 --- a/python/repeated.c +++ b/python/repeated.c @@ -869,12 +869,7 @@ static PyObject* PyUpb_RepeatedContainer_Sort(PyObject* pself, PyObject* args, } } - if (PyUpb_RepeatedContainer_IsFrozen((PyUpb_RepeatedContainer*)pself)) { - return PyUpb_SetFrozenErrorWithMsg("Container is immutable"); - } - // TODO:b/517235198 - Reify even for empty sequences. - if (PyUpb_RepeatedContainer_Length(pself) == 0) Py_RETURN_NONE; upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(pself); if (!arr) return NULL; @@ -918,8 +913,6 @@ static PyObject* PyUpb_RepeatedContainer_Reverse(PyObject* _self) { static PyObject* PyUpb_RepeatedContainer_Clear(PyObject* _self) { Py_ssize_t size = PyUpb_RepeatedContainer_Length(_self); - // TODO: b/517235198 - Reify even for empty sequences. - if (size == 0) Py_RETURN_NONE; PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self);