diff --git a/docs/architecture-refactor-plan.md b/docs/architecture-refactor-plan.md new file mode 100644 index 0000000..7be4b73 --- /dev/null +++ b/docs/architecture-refactor-plan.md @@ -0,0 +1,267 @@ +# Architecture Refactor Plan + +Goal: make the jolt codebase easier to understand and safe to change — for human +maintainers and, specifically, for LLM agents. An agent should be able to find +where a feature lives, see its related code, and make a change without scanning +3000-line files or keeping invisible global state in mind. + +This plan synthesizes a six-part architectural review (one reviewer per +subsystem). It is organized as **independent, gate-validated phases**, ordered by +value-to-risk. Each phase is a PR-sized unit. Nothing here changes behavior; it is +pure reorganization plus a small set of dead-code/bug deletions. + +## Non-negotiable constraints (apply to every phase) + +- **Every phase passes the full gate**: `jpm test` green (conformance ×3 modes, + `clojure-test-suite` ≥ baseline, bench back-to-back vs main, real exit code). + `rm -rf build && jpm clean` before trusting the binary. +- **Load order is load-bearing.** The Janet seed (`src/jolt/*.janet`) and the + Clojure overlay tiers (`jolt-core/clojure/core/NN-*.clj`) load in a fixed order; + a module may only use what loads before it. Splitting a file must preserve the + net load order (a new file is imported where the old code ran). +- **Seed-tier discipline** (the jolt-tzo rules in CLAUDE.md): nothing the + analyzer/IR use may move below the kernel tier; a tier may only use macros from + earlier tiers; expander-called fns stay in 00-syntax. +- **One concern per PR.** Do not combine a file split with a behavior fix. + Dead-code/bug deletions (Phase 0) land first and separately so later diffs are + pure moves. + +## Guiding principles (the target state) + +1. **One obvious home per feature.** Adding a `.someMethod` interop shim, a reader + macro, or a clojure.core fn should have exactly one file an agent edits. +2. **Files map to concerns, not to history.** No 3000-line grab-bags; no module + whose name lies about its contents (`javatime.janet` is 80% not java.time; + `phm.janet` contains LazySeq; `types.janet` holds seven concerns). +3. **Make implicit contracts explicit and checked.** The seed↔overlay split, the + ctx-shaping env-knob list, and the IR op set are tribal knowledge today; turn + each into a single source of truth with a drift check. +4. **No copy-paste dispatch.** Where the same op-set / member-dispatch / cache + dance is hand-written N times, extract one combinator. + +--- + +## Phase 0 — Dead code & concrete bugs (low risk, do first) + +Pure deletions and small fixes, each with a regression row. These remove +*actively misleading* code (comments that contradict behavior) before any moves. + +| Item | Location | Note | +|---|---|---| +| `find` defined twice; the dead copy returns a plain vector and is *wrong* | `jolt-core/clojure/core/20-coll.clj:347-349` | live def at :787 returns a real map-entry; delete the dead one | +| `core-satisfies?` always returns `false` (latent bug + misleading comment) | `src/jolt/core.janet:2412` | either implement over the protocol registry or document why inert; fix the `eval-list` comment that claims it's an overlay fn | +| `File.toURL` stores `:url` but every `:jolt/url` method reads `:spec` | `src/jolt/javatime.janet:636` | broken shim; use `:spec`, add a spec row | +| `core-type->str` — zero references | `src/jolt/core.janet:2416` | delete defn + binding | +| `core-resolve` — unreachable (overridden by `install-stateful-fns!`) | `src/jolt/core.janet:2365` | delete defn + binding; fix stale comment | +| `mark-hint` — unreachable | `jolt-core/jolt/passes.clj:836` | delete | +| `pad2` defined twice | `src/jolt/javatime.janet:41,522` | keep one | +| `phs-to-struct`, `shape-vals`, `ns-imports-fn` — zero references | `phm.janet:302`, `types.janet:655`, `types.janet:527` | delete unless reserved API | +| `pl-rest` no-op `(if (plist? r) r r)` | `src/jolt/plist.janet:62` | collapse / fix; regression check | +| `read-quote` unused `pos` param | `src/jolt/reader.janet:608` | drop | +| Stale/contradictory comments (`extend`, orphan `;; trampoline:` / `;; rand-int:` headers, migration breadcrumbs) | `30-macros.clj:402-404`, `20-coll.clj:558-560,780,790` | sweep | +| **`:map-shapes?` missing from the deps-image cache key** (possible stale-image correctness gap) | `src/jolt/main.janet:440-448` | add to key; confirm with a test | + +--- + +## Phase 1 — Extract the host-interop subsystem (highest value) + +**Problem (corroborated by 3 reviewers).** The JVM-emulation shim layer is the +single worst sprawl, and it is exactly where the recent hiccup/markdown/malli work +landed ad hoc. A single class is split across up to three files: + +- registry *machinery* (`class-statics`/`tagged-methods`/`class-ctors` + + `register-*!`) lives in `evaluator.janet:624-651`; +- four static tables (`Math`/`Thread`/`System`/`Long`/`Number`) are hardcoded in + `evaluator.janet:537-619` and dispatched *outside* the registry in `resolve-sym` + (780-794); instance-method tables (`string/number/object-methods`, 698-775) are + likewise inline; +- the bulk of the shims are in `javatime.janet` (674 lines, ~80% not java.time: + java.io/util/net/nio/sql/text/math all in one `install-io!`); +- `api.janet:18-56` wires the malli statics + `set-coll-interop!` as load-time + side effects in the public-API module; +- `core.janet` holds `File`/JDBC constructors; `types.janet` holds the `:jolt/inst` + representation. + +**Target.** A `src/jolt/interop/` directory, one mechanism file + one file per JDK +area, each owning a class's *full* surface (ctor + statics + methods + `instance?` +predicate + canonical name) and exposing one `install!`: + +``` +src/jolt/interop/ + registry.janet # MOVED from evaluator: class-statics/tagged-methods/class-ctors, + # register-*!, canonical-names, value-overrides, instance? registry + coerce.janet # shared: chr, char->byte, render-piece/writer-piece, pad2, + # the date-format token walker (kills the pad2/render-piece dups) + java_time.janet java_io.janet java_net.janet java_util.janet + java_lang.janet # absorbs evaluator's inline Math/Thread/System/Long/Number + + # string/number/object method tables — the registry becomes the + # ONLY static/method-dispatch mechanism + jdbc.janet + install.janet # (install-all! ctx) — the ONE place api.janet calls (replaces api 18-56) +``` + +`evaluator.janet`'s `resolve-sym` and the dot-dispatch consult only the registry. +`regex.janet` and `async.janet` stay put — they are engines/library-ports consumed +by interop, not shims. + +**Cheap 80% if the full split is too big for one PR:** rename +`javatime.janet`→`host_interop.janet`, pull the four inline static tables + three +method tables out of `evaluator.janet` into it, move `api.janet:18-56` into it, +dedup `pad2`/`chr`/`render-piece`. Collapses the scatter from 5 files to 2. + +**Risk:** medium — touches the hot dot-dispatch path; load order must keep the +registry available before any `install!`. Validate with `host-interop-spec` + +the hiccup/markdown/malli example apps + full gate. + +--- + +## Phase 2 — Decompose the god-files + +The three biggest interpreter/runtime files are the top LLM-navigability tax. +Split along the cohesive clusters the review mapped (line ranges in the review +notes). Each split is a mechanical move + import; behavior unchanged. + +### 2a. `evaluator.janet` (2681 → ~5 files) +- `special_forms.janet` — explode the **680-line `eval-list`** (1921-2599) into + named `eval-