feat(protected): NonEmpty::with and From<integer> for NonEmpty - #314
Conversation
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 0 | 0 | 4 | 0 |
✅ Every mutant in the changed lines was caught by a test.
✅ No CRAP threshold violations573 function(s) analyzed · threshold 30 |
There was a problem hiding this comment.
🟡 Changes recommended
Public documentation is now inconsistent, and the defining no-bound guarantee lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds infallible composition and integer conversion APIs for NonEmpty.
Changes:
- Adds
NonEmpty::with. - Adds integer
Fromimplementations. - Tests AEAD and PRF encoding transparency.
File summaries
| File | Description |
|---|---|
packages/protected/src/non_empty.rs |
Implements APIs and unit tests. |
packages/prf/src/context.rs |
Tests PRF encoding equivalence. |
packages/aead/src/aad/mod.rs |
Tests AAD encoding equivalence. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`NonEmpty::with(tail)` pairs a proven value with any tail and keeps the
proof: a pair is empty only when both halves are, so a non-empty head makes
the pair non-empty whatever the tail. This is how a caller extends a fixed
context (`nonempty!("users/email")`) with a value known only at the call
site (a record id) without re-checking, and without an unchecked
constructor. The pair encodes exactly as the bare tuple would.
`From<T> for NonEmpty<T>` for the ten integer types, which `IsEmpty` already
declares never empty: an API taking `impl Into<NonEmpty<T>>` accepts a bare
integer, where a string goes through `nonempty!` or `NonEmpty::new`.
Claude-Session: https://claude.ai/code/session_019VRX1tXygj5bew3YhyK1B6
`IsEmpty` read as a marker on the type ("values of this type are empty")
when it is a capability ("this value can be asked whether it is empty").
Integers implement it and are never empty, which made the old name
actively misleading at the `NonEmpty::new` bound. `MaybeEmpty` says what
the trait provides and pairs naturally with `NonEmpty`.
BREAKING CHANGE: `vitaminc_protected::IsEmpty` is now `MaybeEmpty`.
Downstream implementations and bounds must be renamed; the method
`is_empty` is unchanged.
Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
…ts shape `with<U>` had no bound, making it the only constructor that could produce a `NonEmpty<X>` whose `X` could never have passed `NonEmpty::new`: an already-encoded `Aad` or `PrfContext`, or another `NonEmpty`, all slipped in as a tail. The invariant held (the head carries the bytes) but the crate-wide rule "wrap the whole composite, not the parts" was no longer enforced. `U: MaybeEmpty` closes that, pinned with a `compile_fail` doctest; every documented tail already satisfies it. The `with` docs now state that chaining nests to the left, `a.with(b).with(c)` is `((a, b), c)`, which PAE-frames differently from `(a, (b, c))`. A caller migrating an existing tuple context would otherwise silently change its AAD or PRF bytes. The struct docs still said there were "two ways to build one" and "no third path" while the same file shipped `From<integer>` and `with`. Rewritten as one rule, checked once where the type cannot prove non-emptiness and converted freely where it can, listing all four routes. The `From<integer>` doc no longer recommends `impl Into<..>` APIs, which contradicted the guidance to take `NonEmpty<C>` directly, and the README qualifies "no implicit conversion" to strings and byte slices. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
…tail `with` is documented as the way to attach a call-site value such as a record id, but the tail inherits nothing from the head beyond the non-emptiness proof. Three consequences were easy to miss and each leads to a silent wire-format or separation bug, so they now sit in the rustdoc: the tail is unchecked, so an empty id must be validated before pairing; an empty tail still frames as a pair and several empty tails encode identically in AEAD, so `()` cannot stand for "absent"; and integers encode untagged in AEAD, so the tail's type is not part of the authenticated context. Also notes that `with` only extends rightwards. A layout with the fixed part on the right, like `ContextTag::aad_with`, goes through `NonEmpty::new` on the whole tuple. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
The `compile_fail` doctest now pins the `MaybeEmpty` bound, so the unit test is renamed to what it actually covers: unchecked tails and the left-nesting shape the rustdoc promises. Its `with(42u64)` line duplicated the doctest and is gone. The prf hunk exercising `with` is removed. `IntoPrfContext for NonEmpty<T>` unwraps and delegates, so it cannot observe how the value was built; the aead hunk stays as the single end-to-end pin of the "encodes to the same bytes as the bare pair" claim. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
A generic consumer holding a `NonEmpty<C>` must be able to extend it without `C: MaybeEmpty` leaking into its own bounds; that is the reason `with` exists instead of `NonEmpty::new` on the pair. Every existing test called `with` on a concrete head, so adding `T: MaybeEmpty` to the method would have passed unnoticed. A generic helper over unconstrained `C` now fails to compile if that bound is ever introduced. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
92bb086 to
28ad006
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Standards
No documented-standard or maintainability findings. The rename is mechanically consistent and correctly marked as breaking; MaybeEmpty better communicates the trait’s capability semantics. Integer conversions are generated from one authoritative list, and with thoroughly documents left nesting, transparent encoding, unchecked tails, and AEAD’s untagged integer caveat. CI is green.
Spec
Changes are required because the implementation diverges from issue #313 in two material ways.
First, #313 specifies with<U> for any tail, with no check. Requiring U: MaybeEmpty excludes encoded contexts, nested proven contexts, and arbitrary downstream context types even though the method never calls is_empty and the proven head alone establishes the result’s invariant.
Second, #313 explicitly says both proposed changes are additive. Removing the public IsEmpty name is a separate breaking API change, regardless of whether MaybeEmpty is a better name. Please keep the existing name in this PR or split/propose the rename with an appropriate compatibility plan; alternatively amend the originating issue to make the changed contract explicit.
The ten integer conversions and tuple-encoding transparency otherwise match the issue.
|
Reviewed — no correctness findings. Two non-blocking notes:
let ctx = nonempty!("users/email");
ctx.with(row_id); // compiles, does nothing
let aad = Aad::from(ctx); // AAD binds only the field name, not the rowSilently narrower AAD than intended, no warning. Not a regression from this PR — nothing else in the file carries
The old name is gone from the public API entirely, so downstream code at 0.2.0 ( #[deprecated(since = "0.3.0", note = "renamed to `MaybeEmpty`")]
pub use non_empty::MaybeEmpty as IsEmpty;would keep existing impls and bounds compiling with a warning that names the replacement. Defensible to skip at 0.x with the |
…constructors `#[must_use]` #313 specifies `with<U>` for any tail with no check, and review made the case that the bound added in 8dee8ec bought nothing the method uses: `with` never evaluates `is_empty`, the head's proof alone makes the pair non-empty, and the bound's real effect was to make a downstream context type such as a `RecordId` newtype implement `MaybeEmpty` purely to be a tail it is never checked as. Already-encoded contexts and nested `NonEmpty` values are harmless tails for the same reason: the pair frames them once, as the tuple would. The bound is gone. `MaybeEmpty` stays exactly where the check runs, on `NonEmpty::new`. The `compile_fail` doctest becomes a positive one with a tail that implements nothing, a unit test pins the same for an opaque type and a nested `NonEmpty`, and the `MaybeEmpty` rustdoc scopes its "wrap the whole composite" rule to `new`. `NonEmpty<T>` is `Copy`, so `ctx.with(row_id);` as a bare statement compiled and did nothing, leaving the AAD bound to the column alone. `with`, `new`, `from_static` and `from_static_bytes` are now `#[must_use]`, so that line warns with a message saying why. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
The rename in 2d993fa removed the public name outright, so code written against 0.2.0 (`use vitaminc_protected::IsEmpty`, `impl IsEmpty for ..`, `T: IsEmpty`) would fail with "cannot find trait" and no pointer to the replacement. #313 also scoped this PR as additive. `pub use MaybeEmpty as IsEmpty`, deprecated, keeps every existing bound and impl compiling with a warning that names `MaybeEmpty`. A test under `#[allow(deprecated)]` pins that the alias binds, implements and feeds `NonEmpty::new`. The rename is now additive plus deprecation rather than a hard break; the alias can go in a later major. Claude-Session: https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd
|
Both notes taken, in 8cd5146 and 4274a82. |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The remediation resolves both prior blockers. NonEmpty::with<U> now accepts any tail without checking or requiring MaybeEmpty, while preserving the tuple encoding, and the deprecated IsEmpty re-export keeps existing bounds and downstream impls compiling. The integer conversions remain complete and the additional #[must_use] annotations are appropriate. No remaining spec or repository-standards concerns.
Closes #313.
What
NonEmpty::with<U: MaybeEmpty>(tail) -> NonEmpty<(T, U)>: pairs a proven head with a tail and keeps the proof, checking nothing. A pair is empty only when both halves are, so a non-empty head makes the pair non-empty whatever the tail,()or""included. The tail must beMaybeEmptyfor the same reasonNonEmpty::newrequires it, so an already-encodedAad/PrfContextor a nestedNonEmptyis rejected (pinned by acompile_faildoctest). The pair encodes exactly as the bare(T, U)would; a test invitaminc-aeadpinsnonempty!("users/email").with(42u64)against("users/email", 42u64).From<$int> for NonEmpty<$int>for the ten integer typesnever_empty!already covers:NonEmpty::from(7u64)/7u64.into()with no check.IsEmptyis renamedMaybeEmpty. The old name read as a marker ("values of this type are empty") when it is a capability ("this value can be asked whether it is empty"); integers implement it and are never empty. The methodis_emptyis unchanged.withdocuments that chaining nests left (a.with(b).with(c)is((a, b), c)), that it only extends rightwards, and three things the tail does not inherit: it is unchecked, an empty tail still frames as a pair, and integer tails encode untagged in AEAD.No encoding changes.
Why
A row-level consumer (
stack-encrypt) derives each field under a fixed context and lets the caller extend it with a record id, so a field is bound to its row as well as its column. Withoutwiththe only way to build that pair isNonEmpty::new, which returns aResultfor a case that cannot fail. The integer conversion removes a?on an infallible check at every call site that passes an id.The rename came out of review: the
U: MaybeEmptybound onwithonly makes sense once the trait name says what it provides rather than what it sounds like.Deferred
with_headmirror for layouts with the fixed part on the right (ContextTag::aad_with).NeverEmptymarker trait replacing the hand-listed integerFromimpls.Both non-breaking to add later.
https://claude.ai/code/session_019VRX1tXygj5bew3YhyK1B6
https://claude.ai/code/session_01QvrvBVdcecdr4LfywiGnDd