type-design-review

Type-Design Review

Overview

Push every invariant you can into the types, so the compiler — not a runtime check, not a test, not a human reviewer — is the thing that says "no." This is the type-system instance of backpressure: a contradictory state that cannot be constructed needs no defensive check, no test, and no "this should never happen" comment, because it genuinely never can.

You are reviewing a diff for this property. Your job is not "do the types compile" — it is "could a wrong state have been represented at all, and if so, why wasn't it designed out?"

Three ideas, one throughline:

• Make impossible states impossible (Richard Feldman, elm-conf 2016): if the app forbids a combination of data, design the data model so that combination cannot be expressed.
• Parse, don't validate (Alexis King, 2019): don't check data and throw the knowledge away — parse it into a type whose existence is the proof of the invariant, then carry that type inward.
• Make illegal states unrepresentable (Yaron Minsky, Effective ML, c. 2010): making the wrong thing hard to express beats checking for the wrong thing at runtime.

When to Use

• A backpressured code-review subagent is reviewing a diff (per-iteration or whole-changeset) and the diff introduces or changes data models, function signatures, or domain types.
• New state is modelled with booleans / optional fields, new switch/match over a union, new as/! assertions, or new functions taking raw string/number.

Not for: pure formatting, dependency bumps, or changes with no type-level surface. And not as a general reviewer — correctness/logic bugs, simplicity/reuse, and test quality belong to the general code-review reviewer. This skill is only the type-design lens: could a wrong state have been represented at all?

The one question — apply it to every type in the diff

For each data model, signature, or domain type the diff adds or changes: enumerate the states the type permits, then ask which of those the domain forbids. Each permitted-but-forbidden state is a candidate finding, and the fix is to restructure so it can no longer be expressed. That single question is the review — the smells below are common instances of it, not a closed checklist. A diff can make an illegal state representable in a shape no row names (end before start, min > max, a list that should be a set, three different nulls meaning three different things); that still counts, and you are expected to catch it from the principle. Run the question first; reach for the table for vocabulary and ready-made fixes, not as the boundary of what to look for.

The smells — common instances (not a checklist)

Treat each row as a finding only when it actually makes a domain-forbidden state representable here — not on sight. The left column is a pattern to notice; it becomes a finding only once you can name the real illegal state it permits in this code (a matching pattern that forbids nothing real is not a finding — see Proportionality).

Smell	Why it's a bug waiting to happen	Push toward
Boolean flags that can't all be true at once (isLoading + isError + isSuccess)	2^n combinations exist; most are nonsense the types permit	one discriminated union with a status discriminant
Optional fields that are really mutually exclusive (error? and data?)	"loaded and erroring" is representable but meaningless	a tagged union, one variant per real state
as / as any / <T>x casting into or out of a union	an assertion the compiler can't verify — reintroduces the runtime error types exist to prevent	narrow with a user-defined type guard, or parse
Non-null assertion ! to silence "possibly undefined"	crashes at runtime if the value is null	narrow with a check, or restructure so it can't be null
switch/match/when with no exhaustiveness check	adding a variant later silently falls through instead of failing the build	default: return assertNever(x) (see below)
Primitive obsession / stringly-typed (string for ids, status, email, money, units)	UserId and OrderId are interchangeable; typos compile	branded/newtype/opaque types, literal unions
Validation returning boolean or void	the knowledge is discarded; every caller must re-trust	a parser returning a proof-carrying type
Two fields that must stay in sync (list + selectedIndex, cached count + array)	desync is representable; one can be updated without the other	a single structure that makes desync impossible; derive don't store
enum where a string-literal union would do	runtime footprint + surprising semantics (numeric enums accept any number)	"a" | "b" union, often as const-derived
Mutable shared/value-object state	aliasing bugs, "who mutated this?"	readonly, ReadonlyArray<T>, Readonly<T>

Proportionality — generalize the principle, don't gold-plate

Applying the principle broadly is not licence to maximize type ceremony. Flag a representable illegal state when all three hold: it's realistically reachable (not merely theoretical), it would cause a real bug or force defensive checks / "this should never happen" comments downstream, and the restructuring is reasonably cheap and lives in domain/core code. Otherwise, leave it.

Do not:

• brand or wrap a value with no invariant to prove — free text a user types (a note, a description) is just string;
• force a union onto booleans/optionals that genuinely can coexist — a booking that is both paid and cancelled (refund pending) is a real state, and merging it would lose information, not add safety;
• restructure a throwaway local or one-shot internal shape with no downstream consumers.

When the domain genuinely permits every combination, the permissive type is the correct type — match the domain, not your appetite for tighter types. The test is always "name the forbidden state this prevents"; if you can't name one, it isn't a finding.

Core pattern: impossible states, before/after

// BEFORE — a bag of optionals. isLoading && error && data is representable and meaningless.
interface State {
  isLoading: boolean;
  error?: Error;
  data?: User;
}

// AFTER — a discriminated union. Only the four real states exist; narrowing on
// `status` gives you exactly the fields valid in that state, nothing else.
type State =
  | { status: "idle" }
  | { status: "loading" }
  | { status: "success"; data: User }
  | { status: "error"; error: Error };

Core pattern: exhaustiveness with never

function assertNever(x: never): never {
  throw new Error(`Unhandled case: ${String(x)}`);
}

function render(s: State): string {
  switch (s.status) {
    case "idle":    return "";
    case "loading": return "Loading…";
    case "success": return s.data.name;
    case "error":   return s.error.message;
    default:        return assertNever(s); // add a variant → this line stops compiling
  }
}

