GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112636: remove check for float subclasses without nb_float by skirpichev · Pull Request #112637 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Dec 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
  • Issue: Remove check for float subclasses with nb_float == NULL in PyNumber_Float() ? #112636

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@skirpichev
pythongh-112636: remove check for float subclasses without nb_float
56d7b83
@bedevere-app bedevere-app Bot added the awaiting review label Dec 3, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Dec 3, 2023
Remove check for float subclasses with nb_float == NULL in PyNumber_Float() ? #112636
Closed
@skirpichev

This comment was marked as resolved.

Sign in to view
serhiy-storchaka
serhiy-storchaka previously approved these changes Dec 3, 2023
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 3, 2023
@serhiy-storchaka serhiy-storchaka added the skip news label Dec 3, 2023
@serhiy-storchaka
serhiy-storchaka dismissed their stale review December 3, 2023 09:46

Second look.

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting merge labels Dec 3, 2023
@skirpichev

This comment was marked as resolved.

Sign in to view
@skirpichev
skirpichev requested a review from serhiy-storchaka March 22, 2024 03:07
@skirpichev

skirpichev commented Sep 27, 2024

Copy link
Copy Markdown
Member Author

CC @vstinner, does it make sense for you?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Oct 1, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@skirpichev skirpichev removed the skip news label Oct 1, 2024
skirpichev added 2 commits October 1, 2024 14:54
@skirpichev
+ news
8a0a04d
@skirpichev
Merge branch 'main' into fix-112636-nb_float
85e3df0
@serhiy-storchaka

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Oct 1, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/C_API/2024-10-01-14-53-55.gh-issue-112636.t4WJjC.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/C_API/2024-10-01-14-53-55.gh-issue-112636.t4WJjC.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev @vstinner
Apply suggestions from code review
7f14215 
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev skirpichev mentioned this pull request Oct 7, 2024
gh-109802: removed inaccessible code from complexobject.c #109642
Closed
@skirpichev

skirpichev commented Nov 2, 2024

Copy link
Copy Markdown
Member Author

Ok, I'm closing this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@skirpichev @vstinner @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.