Use PyType_Spec for creating new types in Rust

[alex]

This is a proof of concept -- implements just enough of it to prove the idea works! There's still a lot of missing elements of this (look at all the commented out code!)

Before I went too much further and implemented all the different function pointers, I wanted to check in and see if this approach looked ok.

[kngwyu]

First I'm sorry that the approach I told does not work since we cannot merge this branch to abi3 now. Maybe I push some commits to your branch and then it can cause some conflicts.

I'm for the PyType_Spec approach, but looking https://docs.python.org/3/c-api/type.html#c.PyType_Slot, there are some protocols that PyType_Spec cannot support.
So I think two important designs are:

• Explicitly separating functionalities that PyType_Spec cannot address. Concretely, I imagine something like:

#[cfg(Py_LIMITED_API)]
fn initialize_tp_obj_additional(type_object: &mut ffi::PyType_Object) {}
#[cfg(not(Py_LIMITED_API))]
fn initialize_tp_obj_additional(type_object: &mut ffi::PyType_Object) { 
    type_object.~ = ...;
}

• Raise compile errors if those functionalities are used with abi3. It's not so easy but maybe we can do.

And another related thing is:

• abi3 feature is not good that it is not additive but exclusive. I.e., we should rather make not(Py_LIMITED_API) as a feature flag and set it by default.
  But I'll address this problem myself after we move to abi3 branch.

[kngwyu]

Ah I forgot an important point: currently we ourselves allocate type objects.
So it is not straightforward to use PyType_FromSpec...

[alex]

Why is it a problem that pyo3 allocates it's own type objects now? This PR switches to PyType_FromSpec and it seems to work ok.

I will go through and finish porting all the other callbacks now.

[alex]

Ok, it's now far enough along that test_arithmetics passes!

[kngwyu]

Why is it a problem that pyo3 allocates it's own type objects now? This PR switches to PyType_FromSpec and it seems to work ok.

Ah I see how you changed get_or_init and it looks OK, except we have to set an additional flag Py_TPFLAGS_HEAPTYPE.

[alex]

It automatically sets that flag for us (very nice of it!) https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2949

[alex]

Ok, going to bed now. I start working on buffers, but it's not working, and I don't really know why. I suppose next step is gdb :-)

[kngwyu]

@alex

[alex]

python/cpython#22031 ... but I didn't realize it's only true since June! Not in any released Python.

…

On Tue, Sep 1, 2020 at 12:37 AM Yuji Kanagawa ***@***.***> wrote: @alex <https://github.com/alex> [image: image] <https://user-images.githubusercontent.com/16046705/91795268-2cbbbe00-ec58-11ea-970f-e55d6b50798e.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1132 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBBSSP7Y2GQL6Y4IMNTSDR267ANCNFSM4QRDGBGQ> .

-- All that is necessary for evil to succeed is for good people to do nothing.

[alex]

Here's a new issue I could use some feedback on. https://github.com/PyO3/pyo3/blob/master/tests/test_class_attributes.rs#L54-L60 is currently failing on this branch, because the mutation is allowed.

Currently on master, it passes because an exception is raised here: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L3399-L3409

However, on this branch, the type now has the Py_TPFLAGS_HEAPTYPE flag! I'm not sure what the right fix is. One option is just removing the test.

[davidhewitt]

🤔 Class immutability is a nice property, but I'm not sure we can preserve the existing behavior without doing something like creating a metaclass.

That's probably overly complicated for now, so I'd be cautiously ok for this feature to go. (At least in the short term, maybe with an issue to track restoring it?) Might want to wait to see what @kngwyu says too.

[alex]

Ok, then we're on to the next failing test: https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L122-L141

This fails because it expects that __module__ on a type whose tp_name doesn't have '.' in it to be "module". However, this is another behavior difference with heap types: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L544-L573

Personally I think this is a bit more correct, builtins was not particularly accurate anyways.

[kngwyu]

However, on this branch, the type now has the Py_TPFLAGS_HEAPTYPE flag! I'm not sure what the right fix is.

I think the only possible fix is having two functions:

#[cfg(Py_LIMITED_API)]
pub(crate) fn initialize_type_object<T>(...) {}
#[cfg(not(Py_LIMITED_API))]
pub(crate) fn initialize_type_object<T>(...) {}

Ok, then we're on to the next failing test: https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L122-L141

This test is not so important and you can either opt-in it by Py_LIMITED_API or just removing it.

