Skip to content

Share code construction and comparison controls - #9713

Draft
youknowdot wants to merge 5 commits into
RustPython:mainfrom
youknowdot:fix/code-controls-20261104
Draft

youknowdot wants to merge 5 commits into
RustPython:mainfrom
youknowdot:fix/code-controls-20261104

Conversation

@youknowdot

@youknowdot youknowdot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Align code-object construction, replacement, comparison and hashing controls with Python 3.15 through shared validation and existing binding primitives in two Rust production files.

  • Compare constant keys before names and tables, releasing temporary keys left-to-right on both success and error.
  • Compare retained Python name/local-plus/table objects through their protocols, preserving tuple/bytes-subclass callbacks instead of comparing normalized internal arrays.
  • Hash the retained objects in CPython's order, preserving callback errors and the cached hash of the original constants tuple. Remove the local-partition count: code objects with identical combined locals-plus names can compare equal despite different local/cell partitions.
  • Share CodeUnits' existing atomic deoptimization/cache traversal between byte serialization and hashing. Exclude inline caches and adaptive counters; hold the monitoring lock only after Python hash callbacks finish.

Validation

Compared against pinned CPython d220409a13b77de7a6c30abe16ea90db01528edc codeobject.c. The original four reference groups and nine expanded groups pass, covering callback order, early false/error exits, retained-field identity, local/cell partitions, original tuple hash caching and stable hashing after ordinary specialization. Constructed or modified code objects were not executed.

Independent source review passed, including bounds/traversal preservation, atomic reads, monitoring-lock scope and equality/hash consistency. Final threaded cargo check passed (1m04s), all-target Clippy passed (2m17s) with exactly the 12 unchanged baseline diagnostics, and normal commit hooks passed.

Limits and integration

The initial validation used external reference checks and source review. Current native validation is recorded in the shared-binding follow-up section; hosted CI remains necessary. No repository tests, markers, Lib files or workflows are changed.

The goal includes observable protocol callbacks and equality/hash consistency, not a promise of identical numerical hashes across distinct interpreter implementations. Signed constructor first-line representation differences remain outside this patch. #9688's filename repr work and #8939's constant-borrow ownership work remain separate and should be preserved on composition.

Integration with #9439 (head 95a4916) requires explicit reconciliation, not a clean automatic composition: retain that PR's removal of per-code monitoring data and its original_bytes-based deopt_code; omit this patch's extracted monitoring resolver and lock; hash through original_instructions_with with identity resolution. Its bounded base-instruction analysis helper has different stop semantics and should not replace serialization/hash traversal blindly. The retained-object comparison/hash controls are not duplicated by #9439.

Shared construction and replacement controls

  • Share signed C-int conversion, original argument retention, audit-event construction, structural validation, name interning and CodeObject assembly. Derived signatures, KwArgs and the shared Clinic validation in bind_for handle binding. The owner-retaining wrapper forwards binding metadata without a separate keyword parser or nested bind.
  • Accept the constructor's optional freevars/cellvars tuples and positional-only signature. Convert replacement controls in declaration order and reject iterable substitutes for tuple fields without calling their iteration hooks.
  • Emit the nine-value code.new audit event with original object identities and propagate vetoes at the constructor/replacement-specific phase. Retain numeric arguments through later callbacks. Snapshot the replacement's default filename before index conversion callbacks can mutate it.
  • Keep constructor name normalization separate from replacement's exact-string validation, matching their distinct error precedence. Share argument/local-capacity validation and retain positive unknown flag bits. Preserve the original comparison/hash changes.

The constructor's negative firstlineno still uses the existing unsigned internal line representation. In-place interning of externally referenced tuples, constant deduplication and exact interned filename/name identity, malformed bytecode parity, generic binder TypeError wording, and broader locals/line representation changes remain outside scope. Existing safe bytecode checks and location-array behavior are retained; no constructed or replaced code is executed in reference probes. #6894's old locals representation redesign is not duplicated, but would require source reconciliation if revived.

