Fix abi3 conversion of __complex__ classes

[jakelishman]

Python classes that were not complex but implemented the __complex__ magic would have that method called via PyComplex_AsCComplex when running against the full API, but the limited-API version PyComplex_RealAsDouble does not attempt this conversion. If the input object is not already complex, we can call the magic before proceeding.

Happy to modify if I've made a mess of testing strategy - I'm not sure I've managed to do it in the same style as other tests.

Fix #3164.

[DataTriny]

First time contributor has agreed to the new licensing scheme.

[jakelishman]

I've pushed a couple more commits on the basis of the review. I'm happy to revert the descriptor-resolution commit and replace it just with a comment about similarity not equality to CPython's implementation, if you'd rather.

edit: I think the CI failures should have been fixed by the merge of #3194.

[jakelishman]

A minor procedural question: this PR is getting a little messy in commit history, and with the CI failures, needs to be updated over main. Do you prefer me to leave the history as-is and push an additional merge commit into this PR, or do you prefer me to rebase these commits back into a coherent story (1. add the new PyAny/ffi method, 2. fix the complex conversion)?

[adamreichold]

A minor procedural question: this PR is getting a little messy in commit history, and with the CI failures, needs to be updated over main. Do you prefer me to leave the history as-is and push an additional merge commit into this PR, or do you prefer me to rebase these commits back into a coherent story (1. add the new PyAny/ffi method, 2. fix the complex conversion)?

We certainly prefer a coherent story and have explicitly configured bors to not squash merges, so turning this back into two commits would be really helpful!

[adamreichold]

I think expect for rebasing, this looks great now. Thank you again for following up on all the side quests.

Will wait a day or two before merging to see if any other maintainers have additional thoughts on this.

[jakelishman]

No trouble - I've just rebased down to two commits.

Just for visibility, there's a couple of my decisions that I think other maintainers might want to question:

• the naming/location/visibility of the new PyAny::getattr_special. I put it on PyAny because it made most sense to me, and the naming/visibility are from Fix abi3 conversion of __complex__ classes #3185 (comment). Maybe since it's on PyAny rather than being a loose helper it might make more sense as a full pub?
• the return type of that method being PyResult<Option<&PyAny>> rather than PyResult<&PyAny> - it's not typically an immediate error if a magic method doesn't exist in my head, at least.

[davidhewitt]

Thanks, the tests are great and I'm happy that the implementation is correct!

I have a couple of suggestions to round this off, I'm happy to see this merged when those are either addressed or dismissed.

[davidhewitt]

As for visibility, I suggest we keep lookup_special private for now until users give us a reason to make this public. _PyObject_LookupSpecial feels like something which is for diving into the weeds and it's not immediately clear to me when we'd want to recommend it to users over self.get_type().getattr(...) (plus descriptor) dance.

[jakelishman]

Well, I'm certainly glad that you mentioned PyType_GetSlots and how it might not work before Python 3.10. It transpires that you're correct on the versioning and that it appears frequently because things like method are static types, so trying PyType_GetSlots on one of those fails to spot the descriptor. Thanks for that advice - I'm fairly new to programming against the C API, and so I've not got great knowledge of all the available surface.

More importantly, though, it made me realise that _PyObject_LookupSpecial changed signature just very recently. I thought I'd been running my tests using Python 3.9 locally, but I hadn't - I'd just been using 3.11, and CI only tests 3.11 so I hadn't noticed any issues. Prior to 3.11, _PyObject_LookupSpecial is what's now called _PyObject_LookupSpecialId, and I don't think that's something we can bind against, because the IDs are shared static objects in the PyRuntimeState of the main interpreter.

What course of action would you like here?

• I could expose _PyObject_LookupSpecial in the FFI for Python 3.11+ only and have PyAny::lookup_special fallback to the getattr-based limited API form in lower CPythons, in addition to the limited API and PyPy?
• Alternatively, I could remove _PyObject_LookupSpecial from the FFI and use only a limited-API-safe version PyAny::lookup_special, since _PyObject_LookupSpecial might not be stable enough (and a special case for latest-Python-only in a probably non-performance-critical code path might be a bit messy). I can switch this to a PyType_GetSlots-based implementation with appropriate checking of the heap-type flags for pre-3.10 CPythons to fallback to a slower getattr-based version.

Fwiw, the complexity I'm finding here in correctly resolving non-slotted magic-method lookups across all Pythons strengthens the case for a central PyAny::lookup_special in my mind, for other PyO3-internal uses - there's so much subtlety I think it'd be very tricky to reproduce correctly elsewhere.

(All your other suggestions are easy and uncontentious to me.)

[adamreichold]

I had already typed the "bors try" comment yesterday to give this a while with the full CI but then decided against spending the CPU time...

Alternatively, I could remove _PyObject_LookupSpecial from the FFI and use only a limited-API-safe version PyAny::lookup_special, since _PyObject_LookupSpecial might not be stable enough (and a special case for latest-Python-only in a probably non-performance-critical code path might be a bit messy). I can switch this to a PyType_GetSlots-based implementation with appropriate checking of the heap-type flags for pre-3.10 CPythons to fallback to a slower getattr-based version.

Personally, I would probably prefer this solution for now. We can always add more fast paths in the future and this variant does not commit us to any new API surface even in the FFI module.

[jakelishman]

Yeah, I think that's what I'd have suggested if someone was contributing code on a project I maintained. I'll write this all up and push a couple of new commits to show what it looks like. Testing against older Pythons makes it look to me like I may have found a bug in Python::run, but I'll investigate that a little more separately - it's totally orthogonal to this PR, except that the particular way I initially wrote the tests needed tweaking.

(Sorry this has got super deep in the weeds - I wasn't expecting that! I can say that I'm having great fun learning way more about Python/CPython internals, though.)

[adamreichold]

Testing against older Pythons makes it look to me like I may have found a bug in Python::run, but I'll investigate that a little more separately - it's totally orthogonal to this PR, except that the particular way I initially wrote the tests needed tweaking.

Please have a look at #2927 and #2891, maybe you hit one of those.

[jakelishman]

I've rebased this branch again with the changes suggested above. It appears to work for me on CPythons 3.7 to 3.11 with both versioned and stable APIs, but I wasn't able to test PyPy myself because the tests seem to segfault immediately for me (even on main) with pypy3 from homebrew on macOS. It's quite possible I'm running them wrong, though - I was doing cargo test --lib -F=full with a pypy venv active.

[adamreichold]

Let's give it a whirl in the CI then.

bors try

[bors]

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion

[davidhewitt]

Nice one! I have just a couple of tiny code style suggestions for the new lookup_special body, otherwise this looks perfect.

cargo test on PyPy is known to not work, it's a deficiency on PyPy's side which I hope to resolve one day. We have a pytests crate which builds an extension module and runs a small set of tests, which PyPy does support.

[jakelishman]

There we go - style changes made.

[about pypy]

Ah yeah, that'd explain it - I'll have a look at doing that properly next time round, thanks!

[adamreichold]

I think this is good to go now having collected two approvals with all threads resolved.

bors r+

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• conclusion

[jakelishman]

Thanks for all the help, chaps!