8000 bpo-38530: Offer suggestions on AttributeError by pablogsal · Pull Request #16856 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-38530: Offer suggestions on AttributeError #16856

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

Merged
merged 7 commits into from
Apr 14, 2021
Merged

bpo-38530: Offer suggestions on AttributeError #16856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9c85b5
bpo-38530: Offer suggestions on AttributeError
pablogsal Oct 19, 2019
61d510b
Address Victor's feedback
pablogsal Apr 10, 2021
dbd88f0
More
pablogsal Apr 10, 2021
a3dd16a
Return the suggestion as a object and handle it in print_exception
pablogsal Apr 12, 2021
c760303
fixup! Return the suggestion as a object and handle it in print_excep…
pablogsal Apr 14, 2021
71e8766
Simplify the algorithm implementation
pablogsal Apr 14, 2021
45061e4
Update docs
pablogsal Apr 14, 2021
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
Prev Previous commit
Next Next commit
More
  • Loading branch information
@pablogsal
pablogsal committed Apr 10, 2021
commit dbd88f086a61041178189c0fcf7911cfcc8f4390
2 changes: 1 addition & 1 deletion Include/internal/pycore_suggestions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
# error "this header requires Py_BUILD_CORE define"
#endif

int _Py_offer_suggestions(PyObject* exception, PyObject* value);
int _Py_Offer_Suggestions(PyObject* exception, PyObject* value);

#endif /* !Py_INTERNAL_SUGGESTIONS_H */
22 changes: 11 additions & 11 deletions Objects/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,15 +889,17 @@ static inline int
set_attribute_error_context(PyObject* v, PyObject* name)
{
assert(PyErr_Occurred());
_Py_IDENTIFIER(name);
_Py_IDENTIFIER(obj);
// Intercept AttributeError exceptions and augment them to offer
// suggestions later.
if (PyErr_ExceptionMatches(PyExc_AttributeError)){
PyObject *type, *value, *traceback;
PyErr_Fetch(&type, &value, &traceback);
PyErr_NormalizeException(&type, &value, &traceback);
if (PyErr_GivenExceptionMatches(value, PyExc_AttributeError) &&
(PyObject_SetAttrString(value, "name", name) ||
PyObject_SetAttrString(value, "obj", v))) {
(_PyObject_SetAttrId(value, &PyId_name, name) ||
_PyObject_SetAttrId(value, &PyId_obj, v))) {
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

Is it really useful to report setattr() failed? the callers don't seem to care.

}
PyErr_Restore(type, value, traceback);
Expand All @@ -922,19 +924,20 @@ PyObject_GetAttr(PyObject *v, PyObject *name)
}
else if (tp->tp_getattr != NULL) {
const char *name_str = PyUnicode_AsUTF8(name);
if (name_str == NULL)
if (name_str == NULL) {
return NULL;
}
result = (*tp->tp_getattr)(v, (char *)name_str);
} else {
}
else {
PyErr_Format(PyExc_AttributeError,
"'%.50s' object has no attribute '%U'",
tp->tp_name, name);
Copy link
Member

Choose a reason for hiding this comment

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

maybe only set result = NULL; in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is code from before, I prefer to not change it in case we introduce some unwanted change

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok. It was just a suggestion, you can ignore it. it's just a coding style preference ;-)

}

if (!result && set_attribute_error_context(v, name)) {
return NULL;
if (result == NULL) {
set_attribute_error_context(v, name);
}

return result;
}

Expand Down Expand Up @@ -1195,10 +1198,7 @@ _PyObject_GetMethod(PyObject *obj, PyObject *name, PyObject **method)
"'%.50s' object has no attribute '%U'",
tp->tp_name, name);

if (set_attribute_error_context(obj, name)) {
return 0;
}

set_attribute_error_context(obj, name);
return 0;
}

Expand Down
4 changes: 2 additions & 2 deletions Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include "errcode.h" // E_EOF
#include "code.h" // PyCodeObject
#include "marshal.h" // PyMarshal_ReadLongFromFile()
#include "pycore_suggestions.h" // _Py_offer_suggestions
#include "pycore_suggestions.h" // _Py_Offer_Suggestions

#ifdef MS_WINDOWS
# include "malloc.h" // alloca()
Expand Down Expand Up @@ -1082,7 +1082,7 @@ PyErr_Display(PyObject *exception, PyObject *value, PyObject *tb)
return;
}

