From bd59df153f0d8c37ff2a6b95a715f2558d0d7298 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 17:32:05 +0300
Subject: [PATCH 01/12] init commit

---
 Doc/c-api/mapping.rst     | 12 ++++-----
 Lib/test/test_capi.py     | 47 +++++++++++++++++++++++++++++++++++-
 Modules/_testcapimodule.c | 24 ++++++++++++++++++
 Objects/abstract.c        | 51 +++++++++++++++++++++++++++------------
 4 files changed, 112 insertions(+), 22 deletions(-)

diff --git a/Doc/c-api/mapping.rst b/Doc/c-api/mapping.rst
index a71e94283776e9..e8765888c6ab5d 100644
--- a/Doc/c-api/mapping.rst
+++ b/Doc/c-api/mapping.rst
@@ -50,20 +50,20 @@ 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*.
 
 
 .. 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*.
 
 
 .. 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*.
 
 
 .. c:function:: PyObject* PyMapping_GetItemString(PyObject *o, const char *key)
diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 1b826ee8d9a64d..5b40c9352f9f1c 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,51 @@ class C(): pass
         self.assertIn(b'MemoryError 2 20', out)
         self.assertIn(b'MemoryError 3 30', out)
 
+    def test_mapping_keys_values_items(self):
+        class MyMapping1(dict):
+            def keys(self):
+                return list(super().keys())
+            def values(self):
+                return list(super().values())
+            def items(self):
+                return list(super().items())
+        class MyMapping1(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}
+        odict = OrderedDict(dict_obj)
+        mapping1 = MyMapping1(dict_obj)
+        mapping2 = MyMapping1(dict_obj)
+
+        for mapping in [dict_obj, odict, mapping1, mapping2]:
+            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/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index 1a296214739c63..e198eaf04edc89 100644
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -4306,6 +4306,27 @@ 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 PyMethodDef TestMethods[] = {
     {"raise_exception",         raise_exception,                 METH_VARARGS},
     {"raise_memoryerror",   (PyCFunction)raise_memoryerror,  METH_NOARGS},
@@ -4518,6 +4539,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},
     {NULL, NULL} /* sentinel */
 };
 
diff --git a/Objects/abstract.c b/Objects/abstract.c
index 38484b79a1b89f..a69072138d3ea9 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2147,55 +2147,76 @@ PyMapping_HasKey(PyObject *o, PyObject *key)
     return 0;
 }
 
+static PyObject *
+method_output_as_list(PyObject *o, const char *err_msg)
+{
+    PyObject *it, *result;
+
+    assert(o != NULL);
+    it = PyObject_GetIter(o);
+    if (it == NULL) {
+        if (PyErr_ExceptionMatches(PyExc_TypeError)) {
+            PyErr_SetString(PyExc_TypeError, err_msg);
+        }
+        return NULL;
+    }
+    result = PySequence_List(it);
+    Py_DECREF(it);
+    return result;
+}
+
 PyObject *
 PyMapping_Keys(PyObject *o)
 {
     PyObject *keys;
-    PyObject *fast;
+    PyObject *keys_list;
     _Py_IDENTIFIER(keys);
 
     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");
+    if (keys == NULL || PyList_CheckExact(keys)) {
+        return keys;
+    }
+    keys_list = method_output_as_list(keys, "o.keys() is not iterable");
     Py_DECREF(keys);
-    return fast;
+    return keys_list;
 }
 
 PyObject *
 PyMapping_Items(PyObject *o)
 {
     PyObject *items;
-    PyObject *fast;
+    PyObject *items_list;
     _Py_IDENTIFIER(items);
 
     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");
+    if (items == NULL || PyList_CheckExact(items)) {
+        return items;
+    }
+    items_list = method_output_as_list(items, "o.items() is not iterable");
     Py_DECREF(items);
-    return fast;
+    return items_list;
 }
 
 PyObject *
 PyMapping_Values(PyObject *o)
 {
     PyObject *values;
-    PyObject *fast;
+    PyObject *values_list;
     _Py_IDENTIFIER(values);
 
     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");
+    if (values == NULL || PyList_CheckExact(values)) {
+        return values;
+    }
+    values_list = method_output_as_list(values, "o.values() is not iterable");
     Py_DECREF(values);
-    return fast;
+    return values_list;
 }
 
 /* isinstance(), issubclass() */

