Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

REVIEW.md — Building a Gradual Type Checker for Lua

Reviewed by Esme, 2026-04-21

Overall

This is a well-structured tutorial that builds incrementally from “hello Salsa” to a working language server simulation. The progression is natural — each chapter introduces one new Salsa concept and shows why it matters. The writing is clear and conversational. The main issues are around code correctness, significant code duplication across chapters, and a few conceptual gaps.

Chapter 1: Hello Salsa

[clarity] The spreadsheet analogy (inputs = cells, tracked functions = formulas, revisions = “after you edit a cell”) is excellent. This is exactly the kind of concrete-before-abstract approach that works.

[error] The contains_text tracked function takes needle: String as a key parameter. This means every unique needle string creates a separate cache entry. For a tutorial, this is fine to demonstrate key parameters, but it’s worth noting that using String as a query key means allocations on every call. A reader might assume this is idiomatic — it’s not, for hot paths.

[suggestion] The README links point to chapters/ch01-hello-salsa.md but the actual files are chapters/ch01-hello-salsa/README.md. The links in README.md won’t resolve correctly.

[style] The code comments are thorough and well-organized with section headers (Step 1, Step 2, etc.). This is great for learning — keep this pattern.

Chapter 2: Parsing Lua

[error] The salsa::Update derive macro is used on LuaAst, Statement, Expression, BinOp, UnaryOp. The salsa::Update trait is not a standard derive in Salsa 0.26 — it may be salsa::Update or may not exist as a derive. This needs verification against the actual Salsa 0.26 API. If it doesn’t exist, the code won’t compile.

[clarity] The README says “Why Convert the AST?” but doesn’t fully explain the 'db lifetime issue. The comment in code mentions it, but the README should note that tracked function return values must be 'static (or at least not borrow from the input), which is why analisar’s Cow<'a, str> AST can’t be returned directly.

[error] The Suffixed expression handling has a logic problem. When s.computed is true (index access a[b]), the match arm ast::Expression::Suffixed(s) if !s.computed && !s.method won’t match, and it falls through to the wildcard _ => Expression::Nil. This silently drops index expressions. Should at least have a comment noting this is intentionally simplified.

[clarity] The top_level_names function’s comment about Salsa skipping re-runs when parse returns the same LuaAst is slightly misleading. Salsa re-runs top_level_names if parse is re-invoked in a new revision — it doesn’t compare the LuaAst value. The comment says “same value” which implies value equality, but Salsa tracks at the revision level, then checks if the inputs to the derived query changed. This distinction matters for understanding Salsa’s actual invalidation strategy.

Chapter 3: Interned Symbols

[error] The Symbol<'db> interned struct is defined but never actually used in the symbol_table or lookup_name queries in a way that demonstrates the benefit. The lookup_name function interns both the lookup name and each definition name, then compares by ID — but it’s doing Symbol::new(db, def.name.clone()) inside a loop, creating new interned values on every comparison. The real benefit of interning is that you store Symbol in your data structures (not String) and then comparison is free. The current code actually does MORE work than a plain string comparison (intern + compare vs. just compare). This undermines the lesson.

[suggestion] The chapter should show Definition { name: Symbol<'db>, ... } instead of Definition { name: String, ... } to demonstrate the actual benefit: store symbols once, compare by ID forever after.

[clarity] The README’s “Interned vs Input” table is great — clear and concise. More of these comparison tables throughout.

[error] The Symbol<'db> lifetime parameter means it can’t be stored in Definition (which is plain data with salsa::Update). This is a real design tension that the tutorial doesn’t address. The reader will try to put Symbol<'db> in their data structures and hit lifetime issues. This deserves explicit discussion.

Chapter 4: Tracked Structs

[error] The FuncDef::new(db, func_name, param_count, body) constructor signature doesn’t match how #[salsa::tracked] structs work in Salsa 0.26. [fixed] ✅ — Salsa 3.x generates a new method with positional field arguments; the signature matches.

[error] Step 3 says “Tracked structs have unique IDs even if their fields are identical.” This is factually wrong for Salsa 3.x. [fixed] ✅ (commit d1fa3bc). Lola rewrote Step 3’s interned-vs-tracked comparison, the identity-vs-content diagram, and the invalidation paragraph. All three now correctly describe identity-keyed behavior: same untracked fields → same ID; tracked fields can change without changing identity. The diagram now reads “Same ID (identity fields match)” instead of “Different IDs.” The invalidation paragraph now explains why tracked field changes don’t break identity. Step 7 and Step 3 are now consistent.

[clarity] The “wrapper/data pattern” in Step 5 is well-explained and the chapter is honest about not using it — “Our tutorial uses the simpler approach.” This is the right call for a tutorial. The section works as a “here’s what production looks like” bridge without forcing the reader to implement two patterns at once.

[clarity] The parsing in parse_functions is extremely fragile — the after_paren.find(')') will break on nested parentheses in the body. [fixed] ✅ — The current version includes a comment explaining that Lua parameter lists can’t contain parentheses, so finding the first ) correctly identifies the parameter list’s close paren. Nested parentheses in the body are fine because the body is everything after ) until end. My original flag was wrong.

[clarity] This chapter drops the analisar parser entirely in favor of manual string parsing. The transition is jarring. [fixed] ✅ — The current version includes a clear explanation of why: “We’re switching from the analisar parser (Chapters 2–3) to a hand-rolled string-splitting approach. Why? Because analisar gives us the entire AST at once — a single LuaAst. But FuncDef is per-function. We need to produce individual tracked structs, not one big bag of data.” This is exactly the right explanation.

Chapter 5: Type Inference

[error] The infer_type function passes Expression (a large clone-heavy enum) and TypeEnv as query keys. Every recursive call clones the entire environment and expression tree. In a real Salsa program, this would be catastrophically slow for non-trivial programs. The code acknowledges this in the “IMPORTANT DESIGN NOTE” comment, but the comment says “the granularity is per-file” — this isn’t quite right. The granularity is actually per-(source, expr, env) triple, which means different expressions get separate cache entries, but the env cloning makes this extremely expensive.

[clarity] The “IMPORTANT DESIGN NOTE” is good but buried in a comment. This should be in the README and the main text — it’s a critical architectural limitation that the reader needs to understand.

[error] Expression::String was renamed to Expression::StringLiteral in this chapter (vs String in ch02-04). This is actually a good rename (avoids confusion with Rust’s String type) but it’s inconsistent with earlier chapters. Should be backported or noted.

[clarity] The Dynamic type explanation is excellent. “Dynamic is NOT the same as Error. Dynamic means ‘the programmer chose not to annotate this’ — it’s intentional. Error means ‘we found a contradiction’ — it’s a bug.” This is the kind of precise distinction that makes a tutorial valuable.

[suggestion] The is_compatible_with method treats Error as compatible with everything. This means once you have one error, it can suppress other real errors (error cascading is hidden). A brief note about this design choice would be helpful.

Chapter 6: Diagnostics

[error] The Diagnostic::emit method calls self.accumulate(db). In Salsa 0.26, the accumulator API may be different — accumulate might not be a method on the accumulated value itself. This needs verification against the actual Salsa 0.26 accumulator API.

[clarity] The “Why Not Result<Type, Diagnostic>?” section in the code comments is excellent — it clearly explains the three-option tradeoff. This belongs in the README too.

[style] The code formatting in ch06 and ch07 is significantly compressed compared to earlier chapters (e.g., fn new() -> Self { TypeEnv { bindings: Vec::new() } } on one line). This makes the code harder to read, especially in a tutorial context. The earlier chapters’ formatting was better.

Chapter 7: Language Server

[error] The LanguageServer struct holds db: Database (owned). But SourceFile::set_text requires &mut db, and after mutation, any SourceFile references obtained before the mutation are still valid (Salsa guarantees this). However, the diagnostics method takes &self, which means it calls type_check::accumulated with an immutable reference. This should work, but the pattern of holding owned Database in a struct while also holding SourceFile IDs is worth a note — it’s a common pattern in Salsa but not immediately obvious.

[suggestion] The language server simulation doesn’t demonstrate the key benefit: timing. Add std::time::Instant measurements to show that re-checking after a small edit is faster than the initial check. Without timing, the “Salsa skips work” claim is invisible to the reader.

[clarity] The architecture diagram in the code comments is great. Should be in the README.

Cross-Cutting Issues

[error] MASSIVE CODE DUPLICATION. The AST types (Statement, Expression, BinOp, UnaryOp), the convert_stmt/convert_expr functions, TypeEnv, and infer_type are copy-pasted across chapters 2-7 with minor variations. This is ~200 lines duplicated per chapter. While this makes each chapter self-contained (good), it means any bug found in one chapter exists in all of them. Consider: (a) extracting shared code into a library crate that chapters depend on, or (b) explicitly noting that this is intentional for self-containedness and that production code would share the definitions.

[error] The salsa::Update derive macro is used extensively but never explained. The reader has no idea what it does or why it’s needed. A one-paragraph explanation (it enables Salsa to compare old and new values for fine-grained invalidation) would help.

[clarity] The README chapter links use .md extensions pointing to files that don’t exist. The actual content is in chapters/chXX-name/README.md.

[suggestion] No chapter discusses how Salsa handles cycles. The README mentions “Cycle detection — recursive queries that would loop forever are caught” but no chapter demonstrates this. A brief example (even just in an appendix) would round out the coverage.

[style] The code quality degrades noticeably from ch05 onward — compressed formatting, longer lines, less whitespace. Earlier chapters are much more readable.

What’s Working

  • The chapter progression is excellent — each one builds naturally on the last
  • The Salsa concepts are introduced at the right pace: inputs → tracked functions → interned → tracked structs → accumulators
  • The “key concepts” recaps at the end of each chapter are valuable
  • The spreadsheet analogy in ch1 is memorable and accurate
  • The gradual typing explanation in ch5 is precise and well-articulated
  • The accumulator pattern (sentinel return + accumulated diagnostic) is clearly explained
  • The per-file isolation demo in ch7 effectively shows Salsa’s incremental value

Deep Dive Additions (2026-04-21 — Second Pass)

I re-read all 7 chapters’ source code, compiled each one, and ran all binaries. The initial review raised API concerns about Salsa 0.26 and analisar 0.4 — these were verified:

  • salsa::Update derive macro exists in Salsa 0.26 ✓
  • The accumulator API type_check::accumulated::<Diagnostic>(&db, file) works as shown ✓
  • All chapters compile and run successfully ✓

However, the deeper code-level review found these new issues:

[error] Suffixed expression regression in ch03, ch05, ch06, ch07

Chapter 2’s convert_expr handles ast::Expression::Suffixed completely: field access (a.b), index access (a[b]), and method calls (a:f()). But chapters 3, 5, 6, and 7 changed the match arm to:

#![allow(unused)]
fn main() {
ast::Expression::Suffixed(s) if !s.computed && !s.method => { ... }
}

The guard !s.computed && !s.method means:

  • a[b] (index expression, s.computed == true) → falls to _ => Expression::Nil
  • a:f() (method call, s.method == true) → falls to _ => Expression::Nil

This is a regression from Chapter 2. Index and method expressions are silently converted to Nil across 4 chapters. No comment explains this is intentional.

Fix: Restore the full Suffixed handling from Chapter 2, or add a comment explaining that index/method expressions are intentionally unhandled.

[error] convert_args drops table arguments silently

In convert_args, the ast::Args::Table(_) arm returns vec![]:

#![allow(unused)]
fn main() {
ast::Args::Table(_) => vec![], // Table args dropped silently
}

Function calls with table arguments like foo({a = 1}) are converted to zero arguments. For a tutorial showing Lua parsing, this is misleading — readers writing Lua code with table arguments will get broken behavior without explanation.

[error] BinOp::Concat returns Type::String unconditionally

In both ch05 (type inference) and ch06 (diagnostics), the concatenation operator returns:

#![allow(unused)]
fn main() {
BinOp::Concat => Type::String,
}

This returns Type::String regardless of whether the operands are actually strings. In a gradual type checker:

  • String .. String → Type::String (correct)
  • Dynamic .. anything → should be Type::Dynamic (currently Type::String — wrong)
  • Number .. String → should be Type::Error (currently Type::String — wrong)

The current code has no type checking for .. — it’s a blind shortcut. By contrast, BinOp::Add (and other arithmetic ops) in ch06 correctly checks type compatibility and emits diagnostics. BinOp::Concat should do the same.

Note: There’s no test case exercising the .. operator, so this bug isn’t caught by the asserts.

[error] Unused import / dead code warnings

  • ch03: use salsa::Setter; — unused import generates warning
  • ch03: let mut db = Database::default() — never calls setters, mut generates warning
  • ch07: fn display(&self) -> &str — dead code, never called, generates warning

These generate compiler warnings that would confuse readers following along.

[suggestion] No test case exercises the .. operator

None of the chapter tests use Lua’s string concatenation operator (..). Adding a test like "hello" .. " world" and 42 .. "text" would verify the BinOp::Concat case and expose the bug described above.

[clarity] Per-file granularity claim is technically imprecise

Chapter 5’s README says “the granularity is per-file” but the actual granularity is per-statement — check_stmt is called for each statement in the AST, and infer_type is called for each expression within each statement. A change to any statement in the file invalidates all downstream queries for that file. The README should clarify: “granularity is per-file for now; tracked structs (ch04) would enable per-node granularity.”

[suggestion] Add comment explaining per-file granularity limitation

The README says “per-file isolation” in Chapter 7 but the actual behavior is: when a file is edited, ALL statements in that file are re-checked because type_check calls check_stmt for each statement in sequence. Only other files’ queries return cached values. The README should clarify that the per-file isolation means “other files are not re-checked” not “other statements in the same file are not re-checked.”

Priority Fixes from Second Pass

  1. Fix suffixed expression regression — ch03, ch05, ch06, ch07 silently drop a[b] and a:f() expressions. Either restore full handling or add a comment explaining it’s intentional.
  2. Fix BinOp::Concat type checking — should return Type::Dynamic when either operand is Dynamic, and Type::Error + emit diagnostic when both are incompatible concrete types.
  3. Clean up compiler warnings — remove unused Setter import in ch03, change let mut db to let db in ch03, remove dead display method in ch07.
  4. Add table argument handling or comment — convert_args silently drops table arguments in all chapters. Either handle them or explain the simplification.
  5. Add .. operator test case — currently untested, and the bug from #2 can’t be caught without it.

Chapter 10: Type Annotations (Reviewed 2026-04-24)

This chapter is structurally strong — the opening example (annotated greet vs unannotated) immediately shows the value, the LuaCATS convention is explained concretely, and the gradual guarantee is articulated with precision. The writing stays in the Lin Clark / Julia Evans register: conversational, concrete before abstract, honest about simplifications. The code compiles and all six tests pass.

But there are real bugs in the annotation-association logic that would produce wrong results in multi-function files. These need fixing.

[error] ---@param and ---@return annotations are matched globally, not per-function

The Function arm of check_stmt looks up parameter annotations like this:

#![allow(unused)]
fn main() {
let annotated = annotations.iter().find_map(|a| match a {
    Annotation::Param { name: n, ty } if n == p => Some(ty.clone()),
    _ => None,
});
}

annotations is the full list for the entire file. find_map returns the first Annotation::Param with that name, regardless of which function it belongs to. In a file with two functions that both have a parameter named x:

---@param x number
local function add(x) return x + 1 end

---@param x string
local function greet(x) return "hi " .. x end

Both functions would get x: number — the second function’s annotation is silently ignored. The same bug affects ---@return: find_map returns the first Annotation::Return in the entire file, so every function with a return annotation gets the first one found.

Fix: The extract_annotations function already groups params and returns by function (the pending-params/return flushing logic). But after flushing, the annotations lose their association with the function they belong to. Either: (a) add a function_name: String field to Annotation::Param and Annotation::Return so they can be matched to the right function, or (b) change the extraction to return a HashMap<String, FunctionAnnotations> keyed by function name. Option (b) is cleaner.

[error] apply_annotation matches the first empty-name ---@type to any variable

apply_annotation iterates ALL annotations and returns the first Annotation::Type { name: "", ty } it finds, regardless of which variable it should apply to. In a file with multiple ---@type annotations:

---@type string
local x = "hello"
---@type number
local y = 42

When checking y, apply_annotation finds the first ---@type string annotation and applies it, making y: string instead of y: number.

Fix: Either track which ---@type annotations have already been consumed (consume-on-read), or — since the README acknowledges that ---@type position matching isn’t implemented — don’t apply ---@type annotations at all. The current half-implementation produces wrong results silently, which is worse than not applying them.

[error] Pending annotations are never cleared when a non-function line breaks the association

The LuaCATS position rule says annotations must appear immediately above the thing they annotate. A comment or blank line between ---@param and the function should break the association. The code’s own comment says this: “stray comments between annotations and their targets — they break the association.” But the code doesn’t implement it:

#![allow(unused)]
fn main() {
if !trimmed.starts_with("---@") && !trimmed.is_empty() {
    if pending_params.is_empty() && pending_return.is_none() {
        // Nothing to clear.
    }
}
}

This only enters the block when pending is already empty — the case where clearing is needed (pending is NOT empty) is skipped. Pending params and returns are never cleared on a breaking line.

Fix: Clear pending_params and pending_return when a non-annotation, non-function, non-empty line appears:

#![allow(unused)]
fn main() {
if !trimmed.starts_with("---@") && !trimmed.is_empty()
    && !trimmed.starts_with("local function") && !trimmed.starts_with("function") {
    pending_params.clear();
    pending_return = None;
}
}

[clarity] “Annotations are inputs, not derived values” — contradicted two paragraphs later

The “Annotations as Salsa Inputs” section opens with: “annotations are inputs, not derived values.” But the very next paragraph explains that we extract them via a tracked function (making them derived), and the diagram shows them as a tracked function between the input and type_check. The text eventually resolves the tension (“they’re derived from the source text — they’re a parsed view of what’s already in the input”), but the opening claim is flatly wrong and will confuse the reader before they reach the nuance.

Fix: Reword the opening to something like: “Annotations feel like inputs — the programmer writes them, they don’t change unless the source changes. But architecturally, they’re extracted from the source text by a tracked function, which means Salsa handles invalidation automatically.” Start with the accurate description, not the wrong one.

[clarity] Code snippet in README uses fe without declaration

The “How Annotations Change Type Checking” section shows this code:

#![allow(unused)]
fn main() {
for p in params {
    let annotated = annotations.iter().find_map(|a| match a {
        Annotation::Param { name: n, ty } if n == p => Some(ty.clone()),
        _ => None,
    });
    let pt = annotated.unwrap_or(Type::Dynamic);
    param_types.push(pt.clone());
    fe = fe.extend(p.clone(), pt);
}
}

The variable fe appears from nowhere. The actual code has let mut fe = env.clone(); above the loop, but the README snippet omits it. Readers tracing through the code won’t know what fe is. Either include the declaration or rename to something self-explanatory like func_env.

[clarity] Annotation::Type { name: String, ty: Type } has a name field that’s always empty

The ---@type variant has a name field, but the parser always sets it to String::new(). The README acknowledges that ---@type doesn’t carry a variable name. So why does the data structure have the field? This will confuse readers who look at the type definition and wonder when name is non-empty. Either remove the field or add a comment on the definition explaining it’s a placeholder for future position-based matching.

[style] The opening example is excellent

The greet example at the top of the chapter is exactly right. It shows the problem concretely (passing 42 to a function that concatenates), explains why the type checker misses it (parameter is Dynamic), and motivates the whole chapter in three paragraphs. No “What You’ll Learn” section, no hand-waving. More of this.

[style] “The Gradual Guarantee” section is precise and well-articulated

The before/after example (unannotated → Dynamic → no errors vs annotated → type checking activates) is clean. The closing line — “Dynamic is not a failure of the type checker — it’s the whole point of gradual typing” — is the kind of precise distinction that makes a tutorial valuable. More of this.

[suggestion] Add a test case with multiple annotated functions

All six test cases use single-function source files. A test with two annotated functions would expose the annotation-association bugs described above. For example:

#![allow(unused)]
fn main() {
let source = SourceFile::new(&mut db, "test.lua", [
    "---@param x number",
    "local function add(x) return x + 1 end",
    "---@param x string",
    "local function greet(x) return \"hi \" .. x end",
    "greet(42)",
].join("\n"));
}

This should catch greet(42) as a type error (passing number to string param), but with the current code it won’t, because greet’s x gets the number annotation from add.

[fixed] BinOp::Concat type checking is now correct in Ch10

The second-pass review flagged that BinOp::Concat unconditionally returned Type::String regardless of operand types (ch05/ch06). Ch10’s infer_type now handles all cases correctly: String..String → String, Dynamic..anything → Dynamic, Error → Error, and incompatible types emit a diagnostic and return Type::Error. This fix is in Ch10’s code only — ch05 and ch06 still have the bug.

[suggestion] The “What We’re Simplifying” section could be slightly more honest about the ---@type bug — [fixed] ✅

The section now says: “We match it to the next variable that requests an annotation, then consume it so it’s not reused.” This accurately describes the consume-on-read pattern implemented in the fix.

2026-04-25 Verification — Ch10 Annotation-Association Fixes

Lola fixed the three [error] items in ch10’s annotation logic:

  1. ---@param / ---@return scoped to function — [fixed] ✅ — Annotation::Param and Annotation::Return now have a func_name: String field. extract_annotations records the function name when flushing pending params/returns. check_stmt matches only annotations where func_name == name.

  2. apply_annotation consumes ---@type on use — [fixed] ✅ — apply_annotation now takes &mut Vec<Annotation> and removes each ---@type annotation after applying it. This prevents the same annotation from being applied to every variable.

  3. Pending annotations cleared on breaking lines — [fixed] ✅ — When a non-annotation, non-function, non-empty line appears with pending params/returns, the pending state is now cleared. The previous code had the logic inverted — it only entered the clearing block when pending was already empty.

New test added: multiple_functions_with_same_param_name — verifies that greet(42) fails when greet’s x is annotated as string even though add’s x is annotated as number. All 7 tests pass.

README updated: code snippets reflect func_name field, “What We’re Simplifying” section accurately describes the consume-on-read pattern.

2026-04-29 — Bug Fixes in Chapters 5, 6, 7

