getTokenAtPosition throws "Cannot read property 'text' of undefined"

[Quantumplation]

TypeScript Version: master (Commit 10b174a)

Search Terms: getChildren, getTokenAtPosition

Code

const ts = require('typescript'); // Built from latest master, commit listed above
//                                  V--- index 13 points right before the 1 here
const exampleSource = 'let z = { w: 1 };'
const sourceFile = ts.createSourceFile('1.ts', exampleSource, ts.ScriptTarget.ES2016)
const node = ts.getTokenAtPosition(sourceFile, 13);

Expected behavior:
node refers to the NumericLiteral token with text value '1' from position 12 to 14.

Actual behavior:

TypeError: Cannot read property 'text' of undefined
    at createChildren (C:\proj\minimal-repro\typescript\typescript.js:110389:65)
    at NodeObject.getChildren (C:\proj\minimal-repro\typescript\typescript.js:110352:56)
    at getTokenAtPositionWorker (C:\proj\minimal-repro\typescript\typescript.js:88902:43)
    at Object.getTokenAtPosition (C:\proj\minimal-repro\typescript\typescript.js:88894:16)
    at Object.<anonymous> (C:\proj\minimal-repro\main.js:5:17)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)

Related Issues:
#23917
#14808

Comments:
As mentioned in #23917 by @Andy-MS, it seems that it is best practice to pass a sourceFile to getChildren and related methods, for two reasons:

• without it, getChildren will attempt to traverse parent nodes, which may not have been set yet, and can lead to the exception above
• even if the parent nodes are set, it results in wasted CPU time

The method getTokenAtPositionWorker (seen in-part below) has a non-optional sourceFile parameter which it passes to getStart, but does not pass to getChildren.

    function getTokenAtPositionWorker(sourceFile: SourceFile, position: number, allowPositionInLeadingTrivia: boolean, includePrecedingTokenAtEndPosition: ((n: Node) => boolean) | undefined, includeEndPosition: boolean): Node {
        let current: Node = sourceFile;
        outer: while (true) {
            // find the child that contains 'position'
            for (const child of current.getChildren()) {
                const start = allowPositionInLeadingTrivia ? child.getFullStart() : child.getStart(sourceFile, /*includeJsDoc*/ true);

I'm happy to submit a pull request. I've already made and manually tested the changes locally and everything is working fine, including existing tests. However, I could use some guidance on where and how to add a test for this change.

[DanielRosenwasser]

The language service APIs (like getTokenAtPosition) really expect all trees to have parent pointers readily available.

I think we'd take a PR to address your suggestion (since it would be more efficient), but having any expectation that the APIs will work without parent pointers isn't something I'd take a hard dependency on.

[Quantumplation]

@DanielRosenwasser

Fair enough, this is basically my first foray into the typescript code base so it's still unclear to me when parent pointers get set / when they can be expected to be set. By "not something you'd take a hard dependency on", do you mean that I should avoid writing a unit test for this case specifically, so as to not codify this practice too strictly?

As soon as the Milestone is set to Community (as detailed here), I'll submit that PR.

On a related note, I audited some similar function calls, and noticed a lot of the locations below could benefit from a similar treatment. In each instance, a method which accepts an optional sourceFile parameter is being called without a value for that parameters, but there's a sourceFile in scope that would likely work. If someone more familiar with the code base wants to give me a thumbs up, i'll test each change and submit a PR that updates all those that don't break anything.

Methods checked:
getChildCount, getChildAt, getChildren,
getStart, getWidth, getLeadingTriviaWidth,
getFullText, getText, getFirstToken, getLastToken
Usages that likely could be using a sourceFile in scope:

• utilities.ts:733
• smartIndenter.ts:235
• breakpoint.ts:110,122,135,460
• fixExtendsInterfaceBecomesImplements:25,37,49
• fixInvalidImportSyntax:43,72
• completion.ts:115,1210,1638,1919
• documentHighlights.ts:311,314,445
• formatting.ts:700
• goToDefinition.ts:166
• extractSymbol.ts:1405,1644
• services.ts:1667
• signatureHelp.ts:295,336
• fixAddMissingMember.ts:157
• fixStrictClassInitialization.ts:57,75,91
• importFixes.ts:442
• textChanges.ts:405,407

[ghost]

We should always provide sourceFile if possible internally. Note that some services may be dealing with nodes in other source files (e.g., goToDefinition may find a definition in another file) so you have to be careful.

[Quantumplation]

@Andy-MS Sure thing! I'll submit one PR tonight with the change I originally proposed, and one in the next few days where I've made more sweeping changes based on the places I highlighted above. I'll try to carefully review whether the sourceFile in scope is applicable to the node.

Should I write a unit test for that first PR? if so, where do those kinds of unit tests belong? Those in cases/compiler seem to all be sample files to attempt compilation of, rather than tests over the API.

[ghost]

Like @DanielRosenwasser said, all services should be considered to need parent pointers set. So this change would just be an optimization and not need new tests.

[Quantumplation]

@Andy-MS I'd like to submit a bunch of similar changes after identifying other places that could safely benefit from this. Should I submit one PR for each? one large PR that fixes many at once? Should I submit an issue first detailing each of them, before submitting the PR, or just link it to this one again?

Trying to be a good citizen here. :)

(I've found 30+ places that could adopt a similar change. This decision would be way above my pay grade, so to speak, but given how widespread this is, would it be worth considering putting a sourceFile property on Node to allow this short-circuiting-if-available behavior without threading optional parameters all through the API and having to remember to pass them around explicitly?)