Fix 32-bit member layout assertions and address member descriptor review by youknowone · Pull Request #8882 · RustPython/RustPython

youknowone · 2026-09-28T13:30:33Z

Follow-up to #8857. This is the part of #8873 that does not depend on bitflagset; #8873 keeps only the bitflagset switch.

Changes

Fix i686 cargo check (it failed on Expose more attributes as member descriptors #8857):
- The tp_flags layout assertion compared AtomicU64 alignment with u64 alignment, which differ on i686. __flags__ is read as an AtomicU64, so only that layout is asserted now.
- The select.kevent assertion required usize/isize/c_ulong/c_long to be 64-bit. It now only requires that pointer-sized values fit into the 64-bit cells.
Review follow-ups on member descriptors
- Internal PY_ATOMIC flag on PyMemberDef: a member is loaded atomically only when its storage is atomic. C extension members never set it.
- _elementtree: new state, entity and target are published only after every handler lookup succeeds.
- genericalias: type names are computed only on the error path.
- unraisable hook: prints <unknown> without a . when __module__ is missing or not a str.
- builtin_function_or_method.__reduce__ looks only at the bound object, like meth_reduce.

Test

fmt; clippy (default and CI feature set); wasm32-wasip1 check
cargo check on i686-unknown-linux-gnu: vm, and the full feature set except sqlite (whose C build needs a sysroot locally)
workspace tests, crates/capi tests
test_types test_descr test_class test_builtin test_xml_etree test_genericalias test_sys test_pickle

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Corrected how native functions bound to non-module objects are represented when pickled.
- Parser initialization now preserves its existing configuration if new handler setup fails.
- Improved errors for invalid type-variable substitutions and made unraisable-exception output more robust when module information is unavailable.
- Corrected access to atomic and read-only member fields to use appropriate loading behavior.
- Improved compatibility with extension-defined member flags by preserving unrecognized flag values.

Member definitions carry an internal `PY_ATOMIC` flag taken from `MemberLayout::ATOMIC`, and loads use an atomic access only for atomic storage; extension members never set it. `XMLParser` publishes its new state, `entity` and `target` only after every handler lookup succeeds. `subs_parameters` reads type names only for the error. The unraisable hook prints `<unknown>` when `__module__` fails or is not a str. The builtin function `__reduce__` decides on the bound object alone. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5

The tp_flags assertion compared AtomicU64 with u64 alignment, which differs on i686. The kevent assertion required usize, isize, c_ulong and c_long to be 64-bit. Both now check only what the member cells need. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5

coderabbitai · 2026-09-28T13:31:06Z

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

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: 3758cf48-be8d-4355-b7a9-08b1490de30b

📥 Commits

Reviewing files that changed from the base of the PR and between 7c41cb9 and 7ace8d0.

📒 Files selected for processing (7)

crates/capi/src/descrobject.rs
crates/derive-impl/src/pyclass.rs
crates/vm/src/builtins/descriptor.rs
crates/vm/src/builtins/mod.rs
crates/vm/src/builtins/type.rs
crates/vm/src/types/slot.rs
crates/vm/src/vm/context.rs

🚧 Files skipped from review as they are similar to previous changes (1)

crates/vm/src/types/slot.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

This pull request adds typed member flags and atomic storage selection for member descriptors. It also changes XML parser initialization, native function reduction, TypeVarTuple substitution errors, unraisable-hook formatting, and pointer-width checks.

Changes

Atomic member storage

Layer / File(s)	Summary
Atomic storage contract and layouts `crates/vm/src/builtins/descriptor.rs`, `crates/vm/src/types/slot.rs`, `crates/stdlib/src/select.rs`	Member layouts identify atomic storage. Compile-time checks permit pointer-sized integers narrower than the atomic storage widths.
Typed flag generation and member definitions `crates/derive-impl/src/pyclass.rs`, `crates/capi/src/descrobject.rs`, `crates/vm/src/builtins/mod.rs`, `crates/vm/src/builtins/type.rs`, `crates/vm/src/vm/context.rs`, `crates/vm/src/builtins/descriptor.rs`	Generated members combine typed read-only and audit-read flags with atomic metadata. Extension member construction retains unknown flag bits and clears internal flags. Member construction uses `PyMemberFlags`.
Atomic member load selection `crates/vm/src/builtins/descriptor.rs`	Member getters use atomic loads for atomic or writable storage and plain reads otherwise.

XMLParser initialization

Layer / File(s)	Summary
Handler resolution and reference update `crates/stdlib/src/elementtree.rs`	`XMLParser.init` resolves target handlers before installing new state and references. Failed lookup leaves the previous entity and target in place.

Native function reduction

Layer / File(s)	Summary
Bound function reduction `crates/vm/src/builtins/builtin_func.rs`	`__reduce__` returns a `getattr` tuple for functions bound to non-module objects. Unbound and module-bound functions return the function name.

TypeVarTuple substitution errors

Layer / File(s)	Summary
Substitution result validation `crates/vm/src/builtins/genericalias.rs`	Unpacked substitutions check whether the result is a tuple before resolving type names for an error.

Unraisable hook formatting

Layer / File(s)	Summary
Exception module handling `crates/vm/src/stdlib/sys.rs`	`_unraisablehook` treats failed, missing, and non-string `__module__` values as unknown and formats them without a trailing period.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: bschoenmaeckers, shaharnaveh

Merge Risk: ⚪ Minimal · up to 7ace8

No actionable issue is established that would prevent merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 7ace8

The descriptor changes warrant design review because they affect extension-facing behavior and concurrent access. The reviewed paths preserve the relevant access controls, and no introduced security issue was established. Some affected behavior remains only partially covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The directly affected boundary is between extension-supplied member definitions and VM descriptor storage access; incorrect storage metadata could affect objects exposed through those descriptors. The inspected construction path does not let extension flags assert internal atomic storage.

Trust Boundaries and Controls

observed — The C-API builder clears the internal ATOMIC flag supplied in extension flags; the VM separately determines atomic access for writable members.
observed — Audit-before-read and readonly-before-write checks remain at descriptor access time, rather than relying on the caller that constructed the descriptor.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 12 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 identifies the 32-bit layout assertion fixes and the member descriptor review changes, which are central parts of the pull request.
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

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

PY_READONLY, PY_AUDIT_READ, PY_RELATIVE_OFFSET and PY_ATOMIC become the repr(transparent) PyMemberFlags. C extension flags are read with from_bits_retain. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5

youknowone added 2 commits September 28, 2026 22:04

youknowone mentioned this pull request Sep 28, 2026

Use bitflagset for type flags #8873

Merged

Use bitflags for member definition flags

7ace8d0

PY_READONLY, PY_AUDIT_READ, PY_RELATIVE_OFFSET and PY_ATOMIC become the repr(transparent) PyMemberFlags. C extension flags are read with from_bits_retain. Assisted-by: Grok:4.7 Assisted-by: Claude:claude-opus-5-5

youknowone merged commit 8f84fea into RustPython:main Sep 28, 2026
29 checks passed

youknowone deleted the member-review-fixes branch September 28, 2026 16:59

Navigation Menu

Repository navigation

Fix 32-bit member layout assertions and address member descriptor review - #8882

Fix 32-bit member layout assertions and address member descriptor review#8882
youknowone merged 3 commits into
RustPython:mainfrom
youknowone:member-review-fixes

youknowone commented Sep 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 28, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Sep 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Sep 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 28, 2026 •

edited

Loading