Skip to content
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.

Sign up for GitHub

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

bpo-32571: Avoid raising unneeded AttributeError and silencing it in C code (alt). #5222

Merged
merged 2 commits into from
Jan 25, 2018
Merged

bpo-32571: Avoid raising unneeded AttributeError and silencing it in C code (alt). #5222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0d3e42
bpo-32571: Avoid raising unneeded AttributeError and silencing it C c…
serhiy-storchaka Jan 16, 2018
c96d0e2
Use different an alternate interface: _PyObject_LookupAttr() and _PyO…
serhiy-storchaka Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions Include/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,20 @@ PyAPI_FUNC(int) PyObject_SetAttr(PyObject *, PyObject *, PyObject *);
PyAPI_FUNC(int) PyObject_HasAttr(PyObject *, PyObject *);
#ifndef Py_LIMITED_API
PyAPI_FUNC(int) _PyObject_IsAbstract(PyObject *);
/* Same as PyObject_GetAttr(), but don't raise AttributeError. */
PyAPI_FUNC(PyObject *) _PyObject_GetAttrWithoutError(PyObject *, PyObject *);
PyAPI_FUNC(PyObject *) _PyObject_GetAttrId(PyObject *, struct _Py_Identifier *);
PyAPI_FUNC(int) _PyObject_SetAttrId(PyObject *, struct _Py_Identifier *, PyObject *);
PyAPI_FUNC(int) _PyObject_HasAttrId(PyObject *, struct _Py_Identifier *);
/* Replacements of PyObject_GetAttr() and _PyObject_GetAttrId() which
don't raise AttributeError.

Return 1 and set *result != NULL if an attribute is found.
Return 0 and set *result == NULL if an attribute is not found;
an AttributeError is silenced.
Return -1 and set *result == NULL if an error other than AttributeError
is raised.
*/
PyAPI_FUNC(int) _PyObject_LookupAttr(PyObject *, PyObject *, PyObject **);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename the method to PyObject_LookupAttr (no leading underscore). I'd use it in my libraries and the leading underscore would make me think that I shouldn't.

I also like Inada-san's suggestion to simplify the return value even further:

  • -1, if an unrelated error occurred during attribute lookup
  • 0 if no errors have occurred; *result will be set to NULL if the attribute was not found, and to a non-NULL value if it was found.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this API is experimental and should be kept private for some time. We are still discussing it, and after making some design decision we may change it in future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @serhiy-storchaka.
While I feel this API is nice, I'm not sure we should set
*result=NULL when returning -1 or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we care C API methods with a leading underscore the same as without? I've never seen us breaking the _ prefixed API to be honest. So leading underscore or not it's set in stone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1st1 Actually speaking, I broke _PyObject_GenericGetAttrWithDict(). I added one argument to suppress AttributeError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. The PR is LGTM.

PyAPI_FUNC(int) _PyObject_LookupAttrId(PyObject *, struct _Py_Identifier *, PyObject **);
PyAPI_FUNC(PyObject **) _PyObject_GetDictPtr(PyObject *);
#endif
PyAPI_FUNC(PyObject *) PyObject_SelfIter(PyObject *);
Expand Down
8 changes: 3 additions & 5 deletions Modules/_collectionsmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1339,12 +1339,10 @@ deque_reduce(dequeobject *deque)
PyObject *dict, *it;
_Py_IDENTIFIER(__dict__);

dict = _PyObject_GetAttrId((PyObject *)deque, &PyId___dict__);
if (_PyObject_LookupAttrId((PyObject *)deque, &PyId___dict__, &dict) < 0) {
return NULL;
}
if (dict == NULL) {
if (!PyErr_ExceptionMatches(PyExc_AttributeError)) {
return NULL;
}
PyErr_Clear();
dict = Py_None;
Py_INCREF(dict);
}
Expand Down
7 changes: 3 additions & 4 deletions Modules/_io/bufferedio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,9 @@ _bufferedreader_read_all(buffered *self)
}
_bufferedreader_reset_buf(self);

readall = _PyObject_GetAttrWithoutError(self->raw, _PyIO_str_readall);
if (_PyObject_LookupAttr(self->raw, _PyIO_str_readall, &readall) < 0) {
goto cleanup;
}
if (readall) {
tmp = _PyObject_CallNoArg(readall);
Py_DECREF(readall);
Expand All @@ -1561,9 +1563,6 @@ _bufferedreader_read_all(buffered *self)
}
goto cleanup;
}
else if (PyErr_Occurred()) {
goto cleanup;
}

chunks = PyList_New(0);
if (chunks == NULL)
Expand Down
8 changes: 3 additions & 5 deletions Modules/_io/fileio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,12 +1073,10 @@ fileio_repr(fileio *self)
if (self->fd < 0)
return PyUnicode_FromFormat("<_io.FileIO [closed]>");

nameobj = _PyObject_GetAttrId((PyObject *) self, &PyId_name);
if (_PyObject_LookupAttrId((PyObject *) self, &PyId_name, &nameobj) < 0) {
return NULL;
}
if (nameobj == NULL) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
PyErr_Clear();
else
return NULL;
res = PyUnicode_FromFormat(
"<_io.FileIO fd=%d mode='%s' closefd=%s>",
self->fd, mode_string(self), self->closefd ? "True" : "False");
Expand Down
38 changes: 13 additions & 25 deletions Modules/_io/iobase.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,18 +133,12 @@ static int
iobase_is_closed(PyObject *self)
{
PyObject *res;
int ret;
/* This gets the derived attribute, which is *not* __IOBase_closed
in most cases! */
res = _PyObject_GetAttrId(self, &PyId___IOBase_closed);
if (res == NULL) {
if (!PyErr_ExceptionMatches(PyExc_AttributeError)) {
return -1;
}
PyErr_Clear();
return 0;
}
Py_DECREF(res);
return 1;
ret = _PyObject_LookupAttrId(self, &PyId___IOBase_closed, &res);
Py_XDECREF(res);
return ret;
}

