Declared class methods fail rules that depend on function body

[soda0289]

Eslint: 3.15 (with eslint-scope#master) TypeScript: next typescipt-eslint-parser: master

What code were you trying to parse?

declare namespace FF {
    class Foo extends Bar.Baz {
        far(): any;
    }
}

or

declare module "FF" {
    class Foo extends Bar.Baz {
        far(): any;
    }
}

or

declare class Foo extends Bar.Baz {
    far(): any;
}

What did you expect to happen? It should parse

What happened? Exception:

Cannot read property 'type' of null
TypeError: Cannot read property 'type' of null
    at EventEmitter.onCodePathStart (/home/reyad/Workspace/eslint/lib/rules/array-callback-return.js:187:34)
    at emitTwo (events.js:92:20)
    at EventEmitter.emit (events.js:172:7)
    at processCodePathToEnter (/home/reyad/Workspace/eslint/lib/code-path-analysis/code-path-analyzer.js:357:30)
    at CodePathAnalyzer.enterNode (/home/reyad/Workspace/eslint/lib/code-path-analysis/code-path-analyzer.js:604:9)
    at CommentEventGenerator.enterNode (/home/reyad/Workspace/eslint/lib/util/comment-event-generator.js:98:23)
    at Controller.enter (/home/reyad/Workspace/eslint/lib/eslint.js:928:36)
    at Controller.__execute (/home/reyad/Workspace/eslint/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/home/reyad/Workspace/eslint/node_modules/estraverse/estraverse.js:501:28)
    at Controller.Traverser.controller.traverse (/home/reyad/Workspace/eslint/lib/util/traverser.js:36:33)

We should detect if the parent is being declared and not use FunctionExpression to define functions that have no body. Similar to what is done with functions in a namespace.

[corbinu]

@soda0289 So just ran a smoke test with the latest eslint#master which I switched to eslint-scope and the latest version of this repo with the your #166, #165, and #163 PRs added in

I set ALL eslint rules to warn plus all the rules for these plugins:

• eslint-plugin-import
• eslint-plugin-lodash
• eslint-plugin-mysticatea
• eslint-plugin-node
• eslint-plugin-promise
• eslint-plugin-unicorn

Plus obviously eslint-plugin-typescript

The projects I smoke tested:

• angular@2.4.7
• glimmer@0.21.1
• vscode@
• nativescript@2.5.0
• vscode@1.9.1
• nativescript grocery sample
• some random definitions from DefiantlyTyped

Obviously there are quite a few rules showing warnings when they should not. However the super good news is only 3 of the plugin rules fail and no eslint rules now fail!!!!!!

I am super excited about this

Not that I expect them fixed here but just an FYI the failing rules are:

• node/no-unsupported-features // Blows up all
• import/newline-after-import //Blows up nativescript, vscode, vscode type files
• unicorn/custom-error-definition // Blows up nativescript

Just wanted to update you. Amazing work!

[soda0289]

@corbinu I have been using your smoke test and have found several issues. I have also linted the typescript code base without error which is pretty cool. Thanks for taking the time to create it!

I will take a look at those plugins and see if they I can fix them up.

[corbinu]

@soda0289 Oh I didn't know anybody was using it will push the latest version. It has gotten a lot more complex so need to just make it totally automated again.

Thanks I am going to work on if I can replicate each of those projects tslint configs and open issues for bad warnings.

[soda0289]

@JamesHenry Should we update the README and start accepting issues for rules producing incorrect errors/warnings?

[corbinu]

@soda0289 Sorry yes I am happy to hold off. Or maybe we should just open one issue either here or on the typescript plugin to track all known incorrect rules.

[soda0289]

@corbinu I'd like to write a test suite for integration tests between typescript parser and eslint rules, which I think should be part of this parser. We already have an issue https://github.com/eslint/typescript-eslint-parser/issues/77 for tracking some rule failures. Hold off until we hear from @JamesHenry, see if he's ok with opening issues here, adding them to #77, or posting them in the typescript plugin.

[corbinu]

@soda0289 Ok either way I will finish up my update to the smoke tests and start figuring out what rules are broke and how to make. At this point I just want to figure out how much and what work needs to be done to make this a valid replacement to TSLint for those projects

[JamesHenry]

Thanks, guys!

It's my view that we just look to get everything to a place where it is not throwing errors. Some of these known issues have been around for a long time, delayed by the idea of a perfect solution. I think we should do whatever we can now to resolve those, and I will give some proper thought to your PRs @soda0289. Thanks, as always.

I also want to refactor the codebase, and massively improve its documentation.

If we do accept PRs which we deem to be "workaround" in nature, I would like to document why we are doing it within the code.

Naturally, user-facing documentation is also due an update.

Feel free to record any of the issues you mentioned against this repo for now. I'll think about what we want to do in the future.

Cheers!

[JamesHenry]

So sorry for the delay on this guys. I herniated a disc in my lower back and have been unable to work (or even stand/walk) for a lot of the last few weeks. Well on the road to recovery now though and excited to focus on this again

[soda0289]

@JamesHenry glad to hear your doing well!

This issue and the other PR's all try to solve the scenario where we have functions with empty bodies. The fix we have in eslint-scope allows for the rules to run but there are many that expect function bodies. I think we should either fix the rules, which might be difficult and hard for rule authors to understand, or label the different kinds of empty body functions.

I will take a look into some of the eslint plugin errors @corbinu mentioned earlier in this thread as well.

[corbinu]

Glad your on the mend. No worries on the delay :)

[soda0289]

@corbinu

I tested the rules you posted. The eslint-plugin-node rule: node/no-unsupported-features This is a bug in the typescript parser thinking that new xml.XmlParser is a metaproperty when it is just a new expression. Will submit PR.

import/newline-after-import Is failing because it assumes all import statements are inside of a Program node. In typescript you can have import and export statements inside of a module node like so:

module Foo {
    import * as bar from 'bar';
}

I can submit a PR for this.

unicorn/custom-error-definition This is another instance of a rule expecting a body. The rule checks for class declarations and finds the constructor by checking all childs for the constructor flag to be set to true.

const constructor = body.find(x => x.kind === 'constructor');

This can be fixed by the typescript parser if we label all classes as ambient with they are in an ambient module or namespace. Will update the PR I already have

`

[corbinu]

Amazing work thanks!

[soda0289]

Closing in favor of general issue #253