Skip to content

Add _zstd stdlib module backing compression.zstd - #7962

Open
JamesClarke7283 wants to merge 20 commits into
RustPython:mainfrom
JamesClarke7283:zstd
Open

Add _zstd stdlib module backing compression.zstd#7962
JamesClarke7283 wants to merge 20 commits into
RustPython:mainfrom
JamesClarke7283:zstd

Conversation

@JamesClarke7283

@JamesClarke7283 JamesClarke7283 commented May 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add new _zstd stdlib module (crates/stdlib/src/zstd.rs) implementing ZstdCompressor, ZstdDecompressor, ZstdDict, and the compression/decompression parameter constants that back the pure-Python compression.zstd package.
  • Wire up zstd and zstd-safe (with the zdict_builder feature) as workspace dependencies and register the module in crates/stdlib/src/lib.rs, gated off for android and wasm32 targets.
  • Convert test_zstd_multithread_compress from a skipIf to an expectedFailureIf when libzstd was built without multi-threading support, so the case is tracked rather than silently skipped.

Testing

  • cargo build -p rustpython-stdlib — Not run
  • cargo run -- -m test test_zstd — Not run

Summary by CodeRabbit

  • New Features
    • Added Zstandard compression support on non-Android, non-WASM platforms.
    • Added streaming compression and single-frame decompression with configurable options, output limits, and input tracking.
    • Added dictionary support, including validation, training, finalization, and multiple attachment modes.
    • Added frame inspection, parameter validation, version information, constants, and improved Zstandard error reporting.

@coderabbitai

coderabbitai Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds zstd workspace dependencies, conditionally registers the stdlib _zstd module, and implements compression, decompression, dictionary, frame, and parameter APIs.

Changes

Zstd Extension Module

Layer / File(s) Summary
Dependencies and module wiring
Cargo.toml, crates/stdlib/Cargo.toml, crates/stdlib/src/lib.rs
Adds the zstd-safe workspace dependency with zdict_builder enabled and default features disabled. Registers the module for non-Android, non-WASM32 targets.
Module contracts and parameter validation
crates/stdlib/src/zstd.rs
Adds constants, version helpers, ZstdError, parameter validation, dictionary argument parsing, parameter-bound lookup, and parameter type registration.
Dictionary representation and loading
crates/stdlib/src/zstd.rs
Adds ZstdDict, dictionary ID handling, attachment forms, and shared dictionary loading.
Streaming compression
crates/stdlib/src/zstd.rs
Adds ZstdCompressor, option and level handling, streaming compression, flushing, pledged input sizes, and mode tracking.
Streaming decompression
crates/stdlib/src/zstd.rs
Adds ZstdDecompressor with bounded output, input buffering, EOF tracking, trailing unused_data, and state accessors.
Frame and dictionary utilities
crates/stdlib/src/zstd.rs
Adds frame inspection, dictionary training and finalization.

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
Loading

Suggested reviewers: shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: adding the _zstd standard-library module for compression.zstd.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 May 23, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The 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:

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d3272e7 and 5f3acc7.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_zstd.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • Cargo.toml
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/lib.rs
  • crates/stdlib/src/zstd.rs

Comment thread crates/stdlib/src/zstd.rs Outdated
Comment on lines +58 to +60
// =========================================================================
// Module-level constants
// =========================================================================

@coderabbitai coderabbitai Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're still there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs Outdated
@ShaharNaveh

Copy link
Copy Markdown
Contributor

And tysm for working on this!

@ShaharNaveh

Copy link
Copy Markdown
Contributor

@JamesClarke7283 please pull from main, we've fixed the CI issue

Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs
@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

@youknowone can this be merged?

Comment thread crates/stdlib/src/zstd.rs Outdated
Comment thread crates/stdlib/src/zstd.rs
@JamesClarke7283 JamesClarke7283 mentioned this pull request May 28, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
crates/stdlib/src/zstd.rs (1)

52-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 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

📥 Commits

Reviewing files that changed from the base of the PR and between fc607c4 and 1b0b077.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_zstd.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • Cargo.toml
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/lib.rs
  • crates/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

Comment thread crates/stdlib/src/zstd.rs Outdated
@joshuamegnauth54

Copy link
Copy Markdown
Contributor

Relevant for this patch (or future updates): https://trifectatech.org/blog/announcing-zstandard-in-rust/

RustPython is already using bzip2 and zlib from Trifecta.

@youknowone

Copy link
Copy Markdown
Member

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:
This is AI-generated patch. then it must be not that hard to move on the new libzstd-rs-sys.

@JamesClarke7283 how do you think?

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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.

@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

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: This is AI-generated patch. then it must be not that hard to move on the new libzstd-rs-sys.

@JamesClarke7283 how do you think?

I think migrating to that pure rust library is a good idea, working on it now...

@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

Review pass after merging main into this branch (resolved the liblzmaxz rename and the new rustpython-unicode crate in Cargo.lock/Cargo.toml).

Changes in bfee2be:

  • Removed the seven remaining decorative // ===... separator blocks in zstd.rs per the comment guidelines — an earlier thread marked them resolved, but they were still in the file.
  • Dropped the unused zstd crate from the dependency graph. Only zstd_safe (and its zstd_sys re-export) are referenced; the zstd crate was incidentally enabling zstd-safe's std feature (which gates WriteBuf for Vec<u8>) through feature unification. std is now declared explicitly on zstd-safe alongside zdict_builder.
  • Converted a /// doc comment on #[extend_class] to a regular comment (derive macros source docstrings from CPython's docs).

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 771290b and bfee2be.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • Cargo.toml
  • crates/stdlib/Cargo.toml
  • crates/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

Comment thread Cargo.toml Outdated
@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

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.

@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

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.

@JamesClarke7283

Copy link
Copy Markdown
Contributor Author

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.

@youknowone
youknowone requested a review from ShaharNaveh August 10, 2026 00:46

@youknowone youknowone left a comment

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.

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.

Comment thread crates/stdlib/src/zstd.rs Outdated
#[pyattr]
const ZSTD_btultra2: i32 = libzstd_rs_sys::lib::zstd::ZSTD_btultra2 as i32;

#[pyattr(once, name = "zstd_version")]

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.

Suggested change
#[pyattr(once, name = "zstd_version")]
#[pyattr(once)]

you don't need to redefine thename

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/stdlib/src/zstd.rs Outdated
// SAFETY: `ZSTD_versionString` returns a pointer to libzstd's static,
// NUL-terminated version string.
unsafe { CStr::from_ptr(ZSTD_versionString()) }
.to_string_lossy()

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.

does version string really have value to use to_string_lossy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/stdlib/src/zstd.rs Outdated
.into_owned()
}

#[pyattr(once, name = "zstd_version_number")]

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.

dup name def

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped as well — same reason as zstd_version above; the function name already produces the attribute name.

Comment thread crates/stdlib/src/zstd.rs Outdated
Comment on lines +151 to +153
const DICT_TYPE_DIGESTED: i32 = 0;
const DICT_TYPE_UNDIGESTED: i32 = 1;
const DICT_TYPE_PREFIX: i32 = 2;

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.

what's the rationale to decide this to be consts instead of enum?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/stdlib/src/zstd.rs Outdated
// 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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

JamesClarke7283 and others added 20 commits August 19, 2026 13:38
- 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>
@youknowone

Copy link
Copy Markdown
Member

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.

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.

6 participants