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

Remove use of tp_new #1217

Merged
merged 26 commits into from
Sep 30, 2024
Merged

Remove use of tp_new #1217

merged 26 commits into from
Sep 30, 2024

Conversation

Thrameos
Copy link
Contributor

This is a placeholder for removing the deprecated tp_new to comply with Python 3.14. This PR won't work yet for a few reasons.

  1. Exceptions currently don't properly inherit the base so everything on the exception path does not work currently. This should be a special case in the MetaClass creation process.
  2. The allocator are not sticky in some places for mysterious reasons, regarding the removal of tp_new. It was the only central location that we could guarantee that all the fields were set properly on new types. I got a fair number of them in this PR but the strays all need to be chased down and resolved.
  3. We have to go back through every version of Python to find out where the patterns work and fail. This is a time consuming task.

I am debating whether I can just override the tp_call method and add our type checks at that point rather than tp_new.

native/python/pyjp_value.cpp Dismissed Show dismissed Hide dismissed
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 79.03226% with 13 lines in your changes missing coverage. Please review.

Project coverage is 87.18%. Comparing base (09f325f) to head (6ea93af).
Report is 27 commits behind head on master.

Files with missing lines Patch % Lines
native/python/pyjp_value.cpp 69.56% 5 Missing and 2 partials ⚠️
native/python/pyjp_class.cpp 81.25% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1217      +/-   ##
==========================================
- Coverage   87.21%   87.18%   -0.03%     
==========================================
  Files         113      113              
  Lines       10277    10281       +4     
  Branches     4088     4075      -13     
==========================================
+ Hits         8963     8964       +1     
- Misses        718      722       +4     
+ Partials      596      595       -1     
Flag Coverage Δ
87.18% <79.03%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Thrameos
Copy link
Contributor Author

@marscher I think that this one should solve the issues on Python 3.14 with deprecated tp_new on meta classes.

The following was done in the PR.

  1. Switch call to use tp_call rather than tp_new. As it turned out we were actually skipping the init method which may have been problematic.
  2. Change the create with Meta to work with the Python provided. It was a problem before because it contained checks for the tp_new
  3. Moved all of the logic from tp_new to tp_init for JClass.
  4. Implemented __init_subclass__ the default version installed by the heap dies on kwargs which prevents the special tokens that we use during class initialization to fail. This was the main reason we needed to use the tp_new slot is that we had to get the kwargs before it reached the misbehaving __init_subclass__. This sort of seems like a bug in Python as making the default eat kwargs is disabling.

If we ever need to add precheck logic on the class (such as allowing classes to be overwritten in Python, then we will need to implement a tp_call on JClassMeta or add a __init_subclass__ in there someplace such that the we can interrogate the methods being installed on the class to create a overload stub.

@marscher marscher added this to the python-3.14 milestone Sep 18, 2024
native/python/*.c Outdated Show resolved Hide resolved
@Thrameos
Copy link
Contributor Author

Thrameos commented Sep 18, 2024 via email

@Thrameos
Copy link
Contributor Author

@marscher October is soon upon us and I will need to make our yearly maintence release. Please tag/approve PRs so I can start the release process.

@marscher
Copy link
Member

@Thrameos do you want to include this for the next release? After reading your comments I got the impression that it is not yet stable - am I wrong?

@Thrameos
Copy link
Contributor Author

The instability appeared to be been tied directly to Python 3.13 and the different attempts to fix the allocator problem. The first was "stable" but extremely dangerous as it was altering active types. The second used approved but unstable methods, but failed because those methods don't work. The third was built on a variation of the Python 3.12 hack which created a dummy type purely to be manipulated. That option proved to be the most stable of the options. Using that as a foundation the tp_new deprecation fix appears completely stable. So I believe it ready to be applied.

@marscher marscher merged commit fbd4724 into jpype-project:master Sep 30, 2024
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants