Use active builtins consistently in descriptor and method reductions by youknowdot · Pull Request #9057 · RustPython/RustPython

youknowdot · 2026-10-10T11:12:40Z

Current revision

Targets Python 3.15 main at f39da82d9d38cd5bdceb9e645205e830f65126fb with one logical commit, bf4bbf65869e47949ffb5abe5d9aeb4798e57098. The unique patch is unchanged from the prior reviewed PR head (range-diff =); current main fixes, original author attribution, and assistance trailers are preserved. No files under Lib/ are changed.

Normal commit hooks, changed-range hooks, whitespace checks, and source-equivalence review passed. Normal hosted CI is enabled for this exact head. Current-revision local validation is source and hooks only; earlier runtime results below remain attributed to their original revisions.

Summary

Follow up on merged #9037 and its original review, expanded after the same-pattern audit request.

Reduction must return the active frame's exact getattr value without calling it, and raise AttributeError('getattr') when absent. Reading the interpreter-global builtins instead changes both the reconstruction callable and the failure behavior.

Use frame-aware lookup for wrapper, method, and member descriptors, method wrappers, bound native methods, and Python bound methods.
Retain a method descriptor's actual defining type, including types such as io.BytesIO that are not attributes of builtins.
Resolve a Python bound method's __name__ before looking up getattr, preserving exception order.
Restrict the builtin helper's dictionary fast path to exact dictionaries. A subclass uses one generic lookup, without retrying __missing__ after its overridden __getitem__ raises KeyError. Existing iterator reducers share this correction.
Expand the existing Python regression table while retaining its original wrapper cases and assertion semantics.

Module-bound native functions still reduce to their names. Classmethod/getset descriptors and pickle's separately cached getattr have different contracts and are unchanged.

The same behavior is established by CPython 3.14.7 and 3.14.8. Sources: descriptor reducers, native method reducer, Python method order, and optional mapping lookup.

Earlier validation before stacking on #8950

Independent source/ownership review and normal repository hooks passed.
Native debug build passed. Exact CPython 3.14.7 and 3.14.8 and the native candidate pass the complete pickle snippet.
Bounded Python matrices match the references for eight reducer targets, custom/missing builtins, defining-type identity, non-callable reconstruction values, module-bound functions, lookup exceptions, name-before-builtin exception order, and exact mapping callback counts. Inherited __missing__ still works; custom __getitem__ is not followed by a second __missing__ call. Negative classmethod/getset controls retain their errors.
Both dedicated snippet pytest entries pass with the repository's 3.14.7 reference.
Four unchanged canonical method/iterator pickle tests pass. Both canonical C-method pickle tests retain their pre-existing expected-failure outcomes; no assertion or marker was changed.
VM and separately configured CAPI Clippy pass with -D warnings.

All new behavior assertions are Python tests.

coderabbitai · 2026-10-10T11:12:50Z

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: 0b57e803-2043-4be6-9c30-e8498e6c7c88

📥 Commits

Reviewing files that changed from the base of the PR and between f39da82 and bf4bbf6.

📒 Files selected for processing (5)

crates/vm/src/builtins/builtin_func.rs
crates/vm/src/builtins/descriptor.rs
crates/vm/src/builtins/function.rs
crates/vm/src/vm/mod.rs
extra_tests/snippets/stdlib_pickle.py

 __________________________________________________________________
< Your documentation is 'TBD'. My patience is 'TBF' (to be found). >
 ------------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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.}

