diff --git a/docs/plans/2026-05-10-22-tidy-strict-clause-helper.md b/docs/plans/2026-05-10-22-tidy-strict-clause-helper.md new file mode 100644 index 0000000..3acf94c --- /dev/null +++ b/docs/plans/2026-05-10-22-tidy-strict-clause-helper.md @@ -0,0 +1,396 @@ +# 22-tidy.7 strict-clause helper consolidation — Implementation Plan + +> **Parent spec:** `docs/specs/2026-05-09-22-typeclasses.md` (milestone 22, closed 2026-05-09) +> +> **Trigger:** 22-tidy.5 JOURNAL entry observation that the duplicate-clause check pattern now appears at 17 sites across the parser (10 from 22b.4a + 8 from 22-tidy.5; the JOURNAL entry's "18 sites" was an off-by-one — re-grep confirms 17). 17 × ~12 LOC of identical structure is past the tipping point for the "three similar lines beats a premature abstraction" rule. +> +> **For agentic workers:** REQUIRED SUB-SKILL: use `skills/implement` +> to run this plan. Steps use `- [ ]` checkboxes for tracking. + +**Goal:** consolidate the 17 inline `if X.is_some() { return Err(ParseError::Production { … }); }` blocks in `crates/ailang-surface/src/parse.rs` into one helper method `Parser::duplicate_clause_err(&self, production: &'static str, subject: &str, clause: &'static str) -> ParseError`, reducing ~150 LOC of duplication to ~17 one-liner call sites. + +**Architecture:** add one method on `Parser` that captures `pos` from `self.peek()` and constructs the `ParseError::Production` shape. Three message templates exist in the current code: +- `" \`\` has duplicate \`( ...)\` clause"` (13 sites: data, fn, const, class, class method) +- `"instance has duplicate \`( ...)\` clause"` (3 sites) +- `"instance method \`\` has duplicate \`( ...)\` clause"` (1 site) + +All three are unified under one helper that takes the message *subject* (everything before `" has duplicate"`) as a `&str`, leaving construction at the call site. Caller passes `&format!("fn `{name}`")` for named productions and the literal `"instance"` for the nameless instance form. + +**Tech Stack:** `crates/ailang-surface/src/parse.rs` only — one file. + +**Files this plan creates or modifies:** + +- Modify: `crates/ailang-surface/src/parse.rs` — adds `Parser::duplicate_clause_err` method; rewrites 17 inline blocks (lines 376-385, 497-506, 511-520, 524-533, 537-546, 699-708, 712-721, 725-734, 782-791, 798-807, 811-820, 880-890, 893-903, 947-955, 961-969, 972-980, 1025-1034) into one-line calls. +- Modify: `docs/JOURNAL.md` — append iteration entry. + +**Helper signature and body:** + +```rust +fn duplicate_clause_err( + &self, + production: &'static str, + subject: &str, + clause: &'static str, +) -> ParseError { + let pos = self.peek().map(|t| t.span.start).unwrap_or(0); + ParseError::Production { + production, + message: format!("{subject} has duplicate `({clause} ...)` clause"), + pos, + } +} +``` + +**Call-site shape (post-refactor):** + +```rust +// Named production (fn / const / class / class method / data): +if X.is_some() { + return Err(self.duplicate_clause_err("fn-def", &format!("fn `{name}`"), "doc")); +} + +// Instance (no name): +if class.is_some() { + return Err(self.duplicate_clause_err("instance-def", "instance", "class")); +} + +// Instance method (with name): +if body.is_some() { + return Err(self.duplicate_clause_err("instance.method", &format!("instance method `{name}`"), "body")); +} +``` + +--- + +## Task 1: introduce `duplicate_clause_err` and rewrite all 17 call sites + +**Files:** +- Modify: `crates/ailang-surface/src/parse.rs` — add helper near other `Parser` methods (suggest: just below `expect_string` at lines 595-610 since both are small private helpers); rewrite 17 inline blocks. + +This is a single mechanical refactor — bundling helper introduction with all call-site rewrites in one commit because: +1. The helper has no consumers without the rewrites; an interim commit with helper + zero callers fires `dead_code` warnings. +2. Splitting by parser (data / fn / const / class / instance) creates intermediate commits where the helper exists with partial callers — bisection still works (each call-site rewrite is independent), and the test suite stays green at every commit. +3. The 17 sites are textually parallel; reviewing them as one diff gives the reviewer the apples-to-apples context that a per-parser split would lose. + +- [ ] **Step 1: add the helper method** + +In `crates/ailang-surface/src/parse.rs`, locate `fn expect_string` (around line 595). Just below it (and above the next helper), add: + +```rust +fn duplicate_clause_err( + &self, + production: &'static str, + subject: &str, + clause: &'static str, +) -> ParseError { + let pos = self.peek().map(|t| t.span.start).unwrap_or(0); + ParseError::Production { + production, + message: format!("{subject} has duplicate `({clause} ...)` clause"), + pos, + } +} +``` + +- [ ] **Step 2: replace the `parse_data` doc-clause block (line 376-385)** + +Locate the block: + +```rust + Some("doc") => { + if doc.is_some() { + let pos = self.peek().map(|t| t.span.start).unwrap_or(0); + return Err(ParseError::Production { + production: "data-def", + message: format!( + "data `{name}` has duplicate `(doc ...)` clause" + ), + pos, + }); + } + doc = Some(self.parse_doc()?); + } +``` + +Replace with: + +```rust + Some("doc") => { + if doc.is_some() { + return Err(self.duplicate_clause_err("data-def", &format!("data `{name}`"), "doc")); + } + doc = Some(self.parse_doc()?); + } +``` + +- [ ] **Step 3: replace `parse_fn`'s 4 clause blocks** + +Locate `parse_fn` (around line 485). Replace its four duplicate-clause blocks (`doc`, `type`, `params`, `body`): + +```rust + Some("doc") => { + if doc.is_some() { + return Err(self.duplicate_clause_err("fn-def", &format!("fn `{name}`"), "doc")); + } + doc = Some(self.parse_doc()?); + } + Some("suppress") => suppress.push(self.parse_suppress_attr()?), + Some("type") => { + if ty.is_some() { + return Err(self.duplicate_clause_err("fn-def", &format!("fn `{name}`"), "type")); + } + ty = Some(self.parse_type_attr()?); + } + Some("params") => { + if params.is_some() { + return Err(self.duplicate_clause_err("fn-def", &format!("fn `{name}`"), "params")); + } + params = Some(self.parse_params_attr()?); + } + Some("body") => { + if body.is_some() { + return Err(self.duplicate_clause_err("fn-def", &format!("fn `{name}`"), "body")); + } + body = Some(self.parse_body_attr()?); + } +``` + +- [ ] **Step 4: replace `parse_const`'s 3 clause blocks** + +Locate `parse_const` (around line 691). Replace its three duplicate-clause blocks (`doc`, `type`, `body`): + +```rust + Some("doc") => { + if doc.is_some() { + return Err(self.duplicate_clause_err("const-def", &format!("const `{name}`"), "doc")); + } + doc = Some(self.parse_doc()?); + } + Some("type") => { + if ty.is_some() { + return Err(self.duplicate_clause_err("const-def", &format!("const `{name}`"), "type")); + } + ty = Some(self.parse_type_attr()?); + } + Some("body") => { + if value.is_some() { + return Err(self.duplicate_clause_err("const-def", &format!("const `{name}`"), "body")); + } + value = Some(self.parse_body_attr()?); + } +``` + +- [ ] **Step 5: replace `parse_class`'s 3 clause blocks** + +Locate `parse_class` (around line 771). Replace its three duplicate-clause blocks (`param`, `superclass`, `doc`): + +```rust + Some("param") => { + if param.is_some() { + return Err(self.duplicate_clause_err("class-def", &format!("class `{name}`"), "param")); + } + self.expect_lparen("class.param")?; + self.expect_keyword("param")?; + param = Some(self.expect_ident("class param ident")?); + self.expect_rparen("class.param")?; + } + Some("superclass") => { + if superclass.is_some() { + return Err(self.duplicate_clause_err("class-def", &format!("class `{name}`"), "superclass")); + } + superclass = Some(self.parse_superclass()?); + } + Some("doc") => { + if doc.is_some() { + return Err(self.duplicate_clause_err("class-def", &format!("class `{name}`"), "doc")); + } + doc = Some(self.parse_doc()?); + } +``` + +- [ ] **Step 6: replace `parse_class_method`'s 2 clause blocks** + +Locate `parse_class_method` (around line 871). Replace its two duplicate-clause blocks (`type`, `default`): + +```rust + Some("type") => { + if ty.is_some() { + return Err(self.duplicate_clause_err("class.method", &format!("method `{name}`"), "type")); + } + ty = Some(self.parse_type_attr()?); + } + Some("default") => { + if default.is_some() { + return Err(self.duplicate_clause_err("class.method", &format!("method `{name}`"), "default")); + } + self.expect_lparen("class.default")?; + self.expect_keyword("default")?; + default = Some(self.parse_term()?); + self.expect_rparen("class.default")?; + } +``` + +(Note: the exact body inside the default-block depends on the existing code. If the inner steps differ, preserve them — this Step 6's job is the `if X.is_some()` guard ONLY, not the rest of the arm. Read lines 880-908 in the source and preserve the non-guard logic verbatim.) + +- [ ] **Step 7: replace `parse_instance`'s 3 clause blocks (no-name form)** + +Locate `parse_instance` (around line 940). Replace its three duplicate-clause blocks (`class`, `type`, `doc`) — these use the no-name `"instance"` subject: + +```rust + Some("class") => { + if class.is_some() { + return Err(self.duplicate_clause_err("instance-def", "instance", "class")); + } + self.expect_lparen("instance.class")?; + self.expect_keyword("class")?; + class = Some(self.expect_ident("instance class name")?); + self.expect_rparen("instance.class")?; + } + Some("type") => { + if type_.is_some() { + return Err(self.duplicate_clause_err("instance-def", "instance", "type")); + } + self.expect_lparen("instance.type")?; + self.expect_keyword("type")?; + type_ = Some(self.parse_type()?); + self.expect_rparen("instance.type")?; + } + Some("doc") => { + if doc.is_some() { + return Err(self.duplicate_clause_err("instance-def", "instance", "doc")); + } + doc = Some(self.parse_doc()?); + } +``` + +(Same caveat as Step 6: preserve the non-guard logic verbatim if the existing inner body differs.) + +- [ ] **Step 8: replace `parse_instance_method`'s 1 clause block** + +Locate `parse_instance_method` (around line 1015). Replace its single duplicate-clause block (`body`): + +```rust + Some("body") => { + if body.is_some() { + return Err(self.duplicate_clause_err("instance.method", &format!("instance method `{name}`"), "body")); + } + body = Some(self.parse_body_attr()?); + } +``` + +(Preserve the non-guard logic if it differs.) + +- [ ] **Step 9: build clean** + +Run: `cargo build --workspace 2>&1 | tail -5` +Expected: builds clean. If a `dead_code` warning fires on `duplicate_clause_err`, verify all 17 sites were rewritten — the helper is private and needs a consumer. + +- [ ] **Step 10: full ailang-surface test sweep stays green** + +Run: `cargo test --workspace -p ailang-surface 2>&1 | grep -E "^test result" | tail -5` +Expected: every line `test result: ok.`; no FAILED. The 8 duplicate-clause unit tests added in 22-tidy.5 (4 fn + 3 const + 1 data) exercise the helper transitively; if any of them flips, the helper's message format diverges from what they assert. + +- [ ] **Step 11: full workspace test sweep** + +Run: `cargo test --workspace 2>&1 | grep -c "FAILED"` +Expected: `0`. + +- [ ] **Step 12: confirm net LOC reduction** + +Run: `git diff --stat HEAD` (after all replacements, before commit) +Expected: net negative line count on `crates/ailang-surface/src/parse.rs` — adding ~12 lines for the helper and removing ~150 lines (≈ 17 sites × ~9 lines avg saved each). + +- [ ] **Step 13: commit** + +```bash +git add crates/ailang-surface/src/parse.rs +git commit -m "iter 22-tidy.7: extract duplicate_clause_err helper, retire 17 inline blocks" +``` + +--- + +## Task 2: verify + JOURNAL + +**Files:** +- Modify: `docs/JOURNAL.md` — append iteration entry. + +- [ ] **Step 1: bench gates** + +Run: `python3 bench/check.py 2>&1 | tail -1 && python3 bench/compile_check.py 2>&1 | tail -1 && python3 bench/cross_lang.py 2>&1 | tail -1` +Expected: each line ends `0 regressed`. + +- [ ] **Step 2: confirm only 1 site retains the inline `ParseError::Production` + `format!("...has duplicate...")` shape, and that site is the helper itself** + +Run: `grep -c 'has duplicate \`' crates/ailang-surface/src/parse.rs` +Expected: `1` (the helper's own format-string template) plus the 8 test-assertion `contains(...)` strings = 9 hits if grep counts contains too. Visually inspect: the inline `ParseError::Production { production: ..., message: format!("...has duplicate...") ... }` shape should appear at zero non-helper sites. + +- [ ] **Step 3: append JOURNAL entry** + +Append to `docs/JOURNAL.md`: + +```markdown +## 2026-05-10 — Iteration 22-tidy.7: strict-clause helper consolidation + +Closing the queued observation from 22-tidy.5: the +duplicate-clause check pattern had grown to 17 inline blocks +(10 from 22b.4a + 8 from 22-tidy.5) of identical 12-line shape +across `parse_data`, `parse_fn`, `parse_const`, `parse_class`, +`parse_class_method`, `parse_instance`, `parse_instance_method`. +17 × 12 LOC of textual duplication is past the tipping point; +this iter consolidates them. + +Introduced one private method +`Parser::duplicate_clause_err(&self, production: &'static str, +subject: &str, clause: &'static str) -> ParseError` that captures +the `pos` lookup, formats the canonical +`" has duplicate \`( ...)\` clause"` message, and +returns the `ParseError::Production` shape. All 17 inline blocks +reduced to one-line calls; net ~150 LOC removed. + +Three subject shapes are unified under the one helper by passing +the formatted subject as a parameter: `" \`\`"` +for named productions (fn, const, data, class, class method), +literal `"instance"` for the nameless instance form, and +`"instance method \`\`"` for the instance method form. The +production tag stays separate (it's the diagnostic code, not the +human-readable text). + +Tasks (commit subjects): + +- 22-tidy.7: extract duplicate_clause_err helper, retire 17 inline blocks + +Acceptance: `cargo test --workspace` 0 FAILED; the 8 +duplicate-clause unit tests from 22-tidy.5 still pass (their +`contains(...)` assertions match the helper's message format +verbatim); bench gates 0/0/0; net LOC reduction on +`crates/ailang-surface/src/parse.rs`. + +Observation (out-of-scope, queued): the 9 22b.4a-era duplicate- +clause sites (class × 3 + class method × 2 + instance × 3 + +instance method × 1) have no dedicated unit tests — they were +shipped without a regression pin. The 22-tidy.5 tests cover +fn/const/data only. Backfilling tests for the 22b.4a-era sites is +defensible but beyond the carried-debt scope; queue under +"parser-test backfill" if and when a 22b.4a-era diagnostic ever +needs to change. + +Carried-debt status (after this iter): + +- ✅ Primitive-name-set consolidation (22-tidy.4). +- ✅ Strict duplicate-clause detection in fn/const/data (22-tidy.5). +- ✅ Form-B (prose) printer arms for ClassDef / InstanceDef + (22-tidy.6). +- ✅ Strict-clause helper consolidation (this iter). +- ✅ Lib.rs gap-related sites — audit confirmed defensive. + +Milestone-22 carried debt + the queued tipping-point observation +are now both closed. Next milestone targets remain +orchestrator's call. +``` + +- [ ] **Step 4: commit** + +```bash +git add docs/JOURNAL.md +git commit -m "iter 22-tidy.7: journal entry" +```