KillingSpark / KillingSpark/zstd-rs

Refactor oversized Rust modules and fix reviewability/correctness issues

Open
#109 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
449
Forks
66
PR merge metrics
No merged PRs in 30d

Description

## Summary

The current Rust codebase has a few concrete correctness and reliability issues, plus a larger maintainability problem: several core `.rs` files are far beyond a reviewable size and mix unrelated responsibilities. This makes the code materially harder to audit, test, and evolve safely.

This issue records the work needed to bring the codebase back toward normal Rust reviewability and module hygiene.

## Why this needs to be done

From a recent code review pass, the main problems are:

1. There is a probable correctness bug in dictionary generation.
2. The CLI still has production panic paths for normal user errors.
3. Decoder-core `unsafe` code is not isolated tightly enough for easy review.
4. Several core files are far too large and mix production code, tuning logic, heuristics, and tests.
5. Documentation quality is uneven, and some module docs are malformed.

We should target modules that are easy to review in isolation. As a working guideline, aim for roughly **300-500 lines per file** where practical, and only exceed that when there is a strong structural reason.

## Concrete findings to address

### 1. Fix the dictionary construction bug

File:
- `ruzstd/src/dictionary/mod.rs`

Problems:
- `epoch_size` is computed, but the actual buffer is hard-coded to `vec![0; 100]`
- a `dbg!(...)` remains in the production path
- the epoch loop appears to read into `current_epoch` but still scores against `collection_sample`

Expected outcome:
- verify and fix the raw dictionary generation logic
- remove debug leftovers
- add or improve tests that would catch this regression

### 2. Remove production panic paths from the CLI

Files:
- `cli/src/main.rs`
- `cli/src/progress.rs`

Problems:
- `unwrap()` / `expect()` are used in user-facing runtime paths
- malformed module documentation exists in `progress.rs`

Expected outcome:
- replace panic paths with proper `Result`-based error propagation
- clean up malformed rustdoc
- keep user-facing failures actionable and non-panicking

### 3. Isolate and better document decoder-core `unsafe`

File:
- `ruzstd/src/decoding/decode_buffer.rs`

Problems:
- `unsafe` is used in a core buffer path
- invariants are not isolated behind the smallest possible API boundary

Expected outcome:
- either reduce/remove the `unsafe`, or isolate it into a minimal, well-documented helper
- document invariants precisely
- add focused tests covering overlap/copy invariants

### 4. Split oversized core modules into logical groups

Highest-priority files:
- `ruzstd/src/encoding/match_generator.rs` (~7700 lines)
- `ruzstd/src/encoding/blocks/compressed.rs` (~2400 lines)
- `ruzstd/src/encoding/mod.rs` (~1500 lines)
- `ruzstd/src/encoding/frame_compressor.rs` (~1100 lines)
- `ruzstd/src/huff0/huff0_encoder.rs` (~970 lines)
- `ruzstd/src/tests/mod.rs` (~1000 lines)

This should be a **logical split**, not a mechanical one.

Suggested direction:

#### `match_generator.rs`
Split into modules such as:
- matcher core/state
- file/profile heuristics
- candidate scoring and tie-breaks
- recency sidecar tracking
- lazy-parse logic
- diagnostics
- tests by topic

#### `compressed.rs`
Split into modules such as:
- block compression config
- literal encoding
- sequence encoding
- exact table-search logic
- tests

#### `encoding/mod.rs`
Split into modules such as:
- public encoding API
- file classification
- file/profile hints
- path/data sampling

#### `tests/mod.rs`
Split by behavior/topic instead of accumulating all integration-style tests in one file.

Expected outcome:
- smaller files with clear ownership boundaries
- easier code review
- reduced cognitive load when changing a single behavior

### 5. Move embedded tests out of giant production files where they harm reviewability

Examples:
- `ruzstd/src/encoding/match_generator.rs`
- `ruzstd/src/huff0/huff0_encoder.rs`

Expected outcome:
- keep close-to-code tests where useful
- but move large topic groups into dedicated test modules/files
- production files should not contain thousands of lines of embedded test code

### 6. Improve internal documentation at module boundaries

Problems:
- some modules have useful docs, but many boundaries are still implicit
- giant files make the docs less useful because responsibilities are blurred

Expected outcome:
- each major module should state:
- what it owns
- what invariants it depends on
- what it intentionally does not handle
- document non-obvious tuning/profile logic and candidate-selection rules
- fix malformed or stale rustdoc while splitting files

## Proposed implementation order

1. Fix the dictionary correctness issue.
2. Remove CLI panic paths and malformed docs.
3. Isolate/document decoder `unsafe`.
4. Split `match_generator.rs`.
5. Split `compressed.rs` and `encoding/mod.rs`.
6. Split remaining oversized modules and test aggregations.
7. Finish with a documentation pass once module boundaries are stable.

## Acceptance criteria

- [ ] dictionary generation bug is fixed and covered by tests
- [ ] no production `unwrap()` / `expect()` remain in normal CLI error paths
- [ ] decoder `unsafe` is either reduced or isolated behind clearly documented invariants
- [ ] `match_generator.rs` is split into logical submodules
- [ ] `compressed.rs` is split into logical submodules
- [ ] `encoding/mod.rs` is reduced to a small module root plus submodules
- [ ] large embedded test bodies are moved into topic-based test modules where appropriate
- [ ] malformed module docs are fixed
- [ ] key internal modules have clear rustdoc describing responsibilities and invariants
- [ ] the resulting structure is materially easier to review than the current layout

## Notes

This issue is intentionally about **reviewability and maintainability**, not just formatting. The goal is to make future compression and decoder changes easier to reason about and safer to review.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the proposed implementation order, beginning in ruzstd/src/dictionary/mod.rs, then review cli/src/main.rs, cli/src/progress.rs, and ruzstd/src/decoding/decode_buffer.rs with their related tests. Next map responsibilities and test groups in the listed oversized encoding and test modules before choosing logical boundaries. Done means the acceptance checklist is met and the resulting structure is materially easier to review.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
cli, documentation, performance, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.