/* Flush and close methods */
Expand Down Expand Up @@ -190,20 +184,16 @@ iobase_check_closed(PyObject *self)
int closed;
/* This gets the derived attribute, which is *not* __IOBase_closed
in most cases! */
res = _PyObject_GetAttrWithoutError(self, _PyIO_str_closed);
if (res == NULL) {
if (PyErr_Occurred()) {
closed = _PyObject_LookupAttr(self, _PyIO_str_closed, &res);
if (closed > 0) {
closed = PyObject_IsTrue(res);
Py_DECREF(res);
if (closed > 0) {
PyErr_SetString(PyExc_ValueError, "I/O operation on closed file.");
return -1;
}
return 0;
}
closed = PyObject_IsTrue(res);
Py_DECREF(res);
if (closed <= 0) {
return closed;
}
PyErr_SetString(PyExc_ValueError, "I/O operation on closed file.");
return -1;
return closed;
}

PyObject *
Expand Down Expand Up @@ -273,8 +263,7 @@ iobase_finalize(PyObject *self)

/* If `closed` doesn't exist or can't be evaluated as bool, then the
object is probably in an unusable state, so ignore. */
res = _PyObject_GetAttrWithoutError(self, _PyIO_str_closed);
if (res == NULL) {
if (_PyObject_LookupAttr(self, _PyIO_str_closed, &res) <= 0) {
PyErr_Clear();
closed = -1;
}
Expand Down Expand Up @@ -538,8 +527,7 @@ _io__IOBase_readline_impl(PyObject *self, Py_ssize_t limit)
PyObject *peek, *buffer, *result;
Py_ssize_t old_size = -1;

peek = _PyObject_GetAttrWithoutError(self, _PyIO_str_peek);
if (peek == NULL && PyErr_Occurred()) {
if (_PyObject_LookupAttr(self, _PyIO_str_peek, &peek) < 0) {
return NULL;
}

Expand Down
48 changes: 18 additions & 30 deletions Modules/_io/textio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,14 +924,10 @@ _textiowrapper_set_encoder(textio *self, PyObject *codec_info,
return -1;

/* Get the normalized named of the codec */
res = _PyObject_GetAttrId(codec_info, &PyId_name);
if (res == NULL) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
PyErr_Clear();
else
return -1;
if (_PyObject_LookupAttrId(codec_info, &PyId_name, &res) < 0) {
return -1;
}
else if (PyUnicode_Check(res)) {
if (res != NULL && PyUnicode_Check(res)) {
const encodefuncentry *e = encodefuncs;
while (e->name != NULL) {
if (_PyUnicode_EqualToASCIIString(res, e->name)) {
Expand Down Expand Up @@ -1177,19 +1173,17 @@ _io_TextIOWrapper___init___impl(textio *self, PyObject *buffer,

if (Py_TYPE(buffer) == &PyBufferedReader_Type ||
Py_TYPE(buffer) == &PyBufferedWriter_Type ||
Py_TYPE(buffer) == &PyBufferedRandom_Type) {
raw = _PyObject_GetAttrId(buffer, &PyId_raw);
Py_TYPE(buffer) == &PyBufferedRandom_Type)
{
if (_PyObject_LookupAttrId(buffer, &PyId_raw, &raw) < 0)
goto error;
/* Cache the raw FileIO object to speed up 'closed' checks */
if (raw == NULL) {
if (PyErr_ExceptionMatches(PyExc_AttributeError))
PyErr_Clear();
if (raw != NULL) {
if (Py_TYPE(raw) == &PyFileIO_Type)
self->raw = raw;
else
goto error;
Py_DECREF(raw);
}
else if (Py_TYPE(raw) == &PyFileIO_Type)
self->raw = raw;
else
Py_DECREF(raw);
}

res = _PyObject_CallMethodId(buffer, &PyId_seekable, NULL);
Expand All @@ -1201,17 +1195,12 @@ _io_TextIOWrapper___init___impl(textio *self, PyObject *buffer,
goto error;
self->seekable = self->telling = r;

res = _PyObject_GetAttrWithoutError(buffer, _PyIO_str_read1);
if (res != NULL) {
Py_DECREF(res);
self->has_read1 = 1;
}
else if (!PyErr_Occurred()) {
self->has_read1 = 0;
}
else {
r = _PyObject_LookupAttr(buffer, _PyIO_str_read1, &res);
if (r < 0) {
goto error;
}
Py_XDECREF(res);
self->has_read1 = r;

self->encoding_start_of_stream = 0;
if (_textiowrapper_fix_encoder_state(self) < 0) {
Expand Down Expand Up @@ -3020,10 +3009,9 @@ textiowrapper_newlines_get(textio *self, void *context)
{
PyObject *res;
CHECK_ATTACHED(self);
if (self->decoder == NULL)
Py_RETURN_NONE;
res = _PyObject_GetAttrWithoutError(self->decoder, _PyIO_str_newlines);
if (res == NULL && !PyErr_Occurred()) {
if (self->decoder == NULL ||
_PyObject_LookupAttr(self->decoder, _PyIO_str_newlines, &res) == 0)
{
Py_RETURN_NONE;
}
return res;
Expand Down
Loading