Skip to content

Fix marshal recursive reference loading - #8501

Draft
youknowone wants to merge 3 commits into
RustPython:mainfrom
youknowone:fix/marshal-recursive-references
Draft

Fix marshal recursive reference loading#8501
youknowone wants to merge 3 commits into
RustPython:mainfrom
youknowone:fix/marshal-recursive-references

Conversation

@youknowone

@youknowone youknowone commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary

Create reference-tracked containers before unmarshalling their children, matching CPython's r_object() construction order. This restores recursive list and dict graphs as well as indirect tuple cycles.

The compiler-core decoder exposes optional placeholder/fill hooks, while the VM bag supplies RustPython object construction. Tuple storage gains an initialization-only mutation path corresponding to PyTuple_New followed by PyTuple_SET_ITEM; tuples remain immutable after decoding. Interned marshal string markers are also preserved through the runtime bag.

Python exceptions raised while inserting unmarshaled set and dictionary members are retained in a call-local pending-error slot. Abnormal self-referential hash containers therefore raise TypeError like CPython instead of being collapsed into a generic ValueError; immutable reference loops continue to raise ValueError. This also removes the remaining test_loads_abnormal_reference_loops expected failure and replaces insertion-path unwrap() calls with ordinary error propagation.

AI assistance disclosure: Codex (gpt-5) assisted with implementation, test execution, and drafting this pull request. The changes were exercised with the RustPython interpreter and full project test commands listed below.

Testing

  • cargo fmt --check
  • prek run --all-files
  • cargo run --release -- -m test test_marshal
  • Direct CPython 3.14-compatible exception checks for malformed recursive dict, set, slice, and frozenset streams
  • cargo test -p rustpython-compiler-core -p rustpython-vm
  • cargo clippy -p rustpython-compiler-core -p rustpython-vm --all-targets
  • cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi
  • (cd crates/capi && cargo test)
  • Consumed the patched compiler-core from Pyre via a workspace-wide Cargo source override and ran cargo check --offline -p pyre-interpreter --features dynasm.

Create reference-tracked containers before reading their children so recursive list, dict, set, and tuple graphs can be unmarshaled. Preserve interned string markers through the runtime bag and add an initialization-only tuple construction path.

Assisted-by: Codex:gpt-5
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 98c89eab-1945-4ff8-ad4a-eee8c7178de8

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

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 Aug 12, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

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

[x] test: cpython/Lib/test/test_marshal.py (TODO: 8)

dependencies:

dependent tests: (25 tests)

  • marshal: test_bool test_exceptions test_importlib test_inspect test_marshal test_zipimport
    • importlib._bootstrap_external: test_importlib test_unittest
      • modulefinder: test_importlib test_modulefinder
      • py_compile: test_argparse test_cmd_line_script test_compileall test_importlib test_multiprocessing_main_handling test_py_compile test_pydoc test_runpy
      • pydoc: test_enum
    • pkgutil: test_pkgutil test_pyrepl
    • profile: test_profile
    • pstats: test_pstats
    • zipimport: test_importlib test_zipimport_support

Legend:

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

Keep Python exceptions raised while constructing unmarshaled sets, frozensets, and dictionaries instead of collapsing them into ValueError. This makes abnormal recursive hash-container streams report TypeError like CPython and removes the remaining test_marshal expected failure.

Assisted-by: Codex:gpt-5
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.

1 participant