Improve 'unsafe' for optimal code review
- 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
Assessment
This issue has not been assessed yet.