Repository navigation
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2e2ec60-4490-44b2-8717-aad478da5b0a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2e2ec60-4490-44b2-8717-aad478da5b0a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2e2ec60-4490-44b2-8717-aad478da5b0a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2e2ec60-4490-44b2-8717-aad478da5b0a
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #645 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 473 473
Lines 45493 45493
=======================================
Hits 45493 45493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Three non-blocking stabilization gaps remain in the proposed bytesbuf 1.0 checklist; this documentation-only PR leaves the paired public API surface unchanged.
Coverage: API design, correctness, tests, performance, naming, telemetry, resilience, consistency, public API, and public docs all completed at cf75956bf67d44c381889e1900933fa9fdb6417c; the public API comparison was byte-identical.
|
|
||
| Stabilize the provider contracts needed by consumers and I/O implementations: | ||
|
|
||
| - `Memory` for reserving owned writable capacity. |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
Memory stabilization omits allocation-failure semantics
Problem
The proposed stable boundary includes Memory, but the checklist does not decide whether reserve remaining infallible - with implementations permitted to panic or abort when capacity is unavailable - is intentional.
Why this matters
Changing this contract after 1.0 to return a typed allocation error would break downstream implementations and callers, while the current shape cannot represent recoverable resource pressure.
Suggested fix
Add an explicit stabilization decision: either document panic/abort as the stable invariant or define a fallible reservation path before 1.0.
|
|
||
| - [ ] Explicitly list and review every method included for `BytesBuf` and | ||
| `BytesView`. | ||
| - [ ] Confirm the writable-slice and vectored-write APIs support the approved |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
The checklist depends on an unavailable approved I/O design
Problem
This item requires validation against "the approved owned-buffer Read/Write design," but the newly added docs/DESIGN.md is empty and no repository document here defines or links that design.
Why this matters
Reviewers cannot independently determine the contract that the writable-slice and vectored-write APIs must satisfy before stabilization.
Suggested fix
Document the approved design in DESIGN.md and link it here, or link the existing authoritative design and remove the empty placeholder.
| - `BytesBuf` for assembling and owning writable byte sequences. | ||
| - `BytesView` for sharing and consuming immutable byte sequences. | ||
| - The iterator and cursor types returned by their stable operations: | ||
| `BytesBufRemaining`, `BytesBufVectoredWrite`, and `BytesViewSlices`. |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
BytesBufRemaining diverges from the slice-iterator family
Problem
The proposed stable list includes BytesBufRemaining, which is returned by BytesBufVectoredWrite::slices_mut(), while the sibling immutable iterator is named BytesViewSlices.
Why this matters
Stabilizing the current name makes the related immutable and mutable slice iterators harder to discover and understand as one API family.
Suggested fix
Consider renaming it to BytesBufSlicesMut before stabilization and update user-facing references.
Martin Taillefer (geeknoid)
left a comment
There was a problem hiding this comment.
Early feedback on the draft, from a static review of the full diff and the bytesbuf API blast radius. Nothing was built or executed.
Headline: the proposed 1.0 boundary is not yet self-contained: MemoryShared publicly inherits thread_aware::ThreadAware, while thread_aware is still 0.8.0. Under Rust API Guidelines C-STABLE, that public dependency must stabilize first or be removed from the stable bytesbuf surface.
Two additional release-readiness gaps are called out inline. The checklist needs an approved, feature-specific API manifest rather than a methods-only inventory, so trait impls, auto traits, feature availability, and unsafe contracts cannot drift unnoticed. Also, if Block remains necessary for custom providers, its pre-existing unsafe constructor contract needs to state the pointer validity and lifetime premises the implementation relies on before it can be stabilized.
The proposed core/deferred type split otherwise matches the current exports: BlockMeta is genuinely exposed by both iterator families and metadata accessors; standard-I/O and bytes adapters are correctly separated for review; and no new unsafe code or runtime behavior is introduced. The temporary stabilization journal is also packaged with the crate, contrary to the repository's adopted M-NO-META-DESIGN-DOCUMENTATION rule; move it outside packaged crate docs or replace it with the finalized end-state contract.
I did not repeat the three existing comments about allocation-failure semantics, the missing approved I/O design, and BytesBufRemaining naming. All five taxonomies were assessed: 820/820 classes, with no unassessed classes.
| Stabilize the provider contracts needed by consumers and I/O implementations: | ||
|
|
||
| - `Memory` for reserving owned writable capacity. | ||
| - `MemoryShared` and `HasMemory` for sharing an endpoint-compatible provider. |
There was a problem hiding this comment.
The proposed stable trait leaks a pre-1.0 public dependency — Conformance · Medium · High
MemoryShared is proposed as stable here, but its declaration publicly inherits thread_aware::ThreadAware (source), and the workspace resolves thread_aware as 0.8.0 (manifest). The Rust API Guidelines' C-STABLE rule says a stable crate cannot expose types from an unstable public dependency. Releasing bytesbuf 1.0 in this shape therefore couples its stable contract to a dependency that may still make 0.x breaking changes.
Direction: stabilize thread_aware first, remove it from the public supertrait boundary, or explicitly version the two stable contracts together.
Done when: every dependency visible in the approved bytesbuf 1.0 API is itself stable, or the exposed dependency has been redesigned out of the public surface.
|
|
||
| ## Pending review | ||
|
|
||
| - [ ] Explicitly list and review every method included for `BytesBuf` and |
There was a problem hiding this comment.
A methods-only inventory cannot define the 1.0 compatibility contract — Testing · Medium · High
The public contract is larger than the inherent methods named here: BytesView currently has Default, Eq, Hash, several PartialEq forms, From<Vec<u8>>, feature-gated Read/BufRead, and bytes compatibility impls; Cargo features also change which API exists. A normal SemVer comparison can detect drift from a previous release, but it cannot decide which current items, impls, auto traits, feature profiles, and safety contracts were intentionally approved for 1.0. Without a committed approved snapshot, an omitted impl or feature can silently become permanent at release.
Direction: generate a public-API manifest for default, no-default, and relevant feature combinations; annotate every item/impl as stable or deferred; include public unsafe contracts and auto-trait expectations; gate the 1.0 release against that manifest.
Done when: the approved snapshots cover all feature profiles and a deliberate mutation such as removing an approved impl or exposing a deferred item makes the compatibility gate fail.
| - [ ] Decide whether `BytesBufWriter` belongs in the stable standard-library | ||
| adapter surface enabled by the `std` feature. | ||
| - [ ] Decide which `bytes` compatibility implementations are stable. | ||
| - [ ] Determine the minimal stable API for implementing custom memory providers, |
There was a problem hiding this comment.
The optional custom-provider surface has an incomplete unsafe contract — Correctness · Medium · High
This PR correctly notes that Block may have to remain public, but the pre-existing Block::new safety section only requires exclusive ownership. The implementation subsequently treats ptr as storage for len bytes and constructs mutable slices from it (sink), which additionally requires a live allocation of sufficient extent whose lifetime is retained by block_ref. A caller can satisfy the documented exclusivity condition while violating those unstated premises. This contract predates the PR, but admitting Block into the stable boundary makes it release-relevant.
Direction: either replace raw Block construction with a safe provider-specific constructor, or document and review every pointer, extent, initialization, ownership, and lifetime invariant before stabilization.
Done when: every public unsafe item admitted to 1.0 passes the five-part unsafe review and its documented preconditions are sufficient to justify every downstream unsafe operation.
| @@ -0,0 +1,76 @@ | |||
| # Stabilization Notes | |||
|
|
|||
| These notes capture the pending decisions for stabilizing `bytesbuf`. They | |||
There was a problem hiding this comment.
The published crate would ship a temporary design journal — Conformance · Low · High
This file explicitly records pending proposals rather than the finalized contract, while the workspace package allowlist includes every docs/**/*.md file (package policy). The repository-adopted M-NO-META-DESIGN-DOCUMENTATION rule requires crate documentation to describe the end state rather than the design journey, because process notes go stale and distract users.
Direction: keep the working stabilization checklist in a non-packaged project location, then replace it with durable user-facing design/compatibility documentation once decisions are final.
Done when: the published crate contains only the finalized contract, while the pending decision log remains available to maintainers outside the package.
Summary
Adds pending stabilization notes for the
bytesbufcrate based on O365 Core work item 7688114. The notes record the proposedBytesBuf,BytesView, owned-buffer I/O, and memory-provider stable boundary, along with deferred low-level APIs and unresolved review items. Also adds an emptydocs/DESIGN.mdplaceholder.Validation
just package=bytesbuf formatjust package=bytesbuf readmecargo build -p bytesbufcargo test -p bytesbufgit diff --checkcargo-spellcheckprocess exits during startup on this machine; the GitHub spell-check job passed.Review
BlockMetaexposure points were addressed.