google / google/autocxx

Improve 'unsafe' for optimal code review

Open
#166 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
2.6k
Forks
175
PR merge metrics
No merged PRs in 30d

Description

Versions of cxx prior to 1.0 and versions of autocxx prior to 0.5 do not require an `unsafe` keyword. That policy changes with 1.0 and 0.5 respectively, such that at least one `unsafe` keyword is required to indicate that all C++ interop is inherently unsafe. It is. Mistakes in C++ can and will result in UB elsewhere in Rust code.

I've not yet made an autocxx 0.5 release with this policy change, because I want to achieve more goals with respect to the `unsafe` keyword. These goals only make sense in the context of a significant existing C++ codebase which aims to adopt small amounts of Rust.

### Goals

1. **Make `unsafe` Rust code scarier than FFI.** When code reviewing a Rust change, reviewers should be able to distinguish `unsafe` pure-Rust code (which is a truly special situation and requires the highest level of code review scrutiny) from function calls into existing C++ (which is `unsafe` from a UB perspective, but pragmatically requires only similar code review to existing C++ code changes). Specifically, I'm very afraid that "real" `unsafe` blocks - which will likely be extremely rare - will be overlooked amongst the _thousands_ of C++ FFI `unsafe` blocks. `unsafe` will lose all meaning at code review time. We can't have that.
2. (related) **Allow lints for `unsafe` which don't trigger on FFI**. For the same reason. Maybe some organizations want all `unsafe` blocks to get additional sign-off from two senior engineers, but they want to allow free use of existing C++ APIs without those kinds of costs. At the very least, these policies would (rightly) _discourage_ use of `unsafe` but we still need to allow C++ FFI in existing codebases.
3. **Distinguish scarier C++ constructs**. If a `std::unique_ptr` passes from C++ to Rust, it's theoretically possible that another pointer exists in C++ and therefore there could be concurrent mutation, race conditions, buffer overflows, etc. But if a `std::shared_ptr` passes from C++ to Rust, it's _very likely_ that these mistakes have been made. It feels that pragmatically some C++ FFI constructs are much scarier than others.

### Design

I propose:

1. Like `autocxx`, the `unsafe` keyword becomes optional, but if it's omitted then all the actual functions become `unsafe`.
2. The `unsafe` keyword currently required by (head) `autocxx` can optionally instead be spelled `unsafe_ffi`, with no functional difference. The intention here is that, at code review, if a reviewer finds a genuine `unsafe` keyword then they give deep scrutiny (and maybe the committer needs to ask for special approval). If they encounter `unsafe_ffi` then they know that this is "only" C++ FFI, and whilst it absolutely could cause action-at-a-distance UB anywhere in the Rust code, it's no scarier than the existing C++ code.
3. The `unsafe` or `unsafe_ffi` keywords can optionally take a policy which state that _some_ function calls remain `unsafe`. Users might choose between these policies:
* All C++ FFI calls are unsafe. (Equivalent to omitting the `unsafe`/`unsafe_ffi` keyword)
* All C++ FFI calls are unsafe, except those passing only primitives (chars, ints etc.)
* All C++ FFI calls are unsafe, except those passing only primitives and `UniquePtr`. (And maybe other types which _typically_ imply sole ownership of a C++ thing).
* No C++ FFI calls are unsafe (the current behavior).

### Syntax

I'm not sure how best syntactically to specify this policy. There are not really any good options:

```rust
unsafe(primitives,UniquePtr)
```

```rust
unsafe=primitives,UniquePtr
```
etc. Also, the specified list is probably those things which are deemed _safe_ rather than unsafe, so it's all very confusing. Thoughts welcome.

### Timing

I plan to achieve step 1 and 2 before releasing 0.5. This issue will remain open for the more difficult job of figuring out step 3.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.