Skip to content

gh-112636: remove check for float subclasses without nb_float #112637

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

Closed
wants to merge 4 commits into from

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Dec 3, 2023

This condition could be triggered only in very special scenario, if users code explicitly override inherited nb_float value and set it to NULL:

/* nb_float will be inherited anyway if it's set to NULL here,
   so this is essentially redundant.  */
static PyNumberMethods xxx_as_number = {
    .nb_float = NULL,
};

static PyObject*
break_it(PyObject *o, PyObject *Py_UNUSED(ignored))
{
    Py_TYPE(o)->tp_as_number->nb_float = NULL;
    Py_RETURN_NONE;
}

static PyMethodDef xxx_methods[] = {
    {"break_it", (PyCFunction) break_it, METH_NOARGS, NULL},
    {NULL},
};

PyTypeObject XXX_Type = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "xxx",
    .tp_base = &PyFloat_Type,
    .tp_as_number = &xxx_as_number,
    .tp_methods = xxx_methods,
};
diff --git a/Objects/abstract.c b/Objects/abstract.c
index 7cca81464c..41216eeb9b 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -1640,6 +1640,7 @@ PyNumber_Float(PyObject *o)
 
     /* A float subclass with nb_float == NULL */
     if (PyFloat_Check(o)) {
+        printf("XXX\n");
         return PyFloat_FromDouble(PyFloat_AS_DOUBLE(o));
     }
     return PyFloat_FromString(o);
>>> a = xxx(3.14)
>>> float(a)  # nb_float == NULL is not triggered, due to inheritance
3.14
>>> a.break_it()
>>> float(a)  # and only NOW this branch will work...
XXX
3.14

@skirpichev

This comment was marked as resolved.

@skirpichev

This comment was marked as resolved.

@skirpichev
Copy link
Contributor Author

CC @vstinner, does it make sense for you?

@vstinner
Copy link
Member

>>> a.break_it()
>>> float(a)  # and only NOW this branch will work...
XXX
3.14

I don't understand the example. float(a) does now return XXX? Or does it print XXX? Which part of the code prints XXX?

@skirpichev
Copy link
Contributor Author

I don't understand the example. float(a) does now return XXX?

Ah, that was just a debug print from the removed (by this pr) code. I've added a diff to description.

The point is that to trigger that code - you must explicitly set nb_float field in the derived class to NULL. But in this way we can break everything! Can't you set nb_add to NULL in a float subclass? Easy! But CPython has enough code, which assuming that float subclasses have working arithmetic dunder methods.

@vstinner
Copy link
Member

What is the behavior on this nb_float=NULL type with your change? Does return PyFloat_FromString(o) fail?

@skirpichev
Copy link
Contributor Author

What is the behavior on this nb_float=NULL type with your change?

It fails with TypeError (after you run first break_it()).

Does return PyFloat_FromString(o) fail?

Yes, because o is not a subclass of str/bytes/etc.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

This change is a backward incompatible change. You should document it in a NEWS entry.

@serhiy-storchaka
Copy link
Member

Sorry, but I now think that we should rather return a fallback for integers.

@skirpichev
Copy link
Contributor Author

This change is a backward incompatible change. You should document it in a NEWS entry.

Ok, I did. News entry was missed just as in 31a6554 (dropped similar check in PyNumber_Long).

I now think that we should rather return a fallback for integers.

But why?! Should we check on same ground that an int subtype has nb_add field set?

@serhiy-storchaka
Copy link
Member

Because PyFloat_AsDouble() never fails for instances of float subclasses. It does not use nb_float in that case.

@skirpichev
Copy link
Contributor Author

In same way we can recover in case when float subtype sets e.g. nb_negative to NULL. I think such subtypes are broken and it's better to fail quickly.

Co-authored-by: Victor Stinner <vstinner@python.org>
@skirpichev
Copy link
Contributor Author

now think that we should rather return a fallback for integers.

There is a difference, if nb_int is set to NULL, but nb_index is not broken (at least not NULL) - then we got an equivalent fallback with _PyLong_Copy, see:

cpython/Objects/abstract.c

Lines 1403 to 1405 in 31516c9

if (PyLong_Check(item)) {
return Py_NewRef(item);
}

and

cpython/Objects/abstract.c

Lines 1446 to 1448 in 31516c9

if (result != NULL && !PyLong_CheckExact(result)) {
Py_SETREF(result, _PyLong_Copy((PyLongObject *)result));
}

I still think we shouldn't keep workarounds for broken subtypes. But if so, at least the nb_int workaround shouldn't be restored.

@skirpichev
Copy link
Contributor Author

Ok, I'm closing this.

@skirpichev skirpichev closed this Nov 2, 2024
@skirpichev skirpichev deleted the fix-112636-nb_float branch November 2, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants