Fix: Space-infix-ops should ignore type annotations in TypeScript

[soda0289]

Typescript allows type annotations in variable declarations. This commit
ensures they are skipped over and not considered when checking Variable
Declaration nodes.

What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (template)
Tell us about your environment

• ESLint Version:
  master
• Node Version:
  node v4
• npm Version:
  npm v4

What parser (default, Babel-ESLint, etc.) are you using?
typescript-eslint-parser 2.0

Please show your full configuration:

    "env": {
        "es6": true,
        "node": true,
        "mocha": true
    },
    "parser": "typescript-eslint-parser",
    "parserOptions": {
        "sourceType": "module",
        "ecmaFeatures": {
            "jsx": true
        }
    },
    "rules": {
        "space-infix-ops": "error"
    }

What did you do? Please include the actual source code causing the issue.

var a: Foo = '';

This code is causes the space-infix-ops rule to fail since it contains a colon(:) inbetween the variable identifier node and the initialization node.
What did you expect to happen?
It should ignore type annotations and only check for a space between the type annotation and the equal(=) sign.
What actually happened? Please include the actual, raw output from ESLint.
Eslint flags the colon(:) as an error and requires a space in-between it and the variable identifier

What changes did you make? (Give an overview)
I changed the way we check variable declaration by checking tokens between the type annotation, if it exists, and the initalizer.

Is there anything you'd like reviewers to focus on?
To make sure I did not miss any cases that would cause false positives. There is also this issue:
#7162 But it has a false positive in JSX, which I cannot reproduce in typescript. I can look into and add a solution to this PR.

[eslintbot]

LGTM

[mention-bot]

@soda0289, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gyandeeps, @michaelficarra and @Kirill89 to be potential reviewers.

[soda0289]

I will add some more tests for invalid cases with type annotations. For example:

var a: Foo='';
function (a: number=0){}

[eslintbot]

LGTM

[soda0289]

I have added the tests. The rule can now identify valid and invalid uses for variable declarations and assignment expressions.

[kaicataldo]

My only concern is that we're starting to add Typescript specific tests into core. Should we also be adding Babel/Flow tests then as well?

[soda0289]

I think these kind of integration tests, between the AST produced by typescript/babel parser and eslint rules, could be moved to the parser themselves. It is important we have tests for all the changes we make to rules, but maybe we could get away with smaller unit tests. Where we only test the node listener functions in the plugin with a mock node and mock rule context. Or create a more generic AST that is aligned with what estree defines.

[kaicataldo]

I really like the idea of creating a way to test these kinds of things in babel-eslint/typescript-eslint-parser 👍

[mattdell]

Does this require more work from @soda0289 or does it just need 1 more approval from @kaicataldo ?

I'm keen to rid my code of // eslint-disable-line space-infix-ops everywhere.

[kaicataldo]

@eslint/eslint-team any other thoughts on this? I'm still of the mind that the unit tests in ESLint should be parser agnostic, but I don't want to block this from landing. Having a way to test with a more generic AST could be nice.

[soda0289]

@kaicataldo I also think we should change this to a more generic AST. I will make this change tonight.

[eslintbot]

LGTM

[soda0289]

@kaicataldo I tried to make the tests more generic by removing the TS prefix on the types and placing the fixtures in a type-annotations folder. The one created from babel flow I didn't change, just moved to the type-annotation folder.

[mattdell]

Anything left for this? Looks all green to me.

[JamesHenry]

@kaicataldo @soda0289 It seems like the concerns on this one were addressed?

[kaicataldo]

One question - otherwise, LGTM!

[kaicataldo]

It seems like this file still has TypeScript-specific properties in the AST, while the others don't. Is this intentional?

[soda0289]

I missed this one. Thanks will fix.

[eslintbot]

LGTM

[soda0289]

@kaicataldo Think it should be more generic now. I could always try to make unit tests but not really sure where to start with those.

[kaicataldo]

We'll most likely be updating more rules like this in the future - might be nice to figure out a way to use the parsers themselves in tests. I think this is a fine for now though 👍

[fruchtose]

Just curious, what's the merge timeline for this PR? It would be nice to fix eslint/typescript-eslint-parser#224 😄