-
-
Notifications
You must be signed in to change notification settings - Fork 32k
GH-103899: Provide a hint when accidentally calling a module #103900
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
Changes from all commits
ca5622d
0edff38
f97d995
06fadfb
833144d
e52c1f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Provide a helpful hint in the :exc:`TypeError` message when accidentally | ||
calling a :term:`module` object that has a callable attribute of the same | ||
name (such as :func:`dis.dis` or :class:`datetime.datetime`). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,6 +167,42 @@ PyObject_VectorcallDict(PyObject *callable, PyObject *const *args, | |
return _PyObject_FastCallDictTstate(tstate, callable, args, nargsf, kwargs); | ||
} | ||
|
||
static void | ||
object_is_not_callable(PyThreadState *tstate, PyObject *callable) | ||
{ | ||
if (Py_IS_TYPE(callable, &PyModule_Type)) { | ||
// >>> import pprint | ||
// >>> pprint(thing) | ||
// Traceback (most recent call last): | ||
// File "<stdin>", line 1, in <module> | ||
// TypeError: 'module' object is not callable. Did you mean: 'pprint.pprint(...)'? | ||
PyObject *name = PyModule_GetNameObject(callable); | ||
if (name == NULL) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we instead suppress the new exception here and go back to the "object is not callable" message? Looks like this can happen if someone deletes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like this could be PyObject *name = PyModule_GetNameObject(callable);
PyObject *attr = NULL;
int suggestion = 0;
if (name != NULL) {
int res = _PyObject_LookupAttr(callable, name, &attr);
if (res > 0 && PyCallable_Check(attr)) {
_PyErr_Format(tstate, PyExc_TypeError,
"'%.200s' object is not callable. "
"Did you mean: '%U.%U(...)'?",
Py_TYPE(callable)->tp_name, name, name);
suggestion = 1;
}
}
Py_XDECREF(attr);
Py_XDECREF(name);
if (suggestion == 0) {
_PyErr_Format(tstate, PyExc_TypeError, "'%.200s' object is not callable",
Py_TYPE(callable)->tp_name);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not an expert on exceptions, but does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could get rid of the multiple appearance of Py_DECREF, the early return and the label There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I just omitted the Module check as I thought that will always be there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then we would still either need to duplicate the "normal" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation effectively "overwrites" the error if any exception is raised. We could add I think of this issue as a simple check - if certain condition is met, raise this error, otherwise that error. No exception during this process matters. When I take a look at the current implementation, I had to go though every logic path to confirm that the reference count was correct. That's another aspect that needs to be "reason about". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, that's something I missed. I would suggest this but it might be considered more difficult to reason about: PyObject *name = NULL;
PyObject *attr = NULL;
if (Py_IS_TYPE(callable, &PyModule_Type)
&& (name = PyModule_GetNameObject(callable))
&& _PyObject_LookupAttr(callable, name, &attr) > 0
&& PyCallable_Check(attr)) {
...
} This still follows the key idea - find the only condition we are interested about. However, there's a small C feature used for pointers that might get frown upon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you like the walrus operator? :) |
||
_PyErr_Clear(tstate); | ||
goto basic_type_error; | ||
} | ||
PyObject *attr; | ||
int res = _PyObject_LookupAttr(callable, name, &attr); | ||
if (res < 0) { | ||
_PyErr_Clear(tstate); | ||
} | ||
else if (res > 0 && PyCallable_Check(attr)) { | ||
_PyErr_Format(tstate, PyExc_TypeError, | ||
"'%.200s' object is not callable. " | ||
"Did you mean: '%U.%U(...)'?", | ||
Py_TYPE(callable)->tp_name, name, name); | ||
Py_DECREF(attr); | ||
Py_DECREF(name); | ||
return; | ||
} | ||
Py_XDECREF(attr); | ||
Py_DECREF(name); | ||
} | ||
basic_type_error: | ||
_PyErr_Format(tstate, PyExc_TypeError, "'%.200s' object is not callable", | ||
Py_TYPE(callable)->tp_name); | ||
} | ||
|
||
|
||
PyObject * | ||
_PyObject_MakeTpCall(PyThreadState *tstate, PyObject *callable, | ||
|
@@ -181,9 +217,7 @@ _PyObject_MakeTpCall(PyThreadState *tstate, PyObject *callable, | |
* temporary dictionary for keyword arguments (if any) */ | ||
ternaryfunc call = Py_TYPE(callable)->tp_call; | ||
if (call == NULL) { | ||
_PyErr_Format(tstate, PyExc_TypeError, | ||
"'%.200s' object is not callable", | ||
Py_TYPE(callable)->tp_name); | ||
object_is_not_callable(tstate, callable); | ||
return NULL; | ||
} | ||
|
||
|
@@ -332,9 +366,7 @@ _PyObject_Call(PyThreadState *tstate, PyObject *callable, | |
else { | ||
call = Py_TYPE(callable)->tp_call; | ||
if (call == NULL) { | ||
_PyErr_Format(tstate, PyExc_TypeError, | ||
"'%.200s' object is not callable", | ||
Py_TYPE(callable)->tp_name); | ||
object_is_not_callable(tstate, callable); | ||
return NULL; | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.