bpo-28280: Make PyMapping_Keys(), PyMapping_Values() and PyMapping_Items() always return a list by orenmn · Pull Request #3840 · python/cpython

orenmn · 2017-09-30T21:19:47Z

In addition:

Add get_mapping_keys(), get_mapping_values() and get_mapping_items() to _testcapi, so that we would be able to test PyMapping_Keys(), PyMapping_Values() and PyMapping_Items().
Add tests to test_capi to verify that each of these functions returns the appropriate list, and that it raises the appropriate error in case of a bad argument.
Update the docs accordingly.

https://bugs.python.org/issue28280

serhiy-storchaka

Add also an entry in What's New. This will help people to revise their code and fix errors in old versions of Python.

serhiy-storchaka · 2017-10-01T07:00:39Z

-   return *NULL*.
+   On success, return a list of the keys in object *o*.  On failure, return
+   *NULL*.



Add a "versionchanged" directive.

This is needed for users that write a code that can be used with older Python.

serhiy-storchaka · 2017-10-01T07:01:41Z

        self.assertIn(b'MemoryError 3 30', out)

+    def test_mapping_keys_values_items(self):
+        class Mapping1(dict):


Is there a need to test this class?

Its purpose is to test cases in which the methods already return a list, i.e. when PyList_CheckExact(meth_output) is true.

serhiy-storchaka · 2017-10-01T07:13:02Z

@@ -0,0 +1,2 @@
+Make `PyMapping_Keys()`, `PyMapping_Values()` and `PyMapping_Items()` always


Mention that it was possible to return a tuple.

serhiy-storchaka · 2017-10-01T07:18:31Z

+            PyErr_Format(PyExc_TypeError,
+                         "%.200s.%s() must return an iterable",
+                         Py_TYPE(o)->tp_name,
+                         meth_id->string);


Use meth_id->string and the %U format. The result of formatting is a PyUnicode object.

I don't think i understand your comment..
meth_id->string is of type const char*, so when i use %U, it causes a crash.

There is a redundant call of PyUnicode_DecodeUTF8Stateful() inside of PyUnicode_FromFormatV called from PyErr_Format(). It can be avoided by passing a PyUnicode object (meth_id->string) and using the %U format.

IIUC, you suggest something like:

PyErr_Format(PyExc_TypeError, "%.200s.%U() returned a non-iterable (type %.200s)", Py_TYPE(o)->tp_name, PyUnicode_FromString(meth_id->string), Py_TYPE(meth_output)->tp_name);

But then i would also have to handle refcounts, and the case in which PyUnicode_FromString() failed. Do you think it's worth it?

No, I suggest to use meth_id->object which is a cached PyUnicode_FromString(meth_id->string).

Ah, I just have noticed typos in my previous comments! Of course I meant meth_id->object rather of meth_id->string!

serhiy-storchaka · 2017-10-01T07:19:37Z

+    if (it == NULL) {
+        if (PyErr_ExceptionMatches(PyExc_TypeError)) {
+            PyErr_Format(PyExc_TypeError,
+                         "%.200s.%s() must return an iterable",


Add an actual type of meth_output.

serhiy-storchaka · 2017-10-01T07:21:17Z

 }

+static PyObject *
+method_output_as_list(PyObject *o, _Py_Identifier *meth_id)


Add a comment that this is mainly a copy of PySequence_Fast.

…hanged directives, and improved the error message.

serhiy-storchaka · 2017-10-01T12:12:36Z


+* 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`.)


No need to mention my name. I'm just a reviewer.

You are the author of the issue in bpo, but as you wish.

The one who opened an issue is not the one who fixed it.

serhiy-storchaka

I'm approving this PR, but since I have proposed this change I can miss flaws of it. I want to get the approve from other core developers.

vstinner · 2017-10-02T09:25:57Z

+    if (meth_output == NULL || PyList_CheckExact(meth_output)) {
+        return meth_output;
+    }
+    it = PyObject_GetIter(meth_output);


Why not just calling list(obj)?

PySequence_List() few lines below is just a C API equivalent of calling list(). But first we want to produce better error message if keys() returns non-iterable. It is in the current code, and removing it will simplify the code, but introduce a regression.

I would prefer to pass meth_output to PySequence_List() rather than iter(meth_output). list_extend() has a fast-path for tuple.

This doesn't matter. keys() returning tuple should be very rare. I don't know any example.

Calling PyObject_GetIter() on an iterator is cheap, it should return the iterator itself. But calling PyObject_GetIter() on a general iterable creates a new iterator object. This is not so cheap. This will add an overhead (and possible side effects) in most common cases.

list(iter(obj)) is not exactly the same than list(obj). My point on performance was the cost of PySequence_List(iterator) vs PySequence_List(tuple).

We could add a special case for tuples:

if (PyTuple_CheckExact(meth_output)) { result = PySequence_List(meth_output); Py_DECREF(meth_output); return result; }

But I think this is a premature optimization. Do you know any class that return keys() as a tuple?

Nah, it's fine. I don't care much. The current code is ok.

orenmn added 9 commits September 30, 2017 17:32

init commit

bd59df1

minimize code duplication

15c1486

fix the test

2d4bd68

improve err msg

f01f279

add a NEWS.d item

4b5852d

shorter names in 1st test

6682e53

add NULL checks

47ea6e7

add tests of empty mappings

3e81e35

remove redundant empty dict arg from the first test

0421b5c

bedevere-bot added the awaiting review label Sep 30, 2017

the-knights-who-say-ni added the CLA signed label Sep 30, 2017

serhiy-storchaka reviewed Oct 1, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Oct 1, 2017

orenmn added 2 commits October 1, 2017 12:57

added a section in whatsnew, improved the NEWS.d item, added versionc…

587a9ba

…hanged directives, and improved the error message.

improve the error message

5dc52c1

serhiy-storchaka reviewed Oct 1, 2017

View reviewed changes

fix whatsnew entry, and use %U and meth_id->object

de2c440

serhiy-storchaka approved these changes Oct 1, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 1, 2017

serhiy-storchaka requested review from rhettinger, skrah and vstinner October 1, 2017 20:53

vstinner reviewed Oct 2, 2017

View reviewed changes

Merge branch 'master' into bpo28280

d492b91

serhiy-storchaka merged commit 0ccc0f6 into python:master Oct 8, 2017

bedevere-bot removed the awaiting merge label Oct 8, 2017

		@@ -0,0 +1,2 @@
		Make `PyMapping_Keys()`, `PyMapping_Values()` and `PyMapping_Items()` always

Uh oh!

Conversation

orenmn commented Sep 30, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenmn Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

orenmn commented Sep 30, 2017 •

edited by bedevere-bot

Loading

orenmn Oct 1, 2017 •

edited

Loading

serhiy-storchaka Oct 1, 2017 •

edited

Loading

serhiy-storchaka Oct 1, 2017 •

edited

Loading