Skip to content

feat(mypyc): proper weakref support #19056

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

BobTheBuidler
Copy link

@BobTheBuidler BobTheBuidler commented May 8, 2025

This is the beginning of a PR intended to fix a compatibility issue between mypyc and weakrefs

Fixes mypyc/mypyc#1102

@BobTheBuidler
Copy link
Author

BobTheBuidler commented May 8, 2025

I've added a supports_weakref attribute to ClassIR and a check to add a weakref slot in has has_dict is false but supports_weakref is True

I have not yet added any logic for setting supports_weakref, it is currently always set == cls.has_dict. I'm thinking to implement one or more of the options listed here: mypyc/mypyc#1102

Something in this PR has broken your serialization tests, it would be helpful if somebody could share some pointers wrt where I should look.

@BobTheBuidler
Copy link
Author

Well... I tried fixing the serialization issue and now I just get segfaults instead. Not sure where to go from here.

@BobTheBuidler
Copy link
Author

BobTheBuidler commented May 13, 2025

Great, the segfaults are fixed and the serialization tests are green

Next steps:

  • add tests for mypyc_attr(supports_weakref=True)
  • add tests for has_dict=False but supports_weakref=True

Distant todo (maybe?):

  • Detect weakref attr defined in init and set ir.supports_weakref=True

@BobTheBuidler BobTheBuidler changed the title feat: proper weakref support feat: proper weakref support for mypyc May 13, 2025
@BobTheBuidler BobTheBuidler changed the title feat: proper weakref support for mypyc feat(mypyc): proper weakref support May 13, 2025
@BobTheBuidler
Copy link
Author

Hell yeah, it works!
@JukkaL let me know what you think, and if the additional test cases are even necessary given that these newly added tests all pass and the to_dict classes didn't break

@BobTheBuidler BobTheBuidler marked this pull request as ready for review May 13, 2025 16:49
@BobTheBuidler
Copy link
Author

BobTheBuidler commented May 13, 2025

TODO: I found this in cpython's docs:

The only further addition is that tp_dealloc needs to clear any weak references (by calling PyObject_ClearWeakRefs()) if the field is non-NULL:

static void
Trivial_dealloc(TrivialObject *self)
{
    /* Clear weakrefs first before calling any destructors */
    if (self->weakreflist != NULL)
        PyObject_ClearWeakRefs((PyObject *) self);
    /* ... remainder of destruction code omitted for brevity ... */
    Py_TYPE(self)->tp_free((PyObject *) self);
}

https://docs.python.org/3.8/extending/newtypes.html#weak-reference-support

@BobTheBuidler
Copy link
Author

BobTheBuidler commented May 13, 2025

I added some logic to the dealloc function as per the python docs above, it fails on all python versions

for python 3.8-3.11 it fails due to the if check, and the fact that weakreflist is not a member of the struct. I believe this can either be fixed by adding the member to the struct (difficult, I noticed you have this commented in the code as a #TODO) or looking up the weakref field instead (potentially easier). I am blocked by not knowing how to access the weakref field here, or whether we can even access a python attribute in the dealloc function.

for python 3.12 and 3.13 I think it segfaults because we need to #include weakref.h so we can call PyObject_ClearWeakRefs. I'm not sure of this however, and don't know how to emit the #include so I can test my theory.

Leaving this PR as-is for now, will come back in a few days and work on it some more.

py_new_weak_ref_op = function_op(
name="weakref.weakref",
arg_types=[object_rprimitive],
# TODO: how do I pass NULL as the 2nd arg?
Copy link
Author

@BobTheBuidler BobTheBuidler May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JukkaL I've also added 2 new primitives for weakref.ref and weakref.proxy, when called with 2 args. I'm now working on 2 more primitives when called with 1 arg. I just need to know how to pass a NULL as the 2nd arg to the c func. what would you advise here?

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.

Compatibility issues with weakrefs
1 participant