fix(lsp): hover & completion for array-of-tables, primitives, and WASM stability#860
Open
sealad886 wants to merge 24 commits intotamasfe:masterfrom
Open
fix(lsp): hover & completion for array-of-tables, primitives, and WASM stability#860sealad886 wants to merge 24 commits intotamasfe:masterfrom
sealad886 wants to merge 24 commits intotamasfe:masterfrom
Conversation
Agent-Logs-Url: https://github.com/sealad886/taplo/sessions/93577231-4748-4e04-9ee2-e26ab9105453 Co-authored-by: sealad886 <[email protected]>
The transitive dependency on getrandom 0.3 (via ahash) requires the `wasm_js` feature to compile for wasm32-unknown-unknown. Add it as a direct dependency in taplo-wasm with the feature enabled. Note: building the WASM target also requires `.cargo/config.toml` with: ``` [target.wasm32-unknown-unknown] rustflags = ['--cfg', 'getrandom_backend=\"wasm_js\"'] ``` Co-authored-by: Copilot <[email protected]>
When position_info_at() already provides an Index in the DOM key path (as it does for array-of-tables entries like [[items]]), lookup_keys() would unconditionally append another Index, producing paths like: [\"items\", Index(N-1), Index(0), \"name\"] This caused collect_schemas() to descend into schema[\"items\"] twice, failing on the second descent since an object schema has no \"items\". As a result, hover on keys inside array-of-tables showed nothing, while hover on values (which bypasses lookup_keys) worked fine. Guard the Index append by checking whether the next key in the path is already an Index, preserving behavior for inline/top-level arrays where position_info_at does not provide one. Co-authored-by: Copilot <[email protected]>
FLOAT tokens were missing from the is_primitive() match in the hover handler, causing hover to return no content for any TOML float value (e.g. lr = 0.001, weight = 5.0). The position selection logic checks query.before/after for IDENT or is_primitive() tokens — without FLOAT, float values failed the filter entirely and hover returned None. Add FLOAT to the import and to is_primitive(). Add integration tests for the array-of-tables hover code path including Query, lookup_keys, and position_info_at.
When the last key segment is an Index (array element), full_range fell through to the named-key path which skipped the node entirely. Return the joined text ranges of the node directly when the trailing segment is an Index, so hover and go-to-definition resolve correctly for individual array elements (e.g. tuple values like [-20.0, -5.0]). Add query_array_item_indices integration test covering tuple array values resolving to distinct index paths.
The taplo WASM LSP returns RpcMessage payloads containing nested Map instances instead of plain objects. vscode-jsonrpc's writer silently drops Map entries during JSON serialization, causing hover responses and other results to arrive as empty objects. Add normalizeRpcMessage helper that recursively converts Map instances to plain objects. Apply it in both server.ts (Node IPC) and server-worker.ts (browser Web Worker) message handlers. Also: - Fix server-worker.ts to use globalThis instead of self for broader compat - Add missing envVars callback to server-worker.ts environment - Add test:unit script and lspMessage.test.ts covering Map and array normalization
Signed-off-by: sealad886 <[email protected]>
Signed-off-by: sealad886 <[email protected]>
ahash 0.8.12 pulls getrandom 0.3 via runtime-rng feature, but getrandom 0.3 requires explicit wasm_js feature for wasm32 targets (unlike 0.2 which used js). Add the dependency to prevent WASM build failures. Co-authored-by: Copilot <[email protected]>
The initPromise race-condition fix in server.ts accidentally dropped the normalizeRpcMessage import, and lspMessage.ts was missing from this branch. This caused a WASM panic at lsp.rs:68 — the onMessage callback referenced an undefined function, crashing the server in a loop until VS Code gave up after 5 restarts. Changes: - Create lspMessage.ts with recursive Map-to-object normalization - Restore import in server.ts - Update server-worker.ts: add import, use globalThis, add envVars - Add unit tests for Map/array normalization - Add test:unit script to package.json Co-authored-by: Copilot <[email protected]>
…dd docs fallback Agent-Logs-Url: https://github.com/sealad886/taplo/sessions/2880bb7b-c866-4549-adfe-51fc27206f94 Co-authored-by: sealad886 <[email protected]>
Agent-Logs-Url: https://github.com/sealad886/taplo/sessions/33fdfd6c-a8bd-49c0-820c-385c9e36c674 Co-authored-by: sealad886 <[email protected]>
…rray-of-tables Fix hover and completion descriptions for array of tables
…race - hover.rs: log actual error values at error level when schema resolution fails, use debug level when schemas simply do not match - server-worker.ts: apply the same initPromise guard used in server.ts to prevent concurrent TaploLsp.initialize() calls in the browser Co-authored-by: Copilot <[email protected]>
serde_wasm_bindgen serializes serde_json::Value::Null via
serialize_unit(), which produces JS undefined. When LSP handlers
return Ok(None) (e.g., hover with no data), the response Message
has result: Some(Value::Null), which becomes result: undefined in JS.
Node.js IPC uses JSON serialization by default, which drops undefined
properties. The client then receives {"jsonrpc":"2.0","id":X} with
neither result nor error, triggering LSP protocol errors.
Root cause fix: Use serde_wasm_bindgen::Serializer::json_compatible()
which sets serialize_missing_as_null: true (None → null instead of
undefined) and serialize_maps_as_objects: true (plain objects instead
of Maps).
Defense in depth: normalizeRpcMessage now converts any remaining
undefined values to null as a safety net.
Co-authored-by: Copilot <[email protected]>
…ker.ts Agent-Logs-Url: https://github.com/sealad886/taplo/sessions/fe6d168e-4691-4c3d-b83b-4b74c3751a79 Co-authored-by: sealad886 <[email protected]>
fix(lsp): array-of-tables hover/completion, WASM null serialization, browser worker init
The `getrandom_03` alias in `[dependencies]` duplicated the `getrandom_0_3` alias already present under `[target.'cfg(target_arch = "wasm32")'.dependencies]`. Cargo rejects two aliases of the same package version, breaking `cargo check --target wasm32-unknown-unknown` (CI `check_wasm32` job). Remove the unconditional alias; the target-gated entry is sufficient and more correct since the `wasm_js` feature is only meaningful on wasm32 targets. Co-authored-by: Copilot <[email protected]>
Add `src/wasm/` to `js/lsp/.gitignore` so that wasm-pack output generated during local `yarn build` is not tracked. Co-authored-by: Copilot <[email protected]>
Restore Cargo.lock baseline from upstream and re-resolve minimally for our getrandom 0.3 addition. Pin wasm-bindgen to 0.2.100 (MSRV 1.57) and js-sys to 0.3.77 to avoid pulling wasm-bindgen 0.2.118 which requires Rust 1.77. This fixes CI MSRV failures caused by time-core 0.1.8 (edition 2024) and wasm-bindgen 0.2.118 (rust-version 1.77) that were introduced by uncontrolled dependency drift. Co-authored-by: Copilot <[email protected]>
Option::is_none_or was stabilized in Rust 1.82 and is not available on the project's MSRV of 1.74. Use the equivalent map_or(true, ...) which has been stable since Rust 1.0. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple Taplo LSP schema-resolution edge cases (notably array-of-tables and primitive hover) and hardens the WASM/VS Code extension RPC bridge so JSON-RPC responses remain well-formed and stable across Node IPC and browser-worker transports.
Changes:
- Improve LSP hover/completion schema lookups for array-of-tables and primitive literals (including
FLOAT) and fix array index path handling. - Stabilize WASM JSON-RPC message serialization and normalize RPC payloads in the VS Code extension (Map/undefined → plain objects/null), including safer initialization/error handling.
- Add regression tests for query key indexing and RPC normalization; update build/test scripts and ignore generated WASM artifacts.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/taplo-lsp/src/handlers/hover.rs |
Adjust hover schema resolution (indexed vs non-indexed paths) and include FLOAT in primitive handling. |
crates/taplo-lsp/src/handlers/completion.rs |
Prefer items schema docs for array-of-tables completion entries (with fallback). |
crates/taplo-lsp/src/query.rs |
Prevent double-appending array indices in lookup_keys; adjust full_range for index-ending paths. |
crates/taplo-lsp/src/world.rs |
Treat empty config_file.path as unset and fall back to config auto-discovery. |
crates/taplo-lsp/tests/query_array_item_indices.rs |
Regression test ensuring tuple/array values resolve to distinct indices. |
crates/taplo-lsp/tests/hover_array_of_tables.rs |
Adds diagnostic hover/query tracing for array-of-tables scenarios (currently not assertion-based). |
crates/taplo-wasm/src/lsp.rs |
Use serde_wasm_bindgen::Serializer::json_compatible() to emit JSON-compatible messages (null/plain objects). |
crates/taplo-wasm/Cargo.toml |
Add wasm32-only getrandom 0.3 dependency with wasm_js feature to satisfy the final dependency graph. |
Cargo.lock |
Lockfile updates to include getrandom 0.3 and related transitive changes. |
editors/vscode/src/lspMessage.ts |
Introduce normalizeRpcMessage helper to normalize Map/undefined in RPC payloads. |
editors/vscode/src/server.ts |
Normalize outgoing RPC messages and guard concurrent initialization with initPromise + error handling. |
editors/vscode/src/server-worker.ts |
Apply the same RPC normalization and init guarding in the browser worker server. |
editors/vscode/test/lspMessage.test.ts |
Unit tests for RPC normalization behavior (Map nesting + undefined → null). |
editors/vscode/package.json |
Add test:unit script for Node test runner + ts-node. |
js/lsp/.gitignore |
Ignore src/wasm build artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…based tests Agent-Logs-Url: https://github.com/sealad886/taplo/sessions/78c8fb29-8016-4a60-bf83-4ea1a6752741 Co-authored-by: sealad886 <[email protected]>
refactor: apply PR tamasfe#860 review comments - peekable iterator and assertion-based tests
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.
Problem
Several pre-existing issues prevented Taplo from surfacing schema
documentation for common TOML patterns and caused instability in the
WASM-based VS Code extension:
Array-of-tables (
[[section]]) hover/completion showed the arrayschema description instead of the items schema — no property
docs were shown for fields inside array-of-table entries.
Relates to No description for array of tables and its properties #278.
FLOATwas missing fromis_primitive(), so hovering numericfloat literals (e.g.
min_lr_ratio = 0.01) returned nothing.lookup_keysdoubled array indices, producing invalid schemapaths like
["plugins", 0, 0]instead of["plugins", 0].Empty config file path — setting
configFile.pathto an emptystring silently failed instead of falling back to auto-discovery.
WASM null serialization —
serde_wasm_bindgenserialisednullas
undefined, causing JSON-RPC responses to lose required fields(e.g.
result: nullwas dropped).VS Code extension (WASM mode) — RPC
Mappayloads from theWASM LSP were not normalised to plain objects,
initPromiserejections were unhandled, and error logging was missing from
fallback paths.
Root Cause
lookup_keysappended array indices correctly, but the hover/completion handlers did not dereferenceschema["items"]for array schemasSyntaxKind::FLOATwas omitted from theis_primitive()matchlookup_keysunconditionally appended0even when the previous segment was already an array indexserde_wasm_bindgendefault serialization emitsundefinedforNone/null, which is silently dropped by JSON serializationFix
itemsschema for array-of-tables; addFLOATtois_primitive(); handle array-index path segments infull_rangeresolution.itemsschema when completing insidearray-of-table entries.
lookup_keys.configFile.pathas unset, falling back toconfig file auto-discovery.
Serializer::json_compatible()to emitnull(not
undefined) and plain objects (notMap).lspMessage.tswithnormalizeRpcMessagehelper; guard
initPromiserejection; restore error logging.getrandom0.3wasm_jsfeature forwasm32targets (needed becauseahashnow pulls it transitively).src/wasm/build artifacts.Testing
crates/taplo-lsp/tests/hover_array_of_tables.rs(307 lines)covering array-of-tables hover resolution.
crates/taplo-lsp/tests/query_array_item_indices.rs(93 lines)testing
lookup_keysindex handling.editors/vscode/test/lspMessage.test.ts(119 lines) for RPCnormalisation.
cargo test -p taplopasses.cargo check -p taplo-wasm --target wasm32-unknown-unknownpasses.integer literals, and array-of-tables properties.
Related