| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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 reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds zstd workspace dependencies, conditionally registers the stdlib _zstd module, and implements compression, decompression, dictionary, frame, and parameter APIs. 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: shaharnaveh 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
📦 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:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f23e2155-6dfa-4c29-a3e4-7664092d9a73
📥 CommitsReviewing files that changed from the base of the PR and between d3272e7 and 5f3acc7.
⛔ Files ignored due to path filters (2)
Sorry, something went wrong.
| // ========================================================================= | ||
| // 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 AgentsVerify 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
Sorry, something went wrong.
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).
Sorry, something went wrong.
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!
Sorry, something went wrong.
There was a problem hiding this comment.
They're still there.
Sorry, something went wrong.
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!
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
And tysm for working on this! |
Sorry, something went wrong.
|
@JamesClarke7283 please pull from main, we've fixed the CI issue |
Sorry, something went wrong.
|
@youknowone can this be merged? |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)crates/stdlib/src/zstd.rs (1)🤖 Prompt for all review comments with AI agents52-54: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Decorative 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 AgentsVerify 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.
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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8b515ccc-727b-4221-b11d-b43ee2f1028e
📥 CommitsReviewing files that changed from the base of the PR and between fc607c4 and 1b0b077.
⛔ Files ignored due to path filters (2)
Sorry, something went wrong.
|
Relevant for this patch (or future updates): https://trifectatech.org/blog/announcing-zstandard-in-rust/ RustPython is already using bzip2 and zlib from Trifecta. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
I think migrating to that pure rust library is a good idea, working on it now... |
Sorry, something went wrong.
|
Review pass after merging main into this branch (resolved the liblzma→xz rename and the new rustpython-unicode crate in Cargo.lock/Cargo.toml). Changes in bfee2be:
Validation: cargo fmt --check, cargo clippy -p rustpython-stdlib, prek run --all-files, cargo test --workspace (excluding rustpython-capi, which CI runs separately), and cargo run --release -- -m test test_zstd (119/119) all pass. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ac0023f-29ca-4539-b907-1aa39a47ac28
📥 CommitsReviewing files that changed from the base of the PR and between 771290b and bfee2be.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
|
Backend swap in da38149: _zstd no longer links the C library. The zstd-safe/zstd-sys dependencies are replaced by libzstd-rs-sys (Trifecta Tech Foundation's pure-Rust c2rust translation of upstream libzstd), pinned to git rev 99fb4c0. Why a direct dependency instead of patching zstd-sys: the crate is 0.0.1-prerelease and git-only, so it can't satisfy zstd-safe's zstd-sys = "2.0.15" requirement. Instead, the module now calls its C-API-compatible surface (pub unsafe extern "C" fns, root-reexported) directly through small owning RAII wrappers (CCtx/DCtx/CDict/DDict with Drop + unsafe impl Send). The safety invariants from earlier review rounds are unchanged: context/dict field drop order in CompressorState/DecompressorState, PyMutex-serialized access, and the allow_threads audit for the (de)compression loops. Parameter ids remain validated through the explicit enum maps — no unchecked int→enum casts. Side effect: this backend builds zstdmt_compress in, so nb_workers bounds are (0, 256) and test_zstd_multithread_compress passes for real — the expectedFailureIf marker (added when the C build lacked MT) is removed. Worth knowing before merge: upstream is a 0.0.1 prerelease — its README calls the decoder experimental and the encoder still largely c2rust output; pinning by rev and the 119-test test_zstd suite (all passing) mitigate drift. On x86_64 its build uses cc for one assembly file (huf_decompress_amd64.S); elsewhere it's pure Rust. Validation: cargo run --release -- -m test test_zstd 119/119, cargo clippy -p rustpython-stdlib clean, prek run --all-files green, workspace test suite green. |
Sorry, something went wrong.
|
Review pass on the libzstd-rs-sys rewrite (da381499f): the RAII wrappers, drop-order invariants, allow_threads usage, and error mapping all check out — but it surfaced one real bug, fixed in 361e82dd3. ZstdDecompressor.decompress(data, max_length=0) silently lost one byte per call. The zero-cap probe handed libzstd a 1-byte buffer, and the post-loop truncation discarded whatever it emitted even though the input counter had already advanced past the bytes that produced it. It predates the backend swap (same logic in the zstd-safe version) and the suite never caught it because max_length=0 was only exercised against zero-output skippable frames. Reproducer: with a content frame, CPython gives decompress(frame, 0) → b'', then two drains → 100 + 1000 = all 1100 bytes; we returned 100 + 999 (and lost 2 bytes across two consecutive zero-cap probes). 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 needs_input identical to CPython on the full/partial/truncated probe matrix. Added extra_tests/snippets/stdlib_zstd.py with lossless zero-cap probe cases (content, truncated, and skippable frames), verified byte-exact against CPython (pytest test_snippets.py -k zstd passes on both runners). Validation: test_zstd 119/119, clippy + fmt clean, prek run --all-files green, workspace test suite green. |
Sorry, something went wrong.
|
Final review pass on the _zstd implementation. No new bugs found; the only change is a one-line tidy in 7235493db (dropped a duplicate spell-checker:ignore line — every token already appeared on earlier lines). This round leaned on differential testing rather than eyeballing: a stress script driving streaming compress/decompress with randomized chunk sizes and max_length values (0 included), multiframe + trailer/unused_data handling, all three dict load modes, pledged input size, options dicts, negative levels, empty payloads, multithreaded compression, and error surfaces — the full operation/flag log is byte-identical between CPython 3.14 and this build. Validation: test_zstd 119/119, snippet suite (-k zstd) green on both runners, clippy + fmt clean, prek run --all-files green, workspace test suite green. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| #[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
Sorry, something went wrong.
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.
Sorry, something went wrong.
| // 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| .into_owned() | ||
| } | ||
|
|
||
| #[pyattr(once, name = "zstd_version_number")] |
There was a problem hiding this comment.
dup name def
Sorry, something went wrong.
There was a problem hiding this comment.
Dropped as well — same reason as zstd_version above; the function name already produces the attribute name.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| // 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
Sorry, something went wrong.
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.
Sorry, something went wrong.
- 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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Testing
Summary by CodeRabbit