youknowone · 2026-10-10T11:56:01Z

    #[pymethod]
    fn __reduce__(&self, vm: &VirtualMachine) -> PyResult {
-        let getattr = vm.builtins.get_attr("getattr", vm)?;
+        let getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;


tell me why this is important. and also this pattern is looking like need to be applied more places. if it does, patch and include here

This is observable compatibility of the reconstruction recipe. With a custom active builtins mapping, CPython returns that mapping’s exact getattr object from reduce, without calling it; if the entry is absent, reduction raises AttributeError("getattr"). Reading vm.builtins instead returns a different callable and hides that lookup failure. This is not a sandbox-security claim.

You are right about the other sites. Exact CPython 3.14.8 source and bounded reference/native checks identify the same issue in method/member descriptors, bound native methods, Python bound methods, and method wrappers. I am including those fixes here, with stored defining-type handling and name lookup order preserved, and expanding the existing parameterized Python regression. Module-bound functions still return a name. Classmethod/getset descriptors and pickle’s separately cached getattr have different contracts and are not being changed.

Included in 4944d19: all five remaining reducer sites now use active builtins; method descriptors retain their actual defining type; Python bound methods keep name lookup first. The helper now performs one generic lookup for dictionary subclasses, preserving inherited missing without an extra fallback after overridden getitem. The original wrapper assertions remain covered in the expanded table.

Exact CPython 3.14.7/3.14.8 reference matrices and the native candidate agree, including callback counts and exception order. The full pickle snippet, both snippet pytest entries, four unchanged canonical method/iterator tests, normal hooks, and VM/CAPI Clippy pass. Two pre-existing C-method pickle expected failures remain unchanged. Module-bound functions, classmethod/getset descriptors, and pickle’s cached getattr retain their distinct contracts. New-head CI is running.

codspeed · 2026-10-11T01:37:43Z

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks¹

_{Comparing youknowdot:fix-wrapper-reduce-active-builtins (bf4bbf6) with main (f39da82)}

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. ↩

github-actions · 2026-10-11T03:05:18Z

📦 Library Dependencies

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

[ ] test: cpython/Lib/test/test_mmap.py (TODO: 19)

dependencies:

dependent tests: (2 tests)

mmap: test_os
- multiprocessing.shared_memory: test_genericalias

Legend:

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

Use frame-aware getattr lookup consistently across descriptor and method reductions. Preserve defining types and exception order, and perform generic builtin-mapping subclass lookup only once. Assisted-by: OpenAI Codex:model identifier unavailable

chatgpt-codex-connector · 2026-10-12T05:49:05Z

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-12T05:52:40.385250Z	`bf4bbf6`	Draft marked ready

ℹ️ 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.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf4bbf6586

ℹ️ 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".

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

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-10-12T05:52:38Z

        assert str(caught.exception) == expected
+
+
+# Reduction returns getattr from the active frame's builtins without calling it.


Remove prohibited test-logic changes

This added regression block introduces new test data, execution logic, and assertions in a test file, while the repository permits test-file edits only for adding or removing expected-failure decorators; the test additions therefore need to be removed unless the maintainers revise that policy first.

AGENTS.md reference: AGENTS.md:L275-L279

Useful? React with 👍 / 👎.

youknowone reviewed Oct 10, 2026

View reviewed changes

youknowdot changed the title ~~Use active builtins in wrapper descriptor reduction~~ Use active builtins consistently in descriptor and method reductions Oct 10, 2026

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from 4944d19 to 4da26ee Compare October 11, 2026 00:50

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from 4da26ee to 7a320ce Compare October 11, 2026 03:01

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from 7a320ce to d9c81d9 Compare October 11, 2026 03:21

youknowone changed the base branch from main to stack/python-3.15-8950 October 11, 2026 03:26

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from d9c81d9 to 9be5575 Compare October 11, 2026 04:07

youknowone changed the base branch from stack/python-3.15-8950 to main October 11, 2026 04:08

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from 9be5575 to 9f54393 Compare October 11, 2026 04:14

youknowdot force-pushed the fix-wrapper-reduce-active-builtins branch from 9f54393 to bf4bbf6 Compare October 11, 2026 18:37

youknowone marked this pull request as ready for review October 12, 2026 05:48

youknowone merged commit 1a1605d into RustPython:main Oct 12, 2026
29 of 30 checks passed

chatgpt-codex-connector Bot reviewed Oct 12, 2026

View reviewed changes

Navigation Menu

Repository navigation

Use active builtins consistently in descriptor and method reductions - #9057

Use active builtins consistently in descriptor and method reductions#9057
youknowone merged 1 commit into
RustPython:mainfrom
youknowdot:fix-wrapper-reduce-active-builtins

youknowdot commented Oct 10, 2026 •

edited by youknowone

Loading

Uh oh!

coderabbitai Bot commented Oct 10, 2026 •

edited

Loading

Uh oh!

youknowone Oct 10, 2026

Uh oh!

youknowdot Oct 10, 2026

Uh oh!

youknowdot Oct 10, 2026

Uh oh!

codspeed Bot commented Oct 11, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 11, 2026 •

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Oct 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		assert str(caught.exception) == expected


		# Reduction returns getattr from the active frame's builtins without calling it.

Conversation

youknowdot commented Oct 10, 2026 • edited by youknowone Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current revision

Summary

Earlier validation before stacking on #8950

Uh oh!

coderabbitai Bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

youknowdot Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

youknowdot Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

github-actions Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Library Dependencies

Uh oh!

chatgpt-codex-connector Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

youknowdot commented Oct 10, 2026 •

edited by youknowone

Loading

coderabbitai Bot commented Oct 10, 2026 •

edited

Loading

codspeed Bot commented Oct 11, 2026 •

edited

Loading

github-actions Bot commented Oct 11, 2026 •

edited

Loading

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

edited

Loading