Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-103092: Port some _ctypes data types to heap types #113630

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

neonene
Copy link
Contributor

@neonene neonene commented Jan 1, 2024

Currently, PyType_From* functions refuse or ignore creating classes whose metaclass overrides tp_new (#103968). The ctypes extension needs some workaround for that.

@neonene
Copy link
Contributor Author

neonene commented Jan 2, 2024

@vstinner Could you review this behavior change?

@neonene neonene changed the title gh-103092: Make Simple_Type in ctypes into heap types gh-103092: Port some _ctypes data types to heap types Jan 3, 2024
@neonene
Copy link
Contributor Author

neonene commented Jan 5, 2024

This is an attempt without hacking type slots.

Problem: It is possible that accessing metaclasses cause a crash with bad arguments (e.g. type(c_int).__init__(...)), which this PR does not tackle.

cc PEP 687 experts: @erlend-aasland @kumaraditya303

@neonene
Copy link
Contributor Author

neonene commented Jan 6, 2024

Leaving an alternative PR (#113774) which does not have the problem above. However, tp_new slot is hacked. Is there any better way to isolate _ctypes?

cc @encukou

@encukou
Copy link
Member

encukou commented Jan 8, 2024

To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the *_Type pointers to ctypes_state, but initializes them pointers to the existing static types for now? Also change all the *_Check calls -- those should look the same regardless of how the metaclass issue is solved. The next PR can then focus only on the problematic case.

IMO, the correct way with the current heap type creation API is to move the initialization to tp_init as you do, but ensure that tp_init was called exactly once: that is, all operations should fail cleanly if it was not run on a given type, and it should fail (or do nothing) if called a second time.

Another option is to improve the type creation API -- e.g. design a slot that works like __subclass_init__ but CPython guarantees to call it exactly once.

@vstinner
Copy link
Member

vstinner commented Jan 8, 2024

@vstinner Could you review this behavior change?

I would prefer a smaller PR. You moved 7 types. Can you start with a PR which change less types and less files at once?

@neonene
Copy link
Contributor Author

neonene commented Jan 10, 2024

@encukou Is it worth opening a new issue for that? Maybe a custom meta tp_init can be checked without switching to a heap type?

@encukou
Copy link
Member

encukou commented Jan 10, 2024

Oh, I think it's worth writing a PEP for that :)
If you want to help there, a good first step would be a proof-of-concept PR (without an issue).

@neonene
Copy link
Contributor Author

neonene commented Jan 10, 2024

Sorry, I'm not qualified, as I'm volunteering anonymously.
A fix for issues around #113055 without porting _ctypes is also fine with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants