Introduce unstable #[zerocopy(invariant(...))] library invariant checking

[jswrenn]

---

• 　  Implement common traits for ReadOnly #3682
• 👉 Introduce unstable #[zerocopy(invariant(...))] library invariant checking #3681

Latest Update: v2 — Compare vs v1

📚 Full Patch History

Links show the diff between the row version and the column version.

Version	v1	Base
v2	vs v1	vs Base
v1		vs Base

⬇️ Download this PR

Branch

git fetch origin refs/heads/G2cef62dbb236b18953ded9a3d514bf0913c0e49c && git checkout -b pr-G2cef62dbb236b18953ded9a3d514bf0913c0e49c FETCH_HEAD

Checkout

git fetch origin refs/heads/G2cef62dbb236b18953ded9a3d514bf0913c0e49c && git checkout FETCH_HEAD

Cherry Pick

git fetch origin refs/heads/G2cef62dbb236b18953ded9a3d514bf0913c0e49c && git cherry-pick FETCH_HEAD

Pull

git pull origin refs/heads/G2cef62dbb236b18953ded9a3d514bf0913c0e49c

Stacked PRs enabled by GHerrit.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.90%. Comparing base (4fbb0b6) to head (32a19cb).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3681   +/-   ##
=======================================
  Coverage   91.90%   91.90%           
=======================================
  Files          20       20           
  Lines        6175     6177    +2     
=======================================
+ Hits         5675     5677    +2     
  Misses        500      500           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a2d740b5d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve TryFromBytes when skipping FromBytes

When this branch is reached through most_traits, skip_on_error converts the rejection into an empty token stream, but most_traits relies on derive_from_bytes to generate its nested TryFromBytes implementation. Consequently, an invariant-bearing type using #[derive(most_traits)] implements neither FromBytes nor the compatible TryFromBytes trait, so its invariant is never usable. Have most_traits separately invoke derive_try_from_bytes for invariant-bearing inputs while continuing to skip the infallible traits.

Useful? React with 👍 / 👎.

[jswrenn]

Confirmed. most_traits now explicitly derives TryFromBytes for invariant-bearing inputs while continuing to skip FromBytes and FromZeros. Added struct, enum, and union regressions that check trait availability and acceptance/rejection by the invariant.

Implemented in 32a19cb2f (originally prepared as local amend c1dc32796152180aa0bd66465fba53648d12f3ec).

Tests: With RUSTFLAGS="--cfg zerocopy_unstable_ptr", ./cargo.sh +all test -p zerocopy-derive --test invariant --test hygiene --test on_error passed on MSRV, stable, and nightly (16 invariant tests and the existing hygiene regression on each; the on-error target provides compile-time assertions). The complete nightly derive suite and the repository pre-push checks also passed.

Update: the fix is now present in the published PR head linked above. The tests reported here ran on the original local amend; the derive sources and tests are unchanged in the published commit. Leaving this thread unresolved as requested.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[chatgpt-codex-connector]

Emit the invariant feature gate on skipped derives

For an unsupported TryFromBytes input with #[zerocopy(on_error = "skip")]—for example, a field-bearing enum without repr(C) or an integer repr—derive_try_from_bytes_enum returns an empty token stream before any ImplBlockBuilder calls this method. Such a crate compiles with #[zerocopy(invariant(...))] even when zerocopy_unstable_ptr is absent, contrary to the documented requirement and the new UI-test intent that skipped derives still require experimental cfgs. Ensure every skip exit returns ctx.cfg_compile_error() rather than an entirely empty stream.

Useful? React with 👍 / 👎.

[jswrenn]

Confirmed. The unsupported-enum exit now uses ctx.error_or_skip(e), preserving the experimental cfg diagnostics instead of returning an empty token stream. Added a no-repr enum to the cfg UI test and an enabled-cfg compile-time regression verifying that its unsupported derive is still skipped.

Implemented in 32a19cb2f (originally prepared as local amend c1dc32796152180aa0bd66465fba53648d12f3ec).

