view: add OwnedView::reborrow() and ViewReborrow trait (#82) #92
view: add OwnedView::reborrow() and ViewReborrow trait (#82) #92tejas-dharani wants to merge 3 commits intoanthropics:mainfrom
Conversation
`OwnedView<V>` stored `V = FooView<'static>` — a soundness fiction where
the actual borrows point into the internal `Bytes` buffer. `Deref` exposed
`&FooView<'static>`, making field borrows appear `'static` to the compiler.
This made it impossible to write an RPC handler that returns a view field
tied to the `OwnedView`'s real lifetime.
Add `ViewReborrow` — an `unsafe trait` with a GAT:
pub unsafe trait ViewReborrow: MessageView<'static> {
type Reborrowed<'b>: MessageView<'b, Owned = ...>;
}
And `OwnedView::reborrow()`:
pub fn reborrow<'b>(&'b self) -> &'b V::Reborrowed<'b>
The implementation uses a pointer cast (`*const V` → `*const V::Reborrowed<'b>`)
rather than `transmute_copy`, so no value is duplicated and there is no
double-free risk for views containing heap-owning fields (RepeatedView,
MapView, MessageFieldView, UnknownFieldsView).
Codegen emits `unsafe impl ViewReborrow` automatically for every generated
view type. Hand-written view types opt in with `unsafe impl`.
- buffa/src/view.rs: ViewReborrow trait + OwnedView::reborrow() + tests
- buffa/src/lib.rs: re-export ViewReborrow; fixture unsafe impl
- buffa-codegen/src/view.rs: emit unsafe impl ViewReborrow per view type
- buffa-types/src/generated/: regenerated WKTs (task gen-wkt-types)
- buffa-test/src/tests/view.rs: end-to-end integration tests
|
All contributors have signed the CLA ✍️ ✅ |
Three fixes from review of anthropics#92: H1 — `OwnedView::reborrow` now contains an inline-const `size_of`/ `align_of` guard that catches mistyped `unsafe impl ViewReborrow` at monomorphization rather than as latent UB at the cast site. The trait's `# Safety` doc previously claimed such a guard existed; that's now true. The doc also calls out explicitly that the same-struct requirement is enforced *only* by the contract — the GAT bound and layout guard rule out subsets of the misuse, but a different struct that happens to share layout and `Owned` is still UB. M1 — codegen-emitted `unsafe impl ViewReborrow` previously had a `// SAFETY:` comment in the codegen template that quote! silently drops. Switched to a `///` doc comment so the safety rationale survives into the generated source (visible in every regenerated buffa-types __view.rs file). Per project convention every `unsafe` needs a SAFETY justification in user-visible source. Ergonomics M2/M3 — `docs/guide.md` "OwnedView in async trait implementations" gains a "Returning a borrow into the request: reborrow()" subsection showing the failing case and the fix; the `OwnedView` rustdoc gains a one-paragraph breadcrumb pointing users who hit `error[E0597]: borrowed value does not live long enough` at `reborrow()`. Buffa-types view files regenerated to pick up the SAFETY doc-attr. All 605 lib + 289 e2e + 9 doctests pass; clippy clean.
|
[claude code] Pushed a fixup commit ( H1 — inline-const layout guard ( M1 — surviving SAFETY comment in generated code ( Ergonomics M2/M3 — guide section + rustdoc breadcrumb ( Verified: workspace check clean, 605 lib + 289 e2e + 9 doctests pass, clippy Lows from review (move-while-borrowed compile_fail doctest, variance compile-time assertion, |
|
Posted #95 as a draft alternative formulation that makes |
|
closing in favor of #95 |
**Alternative to #92 — for review/comparison only.** This PR is a draft to show what the `OwnedView::reborrow` mechanism looks like as a *safe* trait. Intended as decision material; if we land this we should close #92, otherwise close this and keep #92. @tejas-dharani — would you take a look? Curious whether you considered this shape and rejected it, or whether it's worth swapping in. I think the mechanical soundness check at impl-site is a strict win, but I want your read since you wrote the original. ## What changes `ViewReborrow` becomes a safe trait whose method body coerces `&'b Self` (= `&'b FooView<'static>`) into `&'b Self::Reborrowed<'b>` (= `&'b FooView<'b>`). For covariant view types — which all generated views are — Rust's lifetime variance accepts the body `this` as a plain subtype coercion. No `unsafe`, no pointer cast, no `SAFETY` rationale. ```rust // before (#92) pub unsafe trait ViewReborrow: MessageView<'static> { type Reborrowed<'b>: MessageView<'b, Owned = ...>; } // after (this PR) pub trait ViewReborrow: MessageView<'static> { type Reborrowed<'b>: MessageView<'b, Owned = ...> where Self: 'b; fn reborrow<'b>(this: &'b Self) -> &'b Self::Reborrowed<'b>; } ``` Generated code (per view) goes from: ```rust // before // SAFETY: Reborrowed<'b> is the same generated struct ... unsafe impl ::buffa::ViewReborrow for FooView<'static> { type Reborrowed<'b> = FooView<'b>; } ``` to: ```rust // after impl ::buffa::ViewReborrow for FooView<'static> { type Reborrowed<'b> = FooView<'b>; fn reborrow<'b>(this: &'b Self) -> &'b Self::Reborrowed<'b> { this } } ``` `OwnedView::reborrow` collapses to one line: ```rust pub fn reborrow<'b>(&'b self) -> &'b V::Reborrowed<'b> where V: ViewReborrow, { V::reborrow(&self.view) } ``` ## Why this is sound `FooView<'a>` is covariant in `'a` if every field uses `'a` covariantly — `&'a str`, `&'a [u8]`, `RepeatedView<'a>`, `MessageFieldView<'a>`, `PhantomData<&'a ()>`, etc. all qualify. Generated views are uniformly covariant. For a covariant `FooView`, `'static : 'b` (always) gives us `FooView<'static>: FooView<'b>` (subtyping). Then `&'b FooView<'static>: &'b FooView<'b>` (since `&'b _` is covariant in the referent). The body `this` flows into the return slot via this exact subtyping. If a hand-written view type is *invariant* (has `Cell<&'a T>`, `&'a mut T`, `fn(&'a T)` somewhere), the body fails to compile and the impl is rejected — which is precisely what should happen, because narrowing an invariant lifetime is unsound. Compare with #92's design, where the same hand-written view can write a buggy `unsafe impl ViewReborrow` and produce UB; here the compiler catches it for them. ## What goes away - `unsafe trait` keyword on `ViewReborrow` - The pointer cast in `OwnedView::reborrow` - The inline-const `size_of`/`align_of` guard added in #92's review fixes - The `SAFETY:` doc attribute on every generated `impl` - Hand-written impls on test fixtures stop needing `unsafe impl` ## What stays the same - The user-facing API: `owned_view.reborrow()` returns `&'b V::Reborrowed<'b>` exactly as in #92. - All tests from #92 pass without modification (605 lib + 289 e2e + 9 doctests; clippy `-D warnings` clean). - Same `#[diagnostic::on_unimplemented]` notes (text adjusted to drop the `unsafe impl` and add the body). - Same `compile_fail,E0597` doctest. - Same `Send`/`Sync`/variance test scaffolding. ## Diffstat vs current PR #92 (post-fixup) Net **−34 lines** in the buffa crate; the codegen template gets simpler by losing the SAFETY doc-attr and one source line; the trait gains one method. ## Decision needed If you prefer this shape, I'll: - Mark this ready, transfer the PR description / `Closes #82` over. - Close #92 with a note pointing here. If you prefer #92's shape (e.g. for symmetry with `unsafe trait DefaultInstance` from before #68, or because the explicit pointer cast better signals what's happening), I'll close this and we land #92 as-is. References: - [Original PR #92](#92) - Issue [#82](#82) - Prior art for the same pattern: PR [#68](#68) (`DefaultViewInstance` unsafe → safe via covariance) --------- Co-authored-by: Tejas Dharani <tejas.dharani10@gmail.com>
2 participants
Description:
Closes #82.
OwnedView stores V = FooView<'static> — a soundness fiction required to keep the view
self-contained alongside its Bytes buffer. Deref exposes &FooView<'static>, so field borrows appear
'static to the compiler. This makes it impossible to write a handler that returns a view field tied
to the OwnedView's real lifetime.
Fix
New unsafe trait ViewReborrow with a GAT type Reborrowed<'b> and a new OwnedView::reborrow<'b>(&'b
self) -> &'b V::Reborrowed<'b> method. The implementation uses a pointer cast (*const V → *const
V::Reborrowed<'b>) rather than transmute_copy — no value is duplicated, so there is no double-free
risk for views containing heap-owning fields (RepeatedView, MapView, MessageFieldView,
UnknownFieldsView). The cast is sound because ViewReborrow's safety contract requires Reborrowed<'b>
to be the same struct with only its lifetime parameter shortened, and OwnedView's constructor
invariant ensures all borrows point into self.bytes.
Codegen emits unsafe impl ViewReborrow automatically for every generated view type. Hand-written
view types opt in with unsafe impl.
Tests
Unit tests in buffa/src/view.rs cover pointer-identity (no copy), simultaneous reborrows, and a
compile_fail,E0597 doctest confirming the borrow checker rejects escapes. End-to-end tests in
buffa-test/src/tests/view.rs exercise the full generated PersonView: scalars, bytes, repeated,
nested message, oneof, and the motivating RPC handler pattern.
buffa-types/src/generated/ regenerated (task gen-wkt-types).