lsp/dap: one JSON string decoder, and the lexer reads CRLF (#880, #881) - #900
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows/CRLF interoperability across the EigenScript toolchain by (1) centralizing JSON string escape decoding so json_decode, the LSP, and the DAP all interpret JSON-RPC payloads consistently, and (2) teaching the lexer to accept CRLF source files without changing string-literal semantics or byte-offset accounting.
Changes:
- Extracted and reused the runtime JSON string-body decoder (
eigs_json_decode_string_body) forjson_decode,eigenlsp, andeigsdap, including missing\b/\fhandling and existing\uXXXX/surrogate-pair logic. - Updated the lexer to treat the CR in CRLF as ignorable whitespace outside string literals so CRLF files tokenize/parse correctly.
- Declared LSP
positionEncoding: "utf-8"and added targeted regression tests (LSP diagnostic shape equivalence for LF vs CRLF; JSON escape hard cases; suite section [99k]).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_lsp.py | Asserts positionEncoding: utf-8 and pins LF vs CRLF diagnostic equivalence. |
| tests/test_json_hard.eigs | Adds regression coverage for json_decode handling of \b, \f, and \r. |
| tests/run_all_tests.sh | Adds suite section [99k] covering CRLF scripts and CR-in-literal preservation. |
| src/lexer.c | Accepts CRLF sources by skipping the CR of CRLF pairs outside literals. |
| src/eigsdap.c | Replaces local JSON unescape logic with shared runtime decoder. |
| src/eigenscript.h | Exposes eigs_json_decode_string_body for shared use. |
| src/eigenlsp.c | Replaces local JSON unescape logic with shared decoder; advertises positionEncoding: utf-8. |
| src/builtins.c | Extracts the JSON string-body decode routine and adds \b/\f support. |
| CHANGELOG.md | Documents CRLF + LSP/DAP decoder fixes and the positionEncoding change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b8a34cd to
77da8c4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_lsp.py:634
- The new CRLF diagnostic check sends
textDocument/didOpenwithout the requiredinitialize/shutdown/exit handshake (unlike the rest of this file). That makes the test less representative of real clients and could break if the server later starts enforcing protocol ordering (or changes its EOF behavior).
crlf_src = "a is 1\nb is 2\nc is undefined_thing\n"
lf_diags = diagnostics(converse([did_open(crlf_src)]))
crlf_diags = diagnostics(converse([did_open(crlf_src.replace("\n", "\r\n"))]))
Three bugs, stacked. Fixing only the filed one leaves the issue's own
acceptance criterion unmet.
1. The filed defect. src/eigenlsp.c and src/eigsdap.c each hand-rolled
the same five-escape subset and dropped \r, \b, \f and \uXXXX, with a
default arm that re-emitted the backslash verbatim. JSON requires a CR
to be escaped, so a Windows client's document arrived with a literal
backslash-r in its text.
2. The layer underneath. With the CR decoded, it reached the LEXER,
which rejected it — EigenScript could not read a CRLF source file AT
ALL:
$ eigenscript win.eigs
Syntax error line 1: unexpected character ''
Syntax error line 2: unexpected character ''
Not a tooling bug: the language, and a straight blocker on the
Windows Tier-1 roadmap. The lexer now treats the CR of a CRLF pair as
whitespace outside string literals. A CR INSIDE a literal is data and
is preserved (pinned by a test — a normalize-the-whole-buffer fix
would have silently changed such a program, and would also have
shifted the byte offsets the LSP reports). A lone CR is deliberately
not a line break.
3. The one the extraction exposed. Rather than add a sixth copy of an
escape decoder, the runtime's own JSON string decoder — the one
carrying #724's surrogate-pair handling and #776's trailing-backslash
overflow guard — is now eigs_json_decode_string_body, shared by
json_decode, the LSP and the DAP. That made visible that json_decode
ITSELF was missing \b and \f: its default arm dropped the backslash,
so `json_decode of "a\bc"` silently returned "abc". Same subset gap,
in the language's primary parser rather than the tooling.
the correct choice here — LANGUAGE_CONTRACT.md makes the byte model a
language-level promise — so the server was internally consistent; the
defect was not SAYING so. LSP 3.17 reads a server that negotiates
nothing as utf-16, so clients decoded byte offsets as UTF-16 code units
and every range after a non-ASCII character landed in the wrong place,
drifting further along the line with each one.
Verification: a CRLF document now yields diagnostics byte-identical to
the LF document (tests/test_lsp.py compares the full shape, not just a
count — the old failure produced one bogus syntax error and zero real
diagnostics, which a count check could have passed). New suite section
[99k] covers a CRLF script running, a CR inside a string literal
surviving, and LF sources being unaffected. JH98-JH100 pin \b, \f and \r
in json_decode.
Suite 3794/3794 release, 3792/3792 ASan+UBSan with detect_leaks=1, leak
tally 0. LSP suite 85/85.
Closes #880
Closes #881
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
77da8c4 to
f02f2e4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/builtins.c:1156
- In eigs_json_decode_string_body(), the
default:escape arm still accepts unknown JSON escapes (e.g.\q) silently: it drops the backslash and appends the following byte without setting either parse flag. This means strictjson_decodecan succeed on invalid JSON strings, contradicting the strict-decode intent documented in builtin_json_decode and RFC 8259 escape rules.
default: strbuf_append_char(out, s[*pos]); break;
Three bugs, stacked. Fixing only the filed one leaves the issue's own acceptance criterion unmet.
1. The filed defect — the JSON-RPC unescaper
src/eigenlsp.candsrc/eigsdap.ceach hand-rolled the same five-escape subset, dropped\r,\b,\fand\uXXXX, and had adefault:arm that re-emitted the backslash verbatim. JSON requires a CR to be escaped, so a Windows client's document arrived with a literal backslash-r in its text.2. The layer underneath — the language can't read CRLF
With the CR correctly decoded, it reached the lexer, which rejected it:
EigenScript could not read a CRLF source file at all. Not a tooling bug — the language, and a straight blocker on the Windows Tier-1 roadmap. It's also why the LSP was unusable: the unescaper was only the first of two gates.
The lexer now treats the CR of a CRLF pair as whitespace outside string literals. A CR inside a literal is data and is preserved — pinned by a test, because the tempting fix (normalize the whole buffer to LF) would silently change such a program and shift the byte offsets the LSP reports as positions.
A lone CR (classic pre-OS X Mac) is deliberately not a line break.
3. The one the extraction exposed —
json_decodeitselfRather than write a sixth escape decoder, I extracted the runtime's own JSON string decoder — the one already carrying #724's surrogate-pair handling and #776's trailing-backslash overflow guard — as
eigs_json_decode_string_body, shared byjson_decode, the LSP and the DAP.That made a third bug visible:
json_decodewas missing\band\ftoo. Itsdefault:arm dropped the backslash, so:Same subset gap, in the language's primary parser rather than in the tooling.
#881 — say what the encoding is
Positions are byte offsets, which is the correct choice here:
LANGUAGE_CONTRACT.mdmakes the byte model a language-level promise (len of "café"is 5). The server was internally consistent. The defect was not saying so — LSP 3.17 reads a server that negotiates nopositionEncodingasutf-16, so clients decoded byte offsets as UTF-16 code units and every range after a non-ASCII character landed in the wrong place, drifting further along the line with each one. Now"positionEncoding":"utf-8".Verification
A CRLF document now yields diagnostics byte-identical to the LF document:
tests/test_lsp.pycompares the full diagnostic shape, not a count — the old failure produced one bogus syntax error and zero real diagnostics, which a count-only check could have passed.New suite section [99k]: a CRLF script runs, a CR inside a string literal survives as data, and LF sources are unaffected.
JH98–JH100pin\b,\fand\rinjson_decode.detect_leaks=1: 3792/3792, leak tally 0Closes #880
Closes #881
🤖 Generated with Claude Code