From 15c14866ade1256519f830ba655ca6206c0399ea Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 17:51:54 +0300
Subject: [PATCH 02/12] minimize code duplication

---
 Lib/test/test_capi.py |  4 ++--
 Objects/abstract.c    | 49 ++++++++++++++++---------------------------
 2 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 5b40c9352f9f1c..8a78bfb3b05d1f 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -288,11 +288,11 @@ def values(self):
             def items(self):
                 return tuple(super().items())
         dict_obj = {'foo': 1, 'bar': 2, 'spam': 3}
-        odict = OrderedDict(dict_obj)
+        odict_obj = OrderedDict(dict_obj)
         mapping1 = MyMapping1(dict_obj)
         mapping2 = MyMapping1(dict_obj)
 
-        for mapping in [dict_obj, odict, mapping1, mapping2]:
+        for mapping in [dict_obj, odict_obj, mapping1, mapping2]:
             self.assertListEqual(_testcapi.get_mapping_keys(mapping),
                                  list(mapping.keys()))
             self.assertListEqual(_testcapi.get_mapping_values(mapping),
diff --git a/Objects/abstract.c b/Objects/abstract.c
index a69072138d3ea9..124b6906eeb0b5 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2148,15 +2148,23 @@ PyMapping_HasKey(PyObject *o, PyObject *key)
 }
 
 static PyObject *
