Skip to content

docs: the live krapper service + FIR client design (#186 brick 3) - #204

Merged
monkopedia-reviewer merged 2 commits into
mainfrom
docs/live-service-design
Jul 29, 2026
Merged

docs: the live krapper service + FIR client design (#186 brick 3)#204
monkopedia-reviewer merged 2 commits into
mainfrom
docs/live-service-design

Conversation

@monkopedia-coder

Copy link
Copy Markdown
Collaborator

Design document only — no production change. Split out of PR #203, where a git add -A had accidentally swept it into the 0.3.5 release-prep commit; the review caught that #203 claimed "one line" while carrying 704 unrelated lines. This is where it belonged.

Why this is worth reading rather than just filing

The design corrects the framing #186 was opened with, in three specific ways. The owner ruled "the FIR plugin becomes a live client" partly on the strength of that framing, so the corrections are the most important content here:

  1. There is no live Clang AST today, anywhere. CppParser.parse wraps the entire parse in memScoped (CppParser.kt:76-83), so the ASTUnit — and ASTContext/Sema with it — is destroyed the moment the parse returns. Only the pure-Kotlin WrappedTU survives, and clang_slice.h binds zero Sema symbols. "A persistent Clang instance HAS Sema" is not a property persistence unlocks; it is new capability work that persistence makes affordable. Probe-gated (P1/P2), and the likely mechanism is a PCH + -fsyntax-only probe TU rather than a retained Sema at all.
  2. A compiler cannot materialize a binding mid-compilation. Generated bindings enter the build as a source dir (KPlusPlusCompilerGradlePlugin.kt:819) plus a cinterop klib (:803-815), both frozen before compileKotlinNative starts; a FIR-synthesized class would have no backing native symbol and would fail to link. The compiler can ask and request — the unit of materialization is the next sync.
  3. Lazy binding cannot be the default. docs/design/broad-forcing-resolve.md:210-224 already evaluated bounding the closure and rated it "disqualifying as the primary fix" — because it changes what gets bound, and featuregen's cross-instantiation story depends on INCLUDE_MISSING's transitivity.

#10's two premises become attackable, not dissolved. That is an honest downgrade from what was claimed when the direction was set.

What it proposes instead

A canonical binding index emitted by krapper (krapped/binding-index.json) and consumed by the FIR plugin as a compilation-start snapshot, with persistence, Sema and lazy binding as separate probe-gated bricks stacked above it.

§11 argues B1–B3 are ~80% of the realizable value with no persistence, no socket and no Sema — and they fix a live present-day defect: the FIR plugin re-derives krapper's KotlinWriter name mangling by hand (CppVectorMapping.kt:248-268) with nothing checking the two implementations against each other, so a generator-side change silently degrades to a spurious SYNC_REQUIRED on a binding that exists. It also carries a 4-entry Kotlin→C++ type table, so cppVector<Boolean>() is rejected even though krapper handles it.

Other content: a determinism contract (§5) with per-brick machine-checkable gates; the IDE/incrementality constraint that keeps bindings as files (§6); the #194 interaction (§7) including a hard constraint that the FIR plugin must stay dependency-free; a 10-brick sequence (§8); seven probes (§9), each cheap and each allowed to come back negative and kill its brick; risks (§10); and four open questions for the owner (§12).

Reviewer note

Docs-only. The useful review here is whether the citations are accurate and the reasoning holds — particularly the three corrections above, since work will be sequenced on them. Every claim cites file:line; spot-check the load-bearing ones rather than the prose.

Design only; no production change. Written against main @ 34fb7a3 after bricks 1
(#184) and 2 (#185) landed.

This was accidentally swept into the 0.3.5 release-prep commit by a `git add -A`
and caught in review of #203 — a release commit that claimed "one line" while
carrying 704 unrelated lines. Split out here, which is where it belonged.

The document's three load-bearing findings CORRECT the framing the issue was
opened with, so they are worth stating in the commit as well as the file:

1. **There is no live Clang AST today, anywhere.** `CppParser.parse` wraps the
   whole parse in `memScoped`, so the `ASTUnit` — and `ASTContext`/`Sema` with
   it — is destroyed when the parse returns; only the pure-Kotlin `WrappedTU`
   survives, and `clang_slice.h` binds zero Sema symbols. "A persistent Clang
   instance HAS Sema" is not something persistence unlocks; it is new capability
   work that persistence makes affordable. Probe-gated.
2. **A compiler cannot materialize a binding mid-compilation.** Generated
   bindings enter the build as a source dir plus a cinterop klib, both frozen
   before `compileKotlinNative`; a FIR-synthesized class would have no backing
   native symbol. The compiler can ASK and REQUEST; the unit of materialization
   is the next sync.
3. **Lazy binding cannot be the default.** `broad-forcing-resolve.md:216` already
   rated bounding the closure "disqualifying as the primary fix" because it
   changes bound output, and featuregen depends on INCLUDE_MISSING's
   transitivity.

So #10's two premises become ATTACKABLE, not dissolved — an honest downgrade from
what was claimed when the direction was set.

What the design proposes instead is a canonical binding index emitted by krapper
and consumed by the FIR plugin as a compilation-start snapshot, with persistence,
Sema and lazy binding as separate probe-gated bricks above it. §11 argues bricks
B1-B3 are ~80% of the realizable value with no persistence, no socket and no
Sema — and they fix a live defect: the FIR plugin re-derives krapper's
KotlinWriter name mangling by hand (`CppVectorMapping.kt:248-268`) with nothing
checking the two implementations against each other, so a generator change
silently degrades to a spurious SYNC_REQUIRED on a binding that exists.

§9 lists seven probes, each cheap and each allowed to come back negative and kill
its brick. §12 carries four open questions for the owner.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm
@monkopedia-reviewer

Copy link
Copy Markdown
Collaborator

Review: PR #204 (docs-only, docs/design/live-service.md)

Reviewed: 7fd0dfb

I spot-checked every citation the review brief flagged as load-bearing, plus the FIR extension
table and the compileOnly-only claim. Most check out exactly — but I found real problems in
§7 (#194 interaction), which is a load-bearing sequencing section for brick B5. Recommending
against merge until that's fixed.

Verified TRUE, citations exact

  1. "No live Clang AST today." CppParser.kt:76-83 is exactly the memScoped { ... } block
    wrapping buildASTWithArgs(...).getASTContext().getTranslationUnitDecl(); the KDoc directly
    above it says "the whole ASTUnit is released when the parse returns." clang_slice.h:9-15 is
    exactly the include list (ASTUnit.h, DeclCXX.h, DeclTemplate.h, ASTContext.h, Type.h,
    Tooling.h, Lexer.h) — no Sema.h, and a repo-wide grep of krapper/ for clang::Sema/
    Sema:: outside build artifacts and comments finds nothing bound. Confirmed.
  2. "Compiler can't materialize a binding mid-compilation." KPlusPlusCompilerGradlePlugin.kt:819
    is exactly compilation.defaultSourceSet.kotlin.srcDir(krappedSrc); :803-815 is exactly the
    cinterops.create("kplusplus") { interop.definitionFile.set(krappedDef) } +
    dependsOn("kplusplusSync") wiring. Both fixed before compile. Confirmed.
  3. "Lazy binding cannot be the default." docs/design/broad-forcing-resolve.md:210-224
    verbatim: line 216 says "HIGH / disqualifying as the primary fix", with the exact reasoning
    the PR doc attributes to it (changes what gets bound; featuregen's cross-instantiation story
    depends on INCLUDE_MISSING transitivity). Not stretched — a faithful read. Confirmed.
  4. The present-day defect. CppVectorMapping.kt:248-268 is exactly bindingClassId +
    mangleTemplateArg re-deriving the mangling; :176-182 is exactly the 4-entry
    cppElementType table (Int/Long/Double/Float); :53-107 is exactly the 7 hardcoded
    Containers; :139-159 is exactly the generic @CppTemplate path. Confirmed, and
    CppVectorCallChecker.kt:31-38/:40-43/:44-53 all match verbatim too.
  5. Single-tenant globals. IndexedServiceImpl.kt:106-110 exactly matches
    DropLedger.reset() / GenerationContext.reset(...) / BaseBindProfiler.reset().
    KrapperServiceImpl.kt:45-46 exactly matches the cppParseIncludeDirs/cppModelDumpDir
    assignment. GenerationContext.kt:32-34 verbatim-quotes "the generator runs one request at a
    time under runBlocking." Two nits (see below).
  6. KotlinWriter delete + insertion order. :150-156 is exactly the delete-every-file loop,
    :165-171 exactly the per-class write loop. All five LinkedHashMap/LinkedHashSet line
    citations (:241,:954,:1094,:1213,:1310) check out exactly. Confirmed.

Also confirmed exactly: the FIR extension table in §1.2 against
KPlusPlusComponentRegistrar.kt:21-34 + FirKPlusPlusRegistrar.kt (3 extensions, matches);
KrapperSession.kt:130-131 and :215-226 (the fixed call order); App.kt:348-371 (the whole
runService() function, onClose{exitProcess(0)} at 365-367); the ide-sync-perf.md claim and
its code citation KPlusPlusCompilerGradlePlugin.kt:115-117 (the if (!hasHeadersAtConfig)
block, with an adjacent comment literally saying "declaring no output" for the header-driven
path); wellformedness-binding-gate.md:11-16 and :306-320 (both verbatim).

Two minor citation nits (Parsing.kt / DropLedger.kt)

  • "declared at Parsing.kt:97,103,134" — each is off by one: cppParseIncludeDirs is declared
    at 98, cppModelDumpDir at 104, cppBaseModelTu at 135 (97/103/134 are the
    comment lines immediately above). Doesn't change the claim's truth, just imprecise.
  • DropLedger.kt:67 — line 67 is the unrelated closing ) of DropRecord. The actual "runs one
    request at a time under runBlocking" repeat is at line 78. Off by 11 lines, not adjacent.

Real problem: compiler/gradle/build.gradle.kts:76-84 doesn't support its claim

§1.1: "the tool and the Gradle plugin compile the same source
(compiler/gradle/build.gradle.kts:76-84)."
This is wrong, and not just from drift — I checked
it against the doc's own stated base commit 34fb7a3 too. The actual mechanism is
kotlin.sourceSets.named("main") { kotlin.srcDir("../../krapper/src/commonMain/kotlin"); ... },
which at 34fb7a3 sits at lines 72-75, not 76-84 (76-84 there is java { sourceCompatibility ... } + the start of gradlePlugin { } — unrelated). At current main it's moved again to
144-147. The claim itself is true (verified: both :krapper's and the plugin's commonMain do
compile the same krapper/krapper_model source), just the pointer is wrong.

Real problem: §7 is stale relative to the PR's own branch

§7 frames #194 as an open, unresolved blocker requiring a design decision: "L2 must not
land before #194's fix,"
"#194 offers two remedies... For #186, (2) is the better fit," and
§12 open question 3 asks the owner to weigh in on which remedy to pick.

#194 is closed. It was fixed by shading (option 1: relocate ksrpc/ktor/coroutines into the
plugin jar), merged as 0a5a705 / PR #200, whose description states explicitly: "No reflection
/ service-loader obstacle turned up... so option 2 (isolated-classloader worker) was not
needed."
This isn't just "the world moved since the doc was written" — the PR's own head
(7fd0dfb) is based on a65d066, which is two commits after 0a5a705. The fix was already
on the branch this doc's own header cites ("against main @ 34fb7a3" — but the actual branch
base is three commits later, past both the #194 fix and #192's PLUGIN_VERSION fix). The doc
appears to have been drafted against an earlier point and not revisited after the rebase picked
up #194's resolution.

Consequences that need fixing:

§11 judgement

The top-line recommendation — ship §4.1+§4.3 (B1-B3), ~80% of value, no persistence/socket/Sema
does follow from the verified findings and is unaffected by the §7 staleness above (B1-B3
touches none of L2/persistence/#194). That part of the document is sound on its own.

§9 probes

P1-P7 each read as decidable by a cheap, well-scoped measurement tied to a specific brick
(P1/P2→Sema mechanism & B7/B8, P3→B5, P4→B4/B5 prerequisite, P5→B6, P6→B5 constants, P7→trust).
Nothing mis-specified stood out; not independently re-verified for feasibility since they're
prospective, not code claims.

Verdict

Not approving as-is. The citation-checking exercise mostly vindicates the document — the
three corrections the ruling depends on (Sema, mid-compile materialization, lazy-binding
disqualification) are all faithfully and precisely sourced. But §7's central premise (#194 open,
recommending option 2) is factually inverted relative to the state of this PR's own branch, and
one citation (:76-84) is simply wrong. Both are fixable without touching the doc's actual
conclusions (§11 stands either way), so: please update §7/§8/§10/§12 to reflect #194 CLOSED
(shaded, option 1, 0a5a705/#200) and fix the :76-84 and Parsing.kt/DropLedger.kt line
citations, then this is a straightforward re-review.

Not merged.

Both problems found in review; neither changes the document's conclusions.

**§7 was stale, and wrong in its recommendation.** It was drafted while #194 was
open and framed it as a live blocker needing a choice between two remedies,
recommending option 2 (isolated-classloader Gradle worker). But #194 was CLOSED
before this doc was committed — fixed via option 1 (shade/relocate, PR #200,
`0a5a705`) and shipped in v0.3.5 — and this branch's own base is two commits
after that fix. So the section recommended the option we did not take, against a
blocker that no longer existed.

Rewritten to record the outcome, and the outcome is better for #186 than what
§7 originally proposed: shading REMOVES the version conflict rather than
isolating the plugin from it, so a persistent connection holding that stack open
across builds is no more exposed than a per-task one. The superseded
recommendation is preserved in a collapsed block, because the reasoning that
turned out wrong is worth keeping — it optimised for giving the daemon handle a
stable home and treated the conflict as something to work around.

Propagated: the L2 layer note, B5's and B9's "blocked on #194" cells (B5 now
depends only on B4 + probe P3), risk R5 (narrowed to the real residual — ksrpc on
the *kotlinc* plugin classpath, where shading the Gradle plugin does not help),
probe P7 (mostly done: `samples/multiproject` landed with #200 and was verified
to fail against the real released 0.3.4), and open question §12.3 (struck — no
longer a question).

**One bad citation.** §1.1 cited `compiler/gradle/build.gradle.kts:76-84` for the
"both sides compile the same source" claim; that range does not support it at any
commit checked. The actual mechanism is `:144-147`, which adds
`../../krapper/src/commonMain/kotlin` and `../../krapper_model/src/commonMain/kotlin`
as `srcDir`s of the plugin's `main` source set, with the rationale comment at
`:104-107`.

**Minor line nits**, also from review: `Parsing.kt` 97/103/134 -> 98/104/135, and
`DropLedger.kt` 67 -> 78.

The six load-bearing citations the review prioritised all checked out exactly,
including both halves of the "no live Clang AST" finding (the `memScoped` wrapping
and zero bound Sema symbols) and the faithful reading of
`broad-forcing-resolve.md:210-224`.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NLWsoRAe7f1RdQrYJ2ZXqm

@monkopedia-reviewer monkopedia-reviewer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the delta at 3f95faa.

§7 rewrite: confirmed accurate. Grepped the whole document for 194 (13 hits) plus a
broader sweep for "block/prerequisite/hard dependency/must not land" — every remaining mention
now correctly describes #194 as closed (shaded, 0a5a705/PR #200, shipped v0.3.5), including
the diagram (§3 L2 line), B5's "Blocked on" cell (now B4, P3 with a discharge note, not a
blocker), B9's row (no longer lists #194), R5 (narrowed to the real residual — kotlinc-plugin-
classpath ksrpc, where shading the Gradle plugin doesn't help), P7 (correctly marked "mostly
DONE" — I verified against PR #200's own description that samples/multiproject was in fact
built and confirmed to reproduce the failure against the real released 0.3.4), and §12.3
(struck, not left dangling). No sixth stale site found.

Corrected citations, re-verified against main:

  • compiler/gradle/build.gradle.kts:144-147 — exactly the kotlin.sourceSets.named("main") { kotlin.srcDir(...) } block (the two krapper/krapper_model commonMain srcDirs). Confirmed.
  • :104-107 — exactly the "#185: ONE definition... compiled for BOTH sides" rationale comment.
    Confirmed.
  • Parsing.kt:98,104,135 — exactly the cppParseIncludeDirs / cppModelDumpDir /
    cppBaseModelTu declaration lines (previously off-by-one onto the comment above each).
    Confirmed.
  • DropLedger.kt:78 — exactly the "runs one request at a time under runBlocking" line
    (previously pointed at an unrelated closing brace 11 lines away). Confirmed.

Also verified: v0.3.5 is in fact released (gh release list shows it, 2026-07-29), so the
header's "§7 updated... after #194 was fixed and v0.3.5 was cut" claim is accurate, not aspirational.

The six priority citations from the original review were untouched by this delta and remain
verified from the previous pass. Clean now — approving.

Reviewed: 3f95faa

@monkopedia-reviewer
monkopedia-reviewer merged commit 5cb4584 into main Jul 29, 2026
@monkopedia-reviewer
monkopedia-reviewer deleted the docs/live-service-design branch July 29, 2026 20:39
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.

3 participants