Skip to content

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

Medium Primitives

Unsound API smells: two fixes and a review checklist

An API is sound when no safe code, using it in any way the type system allows, can cause undefined behavior (UB). The starter's Registry has two methods whose signatures break that rule. Redesign them so that the code is sound and clippy accepts it, while every command still produces the same output.

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

The command language

A fresh Registry holds 8 slots, indexes 0 through 7, all starting at 0. Each string in ops is one command, and the output has exactly one number per command, in the same order. A line is split on whitespace (leading, trailing and repeated spaces or tabs are ignored), and the command words are case-sensitive.

Command Result
set I V Stores V in slot I and returns V.
add I V Stores the slot's value plus V in slot I and returns the new value.
get I Returns the value in slot I without changing it.
sum Returns the total of all 8 slots.

I is parsed as a usize and V as an i64 with Rust's str::parse. That accepts an optional leading + (and - for V) followed by decimal digits, and rejects underscores, hex prefixes and empty strings. The two error values are:

  • -1001 when I or V does not parse, or I is 8 or larger. Nothing is changed.
  • -1000 when the line is not one of the four shapes above, including a wrong number of words (get, sum 1, set 0) or a different spelling (Get 0). Nothing is changed.

For example, ["set 0 5", "add 0 -2", "get 9", "sum"] returns [5, 3, -1001, 3]. An empty ops returns an empty list. The tests never make add or sum overflow i64; the starter would panic on overflow in a debug build and wrap around in a release build, and you do not need to change that.

The test harness also calls three items directly, so keep them: Registry::new(Vec<i64>), Registry::get(&self, usize) -> Option<i64>, and Registry::raw_sum(*const i64, usize) -> i64 as an associated function (it may be unsafe). Every test expects Registry::new(vec![7, 8, 9]).get(2) to be 9 and raw_sum over [4, 5, 6] to be 15.

What the grader checks

The starter compiles and gives the expected output on every test, but its design is wrong, and clippy reports two errors. Both lints belong to clippy's correctness group, which is denied by default: a match is reported as an error, not a warning. Your submission must compile, pass the tests, and pass clippy.

Passing the lints does not prove soundness. Clippy recognizes particular shapes. A different unsound design, such as returning a raw *mut i64 from &self and writing through it elsewhere, is not reported by either lint. The tests and the lints check the behavior and the two shapes; the reasoning below is what makes the redesign sound.

Smell 1: &mut T from &self

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

Clippy reports the error "mutable borrow from immutable input(s)" (clippy::mut_from_ref).

Check your understanding Two &mut i64 to the same slot. What actually goes wrong? Show answer

