Skip to content

Derive movability and destruction of an existing Rust type from its type arguments. - #2044

Open
copybara-service[bot] wants to merge 1 commit into
mainfrom
test_982826786
Open

copybara-service[bot] wants to merge 1 commit into
mainfrom
test_982826786

Conversation

@copybara-service

Copy link
Copy Markdown

Derive movability and destruction of an existing Rust type from its type arguments.

RsTypeKind::ExistingRustType interpolated its template arguments into the
spelling of the Rust type and then threw them away. As a result, every generic
mapped through CRUBIT_INTERNAL_RUST_TYPE was reported as Unpin and as not
needing destruction, no matter what it was instantiated with.

This is wrong for a generic that stores its payload in place. The motivating
case is absl::StatusOr<T> under CRUBIT_NEW_STATUS, which is mapped onto
status::absl::NewStatusOr<T>. NewStatusOr<T> holds a MaybeUninit<T>
inline and has an unconditional Drop impl, so NewStatusOr<T> is !Unpin
whenever T is. With the layout_compat_string feature enabled,
absl::StatusOr<std::string> maps onto NewStatusOr<cc_std::std::string>, and
Crubit was generating a by-value return:

pub fn MakeStringOrError() -> ::status::absl::NewStatusOr<::cc_std::std::string> {
    ...
    __crubit_return.assume_init()  // relocates a C++ `std::string`
}

This happens to work today only because libc++'s std::string is trivially
relocatable in practice; it is not something Crubit is entitled to assume.

Retain the type arguments on ExistingRustType and consult them from
is_unpin and needs_destruction, via a new RsTypeKind::inline_payloads.
The example above now returns
-> ::ctor::Ctor![::status::absl::NewStatusOr<::cc_std::std::string>].

Type arguments of a UniformReprTemplateType (std::vector<T>,
std::unique_ptr<T>, ...) and of a bridged container are deliberately not
treated as inline payloads: those either keep the payload behind indirection or
already require it to be Rust-movable.

This is complementary to the layout_compat_optional work in unknown commit,
which fixes the same class of bug for std::optional<T> on the
UniformReprTemplateType path.

…ype arguments.

`RsTypeKind::ExistingRustType` interpolated its template arguments into the
spelling of the Rust type and then threw them away. As a result, every generic
mapped through `CRUBIT_INTERNAL_RUST_TYPE` was reported as `Unpin` and as not
needing destruction, no matter what it was instantiated with.

This is wrong for a generic that stores its payload in place. The motivating
case is `absl::StatusOr<T>` under `CRUBIT_NEW_STATUS`, which is mapped onto
`status::absl::NewStatusOr<T>`. `NewStatusOr<T>` holds a `MaybeUninit<T>`
inline and has an unconditional `Drop` impl, so `NewStatusOr<T>` is `!Unpin`
whenever `T` is. With the `layout_compat_string` feature enabled,
`absl::StatusOr<std::string>` maps onto `NewStatusOr<cc_std::std::string>`, and
Crubit was generating a by-value return:

```rust
pub fn MakeStringOrError() -> ::status::absl::NewStatusOr<::cc_std::std::string> {
    ...
    __crubit_return.assume_init()  // relocates a C++ `std::string`
}
```

This happens to work today only because libc++'s `std::string` is trivially
relocatable in practice; it is not something Crubit is entitled to assume.

Retain the type arguments on `ExistingRustType` and consult them from
`is_unpin` and `needs_destruction`, via a new `RsTypeKind::inline_payloads`.
The example above now returns
`-> ::ctor::Ctor![::status::absl::NewStatusOr<::cc_std::std::string>]`.

Type arguments of a `UniformReprTemplateType` (`std::vector<T>`,
`std::unique_ptr<T>`, ...) and of a bridged container are deliberately not
treated as inline payloads: those either keep the payload behind indirection or
already require it to be Rust-movable.

This is complementary to the `layout_compat_optional` work in unknown commit,
which fixes the same class of bug for `std::optional<T>` on the
`UniformReprTemplateType` path.

PiperOrigin-RevId: 982826786
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant