Abstract method definition 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?

export abstract class AbstractSocket {
    abstract createSocket(): Promise<string>;
}

What did you expect to happen?

Correct behavior

What happened?

node_modules\eslint\node_modules\escope\lib\referencer.js:258
            if (node.body.type === _estraverse.Syntax.BlockStatement) {
                         ^

TypeError: Cannot read property 'type' of null
    at Referencer.visitFunction (node_modules\eslint\node_modules\escope\lib\referencer.js:258:26)
    at Referencer.FunctionExpression (node_modules\eslint\node_modules\escope\lib\referencer.js:569:18)
    at Referencer.Visitor.visit (node_modules\eslint\node_modules\escope\node_modules\esrecurse\esrecurse.js:122:34)
    at Referencer.visitProperty (node_modules\eslint\node_modules\escope\lib\referencer.js:297:18)
    at Referencer.MethodDefinition (node_modules\eslint\node_modules\escope\lib\referencer.js:452: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.visitClass (node_modules\eslint\node_modules\escope\lib\referencer.js:281:18)
    at Referencer.ClassDeclaration (node_modules\eslint\node_modules\escope\lib\referencer.js:488: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: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: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:106:26)

Process finished with exit code 1

[JamesHenry]

Thanks @friedbizkvit!

Referenced from the TS spec (because not documented there): microsoft/TypeScript#3578

@nzakas We currently do not have a natural way to record this info and are skipping the modifier node, and there is no precedent in flow or other tools.

TypeScript records this, as with many other things such as the ExportKeyword, in a modifiers array on the ClassDeclaration and MethodDeclaration nodes.

How do you feel about a simple abstract: true | false on those nodes in the final ESTree AST? It feels similar to a static: true | false on a method to me, so I think that would be intuitive

[nzakas]

My only concern is for rules that look for MethodDefinition and expect body to be non-null. I'd almost rather have an AbstractMethodDefinition node instead to avoid that problem. What do you think?

[JamesHenry]

Good thinking! As I say we are on our own in terms of defining this in an ESTree AST so I think AbstractMethodDefinition works great as a starting point

[JamesHenry]

PR coming up with that, and AbstractClassDeclaration of course too. Although I don't think there is a similar concern with the body of an abstract class, it probably makes sense to start with a separate node type and be conservative

[nzakas]

Hmm, I'm not sure agree. Abstract class classes have no tangible difference from regular classes wheras abstract methods do. I think the summation of your point, though, is that it makes sense to represent abstract things in the same way, which makes sense.

So I think the larger question is this: are there core rules that should run on abstract classes/methods or not?

[JamesHenry]

To be honest, I only use classes when I have to (e.g. when using Angular 2) and I was not actually aware of this abstract functionality until this issue 😄. I have not seen it talked about anywhere, and it was not even explicitly documented in the spec.

For those reasons I was leaning towards a new node type for now, and then potentially addressing it in future if people report that there are core rules relating to classes that they would like to run on their abstract classes.

Even though it might lead to future work/issues being reported, AbstractClassDeclaration feels like the safest and easiest option for right now to solve this issue and get us that bit closer to this parser working on everyone's code without any core rule runtime errors

[nzakas]

Fair enough.

[friedbizkvit]

@nzakas 0.3.1 wasn't published on npm. Is it jenkins fall?

[zamb3zi]

I'm still seeing this problem with 0.3.1.

export abstract class Foo {
    abstract bar();
}

Gives:

Cannot read property 'type' of null
TypeError: Cannot read property 'type' of null
    at Referencer.visitFunction (C:\Users\John\AppData\Roaming\npm\node_modules\eslint\node_modules\escope\lib\referencer.js:258:26)

[nzakas]

@friedbizkvit hmm yeah, we did have problems during the release, and I wasn't feeling good so I didn't spend a lot of time trying to figure things out. I just manually pushed now, so hopefully this fix has now been released.