if (_Py_offer_suggestions(exception, value) != 0) {
if (_Py_Offer_Suggestions(exception, value) != 0) {
return;
}

Expand Down
59 changes: 32 additions & 27 deletions Python/suggestions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ distance(const char *string1, const char *string2)
/* initalize first row */
row = (size_t*)PyMem_Malloc(len2*sizeof(size_t));
if (!row) {
return (size_t)(-1);
return (Py_ssize_t)(-1);
}
end = row + len2 - 1;
for (i = 0; i < len2 - half; i++) {
Expand All @@ -72,7 +72,7 @@ distance(const char *string1, const char *string2)
* necessary */
row[0] = len1 - half - 1;
for (i = 1; i < len1; i++) {
size_t *p;
size_t *scan_ptr;
const char char1 = string1[i - 1];
const char *char2p;
size_t D, x;
Expand All @@ -82,17 +82,18 @@ distance(const char *string1, const char *string2)
size_t c3;

char2p = string2 + offset;
p = row + offset;
c3 = *(p++) + (char1 != *(char2p++));
x = *p;
scan_ptr = row + offset;
c3 = *(scan_ptr++) + (char1 != *(char2p++));
x = *scan_ptr;
x++;
D = x;
if (x > c3)
if (x > c3) {
x = c3;
*(p++) = x;
}
*(scan_ptr++) = x;
}
else {
p = row + 1;
scan_ptr = row + 1;
char2p = string2;
D = x = i;
}
Expand All @@ -101,24 +102,26 @@ distance(const char *string1, const char *string2)
end = row + len2 + i - half - 2;
}
/* main */
while (p <= end) {
while (scan_ptr <= end) {
size_t c3 = --D + (char1 != *(char2p++));
x++;
if (x > c3)
if (x > c3) {
x = c3;
D = *p;
}
D = *scan_ptr;
D++;
if (x > D)
x = D;
*(p++) = x;
*(scan_ptr++) = x;
}
/* lower triangle sentinel */
if (i <= half) {
size_t c3 = --D + (char1 != *char2p);
x++;
if (x > c3)
if (x > c3) {
x = c3;
*p = x;
}
*scan_ptr = x;
}
}
i = *end;
Expand All @@ -131,6 +134,7 @@ calculate_suggestions(PyObject* dir,
PyObject* name,
PyObject* oldexceptionvalue)
{
assert(!PyErr_Occurred());
assert(PyList_CheckExact(dir));

Py_ssize_t dir_size = PyList_GET_SIZE(dir);
Expand Down Expand Up @@ -166,13 +170,13 @@ calculate_suggestions(PyObject* dir,

static int
offer_suggestions_for_attribute_error(PyAttributeErrorObject* exc) {
int return_val = 0;
int return_val = -1;

PyObject* name = exc->name;
PyObject* v = exc->obj;
PyObject* name = exc->name; // borrowed reference
PyObject* obj = exc->obj; // borrowed reference

// Aboirt if we don't have an attribute name or we have an invalid one
if ((name == NULL) || (v == NULL) || !PyUnicode_CheckExact(name)) {
// Abort if we don't have an attribute name or we have an invalid one
if (name == NULL || obj == NULL || !PyUnicode_CheckExact(name)) {
return -1;
}

Expand All @@ -181,13 +185,15 @@ offer_suggestions_for_attribute_error(PyAttributeErrorObject* exc) {
switch (nargs) {
case 0:
oldexceptionvalue = PyUnicode_New(0, 0);
if (oldexceptionvalue == NULL) {
return -1;
}
break;
case 1:
oldexceptionvalue = PyTuple_GET_ITEM(exc->args, 0);
Py_INCREF(oldexceptionvalue);
// Check that the the message is an uncode objects that we can use.
// Check that the the message is an unicode objects that we can use.
if (!PyUnicode_CheckExact(oldexceptionvalue)) {
return_val = -1;
goto exit;
}
break;
Expand All @@ -197,8 +203,8 @@ offer_suggestions_for_attribute_error(PyAttributeErrorObject* exc) {
return 0;
}

PyObject* dir = PyObject_Dir(v);
if (!dir) {
PyObject* dir = PyObject_Dir(obj);
if (dir == NULL) {
goto exit;
}

Expand All @@ -210,15 +216,14 @@ offer_suggestions_for_attribute_error(PyAttributeErrorObject* exc) {
goto exit;
}

PyObject* old_args = exc->args;
PyObject* new_args = PyTuple_Pack(1, newexceptionvalue);
Py_DECREF(newexceptionvalue);
if (new_args == NULL) {
return_val = -1;
goto exit;
}
Py_SETREF(exc->args, new_args);
exc->args = new_args;
Copy link
Member

Choose a reason for hiding this comment

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

Why not storing suggestions in a new Exception field, rather than replacing existing args? I may break some applications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I don't want to touch the reporting facility to inspect said field or allowing users to inspect it manually

Copy link
Member Author
@pablogsal pablogsal Apr 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this substitution only happens before printing the value because it has been not caught. Nobody should be able to inspect this exception later, no? Notice this only happens in the default exception display, so if someone has a custom handler, they will not see this AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

Code level comment:
I don't understand why exc->args = new_args; Isn't it set by Py_SETREF?

Higher level comment:
Why do you replace exc->args instead of just printing suggestion after _PyErr_Display() or in _PyErr_Display()?

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 don't understand why exc->args = new_args; Isn't it set by Py_SETREF?

I forgot to remove that after updated the code after @vstinner suggestions.

Why do you replace exc->args instead of just printing suggestion after _PyErr_Display() or in _PyErr_Display()?

Actually, this is a very good point, this can make the code cleaner. I will implement this

Py_DECREF(old_args);
return_val = 0;

exit:
Py_DECREF(oldexceptionvalue);
Expand All @@ -229,7 +234,7 @@ offer_suggestions_for_attribute_error(PyAttributeErrorObject* exc) {
}


int _Py_offer_suggestions(PyObject* exception, PyObject* value) {
int _Py_Offer_Suggestions(PyObject* exception, PyObject* value) {
if (PyErr_GivenExceptionMatches(exception, PyExc_AttributeError) &&
offer_suggestions_for_attribute_error((PyAttributeErrorObject*) value) != 0) {
PyErr_Clear();
Expand Down
0