[alex]

I'm trying very hard to as little duplicate code paths as well, so i"ll remove the test. Here's the next one:

https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L10-L19

Yet again, CPython has completely different behavior for heap types: https://github.com/python/cpython/blob/v3.8.2/Objects/typeobject.c#L4972-L4988 they appear to always get a tp_new no matter what, which makes them callable. Does this seem ok, or does it break something?

[davidhewitt]

I think the only possible fix is having two functions:

#[cfg(Py_LIMITED_API)]
pub(crate) fn initialize_type_object<T>(...) {}
#[cfg(not(Py_LIMITED_API))]
pub(crate) fn initialize_type_object<T>(...) {}

I wondered about this but would vote against splitting the #[pyclass] implementation on such a big way. I think if we duplicate any code, we should be super careful to ensure that the runtime behaviour is identical. I worry lots of people will get caught out if a package has different behaviour silently because it was installed from an abi3 wheel instead of a platform-specific one.

[davidhewitt]

Yet again, CPython has completely different behavior for heap types: https://github.com/python/cpython/blob/v3.8.2/Objects/typeobject.c#L4972-L4988 they appear to always get a tp_new no matter what, which makes them callable. Does this seem ok, or does it break something?

Default tp_new seems scary - it probably allocates the memory but doesn't initialize it? Can we add a default tp_new which raises TypeError?

[alex]

yeah, that works -- is there an example of a raw callback raising an exception that I can use?

[davidhewitt]

The safest way might be to do something like

callback_body!(py { Err(exceptions::PyTypeError::py_err("No constructor defined")) })

and trust the optimizer to remove all the gubbins :)

[alex]

that'll work.

[davidhewitt]

Just as a quick thought, once we've got this ready, should we potentially avoid merging it until after the 0.12 release? (#1104).

It looks like it might touch a fair bit of core code, and given the test coverage on PyO3 is... improving (😄)... it might be wiser to avoid merging until we've released 0.12. Which hopefully will be very soon!

[alex]

That sounds reasonable to me.

[davidhewitt]

IIRC subclass enables the class to be subclassed in Python - it's always possible in Rust regardless of that flag.

[alex]

Good news/bad news! The good news is that PyType_FromSpec seems to result in things being more consistent between Python/Rust... the bad news is that obviously it accomplishes this by being backwards incompatible...

https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2891-L2986 this enforcement is pretty core to PyType_FromSpec...

[alex]

https://bugs.python.org/issue38140 seems to suggest that there's no way to set tp_dictoffset from this API until 3.9 is out... I'm not sure how to proceed.

[alex]

I found a workaround... just set tp_dictoffset on the PyTypeObject* after it's created... it seems to work

[alex]

Ok, __text_signature__ is now behaving differently... I know nothing about this, so time to investigate I suppose!

[alex]

At this point all of cargo test is passing (the example tests still aren't). It required more changes to the tests than I like, but unfortunately heap types just seem to be slightly different in a bunch of ways.

[alex]

locally on CPython the examples tests pass, so it might be a pypy bug :-/

[kngwyu]

I cannot understand why you removed some tests.
I was going to support abi3 with some limited features, but are you going to remove some features of PyO3, even with not(Py_LIMITED_API)?

I feel we're not agreed with that point.

[alex]

I definitely agree it's not an appropriate solution, and I think we need a better one before this could be merged. Here's what happened for each of the tests I changed/removed:

• I added subclass to several classes -- it is required to inherit a class in Rust, the same way it's required in Python. https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2986
• I removed the immutable test because of https://github.com/python/cpython/blob/master/Objects/typeobject.c#L3399-L3409
• I changed the __module__ test because of https://github.com/python/cpython/blob/master/Objects/typeobject.c#L544-L573
• And I removed the __text_signature__ test because of https://github.com/python/cpython/blob/master/Objects/typeobject.c#L864-L870

The thing all of these have in common is that CPython treats a heap type different than a static type.

[kngwyu]

Thank you for listing them.
So, if you're going to retrieve some tests at last, could you please just mark them #[ignore] and add some comments?
If I have some time this weekend I'm going to push some changes to abi3 branch for some backward compatibility, and just #[ignore] is easier to retrieve then.

[alex]

I added #[ignore] with comments.

[alex]

Tests are now green :-)

[kngwyu]

Thank you, let me use some time to create a continuation PR