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

GH-103899: Provide a hint when accidentally calling a module #103900

Merged
merged 6 commits into from
May 4, 2023
Merged

GH-103899: Provide a hint when accidentally calling a module #103900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca5622d
Add failing regression test
brandtbucher Apr 26, 2023
0edff38
Proide a helpful hint for failed module calls
brandtbucher Apr 26, 2023
f97d995
blurb add
brandtbucher Apr 26, 2023
06fadfb
Add another test
brandtbucher Apr 26, 2023
833144d
Add more tests
brandtbucher Apr 26, 2023
e52c1f7
Ignore any other exceptions that pop up
brandtbucher Apr 26, 2023
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
32 changes: 32 additions & 0 deletions Lib/test/test_call.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import gc
import contextlib
import sys
import types


class BadStr(str):
Expand Down Expand Up @@ -202,6 +203,37 @@ def test_oldargs1_2_kw(self):
msg = r"count\(\) takes no keyword arguments"
self.assertRaisesRegex(TypeError, msg, [].count, x=2, y=2)

def test_object_not_callable(self):
msg = r"^'object' object is not callable$"
self.assertRaisesRegex(TypeError, msg, object())

def test_module_not_callable_no_suggestion_0(self):
msg = r"^'module' object is not callable$"
self.assertRaisesRegex(TypeError, msg, types.ModuleType("mod"))

def test_module_not_callable_no_suggestion_1(self):
msg = r"^'module' object is not callable$"
mod = types.ModuleType("mod")
mod.mod = 42
self.assertRaisesRegex(TypeError, msg, mod)

def test_module_not_callable_no_suggestion_2(self):
msg = r"^'module' object is not callable$"
mod = types.ModuleType("mod")
del mod.__name__
self.assertRaisesRegex(TypeError, msg, mod)

def test_module_not_callable_no_suggestion_3(self):
msg = r"^'module' object is not callable$"
mod = types.ModuleType("mod")
mod.__name__ = 42
self.assertRaisesRegex(TypeError, msg, mod)

def test_module_not_callable_suggestion(self):
JelleZijlstra marked this conversation as resolved.
Show resolved Hide resolved
msg = r"^'module' object is not callable\. Did you mean: 'mod\.mod\(\.\.\.\)'\?$"
mod = types.ModuleType("mod")
mod.mod = lambda: ...
self.assertRaisesRegex(TypeError, msg, mod)


class TestCallingConventions(unittest.TestCase):
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-04-26-15-14-23.gh-issue-103899.1pqKPF.rst
View file
Open in desktop
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`).
44 changes: 38 additions & 6 deletions Objects/call.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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 __name__ entry from a module. It would be confusing if you then get an error "module has no attribute name" when you try to call the module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that makes sense.

Copy link
Member

@gaogaotiantian gaogaotiantian Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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);
}

Copy link
Member

Choose a reason for hiding this comment

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

Not an expert on exceptions, but does _PyErr_Format() just replace the previous Exception set?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

But then we would still either need to duplicate the "normal" TypeError raise, or use a label (or flag) like this PR does. Right?

Copy link
Member

Choose a reason for hiding this comment

The 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 _PyErr_Clear(tstate); before setting the fallback error message to explicitly indicate "we don't care what exception is there, I'll just clear it". It has no effect when no exception is set.

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".

Copy link
Member

@gaogaotiantian gaogaotiantian Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But then we would still either need to duplicate the "normal" TypeError raise, or use a label (or flag) like this PR does. Right?

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.

Copy link
Member

Choose a reason for hiding this comment

The 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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down