[Trial] add static type validation to our codebase

[mysticatea]

What is the purpose of this pull request? (put an "X" next to item)

[X] Other, please explain: Add static type validation to our codebase.

ECMAScript can add new syntax every other month and the add can change our AST. It's relatively hard to check how the AST change affects our rules. Static type validation will help it. For example, when SpreadElement was added into ObjectExpression#properties, type validation could report all places we should fix, if the validation was existed.

TypeScript can validate JavaScript files. This PR adds TypeScript static type validation to our codebase.

What changes did you make? (Give an overview)

• Add TypeScript as a dev dependency.
• Add extensible AST types.
• Add types for rule definitions.
• Apply type validation to some core rules.
• Evaluate and discuss the direction with the team.
• Add more comments to types.
• Apply type validation to more core rules.
• Add tsc to CI.
• Expose types as public API. (probably it needs an RFC and a separated PR)

Add extensible AST types

Some popular custom parsers (@typescript-eslint/parser / vue-eslint-parser) are written by TypeScript and each one has defined own AST types from scratch. Each they have to follow ECMAScript updates.

But if we provide the common part of AST types and they can extend it with differential AST definition, it will be nice because custom parser authors can focus on their syntax. Also, we (the ESLint team) can check what differences are from espree's AST easily because the differential definition is there. That will help us to fix core rules to avoid crashes.

Maybe people can extend the AST types with like the following way:

import { AST } from "eslint"

type ASTDefinition = AST.Def.Extends<
    AST.ES2020.ASTDefinition,
    {
        nodes: {
            // Add properties to existing nodes.
            Identifier: {
                // `NodeRef<T>` represents the reference to other nodes.
                typeAnnotation: AST.Def.NodeRef<"TypeAnnotation">
            }

            // Add new node.
            // Four common properties (`type`, `range`, `loc`, `parent`) will be
            // added automatically.
            TypeAnnotation: {
                name: string
            }
            MagicStatement: {
                magic: number
            }
        }
        // Add new statement node to `AST.Def.NodeRef<"Statement">`.
        statementType: "MagicStatement"
        expressionType: never
    }
>

// Define the actual `Node` types from the extended AST definition.
type AST<T> = AST.Def.ExtractNode<ASTDefinition, T>

// The union type of all nodes.
type Node = AST<"Node">
type Statement = AST<"Statement">
type Expression = AST<"Expression">

// Identifier node.
type Identifier = AST<"Identifier">

// Every node has the auto-calculated `parent` property. The type of the `parent`
// property includes only the nodes which reference the node by `NodeRef<T>`.
type IdentifierParent = Identifier["parent"]

// An ESTree node can be represented by the union type of two or more types.
// For example, ESTree `MemberExpression` node is the union type of
// `BasicMemberExpression` and `ComputedMemberExpression`.
// Therefore, you can nallow the type by `computed` proeprty. The following
// expression is valid: `!node.computed && node.property.name === "foo"` because
// the `BasicMemberExpression#property` type is `Identifier`.
type MemberExpression = AST<"MemberExpression">

In fact, this PR has the ES5 AST definition and the differential AST definitions of ES2015-ES2020, then adopts ES2019 AST to verify our codebase.

• ES5
• ES2015
• ES2016
• ES2017
• ES2018
• ES2019
• ES2020

Add types for rule definitions.

This PR adds required types to define rules.

• Scope
• SourceCode
• JSON Schema (convert JSON Schema to the corresponded type)
• Rule

And a small utility to infer types.

• lib/rules/utils/rule.d.ts

Apply type validation to some core rules.

This PR applies the types to the following files:

• 8 rules which don't have any require() in alphabet order
  • lib/rules/accessor-pairs.js
    • It had a wrong assumption that the property name is Identifier, but it can be Literal as well in fact.
    • It forgot about SpreadProperty.
  • lib/rules/block-scoped-var.js
    • No problem.
  • lib/rules/callback-return.js
    • No problem.
  • lib/rules/camelcase.js
    • JSON Schema was wrong.
    • It had a dead code.
    • It had a wrong assumption that the property name is Identifier, but it can be Literal as well in fact.
  • lib/rules/class-methods-use-this.js
    • It had a wrong assumption that the property name is Identifier, but it can be Literal as well in fact.
  • lib/rules/constructor-super.js
    • No problem.
    • But I modified code to fit TypeScript control flow type narrowing.
  • lib/rules/default-case.js
    • No problem.
  • lib/rules/eol-last.js
    • No problem.
