reflect: adopt message values from another descriptor pool on set

[iainmcgin]

Fixes #297.

The bug

A generated type reflects against its defining crate's DescriptorPool. A Duration view always reports buffa-types' pool — never the consumer pool that references it. So a nested cross-crate message value can never be pointer-identical to its parent's pool, and #272's Arc::ptr_eq check on message values rejected every one of them, with an error that says the quiet part out loud:

field "timeout" (#2) on verify.Job
  expects message google.protobuf.Duration,
  got     message google.protobuf.Duration

That makes the vtable rebuild walk — for_each_set + set(fd, vr.to_owned()) — panic for any message with a well-known-type field.

It is broader than the issue title suggests. It is not WKT-specific (any extern_path-mapped proto hits it, and so do two packages generated in a single codegen run, which get distinct pool Arcs), and it is not view-specific (owned types and the bridge path are affected identically; only lazy views escape, having no reflect impls at all).

The fix

try_set adopts rather than rejects. A message of the same full name from a foreign pool is re-homed into the target pool by a wire round-trip — which re-homes the whole subtree in one decode — recursively covering singular, list, and map values.

Descriptors stay strict: you still pass field descriptors resolved from the message's own pool. Values get adopted, because the codegen makes pool-identical values impossible for a cross-crate type. Full name is the adoption key — the same criterion protobuf itself uses across compilations, and what Any's type URL carries — so a genuinely wrong type is still WrongValueKind whatever pool it came from.

Two details worth a reviewer's eye:

• The pre-existing strict check doubles as the "already homed and well-shaped" predicate, so a value needing nothing is returned untouched. The common path — every value the decoder and the JSON parser produce — allocates nothing, not even for repeated and map fields.
• Re-homing goes through try_encode_to_vec, not encode_to_vec: after encode: enforce the protobuf 2 GiB message-size limit #271 the latter panics above 2 GiB, and a value handed to set must never take the process down. An over-limit foreign message is rejected like any other value the pool cannot hold.

Why CI didn't catch it

The conformance job ran only three of the seven modes. via-lazy, view-json, via-reflect and via-vtable existed solely in the local Docker script, and via-vtable is the one that walks this exact rebuild path. All four are now wired into CI. The via-vtable run goes from 20 panicking failures to 1246 successes, 0 failures; the suite is 14/14.

Behavior change

A cross-pool value that previously returned Err(WrongValueKind) now succeeds. Callers that leaned on that rejection as a provenance check must compare ReflectMessage::pool themselves — called out in the changelog fragment. #272's own fragment is still unreleased and claimed "nested message descriptor identity included" among the rejections, so it would have shipped contradicting this one in the same release; it is amended here to say what it actually delivers (nested messages of the wrong type).

Also adds MapValue::into_entries, the by-value counterpart to from_entries, so a map can be rebuilt without cloning its values.

Verification

Beyond the unit tests (which reproduce the cross-pool condition with two pools decoded from identical FDS bytes — no buffa-types needed), I drove the real cross-crate scenario from a scratch consumer crate: a Job message with Duration / Timestamp / repeated Duration fields, generated with vtable reflection, walked through for_each_set + set. On main it panics with the error above. With this change the walk completes, the rebuilt DynamicMessage encodes byte-identically to the generated message (49 bytes), the adopted sub-messages report the consumer's pool, the reflective JSON is correct ("timeout":"90.000500s"), and a Timestamp set into a Duration field is still rejected — now with an error that actually names two different types.

Full workspace tests, clippy -D warnings, and conformance 14/14.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[iainmcgin]

[claude code] Merged current main in (this branch predated #312/#313/#299/#306) and finished the review pass. Ready for review.

The soundness question worth your attention: adoption is keyed on the message's full name. Two pools can hold structurally different messages under the same FQN, so I probed what actually happens rather than reasoning about it. Building two pools that disagree about skew.Payload:

divergence	outcome
int32 → int64 (wire-compatible)	accepted, reinterpreted as the same value
string → int32 (wire-incompatible)	accepted; bytes land in unknown fields, field reads unset
field absent in target	accepted; round-trips intact (verified the bytes survive re-encode)

Nothing is lost in any case. Every outcome is what protobuf's wire contract already does when those bytes arrive from a peer on a different schema vintage, and protobuf's own identity model is name-based — it is what Any's type URL carries. The alternative, a structural equivalence check before adopting, would be worse: it would reject exactly the cross-crate version skew this PR exists to fix. 831b83f adds a test pinning all three outcomes, because the doc asserted this contract and nothing covered it.

Verified against the full conformance suite: 14/14 suites pass, zero failures — std 5549, no_std 5529, via-view 2797, via-lazy 2797, view-json 1246, via-reflect 2783, via-vtable 1246, plus 883 text × 2. That includes via-vtable, the mode whose absence from CI is how #297 shipped green in the first place. All four newly-wired modes have their known-failure lists present.

Two Low findings from review, both applied:

• The round-trip decode applies the standard recursion limit, so a message nested past it was reported as "from an incompatible pool" — naming the wrong cause for two pools that agree perfectly. It now reports what the decoder actually said. The encode comment's "must never take the process down" claim is scoped to size, which is what try_encode_to_vec guards; depth is not checked, and a message nested past the stack's tolerance overflows there as it would on any other encode of that value.
• The try_set performance note claimed "one wire round-trip". The vtable rebuild it cites as the motivating caller pays two for a cross-pool field: to_owned() → to_dynamic() already encodes and decodes the subtree into its defining pool, then set re-homes it. Corrected the note rather than the code — to_owned() has no target-pool parameter to decode into, so there is no clean fix at that seam. Bounded and irrelevant for WKTs, which is why conformance shows no pain.

One hypothesis I chased and can now rule out: the rebuild walk is not quadratic on nesting. reflect_to_dynamic creates its output in m.pool() and walks a single level, and nested to_dynamic decodes a whole subtree into the defining pool in one shot — so same-crate children share the pool Arc and hit the fast path with no round-trip. Only direct cross-crate fields of the root adopt. The non-recursive walk is what saves it, not WKT shallowness.

Also confirmed the two unreleased changelog fragments are coherent: this branch already reconciles the #272 fragment ("nested message descriptor identity" → "nested messages of the wrong type"), since #297 supersedes that identity claim.