Avoid suppressing all exceptions in PyObject_HasAttr()

[serhiy-storchaka]

BPO	31572
Nosy	@pitrou, @vstinner, @serhiy-storchaka
PRs	bpo-32788: Better error handling in sqlite3. #3723 bpo-31572: Get rid of using _PyObject_HasAttrId() in itertools. #3724 bpo-31572: Get rid of _PyObject_HasAttrId() in the ASDL parser. #3725 bpo-31572: Get rid of PyObject_HasAttr() and _PyObject_HasAttrId() in io. #3726 bpo-32787: Better error handling in ctypes. #3727 bpo-31572: Get rid of _PyObject_HasAttrId() in dict and OrderedDict. #3728 bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. #3729 bpo-31572: Get rid of _PyObject_HasAttrId() and PyDict_GetItemString() in warnings. #3731 [3.6] bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (GH-3729). #4081
Dependencies	bpo-32787: Better error handling in ctypes bpo-32788: Better error handling in sqlite3

^{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 = None
created_at = <Date 2017-09-24.17:46:45.898>
labels = ['extension-modules', 'interpreter-core', 'type-bug']
title = 'Avoid suppressing all exceptions in PyObject_HasAttr()'
updated_at = <Date 2018-12-05.14:44:23.566>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2018-12-05.14:44:23.566>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2017-09-24.17:46:45.898>
creator = 'serhiy.storchaka'
dependencies = ['32787', '32788']
files = []
hgrepos = []
issue_num = 31572
keywords = ['patch']
message_count = 12.0
messages = ['302873', '304759', '304764', '304787', '304795', '306084', '306085', '306086', '306087', '310091', '316580', '331121']
nosy_count = 3.0
nosy_names = ['pitrou', 'vstinner', 'serhiy.storchaka']
pr_nums = ['3723', '3724', '3725', '3726', '3727', '3728', '3729', '3731', '4081']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31572'
versions = []

[serhiy-storchaka]

Initially hasattr() suppressed all raised exceptions. In bpo-2196 hasattr() was changed to suppress only Exception exceptions and propagate exceptions like SystemExit and KeyboardInterrupt. In bpo-9666 hasattr() was changed to suppress only AttributeError.

But C API functions, PyObject_HasAttr() and like, were not changed. PyObject_HasAttr() is documented as an equivalent of hasattr(), but there is undocumented difference. The C code that uses PyObject_HasAttr() starves from the same problem as the Python code that used old hasattr().

The only solution of this problem is getting rid of PyObject_HasAttr() if favor of PyObject_GetAttr(). In this issue I'm going to propose a set of PRs that replace PyObject_HasAttr() invocations in different components with more correct code.

[serhiy-storchaka]

New changeset 04e36af by Serhiy Storchaka in branch 'master':
bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (bpo-3729)
04e36af

[pitrou]

The only solution of this problem is getting rid of PyObject_HasAttr() if favor of PyObject_GetAttr().

I don't understand why. Why not fix PyObject_HasAttr() like hasattr() was?

[serhiy-storchaka]

hasattr() can return True, False, or raise an exception. But PyObject_HasAttr() just returns an integer: 0 for False, not 0 for True. There is no way to return an error, and existing code doesn't expect that PyObject_HasAttr() returns an error. Leaking an exception from PyObject_HasAttr() will break existing code.

[serhiy-storchaka]

New changeset 77af0a3 by Serhiy Storchaka in branch '3.6':
[3.6] bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (GH-3729). (bpo-4081)
77af0a3

[serhiy-storchaka]

New changeset d4f8480 by Serhiy Storchaka in branch 'master':
bpo-31572: Don't silence unexpected errors in the _warnings module. (bpo-3731)
d4f8480

[serhiy-storchaka]

New changeset 1707e40 by Serhiy Storchaka in branch 'master':
bpo-31572: Silence only AttributeError when get the __copy__ attribute in itertools.tee(). (bpo-3724)
1707e40

[serhiy-storchaka]

New changeset 60c3d35 by Serhiy Storchaka in branch 'master':
bpo-31572: Get rid of _PyObject_HasAttrId() in dict and OrderedDict. (bpo-3728)
60c3d35

[serhiy-storchaka]

New changeset bba2239 by Serhiy Storchaka in branch 'master':
bpo-31572: Get rid of _PyObject_HasAttrId() in the ASDL parser. (bpo-3725)
bba2239

[serhiy-storchaka]

New changeset 4d9aec0 by Serhiy Storchaka in branch 'master':
bpo-31572: Get rid of PyObject_HasAttr() and _PyObject_HasAttrId() in the _io module. (bpo-3726)
4d9aec0

[vstinner]

It seems like this issue caused a regression in _warnings: please see bpo-33509.

[serhiy-storchaka]

New changeset 398bd27 by Serhiy Storchaka in branch 'master':
bpo-32787: Better error handling in ctypes. (bpo-3727)
398bd27

[vstinner]

I suppose that the next step is to design a migration path to avoid legacy API which suppress exceptions :-) It was discussed at: capi-workgroup/problems#54

Maybe we can start by soft deprecating old APIs.