• lib/rules/utils/ast-utils.js
  • No problem.
  • But I modified code to fit TypeScript control flow type narrowing.
• A rule which uses ast-utils
  • lib/rules/array-bracket-newline.js
    • No problem.

My impression:

• I could add types comfortable because TypeScript told me the variables that it failed to infer the type.
• But there were a few cases that I have to add cast comments around sourceCode.getTokenAfter()-like methods because the return type said "it can be null" but actually never.
• Some rules looked to overlook Literal property names (E.g., { 0: a, "a": b }).

Is there anything you'd like reviewers to focus on?

• Is this PR acceptable?
• Is there a more comfortable way to write rules with types?

[mdjermanovic]

accessor-pairs doesn't work well, not just because of this but the logic for object literals is wrong. It shouldn't check pairs per the whole object but per a property.

PR #12062 should fix this.

[mysticatea]

Yes, you are right. But because this PR focuses on only type-related matters, that's another issue. This branch was forked from an old point, I will merge updates from the master at some point.

[ilyavolodin]

This is a pretty significant change and would most definitely require TSC discussion. On one hand, it's very nice that this PR adds better validation, and make it easier to spot issues with rules and schemas. On the other, it feels like it also adds a huge overhead of maintaining types for a lot of different things. On top of that (and this is completely personal opinion), I'm not 100% convinced that TypeScript is not a passing thing. A few years back everyone used CoffeeScript too and people were convinced that it's here to stay. And while I do like idea of TypeScript, once decorators are added to the ECMA spec, I'm reasonable sure TypeScript will go the way of CoffeeScript, LiveScript and a bunch of others, and we will be stuck with outdated code that needs migration.

[bradzacher]

once decorators are added to the ECMA spec, I'm reasonable sure TypeScript will go the way of CoffeeScript, LiveScript and a bunch of others

@ilyavolodin - why do you think that decorators are the only thing keeping typescript around? Outside of Angular2+, a lot of developers don't use decorators in typescript because the implemented spec is old, limited and not hugely useful.

Both Microsoft and Google have put a lot of time, effort and money behind it, and Microsoft is showing no signs of removing that effort. I firmly believe Facebook would be using it as well, if they hadn't created Flow.
To my knowledge, no other company is creating a real challenger to TypeScript. Flow is ofc an alternative, but the non-FB usage of Flow is a poorly supported afterthought which limits its usage.

The package is close to having as many weekly downloads on NPM as eslint. Which shows how widely it is used by the community.

it feels like it also adds a huge overhead of maintaining types for a lot of different things

I'm curious where the overhead is for maintaining types?

