Typescript: public methods node.getChildren or node.getChildCount throw exception on certain node types

[cancerberoSgx]

TypeScript Version: 2.9.0-dev.201xxxxx

Tested with typescript@next

Searched for getChildren and getChildCount

Code

See
tsissue3.zip.

(Impossible to do it in a snippet since I need to read a file with ts.createProgram)

The code for compiling a ts file was taken mostly from https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API)

Expected behavior:
executing the following commands should not throw error

npm i
node node_modules/typescript/bin/tsc 
node dist/issue1.js

(same using ts-node)

Actual behavior:
it throw exception when I call node.getChildren() and node is the class declaration :

/home/sg/sources/typescript-parse-test1/tests/tsissue1/node_modules/typescript/lib/typescript.js:104147
        ts.scanner.setText((sourceFile || node.getSourceFile()).text);

Notes:
as you can see in tsissue1.ts visiting children with forEachChild
works fine but using getChildren or using getChildCount and getChildAt throws exceptions.
These three methods are public (in typescript.d.ts) so I expect that this simple example don't fail. Otherwise these shouldn't be public, or it should be documented under which conditions they should be called.

[cancerberoSgx]

I make it work by implementing compilerHost.getSourceFile as shwon in the next snippet.

Perhaps this is not an issue then, but nevertheless, is strange why getChildren needs the compilerHost to implement getSourceFile while forEachChild doesn't need that. Why the later is able to read the source file automatically while the first isn't? In my example, this get getChildren working:

export function parse(sourcefile: string): ts.SourceFile | undefined {
  const tsConfigJson = ts.parseConfigFileTextToJson('./tsconfig.json', readFileSync('./tsconfig.json').toString())
  let {options, errors} = ts.convertCompilerOptionsFromJson(tsConfigJson.config.compilerOptions, '.')
  const compilerHost:ts.CompilerHost = {
    ...ts.createCompilerHost(options), 
    getSourceFile: (fileName, languageVersion)=>ts.createSourceFile(fileName, readFileSync(fileName).toString(), ts.ScriptTarget.Latest, true)
  }
  return ts.createProgram([sourcefile], options, compilerHost).getSourceFile(sourcefile);
}

I'm sorry to bother you if this is not an issue....

[mhegazy]

@andy-ms can you take a look

[andy-ms]

The problem is that ts.createProgram(...).getSourceFile() doesn't bind parents by default. It's been that way since at least ts2.0. When you call getChildCount() without passing in a source file, we need to climb parents to get to it, but parents haven't been set.

The following works:

function visitNotWorking(sourceFile: SourceFile, visitor: (node:ts.Node)=>void) {
  visit(sourceFile);
  function visit(node: ts.Node) {
    visitor(node);
    for (const child of node.getChildren(sourceFile)) {
      visit(child);
    }
  }
}

By the way, getChildCount() and getChildAt() are a very silly API, all they do is getChildren() and read the array for you. Also, getChildren() scans for token nodes, while forEachChild only includes the nodes that have already been parsed, which excludes tokens.

[cancerberoSgx]

Thanks! This was really what I needed to know, if it is for me you can close this issue.

I wonder if can contribute with some jsdocs so this information is available to API consumers ? I noticed that AST jsdoc is kind of poor but there is lots of info that describe these kind of things that IMO could be very useful for consumers to understand this kind . Unfortunately this comments are not jsdoc and are not exposed in d.ts . I'm toking about comments that ni general describe what a method or enum does and not particularly describe any internal detail. Do you think if I contribute a PR in this project with toms jsdocs like these could be accepted ? Or do you have other plans ? Or are these APIs not mature enough to start defining these contracts yet ? Thanks again!

[andy-ms]

A PR would probably be accepted. We might also consider making sourceFile non-optional since it hurts performance to forget it, but that would be a breaking change.

[cancerberoSgx]

Thanks will do for things that IMO are hard to grasp and have no jsdocs. Thanks again!

[cancerberoSgx]

Hello Andy, I'm writing some JsDocs for Node and the question is: do the same apply to other Node methods that accept SourceFile as first parameter ? The question is regarding what you said about performance :

consider making sourceFile non-optional since it hurts performance to forget it,

Other nodes methods that accept a SourceFile as first param are getStart, getEnd, getWidth, getLeadingTriviaWidth, getText, getFullText, getFirstToken, etc ? ) Looking at the impl I guess so but since is reference jsdoc I would like to be sure. )

Also, perhaps more important, do you think this implementation detail should be documented ?

Thanks

[andy-ms]

Yeah, any time something accepts a SourceFile optional parameter, it should probably be provided (unless you would have just written node.getStart(node.getSourceFile()) which is equivalent to node.getStart()). IMO a comment that applies to that many methods should be put at the top rather than on each individual method.

[typescript-bot]

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.