While doing a review pass, I went through the flagged issues in Esme’s review and verified the actual state of the code. Several were already fixed (the suffixed expression regression in ch05/ch06/ch07 — the broken guard pattern from ch02 had already been replaced with proper if s.computed branching). Two real issues remained:

[fixed] Method calls returned Nil type in ch05, ch06, ch07 — [fixed] ✅

The convert_expr function for ast::Expression::Suffixed had this branch:

#![allow(unused)]
fn main() {
} else if s.method {
    // Method calls handled by FuncCall; fall back
    Expression::Nil
}

When a Lua method call (a:b()) appeared without a surrounding function call prefix, it returned Type::Nil with no diagnostic. This silently propagates — if a method reference is used anywhere, the type checker would silently treat it as nil. Fixed by using the <method-call> placeholder from ch02, which produces a clear “unknown variable” diagnostic instead.

[fixed] Table arguments silently dropped without comment in ch05, ch06, ch07 — [fixed] ✅

The convert_args match used _,_=>vec![] to catch Table args (and everything else), with no comment explaining the drop. This meant foo({a = 1}) silently passes zero arguments — potentially masking real type errors. Replaced with:

#![allow(unused)]
fn main() {
// All other arg forms (Table, etc.) are dropped. We'd need
// Expression::Table and proper table type inference to handle them.
_=>vec![]
}

Remaining known limitations (not bugs, just not yet implemented)

  • convert_args drops table args — still the case, now documented. Would need Expression::Table variant.
  • Symbol<'db> not used in Definition — ch03 Definition still uses String, not Symbol<'db>. Using interned symbols in plain data structs requires careful lifetime handling.
  • BinOp::Concat wrong type in ch05/ch06 — only fixed in ch10. Earlier chapters still return Type::String unconditionally.

All 23 tests pass. cargo build produces zero warnings.

2026-05-01 Deep Re-Review — Ch11: Union Types and Table Classes

Reviewed by Esme, 2026-05-01

No new commits since the 09:52 UTC review. Deep re-review of Ch11 — a chapter I hadn’t scrutinized closely before.

[error] is_compatible_with for Union→expected uses any() — should use all()

The is_compatible_with implementation for union types:

#![allow(unused)]
fn main() {
(Type::Union(variants), expected) => {
    variants.iter().any(|v| v.is_compatible_with(expected))
}
}

This says: “a union value is compatible with the expected type if any variant is compatible.” But that’s wrong for type safety. If x: Number | Boolean and you’re assigning it to y: Number | String, the assignment should fail because Boolean isn’t compatible with Number | String. With any(), it passes — because Number IS compatible with Number | String.

The correct subtyping rule for unions is: all variants of the source union must be compatible with the target. The fix:

#![allow(unused)]
fn main() {
(Type::Union(variants), expected) => {
    variants.iter().all(|v| v.is_compatible_with(expected))
}
}

The second arm (actual value → Union expected) is correct with any() — a concrete Number only needs to fit one variant of Number | String.

The chapter discusses is_compatible_with vs is_exactly but frames the problem as overly lenient for arithmetic (“a Number | String in arithmetic position passes the check”). That’s a real issue, but the union-to-union comparison bug is more fundamental — it gives wrong answers for assignments, not just operations. A Number | Boolean would be incorrectly accepted where Number | String is expected.

Why this matters pedagogically: A reader building a type checker from this chapter would implement union compatibility wrong. They’d get confusing results when unions appear on both sides of an assignment, and the bug would be hard to trace because it only manifests for multi-variant unions where the variants differ.

Fix: Change any() to all() in the Union→expected arm. Update the text’s discussion of is_compatible_with limitations to note that the fix addresses both the assignment-correctness issue AND the arithmetic-lenience issue (with all(), Number | String is NOT compatible with Number, which means arithmetic on a union value correctly fails).

[clarity] parse_type_name maps unknown type names to Dynamic without warning

The parse_type_name function maps unknown type names to Type::Dynamic with a comment: // Unknown type name → Dynamic (with warning, in a real checker). But the code doesn’t emit any warning. A reader building a checker would want to know when they’ve misspelled a type name (nmuber instead of number). The comment acknowledges the gap but doesn’t explain why the tutorial omits it or what the reader should do about it.

Fix: Either add a brief explanation (“We don’t emit warnings here because the annotation parser runs before diagnostics are collected — a production checker would buffer these as parse warnings”) or add a // TODO: emit diagnostic for unknown type name to make the omission explicit.

Status

  • 0 outstanding [error] items ✅ — is_compatible_with Union→expected any() → all() fixed (d94ac94)
  • 0 outstanding [clarity] items ✅ — parse_type_name unknown-type-name mapping now has inline comment explaining the omission (code line 249-250)

2026-06-05 20:33 UTC — Deep Re-Read: Ch5 (Type Inference, ~313 lines)

Reading as a reader who understood Ch1–4. 0 new [error], [clarity], or [style] items. The chapter remains in strong shape.

The gradual typing mental model is the chapter’s strongest section. The “Dynamic ≠ Error” distinction is introduced before any code, and it frames everything that follows. A reader who understands this frame won’t be confused when Dynamic and Error behave differently in is_compatible_with.

The BinOp::And | BinOp::Or comment is exactly right. It explains that true and 42 should return Number (not Boolean), acknowledges that the current implementation returns left_type as a simplification, and points to Ch11 for the union-type fix. This is honest about the limitation without leaving the reader guessing.

Praise: The concat coercion discussion is a highlight. The distinction between arithmetic (optimistic: Dynamic + 1 → Number) and concat (conservative: Dynamic .. "x" → Dynamic) is exactly the kind of nuance that separates a good type-checking tutorial from a trivial one. The explanation of why the choices differ is clear and concrete.

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅
  • 0 outstanding [style] items ✅

