feat: new mypyc primitive for weakref.ref#19099
Conversation
|
Done. This just needs tests now.
|
e4485d1 to
97f9186
Compare
|
@JukkaL I've cleaned up the commit history and this is now ready for review. |
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a suggestion about how to make this more general.
|
Oh great, I was trying and trying different ways to do this and eventually
gave up. Will put that together sometime this week.
Any thoughts on how I might determine the correct fullname for
weakref.proxy so I can handle that with both cases (1- and 2-arg) at the
same time?
…On Mon, Jun 2, 2025 at 12:12 PM Jukka Lehtosalo ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks for the PR! Left a suggestion about how to make this more general.
------------------------------
In mypyc/primitives/weakref_ops.py
<#19099 (comment)>:
> @@ -0,0 +1,13 @@
+from mypyc.ir.ops import ERR_MAGIC
+from mypyc.ir.rtypes import object_rprimitive
+from mypyc.primitives.registry import function_op
+
+# Weakref operations
+
+new_ref_op = function_op(
+ name="weakref.ReferenceType",
+ arg_types=[object_rprimitive, object_rprimitive],
+ return_type=object_rprimitive,
+ c_function_name="PyWeakref_NewRef",
Can you also support calls with a single argument, since this is probably
quite common?
You'd need to add another function_op with arg_types=[object_rprimitive],
and probably something like extra_int_constants=[(0, pointer_rprimitive)],
to add an implicit NULL argument.
—
Reply to this email directly, view it on GitHub
<#19099 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ3HIHXND656N5PHKHNLJZ33BRZVZAVCNFSM6AAAAAB5JUBQO2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQOBZGEYDCMZSGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@JukkaL I know I said I'd do it later this week, but I've already finished. This is ready for review. |
You can look at how it's defined in |
|
hmm. It shows |
|
It didn't work. I'll keep working on proxy in #19217 , this one is ready to merge if you agree |
|
I got it to work for weakref.proxy on #19217. The C code generates correctly but I believe I've exposed a bug in mypyc's reference counting as my new pytest cases fail both with the new code and with the original builtin. The weakly-proxied object is being destroyed too early, while there should still be a strong reference to it. |
|
@JukkaL Any other changes you want to see to this one before merging? Thanks for your assistance helping me figure out the above. |
|
uh-oh, the tests started to fail after I merged in recent changes to master. Looks like there was a regression of some sort. |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Great, I got it working! Merging this will resolve mypyc/mypyc#1116 |
|
@ilevkivskyi Could I trouble you for a review on this one as well? My other weakref PRs build on top of this and would be much easier to finish up / review once this one is merged in |
ilevkivskyi
left a comment
There was a problem hiding this comment.
LG, but you should rewrite the run test.
|
fixed |
This PR adds a new mypyc primitive for
weakref.refI wasn't able to figure out what name mypyc expects for
weakref.proxy, so I took that out and will keep that for a separate PR later on. ref is more commonly used than proxy anyway.for later, I tried:
no luck with those
also for later, I'll need to finish #19145 to add a primitive for
weakref.ref.__call__