Skip to content

feat: add explicit copying to ProtoString - #442

Merged
iainmcgin merged 3 commits into
anthropics:mainfrom
rioyu123:codex/protostring-copy-from-str-20260914
Sep 21, 2026
Merged

iainmcgin merged 3 commits into
anthropics:mainfrom
rioyu123:codex/protostring-copy-from-str-20260914

Conversation

@rioyu123

@rioyu123 rioyu123 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This implements the explicit copying operation discussed in #441 for 0.10. It lets a string type keep its borrowing From<&str> conversion while providing owned storage to buffa.

ProtoString currently requires From<&str> for every input lifetime. A string library whose conversion borrows from its input cannot satisfy that bound for its owned representation without changing conversion semantics or introducing overlapping implementations.

This adds ProtoString::copy_from_str(&str) with a default through From<String>, removes the higher-ranked From<&str> bound, and uses the copying operation for borrowed JSON strings in non-optional singular fields and for generated view-to-owned conversions. The remote derive and buffa-smolstr example override it to retain their direct string construction path. Binary decoding still uses from_wire; optional/repeated/map/oneof JSON fields keep using the representation's native serde implementation.

Custom types with an efficient From<&str> should override the new method; otherwise these paths now allocate an intermediate String.

The latest additions from @iainmcgin document the fallback allocation cost and pin the copying route with a CountedStr fixture, so tests detect a conversion that produces the right value but bypasses the override.

Compatibility

  • Generic callers that inferred From<&str> from S: ProtoString need S::copy_from_str(value) or an explicit conversion bound.
  • json_helpers::proto_string::deserialize now requires ProtoString; hand-written implementations that only supplied the old From conversions need to implement the trait or provide their own deserializer.
  • Existing trait implementations inherit the new method. Inline/shared-string implementations should override it to avoid the default's intermediate String allocation. Remote-derived types already forward to the inner type's direct conversion.
  • The remote derive still requires a universal From<&str> on its inner type. Types with a borrowing conversion can implement ProtoString by hand; this PR does not add a new derive option.
  • Regenerate custom-string view code before using a representation without the old bound. Generated code for the default String representation and wire encoding are unchanged.

Tests

  • A lifetime-bearing Cow fixture keeps a borrowing From<&str> conversion and implements ProtoString for its owned variant. It cannot satisfy the original supertrait bound.
  • Four integration tests cover copying temporary input, view-to-owned across singular/optional/repeated/map/oneof fields, JSON and null defaults, binary and text round trips, and clearing fields.
  • Two CountedStr tests verify that view-to-owned conversion calls copy_from_str for every string field shape, including map keys and values, and that borrowed-string/null JSON deserialization uses it.
  • Runtime JSON tests exercise a type without any From<&str> implementation; derive and inline-storage tests exercise the copying override.

Rechecked head d1029b1 on Linux with Rust 1.95.0 and protoc 33.5:

  • task lint, task test (2,931 passed, 117 ignored, including doc tests), and task doc passed.
  • cargo check --workspace --all-features and buffa no-default-features checks, with and without json, passed.
  • The custom-types example built and ran its binary/JSON round-trip assertions successfully.
  • Regenerating WKT and bootstrap descriptor types produced no changes.
  • Logging regeneration produced the same three-line difference on both this head and unmodified main (c28717b); that unrelated difference is not included.

The protobuf conformance suite, MSRV check, and cross-target checks were not rerun in this follow-up.

Closes #441.

@github-actions

Copy link
Copy Markdown

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

Warn on ProtoString::copy_from_str and in the changelog fragment that a
type relying on an efficient From<&str> pays a String allocation unless
it overrides the method; add the value-equivalence contract bullet.
Add a counting string fixture and tests that view-to-owned (each field
shape) and JSON deserialization call copy_from_str. Cite anthropics#442/anthropics#441 in
the fragment and fix two comments that described the removed bound.
@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code] @rioyu123 the approach looks right for 0.10. Thank you for the design: a provided copy_from_str, the supertrait removal, and regression fixtures that fail to compile on the old code.

I pushed two commits to your branch:

  • Merged origin/main (own signed commit), keeping your copy_from_str changes; view.rs picked up main's per-message preserve_unknown_fields change cleanly.
  • The changelog fragment and the copy_from_str docs now warn that a custom string type that relies on an efficient From<&str> but does not override copy_from_str pays an intermediate String allocation, and that the remote derive needs no change.
  • # Contract now says copy_from_str(s) must equal From::from(s.to_owned()).
  • New CountedStr fixture and tests: a counter in copy_from_str (not in From) pins that generated view-to-owned code (singular, optional, repeated, map key and value, oneof) and JSON deserialization call copy_from_str. Without it every fixture passes even if codegen falls back to From<String>.
  • The fragment cites (#442, closes #441), and two comments that described the removed bound are corrected.

@rioyu123
rioyu123 marked this pull request as ready for review September 21, 2026 01:37
@rioyu123

Copy link
Copy Markdown
Contributor Author

Thanks for the additions, especially the CountedStr tests. They cover a gap in the original fixtures: checking that the override is actually used, not just that the resulting value is correct.

I rechecked d1029b1 and reran lint, workspace tests (2,931 passed, 117 ignored), strict docs, all-features and no-default-features checks, and the custom-types example on Linux. WKT and bootstrap regeneration were clean; logging regeneration produced the same three-line difference as unmodified main.

I've updated the description and marked this ready for review for 0.10.

@iainmcgin
iainmcgin added this pull request to the merge queue Sep 21, 2026
Merged via the queue into anthropics:main with commit ebc9319 Sep 21, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow ProtoString implementations without requiring From<&str> for every lifetime

2 participants