Escope null body failure

[friedbizkvit]

What version of TypeScript are you using?

1.8.10

What version of typescript-eslint-parser are you using?

0.2.0

What code were you trying to parse?

d3.d.ts from
https://raw.githubusercontent.com/DefinitelyTyped/DefinitelyTyped/master/d3/d3.d.ts

What did you expect to happen?

Correct behavior

What happened?

node_modules\eslint\node_modules\escope\lib\scope.js:113
        for (i = 0, iz = body.body.length; i < iz; ++i) {
                             ^

TypeError: Cannot read property 'body' of null
    at isStrictScope (node_modules\eslint\node_modules\escope\lib\scope.js:113:30)
    at FunctionScope.Scope (node_modules\eslint\node_modules\escope\lib\scope.js:249:25)
    at new FunctionScope (node_modules\eslint\node_modules\escope\lib\scope.js:689:92)
    at ScopeManager.__nestFunctionScope (node_modules\eslint\node_modules\escope\lib\scope-manager.js:241:37)
    at Referencer.visitFunction (node_modules\eslint\node_modules\escope\lib\referencer.js:236:31)
    at Referencer.FunctionDeclaration (node_modules\eslint\node_modules\escope\lib\referencer.js:564:18)
    at Referencer.Visitor.visit (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:122:34)
    at Referencer.visitExportDeclaration (node_modules\eslint\node_modules\escope\lib\referencer.js:603:22)
    at Referencer.ExportNamedDeclaration (node_modules\eslint\node_modules\escope\lib\referencer.js:617:18)
    at Referencer.Visitor.visit (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:122:34)
    at Referencer.Visitor.visitChildren (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:101:38)
    at Referencer.Visitor.visit (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:125:14)
    at Referencer.Visitor.visitChildren (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:106:26)
    at Referencer.Visitor.visit (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:125:14)
    at Referencer.Visitor.visitChildren (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:101:38)
    at Referencer.Program (node_modules\eslint\node_modules\escope\lib\referencer.js:419:18)

[JamesHenry]

Thanks, @friedbizkvit. I can confirm that this is a bug (see below for details).

One thing I would note is that it doesn't really make sense to lint 3rd Party definition files that you don't have control over, and you will likely want to exclude them.

The issue

It is possible for functions in TypeScript to have no body, because they are simply being used to declare a type.

E.g.

declare function foo(bar: string): string;

...or directly from the referenced d3 definition file above:

declare namespace d3 {
  //...other stuff

  /**
   * Find the first element that matches the given selector string.
   */
  export function select(selector: string): Selection<any>;

  //...other stuff
}

In these cases TSNode.body is undefined.

@nzakas as a quick hack to confirm and work around the issue, I did the following to try and mirror the auto-conversion convention.

case SyntaxKind.FunctionDeclaration:
  assign(result, {
    type: (node.body) ? "FunctionDeclaration" : "TSFunctionDeclaration",
    // ...etc

Flow seems to handle the two statements above differently to one another, whereas in TypeScript they are both just FunctionDeclarations.

Let me know if you have any thoughts/questions.

[nzakas]

@JamesHenry it looks like Flow uses DeclareFunction in that case, I'd prefer to do the same.

[JamesHenry]

Ok, these definitely have different implementation details, so I am going to submit them as two separate PRs.

First up, the change to DeclareFunction, as it is pretty straightforward. TSModuleBlocks and their exports will require a bit more work.

[JamesHenry]

So finishing this one off...

Let's use this as our example:

declare namespace d3 {
  export function select(selector: string): Selection<any>;
}

This currently causes issue because, as mentioned above, the function does not have a body.

(Note, there is no declare keyword for the select function, so it is not covered by the first PR that went in for this issue)

Basically the namespace (TSNode.kind is ModuleDeclaration, which contains a ModuleBlock) is significant.

Therefore, the way I propose to update the parser is to check on each FunctionDeclaration whether or not its parent is a ModuleBlock, and if it is, rename it to a TSFunctionDeclaration (to match the fact that its hierarchy is auto-converted to TSModuleDeclaration > TSModuleBlock.

For clarity and consistency, I would also add to the fixExports() logic to add the TS for the ExportNamedDeclaration.

In other words, the types marked here in red would be prefixed with TS:

I feel pretty sure that most of the linting for within TypeScript namespaces will need to come from the plugin, we just need to make sure no core rules are adversely affected, and I believe this prefixing would achieve that.

[JamesHenry]

I have gone ahead and submitted a PR with this logic here: #82

@nzakas Feel free to just give feedback directly on there if you would prefer a different approach

[nzakas]

I think that makes sense, as I don't think core rules would have any idea how to deal with this.