PyAPI: Add useful exceptions to CollectionProperty.move()

The lack of exceptions for invalid indices makes bugs harder
to track down.

Also includes additional tests.

Ref !161833

source/blender/makesrna/RNA_access.hh

@@ -37,6 +37,21 @@ struct bContext;
 
 enum eID_OverrideLib_Op : short;
 
+/**
+ * \note Some functions perform multiple checks whose results are more
+ * accurately and informatively represented using an enum status.
+ */
+enum class eRNAStatus {
+  /** The operation completed successfully. */
+  Success = 0,
+  /** The specified index is outside the valid range. */
+  IndexOutOfRange,
+  /** The property cannot be edited. */
+  Immutable,
+  /** An unexpected property was encountered. */
+  Unsupported,
+};
+
 /* Types */
 BlenderRNA &RNA_blender_rna_get();
 
@@ -696,7 +711,10 @@ void RNA_property_pointer_remove(PointerRNA *ptr, PropertyRNA *prop);
 void RNA_property_collection_add(PointerRNA *ptr, PropertyRNA *prop, PointerRNA *r_ptr);
 bool RNA_property_collection_remove(PointerRNA *ptr, PropertyRNA *prop, int key);
 void RNA_property_collection_clear(PointerRNA *ptr, PropertyRNA *prop);
-bool RNA_property_collection_move(PointerRNA *ptr, PropertyRNA *prop, int key, int pos);
+eRNAStatus RNA_property_collection_move(PointerRNA *ptr,
+                                        PropertyRNA *prop,
+                                        int src_index,
+                                        int dst_index);
 
 /* copy/reset */
 bool RNA_property_copy(Main *bmain,

source/blender/makesrna/intern/rna_access.cc

@@ -5139,47 +5139,55 @@ bool RNA_property_collection_remove(PointerRNA *ptr, PropertyRNA *prop, int key)
   return false;
 }
 
-bool RNA_property_collection_move(PointerRNA *ptr, PropertyRNA *prop, int key, int pos)
+eRNAStatus RNA_property_collection_move(PointerRNA *ptr,
+                                        PropertyRNA *prop,
+                                        int src_index,
+                                        int dst_index)
 {
-  IDProperty *idprop;
-
   BLI_assert(RNA_property_type(prop) == PROP_COLLECTION);
 
   bool is_liboverride;
   if (!property_collection_liboverride_editable(ptr, prop, &is_liboverride)) {
-    return false;
+    return eRNAStatus::Immutable;
   }
 
+  IDProperty *idprop;
   if ((idprop = rna_idproperty_check(&prop, ptr))) {
-    IDProperty tmp, *array;
-    int len;
+    int len = idprop->len;
+    IDProperty *array = IDP_property_array_get(idprop);
 
-    len = idprop->len;
-    array = IDP_property_array_get(idprop);
-
-    if (key >= 0 && key < len && pos >= 0 && pos < len && key != pos) {
-      if (is_liboverride && (array[key].flag & IDP_FLAG_OVERRIDELIBRARY_LOCAL) == 0) {
-        /* We can only move items that we actually inserted in the local override. */
-        return false;
-      }
-
-      memcpy(&tmp, &array[key], sizeof(IDProperty));
-      if (pos < key) {
-        memmove(array + pos + 1, array + pos, sizeof(IDProperty) * (key - pos));
-      }
-      else {
-        memmove(array + key, array + key + 1, sizeof(IDProperty) * (pos - key));
-      }
-      memcpy(&array[pos], &tmp, sizeof(IDProperty));
+    if (src_index < 0 || src_index >= len || dst_index < 0 || dst_index >= len) {
+      return eRNAStatus::IndexOutOfRange;
     }
 
-    return true;
+    if (is_liboverride && (array[src_index].flag & IDP_FLAG_OVERRIDELIBRARY_LOCAL) == 0) {
+      /* We can only move items that we actually inserted in the local override. */
+      return eRNAStatus::Immutable;
+    }
+
+    if (src_index != dst_index) {
+      IDProperty tmp;
+      memcpy(&tmp, &array[src_index], sizeof(IDProperty));
+      if (dst_index < src_index) {
+        memmove(array + dst_index + 1,
+                array + dst_index,
+                sizeof(IDProperty) * (src_index - dst_index));
+      }
+      else {
+        memmove(array + src_index,
+                array + src_index + 1,
+                sizeof(IDProperty) * (dst_index - src_index));
+      }
+      memcpy(&array[dst_index], &tmp, sizeof(IDProperty));
+    }
+    return eRNAStatus::Success;
   }
   if (prop->flag & PROP_IDPROPERTY) {
-    return true;
+    /* No-init empty collection. */
+    return eRNAStatus::IndexOutOfRange;
   }
 
-  return false;
+  return eRNAStatus::Unsupported;
 }
 
 void RNA_property_collection_clear(PointerRNA *ptr, PropertyRNA *prop)

source/blender/makesrna/intern/rna_rna.cc

@@ -3105,7 +3105,8 @@ bool rna_property_override_apply_default(Main *bmain,
           IDP_CopyPropertyContent(item_idprop_dst, item_idprop_src);
 
           ret_success = RNA_property_collection_move(
-              ptr_dst, prop_dst, item_index_added, item_index_dst);
+                            ptr_dst, prop_dst, item_index_added, item_index_dst) ==
+                        eRNAStatus::Success;
           break;
         }
         default:

source/blender/python/intern/bpy_rna.cc

