FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add a pure-Rust _zstd module by youknowone · Pull Request #8917 · RustPython/RustPython · GitHub

Repository navigation

Add a pure-Rust _zstd module - #8917

Draft
youknowone wants to merge 22 commits into
RustPython:mainfrom
youknowone:zstd
Draft

youknowone wants to merge 22 commits into
RustPython:mainfrom
youknowone:zstd

Conversation

youknowone commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

One of checkbox below must be checked.

  • I did not use AI to write the code of this patch.
  • This PR follows our AI policy

Summary

Add _zstd, the accelerator behind compression.zstd, using the pure-Rust crate rusty_zstd 0.2.5. The module is registered on every target, including Android and wasm.

ZstdCompressor, ZstdDecompressor, and ZstdDict cover streaming and one-shot compression, decompression with max_length, frame headers, pledged content size, dictionaries (digested, undigested, and prefix), and train_dict / finalize_dict.

Checked locally with a release rustpython:

  • test_zstd: 119 tests, SUCCESS
  • extra_tests/snippets/stdlib_zstd.py
  • cargo clippy -p rustpython-stdlib --release -Dwarnings, with and without threading

The workspace clippy feature set and an i686 build were not run here. The 32-bit parameter bounds are selected with target_pointer_width.

c5962fbd98 was written with Grok 4.7 (Assisted-by: Grok:grok-4.7). Earlier commits keep their original trailers.

— commented by Grok

JamesClarke7283 and others added 21 commits October 1, 2026 12:59
- Implement ZstdDict, ZstdCompressor, and ZstdDecompressor on top of the zstd-safe crate
- Wire the module into stdlib_module_defs (gated off Android/wasm32)
- Mark test_zstd_multithread_compress as expected failure when libzstd lacks multi-threading support
`PyMutex` is built on `RawCellMutex` on single-threaded targets (iOS,
Android, wasm32) and is not `Sync`, so a `static PyMutex<...>` fails to
compile. `PyTypeRef` is also not `Send` by default, which compounds the
issue.

Replace the static parameter-type registry with class-name comparison
(`"CompressionParameter"` / `"DecompressionParameter"`). The two names
originate from the `compression.zstd` module we ship, so identity vs.
name comparison is equivalent in practice. `set_parameter_types` now
just validates that its arguments are type objects.

Verified locally:
- `cargo build -p rustpython-stdlib` (default features): clean.
- `cargo check -p rustpython-stdlib --no-default-features --features host_env`
  (simulates iOS by dropping the `threading` feature): clean.
- `cargo run -- -m test test_zstd`: 119 tests pass, 1 expected failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RustPython's clippy lint set rejects `std::` imports for items that also
live in `core::` or `alloc::`. Switch every such reference in `zstd.rs`:

- `std::ffi::{c_int, CStr}`        -> `core::ffi::{c_int, CStr}`
- `std::fmt::*`                    -> `core::fmt::*`
- `std::slice::from_raw_parts`     -> `core::slice::from_raw_parts`
- `std::borrow::Cow`               -> `alloc::borrow::Cow`

Also clear up other clippy findings under `-D warnings`:

- Drop the redundant `obj.clone()` on the last use of `obj` in
  `parse_zstd_dict_arg`.
- Collapse `.map(|n| n.get()).unwrap_or(0)` to `.map_or(0, |n| n.get())`
  in the two dict-id readers.
- Replace `&*work_data` with `&work_data` (auto-deref).
- Factor the `(Option<Digested>, Option<PyRef<ZstdDict>>)` return shape
  into a `DictLoadResult<D>` type alias to satisfy `type_complexity`.
- Replace the remaining `std::mem::transmute<u32, ZSTD_cParameter>` /
  `ZSTD_dParameter` in `get_param_bounds` with the existing safe
  `c_param_enum` / `d_param_enum` helpers; surfaces a clear "invalid
  parameter" `ValueError` instead of relying on UB-adjacent transmutes
  for unknown ints.

Verified locally:
- `cargo clippy -p rustpython-stdlib --no-deps -- -Dwarnings`: clean
  (only the pre-existing `socket.rs::sock_wait` warning remains).