Tests: ./cargo.sh +all test -p zerocopy-derive --test ui passed on MSRV, stable, and nightly, including the new missing-cfg diagnostics. Regenerated expectations with ../tools/update-expected-test-output.sh. The complete nightly derive suite with zerocopy_unstable_ptr and the repository pre-push checks also passed.

Update: the fix is now present in the published PR head linked above. The tests reported here ran on the original local amend; the derive sources and tests are unchanged in the published commit. Leaving this thread unresolved as requested.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[chatgpt-codex-connector]

Keep generated candidate names out of invariant resolution

Invariant expressions are expanded directly inside scopes containing generated bindings such as the candidate method parameter and this candidate_ import. Therefore a valid expression that calls an in-scope helper named candidate() or candidate_() resolves to the generated pointer or core::mem::drop instead and fails to compile; enum-local names such as tag have the same problem. Isolate generated identifiers hygienically so that only the documented field bindings can shadow names referenced by the user's expression.

Useful? React with 👍 / 👎.

[jswrenn]

Confirmed. The generated candidate parameter and enum tag now use mixed-site identifiers, and the extra candidate import/rebinding has been removed. Candidate spelling still avoids field names so field imports cannot shadow the source pointer. Added struct, enum, and union regressions calling outer helpers named candidate, candidate_, and tag, while retaining the existing colliding-field-name tests.

Implemented in 32a19cb2f (originally prepared as local amend c1dc32796152180aa0bd66465fba53648d12f3ec).

Tests: The name-collision regression failed against the original implementation and now passes. With RUSTFLAGS="--cfg zerocopy_unstable_ptr", the invariant and hygiene targets passed on MSRV, stable, and nightly (16 invariant tests plus the existing hygiene regression per toolchain). The complete nightly derive suite, regenerated output snapshots, nightly Clippy, and repository pre-push checks also passed.

Update: the fix is now present in the published PR head linked above. The tests reported here ran on the original local amend; the derive sources and tests are unchanged in the published commit. Leaving this thread unresolved as requested.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

Follow-up on current head 32a19cb2f: the candidate parameter and local enum tag are now hygienic, but invariant expressions still sit inside scopes containing other implementation-only call-site names.

is_safe still declares the literal type parameter ___ZcAlignment, so an invariant that intends an outer ___ZcAlignment type resolves to the derive's generic parameter instead. Enums have the same problem with generated items: generate_tag_consts creates ___ZEROCOPY_TAG_<Variant> using variant.span(). For example, an outer fn ___ZEROCOPY_TAG_A() -> bool { true } plus #[zerocopy(invariant(___ZEROCOPY_TAG_A()))] on variant A resolves to the generated tag constant rather than the caller's function.

Please freshen or otherwise isolate every implementation-only identifier visible from an invariant expression, not just these two local bindings, and add regressions for at least a method-generic collision and an enum tag-constant collision. Span::mixed_site() is sufficient for the local bindings, but it should not be assumed to hide item or type names.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Suggested change

	propagate to the caller.
	Only the selected enum variant's fields and invariants are checked.
	propagate to the caller. Only the selected enum variant's fields and
	invariants are checked.

[jswrenn]

I missed this formatting suggestion in the earlier pass. The current PR still splits these two sentences across separate source lines, so this was not applied. Your proposed wrapping preserves the wording and is appropriate; leaving this thread open. No code or test changes were made for this comment.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

Please apply this – we need to reflow everything consistently.

[joshlf]

No semantic implication here. This is only source formatting: the proposed reflow preserves the exact wording and behavior.

It is worth fixing for consistency, but it does not affect the generated code, API contract, or soundness argument.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Is this necessary?

[jswrenn]

The rebinding is no longer needed with the revised lowering. It and the accompanying candidate import were removed in 32a19cb2f. The method parameter now uses a mixed-site identifier whose spelling avoids field names, and field projections use that parameter directly.

The invariant and hygiene regressions passed on MSRV, stable, and nightly with RUSTFLAGS="--cfg zerocopy_unstable_ptr"; the full nightly derive suite also passed. Those tests ran on the original local amend c1dc32796; the derive sources and tests are unchanged in the published commit linked above. Leaving your thread unresolved for your review.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