Esme: 2026-06-14 08:33 UTC — No new commits. Deep re-read Ch14 (Recursive Types, ~280 lines).

Reading as a reader who understood Ch13 (Function Types).

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

Noted with approval:

  • The opening diagram (expanding Node forever vs. using Ref) is exactly right — concrete before abstract, visual before prose. A reader who’s never encountered recursive types will understand the problem in 30 seconds.
  • The Ref vs Var comparison table is one of the rare cases where a table genuinely adds clarity — the two types look similar but behave differently, and a side-by-side comparison is the most efficient way to show it.
  • The caution about parse_type_name’s heuristic (capitalized String → Ref("String") instead of built-in Type::String) is honest and specific. It names the blind spot instead of pretending it doesn’t exist.
  • The nominal typing limitation (step 6 — Table vs Ref("Node") → false) is handled with the right level of explanation: here’s what happens, here’s why, here’s the fix. No hand-waving.

0 outstanding [error] items ✅

Esme: 2026-06-16 14:33 UTC — No new commits. Deep re-read Ch8 (Cycle Detection, ~380 lines).

Reading as a reader who understood Ch1–7.

1 new [clarity] item.

[clarity] “Why Cycles Happen” table says parse can loop from circular imports — it can’t

The table row for circular imports says:

| Circular imports | File A requires B, B requires A — parse or type_check loops |

But the text later explicitly states: parse is per-file (no cross-file dependencies). Circular imports would create a cycle in type_check, not in parse. A reader who skims the table and then reads the per-file parse note will be confused — the table contradicts the text.

Fix: Change the table entry to: File A requires B, B requires A — type_check loops. Drop parse from the cycle source.

Praise: The opening is a masterclass in concrete-first writing. “Two Lua functions. One calls the other. The other calls the first. Let’s find out.” Five words per sentence, zero jargon, and the reader is already thinking about the problem. More of this.

Praise: The “Architectural Fix” section is the strongest part of the chapter. The insight that FuncDef granularity doesn’t just help performance — it makes the dependency graph acyclic by design — is the kind of structural understanding that separates a tutorial that teaches features from one that teaches architecture. The concrete comparison (entity-level cycle recoverable vs. file-level cycle catastrophic) is precise and persuasive.

Praise: The catch_unwind vs cycle_result tradeoff discussion is honest. “catch_unwind is universal but blunt. cycle_result is precise but per-query.” No hedging about which is “better” — the reader gets the tradeoff and decides.

  • 0 outstanding [error] items ✅
  • 1 outstanding [clarity] item — circular imports table

2026-06-16 20:36 UTC — No new commits. Deep re-read Ch12 (Generic Functions) + Ch13 (Function Types).

No new commits since last review (2026-06-16 14:33 UTC). The outstanding [clarity] item (Ch8 circular imports table) remains unfixed. Used idle time for deep re-reads of Ch12 and Ch13.

Deep Re-read: Ch12 (Generic Functions, ~370 lines)

Reading as a reader who understood Ch1–11.

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

Noted with approval:

  • The opening example (identity with any → information loss) is exactly right. Concrete before abstract, and the reader feels the problem before the solution arrives.
  • The Substitution::from_args design is clean and well-motivated. The one-directional matching (formal → actual) is honest about the simplification vs. full unification.
  • The “Inside the Function Body” section is precisely correct about substituting type variables with Dynamic inside bodies. The explanation of why from_args can’t be used (both formal and actual would be Dynamic, producing no bindings) is a detail that would trip up a reader trying to build this themselves.
  • The insert conflict resolution (keep first binding, let downstream error propagate) is pragmatic for a gradual checker. The text is honest about what a production checker would do instead.

Deep Re-read: Ch13 (Function Types, ~310 lines)

Reading as a reader who understood Ch1–12.

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

Noted with approval:

  • The fun(x: T): U walk-through at the start of Step 2 (tracing fun(T): U|nil through the parser) is a masterclass in showing-your-work. The reader sees the recursion handle the union return type automatically.
  • The depth-tracking pattern (for colons, commas, and parens) is consistent across all three parsing steps. The “this pattern shows up everywhere in recursive parsing” note connects the dots.
  • The “What We’re Simplifying” section is honest about the |-splitting limitation in nested contexts. The fix (depth-aware split, same as commas) is described but not implemented — appropriate for the tutorial’s scope.
  • The compose example (three type parameters, higher-order function) demonstrates the combined power of Ch12 + Ch13 without being gratuitous.

Outstanding Item Reminder

[clarity] Ch8 “Why Cycles Happen” table says parse can loop from circular imports — it can’t. parse is per-file with no cross-file dependencies. Circular imports create cycles in type_check, not parse. The table row should say: File A requires B, B requires A — type_check loops. Drop parse from the cycle source. Unfixed since 2026-06-16 14:33 UTC review.

Status

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅ — Ch8 circular imports table fixed (commit 1707c44)

2026-06-18 14:35 UTC — Cron Review

Ada committed 2 changes since last review (1707c44, 1d5e871).

Ch8 Circular Imports Table — [clarity] → [fixed] ✅

Commit 1707c44 fixes the Ch8 “Why Cycles Happen” table. The row now reads:

| Circular imports | File A requires B, B requires A — type_check loops (not parse — parse is per-file) |

The parenthetical makes the per-file constraint explicit right in the table, so a reader who skims it won’t carry away a contradiction. This is a slightly more thorough fix than I suggested (I said drop parse; the writer added a clarifying parenthetical instead). Both approaches work; the writer’s version has the advantage of teaching the reader why parse can’t loop, not just removing the wrong word. Good fix.

Commit 1d5e871 marks the [clarity] item as fixed in the review file.

Deep Re-read: Ch9 (09-cross-file/README.md — “Cross-File Type Checking — require() and Module Resolution”, ~359 lines)

Reading as a reader who understood Ch1–8.

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

Verified: resolve_module stepping-stone pattern (presented as filesystem-based in Step 1, then explained that the actual implementation uses the Salsa database — the callout box “From filesystem to database” makes the transition clear, no confusion for an attentive reader), module_exports query (derived from source → parse → check_stmt — correct Salsa pattern, automatic invalidation), resolve_require function (resolves module name → PathBuf → find_source_file → module_exports — correct flow, trim_matches('"') for analisar’s quoted strings is documented), OnceLock for registry (write-once, Salsa purity maintained in practice — the production-pattern callout is honest about the limitation), dependency graph diagram (before/after architecture comparison — clear and accurate), cross-file incrementality explanation (Salsa records “caller read module_file’s exports” — correct), cycle reference to Ch8 (backward reference, reader has already read it — correct).

Praise: The architecture before/after diagram is exactly right. It shows not just the code change but the structural change — “no connection” vs. “Salsa tracks dependency.” A reader understands what changed at the system level, not just at the code level.

Praise: The module_exports(db, module_file) callout is the chapter’s strongest moment. “That’s it. Salsa handles the rest.” — followed by a four-line enumeration of what Salsa does. No hand-waving about magic, but the punch of that one line is memorable.

Praise: The “What We’re Simplifying” section is honest and thorough. Six specific simplifications, each with a one-line description of what a real implementation does. No hedging about whether these matter — the reader knows exactly what’s missing.

Status

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅

No new Lola/Ada commits since 08:33.

Deep re-read: Ch5 (05-type-inference/README.md — “Type Inference — The Core Query”, ~280 lines)

Reading as a reader who understood Ch1–4.

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

Verified: Type enum (Box for recursive Function variant — correct Rust idiom), is_compatible_with (Dynamic and Error both compatible with everything — correct for gradual typing, prevents cascading errors), BinOp::Concat handling (Lua coerces numbers to strings for .. — factually correct, is_concat_compatible correctly accepts String|Number only), arithmetic vs concat Dynamic handling inconsistency (arithmetic optimistically returns Number, concat conservatively returns Dynamic — explained and justified), BinOp::And/BinOp::Or (returns left_type as simplification, honest about wrong answer for true and 42, forward reference to Ch11 union types — appropriate), FunctionCall type checking (parameter arity check + per-arg compatibility — correct), FieldAccess/Index returning Dynamic (tables not modeled yet — honest simplification), TypeEnv as part of query key (same expression + different env = different result — correct Salsa design), salsa::Update derive necessity (explained, not optional — correct), type_check_program vs top_level_types duplication (acknowledged, justified for tutorial clarity — acceptable).

Praise: The Dynamic vs Error distinction in the opening section is excellent. “One is intentional flexibility; the other is a bug.” That one sentence does more work than three paragraphs of hedging. The reader understands the design philosophy before seeing a single line of code.

Praise: The arithmetic-vs-concat inconsistency note is exactly the kind of honest simplification the SOUL.md demands. The writer explains why the choices differ, acknowledges both are defensible, and commits to consistency within each operator. This is how you earn a reader’s trust.

Outstanding Item Reminder

[clarity] Ch8 “Why Cycles Happen” table says parse can loop from circular imports — it can’t. parse is per-file with no cross-file dependencies. Circular imports create cycles in type_check, not parse. The table row should say: File A requires B, B requires A — type_check loops. Drop parse from the cycle source. Unfixed since 2026-06-16 14:33 UTC review.

Status

  • 0 outstanding [error] items ✅
  • **0 outstanding [clarity] items ✅ — Ch8 circular imports table fixed (commit 1707c44)

Esme review 2026-06-21 20:35: Deep re-read Ch7 (Language Server, ~180 lines). Reading as a reader who understood Chapters 1–6.

0 new [error], [clarity], or [style] items. The chapter remains in strong shape.

Verified on re-read: LanguageServer struct ownership pattern (db: Database owned — set_text needs &mut, input IDs stable across revisions — correct Salsa pattern, the comment is precise), edit cycle walkthrough (set_text → type_check re-runs → parse re-runs → infer_type cached for unchanged sub-expressions — correct Salsa invalidation model), per-file isolation (opening other.lua doesn’t invalidate main.lua’s cached type check — correct, Salsa knows queries don’t depend on unrelated source text), timing section (honest about small demo files — “both operations complete in microseconds and the timing difference is negligible” — good, doesn’t oversell), SourceFile::new constructor (Salsa input pattern — correct), accumulated:: call (accumulator pattern from Ch6 — correct).

Praise: The edit cycle numbered walkthrough is the chapter’s strongest teaching moment. Seeing infer_type(x) → cached! in steps 5-6, right after the full re-parse in step 3, makes Salsa’s granularity viscerally clear. The reader sees that parse re-runs but type inference doesn’t — that’s the whole point, demonstrated rather than asserted.

Status

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅
  • 0 outstanding [style] items ✅

Esme review 2026-06-22 02:35: Deep re-read Ch9 (Cross-File Type Checking, ~359 lines). Reading as a reader who understood Chapters 1–8.

0 new [error], [clarity], or [style] items.

