Use PyDict_GetItemWithError() instead of PyDict_GetItem()

[serhiy-storchaka]

BPO	35459
Nosy	@rhettinger, @vstinner, @methane, @ericsnowcurrently, @serhiy-storchaka, @MojoVampire, @pablogsal, @rmonat
PRs	bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem() #11112 Add a What's New entry for bpo-35459. #12706 bpo-42093: Cleanup _PyDict_GetItemHint() #24582
Files	pydict-getitem-compare.txt

^{Note: 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:

assignee = None
closed_at = <Date 2019-04-10.07:31:08.468>
created_at = <Date 2018-12-11.10:54:17.556>
labels = ['extension-modules', 'interpreter-core', '3.8']
title = 'Use PyDict_GetItemWithError() instead of PyDict_GetItem()'
updated_at = <Date 2021-02-21.11:07:21.269>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-02-21.11:07:21.269>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-04-10.07:31:08.468>
closer = 'serhiy.storchaka'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2018-12-11.10:54:17.556>
creator = 'serhiy.storchaka'
dependencies = []
files = ['48137']
hgrepos = []
issue_num = 35459
keywords = ['patch']
message_count = 17.0
messages = ['331604', '331605', '331621', '331744', '335541', '335651', '336536', '336563', '336647', '336686', '339490', '339616', '339830', '355667', '359480', '359489', '359490']
nosy_count = 8.0
nosy_names = ['rhettinger', 'vstinner', 'methane', 'eric.snow', 'serhiy.storchaka', 'josh.r', 'pablogsal', 'raphaelm']
pr_nums = ['11112', '12706', '24582']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue35459'
versions = ['Python 3.8']

[serhiy-storchaka]

There is an issue with using PyDict_GetItem(). Since it silences all exceptions, it can return incorrect result when an exception like MemoryError or KeyboardInterrupt was raised in the user __hash__() and __eq__(). In addition PyDict_GetItemString() and _PyDict_GetItemId() swallow a MemoryError raised when fail to allocate a temporary string object.

In addition, PyDict_GetItemWithError() is a tiny bit faster than PyDict_GetItem(), because it avoids checking the exception state in successful case.

The proposed PR replaces most calls of PyDict_GetItem(), PyDict_GetItemString() and _PyDict_GetItemId() with calls of PyDict_GetItemWithError(), _PyDict_GetItemStringWithError() and _PyDict_GetItemIdWithError().

[vstinner]

My previous attempt: bpo-20615.

[serhiy-storchaka]

Opened bpo-35461 for documenting flaws of PyDict_GetItem().

[serhiy-storchaka]

Most of changes are straightforward. Just replaced PyDict_GetItem*() with PyDict_GetItem*WithError() and added the check for PyErr_Occurred(). PyDict_GetItemString() with constant argument was replaced with _PyDict_GetItemIdWithError() for performance.

Some code was left unchanged. This was mostly in files where errors are very and error checking is not performed or errors are silenced in any case (Python/compile.c, Python/symtable.c, Objects/structseq.c, etc). These cases needed separate issues.

The most non-trivial change is in Objects/typeobject.c. The check for duplicated descriptors (in add_methods(), add_members() and add_getset()) was moved after creating the descriptor object. This improves performance by avoiding to create a temporary string objects. Duplicate descriptor names is a very uncommon case -- there were only two cases in the stdlib (besides tests), and one of them already is fixed (PR 11053).

[serhiy-storchaka]

Eric requested to run the benchmark suite. Here are results. I do not know how to interpret them. Likely all differences are random.

[ericsnowcurrently]

Thanks, Serhiy. While the benchmark suite is our best tool available for measuring performance, I'm not sure what slowdown is significant in those results.

@victor, any thoughts?

[serhiy-storchaka]

New changeset a24107b by Serhiy Storchaka in branch 'master':
bpo-35459: Use PyDict_GetItemWithError() instead of PyDict_GetItem(). (GH-11112)
a24107b

[vstinner]

It seems like the change introduced a regression: bpo-36110.

[vstinner]

It seems like the change introduced a regression: bpo-36110.

While the test has been fixed, IMHO we need to better document this subtle behavior change since it can be surprising.

The test failed because PyObject_GetAttr() no longer ignores exceptions when getting the attribute from the type dictionary. It's a significant change compared to Python 3.7.

Right now, the change has not even a NEWS entry, whereas it's a backward incompatible change!

Don't get my wrong: the change is correct, we don't want to ignore arbitrary exceptions, it's just a matter of documentation.