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.

[JamesHenry]

@nzakas I can confirm this is still an issue.

It looks like escope is still reaching the value property of the TSAbstractMethodDefinition node, which is a FunctionExpression with body: null.

[AdamWillden]

Is there any way around these errors to get it functional (albeit not supported)? E.g. a development build or something.

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

TypeError: Cannot read property 'type' of null

[soda0289]

If you change that line to
if (node.body && node.body.type === _estraverse.Syntax.BlockStatement) {
It wont throw an error. There is still a bug where it says the Class and function are undefined but you can turn the rule off and use tsc to check for undefined variables.

I will look into writing a pull request this weekend, unless a better solution comes up.

[soda0289]

Looks like this pull request was attempted estools/escope#111 and denied. We might need to change the node type to AbstractFunctionExpression instead of FunctionExpression so it will skip going down into escope.

[soda0289]

I have created some pull requests that will generate a body for abstract methods since the estree sepc always expects a body for function expressions. See pull request #141 and #142

[AdamWillden]

@soda0289 thank you for this.

On the assumption that what you have done is a work around rather than a valid fix (because I know nothing about any of this AST stuff) I think the work around you have proposed should be accepted to get it functional, even if this issue remains open to remove the workaround once escope is 'fixed' officially.

[JamesHenry]

Just FYI, we have begun work on the escope fork. It will take a little while to get everything in order, but you can expect to see a resolution to this issue soon.

[soda0289]

This issue is no longer blocked by escope since ESLint v4. Please see issue #253 for rules that are incompatible.