Conversation
Co-authored-by: Zack Eisbach <zack.eisbach@gmail.com>
…for vscode extension with default configurations.
…to not conflict with the webExtension configuration.
Co-authored-by: Jacob Lefkowitz <jacoblefk@gmail.com> Co-authored-by: ironmoon <me@ironmoon.dev>
Co-authored-by: ironmoon <me@ironmoon.dev>
Co-authored-by: ironmoon <me@ironmoon.dev>
Expand CacheManager with set/get-surface-ast and set/get-named-result. compile-module stores intermediate ASTs via cache-manager during compilation. Rewrite jump-to-def in lsp.arr to read from cache instead of re-running the compiler pipeline. This approach threads cache-manager as a separate parameter — a follow-up will move it onto CompileOptions to avoid breaking external consumers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
File cache delegates to existing standalone functions. In-memory cache uses a single store keyed by URI with partially-filled entries that accumulate surface-ast, named-result, and loadable as compilation progresses. get-builtin-locator still falls through to file-based lookup as a special case (TODO). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The cache-manager is now created once per server process rather than per request, so builtins only need to be compiled on the first request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fill in mem-get-cached stubs: get-dependencies computes from stored surface AST (using get-dependencies for builtins, get-standard-dependencies for user modules to avoid self-loops), get-compiled returns stored loadable, get-native-modules returns empty. - Add on-compile callback in server info handler to store loadables in the cache-manager after compilation. - Use cache-manager.get-loadable instead of standalone CLI.get-loadable in server info handler (standalone does file I/O). - Remove dep-times/get-cached-if-available-known-mtimes remapping step (unnecessary with in-memory cache). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract shared lsp-compile function in server.arr so new LSP features reuse the compilation setup (cache-manager, worklist, starter-modules) - Dispatch LSP queries via info-type field on the info command rather than adding a new command per feature — server.js needs no changes - Add NOTE(lsp) at each extension point: server.arr, server.js, lsp.arr, server-node-tmp.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces initial Language Server Protocol (LSP) scaffolding for Pyret by adding a VS Code node extension entrypoint that starts a language client, plus a TypeScript LSP server that forwards textDocument/definition (jump-to-def) requests to the existing Pyret compiler server, backed by a new in-memory compilation cache.
Changes:
- Add a Node-targeted VS Code extension entry (
src/extension.ts) and webpack build config alongside the existing web extension. - Add a new
lsp/TypeScript server project (including a temporary node server implementation) and wire it into the VS Code extension viavscode-languageclient. - Refactor the Pyret compiler server protocol to support “info” requests and add an in-memory cache manager + LSP-side “jump-to-def” implementation.
Reviewed changes
Copilot reviewed 41 out of 48 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
vscode/webpack.config.js |
Adds a Node-target webpack build for the desktop extension entrypoint. |
vscode/src/webExtension.ts |
Minor formatting change; web extension activation remains. |
vscode/src/extension.ts |
New Node extension entrypoint that starts a language client to the TS LSP server. |
vscode/README.dev.md |
Updates dev instructions for testing web vs local desktop extension. |
vscode/package.json |
Adds main entrypoint + vscode-languageclient dependency and a new trace setting. |
vscode/package-lock.json |
Locks new dependency graph for vscode-languageclient. |
vscode/.vscode/launch.json |
Adds a debug launch configuration for running the extension. |
lsp/tsconfig.json |
TypeScript build config for the LSP server project. |
lsp/src/server-shared.ts |
Shared LSP server setup (initialize, documents, listen). |
lsp/src/server-node.ts |
Node LSP server entrypoint using vscode-languageserver/node. |
lsp/src/server-node-tmp.ts |
Temporary node server implementing jump-to-def via the Pyret compiler server over WS+unix. |
lsp/src/server-browser.ts |
Browser LSP server entrypoint using vscode-languageserver/browser. |
lsp/README.md |
Documents current build/debug hoops for hacking on the LSP. |
lsp/package.json |
Declares LSP server dependencies (vscode-languageserver, ws, etc.). |
lsp/package-lock.json |
Locks LSP server dependency graph. |
lsp/.gitignore |
Ignores node_modules and build output for the lsp/ package. |
lang/tsconfig.json |
Adds a strict editor-only TS config for checking JS via checkJs. |
lang/tests/type-check/main.arr |
Updates AST constructor calls to include locs. |
lang/tests/pyret/tests/test-letrec.arr |
Adds use context empty-context to test file. |
lang/tests/pyret/tests/test-compile-lib.arr |
Updates AST constructor calls to include locs. |
lang/src/types.d.ts |
Adds TS declaration file to improve editor IntelliSense for runtime/ABI. |
lang/src/js/trove/source-map-lib.js |
Wraps module object literal in parentheses; adds module typing hint. |
lang/src/arr/trove/ast.arr |
Changes Name variants (s-global, s-type-global, etc.) to include a Loc; updates visitors. |
lang/src/arr/compiler/type-structs.arr |
Adjusts pattern matches / constructors for updated Name shapes. |
lang/src/arr/compiler/type-defaults.arr |
Updates globals/type globals to pass locs when constructing names. |
lang/src/arr/compiler/type-check.arr |
Updates comparisons/pattern matches for new Name shapes. |
lang/src/arr/compiler/server.js |
Refactors server protocol handler to accept {command, options} and adds a safer run queue. |
lang/src/arr/compiler/server.arr |
Adds in-memory cache manager usage and an info command path for LSP requests. |
lang/src/arr/compiler/resolve-scope.arr |
Threads locs through make-atom/global name creation and pattern matches updated. |
lang/src/arr/compiler/lsp.arr |
New Pyret-side LSP helpers (currently jump-to-def via cached AST/env). |
lang/src/arr/compiler/desugar.arr |
Updates global name construction to include loc. |
lang/src/arr/compiler/desugar-post-tc.arr |
Updates global name construction to include loc. |
lang/src/arr/compiler/compile-structs.arr |
Adds lsp + cache-manager fields and placeholder to-info-json / print-static-info. |
lang/src/arr/compiler/compile-lib.arr |
Stores surface AST + named-result into the cache manager during compilation. |
lang/src/arr/compiler/cli-module-loader.arr |
Introduces cache-manager abstraction, adds in-memory cache, and threads cache-manager through module-finding/caching. |
lang/src/arr/compiler/ast-util.arr |
Updates name constructors and adds helpers for locating a name by srcloc and by cursor position. |
lang/src/arr/compiler/ast-anf.arr |
Updates make-atom calls for new loc parameter. |
lang/src/arr/compiler/anf.arr |
Updates make-atom calls for new loc parameter. |
lang/src/arr/compiler/anf-loop-compiler.arr |
Updates pattern matches and atom creation for new Name shapes. |
lang/package.json |
Adds @types/ws to dev dependencies. |
lang/package-lock.json |
Locks @types/ws addition and related metadata changes. |
docs/src/trove/ast.js.rkt |
Updates AST docs to reflect l member added to name variants. |
code.pyret.org/src/web/js/output-ui.js |
Updates comment about which AST nodes have l fields. |
code.pyret.org/src/web/js/ide.js |
Removes unused React import. |
code.pyret.org/package-lock.json |
Dependency metadata changes (peer flags / bundle reshuffles). |
.gitignore |
Ignores root /.vscode/ directory. |
.biome.json |
Adds Biome formatter config. |
Files not reviewed (4)
- code.pyret.org/package-lock.json: Language not supported
- lang/package-lock.json: Language not supported
- lsp/package-lock.json: Language not supported
- vscode/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
lang/src/arr/compiler/cli-module-loader.arr:512
default-start-context/default-test-contextsetcache-managertoCS.default-compile-options.cache-manager, but that default cache-manager only providesset/get-surface-astandset/get-named-result(nocached-available,get-cached, etc.). Sincemodule-findernow callsctxt.cache-manager.cached-available(...)and friends, these contexts will throw at runtime (e.g. scripts/tests usingCLI.default-start-context). Usemake-file-cache()(or another full CacheManger implementation) for these defaults.
default-start-context = {
cache-manager: CS.default-compile-options.cache-manager,
current-load-path: Filesystem.resolve("./"),
cache-base-dir: Filesystem.resolve("./compiled"),
compiled-read-only-dirs: empty,
url-file-mode: CS.all-remote
}
default-test-context = {
cache-manager: CS.default-compile-options.cache-manager,
current-load-path: Filesystem.resolve("./"),
cache-base-dir: Filesystem.resolve("./tests/compiled"),
compiled-read-only-dirs: empty,
url-file-mode: CS.all-remote
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const child = childProcess.fork( | ||
| compilerPath, | ||
| ["-serve", "--port", portFile], | ||
| { | ||
| stdio: [0, 1, 2, "ipc"], | ||
| execArgv: ["-max-old-space-size=8192"], | ||
| }, |
There was a problem hiding this comment.
child_process.fork execArgv uses Node/V8 flags, which are typically --max-old-space-size=... (double hyphen). Using -max-old-space-size=8192 is not a recognized Node flag and may be ignored, defeating the intended memory limit increase.
| "runtimeExecutable": "${execPath}", | ||
| "args": ["--extensionDevelopmentPath=${workspaceRoot}"], | ||
| "outFiles": ["${workspaceFolder}/out/**/*.js"], | ||
| "preLaunchTask": "npm: compile" | ||
| } |
There was a problem hiding this comment.
The debug configuration’s outFiles points at ${workspaceFolder}/out/**/*.js, but this extension is built via webpack to dist/ (and the new node entrypoint outputs to dist/extension.js). With the current value, breakpoints/source-maps likely won’t bind. Update outFiles to match the emitted dist/**/*.js (or whatever the actual build output is).
| "files": [ | ||
| "dist/web/**/*", | ||
| "media/pyret-logo-128.png", | ||
| "language-configuration.json", | ||
| "syntaxes/pyret.tmLanguage.json" | ||
| ], | ||
| "browser": "./dist/web/extension.js", | ||
| "main": "./dist/extension.js", | ||
| "contributes": { |
There was a problem hiding this comment.
main now points to ./dist/extension.js, but the package’s files allowlist currently only includes dist/web/**/* (not dist/extension.js). This will make the node entrypoint missing from packaged installs. Add the node bundle (and its sourcemaps, if needed) to the files list or adjust the output location.
There was a problem hiding this comment.
TBH I have little idea... might be a Joe question??
There was a problem hiding this comment.
The more important thing is figuring out how to package and distribute for the web...
| ], | ||
| "description": "Controls how `url-file()` imports are resolved" | ||
| } | ||
| }, | ||
| "pyret.trace.server": { | ||
| "type": "string", | ||
| "enum": ["off", "messages", "verbose"], | ||
| "default": "verbose", | ||
| "scope": "resource", | ||
| "description": "Traces logs of the communication between VS Code and the language server." | ||
| } |
There was a problem hiding this comment.
A new setting pyret.trace.server is contributed here, but there’s no code wiring it up (e.g., calling client.setTrace(...) or configuring trace level based on this setting). As-is, changing the setting will have no effect. Either plumb it into LanguageClient initialization or remove it until it’s supported.
There was a problem hiding this comment.
@ironm00n seems legit, maybe check this out?
zackbach
left a comment
There was a problem hiding this comment.
Looking great! Mostly documentation nits, and a few opportunities for cleaner code.
| end | ||
|
|
||
|
|
||
| fun find-name-key-by-srcloc(resolved :: A.Program, srcloc :: Loc) -> Option<String> block: |
There was a problem hiding this comment.
Do we want to move these helpers into lsp.arr? I guess technically they could be used more generally but I kinda have a hard time thinking of a case where that would happen. I don't hold this organization opinion strongly, though.
| # - orignal AST, desugared/resolved AST, env, post-compile-env | ||
| # - imports + provides | ||
|
|
||
| type CacheManager = { |
There was a problem hiding this comment.
The signatures here are not very informative. I'm assuming you can't put the actual types here without creating a circular dependency, but I'd like to see some better comments here.
I also have a hard time actually knowing what each of these functions actually does; maybe the added signatures will make it clearer. But if not, maybe add some short purpose statements here as well? Or if those purpose statements exist elsewhere, a reference here would be helpful!
| end | ||
|
|
||
| fun get-cached(basedir, uri, name, cache-type): | ||
| fun mem-cached-available(store, basedir, uri, name, _) -> Option<Nothing>: |
There was a problem hiding this comment.
Me: can I get Boolean?
Mom: we have Boolean at home
Boolean at home: Option<Nothing>
In all seriousness, this is just a pretty confusing type. Surely cases on this is more annoying than if??
| method get-builtin-locator(self, basedir, read-only-basedirs, modname): | ||
| file-get-builtin-locator(self, basedir, read-only-basedirs, modname) | ||
| end, | ||
| method set-surface-ast(self, _, _): nothing end, |
There was a problem hiding this comment.
Might be worthwhile leaving a comment explaining this, either on the type itself or here (or both) justifying why this is OK / our architectural decisions.
|
|
||
| fun get-cached-if-available(basedir, loc) block: | ||
| get-cached-if-available-known-mtimes(basedir, loc, [SD.string-dict:]) | ||
| fun mem-get-cached(store, _, uri, name, _): |
There was a problem hiding this comment.
Now that this object is used multiple times, it might be worthwhile to document it a little better (as the return type of both mem-get-cached and file-get-cached). At least with a comment, if not with a type (your call for which is more readable).
| end | ||
| end | ||
|
|
||
| # a-blank HAS NO LOC, nor a-checked |
| @@ -1,3 +1,5 @@ | |||
| use context empty-context | |||
| } | ||
|
|
||
| // NOTE(lsp): To add a new LSP feature: | ||
| // 1. Add a send*Request function below (following this pattern) |
There was a problem hiding this comment.
It seems likely that we can factor out some of the boilerplate here. I should be able to do this for hover.
| "files": [ | ||
| "dist/web/**/*", | ||
| "media/pyret-logo-128.png", | ||
| "language-configuration.json", | ||
| "syntaxes/pyret.tmLanguage.json" | ||
| ], | ||
| "browser": "./dist/web/extension.js", | ||
| "main": "./dist/extension.js", | ||
| "contributes": { |
There was a problem hiding this comment.
The more important thing is figuring out how to package and distribute for the web...
| ], | ||
| "description": "Controls how `url-file()` imports are resolved" | ||
| } | ||
| }, | ||
| "pyret.trace.server": { | ||
| "type": "string", | ||
| "enum": ["off", "messages", "verbose"], | ||
| "default": "verbose", | ||
| "scope": "resource", | ||
| "description": "Traces logs of the communication between VS Code and the language server." | ||
| } |
There was a problem hiding this comment.
@ironm00n seems legit, maybe check this out?
fixes: #40
fixes: #17
fixes: #18