The current group changes 887 lines (+511/-376) in two production Rust files. Constructor-specific name diagnostics use native type names and the verified UTF-8-safe 500-byte bound. The added validation includes 103 focused and 103 independent pinned CPython cases, covering audit identity/veto/order, conversion and destruction order, structural errors, tuple subclass controls, mutable filename snapshots, flag retention and diagnostic bounds. Independent source review also confirmed that the original comparison/hash section is unchanged. These are reference checks and source review, not a claim that all oracle wording passes verbatim on RustPython.

Final follow-up validation: threaded host-env/VM/stdlib cargo check passed in 1m02s and all-targets Clippy in 2m20s, with exactly the 12 unchanged baseline diagnostics. The first compile exposed three borrowed-reference ownership conversions; those and two new lint findings were corrected, independently reviewed, and both final commands rerun. Normal commit hooks passed. Those earlier runs did not include full workspace, C-API or native runtime tests. Current results follow; separate WASM/non-threaded graphs remain unexecuted.

Shared-binding follow-up

Rebased independently onto main at fb57e68 and validated commit 886c794. The four existing commits are patch-identical after rebase. Later main changes through ea5fbcc affect ctypes and zlib, not the code-object or binding files used here.

  • Forward PARAMS and BINDING_PARAMS from the typed arguments so shared Clinic validation runs once before conversion. Remove the duplicate arity/keyword preflight, temporary keyword-name vector, and private specification trait.
  • Keep the original argument owners through numeric conversions and audit callbacks. Preserve the replacement filename snapshot before conversion and existing callee diagnostics.
  • Independent review and normal commit hooks passed. No tests, expected-failure markers, or unrelated production files were changed by this follow-up.

Validation on this exact commit:

  • Full workspace tests with the documented WASM, venvlauncher and C-API exclusions: 1,513 passed, 20 ignored across unit and documentation suites.
  • Full workspace all-target Clippy: passed with zero warnings. This was ordinary Clippy, without -D warnings.
  • Separate C-API all-target Clippy: passed with zero warnings. Separate C-API tests: 149 passed, 2 ignored, using CARGO_PROFILE_TEST_DEBUG=0.
  • Fresh release RustPython -m test test_code: SUCCESS, 37 tests run and 15 skipped.

The host PyO3 interpreter was the existing CPython 3.12.14, verified in the fresh build configuration. C-API checks used the repository's RustPython 3.15 configuration without an external interpreter. The pinned CPython 3.15 comparison is source-only; no CPython 3.15 executable was run for this follow-up. Hosted CI for the updated head remains pending.

AI assistance is disclosed in the commit trailers.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • 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

@youknowdot youknowdot changed the title Preserve code comparison and hashing callbacks Share code construction and comparison controls Nov 4, 2026
Comment thread crates/vm/src/builtins/code.rs Outdated
_owners: FuncArgs,
}

impl<T: CodeArgumentSpec> FromArgs for CodeArgs<T> {

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.

check if we still need this after #9563

@youknowdot
youknowdot force-pushed the fix/code-controls-20261104 branch from 4e49d67 to 4c33098 Compare November 6, 2026 14:00
youknowdot and others added 5 commits November 7, 2026 09:24
Assisted-by: OpenAI Codex:model version not exposed
Preserve argument owners, signed validation, audit and name-conversion order through shared construction. Match optional closure tuples and replacement controls without invoking tuple subclass iteration.

Assisted-by: OpenAI Codex:model version not exposed
Remove redundant pyarg name overrides by naming the FromArgs fields codestring and constants. Preserve constructor storage mapping and Python signatures.

Assisted-by: OpenAI: undisclosed
Preserve callback ownership, release ordering, and retained argument owners.

Assisted-by: Codex:model-version-unavailable
Forward the derived binding metadata and let bind_for validate once before converting. Preserve original argument owners through conversion and audit callbacks.

Assisted-by: OpenAI Codex:GPT-6
@youknowdot
youknowdot force-pushed the fix/code-controls-20261104 branch from 4c33098 to 886c794 Compare November 7, 2026 01:53
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.

3 participants