Apply suggestions from code review

Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com>

Author: jbosboom

Doc/library/os.rst

@@ -3411,35 +3411,46 @@ features:
 
 .. class:: statx_result
 
-   Object whose attributes correspond roughly to the members of the
-   :c:struct:`!statx` structure. It is used for the result of :func:`os.statx`.
-   :class:`!statx_result` has all of the attributes of :class:`stat_result`
-   available on Linux, but is not a subclass of :class:`stat_result` nor a
-   tuple.  :class:`!statx_result` has the following additional attributes:
+   Information about a file returned by :func:`os.statx`.
+
+   :class:`!statx_result` has all the attributes that :class:`~stat_result` has
+   on Linux, making it :term:`duck-typing` compatible, but
+   :class:`!statx_result` is not a subclass of :class:`~stat_result` and cannot
+   be used as a tuple.
+
+   :class:`!statx_result` has the following additional attributes:
 
    .. attribute:: stx_mask
 
       Bitmask of :const:`STATX_* <STATX_TYPE>` constants specifying the
-      information retrieved, which may differ from what was requested depending
-      on the filesystem, filesystem type, and kernel version.  All attributes
-      of this class are accessible regardless of the value of
-      :attr:`!stx_mask`, and they may have useful fictitious values.  For
-      example, for a file on a network filesystem, :const:`STATX_UID` and
-      :const:`STATX_GID` may be unset because file ownership on the server is
-      based on an external user database, but :attr:`!st_uid` and
-      :attr:`!st_gid` may contain the IDs of the local user who controls the
-      mount.
+      information retrieved, which may differ from what was requested.
 
    .. attribute:: stx_attributes_mask
 
-      Bitmask of :const:`!STATX_ATTR_* <stat.STATX_ATTR_COMPRESSED>` constants
+      Bitmask of :const:`STATX_ATTR_* <stat.STATX_ATTR_COMPRESSED>` constants
       specifying the attributes bits supported for this file.
 
    .. attribute:: stx_attributes
 
-      Bitmask of :const:`!STATX_ATTR_* <stat.STATX_ATTR_COMPRESSED>` constants
+      Bitmask of :const:`STATX_ATTR_* <stat.STATX_ATTR_COMPRESSED>` constants
       specifying the attributes of this file.
 
+   .. attribute:: stx_dev_major
+
+      Major number of the device on which this file resides.
+
+   .. attribute:: stx_dev_minor
+
+      Minor number of the device on which this file resides.
+
+   .. attribute:: stx_rdev_major
+
+      Major number of the device this file represents.
+
+   .. attribute:: stx_rdev_minor
+
+      Minor number of the device this file represents.
+
    .. attribute:: stx_mnt_id
 
       Mount ID.

Lib/test/test_os/test_os.py

@@ -764,6 +764,7 @@ def check_statx_attributes(self, fname):
             if result.stx_mask & bits == bits and hasattr(basic_result, name):
                 x = getattr(result, name)
                 b = getattr(basic_result, name)
+                self.assertEqual(type(x), type(b))
                 if isinstance(x, float):
                     self.assertAlmostEqual(x, b, msg=name)
                 else:
@@ -786,6 +787,12 @@ def check_statx_attributes(self, fname):
         self.assertEqual(result.stx_attributes & result.stx_attributes_mask,
                          result.stx_attributes)
 
+        # statx_result is not a tuple or tuple-like object.
+        with self.assertRaisesRegex(TypeError, 'not subscriptable'):
+            result[0]
+        with self.assertRaisesRegex(TypeError, 'cannot unpack'):
+            _, _ = result
+
     @unittest.skipUnless(hasattr(os, 'statx'), 'test needs os.statx()')
     def test_statx_attributes(self):
         self.check_statx_attributes(self.fname)

Modules/posixmodule.c

@@ -2632,6 +2632,7 @@ stat_nanosecond_timestamp(_posixstate *state, time_t sec, unsigned long nsec)
         return PyLong_FromLongLong(sec * SEC_TO_NS + nsec);
     }
     else {
+        PyObject *ns_total = NULL;
         PyObject *s_in_ns = NULL;
         PyObject *s = _PyLong_FromTime_t(sec);
         PyObject *ns_fractional = PyLong_FromUnsignedLong(nsec);
@@ -2644,17 +2645,13 @@ stat_nanosecond_timestamp(_posixstate *state, time_t sec, unsigned long nsec)
             goto exit;
         }
 
-        PyObject *ns_total = PyNumber_Add(s_in_ns, ns_fractional);
-        if (ns_total == NULL) {
-            goto exit;
-        }
-        return ns_total;
+        ns_total = PyNumber_Add(s_in_ns, ns_fractional);
 
     exit:
         Py_XDECREF(s);
         Py_XDECREF(ns_fractional);
         Py_XDECREF(s_in_ns);
-        return NULL;
+        return ns_total;
     }
 }
 
@@ -3336,7 +3333,7 @@ static PyMemberDef pystatx_result_members[] = {
     MM(st_size, Py_T_ULONGLONG, size, "total size, in bytes"),
     MM(st_blocks, Py_T_ULONGLONG, blocks, "number of blocks allocated"),
     MM(stx_attributes_mask, Py_T_ULONGLONG, attributes_mask,
-        "Linux inode attribute bits supported for this file"),
+        "Mask of supported bits in stx_attributes"),
     MX(st_atime, Py_T_DOUBLE, atime_sec, "time of last access"),
     MX(st_birthtime, Py_T_DOUBLE, btime_sec, "time of creation"),
     MX(st_ctime, Py_T_DOUBLE, ctime_sec, "time of last change"),
@@ -3366,6 +3363,7 @@ pystatx_result_get_u32(PyObject *op, void *context) {
     Py_statx_result *self = (Py_statx_result *) op;
     uint16_t offset = (uintptr_t)context;
     uint32_t val;
+    assert(offset + sizeof(val) <= sizeof(Py_statx_result));
     memcpy(&val, (void *)self + offset, sizeof(val));
     return PyLong_FromUInt32(val);
 }
@@ -3376,6 +3374,7 @@ pystatx_result_get_nsec(PyObject *op, void *context)
     Py_statx_result *self = (Py_statx_result *) op;
     uint16_t offset = (uintptr_t)context;
     struct statx_timestamp val;
+    assert(offset + sizeof(val) <= sizeof(Py_statx_result));
     memcpy(&val, (void *)self + offset, sizeof(val));
     _posixstate *state = PyType_GetModuleState(Py_TYPE(op));
     assert(state != NULL);
@@ -3561,8 +3560,9 @@ os_statx_impl(PyObject *module, path_t *path, unsigned int mask, int flags,
 {
     if (path_and_dir_fd_invalid("statx", path, dir_fd) ||
         dir_fd_and_fd_invalid("statx", dir_fd, path->fd) ||
-        fd_and_follow_symlinks_invalid("statx", path->fd, follow_symlinks))
+        fd_and_follow_symlinks_invalid("statx", path->fd, follow_symlinks)) {
         return NULL;
+    }
 
     /* reject flags covered by kwargs, but allow unknown flags that may be
        future AT_STATX_* extensions */