Don't bridge a layout-compatible std::string by value. - #2043
Open
copybara-service[bot] wants to merge 1 commit into
Open
copybara-service[bot] wants to merge 1 commit into
copybara-service[bot] wants to merge 1 commit into
Conversation
Under `layout_compat_string`, a C++ `std::string` is spelled in Rust as
`cc_std::std::string`: a `!Unpin`, `PinnedDrop` type which lives in place and is
only ever produced through a `Ctor`.
Bridging works by memcpy'ing a value through a byte buffer, so every part of a
bridged type has to be Rust-movable. `cc_std::std::string` is not. Until now,
the two halves of the decision disagreed: the *spelling* of the type honored
`layout_compat_string` and said `string`, while the *ABI* ignored the feature
and boxed, producing a `string_wrapper`. A `std::string` nested inside a bridged
container -- `std::optional<std::string>`, `std::pair<std::string, T>` -- would
therefore emit generated code which does not compile, taking the whole target
down with it.
Refuse to bind those functions instead. This mirrors what already happens for a
user-defined non-Rust-movable type in the same position:
Type `Foo` must be Rust-movable in order to memcpy through a bridge buffer.
`std::string` was the one type that silently boxed rather than reporting the
problem. Now the cost of a nested layout-compatible `std::string` is limited to
the individual functions which mention it, and, as always, an API annotated with
`CRUBIT_MUST_BIND` still escalates it to a hard error.
The fix for an API caught by this is to make the *containing* type
layout-compatible as well, so that it is never bridged by value. For
`absl::StatusOr<std::string>` that is `CRUBIT_NEW_STATUS`, which the new test
covers.
PiperOrigin-RevId: 982803522
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Don't bridge a layout-compatible
std::stringby value.Under
layout_compat_string, a C++std::stringis spelled in Rust ascc_std::std::string: a!Unpin,PinnedDroptype which lives in place and isonly ever produced through a
Ctor.Bridging works by memcpy'ing a value through a byte buffer, so every part of a
bridged type has to be Rust-movable.
cc_std::std::stringis not. Until now,the two halves of the decision disagreed: the spelling of the type honored
layout_compat_stringand saidstring, while the ABI ignored the featureand boxed, producing a
string_wrapper. Astd::stringnested inside a bridgedcontainer --
std::optional<std::string>,std::pair<std::string, T>-- wouldtherefore emit generated code which does not compile, taking the whole target
down with it.
Refuse to bind those functions instead. This mirrors what already happens for a
user-defined non-Rust-movable type in the same position:
std::stringwas the one type that silently boxed rather than reporting theproblem. Now the cost of a nested layout-compatible
std::stringis limited tothe individual functions which mention it, and, as always, an API annotated with
CRUBIT_MUST_BINDstill escalates it to a hard error.The fix for an API caught by this is to make the containing type
layout-compatible as well, so that it is never bridged by value. For
absl::StatusOr<std::string>that isCRUBIT_NEW_STATUS, which the new testcovers.