- `cargo run -- -m test test_zstd`: 119 pass, 1 expected failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous run's `Lint` job failed with a `401 Unauthorized` from
reviewdog hitting the GitHub API (a transient CI/permissions issue,
unrelated to this PR's diff). Pushing an empty commit to re-run CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
{"subject": "Format zstd module and extend spell-check ignores", "body": "- Apply rustfmt formatting to zstd.rs\n- Add additional spell-checker ignore entries for zstd identifiers"}
Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com>
{"subject": "Remove rustdoc comments from zstd module", "body": "- Strip doc comments from internal items in _zstd module\n- Convert a few API-doc comments into regular implementation notes where appropriate"}
{"subject": "Mark zstd load_dict as unsafe and document invariant", "body": "- Add # Safety docs requiring PyRef<ZstdDict> to outlive the context\n- Wrap calls in load_compressor_dict and load_decompressor_dict with SAFETY comments"}
{
  "subject": "refactor(zstd): release GIL during (de)compress and tighten dict-load safety",
  "body": "- Wrap compress/decompress loops in vm.allow_threads to release the GIL\n- Replace load_*_dict helpers with build_*_state that assemble the full state, making the load_dict safety invariants structural\n- Switch constructor args from OptionalArg to OptionalOption and use try_to_value instead of the pyobj_to_i32/arg_or_none helpers\n- Add check_sample_sizes_match with overflow-safe summing for train_dict/finalize_dict"
}
- Have set_parameter_types stash the CompressionParameter/DecompressionParameter classes as private _zstd module attributes instead of validating and discarding them
- check_wrong_param_kind now compares key classes by identity against the registered type, matching CPython's Py_TYPE check, and skips when unregistered
- Error message names the actual key type as an attribute
- Raise TypeError (not RuntimeError), matching CPython, when both
  `level` and `options` are passed to ZstdCompressor
- Parse set_pledged_input_size's argument before taking the state lock
  so a re-entrant __index__ can't deadlock, and fix the off-by-one in
  its bound message to match CPython

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the remaining `// ===...` section separators from zstd.rs per
the project comment guidelines, drop the unused `zstd` crate
dependency (only zstd-safe/zstd-sys are used) and declare zstd-safe's
std feature explicitly instead of relying on the zstd crate's feature
unification, and convert a doc comment on #[extend_class] to a regular
comment.
Replace the C-library bindings (zstd-safe/zstd-sys) with Trifecta Tech
Foundation's libzstd-rs-sys (pinned git rev), a c2rust translation of
upstream libzstd. The module now calls its C-API-compatible functions
through small owning RAII wrappers (CCtx/DCtx/CDict/DDict) with the
same drop-order invariants as before.

The new backend builds zstdmt_compress in, so nb_workers bounds are
(0, 256) and test_zstd_multithread_compress passes for real; drop the
now-obsolete expectedFailureIf marker.
The zero-cap probe iteration handed libzstd a 1-byte buffer and the
post-loop truncation discarded whatever it emitted, while the input
counter had already advanced past the bytes that produced it — every
decompress(..., max_length=0) call with pending output silently dropped
one byte. The suite never caught it because max_length=0 was only
exercised against zero-output (skippable) frames.

Match CPython's mechanism instead: probe with a zero-size output
buffer, so libzstd consumes input without emitting and never reports
frame completion with un-emitted content. Add a regression snippet
covering lossless zero-cap probes (content, truncated, and skippable
frames), verified byte-exact against CPython.
- Derive the compression/decompression parameter ids from libzstd's own
  `ZSTD_cParameter`/`ZSTD_dParameter` constants instead of restating them
  as literals. The crate declares both as `#[repr(transparent)]` newtypes
  over a private `u32` with no accessor, `From` impl or `Deref`, so the id
  is read back through that guaranteed layout.
- Drop the redundant `name = ` on `#[pyattr(once)] fn zstd_version` and
  `fn zstd_version_number`; the function names already match.
- Use `CStr::to_str` for the version string. libzstd returns a fixed ASCII
  `MAJOR.MINOR.RELEASE` literal, so there is nothing for a lossy
  conversion to repair.
- Replace the `DICT_TYPE_*` int constants with a `DictType` enum carrying
  the pinned discriminants, decoded at the Python boundary by
  `DictType::from_marker`.
- Explain the non-trivial assertions: why `level_bounds`' `expect` cannot
  fire, and why a NULL context allocation panics rather than raising
  `MemoryError`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`COMP_MODE_CONTINUE`/`FLUSH_BLOCK`/`FLUSH_FRAME` become a `CompressMode`
enum with pinned discriminants, decoded at the Python boundary by
`CompressMode::from_int` and mapped to libzstd by
`CompressMode::end_directive`. `CompressorState::last_mode` now holds the
enum, so the states `set_pledged_input_size` accepts are checked by the
type system rather than by comparing ints.

`flush()` keeps rejecting `CONTINUE` and out-of-range modes with its own
message rather than the one `compress()` uses, so the Python-visible
errors are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified each claim the previous two commits added, against the pinned
libzstd-rs-sys rev, CPython's `Modules/_zstd/` and `Lib/test/test_zstd.py`,
and corrected the ones that did not hold:

- The `DictType` discriminants come from CPython's `dictionary_type` enum,
  not a type named `DictType`, and `test_zstd` never hand-builds a valid
  `(zdict, marker)` tuple — it pins the accepted range from the outside
  with `(zd, -1)` and `(zd, 3)` rejection cases.
- `#[repr(transparent)]` guarantees layout, not validity invariants. The
  transmute is sound because every bit pattern is a valid `u32`, which
  holds only in the enum-to-int direction; say so, and note that this is
  why `c_param_enum` decodes untrusted ids with an explicit match.
- The id is unreachable through the crate's public API only in the sense
  that no accessor exists; the derived `Debug` does print it.
- Undigested dictionary loading is not uniformly lazy:
  `ZSTD_DCtx_loadDictionary` digests eagerly and rejects corrupted content
  at construction time, while `ZSTD_CCtx_loadDictionary` accepts it.
- `CompressMode`'s discriminants are libzstd's `ZSTD_e_continue`/
  `ZSTD_e_flush`/`ZSTD_e_end` values.

`CCtx::create`/`DCtx::create` now return `PyResult` and raise `MemoryError`
like CPython's `_zstd` does, rather than panicking. The old justification
for the panic claimed a `vm` reference would have to be threaded through
every construction site; there are two, and both already had one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by reviewing the module against CPython's `Modules/_zstd/` and the
pinned libzstd-rs-sys sources; each item below was reproduced before the
fix and re-checked after.

- `train_dict`/`finalize_dict` sized their output buffer with an
  infallible `Vec::with_capacity`/`vec![]` from a caller-supplied
  `dict_size`, so `train_dict([b'x'], 2**62)` aborted the interpreter
  instead of raising. Reserve fallibly and raise `MemoryError`, as CPython
  does.
- A digested dictionary was always built at `ZSTD_CLEVEL_DEFAULT`. Since
  `ZSTD_CCtx_refCDict` takes its parameters from the CDict, that silently
  discarded the requested level: `level=19` with `as_digested_dict`
  compressed a test corpus to 39817 bytes instead of 31951. Build the
  CDict at the compressor's effective level, tracked through `level=` and
  the `options` dict like CPython's `self->compression_level`.
- A bare `ZstdDict` was loaded as digested in both directions; CPython
  compresses with an undigested dictionary by default and only
  decompresses with a digested one. Give `DictLoader` a per-direction
  default.
- Neither compressor nor decompressor reset its session after an error,
  leaving the object permanently broken — every later call failed with
  "Operation not authorized at current processing stage" or re-reported
  the original corruption on valid input. Reset as CPython's error paths
  do, including restoring `last_mode` to `FLUSH_FRAME`.
- `apply_options` range-checked every parameter, rejecting values libzstd
  and CPython accept: 0 ("use the default") for `window_log`, `strategy`,
  `window_log_max` and friends, any non-zero value for the boolean flags,
  and the clamped `nb_workers`/`job_size`/`overlap_log`. Only
  `compression_level` needs the check — libzstd clamps that one silently,
  which is why `test_compress_parameters` requires a `ValueError` — so
  everything else now goes to libzstd, whose error code already maps to
  the same message.
- `ZstdDict` accepted content shorter than 8 bytes; CPython rejects it up
  front regardless of `is_raw`.
- `compress`/`decompress` refused `data` as a keyword and
  `get_param_bounds` required `is_compress` as one, both contrary to
  CPython's clinic signatures.
- The decompressor allocated a full 128 KiB scratch buffer even when
  `max_length` capped output far below that.

`test_zstd` (119 tests) and the snippet test pass; clippy is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The branch dropped the `skipIf(not SUPPORT_MULTITHREADING)` guard from
`test_zstd_multithread_compress`. That guard can never fire against this
backend: libzstd-rs-sys compiles multi-threaded compression in
unconditionally — its Cargo.toml has no `zstdmt`-style feature — so
`CompressionParameter.nb_workers.bounds()` is never `(0, 0)` and
`SUPPORT_MULTITHREADING` is always true.

Removing the decorator therefore changed nothing except to put a
modification of a vendored CPython test in the diff, and it would turn
into a hard failure rather than a skip if a future backend ever lacked
multi-threading. Restore the file so the PR touches no CPython test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop libzstd-rs-sys and the platform cfg that kept _zstd off Android
and wasm. The module now calls rusty_zstd for streaming compression,
decompression, frame headers, and dictionary training.

Assisted-by: Grok:grok-4.7

coderabbitai Bot commented Oct 1, 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
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

Replace rusty_zstd 0.2.5 with libzstd-rs-sys rev 7afaf79f14f4.
Streaming compression, decompression, frame queries, and dictionary
training go through that crate. zstd_version is 1.5.8.

Assisted-by: Grok:grok-4.7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL