Skip to content

← The Expert Edge: Idiom, Review and Capstones step 11 of 14

Medium Primitives

Unsound API smells: the patterns that are always wrong

Two functions in this Registry are unsound. Restructure them so the compiler and clippy accept the design, without changing what any input produces.

pub fn fix_the_api(ops: Vec<String>) -> Vec<i64>

The starter compiles as a program and passes every test. It fails the lint gate on two deny-by-default correctness lints — the small set clippy treats as bugs rather than opinions — and neither can be satisfied by any unsound shape. That is what makes this gradeable: you cannot pass by tweaking a message, only by changing the design.

Smell 1: fn get(&self) -> &mut T

pub fn slot(&self, i: usize) -> &mut i64

clippy: mutable borrow from immutable input(s) (clippy::mut_from_ref, deny).

::: question A shared reference and an exclusive one to the same i64. What actually goes wrong? Everything, and not only in the obvious way.

&T means shared and immutable, and rustc does not merely trust that — it tells LLVM, by putting noalias and readonly on the parameter. The optimiser is then entitled to cache the value in a register across a call, to reorder reads around writes, and to assume two &i64s that came from different places cannot be the same place. Hand out a &mut derived from a & and every one of those assumptions is false, so the miscompilation is not a bug in the optimiser: you lied.

And it is trivially reachable from safe code:

let a = reg.slot(0);
let b = reg.slot(0);   // two &mut to the same i64, no unsafe in sight

The fix is a guard type, and it is what every interior-mutability type in std already does. RefCell::borrow_mut returns a RefMut, not a &mut T. Mutex::lock returns a MutexGuard. Vec::get_mut takes &mut self. Here, return Option<Slot<'_>> from &mut self: the borrow checker then guarantees the index was valid and that only one slot borrow is alive, and both facts are checked by the compiler rather than asserted in a comment.

Note what the guard buys you beyond soundness — the Option makes the out-of-range case a value rather than a precondition, so callers stop needing to know the length before they can call. :::

Smell 2: a safe pub fn that dereferences a raw-pointer argument

pub fn raw_sum(p: *const i64, n: usize) -> i64

clippy: this public function might dereference a raw pointer but is not marked unsafe (clippy::not_unsafe_ptr_arg_deref, deny).

A pub safe function is a promise that no input at all can cause undefined behaviour. raw_sum(0x1 as *const i64, usize::MAX) is a call any safe caller can write. The function cannot check the pointer — validity, alignment and provenance are not observable at runtime — so the obligation has to move to the caller, which is precisely what unsafe fn means.

Mark it pub unsafe fn, write the # Safety section saying what the caller must guarantee (non-null, aligned, valid for n initialised reads, not mutated during the call), and then call it from total() inside an unsafe block with a // SAFETY: comment explaining why those particular arguments satisfy it. Two comments, both load-bearing: one states the contract, the other discharges it.

The catalogue

Eight shapes that are always worth stopping on in review. The first two are above; the rest are here so you recognise them.

  1. fn get(&self) -> &mut T — deny (mut_from_ref).
  2. A safe pub fn dereferencing a raw-pointer argument — deny (not_unsafe_ptr_arg_deref).
  3. Writing through a pointer derived from a shared reference — rustc’s own invalid_reference_casting, deny.
  4. transmute::<&T, &mut T> — rustc’s mutable_transmutes, deny. Same bug as (1), spelled so it does not even look like a function signature.
  5. A pub unsafe fn with no # Safety section — missing_safety_doc, warn. You imposed an obligation and did not say what it was.
  6. A safe function whose soundness depends on its destructor running. mem::forget is safe, Rc cycles leak, and a panic during unwinding can skip a drop. No lint. You have to know this one.
  7. A pub field, or a #[derive(Clone)], on a type whose unsafe internals depend on it. Deriving Clone on a handle that owns a raw pointer gives you a double free from safe code; a pub len field lets a caller lie about how much is initialised. No lint exists for this, and that is worth saying out loud — it is the most common way a correct-looking unsafe abstraction becomes unsound six months later, in a commit that only touched a derive.
  8. Unsafe hidden behind a macro. clippy::macro_metavars_in_unsafe catches the exported-macro case — a macro_rules! that expands a caller-supplied expression inside an unsafe block, letting callers write unsafe code without typing the word. clippy::unsafe_removed_from_name catches the import-site version: use std::cell::UnsafeCell as Cell; makes the danger invisible at every use.

The rule about #[allow]

The original of this file carried #[allow(clippy::mut_from_ref)] and #[allow(clippy::not_unsafe_ptr_arg_deref)]. They have been removed for you, and putting them back is not a fix.

A #[allow] on a correctness lint is a claim that clippy is wrong about this case. For the style and pedantic groups that claim is often true and an #[allow] with a one-line justification is a normal professional outcome. For the correctness group it is almost never true — these lints exist because the shape they match is unsound in general, and “but my callers are careful” is not a property the compiler, the optimiser, or the next maintainer can rely on.

Nothing in this harness can see whether you added an #[allow]. A reviewer can, in one grep, and that is the audience this item is training you for.

Constraint

Every test asserts the existing output, sentinels and all: -1001 for an index or number that will not parse, -1000 for an unknown command. This is a refactor with a proof attached, not a rewrite.