Fix #186: every arm of a views module gets both doors, not just the nameable ones - #187
Merged
Conversation
…ameable ones A views module is `from<X>` (put a value in) plus `as<X>` (read one back out, #122). The reader was emitted only for a structured KIND (record/callback/tuple/named) or an arm with an explicit name hint — while the CONSTRUCTOR name falls back to the arm's kind. Two different sources for the same decision, so they disagreed, and an arm could get a one-way door: `fromArray: array<string> => t` with no `asArray`. That split tracked NAMING, not soundness. The tell is that it produced distinctions nobody would defend: · `string[]` -> NO reader · `string[][]` -> reader, purely because TS names the inner element `Array` so a hint existed · `number` -> NO reader · `boolean` -> reader, only because `opaqueUnion` hard-codes `name: 'Bool'` The fix makes the gate MIRROR `fromName` instead of re-deriving the arm's identity: every arm reaching the generic `from*` branch gets both doors. This DELETES a special case rather than adding one. The arms that legitimately need no reader (`tagSet`, literal, `none`) each ARE their own runtime value and already `continue` above the gate. CONSUMER COST THIS CLOSES. Highcharts `ColorType = ColorString | GradientColorObject | PatternObject` is RETURNED by `color(…).get()` (`HighchartsSharedTypes.res:2550`, `get: option<string> => ColorType.t`) and almost always holds the plain color string. With no `asString`, the only reader that compiled was `asGradientColorObject` — reading a string as a record, silently, since these views are unchecked. `ColorType.t` appears 1085 times in blend's bindings. `asString` is exactly as safe as the `asGradientColorObject` beside it: both are zero-cost views whose arm the caller must establish first, which is this module's contract either way. ONE DELIBERATE EXCLUSION, and it is now principled where the old one was incidental: a TYPE-VARIABLE arm keeps `fromTypeVar` with no reader, because `as…: t => 'a` unifies with ANY type at the call site — a universal unsafe cast, not a view of one arm. base-ui's `RootFilteredItems` is the real instance. MEASURED, then reconciled exactly. Across the baselines, 53 of 1624 views modules had a reader-less arm (42 of them the `string | number | record` shape). Counting the individual missing readers predicted 96; the fix adds 95, and the 1 remainder is the deliberately excluded typeVar arm. The baseline diff is 95 insertions and ZERO deletions or modifications across 6 files — no existing declaration changes, so unlike #185 there is no rename churn. No metrics.json moved: this adds capability without reclassifying anything. FIXTURE HAS TEETH IN BOTH DIRECTIONS, verified by breaking it two ways: · revert to the old name-hint gate -> 2 cases fail (views-module-readers, record-props) · drop the typeVar exclusion -> 1 case fails (views-module-readers) The fixture carries the `string[]` vs `string[][]` pair specifically so that a future change re-coupling the reader to the name hint makes them diverge again and fails. It also pins the typeVar exclusion in a GOLDEN rather than only in the benchmark, since the benchmark is an opt-in gate that a wholesale "emit readers unconditionally" simplification would sail past. npm test passes, 113 goldens match and compile, benchmark 10/10 identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Benchmark: ✅ PASS
|
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #186.
The bug in one line
A views module emits
from<X>(put a value in) and, since #122,as<X>(read one back out). The constructor name falls back to the arm's kind when there's no name hint; the reader gate had no fallback. Two sources for one decision, so they disagreed:An arm with a constructor but no derivable hint got a one-way door.
Why it's a bug and not a policy
The split tracked naming, not soundness, and it produced distinctions nobody would defend:
string[]string[][]Array, so a hint existednumberbooleanopaqueUnionhard-codesname: 'Bool'string[]having no reader whilestring[][]has one is the give-away. Nothing about safety separates them:asArray: t => array<string>is exactly as unchecked as theasNamedAnext to it, and both require the caller to establish the arm first — which is this module's contract either way.The consumer cost this closes
Highcharts, real and in the baselines:
ColorType.tis returned by the library —HighchartsSharedTypes.res:2550,get: option<string> => ColorType.t, i.e.Highcharts.color("#ff0000").get()— and what comes back is almost always the plain string. Before:There was no way to get the string back out, and the only thing that compiled was a reader for the wrong shape — silently, since these are zero-cost
%identityviews with no runtime check.ColorType.tappears 1085 times in blend's bindings.The fix
Make the reader gate mirror
fromNamerather than re-derive the arm's identity — every arm reaching the genericfrom*branch gets both doors. This deletes a special case instead of adding one. Arms that legitimately need no reader (tagSet, literal,none) each are their own runtime value and alreadycontinueabove the gate.module ColorType = { type t external fromString: string => t = "%identity" + external asString: t => (string) = "%identity" external fromGradientColorObject: gradientColorObject => t = "%identity" external asGradientColorObject: t => (gradientColorObject) = "%identity" module ChartsSonificationSpeechMappingOptionsPitch = { external fromString: string => t = "%identity" + external asString: t => (string) = "%identity" external fromNumber: float => t = "%identity" + external asNumber: t => (float) = "%identity"One deliberate exclusion, now principled where the old one was incidental: a type-variable arm keeps
fromTypeVarand gets no reader, becauseas…: t => 'aunifies with any type at the call site — a universal unsafe cast, not a view of one arm. base-ui'sRootFilteredItems(readonly any[] | readonly Group<any>[]) is the real instance, and the fixture reproduces it.Measured, then reconciled exactly
53 of 1624 views modules across the baselines had a reader-less arm; 42 were the
string | number | recordshape. Counting individual missing readers predicted 96; the fix adds 95, and the 1 remainder is the excluded typeVar arm.Baseline diff: 95 insertions, 0 deletions or modifications, 6 files. No existing declaration changes, so — unlike #185 — no rename churn. No
metrics.jsonmoved: this adds capability without reclassifying anything.Teeth, verified in both directions
Broke it two ways to confirm the fixture actually fails:
views-module-readers,record-props)views-module-readers)The fixture carries the
string[]vsstring[][]pair on purpose, so a future change re-coupling the reader to the name hint makes them diverge again and fails. It also pins the typeVar exclusion in a golden, not only in the benchmark — the benchmark is an opt-in gate that a wholesale "emit readers unconditionally" simplification would sail past.Verification
npm test— smoke + 113 goldens matchnpm run test:compile— 113/113 compile on ReScriptnpm run bench— 10/10 identical to (updated) baselinesdocs/TYPE_MAPPING.md— "Opaque-module unions" section updated; the old text said "every structured arm"🤖 Generated with Claude Code