FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-112636: remove check for float subclasses without nb_float by skirpichev · Pull Request #112637 · python/cpython · GitHub

/ cpython Public

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

Closed
skirpichev wants to merge 4 commits into
python:mainfrom
skirpichev:fix-112636-nb_float
ConversationCommits (4)ChecksFiles changed
Closed

gh-112636: remove check for float subclasses without nb_float#112637
skirpichev wants to merge 4 commits into
python:mainfrom
skirpichev:fix-112636-nb_float

Conversation

skirpichev commented Dec 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

bedevere-app Bot mentioned this pull request Dec 3, 2023
Closed

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

skirpichev commented Sep 27, 2024

Copy link
Copy Markdown
Member Author

CC @vstinner, does it make sense for you?

vstinner commented Sep 27, 2024

Copy link
Copy Markdown
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 commented Sep 28, 2024

Copy link
Copy Markdown
Member 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 commented Sep 30, 2024

Copy link
Copy Markdown
Member

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

skirpichev commented Oct 1, 2024

Copy link
Copy Markdown
Member 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.

vstinner reviewed Oct 1, 2024
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

serhiy-storchaka commented Oct 1, 2024

Copy link
Copy Markdown
Member

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

skirpichev commented Oct 1, 2024

Copy link
Copy Markdown
Member 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 commented Oct 1, 2024

Copy link
Copy Markdown
Member

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

skirpichev commented Oct 1, 2024

Copy link
Copy Markdown
Member 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.

Apply suggestions from code review
7f14215 
Co-authored-by: Victor Stinner <vstinner@python.org>

skirpichev commented Oct 7, 2024

Copy link
Copy Markdown
Member 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 mentioned this pull request Oct 7, 2024
Closed

skirpichev commented Nov 2, 2024

Copy link
Copy Markdown
Member Author

Ok, I'm closing this.

skirpichev closed this Nov 2, 2024
skirpichev deleted the fix-112636-nb_float branch November 2, 2024 05:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner left review comments
serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL