GeezerVison

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.

Yeah, that makes sense.

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

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

This could get rid of the multiple appearance of Py_DECREF, the early return and the label

Sorry I just omitted the Module check as I thought that will always be there.

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

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

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.

Do you like the walrus operator? :)