Fix synthetic location hangs, position lookups, and snippet bounds

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).

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);

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;
     }
 
     /**

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([

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();

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

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<T> = { -readonly [P in keyof T]: DeepWriteable<T[P]> };
@@ -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<BrsFile>('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<Block>(isBlock);
+            const assignment = block.statements[0];
+            block.statements[0] = createIfStatement({
+                condition: createVariableExpression('cond'),
+                thenBranch: createBlock({ statements: [assignment] })
+            });
+            const node = file.ast.findChildAtPosition<VariableExpression>(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<BrsFile>('source/main.brs', [
+                'sub main()',
+                '    a = 1',
+                'end sub'
+            ].join('\n'));
+            const block = file.ast.findChild<Block>(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<LiteralExpression>(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<BrsFile>('source/main.brs', [
+                'sub main()',
+                '    a = 1',
+                'end sub'
+            ].join('\n'));
+            const block = file.ast.findChild<Block>(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<AssignmentStatement>(isAssignmentStatement);
+            block.statements.unshift(snippet);
+            const node = file.ast.findChildAtPosition<LiteralExpression>(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();
         });
     });
 

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<TNodeType extends AstNode = AstNode>(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<TNodeType>((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) {

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;
+    });
+});

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)
      */