BUG: use __index__ in integer argument parsing - #32680

Thanks for working on this! The CI failures are real, use spin test to test locally and fix the problems :)

The CI failures are real but they're my fault 🙃.

See #32682. Once that's merged you can merge with main or rebase.

@shbhmbnsl1 the CI failure has been fixed on main. Either rebase or merge main to your branch to get a proper CI run here.

Just a random thought which may be bad actually. Would it be good or bad to put the error message the index conversion raised in the warning if the legacy conversion works? Like the warning infrorming about why __index__ failed. That's too verbose and unnecessary right or am I onto something here? 🤣

I dunno, I think we could do that as an add_note? But we don't have a habit for it. FWIW, if you raise the warning as an error you do see the chain (but sometimes it's tedious to do that and I suspect many users fail to have the idea to do that to track down issues...).

Yeah I was thinking it from the perspective that normal things that are convertible to integers in python nowadays should support __index__. So I'd guess that __index__ not working and the legacy conversion working to be something that may be worth finding out why __index__ did not work in the first place. Anyways, no suggestions here, mostly blabbering.

@shbhmbnsl1 this looks good from my side as well so feel free to deal with Sebastian's comment and just take it out of draft :)

Thanks @seberg and @ikrommyd for you comments.

What this still needs, is a test. There is no point in covering all functions, but please do cover a few random users of this converter in test_deprecations.py (that file follows a specific, if a bit outdated, pattern to make sure we both test the warning and if the warning is turned into an error).

So I added a test covering some public users of this converter, using a LegacyInteger object that implements __int__ but does not implement __index__. The test failed with a TypeError without emitting the deprecation warning. Because PyNumber_Index first fails because the object does not implement __index__, then the fallback clears that error PyLong_AsLong is called in the else condition, but PyLong_AsLong has also used only __index__ since python 3.10 and therefore does so on all Python versions currently supported by NumPy (>=3.12). So it fails with the same TypeError, and the function returns NPY_FAIL before used_legacy is set or the warning is emitted.
This was the same behavior anyways before this PR as the original converter already used PyLong_AsLong. And this test shows that we don't currently have a successful __int__ only legacy conversion to deprecate. If you think there is some other kind of legacy input that is expected to reach this fallback we can think about handling it. If not, I think we can switch to the modern __index__ completely without the fallback and the deprecation warning.

Thoughts @seberg @ikrommyd ?
I have added the test in the recent commit to showcase how I tested it. It will fail for now because of the fallback code. Once we have a decision on this I can modify the function and the test based on the approach.

Unfortunately I think the bot Sebastian banned made a point actually 🤣

But its a bot, so... 🤣

Unfortunately I think the bot Sebastian banned made a point actually 🤣

Indeed in cpython PyLong_AsLong calls PyLong_AsLongAndOverflow which calls PyNumber_Index. See here: https://github.com/python/cpython/blob/b62e0286858fcd9345b8ec40f490754a306a2186/Objects/longobject.c#L612 So if I'm reading cpython code correctly, we will just hit the error, then clear it, then hit it in the fallback again. So I don't think the fallback is reachable in reality.

I think we both were looking into the exact same thing at the exact same time. See my comment 1 minute ago LOL.

Just because the bot mentioned something that was tangential related doens't mean it had a point, because it didn't (it talked about statefulness which doesn't matter).

That doesn't mean that the whole thing wasn't a red-herring, though :(, sorry @shbhmbnsl1!

On the original agent thread Nathan's agent found that float subclasses changed and I misread the situation: Python 3.12 changed "i" argument parsing, but I missed that it of course cleaned up everything and also changed PyLong_FromLong.

The only thing that "changed" is the explicit float check which only matters for crazy cases like the agent pointed out. Which is, honestly, also ridiculously unimportant.

However: There is still something to do here @shbhmbnsl1, if much reduced: Since we require Python 3.12, we can delete the PyFloat_Check() from the function, it serves no purpose anymore!
(I would honestly not even add Nathan's crazy test, I don't see a point in promising that behavior in the future.)

I believe the function can just be something around the lines of

NPY_NO_EXPORT int
PyArray_PythonPyIntFromInt(PyObject *obj, int *value)
{
    long result = PyLong_AsLong(obj);
    if (NPY_UNLIKELY((result == -1) && PyErr_Occurred())) {
        return NPY_FAIL;
    }
    if (NPY_UNLIKELY((result > INT_MAX) || (result < INT_MIN))) {
        PyErr_SetString(PyExc_OverflowError,
                        "Python int too large to convert to C int");
        return NPY_FAIL;
    }
    *value = (int)result;
    return NPY_SUCCEED;
}

@seberg @ikrommyd Thank you for your comments. I agree with the approach here and have updated the function accordingly. Also I decided to remove the test that I added to showcase that PyLong_AsLong has also started using __index__ because the whole purpose of that test was to test the deprecation but thats no longer needed.
Let me know what you think of the changes, now that its fairly minimal.

Let me know what you think of the changes, now that its fairly minimal.

Yap, looks good, but tests are failing and need to be adapted.

All tests pass now!