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

Argument Clinic does not increase NoneType refcount when returning NoneType from a function with no parameters #95007

Closed
noamcohen97 opened this issue Jul 19, 2022 · 4 comments
Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@noamcohen97
Copy link
Contributor

Bug report

/*[clinic input]
function -> NoneType
[clinic start generated code]*/

will produce this function:

static PyObject *
function(PyObject *module, PyObject *Py_UNUSED(ignored))
{
    return function_impl(module);
}

while function_impl returns Py_None (as it should, if using NoneType return converter), refcount is not increased.

here is a working example of NoneType return converter:

/*[clinic input]
function -> NoneType

    a: int
    /
[clinic start generated code]*/
static PyObject *
function(PyObject *module, PyObject *arg)
{
    PyObject *return_value = NULL;
    int a;
    PyObject *_return_value;

    a = _PyLong_AsInt(arg);
    if (a == -1 && PyErr_Occurred()) {
        goto exit;
    }
    _return_value = function_impl(module, a);
    if (_return_value != Py_None) {
        goto exit;
    }
    return_value = Py_None;
    Py_INCREF(Py_None);

exit:
    return return_value;
}

Generation also fails when generating a function with a single object parameter.

It seems like the problem is with default_return_converter in clinic.py, being set to True

@noamcohen97 noamcohen97 added the type-bug An unexpected behavior, bug, or error label Jul 19, 2022
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 19, 2022
It has confusing semantic which does not provide any benefit (the
only difference is that you should write "return Py_None" instead
of "Py_RETURN_NONE"), it is not currently used, and it is broken.
@serhiy-storchaka
Copy link
Member

Good catch. This converter is not used, so the bug was not exposed. And since it has dubious semantic (requires returning a weak reference to None on success), I think that it is better to remove it.

@noamcohen97
Copy link
Contributor Author

How about we'll get rid of the callback return value, and use PyErr_Occurred() to chose whether to return NULL or Py_None?

@arhadthedev
Copy link
Member

arhadthedev commented Jul 19, 2022

How about we'll get rid of the callback return value, and use PyErr_Occurred() to chose whether to return NULL or Py_None?

When I did the same in gh-91284, I found that PyErr_Occurred takes GIL. Currently (until nogil is implemented, at least) it's cheaper to deal with PyErr_Occurred only on NULL returned from a callback.

serhiy-storchaka added a commit that referenced this issue Jul 20, 2022
It has confusing semantic which does not provide any benefit (the
only difference is that you should write "return Py_None" instead
of "Py_RETURN_NONE"), it is not currently used, and it is broken.
noamcohen97 added a commit to noamcohen97/cpython that referenced this issue Aug 1, 2022
…inic Doc

NoneType return converter was removed in 74b5e4c
erlend-aasland pushed a commit that referenced this issue Aug 1, 2022
AA-Turner pushed a commit to AA-Turner/devguide that referenced this issue Sep 13, 2023
erlend-aasland pushed a commit to python/devguide that referenced this issue Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants