From 0ccc0f6c7495be9043300e22d8f38e6d65e8884f Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Sun, 8 Oct 2017 11:17:46 +0300 Subject: [PATCH] bpo-28280: Make PyMapping_Keys(), PyMapping_Values() and PyMapping_Items() always return a list (#3840) --- Doc/c-api/mapping.rst | 21 +++-- Doc/whatsnew/3.7.rst | 4 + Lib/test/test_capi.py | 46 ++++++++++- .../2017-09-30-19-41-44.bpo-28280.K_EjpO.rst | 2 + Modules/_testcapimodule.c | 22 ++++++ Objects/abstract.c | 76 ++++++++++++------- 6 files changed, 137 insertions(+), 34 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst diff --git a/Doc/c-api/mapping.rst b/Doc/c-api/mapping.rst index a71e9428377..308a9761f87 100644 --- a/Doc/c-api/mapping.rst +++ b/Doc/c-api/mapping.rst @@ -50,20 +50,29 @@ Mapping Protocol .. c:function:: PyObject* PyMapping_Keys(PyObject *o) - On success, return a list or tuple of the keys in object *o*. On failure, - return *NULL*. + On success, return a list of the keys in object *o*. On failure, return + *NULL*. + + .. versionchanged:: 3.7 + Previously, the function returned a list or a tuple. .. c:function:: PyObject* PyMapping_Values(PyObject *o) - On success, return a list or tuple of the values in object *o*. On failure, - return *NULL*. + On success, return a list of the values in object *o*. On failure, return + *NULL*. + + .. versionchanged:: 3.7 + Previously, the function returned a list or a tuple. .. c:function:: PyObject* PyMapping_Items(PyObject *o) - On success, return a list or tuple of the items in object *o*, where each item - is a tuple containing a key-value pair. On failure, return *NULL*. + On success, return a list of the items in object *o*, where each item is a + tuple containing a key-value pair. On failure, return *NULL*. + + .. versionchanged:: 3.7 + Previously, the function returned a list or a tuple. .. c:function:: PyObject* PyMapping_GetItemString(PyObject *o, const char *key) diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst index ecdd2fe2713..24a1dc4d13b 100644 --- a/Doc/whatsnew/3.7.rst +++ b/Doc/whatsnew/3.7.rst @@ -404,6 +404,10 @@ Build and C API Changes is now of type ``const char *`` rather of ``char *``. (Contributed by Serhiy Storchaka in :issue:`28769`.) +* The result of :c:func:`PyMapping_Keys`, :c:func:`PyMapping_Values` and + :c:func:`PyMapping_Items` is now always a list, rather than a list or a + tuple. (Contributed by Oren Milman in :issue:`28280`.) + * Added functions :c:func:`PySlice_Unpack` and :c:func:`PySlice_AdjustIndices`. (Contributed by Serhiy Storchaka in :issue:`27867`.) diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 1b826ee8d9a..921735ef5d0 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -1,7 +1,7 @@ # Run the _testcapi module tests (tests for the Python/C API): by defn, # these are all functions _testcapi exports whose name begins with 'test_'. -from collections import namedtuple +from collections import namedtuple, OrderedDict import os import pickle import platform @@ -272,6 +272,50 @@ class CAPITest(unittest.TestCase): self.assertIn(b'MemoryError 2 20', out) self.assertIn(b'MemoryError 3 30', out) + def test_mapping_keys_values_items(self): + class Mapping1(dict): + def keys(self): + return list(super().keys()) + def values(self): + return list(super().values()) + def items(self): + return list(super().items()) + class Mapping2(dict): + def keys(self): + return tuple(super().keys()) + def values(self): + return tuple(super().values()) + def items(self): + return tuple(super().items()) + dict_obj = {'foo': 1, 'bar': 2, 'spam': 3} + + for mapping in [{}, OrderedDict(), Mapping1(), Mapping2(), + dict_obj, OrderedDict(dict_obj), + Mapping1(dict_obj), Mapping2(dict_obj)]: + self.assertListEqual(_testcapi.get_mapping_keys(mapping), + list(mapping.keys())) + self.assertListEqual(_testcapi.get_mapping_values(mapping), + list(mapping.values())) + self.assertListEqual(_testcapi.get_mapping_items(mapping), + list(mapping.items())) + + def test_mapping_keys_values_items_bad_arg(self): + self.assertRaises(AttributeError, _testcapi.get_mapping_keys, None) + self.assertRaises(AttributeError, _testcapi.get_mapping_values, None) + self.assertRaises(AttributeError, _testcapi.get_mapping_items, None) + + class BadMapping: + def keys(self): + return None + def values(self): + return None + def items(self): + return None + bad_mapping = BadMapping() + self.assertRaises(TypeError, _testcapi.get_mapping_keys, bad_mapping) + self.assertRaises(TypeError, _testcapi.get_mapping_values, bad_mapping) + self.assertRaises(TypeError, _testcapi.get_mapping_items, bad_mapping) + class TestPendingCalls(unittest.TestCase): diff --git a/Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst b/Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst new file mode 100644 index 00000000000..76990f79cb0 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst @@ -0,0 +1,2 @@ +Make `PyMapping_Keys()`, `PyMapping_Values()` and `PyMapping_Items()` always +return a `list` (rather than a `list` or a `tuple`). Patch by Oren Milman. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index b512c05a3fe..6e31105712f 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -4306,6 +4306,25 @@ py_w_stopcode(PyObject *self, PyObject *args) #endif +static PyObject * +get_mapping_keys(PyObject* self, PyObject *obj) +{ + return PyMapping_Keys(obj); +} + +static PyObject * +get_mapping_values(PyObject* self, PyObject *obj) +{ + return PyMapping_Values(obj); +} + +static PyObject * +get_mapping_items(PyObject* self, PyObject *obj) +{ + return PyMapping_Items(obj); +} + + static PyObject * test_pythread_tss_key_state(PyObject *self, PyObject *args) { @@ -4573,6 +4592,9 @@ static PyMethodDef TestMethods[] = { #ifdef W_STOPCODE {"W_STOPCODE", py_w_stopcode, METH_VARARGS}, #endif + {"get_mapping_keys", get_mapping_keys, METH_O}, + {"get_mapping_values", get_mapping_values, METH_O}, + {"get_mapping_items", get_mapping_items, METH_O}, {"test_pythread_tss_key_state", test_pythread_tss_key_state, METH_VARARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/abstract.c b/Objects/abstract.c index 38484b79a1b..3cb7a32b01e 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2147,55 +2147,77 @@ PyMapping_HasKey(PyObject *o, PyObject *key) return 0; } +/* This function is quite similar to PySequence_Fast(), but specialized to be + a helper for PyMapping_Keys(), PyMapping_Items() and PyMapping_Values(). + */ +static PyObject * +method_output_as_list(PyObject *o, _Py_Identifier *meth_id) +{ + PyObject *it, *result, *meth_output; + + assert(o != NULL); + meth_output = _PyObject_CallMethodId(o, meth_id, NULL); + if (meth_output == NULL || PyList_CheckExact(meth_output)) { + return meth_output; + } + it = PyObject_GetIter(meth_output); + if (it == NULL) { + if (PyErr_ExceptionMatches(PyExc_TypeError)) { + PyErr_Format(PyExc_TypeError, + "%.200s.%U() returned a non-iterable (type %.200s)", + Py_TYPE(o)->tp_name, + meth_id->object, + Py_TYPE(meth_output)->tp_name); + } + Py_DECREF(meth_output); + return NULL; + } + Py_DECREF(meth_output); + result = PySequence_List(it); + Py_DECREF(it); + return result; +} + PyObject * PyMapping_Keys(PyObject *o) { - PyObject *keys; - PyObject *fast; _Py_IDENTIFIER(keys); - if (PyDict_CheckExact(o)) + if (o == NULL) { + return null_error(); + } + if (PyDict_CheckExact(o)) { return PyDict_Keys(o); - keys = _PyObject_CallMethodId(o, &PyId_keys, NULL); - if (keys == NULL) - return NULL; - fast = PySequence_Fast(keys, "o.keys() are not iterable"); - Py_DECREF(keys); - return fast; + } + return method_output_as_list(o, &PyId_keys); } PyObject * PyMapping_Items(PyObject *o) { - PyObject *items; - PyObject *fast; _Py_IDENTIFIER(items); - if (PyDict_CheckExact(o)) + if (o == NULL) { + return null_error(); + } + if (PyDict_CheckExact(o)) { return PyDict_Items(o); - items = _PyObject_CallMethodId(o, &PyId_items, NULL); - if (items == NULL) - return NULL; - fast = PySequence_Fast(items, "o.items() are not iterable"); - Py_DECREF(items); - return fast; + } + return method_output_as_list(o, &PyId_items); } PyObject * PyMapping_Values(PyObject *o) { - PyObject *values; - PyObject *fast; _Py_IDENTIFIER(values); - if (PyDict_CheckExact(o)) + if (o == NULL) { + return null_error(); + } + if (PyDict_CheckExact(o)) { return PyDict_Values(o); - values = _PyObject_CallMethodId(o, &PyId_values, NULL); - if (values == NULL) - return NULL; - fast = PySequence_Fast(values, "o.values() are not iterable"); - Py_DECREF(values); - return fast; + } + return method_output_as_list(o, &PyId_values); } /* isinstance(), issubclass() */