diff --git a/.gitignore b/.gitignore index b9c5911..e050997 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ artifacts/ temp_test_project/ output_svc/ output_proto/ +mail/ diff --git a/CLEANUP.md b/CLEANUP.md new file mode 100644 index 0000000..0768d9d --- /dev/null +++ b/CLEANUP.md @@ -0,0 +1,152 @@ +# Best Practices Audit — Cleanup Findings + +> Generated by monkey swarm on 2026-06-30. Four specialist auditors reviewed `runtime`, `codegen`, `roto-tonic`, and project-level structure. + +## Summary + +| Severity | Count | +|----------|-------| +| Critical | 9 | +| Major | 17 | +| Minor | ~40 | + +--- + +## Critical Issues + +### 1. `BufMutBuilder` panics instead of returning `Err` (`runtime`) +- **File:** `runtime/src/lib.rs:1038-1119` +- **Issue:** `BufMutBuilder` returns `Result<()>` on all write methods, but calls `put_slice` which panics if the underlying `BufMut` has no remaining capacity. Unlike `ProtoBuilder`, which checks bounds before every write, `BufMutBuilder` never actually produces an `Err`. +- **Fix:** Check `self.buf.remaining_mut()` before each `put_slice` and return `Err(RotoError::BufferOverflow)`, or remove the `Result` return type and document the panic contract. + +### 2. `#![no_std]` + feature flags don't match reality (`runtime`) +- **File:** `runtime/src/lib.rs:1`, `runtime/Cargo.toml:9-12` +- **Issue:** Crate declares `#![no_std]` but has unconditional `use bytes::BufMut`. The `bytes` crate requires `alloc` (and transitively `std`). The `alloc` and `std` feature flags exist but don't gate any code. The crate can't compile in `no_std` mode. +- **Fix:** Gate `bytes` behind `#[cfg(feature = "alloc")]` or remove the feature flags and document that the crate requires `std`. + +### 3. `types.rs` file corruption (`codegen`) +- **File:** `codegen/src/generator/types.rs:76` +- **Issue:** Line 76 contains a shell artifact (`EOF > /opt/workspace/codegen/src/generator/types.rs`) followed by a duplicated function body. The file likely won't compile. +- **Fix:** Clean up the file immediately. + +### 4. Massive code duplication in generator module (`codegen`) +- **File:** `codegen/src/generator/` (all files) +- **Issue:** `mod.rs` contains full implementations of `write_message`, `write_enum`, `write_service`, `map_type_to_rust_accessor`, `to_pascal_case`, `to_snake_case`, `DATA_IMPORTS` — all of which also exist in `messages.rs`, `services.rs`, `types.rs`, and `utils.rs`. Only the `mod.rs` versions appear to be used. +- **Fix:** Either delete the split files (if `mod.rs` is the single source of truth) or complete the refactor so `mod.rs` re-exports the split modules. + +### 5. ~50 `.expect()` panics in library generator code (`codegen`) +- **File:** `codegen/src/generator/mod.rs` (all generator functions) +- **Issue:** Malformed protos will crash the process instead of returning errors. +- **Fix:** Introduce a `GeneratorError` enum and return `Result, GeneratorError>` from all generator functions. + +### 6. Seven unused heavy dependencies in codegen (`codegen`) +- **File:** `codegen/Cargo.toml` +- **Issue:** `tonic`, `tower`, `bytes`, `http-body`, `http-body-util`, `tokio-stream`, `futures-util` are listed as dependencies but never used in the codegen library. They generate code that references these types via string literals — the generator itself doesn't need them. +- **Fix:** Remove these from `Cargo.toml`. This eliminates ~30+ transitive dependencies. + +### 7. Edition 2024 / resolver = "3" requires nightly (`workspace`) +- **File:** `Cargo.toml:16`, all crate `Cargo.toml` +- **Issue:** `resolver = "3"` is only valid for Edition 2024, which is not yet stable. No `rust-toolchain.toml` exists. This blocks adoption with stable Rust. +- **Fix:** Add `rust-toolchain.toml` with `channel = "nightly"`, or switch to `edition = "2021"` + `resolver = "2"`. + +### 8. No LICENSE file (`workspace`) +- **Issue:** The project cannot legally be used by others without a license. +- **Fix:** Add an MIT or Apache-2.0 license file. + +### 9. `StatusBody::new` dead parameter (`roto-tonic`) +- **File:** `roto-tonic/src/lib.rs:118-125` +- **Issue:** `StatusBody::new(data, status)` takes `status: u8` but never uses it. The code hardcodes `"0"` into the `grpc-status` trailer. +- **Fix:** Wire `status` into the trailer value, or remove the parameter. + +--- + +## Major Issues + +### Runtime + +| # | Issue | File | Fix | +|---|-------|------|-----| +| 1 | `RotoMessage::decode(bytes::Bytes)` forces heap allocation and blocks `no_std` | `lib.rs:74-77` | Accept `&[u8]` instead | +| 2 | 7 core public types lack doc comments (`MapFieldIterator`, `RotoOwned`, `RotoMessage`, `ProtoAccessor`, `ProtoBuilder`, `BufMutBuilder`, `FieldIterator`, `RepeatedFieldIterator`) | `lib.rs` | Add `///` doc comments | + +### Codegen + +| # | Issue | File | Fix | +|---|-------|------|-----| +| 3 | `codegen/src/runtime/mod.rs` duplicates `roto-runtime` types (RotoError, Tag, WireType, read_varint, etc.) | `src/runtime/mod.rs` | Remove the duplicate; depend on `roto-runtime` | +| 4 | `to_pascal_case` has two implementations with different case-handling behavior | `mod.rs:28-38` vs `utils.rs` | Consolidate to single canonical version | +| 5 | `test_oneofs.rs` is completely empty — tests nothing | `tests/test_oneofs.rs` | Implement or remove | +| 6 | All integration tests use hardcoded `/tmp/` paths — won't run in parallel or on Windows | `tests/` | Use `std::env::temp_dir()` or `tempfile` | + +### Roto-tonic + +| # | Issue | File | Fix | +|---|-------|------|-----| +| 7 | `BufferPool::get()` / `put()` call `.unwrap()` on `Mutex::lock()` — aborts on poison | `src/lib.rs:96-109` | Handle poison state or use `tokio::sync::Mutex` | +| 8 | Generated traits use native async (Edition 2024), but test impls use `#[tonic::async_trait]` — ABI mismatch | `src/generated/` + `tests/` | Remove `#[tonic::async_trait]` from test impls | +| 9 | `.unwrap()` on `Response::builder().body()` in 7+ places — panics in hot path | `src/generated/` | Use `.expect()` or extract a helper | +| 10 | Missing `cargo:rerun-if-changed` for `.proto` files — changes don't trigger rebuild | `build.rs` | Add rerun directives | +| 11 | `build.rs` hardcodes `target/debug/protoc-gen-roto` — breaks release and cross-compile | `build.rs` | Use `OUT_DIR` or workspace-level coordination | +| 12 | `tokio = { features = ["full"] }` pulls all subsystems for a library that only needs `net` | `Cargo.toml` | Move to `[dev-dependencies]` | +| 13 | `RotoDecoder::decode` converts all errors to `Status::internal()` — no mapping of error variants | `src/lib.rs:78` | Match `RotoError` variants to specific gRPC status codes | +| 14 | `StatusBody` fields are `pub` — breaks encapsulation | `src/lib.rs:112-126` | Make fields private; add consumer methods | + +### Project Structure + +| # | Issue | File | Fix | +|---|-------|------|-----| +| 15 | No `[workspace.dependencies]` — shared deps duplicated across crates, risking version drift | `Cargo.toml` | Define workspace-level dependency table | +| 16 | `interop_test.rs` asserts `is_ok()` on `UNIMPLEMENTED` call — tests wrong behavior | `tests/interop_test.rs` | Assert `is_err()` with `UNIMPLEMENTED` status | +| 17 | No CI configuration, no `clippy.toml`, no `.rustfmt.toml` — code quality not enforced | Project root | Add GitHub Actions + config files | +| 18 | `roto/roto/` stray directory and `grpc_bench/` empty submodule | Project root | Clean up stale directories | +| 19 | `panic = "abort"` in `[profile.dev]` is too aggressive for a library | `Cargo.toml` | Remove from dev profile; keep only in release | +| 20 | `protos/` crate has no purpose (single `hello()` function) | `protos/` | Remove or give it a real purpose | + +--- + +## Minor Issues (Selected) + +### Runtime +- `ProtoAccessor::new` returns `Result` but never fails — change to direct constructor +- `write_int32` doesn't use zigzag encoding — document this or add `write_sint32` +- `RepeatedFieldIterator::new_range` silently tolerates out-of-bounds ranges +- Tests gated behind `#[cfg(feature = "alloc")]` may be silently skipped +- `RotoError` lacks `source()` chaining — consider `thiserror` + +### Codegen +- Missing `#[must_use]` on public generator functions +- No crate-level or function-level documentation +- Magic numbers for protobuf field types/labels (1-18) +- `env_logger::init()` is deprecated — use `init_from_env()` +- `edition = "2024"` without `rust-version` key +- Utility functions (`to_pascal_case`, `to_snake_case`) are `pub` but should be `pub(crate)` + +### Roto-tonic +- `#![allow(unused, ...)]` blanket suppresses important lints in generated code +- `tokio::time::sleep(100ms)` race condition in `test_server_start` +- Server task spawned but never joined/aborted in `interop_test.rs` +- Missing `#[derive(Debug)]` on `BufferPool`, `StatusBody` +- Error response body `[0, 0, 0, 0, 0]` should be a named constant +- `Infallible` error type hides panic sites + +### Project +- `exclude` list incomplete — test temp dirs not listed +- `Cargo.lock` should be committed (it is, but no `.gitignore` exception) +- `.desc` files may be generated artifacts that should be regenerated, not checked in +- No `rust-toolchain.toml` despite Edition 2024 requirement +- No `[workspace.metadata]` for crates.io readiness +- `alloc` feature flag on `codegen` and `benches` is meaningless for CLI/benchmark crates + +--- + +## Positive Observations + +The project has several strengths: + +- **Zero `unsafe` code** — all memory operations in the runtime use safe slice indexing +- **Consistent error handling** — all fallible operations return `Result` +- **No panics in library code** — `unwrap()`/`expect()` appear only in tests +- **RevBuilder** is a creative, well-implemented design for reverse-direction encoding +- **18 unit tests** in the runtime with good edge-case coverage +- **Comprehensive README** with design rationale, API examples, and benchmark comparisons +- **Well-structured workspace** with clear crate boundaries \ No newline at end of file diff --git a/TASKS.md b/TASKS.md index 7e158a7..fcbfc1b 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1,44 +1,123 @@ -# Tasks for Tonic Integration +# Task: Verify Generated Code Works in `no_std` / `no_alloc` -This document outlines the steps required to integrate the `roto` protobuf library with the `tonic` gRPC framework. +**Goal:** Ensure all generated protobuf code compiles and runs in `no_std` + `no_alloc` environments. Features requiring `alloc` must be gated behind feature flags in the proto generator's output. -## Goals -- Provide a `tonic::codec::Codec` implementation for `roto` messages. -- Enable zero-allocation decoding of gRPC requests and responses. -- Support efficient encoding of gRPC messages using `roto`'s `Builder` pattern. -- Generate `tonic`-compatible service traits and client/server boilerplate via `protoc-gen-roto`. +--- -## 1. Runtime Changes (`roto_runtime`) -- [x] Add `bytes` as a dependency to `roto_runtime`. -- [x] Modify `ProtoBuilder` to support writing to `bytes::BufMut` or provide a specialized `BufMut` based builder to facilitate integration with `tonic::codec::EncodeBuf`. -- [x] Design and implement "Owned" message support: - - [x] Create a mechanism (likely via codegen) to generate structs that hold `bytes::Bytes` and the field offsets (e.g., `OwnedMyMessage`). - - [x] These structs will serve as the owned types required by `tonic`'s `Codec`. - - [x] Add a method to convert an `OwnedMyMessage` into a zero-allocation `Reader` (e.g., `reader(&self) -> MyMessage<'_>`). +## Findings -## 2. Tonic Codec Implementation -- [x] Implement `tonic::codec::Codec` for `roto` messages. -- [x] Implement `tonic::codec::Decoder` for `roto` messages: - - [x] The decoder should take a `DecodeBuf` and produce an `OwnedMyMessage`. - - [x] It must perform the initial scan of the buffer to populate the field offsets. -- [x] Implement `tonic::codec::Encoder` for `roto` messages: - - [x] The encoder should take an `OwnedMyMessage` and write its internal `bytes::Bytes` to the `EncodeBuf`. - - [x] Since `roto` responses are built using `Builder` directly into a buffer, the `Encoder` will primarily handle copying these pre-built buffers. +**Already clean (no_std + no_alloc):** +- All reader structs (`Message<'a>`) — zero-copy accessors over `&[u8]` +- All field accessors, enums, builders (`MessageBuilder<'b>`), typed iterators +- Core runtime: `ProtoAccessor`, `ProtoBuilder`, `RevBuilder`, `read_varint`, `write_varint`, all iterators -## 3. Code Generation Extensions (`protoc-gen-roto`) -- [x] Update the generator to produce `OwnedMyMessage` structs in addition to the `Reader` and `Builder` structs. -- [x] Generate the `OwnedMyMessage::new(data: bytes::Bytes)` method for initial decoding. -- [x] Implement gRPC service generation: - - [x] Generate `tonic`-compatible service traits (using `#[tonic::async_trait]`). - - [x] Generate client and server boilerplate that uses `OwnedMyMessage` as the request and response types. - - [x] Ensure the generated code correctly maps protobuf services to Rust traits. +**Needs `alloc` feature gate:** +- `RotoMessage` trait — uses `bytes::Bytes` (has `Arc` internally) +- `RotoOwned` trait — same issue +- `OwnedMessage` structs generated by codegen +- `RevBuilder::with()` — uses `Vec<_>` to collect raw fields +- `Bytes`, `BytesMut` imports in `DATA_IMPORTS` -## 4. Testing and Validation -- [x] Create a test gRPC project using the updated `protoc-gen-roto`. -- [ ] Implement a sample gRPC service with: - - [ ] A unary call. - - [ ] A server-streaming call. - - [ ] A client-streaming call. - - [ ] A bidirectional-streaming call. -- [ ] Verify that the integration is zero-allocation on the reading path. -- [ ] Perform benchmark tests to compare performance with `prost`. +**Needs `std` feature gate:** +- All service code (`SERVICE_IMPORTS`) — uses `std::sync::Arc`, `std::pin::Pin`, `String`, etc. +- `std::error::Error for RotoError` (already gated) + +--- + +## Plan + +### Phase 1: Runtime (`runtime/src/lib.rs`, `runtime/Cargo.toml`) + +1. **Gate `RotoMessage` and `RotoOwned`** behind `#[cfg(feature = "alloc")]` +2. **Gate `BufMutBuilder`** behind `#[cfg(feature = "alloc")]` — only useful with `BytesMut`, which requires alloc +3. **Gate `use bytes::BufMut`** — only import it conditionally +4. **Wire up `bytes` features in Cargo.toml:** + ```toml + bytes = { version = "1.7", default-features = false } + # ... + [features] + default = ["std", "alloc"] + std = ["bytes/std"] + alloc = ["bytes"] + ``` + +### Phase 2: Codegen — `DATA_IMPORTS` & Generated Data Code (`codegen/src/generator/mod.rs`) + +5. **Split `DATA_IMPORTS`** into unconditional + gated sections: + ```rust + // Always emitted + use roto_runtime::{ProtoAccessor, ProtoBuilder, RevBuilder, Result, RotoError, read_varint, RepeatedFieldIterator}; + use core::str; + use bytes::{Buf, BufMut}; + + // Only emitted when alloc feature is enabled + #[cfg(feature = "alloc")] + use bytes::{Bytes, BytesMut}; + #[cfg(feature = "alloc")] + use roto_runtime::RotoMessage; + ``` + +6. **Gate `OwnedMessage` structs + `RotoMessage` impls** with `#[cfg(feature = "alloc")]` + +7. **Fix `RevBuilder::with()`** — replace `let fields: Vec<_> = ... .collect()` with a forward iteration that doesn't allocate. The reverse order doesn't matter for protobuf encoding (fields are independent): + ```rust + // Before: let fields: Vec<_> = msg.accessor.raw_fields().collect(); + // for (raw, _) in fields.iter().rev() { ... } + // After: for (raw, _) in msg.accessor.raw_fields() { ... } + ``` + Alternatively, gate the `with()` method itself behind `#[cfg(feature = "alloc")]`. + +### Phase 3: Codegen — Service Code (`codegen/src/generator/mod.rs`) + +8. **Gate `SERVICE_IMPORTS`** with `#[cfg(feature = "std")]` +9. **Gate all service traits, server impls, and `*Server` structs** with `#[cfg(feature = "std")]` + +### Phase 4: Generated Consumer Setup + +10. **Emit a `Cargo.toml`** (or instruct consumers) with the right feature setup: + ```toml + [features] + default = ["std"] + std = ["roto-runtime/std", "alloc"] + alloc = ["roto-runtime/alloc", "bytes"] + ``` + +### Phase 5: Validation + +11. **Update `examples/no_std_test`** to build with `--no-default-features` and verify compilation +12. **Run existing tests** with default features to ensure nothing breaks +13. **Add a compile-test** that verifies `--no-default-features` builds succeed for generated code + +--- + +## Dependency Graph + +``` +no_std + no_alloc → readers, enums, builders, iterators + ↓ alloc → + OwnedMessage, RotoMessage trait + ↓ std → + service code, std::error::Error +``` + +## Risk Assessment + +| Risk | Mitigation | +|------|-----------| +| Breaking existing consumers who use `OwnedMessage` | `OwnedMessage` was already behind a manual `#[cfg]` in the `no_std_test` example — consumers aren't relying on it unconditionally | +| `BufMutBuilder` removal in no-alloc mode | It's only useful with `BytesMut` anyway, which requires alloc | +| `RevBuilder::with()` change | Forward iteration is semantically equivalent for protobuf encoding | + +## Files to Modify + +| File | Changes | +|------|---------| +| `runtime/src/lib.rs` | Gate `RotoMessage`, `RotoOwned`, `BufMutBuilder`, `use bytes::BufMut` | +| `runtime/Cargo.toml` | Wire `alloc` → `bytes` dependency, add `bytes/std` to `std` feature | +| `codegen/src/generator/mod.rs` | Split `DATA_IMPORTS`, gate `OwnedMessage`, gate service code, fix `RevBuilder::with()` | +| `examples/no_std_test/` | Update to use new feature structure | + +## Execution Order + +1. Phase 1 (runtime) — foundation +2. Phase 2 (codegen data imports) — depends on runtime +3. Phase 3 (codegen services) — independent +4. Phase 4–5 (consumer setup + validation) — depends on all above \ No newline at end of file