You guys currently maintain comprehensive JSDoc comments to help type your code statically (such as https://github.com/eslint/eslint/blob/master/lib/shared/types.js, as well as on top of every single function).

A few extra types shouldn't be any real maintenance burden right? And IMO TypeScript types are easier to write, maintain, and consume compared to JSDoc comments.

Additionally, the types should be fairly static once written, as eslint doesn't change drastically that often, if at all.

Finally, if anything, the types should reduce maintenance overhead because they catch bugs ahead of time for you.

A few years back everyone used CoffeeScript too and people were convinced that it's here to stay

CoffeeScript introduced a new language with new syntax. A lot of people weren't familiar with it, and it was hard to onboard people onto codebases because of that.
Being a superset of JavaScript, TypeScript is easy to teach people, because they don't have to learn much new - it gives them the familiarity of JavaScript syntax, along with a level of type safety.

Additionally, when CoffeeScript was bigger a thing, I didn't see it have anywhere near the support that TypeScript has. The community was a lot smaller back then, and all of the tooling was in a hugely different place. Now with npm and build tools in the place they are, there is a huge ecosystem built around TypeScript - type definitions for libraries, pieces for build chains, linting and static analysis, type generators - the list goes on.

NPM released numbers stating that 46% of surveyed users (n = 16,345) used typescript. To me, it seems like TypeScript is a thing that people are migrating to, not migrating from.

we will be stuck with outdated code that needs migration

This is the great thing about TypeScript being a superset of JavaScript.
Migrating the code back to plain JS is as easy as just deleting the types. You could write a codemod which does exactly that without any real effort.

It would also be pretty trivial to convert the types into JSDoc comments if you wanted to truly revert it all.

[ljharb]

Your points are good (TS is def not the same as coffeescript) but i can’t help but comment that it’s not a superset - symbols and bigints can’t be property keys; objects can’t have a null prototype, including module namespace objects; to name a few. I wish that disingenuous marketing speak would disappear :-/

[ilyavolodin]

@bradzacher It's pretty clear from your comments on this and other issues that you are a big proponent of TypeScript, and that's perfectly fine. As I mention, I don't have anything against TypeScript (as oppose to having a LOT against CoffeeScript back in a day), but this isn't about personal preferences, it's about what's good for the project. While adding typing will bring in some advantages as shown above, it will also bring in some disadvantages like higher maintenance costs, higher barrier of entry for external contributors, need for additional tooling, etc. None of those are critical or unsolvable problems, but have to be evaluated in the context of pros that we are gaining. And I'm not sure the balance is there.
P.S. Also the fact that the project will be written in TypeScript but doesn't support TypeScript parsing natively will raise a lot of questions, I think.

[mysticatea]

On the other, it feels like it also adds a huge overhead of maintaining types for a lot of different things.

Would you elaborate the overhead? I have written the extensible AST types, so we can maintain it with differential definitions of the future syntax. The differential definitions are like the following file.

• lib/types/ast-es2020.d.ts

I don't think that it's a big cost because we must grasp the differential already to support the new syntax. On the contrary, the static type validation helps us to update core rules to support new syntax. It will tell us where we need to modify, though it's not all.

To clarify, this PR doesn't expose the types as public API. I have a plan that I send an RFC to expose, but this PR is not. This PR is to apply static type validation to our codebase.

On top of that (and this is completely personal opinion), I'm not 100% convinced that TypeScript is not a passing thing. A few years back everyone used CoffeeScript too and people were convinced that it's here to stay.

Don't worry. This PR doesn't rewrite our codebase to TypeScript. Our codebase is still vanilla JavaScript. What we need to stop static type validation is just the uninstall of typescript package. What the typescript package does to JavaScript files is to validate code with inferred types and JSDoc comment's types then report it if it was wrong.

And while I do like idea of TypeScript, once decorators are added to the ECMA spec, I'm reasonable sure TypeScript will go the way of CoffeeScript, LiveScript and a bunch of others, and we will be stuck with outdated code that needs migration.

I'm not sure what you said. Why decorators are related to this? I don't think decorators are related to static type validation.

[mysticatea]

Also, I think that the static type will reduce the fence that people fix existing rules or make new rules because popular editors provide input completion (with JSDoc comments) for typed variables.

[ilyavolodin]

Would you elaborate the overhead? I have written the extensible AST types, so we can maintain it with differential definitions of the future syntax. The differential definitions are like the following file.

Besides definitions for AST of different version which you created, and I agree, will not need a lot of maintenance, I see a pretty significant number of d.ts files for other things like rules, parser, plugin, etc. All of those will have to be maintained. You might argue that it's not very much different then jsdoc that we already have, but my position is that it is. It's located in a different file, you have to know about it, you have use editor that supports it, so it would flag things as requiring updates, and so on.

Don't worry. This PR doesn't rewrite our codebase to TypeScript. Our codebase is still vanilla JavaScript. What we need to stop static type validation is just the uninstall of typescript package. What the typescript package does to JavaScript files is to validate code with inferred types and JSDoc comment's types then report it if it was wrong.

True, but it does require adding type references on top of rules, and other places. Again, for somebody who is unfamiliar with TypeScript types, I feel like, it would present a huge barrier for entry.

[platinumazure]

Can we get data somehow on whether contributors would be impacted by Typescript components in ESLint? E.g., a Twitter poll? Might be best to try to hear from impacted stakeholders directly rather than guessing.

[mysticatea]

Besides definitions for AST of different version which you created, and I agree, will not need a lot of maintenance, I see a pretty significant number of d.ts files for other things like rules, parser, plugin, etc. All of those will have to be maintained.

But those types just describe what public APIs are. As same as documentation, We need to modify those only when we updated public API. If a public API was changed, the update of types will be a line and comments that show in input completion. I don't think it's a big cost.

You might argue that it's not very much different then jsdoc that we already have, but my position is that it is. It's located in a different file, you have to know about it, you have use editor that supports it, so it would flag things as requiring updates, and so on.

Yes, because TypeScript supports JSDoc. TypeScript will support contributors to write the right JSDoc as it reports wrong types. Here the new overheads for contributors are,

• Write a magic at the top of files.

  /**
   * @template T
   * @typedef {import("./utils/rule").AST<T>} AST
   */

  I said "a magic", but this is JSDoc comment (@template and @typedef) and dynamic import. It's not far from JavaScript knowledge.
  If other types were needed, all are in lib/rules/utils/rule.d.ts. Write /** @typedef {import("./utils/rule").Foo} Foo */.
• Write AST<"Identifier">-like types instead of ASTNode. Here, the type argument is the ESTree type. This is not an overhead because contributors have ESTree knowledge already.

Editor support is not required. Test (npm test) shows type validation results. But the editor support is very strong, so if contributors used an editor which support TypeScript, the development will be comfortable.

Off course, we should describe those in "Working with Rules" page. But I believe that types reduce the barriers for entry, as the opposite side of you. Because types reduce much of existing barriers.

Currently, contributors have to come and go the editor and a lot of documentation to write rules; ESTree spec, astexplorer, "Working with Rules" page, etc. The knowledge to write rules is much. Input completion and popup comments will reduce much of this cost.

Currently, contributors have to understand ESTree spec entirely to write rules. Otherwise, rules get buggy and they will receive a lot of suggestions in review. But, as this PR found that 1/3 core rules have bugs (I applied types to some core rules in the alphabet order, then 1/3 had bugs), I guess that to check the rules with ESTree spec is hard to reviewers as well. Type validation can catch it correctly. It will reduce the anxiety of contributors as ensuring the rule handles ESTree correctly. Also, it will reduce the cost of reviewers as getting focus on the purpose of the rule.

I believe that we love static analysis because we live here :)
Please mind this is also a static analysis.

