diff --git a/docs/plugins.md b/docs/plugins.md index fd2cf434f..02c53d96d 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` 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))`. -`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/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 ca688f090..9af1affb9 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 give a new token or node a position in this file */ - 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/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..a0f76bc5b 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,10 +124,18 @@ 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) */ diff --git a/src/util.spec.ts b/src/util.spec.ts index f6b6a7169..04a361b3c 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 }); @@ -1198,26 +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); + const srcPath = s`${rootDir}/source/main.brs`; + + 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 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('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); + 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('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 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('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('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; + }); + + 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 730b4da72..24b8b50e3 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); @@ -1381,54 +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, 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 - * 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` + * 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): 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; + } else { + target.source = sourceInfo; + target.pos = pos; + target.end = end; } - 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; return target; } - /** - * The spacing between the lines of a synthetic source. Characters must be smaller than this to round-trip through `getLocation()` - */ - private static readonly syntheticLineLength = 2 ** 20; - - private syntheticSources = new Map(); - - /** - * Get the synthetic source for the given uri, making sure it has at least enough lines to contain `line` - */ - private getSyntheticSource(uri: string, line: number): SourceInfo { - let source = this.syntheticSources.get(uri); - if (!source) { - source = { uri: uri, lineStarts: [0] }; - 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; - } - /** * Get the end offset of a locatable, excluding any trailing newline (i.e. a `Newline` token, or an escaped newline in a template string) */