Replace nwnsc subprocess with WASM compiler for diagnostics#82
Closed
PhilippeChab wants to merge 8 commits into
Closed
Replace nwnsc subprocess with WASM compiler for diagnostics#82PhilippeChab wants to merge 8 commits into
PhilippeChab wants to merge 8 commits into
Conversation
PR #62's intent was to migrate from spawning the native nwnsc binary to the in-process Beamdog nwn_script_comp; this commit accomplishes that goal but uses our WebAssembly build instead, which: - has multi-error recovery (one compile pass, every diagnostic in the file plus its #include'd helpers) - has no entry-point requirement (helper / include scripts can be edited and validated without a void main) - is ~210KB (vs 4-5MB native binary per platform) - works on every platform that runs Node, no separate Linux/Mac/ Windows binaries to ship DiagnosticsProvider rewritten: - Loads server/wasm/nwscript_compiler.{js,wasm} once on first use. - Per publish, creates a fresh compiler instance, sets collect-all-errors and require-entry-point=false, runs compile, drains the captured-error vector. - Resolver callback consults a per-call cache first then asks the workspace file system for any name the compiler requests, so nwscript.nss and #include'd files are picked up automatically. - Errors that fire inside an include are routed to the include's own URI (when we can resolve it via the workspace) rather than being attached to the file we were asked to check; the user sees the squiggle on the file that actually contains the bug. - Pre-seeds an empty diagnostic list for the target plus all transitive includes, so fixing an error clears the squiggle immediately. Removed: - spawn(child_process) machinery - getExecutablePath / hasSupportedOS - the WASM is platform-neutral - nwnsc command-line flag construction (-y -c -l -r -h -n -i) - stdout/stderr line-by-line parser (replaced by the collected-errors API) The compiler config keeps `enabled` and `verbose`; `os`, `nwnHome`, `nwnInstallation`, `reportWarnings` are now no-ops (kept in Config.ts for backwards-compat with any user settings.json that sets them). server/resources/compiler/{linux,mac,windows}/ binaries are no longer referenced; left in place so this commit can be reverted cleanly if the WASM approach hits a snag.
Drives the WASM compiler the same way DiagnosticsProvider does - through cwrap signatures, the resolver-callback pattern, the collect-all-errors / require-entry-point flags - so any breakage in the bundle, the C exports, or the API contract surfaces here long before it would in a real LSP session. Covers: ABI version, clean compile, single error, multiple errors per file, include-only validation, errors in #include'd files with the [via:] trace, errors from include + main reported together, file-not-found for unresolvable includes, determinism across repeated compiles. Run with: cd server && yarn test
The integration test spawns the bundled server.js as a child process and speaks JSON-RPC over stdin/stdout exactly like VS Code does: sends initialize, replies to workspace/configuration with the default config, opens documents, waits for publishDiagnostics notifications. Catches anything that breaks in real LSP usage that the WASM unit test can't see - server boot, document indexing, didOpen / didChange / didSave handling, the connection.sendDiagnostics path, file-system resolution from a real workspace. Six scenarios covered: - clean file -> empty diagnostics - broken file -> type-error diagnostic with severity=Error - multi-error file -> three diagnostics in one pass - include-only file -> empty diagnostics (no entry-point required) - edit + save -> diagnostics clear - error in #include'd file -> some diagnostic surfaces Bug surfaced and fixed while writing the test: DiagnosticsProvider had `__dirname/../../wasm` for the WASM path. That path was correct for the unbundled ts source layout but wrong for the bundled server.js, which is at server/out/server.js - __dirname/../../wasm goes one level too high and produces "Cannot find module" at runtime. Changed to `__dirname/../wasm` which is correct for the bundled layout (server/out -> server/wasm) that VS Code actually runs. Run with: cd server && yarn test Verbose server logs: LSP_VERBOSE=1 cd server && yarn test
.vscodeignore broad-excludes server/** and re-includes specific paths, but server/wasm wasn't in the re-include list. Result: the packaged extension was missing the WASM compiler entirely - the installed v3.0.1 logs "Cannot find module 'server/wasm/nwscript_ compiler.js'" on every save and produces no diagnostics. Adding server/wasm to the un-ignore list fixes future packages.
Helper / include files now get their semantic errors reported in multi-error mode. Previously the compiler skipped the tree walk entirely when no entry point was found, so a user editing a helper function would see no diagnostics on type errors.
The resolver callback freed the previous delivery buffer before allocating a new one, then pushed each allocation onto an `owned` list. At end of compile, every entry in `owned` was freed - so every buffer except the last got freed twice. The corrupted heap state didn't surface inside the same compile; it manifested as "memory access out of bounds" one or two compiles later, when malloc returned a bad pointer and a subsequent write trampled something it shouldn't. Reported on cmds_player.nss with a chained #include "inc_roles" -> consts_roles -> consts_module: each save of a .nss file in the workspace would resolve a few includes, leak double-frees, and eventually crash. Fix: drop the owned list. Free only the most recent delivery buffer at the end of compile, since each loadCb call already freed the previous one before allocating its own. Added a regression test that compiles a 4-file include chain ten times in a row and asserts no crash + no diagnostics. Without the fix, this surfaces the heap corruption reliably by iter 3-4.
The old nwnsc-based diagnostics handed -h / -n flags to the binary, which then read scripts directly out of nwn_base.bif and friends. The WASM build can't open BIF archives, but Beamdog also ships the same scripts extracted under data/base_scripts/ and ovr/. Those directories are what the existing compiler.nwnInstallation / compiler.nwnHome settings already point at, so the resolver now walks them as a fallback when a script isn't in the workspace. Indexed once on first miss and cached for the life of the LSP process. The user-visible effect: #include "x0_i0_stringlib" and the rest of the stock NWN library now resolve, so files that depend on them (most real NWN modules) actually validate instead of bailing with "FILE NOT FOUND". The override directory under nwnHome is also indexed so per-game local edits show up. Workspace files still take precedence over both - if you've copied a stock script into your workspace and modified it, the workspace copy wins. Reload the LSP window after changing the NWN paths in settings; the index is built once per process.
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
1 participant
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.
Summary
PR #62's intent was to migrate from spawning the native `nwnsc` binary
to the in-process Beamdog `nwn_script_comp`. This PR accomplishes that
goal but uses a WebAssembly build of the compiler instead, removing
the need for any native subprocess.
The WASM build comes from a fork of `neverwinter.nim` extended with
multi-error recovery, no-entry-point validation, and a clean C API
suitable for Emscripten.
Why WASM over the native binary
its `#include`'d helpers in a single compile, instead of stopping at
the first error.
include scripts can be edited and validated as the user types.
runs. No separate Linux/Mac/Windows binaries to ship, no platform
detection, no `shell: true` / `powershell.exe` quirks.
process startup overhead.
Diff
3 files, +254 / -210:
`collect-all-errors` and `require-entry-point=false`, runs
compile, drains the captured-error vector.
workspace file system for any name the compiler requests, so
`nwscript.nss` and `#include`d files are picked up
automatically.
own URI when we can resolve it, so the squiggle lands on the file
that actually contains the bug.
transitive includes, so fixing an error clears the squiggle
immediately.
— added (~210KB).
Removed
Kept (no-op for backwards compat with existing settings.json)
Not touched in this PR
native binaries are no longer referenced but kept on the branch
so this PR can be reverted cleanly. ~13MB of binaries can be
deleted in a follow-up once the WASM approach is verified.
Test plan
lines should show three squiggles in one pass.
produces diagnostics directly, no longer requires the file to be
imported by a main script.
`#include "lib"` from `main.nss` shows on `lib.nss`.