---

Can we get data somehow on whether contributors would be impacted by Typescript components in ESLint? E.g., a Twitter poll? Might be best to try to hear from impacted stakeholders directly rather than guessing.

Sounds good idea.

[mysticatea]

TSC Summary

This PR adds a new static analysis, to check the types of JSDoc, to the source code of core rules. The static analysis will find the overlooks to handle AST properly, then fail tests.

Pros:

• Contributors can notice the edge cases of AST handling. (for example, 3/8 core rules in alphabet order have overlooked the literal property names.)
• Reviewers can focus on what the rule does rather than whether the rule handles AST properly or not.
• Popular editors can provide proper input completion and popup documents when people implement rules.

Cons:

• Contributors need knowledge about JSDoc notations and TypeScript type notations.
• The static analysis has false positive due to the limitation of the static type checking. For example, the static analysis says sourceCode.getTokenAfter(token) can return null even if never returns null in the situation.
• We need much work to add the static analysis to every core rule.

TSC Question

Should we go to this direction?

[mysticatea]

TSC Resolution: We need more research before accepting it. Meeting Notes

I will apply types to more rules.

[golopot]

Is there a reason you choose AST<"CallExpression"> over AST.CallExpression? The namespace approach seemed more common, and needs less key strokes.

[mysticatea]

@golopot Because we cannot define namespace members dynamically.

[mysticatea]

And JSDoc @typedef cannot import namespaces. Writing @typedef for every node types into every rule file is trouble.

[kaicataldo]

How do we want to proceed with this? Are we still actively researching this?

[mysticatea]

I will do in the near future, but I don't have enough time currently because of new syntaxes and 7.0.0 development.

[G-Rath]

@mysticatea I thought I'd already posted this, but apparently not: I'm willing to lend a hand with this if you'd like, when you have the bandwidth to pick it back up.

I can also look into picking it up now, and see how far I can get if that'd be useful :)

[kaicataldo]

@mysticatea Just wanted to check in and see if you're still championing this.

[mysticatea]

Yes. I believe that static typing is helpful for our regular work: support new syntaxes.

I have separated some works on this PR to eslint-ast package recently (I'm happy if I can transfer it to eslint org). I will update this PR to use that AST types in core rules when I get time.

...but, current priority in my head is, (1) support optional chaining syntax (#13416 and several my plugins), (2) update eslint-plugin-node, (3) parallel linting (RFC42), (4) update eslint-utils with eslint-ast types, then (5) this. ES modules' support of our config system may be inserted somewhere. It's far a bit.

[nzakas]

I'd like to suggest we close this pull request. As it's a proof-of-concept that is over a year old, the likelihood of merging it is pretty small.

I'd also like to suggest that we put this suggestion through the RFC process so we understand the cost/benefits of this approach better.

[nzakas]

Closing, as there hasn't been any activity and I think this should be put through the RFC process.