Test-coverage pass: the current suite covers the main container kinds, field ordering and short-circuiting, return containment, basic DST bit-validity, generics, non-Immutable union fields, experimental-cfg gating, and several hygiene cases. I would add the following before treating the feature as exhaustively covered:

1. Panic propagation. Add a struct test whose invariant panics and assert that catch_unwind observes the panic. Also add a union case whose first field invariant panics while a later field would pass, to prove that union fallback does not turn a panic into false and continue.
2. An invariant on the DST field itself. Unsized currently puts the invariant on a; the [bool] tail only exercises bit validation. Put an invariant on b that inspects its metadata/elements (and preferably also a) so the user-visible Ptr for a dynamically sized current field is exercised.
3. Nested invariant-bearing fields. Have Outer contain an Inner with its own invariant, then verify that an Inner invariant failure prevents Outer's field invariant and later-field validation from running. This locks in compositional validation through ReadOnly<T>::TryFromBytes rather than testing only primitive field validators.
4. Mutable conversion semantics. Exercise an invariant-bearing IntoBytes type end-to-end through try_mut_from_bytes: invalid input should be rejected, valid input should be accepted, and the returned value should then be mutable into a state that violates the invariant. That last case is part of the documented contract that invariants validate conversions rather than constrain later mutation.
5. A multi-variant repr(C) enum. CEnum has only one fieldful variant. Add two fieldful variants with different predicates so the C-layout variant-to-field projection path is tested independently of the existing multi-variant primitive-repr enum.
6. ZST/overlapping projections. Add a named zero-sized field with an invariant before another field, so multiple retained shared Safe field projections can have the same address. This would be a useful Miri regression case for the new behavior of keeping prior field pointers in scope.

I am not repeating the existing review threads for most_traits, skipped experimental-cfg emission, or generated-name hygiene; those already identify separate missing cases.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Generated-syntax audit on exact head 6a2d740b5d810f435af98a5cd18a0a989a02ebd4. I traced the new invariant expression lowering through the projection generator, generic/enum helper types, identifier hygiene, attribute grammar, and the accepted syn AST surface. I am not duplicating the known repository-wide derive unsoundness tracked by #2762 and #388; those risks predate this PR.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

These poison imports are block-scoped items, so each import is in scope from the start of this block, not only from its textual position. As a result, names for later fields affect earlier invariants even though the documented DSL exposes only the current and preceding fields.

For example, with an outer fn b(_: bool) { panic!() }, an invariant on a such as { b(false); true }, and a later field named b, the generated use core::mem::drop as b makes that earlier call resolve to drop(false). The invariant silently returns true instead of propagating the intended panic. This is stronger than a diagnostic-only collision: generated syntax changes the executed predicate.

Please arrange the lowering so a field name enters the invariant's lexical scope only after that field has been validated. In particular, avoid block-wide item aliases for future fields; add a regression where an earlier invariant refers to an outer helper whose name matches a later field.

Authored by an AI agent acting on Josh's behalf.

[jswrenn]

This remains unaddressed. The current generator still emits all field-name use ... as ... items in one block. Removing the separate candidate alias only addresses the generated candidate-name collision; it does not prevent an import for a later field from affecting an earlier invariant. The field imports need narrower scopes, with a regression for an earlier invariant calling a helper named after a later field. The existing name-collision tests do not cover that case, so their passing results do not establish that this concern is fixed. Leaving this thread open.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

The implication here is stronger than a name-resolution nuisance: this can undermine the safety purpose of invariant(...).

A caller can use an invariant to establish a library invariant that later safe methods rely on when they execute unsafe internally. If a later field's generated use ... as ... changes what an earlier invariant expression calls, the derive can evaluate a different predicate from the one the author wrote. That can let TryFromBytes accept bytes that violate the intended library invariant. A later safe method may then reach unsafe code with its preconditions false.

