gh-100980: ctypes: Test, document, and fix finalizing _fields_ (GH-124292)

- If setting `_fields_` fails, e.g. with AttributeError, don't set the attribute in `__dict__`
- Document the “finalization” behaviour
- Beef up tests: add `getattr`, test Union as well as Structure
- Put common functionality in a common function

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

4 files changed, 67 insertions, 55 deletions

diff --git a/Doc/library/ctypes.rst b/Doc/library/ctypes.rst
index 5981018..a218304 100644
--- a/Doc/library/ctypes.rst
+++ b/Doc/library/ctypes.rst
@@ -2499,6 +2499,8 @@ Structured data types
 
    Abstract base class for unions in native byte order.
 
+   Unions share common attributes and behavior with structures;
+   see :class:`Structure` documentation for details.
 
 .. class:: BigEndianUnion(*args, **kw)
 
@@ -2558,14 +2560,19 @@ fields, or any other data types containing pointer type fields.
                           ...
                          ]
 
-      The :attr:`_fields_` class variable must, however, be defined before the
-      type is first used (an instance is created, :func:`sizeof` is called on it,
-      and so on).  Later assignments to the :attr:`_fields_` class variable will
-      raise an AttributeError.
+      The :attr:`!_fields_` class variable can only be set once.
+      Later assignments will raise an :exc:`AttributeError`.
 
-      It is possible to define sub-subclasses of structure types, they inherit
-      the fields of the base class plus the :attr:`_fields_` defined in the
-      sub-subclass, if any.
+      Additionally, the :attr:`!_fields_` class variable must be defined before
+      the structure or union type is first used: an instance or subclass is
+      created, :func:`sizeof` is called on it, and so on.
+      Later assignments to :attr:`!_fields_` will raise an :exc:`AttributeError`.
+      If :attr:`!_fields_` has not been set before such use,
+      the structure or union will have no own fields, as if :attr:`!_fields_`
+      was empty.
+
+      Sub-subclasses of structure types inherit the fields of the base class
+      plus the :attr:`_fields_` defined in the sub-subclass, if any.
 
 
    .. attribute:: _pack_
diff --git a/Lib/test/test_ctypes/test_struct_fields.py b/Lib/test/test_ctypes/test_struct_fields.py
index 7d7a518..b5e165f 100644
--- a/Lib/test/test_ctypes/test_struct_fields.py
+++ b/Lib/test/test_ctypes/test_struct_fields.py
@@ -4,7 +4,9 @@ from ._support import (CField, Py_TPFLAGS_DISALLOW_INSTANTIATION,
                        Py_TPFLAGS_IMMUTABLETYPE)
 
 
-class StructFieldsTestCase(unittest.TestCase):
+NOTHING = object()
+
+class FieldsTestBase:
     # Structure/Union classes must get 'finalized' sooner or
     # later, when one of these things happen:
     #
@@ -14,42 +16,47 @@ class StructFieldsTestCase(unittest.TestCase):
     # 4. The type is subclassed
     #
     # When they are finalized, assigning _fields_ is no longer allowed.
+
+    def assert_final_fields(self, cls, expected=NOTHING):
+        self.assertRaises(AttributeError, setattr, cls, "_fields_", [])
+        self.assertEqual(getattr(cls, "_fields_", NOTHING), expected)
+
     def test_1_A(self):
-        class X(Structure):
+        class X(self.cls):
             pass
         self.assertEqual(sizeof(X), 0) # not finalized
         X._fields_ = [] # finalized
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X, expected=[])
 
     def test_1_B(self):
-        class X(Structure):
+        class X(self.cls):
             _fields_ = [] # finalized
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X, expected=[])
 
     def test_2(self):
-        class X(Structure):
+        class X(self.cls):
             pass
         X()
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X)
 
     def test_3(self):
-        class X(Structure):
+        class X(self.cls):
             pass
-        class Y(Structure):
+        class Y(self.cls):
             _fields_ = [("x", X)] # finalizes X
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X)
 
     def test_4(self):
-        class X(Structure):
+        class X(self.cls):
             pass
         class Y(X):
             pass
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X)
         Y._fields_ = []
-        self.assertRaises(AttributeError, setattr, X, "_fields_", [])
+        self.assert_final_fields(X)
 
     def test_5(self):
-        class X(Structure):
+        class X(self.cls):
             _fields_ = (("char", c_char * 5),)
 
         x = X(b'#' * 5)
@@ -59,14 +66,8 @@ class StructFieldsTestCase(unittest.TestCase):
     def test_6(self):
         self.assertRaises(TypeError, CField)
 