Rust has two kinds of reference. A shared reference &T may coexist with other shared references, and the value behind it must not change while any of them is in use (unless the type uses UnsafeCell, Rust's building block for interior mutability). A mutable reference &mut T is exclusive: while it is in use, no other reference can read or write that value. These are language rules, not style. The compiler optimizes on the assumption that they hold, for example by reusing a value it already loaded instead of reading memory again. A program that breaks them has UB, and the optimized code may do anything.

This slot lets safe code break the exclusivity rule with two calls:

let a = reg.slot(0);
let b = reg.slot(0); // two live &mut i64 to the same slot, and no `unsafe` here

The unsafe block is inside slot, so the caller cannot see it, and the // SAFETY: comment there ("callers only pass indices they got from size()") is a rule nothing enforces. It also ignores the second problem: Vec::as_ptr is documented as not to be used for writing; as_mut_ptr is the method for that.

The fix is to take &mut self. get_mut (a slice method that a Vec also offers) already has the right shape: called with a usize index it takes &mut self and returns Option<&mut T>. The Option is the bounds check, and the &mut self borrow means the registry stays borrowed for as long as the returned reference is in use. A second slot call while the first result is still in use is then a compile error (E0499, "cannot borrow reg as mutable more than once at a time").

The suggested design wraps the reference in a small guard type, Slot<'a>, with read and write methods, and returns Option<Slot<'_>>. The lifetime 'a ties the guard to the &mut self borrow. Standard library types that check access at run time use the same idea: RefCell::borrow_mut returns a RefMut and Mutex::lock returns a MutexGuard (wrapped in a LockResult), not a bare &mut T. The Option also turns an out-of-range index into a value the caller handles, rather than a precondition the caller has to remember.

A method that takes &self and returns &mut T can be sound when each call returns memory that nothing else can reach, as an arena allocator does when it hands out a fresh allocation. Clippy only reports this lint for functions that contain unsafe code, and such libraries document why their case is sound. The Registry case is not one of them: two calls can return the same slot.

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

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

Clippy reports the error "this public function might dereference a raw pointer but is not marked unsafe" (clippy::not_unsafe_ptr_arg_deref).

A public safe function promises that no arguments a safe caller can write will cause UB. Any safe caller can write Registry::raw_sum(0x8 as *const i64, 1000), and creating a raw pointer from an integer needs no unsafe. The function cannot check everything it relies on. Null and misalignment can be tested at run time, but whether n initialized i64s really exist at p, inside one allocation, is not something the pointer value can tell you. So the obligation has to move to the caller, and that is what unsafe fn means: the caller must write unsafe to call it, and in doing so takes responsibility for its stated conditions.

Make it pub unsafe fn and give it a # Safety doc section listing those conditions. They come from std::slice::from_raw_parts: p is non-null and aligned for i64 even when n is 0, the n values are initialized and lie within one allocation, they total at most isize::MAX bytes, and nothing mutates them during the call. Then total() calls it inside an unsafe block with a // SAFETY: comment explaining why its particular arguments meet those conditions. The # Safety section states the contract; the // SAFETY: comment shows that one call site keeps it.

Inside an unsafe fn, Rust 2024 expects unsafe operations to sit in an unsafe { ... } block as well: leaving the block out triggers the unsafe_op_in_unsafe_fn lint, which warns by default in this edition.

A review checklist

These eight shapes deserve a close look in code review. Items 1 and 2 are the two above. The severity is the default in Rust 1.98: "error" lints are denied by default, "warning" lints are warned. Items 6 and 7 have no lint at all.

  1. fn get(&self) -> &mut T in a function containing unsafe code: clippy's mut_from_ref, error.
  2. A safe pub fn dereferencing a raw-pointer argument: clippy's not_unsafe_ptr_arg_deref, error.
  3. Writing through a pointer cast from a shared reference, such as *(r as *const T as *mut T) = x: rustc's invalid_reference_casting, error. The lint catches direct casts like this one, not every route to the same write.
  4. transmute::<&T, &mut T>: rustc's mutable_transmutes, error. This is the same bug as item 1, written as a type conversion.
  5. A pub unsafe fn without a # Safety doc section: clippy's missing_safety_doc, warning. The function imposes conditions on its callers but does not say what they are.
  6. Safe code whose soundness depends on a destructor running. Rust does not guarantee that Drop runs: mem::forget is a safe function, and a cycle of Rc values is never freed. No lint detects this.
  7. A pub field or a #[derive(Clone)] on a type whose unsafe code relies on that field's value or on being the only owner. Deriving Clone on a handle that frees a raw pointer in Drop copies the pointer, so dropping both copies frees it twice, from safe code. A pub len field lets a caller claim more initialized elements than exist. No lint detects these.
  8. Unsafe code hidden behind a name. Clippy's macro_metavars_in_unsafe (warning, exported macros only by default) reports a macro_rules! that places a caller-supplied expression inside its own unsafe block, which lets callers run unsafe operations without writing unsafe. Clippy's unsafe_removed_from_name (warning) reports an import such as use std::cell::UnsafeCell as Cell;, which hides the word at every use.

Why #[allow] is not the fix

Adding #[allow(clippy::mut_from_ref)] and #[allow(clippy::not_unsafe_ptr_arg_deref)] would make both errors disappear, and the grader cannot detect that. It would not make the code sound. An #[allow] says "this lint is wrong about this code", and it should come with a comment saying why. For lints in clippy's style and pedantic groups that is often a reasonable judgment. For correctness lints it needs a real argument, like the arena case above. Here none exists: the starter's slot lets safe code create two &mut to one value.