So I think this is a real soundness issue, not just a hygiene or diagnostics issue. The fix needs to ensure that later fields cannot affect name resolution in earlier invariants.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

I prototyped a minimal witness for the soundness consequence here. The key is that the later field can suppress the check that a safe method later relies on:

fn require_nonzero(ok: bool) {
    assert!(ok);
}

#[derive(TryFromBytes, KnownLayout, Immutable)]
#[repr(C)]
struct Victim {
    #[zerocopy(invariant({
        require_nonzero(read(a) != 0);
        true
    }))]
    a: u8,

    require_nonzero: u8,
}

impl Victim {
    fn nonzero(&self) -> NonZeroU8 {
        // SAFETY: safe construction is supposed to establish `a != 0`.
        unsafe { NonZeroU8::new_unchecked(self.a) }
    }
}

On the current lowering, the second field emits use core::mem::drop as require_nonzero; as a block-scoped item. That import is already in scope inside the earlier invariant, so require_nonzero(read(a) != 0) resolves to drop(false) when a == 0. The invariant then evaluates its final true, and the safe TryFromBytes conversion can admit [0, 0].

That admitted value violates the premise used by the safe nonzero method. Calling nonzero() then executes NonZeroU8::new_unchecked(0), whose safety precondition is false. Under Miri, that final call is a direct UB witness; the successful safe conversion is already the witness that the derive admitted a state outside the abstraction invariant.

So the failure mode is: safe byte ingress -> generated name capture changes the predicate -> invalid library state is admitted -> safe API reaches unsafe with a false precondition. This is why I think this needs to be treated as a soundness fix rather than only a hygiene fix.

I prepared this as a regression test against head 32a19cb2f; this environment does not have a Rust toolchain installed, so I have not executed the test locally here.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

The field-name DSL should not depend on the source identifier's syntax context. Cloning field.ident also clones its hygiene context, but the same-spelling identifier inside #[zerocopy(invariant(...))] can have a different context when an enclosing macro constructs the item. In that case the invariant token is not guaranteed to resolve to this generated local; it can fail to bind here or resolve to another same-spelling call-site name.

This repository already has __test_hygienically_mixed_into_bytes specifically to construct identically printed def_site/call_site identifiers. Please add the analogous adversarial fixture for an invariant-bearing named field, with the field declaration and invariant reference deliberately given different contexts, and lower field references according to the invariant DSL's semantic field names rather than assuming their original spans make ordinary local-variable lookup work.

Authored by an AI agent acting on Josh's behalf.

[jswrenn]

This remains unaddressed. The generator still clones field.ident for named field bindings. The earlier fix gives generated candidate/tag locals mixed-site identifiers, but it does not change the relationship between a caller-supplied field declaration’s syntax context and identifiers in its invariant. I have not added or run the requested adversarial mixed-context fixture, so I cannot claim that the current field-name binding behavior handles it. Leaving this separate hygiene concern open.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

The implication is similar, but the evidence is weaker until we have the adversarial macro test.

The feature promises that a field name inside invariant(...) refers to that field's validated pointer. If macro hygiene lets the invariant token and the generated binding have different syntax contexts, that promise may fail: the expression can fail to bind to the field or bind to some other same-spelling name.

If it merely fails to compile, this is a usability bug. If it resolves to another value and the predicate returns true, it becomes soundness-relevant for the same reason as the later-field collision: TryFromBytes can accept bytes without actually checking the library invariant that downstream unsafe code relies on.

I would therefore treat this as a soundness blocker until the mixed-context fixture shows which behavior rustc actually gives us.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

I reduced this to a concrete mixed-context PoC.

The proc-macro fixture gives the struct field named field a def_site span, but gives the field token inside #[zerocopy(invariant(...))] a call_site span. At the invocation site it also defines a call-site field value for which the invariant returns true.

