gh-112636: remove check for float subclasses without nb_float

[skirpichev]

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. */staticPyNumberMethodsxxx_as_number={.nb_float=NULL, }; staticPyObject*break_it(PyObject*o, PyObject*Py_UNUSED(ignored)){Py_TYPE(o)->tp_as_number->nb_float=NULL; Py_RETURN_NONE} staticPyMethodDefxxx_methods[] ={{"break_it", (PyCFunction) break_it, METH_NOARGS, NULL},{NULL}, }; PyTypeObjectXXX_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

---

• Issue: Remove check for float subclasses with nb_float == NULL in PyNumber_Float() ? #112636

Second look.

[skirpichev]

CC @vstinner, does it make sense for you?

[vstinner]

>>> 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]

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]

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

[skirpichev]

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.

[vstinner]

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

[serhiy-storchaka]

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

[skirpichev]

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]

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

[skirpichev]

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.

[skirpichev]

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)){
	returnPy_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]

Ok, I'm closing this.