# Code Review: `corophage`
## Overview
`corophage` is a well-structured algebraic effects library built on `fauxgen` (coroutines) and `frunk_core` (heterogeneous lists/coproducts). The overall design is sound and the code is clean. Tests pass and Clippy is clean. Below are findings organized by severity.
---
## Bugs
### 1. `Never` is documented but not exported — `README.md`
The README at line 64 shows:
```rust
use corophage::{Effect, Never};
```
and uses `Never` throughout as a library type. But `Never` is **not** exported by the crate. Users must define it themselves (as seen in the examples). This is a documentation bug that will confuse new users.
### 2. `asynk::run_mut` test uses the sync runner — `tests/simple.rs:180`
```rust
// In mod asynk { ... #[tokio::test] async fn run_mut() { ... }
let result = corophage::sync::run( // <-- uses sync, not async
co(),
&mut hlist![...],
);
```
The test is inside `mod asynk` and annotated with `#[tokio::test]`, but it calls `corophage::sync::run`. While this compiles and passes (sync code can run inside an async context), it's almost certainly a copy-paste error that provides no async coverage. It should use `corophage::run(co(), &mut hlist![...]).await`.
---
## Correctness Concerns
### 3. `unreachable!()` instead of `match self {}` in uninhabited impls — `src/coproduct.rs:17-19, 48-51, 81-83, 113-115`
```rust
impl<R> FoldMut<HNil, R> for CNil {
fn fold_mut(self, _: &mut HNil) -> R {
unreachable!() // <-- should be `match self {}`
}
}
```
`CNil` is uninhabited (it's `frunk_core`'s empty coproduct), so these function bodies are never reached. However, `unreachable!()` is a runtime panic while `match self {}` is a compile-time proof of exhaustiveness. The latter is more correct — it allows the compiler to verify the code path truly cannot execute rather than relying on a runtime assertion. The same issue appears in all four of the `HNil`-receiver impls.
### 4. `unsafe { self.map_unchecked_mut(...) }` soundness dependency — `src/coroutine.rs:54`
```rust
pub(crate) fn resume(self: Pin<&mut Self>, ...) {
let mut g = unsafe { self.map_unchecked_mut(|s| &mut s.generator) };
...
}
```
This is sound today because `Co<Effs, Return>` is not `Unpin` (its `SyncGenerator` field is not `Unpin`). However, there is no `impl !Unpin` to make this explicit. If someone adds `#[pin_project]` or an `Unpin` impl in the future (e.g., adding a `PhantomPinned`-free field), this could silently become unsound. Adding `_pin: PhantomPinned` to `Co` or using `pin_project` would make this robust.
---
## Design Limitations (worth documenting explicitly)
### 5. `Co<Effs, Return>` is not `Send`
`PinBoxFuture<A>` is `Pin<Box<dyn Future<Output = A>>>` — no `+ Send` bound. This means `Co` is not `Send` even if `Effs` and `Return` are `Send`, making it impossible to use with `tokio::spawn` or any multi-threaded executor. This is a significant limitation for async users. The fix would be to conditionally use `Pin<Box<dyn Future<Output = A> + Send>>` (perhaps via a `Co<Effs, Return, Marker = Local | Send>` pattern, or a separate `SendCo` type).
### 6. All effects must be `'static`
`Effects: MapResume + 'static` propagates up to `Co::new`, requiring `f: impl FnOnce(...) + 'static`. This means effects cannot carry non-`'static` lifetimes. The examples work around this with `Log<'static>`, but the README shows `Log<'a>` as if lifetime parameters are usable, which they aren't for the effects list. This constraint comes from the `Box<dyn Future<...>>` type erasure and is hard to lift without GATs or a significantly different approach.
### 7. `Cancelled` doesn't implement `std::error::Error` or `Display`
```rust
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct Cancelled;
```
`Cancelled` is returned as `Err(Cancelled)` from the run functions, making it an error type in practice. Without `impl std::fmt::Display` and `impl std::error::Error`, users can't use `?` to propagate it through functions returning `Box<dyn Error>` or `anyhow::Error`.
---
## Minor / Style
### 8. The `run!` macro captures `Effs` as a free variable — `src/lib.rs:43`
```rust
let resume: CoControl<Effs> = $handle;
```
`Effs` is not bound by the macro itself — it's implicitly the type parameter of the enclosing `run` / `run_with` function. This works, but it's a hidden dependency that makes the macro fragile to use outside its current context and difficult to reason about in isolation.
### 9. `SetState<S>::Resume = ((), ())` in examples is unintuitive
```rust
impl<S> Effect for SetState<S> {
type Resume = ((), ()); // why not `()`?
}
```
This appears in `examples/asynk.rs:37`, `examples/sync.rs:37`, and `tests/simple.rs:37`. The double-unit tuple is there probably as an artifact of some earlier encoding, but it adds noise at every call site (`CoControl::resume(((), ()))`). Since `()` is the natural "no meaningful result" type, this should just be `()`.
---
## Performance Observations
The coproduct-based dispatch is effectively **O(1)** at runtime per effect because the match tree is monomorphized and inlined by the compiler. The benchmarks confirm this (dispatch position has negligible impact). The main cost is:
- **`Co::new`**: One `Box::pin` heap allocation per coroutine. Unavoidable with the current type-erasure approach.
- **Per-yield cost**: ~9–10 ns sync, ~11 ns async — competitive.
- **`RefCell` pattern** vs `run_with`: Essentially equivalent (~55 ns vs ~53 ns), validating that both state-sharing patterns are viable.
No gratuitous allocations, no unnecessary cloning. The fold implementations properly borrow the handler in place rather than moving it.
---
## Summary Table
| 1 | Bug | `README.md` | `Never` documented as exported, not actually exported |
| 2 | Bug | `tests/simple.rs:180` | `asynk::run_mut` calls sync runner |
| 3 | Correctness | `src/coproduct.rs` | `unreachable!()` should be `match self {}` |
| 4 | Correctness | `src/coroutine.rs:54` | Pinning soundness not structurally enforced |
| 5 | Design | `src/coroutine.rs` | `Co` is not `Send`, blocking `tokio::spawn` |
| 6 | Design | `src/effect.rs` | `'static` requirement on effects is undocumented |
| 7 | Design | `src/control.rs` | `Cancelled` missing `Error`/`Display` impls |
| 8 | Minor | `src/lib.rs` | `run!` macro's implicit `Effs` free variable |
| 9 | Minor | examples/tests | `SetState::Resume = ((), ())` should be `()` |