The current derive clones field.ident for the validated-pointer binding, so that binding keeps the field declaration's def-site context. The invariant expression keeps its call-site context. Thus the field in the invariant can resolve to the call-site value instead of the validated field pointer. The PoC chooses those values so the actual byte is 0 while the call-site value makes the invariant succeed. A safe TryFromBytes conversion can therefore admit a value whose intended library invariant is false; a later safe method that relies on that invariant can then reach unsafe with a false precondition.

I was not able to execute this fixture locally: this environment has no Rust toolchain, and its network restrictions prevented installing one. So this is a source-level PoC and name-resolution argument, not an observed compiler run. I still think it is sufficient to treat the mixed-context case as soundness-relevant, but an in-repo nightly regression would close the remaining empirical gap.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Meta::List does not imply parentheses; Syn preserves (), [], and {} as distinct MacroDelimiters. This therefore accepts #[zerocopy(invariant[true])] and #[zerocopy(invariant{true})], even though the public syntax and this parser's own diagnostics specify invariant(...).

If parentheses are the intended grammar, require MacroDelimiter::Paren here and add brace/bracket UI cases. Otherwise the broader grammar should be documented and tested explicitly rather than accepted accidentally by the AST representation.

Authored by an AI agent acting on Josh's behalf.

[jswrenn]

The parser still accepts Meta::List without inspecting its delimiter, while its diagnostics and public examples specify invariant(...). The previous fix did not change this. I would enforce the documented parentheses grammar with a MacroDelimiter::Paren check and brace/bracket UI regressions; accepting the broader grammar instead needs an explicit documentation decision. No delimiter regression was added or run in the earlier pass. Leaving this thread open.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

The practical implication here is API/grammar consistency, not memory safety by itself.

Today the parser accepts invariant(...), invariant[...], and invariant{...} because all three arrive as Meta::List, even though the documentation and diagnostics describe only the parenthesized form. Once accepted, they all parse the enclosed tokens as the same Rust expression, so this does not appear to change the safety check that runs.

The risk is that we accidentally make undocumented syntax part of the de facto interface, then have to support it or break users later. I would still fix or explicitly document it, but I would not block this PR on soundness grounds for this item alone.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Independent bug-finding pass: one additional issue found below. I avoided repeating the existing most_traits, feature-gate, generated-name, future-field-scope, hygiene, and delimiter findings.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Caller-authored invariant code is inserted under ImplBlockBuilder's derive-wide const_block, which carries #[allow(deprecated, non_snake_case, non_local_definitions, ...)]. Unlike ordinary generated glue, the invariant expression is executable source supplied by the caller and is only semantically checked after this expansion. The derive can therefore silently weaken the caller's lint policy—for example, a crate with #![deny(deprecated)] can call a deprecated helper solely from #[zerocopy(invariant(...))], while the enclosing allow(deprecated) suppresses that error.

Please keep caller-authored invariant expressions outside the blanket generated-code lint suppressions, or narrow those suppressions so they do not cover the expression. Add a UI regression with at least deny(deprecated).

Authored by an AI agent acting on Josh's behalf.

[jswrenn]

This remains unaddressed. The invariant expressions are still emitted inside the impl wrapped by const_block, and that wrapper still carries allow(deprecated) and the other broad lint allowances. The candidate-name hygiene fix did not narrow those attributes or move caller expressions outside them. The requested deny(deprecated) UI regression was not part of the prior test run; narrowing generated-code lint suppression still needs a separate fix. Leaving this thread open.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

The implication here is that caller-written executable code is being compiled under a different lint policy than the caller requested.

For example, #![deny(deprecated)] normally makes a deprecated call a hard error, but the derive's surrounding #[allow(deprecated)] can make the same call compile when it appears inside invariant(...). The same principle applies to the other blanket allowances: they were intended for generated glue, but they also cover user code copied into that glue.

I do not see a direct memory-safety hole from this alone. These attributes suppress lints, not Rust's type system, validity rules, or unsafe-context checks. The concrete problem is that the macro can silently defeat project policy and admit code the caller explicitly asked rustc to reject. I would fix it, but classify it separately from the soundness blockers above.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Round 2 found one additional behavioral-contract issue. I am not repeating any previously reported findings.

