Add _zstd stdlib module backing compression.zstd - #7962
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds zstd workspace dependencies, conditionally registers the stdlib ChangesZstd Extension Module
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Python
participant ZstdModule as _zstd
participant Libzstd
Python->>ZstdModule: call compression or decompression API
ZstdModule->>Libzstd: process data, dictionaries, or parameters
Libzstd-->>ZstdModule: output or error
ZstdModule-->>Python: bytes, metadata, or exception
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_zstd.py dependencies: dependent tests: (no tests depend on zstd) Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/stdlib/src/zstd.rs`:
- Around line 58-60: Remove the decorative `//
=========================================================================`
separators in this file and replace them with concise, meaningful comments or
doc comments only where they add value (e.g., replace the separator above
"Module-level constants" with a short `// Module-level constants` or `///` doc
comment if documentation is needed); similarly remove/replace the separators at
the other noted locations (lines around 164-166, 475-477, 628-630, 1136-1138,
1425-1427) with short explanatory `//` comments or `///` doc-comments tied to
the related symbols/sections so the file uses informative comments instead of
decorative dividers.
- Around line 1659-1668: get_param_bounds() is incorrectly validating parameter
IDs by calling cparameter_from_int(args.parameter, 0, vm) /
dparameter_from_int(..., 0, ...) which causes strategy-specific value validation
to reject valid IDs; instead call cparameter_from_int(args.parameter, 0, vm) /
dparameter_from_int(args.parameter, 0, vm) only to retrieve the parameter enum
(rename to c_param_enum/d_param_enum), then pass that enum value (not a
transmuted u32) into the corresponding zstd_sys ZSTD_cParam_getBounds /
ZSTD_dParam_getBounds FFI functions; remove the unsafe transmute logic and also
delete the decorative "//
========================================================================="
separators in crates/stdlib/src/zstd.rs to match style guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f23e2155-6dfa-4c29-a3e4-7664092d9a73
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_zstd.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/zstd.rs
| // ========================================================================= | ||
| // Module-level constants | ||
| // ========================================================================= |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove decorative section-separator comments.
Please replace the // ========================================================================= separators with regular short comments or doc comments only where needed.
As per coding guidelines, "Do not add decorative section separators (e.g. // -----------, // ===, /* *** */). Use /// doc-comments or short // comments only when they add value".
Also applies to: 164-166, 475-477, 628-630, 1136-1138, 1425-1427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/stdlib/src/zstd.rs` around lines 58 - 60, Remove the decorative `//
=========================================================================`
separators in this file and replace them with concise, meaningful comments or
doc comments only where they add value (e.g., replace the separator above
"Module-level constants" with a short `// Module-level constants` or `///` doc
comment if documentation is needed); similarly remove/replace the separators at
the other noted locations (lines around 164-166, 475-477, 628-630, 1136-1138,
1425-1427) with short explanatory `//` comments or `///` doc-comments tied to
the related symbols/sections so the file uses informative comments instead of
decorative dividers.
✅ Addressed in commit 0bc36b6
There was a problem hiding this comment.
Follow-up: this thread was marked resolved earlier but the decorative // ===... separators were still in the file. They have now been removed (seven section dividers around Module-level constants, ZstdError exception, Parameter helpers, ZstdDict, ZstdCompressor, ZstdDecompressor, Module-level functions). The struct/fn definitions are self-describing without them, and cargo test is still green (119/119 in test_zstd).
There was a problem hiding this comment.
@JamesClarke7283, thanks for the confirmation and for verifying with cargo test — all seven separators removed and tests still passing sounds great!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Confirmed — they were indeed still there; thanks @fanninpm. All seven // ===... blocks are removed in bfee2be (Module-level constants, ZstdError exception, Parameter helpers, ZstdDict, ZstdCompressor, ZstdDecompressor, Module-level functions). cargo fmt --check, cargo clippy -p rustpython-stdlib, and test_zstd (119/119) are green.
|
And tysm for working on this! |
|
@JamesClarke7283 please pull from main, we've fixed the CI issue |
|
@youknowone can this be merged? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/stdlib/src/zstd.rs (1)
52-54: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDecorative section separators still present.
The
// ===...separators flagged in a previous review are still in the file (lines 52-54, 151-153, 176-178, 454-456, 570-572, 1081-1083, 1335-1337). Please remove them.As per coding guidelines, "Do not add decorative section separators (e.g.
// -----------,// ===,/* *** */). Use///doc-comments or short//comments only when they add value."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/zstd.rs` around lines 52 - 54, Remove the decorative section separators (the repeated // ===... lines) found in crates/stdlib/src/zstd.rs (e.g., around the "Module-level constants" comment and the other flagged locations)—delete those purely decorative comment blocks and, where a section boundary is helpful, replace them with a brief single-line comment or an appropriate /// doc-comment tied to the nearby item (e.g., the constants block or the functions/structs) so the file no longer contains decorative separators but still documents sections meaningfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/stdlib/src/zstd.rs`:
- Around line 52-54: Remove the decorative section separators (the repeated //
===... lines) found in crates/stdlib/src/zstd.rs (e.g., around the "Module-level
constants" comment and the other flagged locations)—delete those purely
decorative comment blocks and, where a section boundary is helpful, replace them
with a brief single-line comment or an appropriate /// doc-comment tied to the
nearby item (e.g., the constants block or the functions/structs) so the file no
longer contains decorative separators but still documents sections meaningfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8b515ccc-727b-4221-b11d-b43ee2f1028e
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_zstd.pyis excluded by!Lib/**
📒 Files selected for processing (4)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/zstd.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/stdlib/Cargo.toml
- crates/stdlib/src/lib.rs
|
Relevant for this patch (or future updates): https://trifectatech.org/blog/announcing-zstandard-in-rust/ RustPython is already using |
|
I am sorry that I will not have enough review capacity for open source projects until late this month. due to prev reviews, I feel like i need more time to spend on this patch. This is not a usual case, but I'd like to share a short conclusion about this patch: @JamesClarke7283 how do you think? |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
I think migrating to that pure rust library is a good idea, working on it now... |
|
Review pass after merging main into this branch (resolved the Changes in bfee2be:
Validation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 326: Update the zstd-safe dependency declaration in Cargo.toml to include
the zstdmt feature alongside std and zdict_builder, preserving default-features
= false so native compression.zstd builds enable multithreading.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ac0023f-29ca-4539-b907-1aa39a47ac28
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/zstd.rs
💤 Files with no reviewable changes (1)
- crates/stdlib/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/zstd.rs
|
Backend swap in da38149: Why a direct dependency instead of patching Side effect: this backend builds Worth knowing before merge: upstream is a Validation: |
|
Review pass on the libzstd-rs-sys rewrite (
Reproducer: with a content frame, CPython gives Fix: match CPython's mechanism — probe with a zero-size output buffer. libzstd then consumes input without emitting anything and never reports frame completion with un-emitted content, so the discard path disappears entirely (the truncation block is gone). Also treats the zero-size probe as not-output-full for loop control, which keeps Added Validation: |
|
Final review pass on the This round leaned on differential testing rather than eyeballing: a stress script driving streaming compress/decompress with randomized chunk sizes and Validation: |
youknowone
left a comment
There was a problem hiding this comment.
I read first 300 lines and stopped now. please check and verify every insists the code includes and if they are not trivial, add enough explanation yourself. Please make sure review them yourself, not your AI.
| #[pyattr] | ||
| const ZSTD_btultra2: i32 = libzstd_rs_sys::lib::zstd::ZSTD_btultra2 as i32; | ||
|
|
||
| #[pyattr(once, name = "zstd_version")] |
There was a problem hiding this comment.
| #[pyattr(once, name = "zstd_version")] | |
| #[pyattr(once)] |
you don't need to redefine thename
There was a problem hiding this comment.
Done — dropped to #[pyattr(once)].
I checked the macro rather than assuming: AttributeItem::gen_module_item in crates/derive-impl/src/pymodule.rs resolves the name via AttrItemMeta::simple_name(), which falls back to the function ident, so name = "zstd_version" was a no-op. Same for zstd_version_number below.
I left name in place on ZSTD_DStreamOutSize, where the function is zstd_dstream_out_size and the two genuinely differ.
| // SAFETY: `ZSTD_versionString` returns a pointer to libzstd's static, | ||
| // NUL-terminated version string. | ||
| unsafe { CStr::from_ptr(ZSTD_versionString()) } | ||
| .to_string_lossy() |
There was a problem hiding this comment.
does version string really have value to use to_string_lossy?
There was a problem hiding this comment.
No, it doesn't earn its keep — changed to to_str().
In the backend crate this is not a runtime-varying string at all:
// libzstd-rs-sys lib/common/zstd_common.rs
pub const extern "C" fn ZSTD_versionString() -> *const core::ffi::c_char {
c"1.5.8".as_ptr()
}A fixed ASCII MAJOR.MINOR.RELEASE literal, so there is nothing for a lossy conversion to repair. It now uses to_str() with a comment saying why the failure case can't arise.
| .into_owned() | ||
| } | ||
|
|
||
| #[pyattr(once, name = "zstd_version_number")] |
There was a problem hiding this comment.
Dropped as well — same reason as zstd_version above; the function name already produces the attribute name.
| const DICT_TYPE_DIGESTED: i32 = 0; | ||
| const DICT_TYPE_UNDIGESTED: i32 = 1; | ||
| const DICT_TYPE_PREFIX: i32 = 2; |
There was a problem hiding this comment.
what's the rationale to decide this to be consts instead of enum?
There was a problem hiding this comment.
No good rationale, honestly — converted to an enum.
DictType { Digested = 0, Undigested = 1, Prefix = 2 }, with DictType::from_marker decoding the user-supplied int once at the Python boundary. parse_zstd_dict_arg now returns the enum, so load_dict matches exhaustively over the three variants instead of treating "everything else" as undigested via a _ arm.
I verified the discriminants against CPython rather than trusting the old comment: they are the values of the dictionary_type enum in Modules/_zstd/_zstdmodule.h (DICT_TYPE_DIGESTED = 0, DICT_TYPE_UNDIGESTED = 1, DICT_TYPE_PREFIX = 2). The old comment named the type DictType, which does not exist — corrected.
I applied the same treatment to the flush modes further down: COMP_MODE_CONTINUE/FLUSH_BLOCK/FLUSH_FRAME are now a CompressMode enum whose discriminants are libzstd's own ZSTD_e_continue/ZSTD_e_flush/ZSTD_e_end (0/1/2), and CompressorState::last_mode holds the enum, so the state set_pledged_input_size() requires is checked by the type system rather than by comparing ints. flush() still rejects CONTINUE and out-of-range modes with its own message, so the Python-visible errors are unchanged.
| // enum in libzstd, which is what the public `CompressionParameter` IntEnum | ||
| // in `Lib/compression/zstd/__init__.py` derives its members from. | ||
| // libzstd-rs-sys models the enum as a newtype whose inner value is | ||
| // crate-private, so the ids — frozen C ABI values from zstd.h — are |
There was a problem hiding this comment.
did you verify this?
please share why https://github.com/trifectatechfoundation/libzstd-rs-sys/blob/main/lib/zstd.rs#L428-L492 can't be used
There was a problem hiding this comment.
You're right that they can be used — fixed, the literals are gone.
What I found at the pinned rev (99fb4c0):
// lib/zstd.rs:311-313
#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct ZSTD_dParameter(u32);
// lib/zstd.rs:425-427
#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct ZSTD_cParameter(pub(crate) u32);There is no accessor, From impl or Deref on either (grep for impl .*ZSTD_[cd]Parameter returns only the two inherent const blocks), so you can't ask the type for its id — but #[repr(transparent)] means it can be read back out. So all 20 constants now derive from ZSTD_cParameter::* / ZSTD_dParameter::* through a small const fn, and libzstd stays the single source of truth:
const ZSTD_c_windowLog: i32 = c_param_id(ZSTD_cParameter::ZSTD_c_windowLog);The unsafe is only sound in this direction — every bit pattern is a valid u32, so newtype→int is a narrowing. The reverse would be a widening into a type whose valid values libzstd defines, which is why c_param_enum still decodes untrusted ids from Python with an explicit match. That asymmetry is now stated in the SAFETY comment.
On "did you verify this": yes, and the values are unchanged — 100–107, 160–164, 200–202, 400–402, and ZSTD_d_windowLogMax = 100, confirmed by reading them back out of the built module, not just from the source.
- 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>
|
Please make sure you reviewed this patch entirely again in higher standard. And then ping me. Of course I have to review project level choices, but you are also in charge to review what the code and comments you submitted is stating. |
Summary
_zstdstdlib module (crates/stdlib/src/zstd.rs) implementingZstdCompressor,ZstdDecompressor,ZstdDict, and the compression/decompression parameter constants that back the pure-Pythoncompression.zstdpackage.zstdandzstd-safe(with thezdict_builderfeature) as workspace dependencies and register the module incrates/stdlib/src/lib.rs, gated off forandroidandwasm32targets.test_zstd_multithread_compressfrom askipIfto anexpectedFailureIfwhen libzstd was built without multi-threading support, so the case is tracked rather than silently skipped.Testing
cargo build -p rustpython-stdlib— Not runcargo run -- -m test test_zstd— Not runSummary by CodeRabbit