Skip to content

Fix #191: reachability sweep drops return-only-generic orphan records - #195

Merged
jagguji merged 2 commits into
mainfrom
agent/191-reachability-sweep
Aug 17, 2026
Merged

Fix #191: reachability sweep drops return-only-generic orphan records#195
jagguji merged 2 commits into
mainfrom
agent/191-reachability-sweep

Conversation

@jagguji

@jagguji jagguji commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

A function whose type parameter appears only in its return (jsonBoxed<T>(): BoxOf<T>) has that return flagged — a return-only 'a can't round-trip (contract rule #4). But classify had already registered the record it built for the discarded return, and nothing un-registered it, so an unreferenced boxOf<'a> reached the output — and shoved the real readBox(): BoxOf<string> type off the clean base name to boxOfV1hnll.

The per-site snapshot/rollback used for the method path (#189) does not work here: the orphan survives through late name/key resolution at emit (#128 + stabilizeNames run after the rollback). So this adds the post-traversal reachability sweep that #178 explicitly called for.

What it does

sweepUnreachableEntries — after every IR tree is final but before stabilizeNames:

  • walks the emitted roots (component props + baseSpreads, function/const/context signatures, class members) and drops any registered entry unreachable from them;
  • is key-based (every module-mode ref is keyed via refTo), so a type reached only through a keyed ref whose home is late-bound (Callable-module home placement ignores prop-derived deps (deferred from #103 review) #128) is never wrongly dropped;
  • uses a deep walk (collectAllRefKeys) so refs inside inline-record fields and record spreads — which the shallow collectRefKeys skips — are covered;
  • runs before naming, so a live sibling reclaims a base name an orphan squatted;
  • removing an orphan can also dissolve a spurious SCC merge it was wedged into → cleaner module homes.

Before / after

jsonBoxed<T>(): BoxOf<T> + readBox(): BoxOf<string>:

// before
type boxOfV5tl5y<'a> = { v: 'a }   // orphan — referenced by nothing
type boxOfV1hnll   = { v: string }
// after
type boxOf = { v: string }         // clean; jsonBoxed still flagged unit => string

Validation

  • npm test — 116 golden cases match; zero existing goldens changed (no orphans there), only the new fixture added.
  • npm run test:compile — all 116 compile on ReScript.
  • npm run bench — real orphan removals surfaced and accepted: @smastrom/react-rating (inputProps<'a>, a callback-return generic) and @juspay/blend-design-system (plus a spurious HighchartsSharedTypes SCC merge dissolving into ChartsTypes); all metrics equal-or-better, all compile. Baselines updated in this PR.
  • Fixture: return-only-generic-orphan; docs/TYPE_MAPPING.md + CHANGELOG updated.

The #189 method-path rollback is kept as a cheap early exit; the sweep is now the general backstop (and the mechanism #178 asked for).

Fixes #191

🤖 Generated with Claude Code

A function whose type parameter appears ONLY in its return (`jsonBoxed<T>():
BoxOf<T>`) has that return flagged — a return-only `'a` can't round-trip (rule
#4). But `classify` had already registered the record it built for the discarded
return, and nothing un-registered it, so an unreferenced `boxOf<'a>` reached the
output (and shoved the real `readBox(): BoxOf<string>` type off the clean base
name to `boxOfV1hnll`).

The per-site snapshot/rollback used for the METHOD path (#189) doesn't work here:
the orphan survives through late name/key resolution at emit (#128 + stabilizeNames
run after the rollback). So instead, add the post-traversal reachability sweep #178
called for: after every IR tree is final but BEFORE stabilizeNames, walk the emitted
roots (component props + baseSpreads, function/const/context signatures, class
members) and drop any registered entry unreachable from them.

- Key-based (every module-mode ref is keyed via refTo), so a type reached only
  through a keyed ref whose home is late-bound is never wrongly dropped.
- A deep walk (collectAllRefKeys) covers refs inside inline-record fields and record
  spreads that the shallow collectRefKeys skips.
- Runs before naming, so a live sibling reclaims a base name an orphan squatted.
- Removing an orphan can also dissolve a spurious SCC merge, giving cleaner homes.

Fixture: return-only-generic-orphan. All 116 goldens compile; benchmark baselines
updated (real orphan removals in blend + react-rating, metrics equal-or-better).

Fixes #191
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jagguji
jagguji deployed to benchmark August 17, 2026 12:10 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown

Benchmark: ✅ PASS

Package Compile Diff vs baseline usable review broken Verdict
@juspay/blend-design-system@0.0.36 identical 102 5 0 ✅ PASS
@juspay/blend-design-system@0.0.37-beta.8 identical 215 7 0 ✅ PASS
@juspay/blend-design-system@0.0.37 identical 219 7 0 ✅ PASS
react-day-picker@10.0.1 identical 19 7 0 ✅ PASS
react-tooltip@6.0.7 identical 1 0 0 ✅ PASS
react-markdown@10.1.0 identical 0 2 0 ✅ PASS
@smastrom/react-rating@1.5.0 identical 1 0 0 ✅ PASS
clsx@2.1.1 identical 0 0 0 ✅ PASS
hono@4.12.25 identical 0 0 0 ✅ PASS
@base-ui-components/react@1.0.0-rc.0 identical 174 21 0 ✅ PASS

@pkg-pr-new

pkg-pr-new Bot commented Aug 17, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@juspay/rescript-bindgen@195

commit: 99d68cc

…s/statics

The class-member root walk seeded only ctor/methods/getters, but the class IR also
emits setters (`@set external …Set`) and static members (`@scope(...) external`).
A shared type reachable ONLY through a write-only setter or a static-only member
had no other path to a root, so the sweep dropped it and the emitted external
dangled — uncompilable output for any such class. (Getters+setters usually pair up,
so the getter hid this; no bench package has a write-only or static-only case.)

Add setters, staticMethods, and staticValues as roots (whole-element, since
collectAllRefKeys deep-walks). Fixture: class-setter-static-reachable (a write-only
setter + a static-only type), compile-gated. Also note the early-out counts `>=`
because `reachable` can hold non-entry `deps` keys (safe-direction: skips the sweep,
never drops a live type).

All 117 goldens compile; the #191 orphan fix is unchanged.
@jagguji
jagguji deployed to benchmark August 17, 2026 12:31 — with GitHub Actions Active
@jagguji
jagguji merged commit 3aadc35 into main Aug 17, 2026
11 of 12 checks passed
@jagguji
jagguji deleted the agent/191-reachability-sweep branch August 17, 2026 12:39
jagguji added a commit that referenced this pull request Aug 18, 2026
…ow-up) (#196)

Docs-only follow-up to #195 (the #191 reachability sweep), enriching the
`sweepUnreachableEntries` section of `docs/TYPE_MAPPING.md`.

## What changed
1. **Precise ReScript v12 vocabulary for the class-member roots** —
names exactly what each member emits and therefore why it must be a
root: `@new` ctor, `@send` methods, `@get` getters, `@set` write-only
setters, `@scope` statics (cross-checked against `emit.mjs` and the v12
interop cheatsheet). Replaces the vague "class members".
2. **The completeness invariant + second fixture link** — states why
*all* binding class members must be roots: a type reached only through a
`@set` setter or a `@scope` static has no other path, so dropping it
dangles the external → ReScript compile error. This is the exact
regression the #195 follow-up commit fixed; it's now written into the
contract doc and linked to the
[`class-setter-static-reachable`](../test/golden/cases/class-setter-static-reachable)
fixture (previously only `return-only-generic-orphan` was linked), so a
future maintainer won't trim the root walk back to getters.
3. **SCC acyclicity note** — clarifies that ReScript's cross-file module
graph must be acyclic (a dependency cycle between two `*Types.res` homes
is a compile error), which is why an orphan removal can dissolve a
forced merge (#35).

## Notes
- **Docs only** — no source/behavior change; both linked fixtures
already exist on `main`.
- Landed as a fresh branch off `main` because #195 is already merged (it
can't be reopened to receive this).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
jagguji added a commit that referenced this pull request Aug 18, 2026
…ivor scan

The post-sweep usesJsFn recompute scanned roots + entries.flatMap(entryChildTypes),
but entryChildTypes's record branch omits e.indexValue — the `@set_index` value
type. A SURVIVING record whose only JsFn.t is a Function-typed index signature
(`[k: string]: Function` -> `@set_index …Set: (rec, string, JsFn.t)`) was therefore
not scanned, so usesJsFn was wrongly cleared and JsFn.res dropped, leaving the
@set_index external dangling — uncompilable. (Same class as the #195 setter/static
gap: the survivor surface was narrower than what the emitter produces.)

Surgical fix: add each entry's indexValue to the survivor set (kept out of
entryChildTypes, which feeds deps/home/SCC and would move the bench). Fixture
jsfn-function-index-reachable guards it — JsFn.res stays in the emitted file set
and the output compiles.

All 119 goldens compile; benchmark unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return-only generic dead end strands an orphan record in the FUNCTION path (method path is fixed)

1 participant