Skip to content

Wrap native static methods in staticmethod descriptors - #9453

Open
1ndahous3 wants to merge 2 commits into
RustPython:mainfrom
1ndahous3:native_staticmethod_descriptors
Open

1ndahous3 wants to merge 2 commits into
RustPython:mainfrom
1ndahous3:native_staticmethod_descriptors

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Found while round-tripping str.__dict__["maketrans"] with Cloudpickle.

Summary

  • Store native static methods as staticmethod descriptors in class dictionaries, exposing __func__ and __wrapped__ while preserving access through classes and instances.
  • Preserve the wrapped function's metadata. During bootstrap, defer hashing the metadata until the first interpreter chooses its hash seed.

AI assistance

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

Summary by CodeRabbit

  • Bug Fixes
    • Built-in static methods now expose the expected __module__, __name__, __qualname__, and __doc__ attributes.
    • Static methods accessed through built-in types retain their static-method behavior and function metadata.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T06:41:17.911414Z ff1d372 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters: 1
  • Lib/test/test_pickletools.py is excluded by !Lib/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between b85582d and ff1d372.

Files ignored due to path filters: 1
  • Lib/test/test_pickle.py is excluded by !Lib/**
Files selected for processing: 5
  • crates/vm/src/builtins/staticmethod.rs
  • crates/vm/src/dict_inner.rs
  • crates/vm/src/function/method.rs
  • crates/vm/src/vm/mod.rs
  • extra_tests/snippets/builtin_type.py

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

Native static methods now expose module, name, qualified name, and documentation metadata. Their dictionaries initialize immediately when the hash secret is available, or after VM hash-secret initialization otherwise.

Changes: Native static-method metadata

Layer / File(s) Summary
Metadata construction and dictionary initialization
crates/vm/src/builtins/staticmethod.rs, crates/vm/src/dict_inner.rs
PyStaticMethod::new_native prepares metadata attributes. Dict::initialize_attributes stores them using the supplied hash secret. If the secret is not yet available, the dictionary and attributes are queued for initialization later.
Method construction and VM integration
crates/vm/src/function/method.rs, crates/vm/src/vm/mod.rs, extra_tests/snippets/builtin_type.py
Static-flagged native methods use PyStaticMethod::new_native. VM hash-secret initialization triggers initialization of queued metadata. The str.maketrans test checks the descriptor, underlying function, and metadata attributes.

Priority: Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant MethodBuilder
  participant PyStaticMethod
  participant Dict
  participant VM
  MethodBuilder->>PyStaticMethod: Construct static-flagged native method
  PyStaticMethod->>Dict: Initialize metadata when hash secret is available
  PyStaticMethod->>PyStaticMethod: Queue metadata otherwise
  VM->>PyStaticMethod: Initialize queued metadata with shared hash secret
  PyStaticMethod->>Dict: Store queued metadata attributes
Loading

Possibly related PRs

  • RustPython/RustPython#9437: Changes HashSecret::hash_bytes, which affects hashes used for native static-method metadata dictionary keys.

Suggested reviewers: youknowone

Merge Risk: Minimal · previous review ff1d3

No concrete merge-blocking issue is established for the native static-method metadata change. Merge after normal build and test checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 and concisely describes the main change: wrapping native static methods in staticmethod descriptors.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 5 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · 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.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/pickle.py
[ ] lib: cpython/Lib/_compat_pickle.py
[x] test: cpython/Lib/test/picklecommon.py
[ ] test: cpython/Lib/test/test_pickle.py (TODO: 3)
[x] test: cpython/Lib/test/test_picklebuffer.py (TODO: 1)
[x] test: cpython/Lib/test/test_pickletools.py
[x] test: cpython/Lib/test/test_xpickle.py
[x] test: cpython/Lib/test/xpickle_worker.py

dependencies:

  • pickle (native: _pickle, itertools, sys)
    • _compat_pickle
    • _compat_pickle
    • io (native: _io, _thread, errno, msvcrt, sys)
    • argparse, codecs, copyreg, functools, pprint, struct, types

dependent tests: (102 tests)

  • pickle: test_annotationlib test_argparse test_array test_ast test_asyncio test_bool test_builtin test_bytes test_bz2 test_codecs test_collections test_concurrent_futures test_configparser test_coroutines test_csv test_ctypes test_decimal test_defaultdict test_deque test_descr test_dict test_dictviews test_email test_enum test_enumerate test_exceptions test_external_inspection test_fractions test_free_threading test_functools test_generators test_genericalias test_genericpath test_http_cookies test_importlib test_inspect test_interpreters test_io test_ipaddress test_iter test_itertools test_json test_list test_logging test_lzma test_memoryview test_minidom test_opcache test_operator test_ordered_dict test_os test_pathlib test_pickle test_picklebuffer test_pickletools test_platform test_plistlib test_positional_only_arg test_random test_range test_re test_set test_shelve test_slice test_socket test_statistics test_str test_string test_structseq test_super test_time test_trace test_tuple test_turtle test_type_aliases test_type_params test_types test_typing test_unittest test_urlparse test_uuid test_xml_dom_minicompat test_xml_etree test_xpickle test_zipfile test_zlib test_zoneinfo
    • logging: test_asyncio test_concurrent_futures test_hashlib test_pkgutil test_support test_urllib2net
      • asyncio.futures: test_asyncio
      • concurrent.futures._base: test_concurrent_futures
      • http.cookiejar: test_http_cookiejar test_urllib2
      • multiprocessing.util: test_asyncio test_compileall test_concurrent_futures
      • venv: test_venv
    • tracemalloc: test_tracemalloc

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff1d372ea8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread crates/vm/src/function/method.rs
Comment thread Lib/test/test_pickle.py
Comment thread crates/vm/src/builtins/staticmethod.rs
@codspeed

codspeed Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks1


Comparing 1ndahous3:native_staticmethod_descriptors (59200e2) with main (eccb9ad)2

Open in CodSpeed

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (8eee162) during the generation of this report, so eccb9ad was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

// Context::genesis builds native descriptors before the first interpreter
// chooses its hash seed. Delay hashing their metadata until that seed exists.
rustpython_common::static_cell! {
static NATIVE_METADATA: PyMutex<NativeMetadata>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's check if this is really necessary or not

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.

2 participants