The default: assertNever(s) line is the point: a forgotten case becomes a build failure, not a silent runtime fallthrough. Flag every exhaustive-looking switch that lacks it.

Core pattern: parse, don't validate

// BEFORE — validate returns a boolean; the result is lost, every caller must re-trust.
function isEmail(s: string): boolean { /* ... */ }
function sendInvite(email: string) { /* hope the caller checked */ }

// AFTER — parse returns a branded type; sendInvite cannot be called with an unparsed string.
type Email = string & { readonly __brand: "Email" };
function parseEmail(s: string): Email | undefined { /* check, then brand */ }
function sendInvite(email: Email) { /* the type IS the proof it was parsed */ }

Parse at the boundary (where untyped input enters), then pass the precise type inward. King's rule of thumb: treat functions that return void (Haskell's ()) with deep suspicion — their effects are all you get; nothing is preserved in the types.

Recommend the project's existing parser before a hand-rolled one. Before suggesting a x is T guard, check what the repo already has — a schema/validation library in package.json or imported nearby (Zod, Valibot, ArkType, io-ts, runtypes, Yup, @effect/schema, …). If one exists, prefer pointing the fix at it (Schema.safeParse(input)): usually less code than a bespoke guard, a runtime error message for free, and consistent with the rest of the codebase. A hand-written guard is still perfectly fine for a trivial or local check — the rule isn't "always use the library," it's don't reinvent a parser the project already has, and don't tell the author to add a new dependency just to satisfy this review (suggesting they adopt one is fair at a real, repeated boundary; for a one-off, a hand guard or a flagged follow-up is the lighter call). Match the tool the project already uses, not your favorite.

Branded / nominal types

TypeScript is structural, so UserId and OrderId are both just string and freely interchangeable. A phantom brand makes them distinct:

type Brand<T, B> = T & { readonly __brand: B };
type UserId = Brand<string, "UserId">;
type OrderId = Brand<string, "OrderId">;

declare function getUser(id: UserId): User;
declare const oid: OrderId;
getUser(oid); // ERROR — OrderId is not assignable to UserId

Brands should only be minted by a parser/constructor — that is what ties them to "parse, don't validate."

When an escape hatch is acceptable

as and ! are not banned — they are unchecked, so each one must be justified in the diff:

• ✅ as const (narrows + freezes a literal — not an unsafe assertion).
• ✅ x satisfies T instead of const x: T = … when you want the check and the narrow inferred type.
• ✅ An as/! immediately after a real runtime check you can see, against a truly external invariant.
• ❌ JSON.parse(body) as User, resp as SuccessResponse, maybeUser!.name — assertions of facts nothing verified. Push toward a type guard (x is T) backed by a real check, or a parser built with the project's existing schema library (see "Recommend the project's existing parser" above).

If the diff adds an as/! with no nearby justifying check, flag it and name the narrowing or parse that would replace it.

Same principle, other languages

The TypeScript mechanics above are one dialect of a language-agnostic idea: a closed sum type the compiler checks for exhaustiveness, no implicit null, and a newtype to defeat primitive interchangeability.

Language	Sum type / illegal-state mechanism	Exhaustiveness & null
Rust	enum with data per variant; newtype struct UserId(u64)	match exhaustive by default; Option<T>/Result<T,E>, no null
Haskell / OCaml	ADTs + smart constructors (the origin lineage)	incomplete-pattern warnings; Maybe/option, no null
Swift	enum with associated values; struct for products	switch must be exhaustive; Optional<T> (T?)
Kotlin	sealed class/interface hierarchies	when expression must be exhaustive; nullable T? tracked

When reviewing non-TypeScript code, apply the same questions: is this a closed set of states the compiler can check? Is absence modelled explicitly rather than with null? Are domain ids distinct types or bare primitives?

How to report findings

For each finding give: the smell (from the table), the state that is wrongly representable ("error and data can both be set"), and the concrete restructuring (the union/brand/parser to use). A finding without a representable-wrong-state is not a type-design finding — hand it to the general code-review reviewer instead. Push back is expected: if the author justifies an escape hatch with a real boundary check, that is a resolved finding, not a violation.

Common rationalizations

Rationalization	Reality
"The booleans are never set inconsistently in practice"	"In practice" is exactly the invariant the types should enforce. If it can't happen, make it unrepresentable.
"as User is fine, the API always returns that shape"	The compiler didn't verify it and the API can change. Parse at the boundary; carry the type in.
"Adding assertNever is boilerplate"	It is the difference between a forgotten case being a compile error vs. a production bug.
"It's just an id, a string is simpler"	A string lets OrderId flow into getUser(userId) silently. A brand costs one type and removes the whole class.
"Validation already happens at the controller"	If the type doesn't say so, the next caller doesn't know. Encode the proof, don't rely on convention.

Sources

• Richard Feldman, Making Impossible States Impossible, elm-conf 2016 — the SelectList example (a list whose selection is part of the structure, so you cannot select an item not in the list).
• Alexis King, Parse, don't validate (2019-11-05): parse into a proof-carrying type at the boundary. Two of her closing maxims (distinct bullet points in the essay): "Use a data structure that makes illegal states unrepresentable." and "Push the burden of proof upward as far as possible, but no further."
• Yaron Minsky, Effective ML (c. 2010, Harvard CS51 / Jane Street), popularized via Real World OCaml: "make illegal states unrepresentable."