@@ -226,6 +226,30 @@ static void pyrna_prop_warn_deprecated(const PointerRNA *ptr,
                    deprecated->note);
 }
 
+static bool pyrna_status_ok_or_error(eRNAStatus status, const char *error_prefix)
+{
+  switch (status) {
+    case eRNAStatus::Success: {
+      return true;
+    }
+    case eRNAStatus::IndexOutOfRange: {
+      PyErr_Format(PyExc_IndexError, "%.200s: index out of range", error_prefix);
+      return false;
+    }
+    case eRNAStatus::Immutable: {
+      PyErr_Format(PyExc_TypeError, "%.200s: is not editable", error_prefix);
+      return false;
+    }
+    case eRNAStatus::Unsupported: {
+      PyErr_Format(PyExc_TypeError, "%.200s: not supported for this collection", error_prefix);
+      return false;
+    }
+  }
+
+  BLI_assert_unreachable();
+  return true;
+}
+
 #ifdef USE_PYRNA_INVALIDATE_GC
 #  define FROM_GC(g) ((PyObject *)(((PyGC_Head *)g) + 1))
 
@@ -5436,7 +5460,7 @@ PyDoc_STRVAR(
     "   :type dst_index: int\n");
 static PyObject *pyrna_prop_collection_idprop_move(BPy_PropertyRNA *self, PyObject *args)
 {
-  int key = 0, pos = 0;
+  int src_index = 0, dst_index = 0;
 
 #ifdef USE_PEDANTIC_WRITE
   if (rna_disallow_writes && rna_id_write_error(&self->ptr.value(), nullptr)) {
@@ -5448,16 +5472,16 @@ static PyObject *pyrna_prop_collection_idprop_move(BPy_PropertyRNA *self, PyObje
                         "i" /* `src_index` */
                         "i" /* `dst_index` */
                         ":move",
-                        &key,
-                        &pos))
+                        &src_index,
+                        &dst_index))
   {
     PyErr_SetString(PyExc_TypeError, "bpy_prop_collection.move(): expected two ints as arguments");
     return nullptr;
   }
 
-  if (!RNA_property_collection_move(&self->ptr.value(), self->prop, key, pos)) {
-    PyErr_SetString(PyExc_TypeError,
-                    "bpy_prop_collection.move() not supported for this collection");
+  eRNAStatus status = RNA_property_collection_move(
+      &self->ptr.value(), self->prop, src_index, dst_index);
+  if (!pyrna_status_ok_or_error(status, "bpy_prop_collection.move")) {
     return nullptr;
   }
 

tests/python/bl_pyapi_prop.py

@@ -501,12 +501,14 @@ class TestPropCollectionAndPointer(unittest.TestCase):
         id_type.test_pointer_ID = PointerProperty(type=bpy.types.ID)
         id_type.test_pointer_ID_poll = PointerProperty(type=bpy.types.ID, poll=lambda s, v: v.id_type == 'OBJECT')
         id_type.test_collection = CollectionProperty(type=TestPropertyGroup)
+        id_type.test_collection_move = CollectionProperty(type=TestPropertyGroup)
 
     def tearDown(self):
         del id_type.test_pointer
         del id_type.test_pointer_ID
         del id_type.test_pointer_ID_poll
         del id_type.test_collection
+        del id_type.test_collection_move
 
         bpy.utils.unregister_class(TestPropertyGroup)
 
@@ -578,6 +580,45 @@ class TestPropCollectionAndPointer(unittest.TestCase):
         self.assertNotEqual(id_inst.test_collection[1], test_item_3)
         self.assertEqual(id_inst.test_collection[1].test_prop, 24)
 
+    def test_access_collection_move(self):
+        self.assertEqual(len(id_inst.test_collection_move), 0)
+
+        # Out of range in no-init empty collection.
+        with self.assertRaises(IndexError) as context:
+            id_inst.test_collection_move.move(0, 0)
+        self.assertEqual(str(context.exception), "bpy_prop_collection.move: index out of range")
+
+        # Out of range in init empty collection.
+        id_inst.test_collection_move.add()
+        id_inst.test_collection_move.clear()
+        with self.assertRaises(IndexError) as context:
+            id_inst.test_collection_move.move(0, 0)
+        self.assertEqual(str(context.exception), "bpy_prop_collection.move: index out of range")
+
+        # Fill.
+        for i in range(4):
+            id_inst.test_collection_move.add().test_prop = i
+
+        # Out of range.
+        with self.assertRaises(IndexError) as context:
+            id_inst.test_collection_move.move(0, 4)
+        self.assertEqual(str(context.exception), "bpy_prop_collection.move: index out of range")
+
+        # Negative index.
+        with self.assertRaises(IndexError) as context:
+            id_inst.test_collection_move.move(-1, 0)
+        self.assertEqual(str(context.exception), "bpy_prop_collection.move: index out of range")
+
+        # Regular move case.
+        id_inst.test_collection_move.move(0, 3)
+        self.assertEqual([i.test_prop for i in id_inst.test_collection_move], [1, 2, 3, 0])
+
+        # Single element move.
+        id_inst.test_collection_move.clear()
+        test_item = id_inst.test_collection_move.add()
+        id_inst.test_collection_move.move(0, 0)
+        self.assertEqual(id_inst.test_collection_move[0], test_item)
+
     # TODO: Add expected failure cases (e.g. assigning propertygroup to a Pointer property, etc.).