Skip to content

[mypyc] Fix memory leak on property setter call #21095

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

Merged
p-sawicki merged 9 commits into from
Mar 24, 2026
+181 −6
Merged

[mypyc] Fix memory leak on property setter call #21095

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f1b5d67
Add decref
p-sawicki Mar 23, 2026
6482508
Update test
p-sawicki Mar 23, 2026
bea3c66
Add comment
p-sawicki Mar 23, 2026
e548a82
Check if passed value is refcounted
p-sawicki Mar 23, 2026
52d4a58
Don't mark prop set source as stolen in SetAttr
p-sawicki Mar 24, 2026
1990b6d
Disable test on 32-bit python
p-sawicki Mar 24, 2026
26bd874
Use is_propset in emitfunc
p-sawicki Mar 24, 2026
d8e8a11
Fix determination of propset in multimodule compilation
p-sawicki Mar 24, 2026
b73f03a
Use is_propset in emitfunc again
p-sawicki Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Check if passed value is refcounted
  • Loading branch information
@p-sawicki
p-sawicki committed Mar 23, 2026
commit e548a8208c8eba2393b49adcbb0842c191a7323b
4 changes: 3 additions & 1 deletion mypyc/codegen/emitfunc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,9 @@ def visit_set_attr(self, op: SetAttr) -> None:
)
# The property setter method increfs the passed value (src) so we need to decref it here
# to avoid leaking.
self.emitter.emit_dec_ref(src, op.src.type)
src_rtype = op.src.type
if src_rtype.is_refcounted:
self.emitter.emit_dec_ref(src, src_rtype)
Copy link
Copy Markdown
Collaborator

@JukkaL JukkaL Mar 24, 2026

Choose a reason for hiding this comment

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

A potentially cleaner approach would be to adjust stolen of SetAttr for properties -- i.e. don't steal the value if it's a property. Thoughts?

Copy link
Copy Markdown
Collaborator Author

@p-sawicki p-sawicki Mar 24, 2026

Choose a reason for hiding this comment

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

ah good call, this way the decref is visible in the IR which makes for better tests.

i've removed the leak test from the run test as it seemed a little flaky when running it multiple times in a row.

Copy link
Copy Markdown
Collaborator Author

@p-sawicki p-sawicki Mar 24, 2026

Choose a reason for hiding this comment

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

looks like the failures were actually an implementation mistake - using ClassIR.methods during IR generation may not work in multimodule compilation because the SetAttr constructor might run before the property setter method is added in the other module.

i've changed it to use ClassIR.method_decls instead which is populated in prepare.py so it's safe to use.

else:
# ...and struct access for normal attributes.
attr_expr = self.get_attr_expr(obj, op, decl_cl)
Expand Down
Loading