-method_output_as_list(PyObject *o, const char *err_msg)
+method_output_as_list(PyObject *o, _Py_Identifier *meth_id)
 {
-    PyObject *it, *result;
+    PyObject *it, *result, *meth_output;
 
     assert(o != NULL);
-    it = PyObject_GetIter(o);
+    meth_output = _PyObject_CallMethodId(o, meth_id, NULL);
+    if (meth_output == NULL || PyList_CheckExact(meth_output)) {
+        return meth_output;
+    }
+    it = PyObject_GetIter(meth_output);
+    Py_DECREF(meth_output);
     if (it == NULL) {
         if (PyErr_ExceptionMatches(PyExc_TypeError)) {
-            PyErr_SetString(PyExc_TypeError, err_msg);
+            PyErr_Format(PyExc_TypeError,
+                         "%.200s.%s() is not iterable",
+                         Py_TYPE(o)->tp_name,
+                         meth_id->string);
         }
         return NULL;
     }
@@ -2168,55 +2176,34 @@ method_output_as_list(PyObject *o, const char *err_msg)
 PyObject *
 PyMapping_Keys(PyObject *o)
 {
-    PyObject *keys;
-    PyObject *keys_list;
     _Py_IDENTIFIER(keys);
 
-    if (PyDict_CheckExact(o))
+    if (PyDict_CheckExact(o)) {
         return PyDict_Keys(o);
-    keys = _PyObject_CallMethodId(o, &PyId_keys, NULL);
-    if (keys == NULL || PyList_CheckExact(keys)) {
-        return keys;
     }
-    keys_list = method_output_as_list(keys, "o.keys() is not iterable");
-    Py_DECREF(keys);
-    return keys_list;
+    return method_output_as_list(o, &PyId_keys);
 }
 
 PyObject *
 PyMapping_Items(PyObject *o)
 {
-    PyObject *items;
-    PyObject *items_list;
     _Py_IDENTIFIER(items);
 
-    if (PyDict_CheckExact(o))
+    if (PyDict_CheckExact(o)) {
         return PyDict_Items(o);
-    items = _PyObject_CallMethodId(o, &PyId_items, NULL);
-    if (items == NULL || PyList_CheckExact(items)) {
-        return items;
     }
-    items_list = method_output_as_list(items, "o.items() is not iterable");
-    Py_DECREF(items);
-    return items_list;
+    return method_output_as_list(o, &PyId_items);
 }
 
 PyObject *
 PyMapping_Values(PyObject *o)
 {
-    PyObject *values;
-    PyObject *values_list;
     _Py_IDENTIFIER(values);
 
-    if (PyDict_CheckExact(o))
+    if (PyDict_CheckExact(o)) {
         return PyDict_Values(o);
-    values = _PyObject_CallMethodId(o, &PyId_values, NULL);
-    if (values == NULL || PyList_CheckExact(values)) {
-        return values;
     }
-    values_list = method_output_as_list(values, "o.values() is not iterable");
-    Py_DECREF(values);
-    return values_list;
+    return method_output_as_list(o, &PyId_values);
 }
 
 /* isinstance(), issubclass() */

From 2d4bd68aa184cb21846d528437aa519c5daf46e2 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 17:57:30 +0300
Subject: [PATCH 03/12] fix the test

---
 Lib/test/test_capi.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 8a78bfb3b05d1f..09ee6f8a24089c 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -280,7 +280,7 @@ def values(self):
                 return list(super().values())
             def items(self):
                 return list(super().items())
-        class MyMapping1(dict):
+        class MyMapping2(dict):
             def keys(self):
                 return tuple(super().keys())
             def values(self):
@@ -290,7 +290,7 @@ def items(self):
         dict_obj = {'foo': 1, 'bar': 2, 'spam': 3}
         odict_obj = OrderedDict(dict_obj)
         mapping1 = MyMapping1(dict_obj)
-        mapping2 = MyMapping1(dict_obj)
+        mapping2 = MyMapping2(dict_obj)
 
         for mapping in [dict_obj, odict_obj, mapping1, mapping2]:
             self.assertListEqual(_testcapi.get_mapping_keys(mapping),

From f01f279b8e27c90d01a2256d10a46bb9b8fff7b3 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 18:04:30 +0300
Subject: [PATCH 04/12] improve err msg

---
 Objects/abstract.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Objects/abstract.c b/Objects/abstract.c
index 124b6906eeb0b5..c147cebd8c9502 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2162,7 +2162,7 @@ method_output_as_list(PyObject *o, _Py_Identifier *meth_id)
     if (it == NULL) {
         if (PyErr_ExceptionMatches(PyExc_TypeError)) {
             PyErr_Format(PyExc_TypeError,
-                         "%.200s.%s() is not iterable",
+                         "%.200s.%s() must return an iterable",
                          Py_TYPE(o)->tp_name,
                          meth_id->string);
         }

From 4b5852d9a890e9aca92144071c9d4db6b1c27eea Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 19:43:02 +0300
Subject: [PATCH 05/12] add a NEWS.d item

---
 Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100644 Misc/NEWS.d/next/C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst

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 00000000000000..6522e2a987616d
--- /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`. Patch by Oren Milman.

From 6682e53e872f373e1f31a9e6da9ccb768dcbc6a5 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 19:48:00 +0300
Subject: [PATCH 06/12] shorter names in 1st test

---
 Lib/test/test_capi.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 09ee6f8a24089c..5192e819739766 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -273,14 +273,14 @@ class C(): pass
         self.assertIn(b'MemoryError 3 30', out)
 
     def test_mapping_keys_values_items(self):
-        class MyMapping1(dict):
+        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 MyMapping2(dict):
+        class Mapping2(dict):
             def keys(self):
                 return tuple(super().keys())
             def values(self):
@@ -289,8 +289,8 @@ def items(self):
                 return tuple(super().items())
         dict_obj = {'foo': 1, 'bar': 2, 'spam': 3}
         odict_obj = OrderedDict(dict_obj)
-        mapping1 = MyMapping1(dict_obj)
-        mapping2 = MyMapping2(dict_obj)
+        mapping1 = Mapping1(dict_obj)
+        mapping2 = Mapping2(dict_obj)
 
         for mapping in [dict_obj, odict_obj, mapping1, mapping2]:
             self.assertListEqual(_testcapi.get_mapping_keys(mapping),

From 47ea6e7875531d4f71912c104500fc861d0791d7 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 20:39:42 +0300
Subject: [PATCH 07/12] add NULL checks

---
 Objects/abstract.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Objects/abstract.c b/Objects/abstract.c
index c147cebd8c9502..8fff14456d1eb0 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2178,6 +2178,9 @@ PyMapping_Keys(PyObject *o)
 {
     _Py_IDENTIFIER(keys);
 
+    if (o == NULL) {
+        return null_error();
+    }
     if (PyDict_CheckExact(o)) {
         return PyDict_Keys(o);
     }
@@ -2189,6 +2192,9 @@ PyMapping_Items(PyObject *o)
 {
     _Py_IDENTIFIER(items);
 
+    if (o == NULL) {
+        return null_error();
+    }
     if (PyDict_CheckExact(o)) {
         return PyDict_Items(o);
     }
@@ -2200,6 +2206,9 @@ PyMapping_Values(PyObject *o)
 {
     _Py_IDENTIFIER(values);
 
+    if (o == NULL) {
+        return null_error();
+    }
     if (PyDict_CheckExact(o)) {
         return PyDict_Values(o);
     }

From 3e81e35a3e5864ff876587ad7d74280404d77d74 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 23:36:41 +0300
Subject: [PATCH 08/12] add tests of empty mappings

---
 Lib/test/test_capi.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index 5192e819739766..ee66a653b99fa5 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -288,11 +288,10 @@ def values(self):
             def items(self):
                 return tuple(super().items())
         dict_obj = {'foo': 1, 'bar': 2, 'spam': 3}
-        odict_obj = OrderedDict(dict_obj)
-        mapping1 = Mapping1(dict_obj)
-        mapping2 = Mapping2(dict_obj)
 
-        for mapping in [dict_obj, odict_obj, mapping1, mapping2]:
+        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),

From 0421b5cbbfa1a8fb8cbd6666683a2c4d7473e696 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sat, 30 Sep 2017 23:38:54 +0300
Subject: [PATCH 09/12] remove redundant empty dict arg from the first test

---
 Lib/test/test_capi.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index ee66a653b99fa5..921735ef5d016b 100644
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -289,7 +289,7 @@ def items(self):
                 return tuple(super().items())
         dict_obj = {'foo': 1, 'bar': 2, 'spam': 3}
 
-        for mapping in [{}, OrderedDict({}), Mapping1({}), Mapping2({}),
+        for mapping in [{}, OrderedDict(), Mapping1(), Mapping2(),
                         dict_obj, OrderedDict(dict_obj),
                         Mapping1(dict_obj), Mapping2(dict_obj)]:
             self.assertListEqual(_testcapi.get_mapping_keys(mapping),

From 587a9ba047f24f808612bfe04c3394fb160be16d Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sun, 1 Oct 2017 12:57:33 +0300
Subject: [PATCH 10/12] added a section in whatsnew, improved the NEWS.d item,
 added versionchanged directives, and improved the error message.

---
 Doc/c-api/mapping.rst                                 |  9 +++++++++
 Doc/whatsnew/3.7.rst                                  |  4 ++++
 .../C API/2017-09-30-19-41-44.bpo-28280.K_EjpO.rst    |  2 +-
 Objects/abstract.c                                    | 11 ++++++++---
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Doc/c-api/mapping.rst b/Doc/c-api/mapping.rst
index e8765888c6ab5d..308a9761f87ea9 100644
--- a/Doc/c-api/mapping.rst
+++ b/Doc/c-api/mapping.rst
@@ -53,18 +53,27 @@ Mapping Protocol
    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 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 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 a19a2895828116..7bfa13a13f49f0 100644
--- a/Doc/whatsnew/3.7.rst
+++ b/Doc/whatsnew/3.7.rst
@@ -336,6 +336,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 Serhiy Storchaka and 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/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
index 6522e2a987616d..76990f79cb0a3a 100644
--- 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	
@@ -1,2 +1,2 @@
 Make `PyMapping_Keys()`, `PyMapping_Values()` and `PyMapping_Items()` always
-return a `list`. Patch by Oren Milman.
+return a `list` (rather than a `list` or a `tuple`). Patch by Oren Milman.
diff --git a/Objects/abstract.c b/Objects/abstract.c
index 8fff14456d1eb0..7df0328d1e3e9b 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2147,6 +2147,9 @@ 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)
 {
@@ -2158,16 +2161,18 @@ method_output_as_list(PyObject *o, _Py_Identifier *meth_id)
         return meth_output;
     }
     it = PyObject_GetIter(meth_output);
-    Py_DECREF(meth_output);
     if (it == NULL) {
         if (PyErr_ExceptionMatches(PyExc_TypeError)) {
             PyErr_Format(PyExc_TypeError,
-                         "%.200s.%s() must return an iterable",
+                         "%.200s.%s() must return an iterable, not %.200s",
                          Py_TYPE(o)->tp_name,
-                         meth_id->string);
+                         meth_id->string,
+                         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;

From 5dc52c11a5d7a66005704c19ef62a6bf552f27e5 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sun, 1 Oct 2017 13:01:41 +0300
Subject: [PATCH 11/12] improve the error message

---
 Objects/abstract.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Objects/abstract.c b/Objects/abstract.c
index 7df0328d1e3e9b..08882fae016c70 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2164,7 +2164,7 @@ method_output_as_list(PyObject *o, _Py_Identifier *meth_id)
     if (it == NULL) {
         if (PyErr_ExceptionMatches(PyExc_TypeError)) {
             PyErr_Format(PyExc_TypeError,
-                         "%.200s.%s() must return an iterable, not %.200s",
+                         "%.200s.%s() returned a non-iterable (type %.200s)",
                          Py_TYPE(o)->tp_name,
                          meth_id->string,
                          Py_TYPE(meth_output)->tp_name);

From de2c44038ee715dbe1eed69a00407db6d8bb6237 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Sun, 1 Oct 2017 19:55:18 +0300
Subject: [PATCH 12/12] fix whatsnew entry, and use %U and meth_id->object

---
 Doc/whatsnew/3.7.rst | 2 +-
 Objects/abstract.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst
index 7bfa13a13f49f0..5c335669bda314 100644
--- a/Doc/whatsnew/3.7.rst
+++ b/Doc/whatsnew/3.7.rst
@@ -338,7 +338,7 @@ Build and C API Changes
 
 * 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 Serhiy Storchaka and Oren Milman in :issue:`28280`.)
+  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/Objects/abstract.c b/Objects/abstract.c
index 08882fae016c70..3cb7a32b01ee5a 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2164,9 +2164,9 @@ method_output_as_list(PyObject *o, _Py_Identifier *meth_id)
     if (it == NULL) {
         if (PyErr_ExceptionMatches(PyExc_TypeError)) {
             PyErr_Format(PyExc_TypeError,
-                         "%.200s.%s() returned a non-iterable (type %.200s)",
+                         "%.200s.%U() returned a non-iterable (type %.200s)",
                          Py_TYPE(o)->tp_name,
-                         meth_id->string,
+                         meth_id->object,
                          Py_TYPE(meth_output)->tp_name);
         }
         Py_DECREF(meth_output);