docs: backfill TOC + decision trees, fix review findings
- Add ## Contents and ## Decision Tree to all 10 existing pattern files - Fix embed_as/1 semantics inversion in types.md (:self → :dump) - Fix fabricated __meta__.changes reference in changesets.md - Fix default primary key type (:integer → :id) in schemas.md - Combine @impl subsections into single "Minimal Callback Annotation"
This commit is contained in:
+61
-66
@@ -2,6 +2,21 @@
|
||||
|
||||
Analysis of `lib/elixir/lib/gen_server.ex`, `lib/elixir/lib/agent.ex`, and related modules.
|
||||
|
||||
## Contents
|
||||
|
||||
1. [Pattern 1: Client/Server API Separation](#pattern-1-clientserver-api-separation)
|
||||
2. [Pattern 2: `@impl true` Annotations on All Callbacks](#pattern-2-impl-true-annotations-on-all-callbacks)
|
||||
3. [Pattern 3: Guard-Protected `start_link`](#pattern-3-guard-protected-start_link)
|
||||
4. [Pattern 4: `handle_continue` for Post-Init Work](#pattern-4-handle_continue-for-post-init-work)
|
||||
5. [Pattern 5: Timeout-Based Idle Shutdown](#pattern-5-timeout-based-idle-shutdown)
|
||||
6. [Pattern 6: Periodic Work via `Process.send_after`](#pattern-6-periodic-work-via-processsend_after)
|
||||
7. [Pattern 7: Call vs Cast Decision (Synchronous vs Asynchronous)](#pattern-7-call-vs-cast-decision-synchronous-vs-asynchronous)
|
||||
8. [Pattern 8: Default Callback Implementations with Clear Error Messages](#pattern-8-default-callback-implementations-with-clear-error-messages)
|
||||
9. [Pattern 9: `child_spec/1` Generation and Customization via `use` Options](#pattern-9-child_spec1-generation-and-customization-via-use-options)
|
||||
10. [Pattern 10: Agent as Minimal State Wrapper (GenServer Under the Hood)](#pattern-10-agent-as-minimal-state-wrapper-genserver-under-the-hood)
|
||||
11. [Pattern 11: Name Registration via `:via` Tuple](#pattern-11-name-registration-via-via-tuple)
|
||||
12. [Pattern 12: GenServer as Anti-Pattern — Don't Use Processes for Code Organization](#pattern-12-genserver-as-anti-pattern--dont-use-processes-for-code-organization)
|
||||
|
||||
---
|
||||
|
||||
## Pattern 1: Client/Server API Separation
|
||||
@@ -189,75 +204,35 @@ end
|
||||
|
||||
**Why:** `@impl true` only makes sense in the context of a declared behaviour. Using it without one causes a compiler warning, not a benefit.
|
||||
|
||||
### Multi-Clause Callbacks — `@impl` on First Clause Only
|
||||
### Minimal Callback Annotation
|
||||
|
||||
**Source:** [lib/elixir/lib/module.ex#L72](https://github.com/elixir-lang/elixir/blob/f4e1b34617ef92052b65781f18eae5b88a490098/lib/elixir/lib/module.ex#L72) (`@impl` documentation)
|
||||
|
||||
**What it does:** `@impl true` is a module attribute that applies to the function as a whole — all clauses, not just the one it precedes. Place it once before the first clause; subsequent clauses of the same function inherit the annotation.
|
||||
**What it does:** Once `@impl true` is on a callback, two things become redundant:
|
||||
|
||||
**Why:** Repeating `@impl true` on every clause is noise. Worse, it can mislead readers into thinking each clause is a separate function. The compiler already associates all clauses with the behaviour callback after seeing `@impl` on the first one. Additionally, if you mark one callback with `@impl`, the compiler requires all callbacks in that module to be marked — this enforcement operates at the function level, not the clause level.
|
||||
1. **Repeating `@impl true` on subsequent clauses** — the annotation applies to the function as a whole, not individual clauses. All clauses inherit it from the first.
|
||||
2. **Adding `@spec`** — the behaviour's `@callback` already defines the type contract. Dialyzer uses the callback spec to check implementations, so a redundant `@spec` creates a second source of truth that can drift.
|
||||
|
||||
**Anti-pattern:** Annotating every clause:
|
||||
**Why:** The behaviour owns the contract. Adding `@spec init(term()) :: {:ok, map()}` on a GenServer callback just restates `@callback init(init_arg :: term) :: {:ok, state} | ...` with less information. Repeating `@impl true` on every clause is noise that misleads readers into thinking each clause is a separate function. The minimal annotation communicates "this is a callback, the behaviour defines the contract."
|
||||
|
||||
**Anti-pattern:**
|
||||
|
||||
```elixir
|
||||
# BAD — @impl repeated on each clause of the same function
|
||||
@impl true
|
||||
def handle_call({:get, key}, _from, state) do
|
||||
{:reply, Map.get(state, key), state}
|
||||
end
|
||||
|
||||
@impl true
|
||||
def handle_call({:keys}, _from, state) do
|
||||
{:reply, Map.keys(state), state}
|
||||
end
|
||||
|
||||
@impl true
|
||||
def handle_call({:size}, _from, state) do
|
||||
{:reply, map_size(state), state}
|
||||
end
|
||||
```
|
||||
|
||||
**Example — after:**
|
||||
|
||||
```elixir
|
||||
@impl true
|
||||
def handle_call({:get, key}, _from, state) do
|
||||
{:reply, Map.get(state, key), state}
|
||||
end
|
||||
|
||||
def handle_call({:keys}, _from, state) do
|
||||
{:reply, Map.keys(state), state}
|
||||
end
|
||||
|
||||
def handle_call({:size}, _from, state) do
|
||||
{:reply, map_size(state), state}
|
||||
end
|
||||
```
|
||||
|
||||
**When NOT to apply this:** If clauses of the same callback are separated by other functions (non-consecutive definitions), the compiler may not associate them. Keep multi-clause callbacks grouped together — this is independently good practice for readability.
|
||||
|
||||
### Omit `@spec` on `@impl true` Callbacks
|
||||
|
||||
**Source:** [lib/elixir/lib/module.ex#L121](https://github.com/elixir-lang/elixir/blob/f4e1b34617ef92052b65781f18eae5b88a490098/lib/elixir/lib/module.ex#L121) (`@impl` marks function as `@doc false`)
|
||||
|
||||
**What it does:** When `@impl true` is present, omit the `@spec`. The behaviour's `@callback` already defines the type contract for the function. Dialyzer uses the callback spec to check implementations — a redundant `@spec` on the implementation adds no safety and creates a second source of truth that can drift.
|
||||
|
||||
**Why:** The behaviour owns the contract. Adding `@spec init(term()) :: {:ok, map()}` on a GenServer callback just restates `@callback init(init_arg :: term) :: {:ok, state} | ...` with less information (the callback documents the full union of valid return types). When the behaviour updates its callback spec, implementations with stale `@spec` annotations silently disagree.
|
||||
|
||||
**Anti-pattern:** Verbatim-copying the callback spec onto the implementation:
|
||||
|
||||
```elixir
|
||||
# BAD — spec restates what the @callback already defines
|
||||
@spec init(term()) :: {:ok, map()}
|
||||
@impl true
|
||||
def init(opts) do
|
||||
{:ok, %{started_at: DateTime.utc_now(), config: opts}}
|
||||
end
|
||||
|
||||
# BAD — redundant @spec and @impl on every clause
|
||||
@spec handle_call(term(), GenServer.from(), map()) :: {:reply, term(), map()}
|
||||
@impl true
|
||||
def handle_call(:status, _from, state) do
|
||||
{:reply, state.started_at, state}
|
||||
def handle_call({:get, key}, _from, state) do
|
||||
{:reply, Map.get(state, key), state}
|
||||
end
|
||||
|
||||
@impl true
|
||||
def handle_call({:keys}, _from, state) do
|
||||
{:reply, Map.keys(state), state}
|
||||
end
|
||||
|
||||
@impl true
|
||||
def handle_call({:size}, _from, state) do
|
||||
{:reply, map_size(state), state}
|
||||
end
|
||||
```
|
||||
|
||||
@@ -265,17 +240,23 @@ end
|
||||
|
||||
```elixir
|
||||
@impl true
|
||||
def init(opts) do
|
||||
{:ok, %{started_at: DateTime.utc_now(), config: opts}}
|
||||
def handle_call({:get, key}, _from, state) do
|
||||
{:reply, Map.get(state, key), state}
|
||||
end
|
||||
|
||||
@impl true
|
||||
def handle_call(:status, _from, state) do
|
||||
{:reply, state.started_at, state}
|
||||
def handle_call({:keys}, _from, state) do
|
||||
{:reply, Map.keys(state), state}
|
||||
end
|
||||
|
||||
def handle_call({:size}, _from, state) do
|
||||
{:reply, map_size(state), state}
|
||||
end
|
||||
```
|
||||
|
||||
**When NOT to apply this:** When the implementation intentionally accepts a narrower type than the callback declares, a more specific `@spec` documents that constraint for readers and Dialyzer:
|
||||
**When NOT to apply this:**
|
||||
|
||||
- **Non-consecutive clauses:** If clauses of the same callback are separated by other functions, the compiler may not associate them. Keep multi-clause callbacks grouped together.
|
||||
- **Intentionally narrower types:** When the implementation accepts a narrower type than the callback declares, a more specific `@spec` documents that constraint:
|
||||
|
||||
```elixir
|
||||
# Justified — documents that this init/1 only accepts keyword lists,
|
||||
@@ -1209,4 +1190,18 @@ end
|
||||
|
||||
**Why:** The pattern cuts both ways. Over-using GenServer creates bottlenecks. Under-using it means reinventing state management poorly. The litmus test: does the state need to survive between function calls? Does access need serialization? If yes, you need a process.
|
||||
|
||||
## Decision Tree
|
||||
|
||||
- If other modules will interact with your GenServer → define a client API wrapping call/cast (Pattern 1)
|
||||
- If implementing any behaviour callback → annotate with `@impl true` (Pattern 2)
|
||||
- If `start_link` accepts arguments with a specific expected shape → add guards for fail-fast validation (Pattern 3)
|
||||
- If `init/1` does expensive work (DB, network, cache warming) → split into fast init + `handle_continue` (Pattern 4)
|
||||
- If the process is ephemeral (per-user, per-session) and should clean up when idle → use timeout-based idle shutdown (Pattern 5)
|
||||
- If you need work at regular intervals regardless of message traffic → use `Process.send_after` self-scheduling loop (Pattern 6)
|
||||
- If the caller needs confirmation or backpressure → use `call`; only use `cast` for genuine fire-and-forget (Pattern 7)
|
||||
- If the process needs non-default restart/shutdown behavior → customize via `use GenServer` options (Pattern 9)
|
||||
- If the process is purely about state (no custom messages, no timers) → use Agent instead of GenServer (Pattern 10)
|
||||
- If spawning processes dynamically with unbounded names → use `{:via, Registry, ...}` to avoid atom leaks (Pattern 11)
|
||||
- If the operation is stateless pure computation → don't use a GenServer at all, use a plain function (Pattern 12)
|
||||
|
||||
<!-- PATTERN_COMPLETE -->
|
||||
|
||||
Reference in New Issue
Block a user