-    def test_cfield_type_flags(self):
-        self.assertTrue(CField.__flags__ & Py_TPFLAGS_IMMUTABLETYPE)
-
-    def test_cfield_inheritance_hierarchy(self):
-        self.assertEqual(CField.mro(), [CField, object])
-
     def test_gh99275(self):
-        class BrokenStructure(Structure):
+        class BrokenStructure(self.cls):
             def __init_subclass__(cls, **kwargs):
                 cls._fields_ = []  # This line will fail, `stginfo` is not ready
 
@@ -77,26 +78,28 @@ class StructFieldsTestCase(unittest.TestCase):
     # __set__ and __get__ should raise a TypeError in case their self
     # argument is not a ctype instance.
     def test___set__(self):
-        class MyCStruct(Structure):
+        class MyCStruct(self.cls):
             _fields_ = (("field", c_int),)
         self.assertRaises(TypeError,
                           MyCStruct.field.__set__, 'wrong type self', 42)
 
-        class MyCUnion(Union):
-            _fields_ = (("field", c_int),)
-        self.assertRaises(TypeError,
-                          MyCUnion.field.__set__, 'wrong type self', 42)
-
     def test___get__(self):
-        class MyCStruct(Structure):
+        class MyCStruct(self.cls):
             _fields_ = (("field", c_int),)
         self.assertRaises(TypeError,
                           MyCStruct.field.__get__, 'wrong type self', 42)
 
-        class MyCUnion(Union):
-            _fields_ = (("field", c_int),)
-        self.assertRaises(TypeError,
-                          MyCUnion.field.__get__, 'wrong type self', 42)
+class StructFieldsTestCase(unittest.TestCase, FieldsTestBase):
+    cls = Structure
+
+    def test_cfield_type_flags(self):
+        self.assertTrue(CField.__flags__ & Py_TPFLAGS_IMMUTABLETYPE)
+
+    def test_cfield_inheritance_hierarchy(self):
+        self.assertEqual(CField.mro(), [CField, object])
+
+class UnionFieldsTestCase(unittest.TestCase, FieldsTestBase):
+    cls = Union
 
 
 if __name__ == "__main__":
diff --git a/Misc/NEWS.d/next/Library/2024-09-20-18-23-19.gh-issue-100980.8nVAB6.rst b/Misc/NEWS.d/next/Library/2024-09-20-18-23-19.gh-issue-100980.8nVAB6.rst
new file mode 100644
index 0000000..2279c20
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2024-09-20-18-23-19.gh-issue-100980.8nVAB6.rst
@@ -0,0 +1,3 @@
+The :attr:`~ctypes.Structure._fields_` attribute of
+:class:`ctypes.Structure` and :class:`~ctypes.Union` is no longer set if
+the setattr operation raises an error.
diff --git a/Modules/_ctypes/_ctypes.c b/Modules/_ctypes/_ctypes.c
index 1d9e169..951e691 100644
--- a/Modules/_ctypes/_ctypes.c
+++ b/Modules/_ctypes/_ctypes.c
@@ -1067,32 +1067,31 @@ CType_Type_repeat(PyObject *self, Py_ssize_t length)
     return PyCArrayType_from_ctype(st, self, length);
 }
 
-
 static int
-PyCStructType_setattro(PyObject *self, PyObject *key, PyObject *value)
+_structunion_setattro(PyObject *self, PyObject *key, PyObject *value, int is_struct)
 {
     /* XXX Should we disallow deleting _fields_? */
-    if (-1 == PyType_Type.tp_setattro(self, key, value))
-        return -1;
+    if (PyUnicode_Check(key)
+            && _PyUnicode_EqualToASCIIString(key, "_fields_"))
+    {
+        if (PyCStructUnionType_update_stginfo(self, value, is_struct) < 0) {
+            return -1;
+        }
+    }
 
-    if (value && PyUnicode_Check(key) &&
-        _PyUnicode_EqualToASCIIString(key, "_fields_"))
-        return PyCStructUnionType_update_stginfo(self, value, 1);
-    return 0;
+    return PyType_Type.tp_setattro(self, key, value);
 }
 
+static int
+PyCStructType_setattro(PyObject *self, PyObject *key, PyObject *value)
+{
+    return _structunion_setattro(self, key, value, 1);
+}
 
 static int
 UnionType_setattro(PyObject *self, PyObject *key, PyObject *value)
 {
-    /* XXX Should we disallow deleting _fields_? */
-    if (-1 == PyType_Type.tp_setattro(self, key, value))
-        return -1;
-
-    if (PyUnicode_Check(key) &&
-        _PyUnicode_EqualToASCIIString(key, "_fields_"))
-        return PyCStructUnionType_update_stginfo(self, value, 0);
-    return 0;
+    return _structunion_setattro(self, key, value, 0);
 }
 
 static PyType_Slot pycstruct_type_slots[] = {