-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SystemError or crash in sorted(iterable=[]) #73513
Comments
In Python 3.6: >>> sorted(iterable=[])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
SystemError: Objects/tupleobject.c:81: bad argument to internal function In Python 3.5 and 2.7: >>> sorted(iterable=[])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'iterable' is an invalid keyword argument for this function |
Seems this regression was introduced in bpo-27809. |
In debug build Python is crashed. >>> sorted(iterable=[])
python: Objects/abstract.c:2317: _PyObject_FastCallDict: Assertion `nargs >= 0' failed.
Aborted (core dumped) |
Proposed patch fixes the bug. |
Please be careful with all of these AC changes. They need to have tests. They need to not change APIs. They need to not introduce bugs. The whole effort seems to be being pushed forward in a cavalier and aggressive manner. In this case, there was an API change and bugs introduced for basically zero benefit. |
Oh, this issue is very subtle. Since the list.sorted() class method became a builtin sorted() method (Python 2.4.0, change c06b570adf12 by Raymond Hettinger), the sorted() function accepts an iterable as a keyword argument, whereas list.sort() doesn't. sorted(iterable=[]) fails on calling internally list.sort(iterable=[]), not when sorted() parses its arguments. The change 6219aa966a5f (issue bpo-20184) converted sorted() to Argument Clinic. This change was released in Python 3.5.3 and 3.6.0... but it didn't introduce the bug. It's not the fault of Argument Clinic! The change b34d2ef5c412 (issue bpo-27809) replacing METH_VARARGS|METH_KEYWORDS with METH_FASTCALL didn't introduce the bug neither. It's the change 15eab21bf934 (issue bpo-27809) which replaced PyEval_CallObjectWithKeywords() with _PyObject_FastCallDict(). This change also replaces PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1) which introduced the bug. I didn't notice that args can be an empty tuple. I never tried to call sorted(iterable=seq), I didn't know the name of the first parameter :-) I also replaced PyTuple_GetSlice(args, 1, argc) with &PyTuple_GET_ITEM(args, 1) in methoddescr_call(), but this function make sure that we have at least one positional argument and so doesn't have this bug. Moreover, this method doesn't use Argument Clinic (yet? ;-)). I'm quite sure that I didn't replace PyTuple_GetSlice() in other functions. |
While Python 3.5 doesn't crash, I consider that it has also the bug. So I added Python 3.5 in Versions. sorted.diff LGTM. And thank you for the unit test! |
Raymond: "Please be careful with all of these AC changes. They need to have tests. They need to not change APIs. They need to not introduce bugs." The bug was not introduced by an Argument Clinic change. I agree that switching to AC must not change the API. I didn't look much at the "recent" AC changes (it started somethings like 2 years ago, no?), but it seems like the most common trap are default values of optional positional arguments. Sometimes, there are inconsistencies between .rst doc (in Doc/ directory), the docstring and the C implementation. The trap is when the default is documented as None, the C code uses NULL and passing None behaves differently... Raymond: "The whole effort seems to be being pushed forward in a cavalier and aggressive manner. In this case, there was an API change and bugs introduced for basically zero benefit." I converted a few functions to AC. I used regular reviews for that, since I noticed that it's easy to make mistakes. My hope is that the AC conversion will also help to fix inconsistencies. The benefit of switching to AC is a better docstring and a correct signature for inspect.signature(func). Python 3.5: sorted(...)
sorted(iterable, key=None, reverse=False) --> new sorted list versus Python 3.7 sorted(iterable, key=None, reverse=False)
Return a new list containing all items from the iterable in ascending order.
A custom key function can be supplied to customize the sort order, and the
reverse flag can be set to request the result in descending order. IHMO Python 3.7 docstring looks better. (Sadly, sorted() doesn't use AC yet, the "AC code" was genereted manually. AC should support **kwargs, issue bpo-20291.) I guess "cavalier" and "aggressive" is more related to my FASTCALL work. I'm waiting for reviews for major API changes. I agree that I pushed a lot of code without reviews when I considered that the change was safe and simple. It became hard to get a review, there are too few reviewers, especially on the C code. The benefit of FASTCALL are better performances. I'm trying to provide benchmarks whenever possible, but since I modified dozens of functions, sometimes I didn't publish all benchmark results (sometimes even to not spam an issue). Microbenchmark on sorted() on Python 3.7 compared to 3.5 (before FASTCALL): haypo@smithers$ ./python -m perf timeit 'seq=list(range(10)); k=lambda x:x' 'sorted(seq, key=k)' --compare-to=../3.5/python -v It's not easy nor interesting to measure the speedup of the changes limited to sorted(). Sadly, FASTCALL requires to modify a lot of code to show its full power. Otherwise, the gain is much smaller. The latest major "API change" was made by you 14 years ago. Yes, I introduced a bug, and I'm sorry about that. Shit happens. Let's be more constructive: to avoid bugs, the best is to get reviews. I have dozens of patches waiting for your review, so please come to help me to spot bugs before releases ;-) |
A few random thoughts that may or may not be helpful:
|
The patch uses new feature of 3.6 -- supporting positional-only parameters in Actually I think that argument parsing code of sorted() and list.sort() can be |
Serhiy, this patch passes tests and looks fine. I think you can go ahead with this patch. |
Right now I'm building 3.6 with applied patch before final testing and |
The title is spoiled when reply by email. I suppose [] has special meaning in email subject. |
New changeset 1827b64cfce8 by Serhiy Storchaka in branch '3.6': New changeset f44f44b14dfc by Serhiy Storchaka in branch 'default': |
"The patch uses new feature of 3.6 -- supporting positional-only Oh, I didn't recall that it's a new feature. It's a nice feature by In that case, yeah it's ok to leave Python 3.5 unchanged. I proposed |
Raymond Hettinger: "A few random thoughts that may or may not be helpful: (...)" I replied on the python-committers mailing list: I'm not sure that this specific issue is the best place to discuss, I propose to continue the discussion there. |
Thank you for your reviews Victor and Raymond. As for more general Raymond comments, I agreed with many of them in principle, but this isn't directly related to this issue. Victor opened a topic on the python-committers mailing list: https://mail.python.org/pipermail/python-committers/2017-January/004129.html |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: