From f82ca467553e84647aa3a64985528248af6ea67c Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 10 Oct 2023 16:04:50 +0300 Subject: [PATCH 1/4] gh-110525: Add tests for internal `set` CAPI --- Lib/test/test_capi/test_set.py | 43 +++++++++++++++- Modules/Setup.stdlib.in | 2 +- Modules/_testinternalcapi.c | 3 ++ Modules/_testinternalcapi/parts.h | 1 + Modules/_testinternalcapi/set.c | 83 +++++++++++++++++++++++++++++++ PCbuild/_testinternalcapi.vcxproj | 1 + 6 files changed, 130 insertions(+), 3 deletions(-) create mode 100644 Modules/_testinternalcapi/set.c diff --git a/Lib/test/test_capi/test_set.py b/Lib/test/test_capi/test_set.py index e9165e7e6806dd..68ce40344861ad 100644 --- a/Lib/test/test_capi/test_set.py +++ b/Lib/test/test_capi/test_set.py @@ -2,8 +2,9 @@ from test.support import import_helper -# Skip this test if the _testcapi module isn't available. +# Skip this test if the _testcapi or _testinternalcapi modules aren't available. _testcapi = import_helper.import_module('_testcapi') +_testinternalcapi = import_helper.import_module('_testinternalcapi') class set_subclass(set): pass @@ -12,13 +13,15 @@ class frozenset_subclass(frozenset): pass -class TestSetCAPI(unittest.TestCase): +class BaseSetTests: def assertImmutable(self, action, *args): self.assertRaises(SystemError, action, frozenset(), *args) self.assertRaises(SystemError, action, frozenset({1}), *args) self.assertRaises(SystemError, action, frozenset_subclass(), *args) self.assertRaises(SystemError, action, frozenset_subclass({1}), *args) + +class TestSetCAPI(BaseSetTests, unittest.TestCase): def test_set_check(self): check = _testcapi.set_check self.assertTrue(check(set())) @@ -213,3 +216,39 @@ def test_clear(self): clear(object()) self.assertImmutable(clear) # CRASHES: clear(NULL) + + +class TestInternalCAPI(BaseSetTests, unittest.TestCase): + def test_set_update(self): + update = _testinternalcapi.set_update + for cls in (set, set_subclass): + for it in ('ab', ('a', 'b'), ['a', 'b'], + set('ab'), set_subclass('ab'), + frozenset('ab'), frozenset_subclass('ab')): + with self.subTest(cls=cls, it=it): + instance = cls() + self.assertEqual(update(instance, it), 0) + self.assertEqual(instance, {'a', 'b'}) + instance = cls(it) + self.assertEqual(update(instance, it), 0) + self.assertEqual(instance, {'a', 'b'}) + with self.assertRaisesRegex(TypeError, 'object is not iterable'): + update(cls(), 1) + with self.assertRaisesRegex(TypeError, "unhashable type: 'dict'"): + update(cls(), [{}]) + with self.assertRaises(SystemError): + update(object(), 'ab') + self.assertImmutable(update, 'ab') + # CRASHES: update(NULL, object()) + # CRASHES: update(instance, NULL) + # CRASHES: update(NULL, NULL) + + def test_set_next_entry(self): + set_next = _testinternalcapi.set_next_entry + for cls in (set, set_subclass, frozenset, frozenset_subclass): + with self.subTest(cls=cls): + res = set_next(cls([1, 2, 3])) + self.assertEqual(set(res), {(1, 1, 1), (2, 2, 2), (3, 3, 3)}) + with self.assertRaises(SystemError): + set_next(object()) + # CRASHES: set_next(NULL) diff --git a/Modules/Setup.stdlib.in b/Modules/Setup.stdlib.in index 8428142a852529..647f44280b9ea1 100644 --- a/Modules/Setup.stdlib.in +++ b/Modules/Setup.stdlib.in @@ -158,7 +158,7 @@ @MODULE_XXSUBTYPE_TRUE@xxsubtype xxsubtype.c @MODULE__XXTESTFUZZ_TRUE@_xxtestfuzz _xxtestfuzz/_xxtestfuzz.c _xxtestfuzz/fuzzer.c @MODULE__TESTBUFFER_TRUE@_testbuffer _testbuffer.c -@MODULE__TESTINTERNALCAPI_TRUE@_testinternalcapi _testinternalcapi.c _testinternalcapi/test_lock.c _testinternalcapi/pytime.c +@MODULE__TESTINTERNALCAPI_TRUE@_testinternalcapi _testinternalcapi.c _testinternalcapi/test_lock.c _testinternalcapi/pytime.c _testinternalcapi/set.c @MODULE__TESTCAPI_TRUE@_testcapi _testcapimodule.c _testcapi/vectorcall.c _testcapi/vectorcall_limited.c _testcapi/heaptype.c _testcapi/abstract.c _testcapi/unicode.c _testcapi/dict.c _testcapi/set.c _testcapi/getargs.c _testcapi/datetime.c _testcapi/docstring.c _testcapi/mem.c _testcapi/watchers.c _testcapi/long.c _testcapi/float.c _testcapi/structmember.c _testcapi/exceptions.c _testcapi/code.c _testcapi/buffer.c _testcapi/pyatomic.c _testcapi/pyos.c _testcapi/immortal.c _testcapi/heaptype_relative.c _testcapi/gc.c @MODULE__TESTCLINIC_TRUE@_testclinic _testclinic.c @MODULE__TESTCLINIC_LIMITED_TRUE@_testclinic_limited _testclinic_limited.c diff --git a/Modules/_testinternalcapi.c b/Modules/_testinternalcapi.c index 05bac0936b155d..ddeb38938a331f 100644 --- a/Modules/_testinternalcapi.c +++ b/Modules/_testinternalcapi.c @@ -1602,6 +1602,9 @@ module_exec(PyObject *module) if (_PyTestInternalCapi_Init_PyTime(module) < 0) { return 1; } + if (_PyTestInternalCapi_Init_Set(module) < 0) { + return 1; + } if (PyModule_Add(module, "SIZEOF_PYGC_HEAD", PyLong_FromSsize_t(sizeof(PyGC_Head))) < 0) { diff --git a/Modules/_testinternalcapi/parts.h b/Modules/_testinternalcapi/parts.h index bbb8e62ddaf7a2..3d2774e3f1b404 100644 --- a/Modules/_testinternalcapi/parts.h +++ b/Modules/_testinternalcapi/parts.h @@ -12,5 +12,6 @@ int _PyTestInternalCapi_Init_Lock(PyObject *module); int _PyTestInternalCapi_Init_PyTime(PyObject *module); +int _PyTestInternalCapi_Init_Set(PyObject *module); #endif // Py_TESTINTERNALCAPI_PARTS_H diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c new file mode 100644 index 00000000000000..4265f900c653bb --- /dev/null +++ b/Modules/_testinternalcapi/set.c @@ -0,0 +1,83 @@ +#include "parts.h" +#include "../_testcapi/util.h" // NULLABLE, RETURN_INT + +#include "pycore_setobject.h" + + +static PyObject * +set_update(PyObject *self, PyObject *args) +{ + PyObject *set, *iterable; + if (!PyArg_ParseTuple(args, "OO", &set, &iterable)) { + return NULL; + } + NULLABLE(set); + NULLABLE(iterable); + RETURN_INT(_PySet_Update(set, iterable)); +} + +static PyObject * +set_next_entry(PyObject *self, PyObject *obj) +{ + NULLABLE(obj); + PyObject *result = PyList_New(0); + if (result == NULL) { + return NULL; + } + + int set_next = 0; + Py_ssize_t i = 0, count = 0; + Py_hash_t h; + PyObject *item; + while ((set_next = _PySet_NextEntry(obj, &i, &item, &h))) { + if (set_next == -1 && PyErr_Occurred()) { // obj is not a set + goto error; + } + + count++; + PyObject *index = PyLong_FromSsize_t(count); + if (index == NULL) { + goto error; + } + PyObject *hash = PyLong_FromSize_t((size_t)h); + if (hash == NULL) { + Py_DECREF(index); + goto error; + } + PyObject *tup = PyTuple_Pack(3, index, item, hash); + Py_DECREF(index); + Py_DECREF(item); + Py_DECREF(hash); + if (tup == NULL) { + goto error; + } + int res = PyList_Append(result, tup); + Py_DECREF(tup); + if (res < 0) { + goto error; + } + } + assert(count == PyList_GET_SIZE(result)); + return result; + +error: + Py_DECREF(result); + return NULL; +} + + +static PyMethodDef TestMethods[] = { + {"set_update", set_update, METH_VARARGS}, + {"set_next_entry", set_next_entry, METH_O}, + + {NULL}, +}; + +int +_PyTestInternalCapi_Init_Set(PyObject *m) +{ + if (PyModule_AddFunctions(m, TestMethods) < 0) { + return -1; + } + return 0; +} diff --git a/PCbuild/_testinternalcapi.vcxproj b/PCbuild/_testinternalcapi.vcxproj index fb474f06f38fe8..a729ab3877d91f 100644 --- a/PCbuild/_testinternalcapi.vcxproj +++ b/PCbuild/_testinternalcapi.vcxproj @@ -96,6 +96,7 @@ + From 338d340eaac7b42f377b184474e74bae2a42b527 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 10 Oct 2023 16:52:14 +0300 Subject: [PATCH 2/4] Address review: simplify `set_next_entry` --- Lib/test/test_capi/test_set.py | 17 +++++++--- Modules/_testinternalcapi/set.c | 58 +++++++++------------------------ 2 files changed, 29 insertions(+), 46 deletions(-) diff --git a/Lib/test/test_capi/test_set.py b/Lib/test/test_capi/test_set.py index 68ce40344861ad..7fd4f6b57e798f 100644 --- a/Lib/test/test_capi/test_set.py +++ b/Lib/test/test_capi/test_set.py @@ -247,8 +247,17 @@ def test_set_next_entry(self): set_next = _testinternalcapi.set_next_entry for cls in (set, set_subclass, frozenset, frozenset_subclass): with self.subTest(cls=cls): - res = set_next(cls([1, 2, 3])) - self.assertEqual(set(res), {(1, 1, 1), (2, 2, 2), (3, 3, 3)}) + instance = cls([1, 2, 3]) + pos = 0 + while True: + res = set_next(instance, pos) + if res is None: + break + rc, pos, hash_, item = res + self.assertEqual(rc, 1) + self.assertIsInstance(pos, int) + self.assertIn(item, instance) + self.assertEqual(hash(item), hash_) with self.assertRaises(SystemError): - set_next(object()) - # CRASHES: set_next(NULL) + set_next(object(), 0) + # CRASHES: set_next(NULL, 0) diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index 4265f900c653bb..00ec5506cdf84b 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -17,58 +17,32 @@ set_update(PyObject *self, PyObject *args) } static PyObject * -set_next_entry(PyObject *self, PyObject *obj) +set_next_entry(PyObject *self, PyObject *args) { - NULLABLE(obj); - PyObject *result = PyList_New(0); - if (result == NULL) { + int rc; + Py_ssize_t pos; + Py_hash_t hash; + PyObject *set, *item = UNINITIALIZED_PTR; + if (!PyArg_ParseTuple(args, "On", &set, &pos)) { return NULL; } + NULLABLE(set); - int set_next = 0; - Py_ssize_t i = 0, count = 0; - Py_hash_t h; - PyObject *item; - while ((set_next = _PySet_NextEntry(obj, &i, &item, &h))) { - if (set_next == -1 && PyErr_Occurred()) { // obj is not a set - goto error; - } - - count++; - PyObject *index = PyLong_FromSsize_t(count); - if (index == NULL) { - goto error; - } - PyObject *hash = PyLong_FromSize_t((size_t)h); - if (hash == NULL) { - Py_DECREF(index); - goto error; - } - PyObject *tup = PyTuple_Pack(3, index, item, hash); - Py_DECREF(index); - Py_DECREF(item); - Py_DECREF(hash); - if (tup == NULL) { - goto error; - } - int res = PyList_Append(result, tup); - Py_DECREF(tup); - if (res < 0) { - goto error; - } + rc = _PySet_NextEntry(set, &pos, &item, &hash); + if (rc == 1) { + return Py_BuildValue("innO", rc, pos, hash, item); } - assert(count == PyList_GET_SIZE(result)); - return result; - -error: - Py_DECREF(result); - return NULL; + assert(item == UNINITIALIZED_PTR); + if (PyErr_Occurred()) { + return NULL; + } + Py_RETURN_NONE; } static PyMethodDef TestMethods[] = { {"set_update", set_update, METH_VARARGS}, - {"set_next_entry", set_next_entry, METH_O}, + {"set_next_entry", set_next_entry, METH_VARARGS}, {NULL}, }; From 21ca91f6fea7d6edbe8ff9765c881d146b8741a1 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 10 Oct 2023 18:00:23 +0300 Subject: [PATCH 3/4] Address review --- Lib/test/test_capi/test_set.py | 6 ++++-- Modules/_testinternalcapi/set.c | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_capi/test_set.py b/Lib/test/test_capi/test_set.py index 7fd4f6b57e798f..5235f81543e0b6 100644 --- a/Lib/test/test_capi/test_set.py +++ b/Lib/test/test_capi/test_set.py @@ -247,17 +247,19 @@ def test_set_next_entry(self): set_next = _testinternalcapi.set_next_entry for cls in (set, set_subclass, frozenset, frozenset_subclass): with self.subTest(cls=cls): - instance = cls([1, 2, 3]) + instance = cls('abc') pos = 0 + items = [] while True: res = set_next(instance, pos) if res is None: break rc, pos, hash_, item = res + items.append(item) self.assertEqual(rc, 1) - self.assertIsInstance(pos, int) self.assertIn(item, instance) self.assertEqual(hash(item), hash_) + self.assertEqual(items, list(instance)) with self.assertRaises(SystemError): set_next(object(), 0) # CRASHES: set_next(NULL, 0) diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index 00ec5506cdf84b..352a2aceb1662b 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -21,7 +21,7 @@ set_next_entry(PyObject *self, PyObject *args) { int rc; Py_ssize_t pos; - Py_hash_t hash; + Py_hash_t hash = (Py_hash_t)UNINITIALIZED_SIZE; PyObject *set, *item = UNINITIALIZED_PTR; if (!PyArg_ParseTuple(args, "On", &set, &pos)) { return NULL; @@ -33,7 +33,8 @@ set_next_entry(PyObject *self, PyObject *args) return Py_BuildValue("innO", rc, pos, hash, item); } assert(item == UNINITIALIZED_PTR); - if (PyErr_Occurred()) { + assert(hash == (Py_hash_t)UNINITIALIZED_SIZE); + if (rc == -1) { return NULL; } Py_RETURN_NONE; From e4ff92bd88d344fc5b2a3f3390fc69b4ba684a46 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 10 Oct 2023 18:13:30 +0300 Subject: [PATCH 4/4] Add assertion that `rc` cannot be anything else than `[-1, 0, 1]` --- Modules/_testinternalcapi/set.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index 352a2aceb1662b..0305a7885d217c 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -37,6 +37,7 @@ set_next_entry(PyObject *self, PyObject *args) if (rc == -1) { return NULL; } + assert(rc == 0); Py_RETURN_NONE; }