Discussion: Extending core ESLint rules to "just work" with TS-specific nodes

[JamesHenry]

In agreement with @nzakas, I wanted to start a discussion here which summarises the findings so far on which core ESLint rules currently have issues with TypeScript-specific concepts, such as interfaces and decorators.

We do have a few options open to us with regards to making this all work (and the eslint-plugin-typescript plugin will still have its place regardless), but we are in agreement that it would be awesome if we did not have to duplicate any existing rules where all we are trying to do is match the same functionality as on standard JS nodes.

I have been running all of the core rules over a large TypeScript codebase (definitely not an exhaustive use of TypeScript features in there, but it's a great start) and noted the following, some of which is fairly subjective:

---

Rule: camelcase

We have the ability to control whether or not camelcase will apply to object properties, e.g.

"camelcase": ["error", { "properties": "never" }]

Opinion: We should be able to do this for TypeScript interfaces properties too

---

Rule: keyword-spacing

We can enforce spaces before and after keywords like so:

"keyword-spacing": ["error", { "before": true, "after": true }]

Opinion: We should be able to make type "casting" exempt from this in some way

E.g. I had this example:

<models.ICreativeTemplate>this.currentCreative.template

...where I would not want to put a space between the > and this keyword, but the rule naturally wants me to.

---

Rule: no-undef

With TypeScript, we are supporting class properties and interfaces. These are both causing false negatives with no-undef:

class A {
  foo = true
}

/* ESLint:
error, 'foo' is not defined

interface MyInterface {
  bar: string,
  baz: number
}

/* ESLint:
error, 'MyInterface' is not defined
error, 'bar' is not defined
error, 'baz' is not defined

It is very likely there are more TypeScript features that will cause the same, but currently those two are so noisy in my codebase that it is not worth investigating further.

This was also reported here: #75

---

Rule: no-used-vars

There are a couple of things causing false negatives with no-unused-vars:

(1) Class properties created via the constructor, and then later used in a method

class SomeClass {
    constructor(
        private foo: string
    ) {}
    someMethod() {
        return this.foo
    }
}

/* ESLint:
error, 'foo' is defined but never used

(2) Decorators

import { Injectable } from 'foo-framework'

@Injectable()
class Service {}

/* ESLint:
error, 'Injectable' is defined but never used

This was also reported here: #55

---

Rule: no-useless-constructor

Using the same class property assignment within a constructor example from above, we will also get a false negative on no-useless-constructor

class SomeClass {
    constructor(
        private foo: string
    ) {}
    someMethod() {
        return this.foo
    }
}

/* ESLint:
error, Useless constructor

---

Rule: space-infix-ops

This rule is basically unusable, given the type annotation syntax :

---

Rule: semi

It was pointed out here #61 (comment) that TypeScript-only statements will currently not be detectable as needing a semi-colon.

E.g.

"semi": ["error", "always"]

type Result<T> = Success<T> | Failure

/* ESLint:
(No error reported, but should point out missing semi-colon)

---

[JamesHenry]

I would also like to point out that in spite of the above, I am running over 100 of the core ESLint rules on the large TypeScript codebase without any problems at all!

🎉

[nzakas]

Thanks for doing this! I'm low on energy, so probably will dig in more next week.

Off the top of my head, I think we can make space-infix-ops skip type annotations in the core rule (we've done this elsewhere).

For semi, are there many TS-specific constructs that need semicolons? Or is it just a subset?

[JamesHenry]

No problem at all! Just reply whenever you can.

For semi, are there many TS-specific constructs that need semicolons? Or is it just a subset?

I cannot say that I have ever thought about it (I personally use no semi-colons, regardless of JavaScript or TypeScript), but I feel like it would be limited to a subset.

I will look into submitting a PR to eslint for the space-infix-ops rule.

[nzakas]

Part of what I'm wondering is if things like the type statement can be represented as VariableDeclaration so it would transparently work, but if there's a large number of statements besides that, then we need something a bit more generic.

[JamesHenry]

There is no reference to semi-colons in the language spec: https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md

I have asked the TypeScript team here: microsoft/TypeScript#10730

[JamesHenry]

@nzakas It seems like it might be worth doing it for just the type alias (e.g. type Result<T> = Success<T> | Failure) initially:

From Mohamed on the TS Team:

The only place that it is required by the grammar is the type alias declaration. and the implementation enforces the same ASI rules.

So, coming back to your suggestion:

Part of what I'm wondering is if things like the type statement can be represented as VariableDeclaration so it would transparently work

That would be an update to the core rule right? Not an AST (and therefore parser) change?

[nzakas]

@JamesHenry what I meant was, we change what the parser output so that the type alias declaration outputs a VariableDeclaration node instead of whatever we're doing now. That way, the core rule will just work because it already checks VariableDeclaration nodes (though we'd have to double-check that other core rules aren't confused by this, but giving it a brief once-over, I think it should work fine).

Basically, VariableDeclaration.kind would be "type" instead of "var" or "let" or "const".

[JamesHenry]

Ah gotcha! Yeah let's give that a go as a first step

[JamesHenry]

🎉

[vitorbal]

This is great, awesome job @JamesHenry!

[jkillian]

@JamesHenry nice work on the type aliases, I think treating them as a special kind of variable declaration makes sense - since it's very similar syntacticly.

Not sure if you have a list of TS-specific constructs you're testing, but a couple more worth checking if they need special treatment:

Module declarations, for example shorthand ambient module declarations:

declare module "foo/*";

Type guards:

function isCat(a: any): a is Cat {   // specifically, the `a is Cat` part
  return a.name === 'kitty';
}

The non-null assertion operator:

function processEntity(e?: Entity) {
    validateEntity(e);
    let s = e!.name;  // Assert that e is non-null and access name
}

Typed usages of this (this is not really a parameter):

function f(this: void) {
    // make sure `this` is unusable in this standalone function
}

I don't think any of these will cause too big of issues, and some of them are relatively new TS features, but wanted to throw them out there as things to watch out for!