From 08245124618ac1c3efddd3d0a3ba5e469f11c5a1 Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 15 Oct 2026 09:07:19 -0400 Subject: [PATCH 1/2] Fix synthetic location hangs, position lookups, and snippet bounds Parsed positions describe where code came from, so the deprecated `location` setters are removed. - `setLocation()` can map into a real `SourceInfo`, and synthetic sources compute lines arithmetically (no growing `lineStarts`), clamping sentinel ranges like `MAX_SAFE_INTEGER` lines instead of hanging - `findChildAtPosition()` only matches nodes from the searched source; source-less and snippet nodes are stepped into but never matched themselves - `setBounds()` prefers a source with a uri over a snippet's, and source maps leave uri-less (snippet) code unmapped - a lone `\r` in a template string is a line start, like everywhere else in the lexer - `BrsFile.sourceInfo` is public so plugins can pass it to `setLocation()` Co-Authored-By: Claude Opus 5.5 --- docs/plugins.md | 8 ++- src/files/BrsFile.ts | 11 ++-- src/lexer/Lexer.spec.ts | 15 +++++ src/lexer/Lexer.ts | 12 ++++ src/lexer/Token.ts | 4 ++ src/lexer/TokenObject.ts | 3 - src/parser/AstNode.spec.ts | 61 +++++++++++++++--- src/parser/AstNode.ts | 12 ++-- src/parser/TranspileState.spec.ts | 25 ++++++++ src/parser/TranspileState.ts | 14 +++- src/util.spec.ts | 42 ++++++++++++ src/util.ts | 103 ++++++++++++++++++++---------- 12 files changed, 246 insertions(+), 64 deletions(-) create mode 100644 src/parser/TranspileState.spec.ts diff --git a/docs/plugins.md b/docs/plugins.md index fd2cf434f..264cd5a3b 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -594,11 +594,13 @@ const location = util.getLocation(funcStmt.tokens.name); // { uri, range } `util.getLocation()` returns `undefined` for synthetic tokens and nodes (ones that were created in code rather than parsed from a file), since they have no position. A synthetic node won't get a source map entry, and diagnostics can't point at it. -To give a synthetic token or node a position: +A position describes where the code came from in the original source, so it's used as-is for source maps and diagnostics. Don't change the position of a parsed token or node. Only give one to a synthetic token or node you're creating: - **Borrow it from a real token or node in the same file** (preferred): `util.setBounds(node, someToken)`. This shares the file's real `source`, so the node combines correctly with the rest of that file's AST. The `create*` functions do the same when given a token or node: `createToken(TokenKind.Identifier, 'name', someToken)`. -- **From a `Location`** (i.e. one from an xml file): `util.setLocation(node, location)`. To pass one to a `create*` function, convert it first: `createToken(TokenKind.Identifier, 'name', util.setLocation({} as Locatable, location))`. +- **From a `Location`** (i.e. one from an xml file): `util.setLocation(node, location, file.sourceInfo)`. If the location is in the file whose `sourceInfo` you pass, the position is mapped into that file, so it combines with the rest of its AST. Otherwise (or without a `sourceInfo`), it's stored against a separate synthetic source for the location's uri. To pass one to a `create*` function, convert it first: `createToken(TokenKind.Identifier, 'name', util.setLocation({} as Locatable, location))`. -`Location` is only used for language server output (diagnostics, hovers, etc.), so use `util.getLocation()` when you need one. Tokens and nodes still have `location` and `range` properties to help existing plugins migrate, but they're deprecated and will be removed in a future release. They build a new `Location` on every read (so don't compare them with `===`), and setting `location` is the same as calling `util.setLocation()`. Tokens created by hand (i.e. `{ kind: ..., text: ... }` or `{ ...token }`) don't have them. +Code parsed from a snippet (i.e. `Parser.parse('...')` without a `srcPath`) has positions, but no uri. Those positions aren't in the file the code is added to, so lookups by position in that file (hover, definition, etc.) skip it, it isn't included in the bounds of nodes that also contain code from a file, and it doesn't get source map entries. + +`Location` is only used for language server output (diagnostics, hovers, etc.), so use `util.getLocation()` when you need one. Tokens and nodes still have read-only `location` and `range` properties to help existing plugins migrate, but they're deprecated and will be removed in a future release. They build a new `Location` on every read (so don't compare them with `===`). Tokens created by hand (i.e. `{ kind: ..., text: ... }` or `{ ...token }`) don't have them. ## Modifying code Sometimes plugins will want to modify code before the project is transpiled. While you can technically edit the AST directly at any point in the file's lifecycle, this is not recommended as those changes will remain changed as long as that file exists in memory and could cause issues with file validation if the plugin is used in a language-server context (i.e. inside vscode). diff --git a/src/files/BrsFile.ts b/src/files/BrsFile.ts index ca688f090..0703a47cd 100644 --- a/src/files/BrsFile.ts +++ b/src/files/BrsFile.ts @@ -11,7 +11,7 @@ import { SymbolTable } from '../SymbolTable'; import { FunctionScope } from '../FunctionScope'; import type { Callable, CallableParam, CommentFlag, BsDiagnostic, FileReference, FileLink, SerializedCodeFile } from '../interfaces'; import type { NamespaceContainer } from '../Scope'; -import type { Token } from '../lexer/Token'; +import type { SourceInfo, Token } from '../lexer/Token'; import { Lexer } from '../lexer/Lexer'; import { TokenKind, AllowedLocalIdentifiers } from '../lexer/TokenKind'; import { Parser, ParseMode } from '../parser/Parser'; @@ -299,12 +299,13 @@ export class BrsFile implements BscFile { } /** - * Info about the source of this file's tokens (shared by every token from the same parse) + * Info about the source of this file's tokens (shared by every token from the same parse). + * Pass it to `util.setLocation()` to map a `Location` in this file into it */ - private get sourceInfo() { - const tokens = this.parser.tokens; + public get sourceInfo(): SourceInfo | undefined { + const tokens = this.parser?.tokens; //every token from the same parse shares the same source, and the last token (`Eof`) always exists - return tokens[tokens.length - 1]?.source; + return tokens?.[tokens.length - 1]?.source; } /** diff --git a/src/lexer/Lexer.spec.ts b/src/lexer/Lexer.spec.ts index 9f2a25a4f..b95273461 100644 --- a/src/lexer/Lexer.spec.ts +++ b/src/lexer/Lexer.spec.ts @@ -463,6 +463,21 @@ describe('lexer', () => { ]); }); + it('treats a lone carriage return as a newline', () => { + let { tokens } = Lexer.scan('`a\rb`'); + expect(tokens.map(t => t.kind)).to.deep.equal([ + TokenKind.BackTick, + TokenKind.TemplateStringQuasi, + TokenKind.EscapedCharCodeLiteral, // carriage return + TokenKind.TemplateStringQuasi, + TokenKind.BackTick, + TokenKind.Eof + ]); + expect((tokens[2] as any).charCode).to.equal(13); + //`b` starts the next line + expect(util.getLocation(tokens[3]).range).to.eql(util.createRange(1, 0, 1, 1)); + }); + it('prevents expressions when escaping the dollar sign', () => { let { tokens } = Lexer.scan('`\\${just text}`'); expect(tokens.map(t => t.kind)).to.deep.equal([ diff --git a/src/lexer/Lexer.ts b/src/lexer/Lexer.ts index 8fd259e1f..0da580cba 100644 --- a/src/lexer/Lexer.ts +++ b/src/lexer/Lexer.ts @@ -541,6 +541,18 @@ export class Lexer { this.sourceInfo?.lineStarts.push(this.current); continue; + //a lone `\r` is a newline too (the same as everywhere else in the lexer, and in LSP) + } else if (this.check('\r')) { + this.templateQuasiString(); + + this.advance(); + let token = this.addToken(TokenKind.EscapedCharCodeLiteral) as Token & { charCode: number }; + token.charCode = 13; + + //track the start of the next line + this.sourceInfo?.lineStarts.push(this.current); + continue; + //escaped chars } else if (this.check('\\')) { this.templateQuasiString(); diff --git a/src/lexer/Token.ts b/src/lexer/Token.ts index 783ee3bec..6962fbd4f 100644 --- a/src/lexer/Token.ts +++ b/src/lexer/Token.ts @@ -56,6 +56,10 @@ export interface SourceInfo { * Where each line starts (the gap before its first character), counted the same way as `Locatable.pos` */ lineStarts: number[]; + /** + * Only set for synthetic sources (see `util.setLocation()`), where every line is this long and `lineStarts` is unused + */ + lineLength?: number; } /** diff --git a/src/lexer/TokenObject.ts b/src/lexer/TokenObject.ts index 4748ecd5f..e66b7a7eb 100644 --- a/src/lexer/TokenObject.ts +++ b/src/lexer/TokenObject.ts @@ -24,9 +24,6 @@ export class TokenObject implements Token { public get location(): Location | undefined { return util.getLocation(this); } - public set location(value: Location | undefined) { - util.setLocation(this, value); - } /** * @deprecated use `util.getLocation(token)?.range`. This is computed on every read diff --git a/src/parser/AstNode.spec.ts b/src/parser/AstNode.spec.ts index 4b4077c1d..3ee923cdc 100644 --- a/src/parser/AstNode.spec.ts +++ b/src/parser/AstNode.spec.ts @@ -13,7 +13,7 @@ import { ParseMode, Parser } from './Parser'; import type { AstNode } from './AstNode'; import { WalkMode } from '../astUtils/visitors'; import { isStatement } from '../astUtils/reflection'; -import { createToken, createVariableExpression } from '../astUtils/creators'; +import { createAssignmentStatement, createBlock, createIfStatement, createToken, createVariableExpression } from '../astUtils/creators'; import { TokenKind } from '../lexer/TokenKind'; type DeepWriteable = { -readonly [P in keyof T]: DeepWriteable }; @@ -259,6 +259,50 @@ describe('AstNode', () => { const node = file.ast.findChildAtPosition(util.createPosition(1, 12)); expect(isVariableExpression(node) && node.tokens.name.text === 'bb').to.be.false; }); + + it('finds real code inside a synthetic node', () => { + const file = program.setFile('source/main.brs', [ + 'sub main()', + ' a = b', + 'end sub' + ].join('\n')); + //wrap `a = b` in a synthetic `if cond then ... end if` + const block = file.ast.findChild(isBlock); + const assignment = block.statements[0]; + block.statements[0] = createIfStatement({ + condition: createVariableExpression('cond'), + thenBranch: createBlock({ statements: [assignment] }) + }); + const node = file.ast.findChildAtPosition(util.createPosition(1, 8)); + expect(isVariableExpression(node) && node.tokens.name.text).to.eql('b'); + }); + + it('does not match a node without a source on its own', () => { + const file = program.setFile('source/main.brs', [ + 'sub main()', + ' a = 1', + 'end sub' + ].join('\n')); + const block = file.ast.findChild(isBlock); + //a synthetic statement before the real one would be visited first + block.statements.unshift(createAssignmentStatement({ name: 'x', value: createVariableExpression('y') })); + const node = file.ast.findChildAtPosition(util.createPosition(1, 8)); + expect(isLiteralExpression(node) && node.tokens.value.text).to.eql('1'); + }); + + it('does not match code parsed from a snippet against the positions of the file', () => { + const file = program.setFile('source/main.brs', [ + 'sub main()', + ' a = 1', + 'end sub' + ].join('\n')); + const block = file.ast.findChild(isBlock); + //the snippet's `zz = 2` is on line 1 too, but of a different source + const snippet = Parser.parse('sub snippet()\n zz = 2\nend sub').ast.findChild(isAssignmentStatement); + block.statements.unshift(snippet); + const node = file.ast.findChildAtPosition(util.createPosition(1, 8)); + expect(isLiteralExpression(node) && node.tokens.value.text).to.eql('1'); + }); }); describe('findChild', () => { @@ -425,18 +469,17 @@ describe('AstNode', () => { expect((createToken(TokenKind.Identifier, 'b') as any).location).to.be.undefined; }); - it('sets the location of a synthetic node and token', () => { + it('does not allow setting the location, since it describes where the code came from', () => { const location = util.createLocation(2, 3, 2, 7, 'file:///a.brs'); const node = createVariableExpression('x'); - (node as any).location = location; - expect(util.getLocation(node)).to.eql(location); + expect(() => { + (node as any).location = location; + }).to.throw(); const token = createToken(TokenKind.Identifier, 'x'); - (token as any).location = location; - expect(util.getLocation(token)).to.eql(location); - - (token as any).location = undefined; - expect(util.getLocation(token)).to.be.undefined; + expect(() => { + (token as any).location = location; + }).to.throw(); }); }); diff --git a/src/parser/AstNode.ts b/src/parser/AstNode.ts index 6e031b781..15117c64a 100644 --- a/src/parser/AstNode.ts +++ b/src/parser/AstNode.ts @@ -38,9 +38,6 @@ export abstract class AstNode implements Locatable { public get location(): Location | undefined { return util.getLocation(this); } - public set location(value: Location | undefined) { - util.setLocation(this, value); - } /** * @deprecated use `util.getLocation(node)?.range`. This is computed on every read @@ -170,10 +167,13 @@ export abstract class AstNode implements Locatable { * FInd the deepest child that includes the given position */ public findChildAtPosition(position: Position, options?: WalkOptions): TNodeType | undefined { + //a position only means something within one source (the one this search starts from), so only nodes from that source can match. + //Nodes without a source (i.e. a synthetic `if` wrapping real code) or from another source (i.e. code parsed from a snippet) never match + //on their own, but the walk still steps into them, so the real code inside them can still be found + const searchSource = this.source; return this.findChild((node) => { - //if the current node includes this position, keep that node (nodes without a source include every position) - if (!node?.source) { - return node.findChildAtPosition(position, options) ?? node; + if (!node?.source || (searchSource && node.source !== searchSource)) { + return; } const offset = util.getOffset(node.source, position); if (offset !== undefined && node.pos <= offset && offset <= node.end) { diff --git a/src/parser/TranspileState.spec.ts b/src/parser/TranspileState.spec.ts new file mode 100644 index 000000000..cfb09fc78 --- /dev/null +++ b/src/parser/TranspileState.spec.ts @@ -0,0 +1,25 @@ +import { expect } from '../chai-config.spec'; +import { Lexer } from '../lexer/Lexer'; +import { standardizePath as s } from '../util'; +import { rootDir } from '../testHelpers.spec'; +import { TranspileState } from './TranspileState'; + +describe('TranspileState', () => { + const srcPath = s`${rootDir}/source/main.brs`; + + it('maps code from a file to its position', () => { + const state = new TranspileState(srcPath, {}); + const token = Lexer.scan('\n print', { srcPath: srcPath }).tokens[1]; + const node = state.tokenToSourceNode(token); + expect([node.line, node.column]).to.eql([2, 4]); + expect(state.sourceNode(token, 'print').line).to.eql(2); + }); + + it('does not map code parsed from a snippet, since its position is not in any file', () => { + const state = new TranspileState(srcPath, {}); + const token = Lexer.scan('\n print').tokens[1]; + expect(token.source).to.exist; + expect(state.tokenToSourceNode(token).line).to.be.null; + expect(state.sourceNode(token, 'print').line).to.be.null; + }); +}); diff --git a/src/parser/TranspileState.ts b/src/parser/TranspileState.ts index 66add87d9..c2e46c84a 100644 --- a/src/parser/TranspileState.ts +++ b/src/parser/TranspileState.ts @@ -91,7 +91,7 @@ export class TranspileState { public sourceNode(locatable: RangeLike, code: string | SourceNode | TranspileResult): SourceNode { //source maps only need the start, so skip building a whole `Location` for locatables const start = (locatable && 'source' in locatable) - ? util.getStartPosition(locatable) + ? this.getLocatableStart(locatable) : util.extractRange(locatable)?.start; return util.sourceNodeFromTranspileResult( //convert 0-based range line to 1-based SourceNode line @@ -124,16 +124,24 @@ export class TranspileState { */ private getTokenStart(token: TranspileToken): Position | undefined { return token?.source - ? util.getStartPosition(token as Locatable) + ? this.getLocatableStart(token as Locatable) : token?.location?.range?.start; } + /** + * Get the start position of a locatable for a source map. Code parsed from a snippet has a position but no uri, so there's no file + * that position belongs to. Mapping it would point at the same lines of whatever file is being transpiled, so leave it unmapped instead + */ + private getLocatableStart(locatable: Locatable): Position | undefined { + return locatable.source?.uri ? util.getStartPosition(locatable) : undefined; + } + /** * Does this token span more than one line? (a trailing newline doesn't count) */ private isMultiLine(token: TranspileToken, start: Position) { if (token.source) { - const nextLineStart = token.source.lineStarts[start.line + 1]; + const nextLineStart = util.getLineStart(token.source, start.line + 1); return nextLineStart !== undefined && nextLineStart <= util.getContentEnd(token as Locatable); } return token.location?.range?.end?.line > start.line; diff --git a/src/util.spec.ts b/src/util.spec.ts index f6b6a7169..948687bf5 100644 --- a/src/util.spec.ts +++ b/src/util.spec.ts @@ -1127,6 +1127,13 @@ describe('util', () => { expect(util.getLocation(result).range).to.eql(util.createRange(0, 2, 0, 4)); }); + it('prefers a source with a uri over one parsed from a snippet, no matter the order', () => { + const real = Lexer.scan('\n\nabcdef', { srcPath: s`${rootDir}/source/main.brs` }).tokens[2]; + const snippet = Lexer.scan('xyz').tokens[0]; + expect(util.getLocation(util.setBounds({} as Locatable, snippet, real))).to.eql(util.getLocation(real)); + expect(util.getLocation(util.setBounds({} as Locatable, real, snippet))).to.eql(util.getLocation(real)); + }); + it('clears the bounds when nothing has a source', () => { const result = util.setBounds({ pos: 1, end: 2, source: testLocatable(0, 0, 0, 0).source }, createToken(TokenKind.Identifier, 'a')); expect(result).to.eql({ pos: undefined, end: undefined, source: undefined }); @@ -1219,6 +1226,41 @@ describe('util', () => { const result = util.setLocation(testLocatable(0, 0, 0, 1), undefined); expect(result).to.eql({ pos: undefined, end: undefined, source: undefined }); }); + + it('maps into a real source when given one, so it combines with that source', () => { + const tokens = Lexer.scan('a = 1\nbb = 2', { srcPath: s`${rootDir}/source/main.brs` }).tokens; + const sourceInfo = tokens[0].source; + const result = util.setLocation({} as Locatable, util.createLocation(1, 0, 1, 2, sourceInfo.uri), sourceInfo); + expect(result.source).to.equal(sourceInfo); + expect(result.pos).to.eql(6); + expect(result.end).to.eql(8); + expect(util.getLocation(util.setBounds({} as Locatable, tokens[0], result)).range).to.eql(util.createRange(0, 0, 1, 2)); + }); + + it('does not map into a real source for a different uri', () => { + const sourceInfo = Lexer.scan('a = 1', { srcPath: s`${rootDir}/source/main.brs` }).tokens[0].source; + const location = util.createLocation(0, 0, 0, 1, 'file:///other.brs'); + const result = util.setLocation({} as Locatable, location, sourceInfo); + expect(result.source).not.to.equal(sourceInfo); + expect(util.getLocation(result)).to.eql(location); + }); + + it('round-trips characters past the old 2^20 line length', () => { + const location = util.createLocation(2, (2 ** 20) + 5, 3, 2 ** 21, 'file:///d.brs'); + expect(util.getLocation(util.setLocation({} as Locatable, location))).to.eql(location); + }); + + it('clamps huge lines and characters instead of hanging or moving to another line', () => { + //bsc builds ranges like these for whole-file diagnostics and comment flags + const wholeFile = util.setLocation({} as Locatable, util.createLocation(0, 0, 0, Number.MAX_VALUE, 'file:///e.brs')); + expect(util.getLocation(wholeFile).range.end.line).to.eql(0); + + const toEnd = util.setLocation({} as Locatable, util.createLocation(1, 0, Number.MAX_SAFE_INTEGER, 0, 'file:///e.brs')); + const range = util.getLocation(toEnd).range; + expect(range.start).to.eql(util.createPosition(1, 0)); + expect(range.end.line).to.be.greaterThan(1_000_000); + expect(Number.isSafeInteger(toEnd.end)).to.be.true; + }); }); describe('range creation', () => { diff --git a/src/util.ts b/src/util.ts index 730b4da72..6bfd02395 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1355,7 +1355,8 @@ export class Util { /** * Set the `pos`, `end`, and `source` of `target` to the bounds of all the locatables. - * Offsets from different sources can't be compared, so only locatables from the same source as the first one are included. + * Offsets from different sources can't be compared, so only locatables from one source are included: the first one that has a uri, + * or the first one if none of them do. That way, code parsed from a snippet (which has no uri) never takes over the bounds of real code from a file. * A trailing newline is not included in the bounds, so the target never ends at the start of the next line */ public setBounds(target: T, ...locatables: Array): T { @@ -1365,7 +1366,7 @@ export class Util { for (const locatable of locatables) { if (!locatable?.source) { continue; - } else if (!source) { + } else if (!source || (!source.uri && locatable.source.uri)) { source = locatable.source; pos = locatable.pos; end = this.getContentEnd(locatable); @@ -1382,15 +1383,17 @@ export class Util { /** * Give a locatable (such as a synthetic token or node created by a plugin) a position from a line/character `Location`. + * This is meant for building new items. Parsed tokens and nodes describe where they came from in the original source, so don't move them. * - * Prefer `util.setBounds(target, someRealToken)` when a token or node from the same file is available, since that shares the - * file's real `source` and so combines correctly with the rest of that file's AST. This is for when only a `Location` is + * Prefer `util.setBounds(target, someRealToken)` when a token or node from the same file is available. This is for when only a `Location` is * available (i.e. one from an xml file or another program). Passing `undefined` clears the position. * - * The position is stored against a synthetic `source` (one per uri) whose lines are spaced far enough apart that the offsets - * encode the line and character exactly, so `util.getLocation()` returns the same `Location` + * If `sourceInfo` is given (and is for the same uri as the location), the position is mapped into that source, so the item combines with + * the rest of that source's AST like any parsed item would. Otherwise the position is stored against a synthetic source (one per uri) where + * every line is `Util.syntheticLineLength` long, so the offsets encode the line and character directly and `util.getLocation()` returns the same `Location`. + * Lines and characters too large for that encoding are clamped */ - public setLocation(target: T, location: Location | undefined): T { + public setLocation(target: T, location: Location | undefined, sourceInfo?: SourceInfo): T { const range = location?.range; if (!range) { target.source = undefined; @@ -1398,37 +1401,62 @@ export class Util { target.end = undefined; return target; } - const source = this.getSyntheticSource(location.uri, Math.max(range.start.line, range.end.line)); - target.source = source; - target.pos = (range.start.line * Util.syntheticLineLength) + range.start.character; - target.end = (range.end.line * Util.syntheticLineLength) + range.end.character; + if (sourceInfo && !sourceInfo.lineLength && (!location.uri || location.uri === sourceInfo.uri)) { + const pos = this.getOffset(sourceInfo, range.start); + const end = this.getOffset(sourceInfo, range.end); + if (pos !== undefined && end !== undefined) { + target.source = sourceInfo; + target.pos = pos; + target.end = end; + return target; + } + } + target.source = this.getSyntheticSource(location.uri); + target.pos = this.getSyntheticOffset(range.start); + target.end = this.getSyntheticOffset(range.end); return target; } /** - * The spacing between the lines of a synthetic source. Characters must be smaller than this to round-trip through `getLocation()` + * The length of every line in a synthetic source. Characters must be smaller than this to round-trip through `getLocation()` + */ + private static readonly syntheticLineLength = 2 ** 26; + + /** + * The largest line a synthetic source can hold, so offsets stay within `Number.MAX_SAFE_INTEGER` */ - private static readonly syntheticLineLength = 2 ** 20; + private static readonly syntheticMaxLine = 2 ** 26; private syntheticSources = new Map(); /** - * Get the synthetic source for the given uri, making sure it has at least enough lines to contain `line` + * Get the synthetic source for the given uri. Its lines are a fixed length, so it doesn't need any `lineStarts` */ - private getSyntheticSource(uri: string, line: number): SourceInfo { + private getSyntheticSource(uri: string): SourceInfo { let source = this.syntheticSources.get(uri); if (!source) { - source = { uri: uri, lineStarts: [0] }; + source = { uri: uri, lineStarts: [], lineLength: Util.syntheticLineLength }; this.syntheticSources.set(uri, source); } - const lineStarts = source.lineStarts; - //include the line after, so the end of a range that runs to the end of its line still resolves to that line - for (let i = lineStarts.length; i <= line + 1; i++) { - lineStarts.push(i * Util.syntheticLineLength); - } return source; } + private getSyntheticOffset(position: Position) { + const line = Math.min(Math.max(position.line, 0), Util.syntheticMaxLine); + const character = Math.min(Math.max(position.character, 0), Util.syntheticLineLength - 1); + return (line * Util.syntheticLineLength) + character; + } + + /** + * Get the offset where the given line starts in the source + */ + public getLineStart(source: SourceInfo, line: number): number | undefined { + if (source.lineLength) { + return line >= 0 && line <= Util.syntheticMaxLine ? line * source.lineLength : undefined; + } + return source.lineStarts[line]; + } + /** * Get the end offset of a locatable, excluding any trailing newline (i.e. a `Newline` token, or an escaped newline in a template string) */ @@ -1455,16 +1483,15 @@ export class Util { if (!source) { return undefined; } - const lineStarts = source.lineStarts; - const startLine = this.getLineIndex(lineStarts, locatable.pos, 0); - let endLine = this.getLineIndex(lineStarts, locatable.end, startLine); + const startLine = this.getLineIndex(source, locatable.pos, 0); + let endLine = this.getLineIndex(source, locatable.end, startLine); //a token that ends with a newline ends on its own line rather than at the start of the next one - if (endLine > startLine && lineStarts[endLine] === locatable.end && this.getContentEnd(locatable) !== locatable.end) { + if (endLine > startLine && this.getLineStart(source, endLine) === locatable.end && this.getContentEnd(locatable) !== locatable.end) { endLine--; } return { uri: source.uri, - range: this.createRange(startLine, locatable.pos - lineStarts[startLine], endLine, locatable.end - lineStarts[endLine]) + range: this.createRange(startLine, locatable.pos - this.getLineStart(source, startLine), endLine, locatable.end - this.getLineStart(source, endLine)) }; } @@ -1473,12 +1500,12 @@ export class Util { * Returns `undefined` for synthetic items (those without a `source`) */ public getStartPosition(locatable: Locatable): Position | undefined { - const lineStarts = locatable?.source?.lineStarts; - if (!lineStarts) { + const source = locatable?.source; + if (!source) { return undefined; } - const line = this.getLineIndex(lineStarts, locatable.pos, 0); - return this.createPosition(line, locatable.pos - lineStarts[line]); + const line = this.getLineIndex(source, locatable.pos, 0); + return this.createPosition(line, locatable.pos - this.getLineStart(source, line)); } /** @@ -1487,20 +1514,26 @@ export class Util { * Returns `undefined` if the source or the position's line is missing */ public getOffset(source: SourceInfo | undefined, position: Position | undefined): number | undefined { - const lineStarts = source?.lineStarts; - const lineStart = lineStarts?.[position?.line]; + if (!source || !position) { + return undefined; + } + const lineStart = this.getLineStart(source, position.line); if (lineStart === undefined) { return undefined; } const offset = lineStart + position.character; - const nextLineStart = lineStarts[position.line + 1]; + const nextLineStart = this.getLineStart(source, position.line + 1); return nextLineStart !== undefined && offset >= nextLineStart ? nextLineStart - 1 : offset; } /** - * Binary search `lineStarts` (beginning at line index `low`) for the index of the line that contains `offset` + * Get the index of the line (at or after line index `low`) that contains `offset`. Binary searches `lineStarts` for real sources */ - private getLineIndex(lineStarts: number[], offset: number, low: number) { + private getLineIndex(source: SourceInfo, offset: number, low: number) { + if (source.lineLength) { + return Math.max(low, Math.floor(offset / source.lineLength)); + } + const lineStarts = source.lineStarts; let high = lineStarts.length - 1; while (low < high) { const mid = Math.floor((low + high + 1) / 2); From d4c1b05d91285ea06737bf9e2ff9bcb709ed12ea Mon Sep 17 00:00:00 2001 From: Bronley Plumb Date: Thu, 15 Oct 2026 10:51:44 -0400 Subject: [PATCH 2/2] Remove synthetic sources; setLocation requires a real SourceInfo Nothing in bsc calls `setLocation()`, and synthetic sources only existed to back the removed `location` setter. `setLocation(target, location, sourceInfo)` now maps into the given source, or leaves the item without a position when the location doesn't fit it. This drops the per-uri synthetic source map and the `lineLength` field. Co-Authored-By: Claude Opus 5.5 --- docs/plugins.md | 2 +- src/astUtils/creators.spec.ts | 5 +- src/files/BrsFile.ts | 2 +- src/lexer/Token.ts | 4 -- src/parser/TranspileState.ts | 2 +- src/util.spec.ts | 67 ++++++++------------- src/util.ts | 109 ++++++++-------------------------- 7 files changed, 57 insertions(+), 134 deletions(-) diff --git a/docs/plugins.md b/docs/plugins.md index 264cd5a3b..02c53d96d 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -596,7 +596,7 @@ const location = util.getLocation(funcStmt.tokens.name); // { uri, range } A position describes where the code came from in the original source, so it's used as-is for source maps and diagnostics. Don't change the position of a parsed token or node. Only give one to a synthetic token or node you're creating: - **Borrow it from a real token or node in the same file** (preferred): `util.setBounds(node, someToken)`. This shares the file's real `source`, so the node combines correctly with the rest of that file's AST. The `create*` functions do the same when given a token or node: `createToken(TokenKind.Identifier, 'name', someToken)`. -- **From a `Location`** (i.e. one from an xml file): `util.setLocation(node, location, file.sourceInfo)`. If the location is in the file whose `sourceInfo` you pass, the position is mapped into that file, so it combines with the rest of its AST. Otherwise (or without a `sourceInfo`), it's stored against a separate synthetic source for the location's uri. To pass one to a `create*` function, convert it first: `createToken(TokenKind.Identifier, 'name', util.setLocation({} as Locatable, location))`. +- **From a `Location` in a file**: `util.setLocation(node, location, file.sourceInfo)`. This maps the location into that file, so the node combines with the rest of its AST. If the location is in a different file (or its lines aren't in the file), the node is left without a position. To pass one to a `create*` function, convert it first: `createToken(TokenKind.Identifier, 'name', util.setLocation({} as Locatable, location, file.sourceInfo))`. Code parsed from a snippet (i.e. `Parser.parse('...')` without a `srcPath`) has positions, but no uri. Those positions aren't in the file the code is added to, so lookups by position in that file (hover, definition, etc.) skip it, it isn't included in the bounds of nodes that also contain code from a file, and it doesn't get source map entries. diff --git a/src/astUtils/creators.spec.ts b/src/astUtils/creators.spec.ts index 79afca23a..a01570996 100644 --- a/src/astUtils/creators.spec.ts +++ b/src/astUtils/creators.spec.ts @@ -32,8 +32,9 @@ describe('creators', () => { }); it('accepts a location from util.setLocation', () => { - const location = util.createLocation(2, 4, 2, 7, 'file:///source/main.brs'); - const locatable = util.setLocation({} as Locatable, location); + const sourceInfo = Lexer.scan('\n\n abc', { srcPath: 'source/main.brs' }).tokens[0].source; + const location = util.createLocation(2, 4, 2, 7, sourceInfo.uri); + const locatable = util.setLocation({} as Locatable, location, sourceInfo); expect(util.getLocation(createToken(TokenKind.Identifier, 'xyz', locatable))).to.eql(location); expect(util.getLocation(createIdentifier('xyz', locatable))).to.eql(location); expect(util.getLocation(createVariableExpression('xyz', locatable))).to.eql(location); diff --git a/src/files/BrsFile.ts b/src/files/BrsFile.ts index 0703a47cd..9af1affb9 100644 --- a/src/files/BrsFile.ts +++ b/src/files/BrsFile.ts @@ -300,7 +300,7 @@ export class BrsFile implements BscFile { /** * Info about the source of this file's tokens (shared by every token from the same parse). - * Pass it to `util.setLocation()` to map a `Location` in this file into it + * Pass it to `util.setLocation()` to give a new token or node a position in this file */ public get sourceInfo(): SourceInfo | undefined { const tokens = this.parser?.tokens; diff --git a/src/lexer/Token.ts b/src/lexer/Token.ts index 6962fbd4f..783ee3bec 100644 --- a/src/lexer/Token.ts +++ b/src/lexer/Token.ts @@ -56,10 +56,6 @@ export interface SourceInfo { * Where each line starts (the gap before its first character), counted the same way as `Locatable.pos` */ lineStarts: number[]; - /** - * Only set for synthetic sources (see `util.setLocation()`), where every line is this long and `lineStarts` is unused - */ - lineLength?: number; } /** diff --git a/src/parser/TranspileState.ts b/src/parser/TranspileState.ts index c2e46c84a..a0f76bc5b 100644 --- a/src/parser/TranspileState.ts +++ b/src/parser/TranspileState.ts @@ -141,7 +141,7 @@ export class TranspileState { */ private isMultiLine(token: TranspileToken, start: Position) { if (token.source) { - const nextLineStart = util.getLineStart(token.source, start.line + 1); + const nextLineStart = token.source.lineStarts[start.line + 1]; return nextLineStart !== undefined && nextLineStart <= util.getContentEnd(token as Locatable); } return token.location?.range?.end?.line > start.line; diff --git a/src/util.spec.ts b/src/util.spec.ts index 948687bf5..04a361b3c 100644 --- a/src/util.spec.ts +++ b/src/util.spec.ts @@ -1205,61 +1205,44 @@ describe('util', () => { }); describe('setLocation', () => { - it('round-trips through getLocation', () => { - const location = util.createLocation(3, 4, 5, 6, 'file:///a.brs'); - expect(util.getLocation(util.setLocation({} as Locatable, location))).to.eql(location); - }); - - it('round-trips a range that ends at the end of the last line it has seen', () => { - const location = util.createLocation(7, 0, 7, 50, 'file:///b.brs'); - expect(util.getLocation(util.setLocation({} as Locatable, location))).to.eql(location); - }); + const srcPath = s`${rootDir}/source/main.brs`; - it('shares one source per uri, so the results can be combined with setBounds', () => { - const first = util.setLocation({} as Locatable, util.createLocation(1, 2, 1, 4, 'file:///c.brs')); - const second = util.setLocation({} as Locatable, util.createLocation(3, 0, 3, 8, 'file:///c.brs')); - expect(first.source).to.equal(second.source); - expect(util.getLocation(util.setBounds({} as Locatable, first, second)).range).to.eql(util.createRange(1, 2, 3, 8)); - }); - - it('clears the position when given undefined', () => { - const result = util.setLocation(testLocatable(0, 0, 0, 1), undefined); - expect(result).to.eql({ pos: undefined, end: undefined, source: undefined }); - }); - - it('maps into a real source when given one, so it combines with that source', () => { - const tokens = Lexer.scan('a = 1\nbb = 2', { srcPath: s`${rootDir}/source/main.brs` }).tokens; + it('maps the location into the source, so it combines with that source', () => { + const tokens = Lexer.scan('a = 1\nbb = 2', { srcPath: srcPath }).tokens; const sourceInfo = tokens[0].source; - const result = util.setLocation({} as Locatable, util.createLocation(1, 0, 1, 2, sourceInfo.uri), sourceInfo); + const location = util.createLocation(1, 0, 1, 2, sourceInfo.uri); + const result = util.setLocation({} as Locatable, location, sourceInfo); expect(result.source).to.equal(sourceInfo); expect(result.pos).to.eql(6); expect(result.end).to.eql(8); + expect(util.getLocation(result)).to.eql(location); expect(util.getLocation(util.setBounds({} as Locatable, tokens[0], result)).range).to.eql(util.createRange(0, 0, 1, 2)); }); - it('does not map into a real source for a different uri', () => { - const sourceInfo = Lexer.scan('a = 1', { srcPath: s`${rootDir}/source/main.brs` }).tokens[0].source; - const location = util.createLocation(0, 0, 0, 1, 'file:///other.brs'); - const result = util.setLocation({} as Locatable, location, sourceInfo); - expect(result.source).not.to.equal(sourceInfo); - expect(util.getLocation(result)).to.eql(location); + it('clears the position when given no location or source', () => { + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + expect(util.setLocation(testLocatable(0, 0, 0, 1), undefined, sourceInfo)).to.eql({ pos: undefined, end: undefined, source: undefined }); + expect(util.setLocation(testLocatable(0, 0, 0, 1), util.createLocation(0, 0, 0, 1), undefined)).to.eql({ pos: undefined, end: undefined, source: undefined }); }); - it('round-trips characters past the old 2^20 line length', () => { - const location = util.createLocation(2, (2 ** 20) + 5, 3, 2 ** 21, 'file:///d.brs'); - expect(util.getLocation(util.setLocation({} as Locatable, location))).to.eql(location); + it('clears the position for a location in a different file', () => { + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + const result = util.setLocation(testLocatable(0, 0, 0, 1), util.createLocation(0, 0, 0, 1, 'file:///other.brs'), sourceInfo); + expect(util.getLocation(result)).to.be.undefined; }); - it('clamps huge lines and characters instead of hanging or moving to another line', () => { - //bsc builds ranges like these for whole-file diagnostics and comment flags - const wholeFile = util.setLocation({} as Locatable, util.createLocation(0, 0, 0, Number.MAX_VALUE, 'file:///e.brs')); - expect(util.getLocation(wholeFile).range.end.line).to.eql(0); + it('clears the position for lines that are not in the source', () => { + const sourceInfo = Lexer.scan('a = 1', { srcPath: srcPath }).tokens[0].source; + //bsc builds ranges like this for comment flags that run to the end of the file + const result = util.setLocation({} as Locatable, util.createLocation(0, 0, Number.MAX_SAFE_INTEGER, 0, sourceInfo.uri), sourceInfo); + expect(util.getLocation(result)).to.be.undefined; + }); - const toEnd = util.setLocation({} as Locatable, util.createLocation(1, 0, Number.MAX_SAFE_INTEGER, 0, 'file:///e.brs')); - const range = util.getLocation(toEnd).range; - expect(range.start).to.eql(util.createPosition(1, 0)); - expect(range.end.line).to.be.greaterThan(1_000_000); - expect(Number.isSafeInteger(toEnd.end)).to.be.true; + it('keeps a character past the end of a line on that line', () => { + const sourceInfo = Lexer.scan('a = 1\nb = 2', { srcPath: srcPath }).tokens[0].source; + //bsc builds ranges like this for whole-file diagnostics + const result = util.setLocation({} as Locatable, util.createLocation(0, 0, 0, Number.MAX_VALUE, sourceInfo.uri), sourceInfo); + expect(util.getLocation(result).range.end.line).to.eql(0); }); }); diff --git a/src/util.ts b/src/util.ts index 6bfd02395..24b8b50e3 100644 --- a/src/util.ts +++ b/src/util.ts @@ -1382,81 +1382,29 @@ export class Util { } /** - * Give a locatable (such as a synthetic token or node created by a plugin) a position from a line/character `Location`. + * Give a locatable (such as a token or node created by a plugin) a position from a line/character `Location` in the given source + * (i.e. `file.sourceInfo`), so it combines with the rest of that source's AST like any parsed item would. * This is meant for building new items. Parsed tokens and nodes describe where they came from in the original source, so don't move them. * - * Prefer `util.setBounds(target, someRealToken)` when a token or node from the same file is available. This is for when only a `Location` is - * available (i.e. one from an xml file or another program). Passing `undefined` clears the position. - * - * If `sourceInfo` is given (and is for the same uri as the location), the position is mapped into that source, so the item combines with - * the rest of that source's AST like any parsed item would. Otherwise the position is stored against a synthetic source (one per uri) where - * every line is `Util.syntheticLineLength` long, so the offsets encode the line and character directly and `util.getLocation()` returns the same `Location`. - * Lines and characters too large for that encoding are clamped + * Prefer `util.setBounds(target, someRealToken)` when a token or node from the same file is available. + * The position is cleared if there's no location or source, if the location is for a different uri, or if its lines aren't in the source */ - public setLocation(target: T, location: Location | undefined, sourceInfo?: SourceInfo): T { + public setLocation(target: T, location: Location | undefined, sourceInfo: SourceInfo | undefined): T { const range = location?.range; - if (!range) { + const pos = this.getOffset(sourceInfo, range?.start); + const end = this.getOffset(sourceInfo, range?.end); + if (pos === undefined || end === undefined || (location.uri && location.uri !== sourceInfo.uri)) { target.source = undefined; target.pos = undefined; target.end = undefined; - return target; - } - if (sourceInfo && !sourceInfo.lineLength && (!location.uri || location.uri === sourceInfo.uri)) { - const pos = this.getOffset(sourceInfo, range.start); - const end = this.getOffset(sourceInfo, range.end); - if (pos !== undefined && end !== undefined) { - target.source = sourceInfo; - target.pos = pos; - target.end = end; - return target; - } + } else { + target.source = sourceInfo; + target.pos = pos; + target.end = end; } - target.source = this.getSyntheticSource(location.uri); - target.pos = this.getSyntheticOffset(range.start); - target.end = this.getSyntheticOffset(range.end); return target; } - /** - * The length of every line in a synthetic source. Characters must be smaller than this to round-trip through `getLocation()` - */ - private static readonly syntheticLineLength = 2 ** 26; - - /** - * The largest line a synthetic source can hold, so offsets stay within `Number.MAX_SAFE_INTEGER` - */ - private static readonly syntheticMaxLine = 2 ** 26; - - private syntheticSources = new Map(); - - /** - * Get the synthetic source for the given uri. Its lines are a fixed length, so it doesn't need any `lineStarts` - */ - private getSyntheticSource(uri: string): SourceInfo { - let source = this.syntheticSources.get(uri); - if (!source) { - source = { uri: uri, lineStarts: [], lineLength: Util.syntheticLineLength }; - this.syntheticSources.set(uri, source); - } - return source; - } - - private getSyntheticOffset(position: Position) { - const line = Math.min(Math.max(position.line, 0), Util.syntheticMaxLine); - const character = Math.min(Math.max(position.character, 0), Util.syntheticLineLength - 1); - return (line * Util.syntheticLineLength) + character; - } - - /** - * Get the offset where the given line starts in the source - */ - public getLineStart(source: SourceInfo, line: number): number | undefined { - if (source.lineLength) { - return line >= 0 && line <= Util.syntheticMaxLine ? line * source.lineLength : undefined; - } - return source.lineStarts[line]; - } - /** * Get the end offset of a locatable, excluding any trailing newline (i.e. a `Newline` token, or an escaped newline in a template string) */ @@ -1483,15 +1431,16 @@ export class Util { if (!source) { return undefined; } - const startLine = this.getLineIndex(source, locatable.pos, 0); - let endLine = this.getLineIndex(source, locatable.end, startLine); + const lineStarts = source.lineStarts; + const startLine = this.getLineIndex(lineStarts, locatable.pos, 0); + let endLine = this.getLineIndex(lineStarts, locatable.end, startLine); //a token that ends with a newline ends on its own line rather than at the start of the next one - if (endLine > startLine && this.getLineStart(source, endLine) === locatable.end && this.getContentEnd(locatable) !== locatable.end) { + if (endLine > startLine && lineStarts[endLine] === locatable.end && this.getContentEnd(locatable) !== locatable.end) { endLine--; } return { uri: source.uri, - range: this.createRange(startLine, locatable.pos - this.getLineStart(source, startLine), endLine, locatable.end - this.getLineStart(source, endLine)) + range: this.createRange(startLine, locatable.pos - lineStarts[startLine], endLine, locatable.end - lineStarts[endLine]) }; } @@ -1500,12 +1449,12 @@ export class Util { * Returns `undefined` for synthetic items (those without a `source`) */ public getStartPosition(locatable: Locatable): Position | undefined { - const source = locatable?.source; - if (!source) { + const lineStarts = locatable?.source?.lineStarts; + if (!lineStarts) { return undefined; } - const line = this.getLineIndex(source, locatable.pos, 0); - return this.createPosition(line, locatable.pos - this.getLineStart(source, line)); + const line = this.getLineIndex(lineStarts, locatable.pos, 0); + return this.createPosition(line, locatable.pos - lineStarts[line]); } /** @@ -1514,26 +1463,20 @@ export class Util { * Returns `undefined` if the source or the position's line is missing */ public getOffset(source: SourceInfo | undefined, position: Position | undefined): number | undefined { - if (!source || !position) { - return undefined; - } - const lineStart = this.getLineStart(source, position.line); + const lineStarts = source?.lineStarts; + const lineStart = lineStarts?.[position?.line]; if (lineStart === undefined) { return undefined; } const offset = lineStart + position.character; - const nextLineStart = this.getLineStart(source, position.line + 1); + const nextLineStart = lineStarts[position.line + 1]; return nextLineStart !== undefined && offset >= nextLineStart ? nextLineStart - 1 : offset; } /** - * Get the index of the line (at or after line index `low`) that contains `offset`. Binary searches `lineStarts` for real sources + * Binary search `lineStarts` (beginning at line index `low`) for the index of the line that contains `offset` */ - private getLineIndex(source: SourceInfo, offset: number, low: number) { - if (source.lineLength) { - return Math.max(low, Math.floor(offset / source.lineLength)); - } - const lineStarts = source.lineStarts; + private getLineIndex(lineStarts: number[], offset: number, low: number) { let high = lineStarts.length - 1; while (low < high) { const mid = Math.floor((low + high + 1) / 2);