FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Own generated native closures and retain their method definitions by 1ndahous3 · Pull Request #8962 · RustPython/RustPython · GitHub

  1. RustPython
  2. RustPython

Repository navigation

Own generated native closures and retain their method definitions - #8962

Open
1ndahous3 wants to merge 1 commit into
RustPython:mainfrom
1ndahous3:native_function_owner
ConversationCommits (1)ChecksFiles changed
Open

Own generated native closures and retain their method definitions#8962
1ndahous3 wants to merge 1 commit into
RustPython:mainfrom
1ndahous3:native_function_owner

Conversation

1ndahous3 commented Oct 4, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Release dynamically generated native closures with their method definitions instead of leaking each allocation.
  • Keep method definitions alive through descriptor binding and GC clearing, so bound methods retain their callable storage.
  • Tie callable borrows to the function's lifetime and prevent descriptors from exposing an unowned static method definition.

Extracted from #8944 as an independent fix.

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Native methods and descriptors now retain captured state for as long as it is needed, including after the original definition is dropped.
    • Captured state is released when no longer referenced, improving memory cleanup and preventing premature loss of state.
    • Bound native methods now preserve their owning definition, improving reliability when methods are accessed through descriptors.

chatgpt-codex-connector Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

coderabbitai Bot commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used 📚 Code guidelines (1) 
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration
  • Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0279fb5f-d5ae-43fc-8eca-561542194cb0
📥 Commits

Reviewing files that changed from the base of the PR and between 7536730 and a997f2b.

📒 Files selected for processing (4)
  • crates/vm/src/builtins/builtin_func.rs
  • crates/vm/src/builtins/descriptor.rs
  • crates/vm/src/function/method.rs
  • crates/vm/src/vm/context.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

Native method definitions now own boxed native functions. Native functions and descriptors use manual GC traversal, and descriptor binding carries the method-definition owner into bound native methods.

Changes

Native method lifetime

Layer / File(s) Summary
Owned method definitions
crates/vm/src/function/method.rs, crates/vm/src/vm/context.rs		 HeapMethodDef can own a boxed native function. Context::new_method_def uses the owning constructor.
Native function traversal
crates/vm/src/builtins/builtin_func.rs		 PyNativeFunction and PyNativeMethod manually traverse references. as_func returns a reference tied to self.
Descriptor ownership and binding
crates/vm/src/builtins/descriptor.rs, crates/vm/src/function/method.rs		 Descriptors manually traverse the method-definition owner and use shared binding logic to pass it to bound native methods. A test checks that captured native state remains available while functions and bound methods are referenced, and is released when those references are dropped.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to a997f

The ownership paths inspected remain callable through binding and clearing. No actionable issue remains before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a997f

This change replaces leaked resources with explicit lifetime management while preserving existing access checks. No introduced security defect was established, but cleanup under concurrent or reentrant execution has not been validated end to end.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The security-relevant scope is in-process native-callable memory safety and resource lifetime. Python callers can invoke existing registered callbacks, but the changed paths do not establish a new capability to select arbitrary native pointers or acquire additional callback authority.

Trust Boundaries and Controls

  • observed — Existing receiver checks before direct and vectorcall dispatch, and type/subtype checks before classmethod binding, remain unchanged. Existing static/class-method exemptions predate this PR; owner propagation changes storage lifetime, not those validation rules.

Resilience and Maintainability Implications

  • observed — The collector subtracts only references into its active candidate set. Before clearing, it untracks dead objects and preserves late-resurrected objects and their reachable dependents. Clearing and final drops run within deferred-drop handling, while callable clearing retains the borrowed storage's owner.

Hardening Proposals

  • proposed — Extend lifetime regression coverage with forced GC, repeated binding, reentrant capture destruction, and concurrent resurrection where threading is enabled. Assert that callable storage remains valid through clearing and is released only after the final owner disappears.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main changes: owning generated native closures and retaining their method definitions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

Back | FazBrowse Home | New Git URL