| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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.
#[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.~ = ...;
}
And another related thing is:
|
Sorry, something went wrong.
|
Ah I forgot an important point: currently we ourselves allocate type objects. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Ok, it's now far enough along that test_arithmetics passes! |
Sorry, something went wrong.
Ah I see how you changed get_or_init and it looks OK, except we have to set an additional flag Py_TPFLAGS_HEAPTYPE. |
Sorry, something went wrong.
|
It automatically sets that flag for us (very nice of it!) https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2949 |
Sorry, something went wrong.
|
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 :-) |
Sorry, something went wrong.
Sorry, something went wrong.
|
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.
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
🤔 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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>(...) {}
This test is not so important and you can either opt-in it by Py_LIMITED_API or just removing it. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
yeah, that works -- is there an example of a raw callback raising an exception that I can use? |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
that'll work. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
That sounds reasonable to me. |
Sorry, something went wrong.
|
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... |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I found a workaround... just set tp_dictoffset on the PyTypeObject* after it's created... it seems to work |
Sorry, something went wrong.
|
Ok, __text_signature__ is now behaving differently... I know nothing about this, so time to investigate I suppose! |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
locally on CPython the examples tests pass, so it might be a pypy bug :-/ |
Sorry, something went wrong.
|
I cannot understand why you removed some tests. I feel we're not agreed with that point. |
Sorry, something went wrong.
|
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:
The thing all of these have in common is that CPython treats a heap type different than a static type. |
Sorry, something went wrong.
|
Thank you for listing them. |
Sorry, something went wrong.
|
I added #[ignore] with comments. |
Sorry, something went wrong.
|
Tests are now green :-) |
Sorry, something went wrong.
|
Thank you, let me use some time to create a continuation PR |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.