Repository navigation
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/esrap/c/cecb362608d64dfbebe74f675a59587d1bd13150Open in Note This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed. |
🦋 Changeset detectedLatest commit: cecb362 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dummdidumm
left a comment
There was a problem hiding this comment.
Please split this up into separate PRs - the bugfixes sound like independent fixes, separate to the token feature
5533907 to
c516839
Compare
…ryTokens maps The AST records where a computed key's expression is, not where its brackets are. `boundaryTokens` (sveltejs#148) anchored those brackets by assuming they touch the key, which is wrong whenever whitespace, newlines or comments sit between bracket and key. Add a `tokens` option: the parser's token stream (Acorn `onToken`, Babel `tokens`, ESLint-style `ast.tokens`). With it, every delimiter internal to a node — computed-key brackets, the parentheses of calls, functions, signatures and control-flow statements, the braces of import/export lists, enums, static blocks and template interpolations — is anchored to its real source position via a bounded nearest-token search. Without it those delimiters are left unmapped rather than guessed. Node-boundary anchors are unchanged and output stays byte-identical. The token index sorts on demand: acorn-typescript re-emits tokens when it backtracks, so its stream is not monotone and a plain binary search would silently miss brackets. The anchoring lives in `boundary-tokens.js`, memoised per options object and shared with `tsx`; `write_params`, `write_keyword_paren` and `write_parenthesized` replace the per-visitor copies of the same open/close pattern.
c516839 to
2e2b2ac
Compare
|
@dummdidumm done! once we merge this one #183, I'll need to resolve conflicts on #185 or vice versa. |
…token-locations # Conflicts: # src/languages/ts/index.js
|
@dummdidumm just resolved the conflicts here after the other pr's were merged. |
…token-locations # Conflicts: # src/languages/ts/index.js
|
@dummdidumm any chance you could take another look at this please? |
|
@dummdidumm would really appreciate a review for this as it's holding up improving diagnostics mappings for multiple projects: tsrx, ripple and octane. Thank you! |
|
Can you please provide a reproduction illustrating the problem more concretely? When does it actually matter that a character like |
|
@Rich-Harris sure, here's the breakdown of the problem: Who uses these mapsTSRX compiles Volar maps a range back only if its start falls inside a mapped region. If it doesn't, the diagnostic is dropped and the user sees nothing. If the map points at the wrong place, the squiggle is drawn in the wrong place. Why
|
| source | TypeScript reports on | esrap 2.3.7 maps it to | this PR with tokens |
|---|---|---|---|
[ Symbol.iterator ]() {} in a class (TS2416) |
[Symbol.iterator] |
Symbol.iterator — starts on a space, misses both brackets |
[ Symbol.iterator ] |
| a computed key split across lines (TS1170) | [key] |
the wrong line | [ … ] exactly |
[/* id */ key] (TS2464) |
[/* id */ key] |
key] |
[/* id */ key] |
return-type inlay hint after a prettier-style ,\n) { |
position right after ) |
unmapped — hint is dropped | right after ) |
script
// node repro.mjs <path to esrap/src> [tokens=0|1]
import * as acorn from 'acorn';
import { tsPlugin } from '@sveltejs/acorn-typescript';
import { decode } from '@jridgewell/sourcemap-codec';
import ts from 'typescript';
import { pathToFileURL } from 'node:url';
import path from 'node:path';
const root = process.argv[2];
const with_tokens = process.argv[3] === '1';
const { print } = await import(pathToFileURL(path.join(root, 'index.js')).href);
const { default: language } = await import(pathToFileURL(path.join(root, 'languages/ts/index.js')).href);
const parser = acorn.Parser.extend(tsPlugin());
function compile(source) {
const comments = [];
const tokens = [];
const ast = parser.parse(source, {
ecmaVersion: 'latest', sourceType: 'module', locations: true, onToken: tokens,
onComment: (block, value, start, end, startLoc, endLoc) =>
comments.push({ type: block ? 'Block' : 'Line', value, start, end, loc: { start: startLoc, end: endLoc } })
});
const options = { comments, boundaryTokens: true };
if (with_tokens) options.tokens = tokens;
const { code, map } = print(ast, language(options));
const lines = decode(map.mappings);
// originalPositionFor, greatest lower bound — same as @jridgewell/trace-mapping
const original = (line, column) => {
let best = null;
for (const s of lines[line] ?? []) if (s[0] <= column) best = s; else break;
return best && best.length >= 4 ? { line: best[2], column: best[3] } : null;
};
const to_pos = (text, offset) => {
const before = text.slice(0, offset).split('\n');
return { line: before.length - 1, column: before.at(-1).length };
};
const to_offset = (text, p) => text.split('\n').slice(0, p.line).reduce((n, l) => n + l.length + 1, 0) + p.column;
const map_range = (start, end) => {
const s = to_pos(code, start), e = to_pos(code, end - 1);
const os = original(s.line, s.column), oe = original(e.line, e.column);
if (!os || !oe) return '(unmapped)';
const a = to_offset(source, os), b = to_offset(source, oe) + 1;
return a <= b ? JSON.stringify(source.slice(a, b)) : '(end before start)';
};
const map_point = (offset) => {
const p = to_pos(code, offset), o = original(p.line, p.column);
if (!o) return '(unmapped)';
const at = to_offset(source, o);
return `${o.line + 1}:${o.column} ${JSON.stringify(source.slice(at, at + 6))}`;
};
return { code, map_range, map_point };
}
function language_service(code) {
return ts.createLanguageService({
getScriptFileNames: () => ['gen.ts'],
getScriptVersion: () => '1',
getScriptSnapshot: (f) => f === 'gen.ts' ? ts.ScriptSnapshot.fromString(code)
: ts.sys.fileExists(f) ? ts.ScriptSnapshot.fromString(ts.sys.readFile(f)) : undefined,
getCurrentDirectory: () => process.cwd(),
getCompilationSettings: () => ({ strict: true, lib: ['lib.es2022.d.ts'] }),
getDefaultLibFileName: (o) => ts.getDefaultLibFilePath(o),
fileExists: ts.sys.fileExists, readFile: ts.sys.readFile
});
}
const cases = [
['class member with spaces', `class Bag implements Iterable<string> {\n\t[ Symbol.iterator ]() {\n\t\treturn 42;\n\t}\n}`],
['computed key across lines', `declare const key: string;\ntype Handlers = {\n\t[\n\t\tkey\n\t]: string;\n};`],
['computed key with a comment', `declare const key: boolean;\nconst o = { [/* id */ key]: 1 };`]
];
console.log(`\n=== ${with_tokens ? 'PR + tokens' : 'no tokens'} ===`);
for (const [name, source] of cases) {
const { code, map_range } = compile(source);
for (const d of language_service(code).getSemanticDiagnostics('gen.ts')) {
const text = ts.flattenDiagnosticMessageText(d.messageText, '\n').split('\n')[0];
console.log(`${name}: TS${d.code} ${text.slice(0, 60)}…`);
console.log(` generated ${JSON.stringify(code.slice(d.start, d.start + d.length))} -> source ${map_range(d.start, d.start + d.length)}`);
}
}
// Return-type inlay hints are inserted right after the parameter list's `)`.
// Prettier's default `trailingComma: "all"` puts that `)` on its own line.
const source = `export function createStore(\n\tinitialState: Record<string, unknown>,\n\toptions: { persist?: boolean },\n) {\n\treturn { ...initialState, ...options };\n}`;
const { code, map_point } = compile(source);
for (const h of language_service(code).provideInlayHints('gen.ts', { start: 0, length: code.length }, { includeInlayFunctionLikeReturnTypeHints: true })) {
console.log(`return-type hint ${JSON.stringify(h.text)}: generated after ${JSON.stringify(code[h.position - 1])} -> source ${map_point(h.position)}`);
}Output:
=== esrap 2.3.7 ===
class member with spaces: TS2416 Property '[Symbol.iterator]' in type 'Bag' is not assignable…
generated "[Symbol.iterator]" -> source " Symbol.iterator "
computed key across lines: TS1170 A computed property name in a type literal must refer to an …
generated "[key]" -> source "\tkey\n"
computed key with a comment: TS2464 A computed property name must be of type 'string', 'number',…
generated "[/* id */ key]" -> source " key]"
return-type hint ": { persist?: boolean | undefined; }": generated after ")" -> source (unmapped)
=== PR + tokens ===
class member with spaces: TS2416 Property '[Symbol.iterator]' in type 'Bag' is not assignable…
generated "[Symbol.iterator]" -> source "[ Symbol.iterator ]"
computed key across lines: TS1170 A computed property name in a type literal must refer to an …
generated "[key]" -> source "[\n\t\tkey\n\t]"
computed key with a comment: TS2464 A computed property name must be of type 'string', 'number',…
generated "[/* id */ key]" -> source "[/* id */ key]"
return-type hint ": { persist?: boolean | undefined; }": generated after ")" -> source 4:1 " {\n\tre"
What we do about it today
Both projects carry workarounds for exactly this, and they are the reason I'd like esrap to own it:
- Ripple (framework that implements TSRX) overrides esrap's
PropertyandMethodDefinitionvisitors to write[and]itself with a guessedcolumn - 1/column + 1location: client/index.js#L8556-L8565 and #L8739-L8748. The same guess breaks on whitespace, and goes negative when the key starts a line. - tsrx's mapping step makes the same guess when it builds the Volar mapping for a computed key, and since tsrx-org/tsrx@6c34d7d it silently skips the mapping when the guess isn't in esrap's map, so those errors just disappear instead of crashing: segments.js#L462-L492.
- A list of the remaining spots we still patch by hand, with what each one compensates for, is in jsx/helpers.js#L321-L364.
Only the printer knows where it wrote each [, ( or { in the output. With the parser's token stream it also knows where they came from, so it can map them exactly and all of that guessing can be deleted. Without tokens this PR leaves those characters unmapped rather than guessing, which is the honest answer for consumers that need exact positions.
On the complexity: I get the concern, and the diff is bigger than the feature. I wanted to break it down for you so it's easier to follow and get a clearer mental picture of the changes.
The feature itself is one small file
All of the new logic lives in src/languages/ts/boundary-tokens.js. It is 194 lines, about half of them comments; the code is under 100 lines and does three things:
- Pull the seven delimiters we care about (
()[]{}${) out of the parser's token list, keeping only their positions. Sort them if the parser handed them over out of order (acorn-typescript does, because it backtracks). find: given a position and a direction, binary-search for the nearest[(or whatever) on that side, and stop if it would leave the node we're printing. That is the whole lookup.- Two tiny wrappers,
located_tokenandenclosing_token, so a visitor can say "the[just before this key" in one call.
The two helpers that already existed for boundaryTokens, token_at and token_before, moved into the same file unchanged, so that ts and tsx share one copy instead of two.
It costs nothing unless you turn it on
Everything is gated twice: boundaryTokens has to be true and tokens has to be supplied. If either is missing, every lookup returns undefined on the first line, before touching the token list, and the printer writes exactly what it wrote before. The token index is also built lazily, on the first lookup, so a print that never asks for it never pays for it.
The only behaviour change for existing boundaryTokens users is that the computed-key brackets are no longer guessed. If you don't pass tokens, they are left unmapped instead of mapped to a guessed column. Printed output is byte-identical in every mode, and the tests check that for each construct with and without tokens.
Why the printer diff looks big
src/languages/ts/index.js is +229/-183, but most of that is removing copies of the same code, which I needed to do so I could anchor the parentheses in one place instead of nine:
write_paramsreplaces nine copies of "write(, print the parameter list, write)" across function declarations and expressions, methods, object-literal methods, arrows,declare function, method signatures, call and construct signatures, and function types.write_keyword_parenreplaces eight copies ofkeyword (forif,for,for..in/of,while,with,switch,catchandimport().write_parenthesizedreplaces the duplicated parenthesised-expression and parenthesised-type visitors.- Two one-line helpers,
signature_parametersandsignature_return_type, take care of the acorn-typescript / typescript-eslint naming difference in one place. That removes sixteen@ts-expect-errorcomments and adds none.
Everything else in that file is a one-line change of the form:
- context.write('[');
+ context.write('[', enclosing_token(node, node.key, '['));There are 53 of those with the output, plus an optional location. That's the entire per-visitor cost.
|
Closing this in favour of two PRs rebuilt on top of #198 and #199, since this branch predates both and every file it touches now conflicts:
The synthesized-parentheses case that came up alongside this is tracked separately in #211. |
Problem
For a computed key like
{ [\nkey\n]: value }the AST only records the key expression's location; the brackets' positions cannot be derived from node boundaries.boundaryTokens(#148) anchored them by assuming they sit one character before and after the key, which is wrong whenever whitespace, newlines or comments separate bracket and key. That was the one case in the feature that guessed; every other boundary token is derived from a node's own span.Change (
minor)tokensoption onts()/tsx(): the parser's token stream, in any of the common shapes (AcornonTokenwithtype.label, Babeltokens, ESLint-style{ type: 'Punctuator', value }). Only consulted whenboundaryTokensis on.onTokenarray is not monotone and a plain binary search would silently miss brackets.src/languages/ts/boundary-tokens.js, memoised per options object and shared withtsx.write_params,write_keyword_parenandwrite_parenthesizedreplace the per-visitor copies of the same open/close pattern.Tests
test/sourcemap-keywords.test.jschecks, per construct, that every source delimiter maps to its exact position and that output is byte-identical with and without tokens, plus: comments and line breaks inside brackets, the no-guess behaviour without tokens, tokens without locations, empty tokens, ESLint-style tokens, and out-of-order tokens.Related
The bug fixes that were previously bundled here are now separate PRs: #184 (object-literal method type parameters), #185 (comments after a signature's
)or=>), #186 (semicolon after body-less class methods). This PR preserves current behaviour in all three areas; the explicituntilarguments passed towrite_paramsby the three signature visitors exist only to keep the comment bound unchanged and can go once #185 lands.🤖 Generated with Claude Code