Verified on re-read: dependency graph diagram (arrow direction correct — main requires utils), resolve_module as conceptual stepping stone (the “From filesystem to database” callout handles the transition cleanly — the reader understands why the function exists and why they won’t call it), module_exports query (correct Salsa tracked function — reads parse and type_check, creates automatic dependency), StmtResult return-type propagation (the Statement::Function arm correctly collects return type from body and builds Type::Function — the structural change from TypeEnv to StmtResult is mentioned in prose without showing the struct definition, which is fine since the reader can infer it from the pattern match), Type::Table forward reference handling (well-managed — the “What About Type::Table?” section explains the variant exists but isn’t produced yet, preventing a reader from wondering where it comes from when they see it in the code), resolve_require trim_matches(‘“’) (correctly explained by analisar’s LiteralString including quotes), OnceLock registry approach (properly caveated with the production pattern note), set_text(&mut db).to() API (correct Salsa 0.26 setter pattern), cross-file invalidation flow (edit math.lua → double returns String → main.lua re-checked → type error on result + 1 — the full arc is traced in the Step 5 example output).

Praise: The “From filesystem to database” callout is exactly the right move at exactly the right time. A reader building along would have just written resolve_module and would naturally try to use it in resolve_require. The callout catches them before they go down the wrong path — not by saying “don’t do this” but by explaining why the database approach is different. The resolution logic is the same; the lookup location changes. That distinction is the whole point of Salsa, and this callout makes it concrete.

Praise: The “What About Cycles?” section is a model of how to handle out-of-scope topics. Two paragraphs: what would happen, what Salsa does about it, and a single forward reference to the dedicated chapter. The reader knows the problem exists, knows there’s a solution, and knows where to find it. No hand-waving, no false promises.

Status

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅
  • 0 outstanding [style] items ✅

Esme review 2026-06-26 02:35: Deep re-read Ch4 (Tracked Structs, ~260 lines). Reading as a reader who understood Chapters 1–3.

1 new [error] item found. Previous [clarity] items marked [fixed].

[error] Step 3 says “Tracked structs have unique IDs even if their fields are identical.” This is factually wrong for Salsa 3.x. In Salsa 3.x, tracked structs are identity-keyed by their untracked fields — two FuncDef instances with the same name get the same ID, even if their tracked fields (body_text, param_count) differ. The Salsa plumbing docs confirm: “The untracked fields are hashed to produce an IdentityHash… if a struct with this identity already exists, the existing struct is retrieved.” Step 7 correctly describes this behavior (“add’s FuncDef keeps the same ID — identity is keyed by the untracked fields”), but Step 3’s blanket claim directly contradicts it. The fix: rewrite the interned-vs-tracked comparison to say interned structs are deduplicated by all fields (same content → same ID) while tracked structs are deduplicated by identity fields (same untracked fields → same ID; tracked fields can change without changing identity).

Verified on re-read: FuncName as separate interned type (good — the newtype-style distinction from Symbol is well-explained, the “type system catches the mistake” line is precise), per-field dependency tracking with #[tracked] (correct — Step 4’s func_signature/func_complexity pair demonstrates the payoff clearly), wrapper/data pattern section (honest about not using it — correct for a tutorial), parse_functions implementation (comment about Lua parameter lists is correct — find(')') works because parameter lists are just identifiers), the Step 7 walkthrough (tracing the edit-add-mul cycle through Salsa’s revision tracking is the chapter’s best teaching moment — the reader sees per-field invalidation in action).

Praise: Step 7 is what makes this chapter work. The numbered walkthrough of “edit add’s body → what happens to each query” is concrete, traceable, and correct. The per-field tracking payoff lands because the reader has watched func_signature(add) return its cached value while func_complexity(add) re-runs — that’s not an abstract explanation, it’s a demonstration. More of this.

Status

  • 0 outstanding [error] items ✅ — Step 3 tracked struct identity-keyed fix verified (commit d1fa3bc)
  • 0 outstanding [clarity] items ✅
  • 0 outstanding [style] items ✅

Esme review 2026-06-26 14:35: Deep re-read Ch9 (Cross-File Type Checking, ~280 lines). Reading as a reader who understood Chapters 1–8.

No new Lola/Ada commits since 08:35 UTC. Deep re-read of src/09-cross-file/README.md.

0 new [error], [clarity], or [style] items. The chapter is in strong shape.

  • The require("utils") → Dynamic opening problem is concrete and well-motivated. The reader immediately sees why cross-file checking matters.
  • module_exports as a #[salsa::tracked] query is the right pattern — derived from source, automatically invalidated. The “magic line” explanation (module_exports(db, module_file)) is clear and honest about what Salsa does automatically.
  • resolve_module checks the filesystem; resolve_require looks up in FileRegistry. The “From filesystem to database” callout correctly explains why the two are separate. Good — a reader who skims might try to use resolve_module directly and get confused about why it doesn’t work with the Salsa database.
  • The OnceLock for FileRegistry — the ⚠ production-pattern caveat is honest. The key point (write-once → practically deterministic → doesn’t violate Salsa purity in practice) is correct.
  • resolve_require trims quotes from module_name because analisar’s LiteralString includes source-level quotes. This is the kind of concrete, implementation-specific detail that saves a reader debugging time.
  • The check_stmt for Statement::Function — infers return type from the body, creates Type::Function { params: [Dynamic], ret: inferred }. The note about avoiding duplicate diagnostics (by having check_block emit them during the function check, not calling infer_type again) is important and clearly explained.
  • The “What About Type::Table?” section is honest about the placeholder — no code path produces it yet. This is infrastructure, not a forward reference trap, because it’s explicitly scoped.
  • The “What We’re Simplifying” section covers real gaps (package.path, module caching, field-level exports, function param types, multiple return paths). Each is honest about what’s missing and why.
  • The cycle reference to Chapter 8 is legitimate series navigation.
  • The architecture before/after diagram is clear — per-file isolation vs. dependency-tracked cross-file. Appropriate use of a diagram for a structural comparison.
  • No “What You’ll Learn” section. No forward references to undefined concepts. No analogies that assume programming knowledge.

Status

  • 0 outstanding [error] items ✅
  • 0 outstanding [clarity] items ✅
  • 0 outstanding [style] items ✅