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.

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

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Apr 26, 2023

@brandtbucher brandtbucher added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 26, 2023
@brandtbucher brandtbucher self-assigned this Apr 26, 2023
Lib/test/test_call.py Show resolved Hide resolved
// 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

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

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? :)

@JelleZijlstra
Copy link
Member

@brandtbucher any reason not to merge this? Would be great to get it into 3.12. I'd merge it myself but maybe you have some reason to hold off.

@brandtbucher
Copy link
Member Author

Yep, it just fell off my radar. If anybody thinks the logic should be cleaned up, then that can be considered in another PR.

@brandtbucher brandtbucher merged commit 7d35c31 into python:main May 4, 2023
carljm added a commit to carljm/cpython that referenced this pull request May 5, 2023
* main: (61 commits)
  pythongh-64595: Argument Clinic: Touch source file if any output file changed (python#104152)
  pythongh-64631: Test exception messages in cloned Argument Clinic funcs (python#104167)
  pythongh-68395: Avoid naming conflicts by mangling variable names in Argument Clinic (python#104065)
  pythongh-64658: Expand Argument Clinic return converter docs (python#104175)
  pythonGH-103092: port `_asyncio` freelist to module state (python#104196)
  pythongh-104051: fix crash in test_xxtestfuzz with -We (python#104052)
  pythongh-104190: fix ubsan crash (python#104191)
  pythongh-104106: Add gcc fallback of mkfifoat/mknodat for macOS (pythongh-104129)
  pythonGH-104142: Fix _Py_RefcntAdd to respect immortality (pythonGH-104143)
  pythongh-104112: link from cached_property docs to method-caching FAQ (python#104113)
  pythongh-68968: Correcting message display issue with assertEqual (python#103937)
  pythonGH-103899: Provide a hint when accidentally calling a module (pythonGH-103900)
  pythongh-103963: fix 'make regen-opcode' in out-of-tree builds (python#104177)
  pythongh-102500: Add PEP 688 and 698 to the 3.12 release highlights (python#104174)
  pythonGH-81079: Add case_sensitive argument to `pathlib.Path.glob()` (pythonGH-102710)
  pythongh-91896: Deprecate collections.abc.ByteString (python#102096)
  pythongh-99593: Add tests for Unicode C API (part 2) (python#99868)
  pythongh-102500: Document PEP 688 (python#102571)
  pythongh-102500: Implement PEP 688 (python#102521)
  pythongh-96534: socketmodule: support FreeBSD divert(4) socket (python#96536)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) sprint type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants