Skip to content

feat: new mypyc primitives for weakref.proxy #19217

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

feat: new mypyc primitives for weakref.proxy #19217

wants to merge 25 commits into from

Conversation

BobTheBuidler
Copy link
Contributor

@BobTheBuidler BobTheBuidler commented Jun 3, 2025 •
edited
Loading

This PR adds 2 new weakref primitives for weakref.proxy (1 and 2 arg)

The C code generates correctly, but I'm not entirely sure why this test is failing. The weakly-proxied object is being destroyed too early, while there should still be a strong reference to it. It also fails if we use the builtin weakref.proxy, so I believe this might be exposing a reference counting bug unrelated to this PR.

@BobTheBuidler BobTheBuidler mentioned this pull request Jun 3, 2025
Merged
@BobTheBuidler BobTheBuidler marked this pull request as ready for review June 3, 2025 18:02
This was referenced Aug 2, 2025
Closed
Closed
@BobTheBuidler BobTheBuidler force-pushed the weak-proxy branch 2 times, most recently from 5117b20 to 834e739 Compare August 2, 2025 20:09
@BobTheBuidler

This comment was marked as resolved.

Sign in to view
@BobTheBuidler
Copy link
Contributor Author

Tests are fixed, this PR is now ready for review.

JukkaL
JukkaL reviewed Aug 15, 2025
View reviewed changes
@BobTheBuidler
Copy link
Contributor Author

Thanks for the feedbacks, I've addressed all of them and this PR is again ready for review.

@BobTheBuidler BobTheBuidler requested a review from JukkaL August 17, 2025 16:24
@BobTheBuidler
Copy link
Contributor Author

Hmm.. what happened here? the testutil import broke the tests?

@BobTheBuidler
Copy link
Contributor Author

@JukkaL any idea why replacing pytest.raises with assertRaises is giving us all of these type-related errors?

its another typeshed thing?

JukkaL
JukkaL reviewed Aug 29, 2025
View reviewed changes
mypyc/test-data/fixtures/test-weakref.pyi Outdated
class list: ...
class BaseException: ...
class Exception(BaseException): ...
class ReferenceError(Exception): ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fixture is probably causing the problems -- some of these definitions are too simplified (or missing). If ReferenceError is the only new thing you need here, what about adding it to the generic ir.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I saw a comment somewhere indicating not to add things to that file as it would slow down the whole suite.

No worries, I've updated the PR accordingly and the tests pass now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BobTheBuidler @JukkaL