Repository: python/cpython
Pull request: gh-110235: Raise `TypeError` for duplicate/unknown fields in PyStructSequence constructor
<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
gh-NNNNN: Summary of the changes made
```

Where: gh-NNNNN refers to the GitHub issue number.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `main`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `main`.

-->

Fixes #110235

In this PR, we iterate over `dict` keys and check if there are duplicate fields provided by `sequence` or there are any unexpected field names.


<!-- gh-issue-number: gh-110235 -->
* Issue: gh-110235
<!-- /gh-issue-number -->


Problem statement:
PyStructSequence constructor ignores unknown field names
# Bug report

PyStructSequence constructor takes two arguments: tuple and dict. The tuple specifies values for "visible" fields (i.e. what you get when interpret PyStructSequence as a tuple), the dict specifies values for fields that are accessible only by name (they are like `__slot__` attributes).

```pyshell
>>> import time
>>> time.struct_time((2023, 10, 2, 17, 50, 53, 0, 275, 0), {'tm_zone': 'GMT', 'tm_gmtoff': 0})
time.struct_time(tm_year=2023, tm_mon=10, tm_mday=2, tm_hour=17, tm_min=50, tm_sec=53, tm_wday=0, tm_yday=275, tm_isdst=0)
```

The problem is that all invalid names are silently ignored. It includes names of "visible" fields and typos.

```pyshell
>>> time.struct_time((0,)*9, {'tm_zone': 'GMT', 'tm_gmtoff': 0, 'tm_year': 2023, 'invalid': 1})
time.struct_time(tm_year=0, tm_mon=0, tm_mday=0, tm_hour=0, tm_min=0, tm_sec=0, tm_wday=0, tm_yday=0, tm_isdst=0)
```


<!-- gh-linked-prs -->
### Linked PRs
* gh-110258
<!-- /gh-linked-prs -->

Hints from issue discussion:
The original code was added by @mwhudson in bpo-526072 (#36205, ce358e3015bc8cafdf949dee624b01f4a38bfad2). Because it was only for pickling support, omitting some validity checks was acceptable.
@XuehaiPan, it may be interesting to you, since you already do something similar in the `__replace__()`.

The difference is that this code cannot modify the input dict, because it is not kwargs. But it can count the found names.
> The problem is that all invalid names are silently ignored. It includes names of "visible" fields and typos.

Any thoughts on allowing the present visible fields in `dict` to override the values in the input `sequence`? Currently, we ignore visible field names in `dict`.

```python
PyStructSequence.__new__(cls: Self, sequence: Iterable[Any], dict: dict[str, Any]) -> Self
```

Since we need `n_visible_fields <= len(sequence) <= n_fields` in `__new__()`, all visible fields should be present in the `sequence` argument. If a field value is not allowed to come from both `sequence` and `dict`, the `dict` argument should only contain invisible field names. Also, we need to check if `len(sequence) > n_visible_fields`, the given invisible field provided in `sequence` should not present again in `dict`.

------

For `collections.namedtuple`, it will raise `TypeError` for duplicate arguments:

```python
namedtuple.__new__(cls: Self, *args: Any, **kwargs: Any) -> Self
```

```python
>>> import collections
>>> MyTuple = collections.namedtuple('MyTuple', ['a', 'b', 'c', 'd'])
>>> MyTuple(1, 2, 3, 4, a=5, b=6)
TypeError: MyTuple.__new__() got multiple values for argument 'a'
```
I opened a PR to resolve this.

- #110258

Commit message: Add more test cases

Patch to review:
diff --git a/Lib/test/test_structseq.py b/Lib/test/test_structseq.py
index a9fe193028ebe4..1968fbd6ac27a9 100644
--- a/Lib/test/test_structseq.py
+++ b/Lib/test/test_structseq.py
@@ -1,4 +1,5 @@
 import os
+import re
 import time
 import unittest
 
@@ -93,6 +94,53 @@ def test_constructor(self):
         s = "123456789"
         self.assertEqual("".join(t(s)), s)
 
+    def test_constructor_with_duplicate_fields(self):
+        t = time.struct_time
+        self.assertEqual(t("123456789"), tuple("123456789"))
+        self.assertEqual(t("1234567890"), tuple("123456789"))
+        self.assertEqual(t("1234567890").tm_zone, "0")
+        self.assertEqual(t("123456789", dict={"tm_zone": "some zone"}), tuple("123456789"))
+        self.assertEqual(t("123456789", dict={"tm_zone": "some zone"}).tm_zone, "some zone")
+
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_year'"):
+            t("123456789", dict={"tm_year": 0})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_year'"):
+            t("123456789", dict={"tm_year": 0, "tm_mon": 1})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_mon'"):
+            t("123456789", dict={"tm_zone": "some zone", "tm_mon": 1})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_mon'"):
+            t("123456789", dict={"tm_zone": "some zone", "tm_mon": 1, "error": 0})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_mon'"):
+            t("123456789", dict={"error": 0, "tm_zone": "some zone", "tm_mon": 1})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_zone'"):
+            t("1234567890", dict={"tm_zone": "some zone"})
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'tm_zone'"):
+            t("1234567890", dict={"error": 0, "tm_zone": "some zone"})
+
+    def test_constructor_with_duplicate_unnamed_fields(self):
+        assert os.stat_result.n_unnamed_fields > 0
+        n_visible_fields = os.stat_result.n_sequence_fields
+
+        r = os.stat_result(range(n_visible_fields), {'st_atime': -1.0})
+        self.assertEqual(r.st_atime, -1.0)
+        self.assertEqual(r, tuple(range(n_visible_fields)))
+
+        r = os.stat_result((*range(n_visible_fields), -1.0))
+        self.assertEqual(r.st_atime, -1.0)
+        self.assertEqual(r, tuple(range(n_visible_fields)))
+
+        with self.assertRaisesRegex(TypeError, "got multiple values for field 'st_atime'"):
+            os.stat_result((*range(n_visible_fields), -1.0), {'st_atime': -1.0})
+
+    def test_constructor_with_unknown_fields(self):
+        t = time.struct_time
+        with self.assertRaisesRegex(TypeError,
+                                    re.escape("got unexpected field name(s): {'error'}")):
+            t("123456789", dict={"error": 0})
+        with self.assertRaisesRegex(TypeError,
+                                    re.escape("got unexpected field name(s): {'error'}")):
+            t("123456789", dict={"tm_zone": "some zone", "error": 0})
+
     def test_eviltuple(self):
         class Exc(Exception):
             pass
@@ -108,7 +156,23 @@ def __len__(self):
 
     def test_reduce(self):
         t = time.gmtime()
-        x = t.__reduce__()
+        cls, (tup, dct) = t.__reduce__()
+        self.assertIs(cls, time.struct_time)
+        self.assertEqual(tup, tuple(t))
+        reconstructed = cls(tup, dct)
+        self.assertEqual(reconstructed, t)
+        self.assertEqual(reconstructed.tm_zone, t.tm_zone)
+
+    def test_reduce_with_unnamed_fields(self):
+        r = os.stat_result(range(os.stat_result.n_sequence_fields), {'st_atime': 1.0})
+        self.assertEqual(r.st_atime, 1.0)
+        cls, (tup, dct) = r.__reduce__()
+        self.assertIs(cls, os.stat_result)
+        self.assertEqual(tup, tuple(r))
+        self.assertIn('st_atime', dct)
+        reconstructed = cls(tup, dct)
+        self.assertEqual(reconstructed, r)
+        self.assertAlmostEqual(reconstructed.st_atime, r.st_atime)
 
     def test_extended_getslice(self):
         # Test extended slicing by comparing with list slicing.
diff --git a/Misc/NEWS.d/next/C API/2023-10-03-06-19-10.gh-issue-110235.uec5AG.rst b/Misc/NEWS.d/next/C API/2023-10-03-06-19-10.gh-issue-110235.uec5AG.rst
new file mode 100644
index 00000000000000..ff26f25fe71d61
--- /dev/null
+++ b/Misc/NEWS.d/next/C API/2023-10-03-06-19-10.gh-issue-110235.uec5AG.rst	
@@ -0,0 +1,2 @@
+Raise :exc:`TypeError` for duplicate/unknown fields in ``PyStructSequence`` constructor.
+Patched by Xuehai Pan.
diff --git a/Objects/structseq.c b/Objects/structseq.c
index 0ca622edc2ba37..2adf8010e51eef 100644
--- a/Objects/structseq.c
+++ b/Objects/structseq.c
@@ -216,10 +216,67 @@ structseq_new_impl(PyTypeObject *type, PyObject *arg, PyObject *dict)
         res->ob_item[i] = Py_NewRef(v);
     }
     Py_DECREF(arg);
-    for (; i < max_len; ++i) {
+    if (dict != NULL) {
+        for (i = 0; i < len; ++i) {
+            // unnamed fields can be present in both sequence and dict
+            if (i >= min_len - n_unnamed_fields && i < min_len) {
+                continue;
+            }
+
+            const char *name = type->tp_members[i < min_len ? i : i - n_unnamed_fields].name;
+            PyObject *ob = NULL;
+            if (PyDict_GetItemStringRef(dict, name, &ob) < 0) {
+                Py_DECREF(res);
+                return NULL;
+            }
+            if (ob != NULL) {
+                Py_DECREF(ob);
+                PyErr_Format(PyExc_TypeError,
+                             "%.500s() got multiple values for field '%s'",
+                             type->tp_name, name);
+                Py_DECREF(res);
+                return NULL;
+            }
+        }
+        PyObject *unknown_keys = PySet_New(dict);
+        if (unknown_keys == NULL) {
+            Py_DECREF(res);
+            return NULL;
+        }
+        for (i = min_len; i < max_len; ++i) {
+            PyObject *ob = NULL;
+            const char *name = type->tp_members[i - n_unnamed_fields].name;
+            PyObject *key = PyUnicode_FromString(name);
+            if (!key || PyDict_GetItemRef(dict, key, &ob) < 0) {
+                Py_XDECREF(key);
+                Py_DECREF(res);
+                Py_DECREF(unknown_keys);
+                return NULL;
+            }
+            if (ob != NULL) {
+                Py_DECREF(ob);
+                if (PySet_Discard(unknown_keys, key) < 0) {
+                    Py_DECREF(key);
+                    Py_DECREF(res);
+                    Py_DECREF(unknown_keys);
+                    return NULL;
+                }
+            }
+            Py_DECREF(key);
+        }
+        if (PySet_GET_SIZE(unknown_keys) > 0) {
+            PyErr_Format(PyExc_TypeError,
+                         "%.500s() got unexpected field name(s): %R",
+                         type->tp_name, unknown_keys);
+            Py_DECREF(res);
+            Py_DECREF(unknown_keys);
+            return NULL;
+        }
+    }
+    for (i = len; i < max_len; ++i) {
         PyObject *ob = NULL;
         if (dict != NULL) {
-            const char *name = type->tp_members[i-n_unnamed_fields].name;
+            const char *name = type->tp_members[i - n_unnamed_fields].name;
             if (PyDict_GetItemStringRef(dict, name, &ob) < 0) {
                 Py_DECREF(res);
                 return NULL;


Code review comments:
diff_hunk: @@ -93,6 +94,53 @@ def test_constructor(self):
         s = "123456789"
         self.assertEqual("".join(t(s)), s)
 
+    def test_constructor_with_duplicate_fields(self):
+        t = time.struct_time
+        self.assertEqual(t("123456789"), tuple("123456789"))
+        self.assertEqual(t("1234567890"), tuple("123456789"))
+        self.assertEqual(t("1234567890").tm_zone, "0")
+        self.assertEqual(t("123456789", dict={"tm_zone": "some zone"}), tuple("123456789"))
+        self.assertEqual(t("123456789", dict={"tm_zone": "some zone"}).tm_zone, "some zone")
path: Lib/test/test_structseq.py
text: @user1:
These tests do not actually test duplicate fields, so it is better to move them to `test_constructor`.

There is a test for positional first argument and keyword "dict" argument. I think that it is better to pass both arguments as positional for most tests, but add also a case for both passed by keyword.