core+lsp: one registration seam; the LSP builtin table is generated (#742) - #818
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #742 by centralizing global environment builtin registration behind a single register_builtins seam (fixing missing gfx builtins in embed+gfx builds), and by generating the LSP builtin hover/completion table from the live registration sources so it can’t drift.
Changes:
- Move gfx (when built) and store (always) registration into
register_builtins, removing per-entry-point hand registration. - Add
tools/gen_lsp_builtin_index.shand wire it intomake lsp/build.sh lspto generatesrc/lsp_builtin_index.h(and ignore it in git). - Update LSP hover logic to treat dotted tokens (
x.name) as member access to avoid builtin-vs-stdlib collisions (e.g.meanvsstats.mean), and add CI coverage for embed+gfx (make embed-smoke-gfx).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/gen_lsp_builtin_index.sh | New generator for LSP builtin docs table (names + signature-comment scrape). |
| src/builtins.c | register_builtins now composes gfx/store, making it the single env composition seam. |
| src/main.c | Removes hand registration of gfx/store; relies on register_builtins composition. |
| src/eigs_embed.c | Embed runtime init now uses only register_builtins (seam consolidation). |
| src/ext_http.c | Worker thread env init now uses only register_builtins (seam consolidation). |
| src/lint.c | Removes explicit store registration; uses the composed seam env consistently. |
| src/eigenscript.h | Adds public prototype for register_gfx_builtins. |
| src/embed_smoke.c | Adds assertions that store (and gfx when built) are present via embed seam. |
| src/eigenlsp.c | Replaces hand builtin table with generated header; updates hover for dotted tokens. |
| Makefile | Adds lsp builtin index generation and a new embed-smoke-gfx target. |
| build.sh | Ensures builtin index header is generated in the lsp build path. |
| .gitignore | Ignores generated src/lsp_builtin_index.h. |
| .github/workflows/ci.yml | Runs make embed-smoke-gfx in CI. |
| CHANGELOG.md | Documents the seam fix and the generated LSP builtin index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # default output: src/lsp_builtin_index.h | ||
| # Exit 0 on success; nonzero (and no output written) on a parse failure. | ||
|
|
||
| set -u |
Comment on lines
+217
to
+220
| $(SRC_DIR)/lsp_builtin_index.h: $(SRC_DIR)/builtins.c $(SRC_DIR)/builtins_host.c \ | ||
| $(SRC_DIR)/hash.c $(SRC_DIR)/ext_store.c $(SRC_DIR)/ext_names.h \ | ||
| tools/gen_lsp_builtin_index.sh | ||
| bash tools/gen_lsp_builtin_index.sh |
…742) gfx and store were registered by hand at each entry point — gfx only in main.c, so a gfx build used through the embedding API had no gfx builtins at all. register_builtins now composes store (always) and gfx (when built); the five hand-call sites are gone; make embed-smoke-gfx pins the previously-broken leg in the extensions CI job. eigenlsp's hand-written builtin_docs (68 of 235 builtins, phantom `exec`) is replaced by a generated lsp_builtin_index.h: names from the registration seams + ext_names.h (set-identical to --api), hover detail from the signature comments. Completing the table surfaced the builtin/stdlib collision the gap hid: dotted tokens now resolve as member accesses, so stats.mean hovers as the stats function, bare mean as the tensor builtin. Closes #742 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
InauguralPhysicist
force-pushed
the
fix/742-one-registration-seam
branch
from
August 2, 2026 22:44
ff8e46e to
f0c8e2c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Makefile:220
- The lsp_builtin_index.h generator scrapes signature comments from additional TUs (builtins_tensor.c, ext_gfx.c, ext_http.c, ext_db.c, ext_net.c, model_train.c), but the Makefile rule only depends on the seam sources + ext_names.h. That means changing a builtin’s signature comment (or moving/adding one) in those files won’t regenerate the header on incremental builds, leaving eigenlsp hover text stale until a clean build.
# Builtin half of the same idea (#742): names from the registration seams +
# ext_names.h, hover detail from the signature comments. Also a build
# artifact, never committed.
$(SRC_DIR)/lsp_builtin_index.h: $(SRC_DIR)/builtins.c $(SRC_DIR)/builtins_host.c \
$(SRC_DIR)/hash.c $(SRC_DIR)/ext_store.c $(SRC_DIR)/ext_names.h \
tools/gen_lsp_builtin_index.sh
bash tools/gen_lsp_builtin_index.sh
tools/gen_lsp_builtin_index.sh:35
- The script comment says it exits nonzero "and no output written" on parse failure, but currently pipeline failures (e.g., missing source files, grep/sed/awk errors) won’t necessarily stop the script before the final
mv, so a partial/empty header could be installed. Enabling-e+pipefailmakes failures loud and prevents writing a broken lsp_builtin_index.h.
set -u
cd "$(dirname "$0")/.."
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.
What does this PR do?
Closes #742 — both halves: env composition goes through one registration seam, and the LSP's builtin table becomes generated, set-identical to the live registry.
Closes #742
Half 1 — the registration seam
register_builtinsnow composes store (always compiled; freestanding gets ext_store's linkable no-op) and gfx (behind#if EIGENSCRIPT_EXT_GFX, matching the http/db/net/model rows) itself. The five hand-call sites —main.c×2,eigs_embed.c,ext_http.c's worker-state path,lint.c×3 (is_builtin_name,eigs_api_dump, E003) — are gone, andregister_gfx_builtinsgets a real declaration ineigenscript.hinstead ofmain.c's private extern.make gfxbuild used through the embedding API had no gfx builtins (gfx_open→undefined variable, reproduced red on pre-fix main with an embed probe against the gfx objects; green after).embed_smoke.cnow asserts both compositions, and a newmake embed-smoke-gfxtarget (extensions CI job) links the same smoke againstbuild/gfx/*.o— headless, registration needs no SDL init.[0, g_builtin_binding_count), soeigs_is_registered_builtinnow correctly claims them (sole consumer isbuild_corpus's identifier filter; previously it mis-classifiedstore_openetc. as user names).Half 2 — the generated LSP builtin table
tools/gen_lsp_builtin_index.sh(same recipe and hygiene asgen_lsp_stdlib_index.sh: LC_ALL=C, zero dependencies, build artifact, never committed,.gitignored) generatessrc/lsp_builtin_index.h:builtins.c,builtins_host.c,hash.c,ext_store.c) + everyEIGS_*_BUILTINSgroup inext_names.h— the LSP describes the language surface, not this binary's flags (the E003 binding-base rule);/* name of ... */signature comment above each C definition; the 124 builtins whose definitions never wrote one fall back to a BUILTINS.md pointer rather than papering the gap over.--api --json's registry; 202 with signature detail — versus the hand table's 68 entries and phantomexec. Wired into the Makefilelsptarget andbuild.sh lsp.meanis both a tensor builtin andstats.mean— the old table only "handled" it by not knowing the builtin existed (tests/test_lsp.py's define-shape-fallback check caught it immediately, 81/82). Hover now treats a dotted token as a member access: builtin/keyword/document-symbol lookups skip it and only the module-qualified stdlib branch may claim it.stats.meanhovers as the stats function; baremeanas the builtin.Testing
detect_leaks=13401/3401, zero sanitizer lines, leak floor 0 held.tools/freestanding_check.shboth stages green, allowlist untouched (store's registration under the profile is the existing no-op).inputexclusion and the repaired dotted-hover check);make lsp,make gfx,make http,make fullall build.make embed-smokeandmake embed-smoke-gfxgreen.undefined variable 'gfx_open'; this branch: OK.probe_gen_741xat the seam puts it in the regenerated table; the generator fails loudly on an empty scrape.Follow-ups / Known Limitations
module.prefix — pre-existing behavior with the old table too; noting rather than expanding scope.Checklist
make testpasses locally — 3397/3397 release, 3401/3401 ASan+UBSan leak-cleandocs/BUILTINS.md— n/a, no new builtins🤖 Generated with Claude Code