The new invariant expressions are arbitrary executable user code. They may mutate external state as well as panic; for example, an invariant can increment an AtomicUsize and return true. However, the existing TryFromBytes conversion entry points are still annotated #[must_use = "has no side effects"]. That diagnostic is now false for invariant-bearing types: merely calling try_ref_from_bytes, try_mut_from_bytes, try_read_from_bytes, and the prefix/suffix variants can execute user side effects even when their result is discarded.

Please update the affected TryFromBytes must_use reasons (or otherwise restrict/document invariant effects if purity is intended). A regression with a side-effecting invariant would also make the changed behavior explicit. The new field-invariant documentation should mention arbitrary side effects if they are intentionally supported, alongside the existing panic behavior.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

A second new round-2 issue is in test integration rather than product semantics.

The new positive/runtime test target zerocopy-derive/tests/invariant.rs is guarded wholesale by #![cfg(zerocopy_unstable_ptr)]. CI runs tests through ./cargo.sh, whose get_rustflags currently injects zerocopy_unstable_linux, zerocopy_derive_union_into_bytes, and internal cfgs, but not zerocopy_unstable_ptr. The workflow-level RUSTFLAGS is only -Dwarnings; zerocopy_unstable_ptr appears in RUSTDOCFLAGS, which does not enable this test target for cargo test. Thus the main positive invariant tests compile as an empty integration target in ordinary CI, while only the separately driven UI cases exercise the feature.

Please add --cfg zerocopy_unstable_ptr to the test wrapper's enabled experimental cfg set (or otherwise run this integration target explicitly with the cfg) so these runtime tests actually execute in CI. A small assertion/check that the target contains or runs at least one invariant test would make this harder to regress silently.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Round 3 found one additional public-contract/documentation issue. I am not repeating any previously reported finding.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

The existing TryFromBytes documentation defines a “valid instance” in terms of Rust bit validity, and the conversion methods say that if the source bytes are a valid instance they return Ok. Field invariants make that promise false unless “valid instance” is explicitly broadened: for example, struct Foo { #[zerocopy(invariant(false))] a: u8 } is bit-valid for every u8, but every TryFromBytes conversion rejects it.

Please update the controlling TryFromBytes/conversion documentation to distinguish Rust bit validity from these additional user-defined acceptance predicates—either redefine the relevant notion of validity to include configured invariants, or state that bit-valid inputs may still be rejected when an invariant returns false. The new section should link that distinction back to the existing “What is a valid instance?” text so the public contract is internally consistent.

Authored by an AI agent acting on Josh's behalf.

[jswrenn]

The documentation gap remains. The trait already warns that TryFromBytes need not accept every byte sequence produced by IntoBytes, and the new invariant section explains the additional checks. However, “What is a valid instance?” still discusses Rust bit validity, and the conversion-method wording does not explicitly connect that definition to invariant-based rejection. The controlling docs should state that bit-valid input may still be rejected by a configured invariant and link that distinction to the invariant section. The previous fixes did not update these docs or add a regression specifically for this contract clarification. Leaving this thread open.

Authored by Codex, an AI agent acting on Jack Wrenn’s behalf.

[joshlf]

Maybe we should update the TryFromBytes docs to refer to safety more generally? We've recently done something similar internally: #3665

[joshlf]

The implication here is a broken public contract, even if the implementation remains memory-safe.

The current docs define a "valid instance" in terms of Rust bit validity, then say the conversion succeeds when the source contains such an instance. With field invariants, that is no longer true: bytes can be fully bit-valid and still be rejected because an invariant returns false.

That matters because users should be able to reason from the documented contract, including in unsafe code. Returning Err for a bit-valid value does not itself create UB, but a false documented guarantee can invalidate downstream reasoning that relied on the stated success condition. The docs should therefore describe the actual acceptance condition explicitly rather than overloading "valid instance" to mean two different things.

Authored by an AI agent acting on Josh's behalf.

[joshlf]

Maybe we should update the TryFromBytes docs to refer to safety more generally? We've recently done something similar internally: #3665