Enable type-checking in src #393

ghost · 2017-03-02T23:33:45Z

Q	A
Bug fix?	no
Breaking change?	no
New feature?	no
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	Tests pass
Fixed tickets	None
License	MIT

This change enables type-checking in all src files (via all=true in .flowconfig).

Two big changes were needed:

Prototype assignment (Parser.prototype.foo = function () { ... }) doesn't get a type-checked this. Changed these to Parser.mixin(class extends Parser { ... }, and did the actual prototype assignment in mixin.
Similarly, instead of using parser.extend, a plugin is now a function (superClass: Class<Parser>): Class<Parser> => class extends superClass { ... }, and parser.usePlugin does the actual extending (and setting of super).

Other than those, I tried as much as possible to keep the code as it is, and just add type annotations and comments.

Notes:

For mixed-in methods used outside of the mixin class, they must be declared in parserTypes.js or they are invisible to the outside.
This disables eslint's indent rule so that it can get a readable diff. Indenting could be fixed in another PR.
This declares [key: string]: any on Node, since often a node will be downcast to another node type and writing all those downcasts would be a pain.
estree.js contains lots of $FlowIgnore since we're changing the node to a different type.
Added throw in front of this.raise(...) and this.unexpected(...) so that control-flow analysis can know about these.

codecov · 2017-03-02T23:40:06Z

Codecov Report

Merging #393 into master will decrease coverage by 0.13%.
The diff coverage is 98.48%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   98.26%   98.13%   -0.14%     
==========================================
  Files          20       20              
  Lines        3511     3478      -33     
  Branches      934      936       +2     
==========================================
- Hits         3450     3413      -37     
- Misses         22       25       +3     
- Partials       39       40       +1

Flag	Coverage Δ
#babel	`82.2% <85.9%> (-0.14%)`	⬇️
#babylon	`96.89% <98.04%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
src/util/identifier.js	`100% <100%> (ø)`	⬆️
src/options.js	`100% <100%> (ø)`	⬆️
src/tokenizer/index.js	`98.35% <100%> (ø)`	⬆️
src/parser/location.js	`100% <100%> (ø)`	⬆️
src/parser/comments.js	`96.07% <100%> (-0.08%)`	⬇️
src/parser/statement.js	`99.07% <100%> (ø)`	⬆️
src/parser/node.js	`96.87% <100%> (-0.19%)`	⬇️
src/parser/lval.js	`98.63% <100%> (ø)`	⬆️
src/tokenizer/state.js	`100% <100%> (ø)`	⬆️
src/tokenizer/types.js	`100% <100%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c07efb...0dd3b8a. Read the comment docs.

danez · 2017-03-03T20:34:46Z

.eslintrc

@@ -2,5 +2,8 @@
  "extends": "babel",
  "env": {
    "node": true
+  },
+  "rules": {
+    "indent": 0


Why did you disable this rule? Where there problems with it?

If I indent things correctly, this PR will have an unreadable diff because the indent of most lines will change. Don't know a good solution for that.

Huh? Wasn't it properly indented before?

Edit: Ah I see the mixin part. Nice catch. Yes we can enable and autofix later

👍

danez · 2017-03-03T20:36:05Z

src/options.js

+  allowImportExportEverywhere: boolean;
+  allowSuperOutsideMethod: boolean;
+  plugins: $ReadOnlyArray<string>;
+  strictMode: any;


strictMode can be boolean or null

danez · 2017-03-03T21:13:42Z

src/plugins/flow.js

+    jsx_readToken() {
+      // $FlowFixMe This method doesn't override anything!
+      if (!this.state.inType) return super.jsx_readToken();
+    }


Looking at the history this was never working, so can you please remove this?

danez · 2017-03-04T08:37:25Z

src/plugins/estree.js

-        node.body.unshift(this.directiveToStmt(directive));
-      });
+      const directiveStatements = node.directives.map((d) => this.directiveToStmt(d));
+      node.body = directiveStatements.concat(node.body);


reverse() got missing.

reverse + forEach + unshift = concat. ☺️

vkurchatkin · 2017-03-07T12:27:42Z

src/types.js

+  end: number;
+  loc: SourceLocation;
+  range: [number, number];
+  leadingComments?: ?Array<Comment>;


Flow has a bug with maybe-typed properties in interfaces. It's better to avoid interfaces completely unless they only contain methods

This is implemented by a class, so I think we need the interface?

vkurchatkin · 2017-03-07T12:38:01Z

src/parser/comments.js

-
-pp.addComment = function (comment) {
+Parser.mixin(class extends Parser {
+addComment(comment: N.Comment): void {


You can omit annotations if you don't export class or function

Some of them could be omitted, but I've tested around and some of them can't without breaking compilation. I also think it's nice to have the type annotations locally so I don't have to look them up in parserTypes.js. The code worked fine without any type annotations, the point was to make it easier to read.

vkurchatkin · 2017-03-07T12:41:49Z

src/parser/node.js

    this.type = "";
+    // $FlowNonNull
    this.start = pos;


If it's always present, then why pos is optional?

new Node; is called in one place, but I just added a $FlowFixMe comment at that place and no longer need hacks here. Thanks!

vkurchatkin · 2017-03-07T12:44:21Z

src/parser/statement.js

@@ -238,6 +244,7 @@ pp.parseForStatement = function (node) {
    if (forAwait) {
      this.unexpected();
    }
+    // $FlowCast node: N.ForStatement


It's better to do this explicitly with any: ((node: any): N.ForStatement)

…with actual casts

Jessidhia · 2017-03-10T01:12:40Z

src/parser/expression.js


-pp.shouldParseAsyncArrow = function () {
+shouldParseAsyncArrow(): boolean {


Does flow not infer this, or do you want to assert that it returns a boolean?

For a function declared in parserTypes.ts, the return type should be carried along from there. But for a function declared only locally, you may get no (or confusing) errors without a type annotation. I think it's better to just consistently declare types. E.g., remove the return type from parseCallExpressionArguments, add elts.push(0); somewhere, and you won't get any errors.

Jessidhia · 2017-03-10T01:13:41Z

src/parser/expression.js

      const value = this.state.value;
-      node = this.parseLiteral(value.value, "RegExpLiteral");
+      const node = this.parseLiteral(value.value, "RegExpLiteral");


A similar change could be made in most of the switch cases, but probably best to not make the diff much larger than it needs to be

Whoops, must have been trying something and forgot to change it back.

Jessidhia · 2017-03-10T01:17:11Z

src/parser/index.js

+        continue;
+      if (proto[key])
+        throw new Error(`Can't mixin '${key}', already defined`);
+      proto[key] = mixedProto[key];


The copied keys will be enumerable, but they are not when defined in the class expression.

On the other hand, before the change to class expression, they were enumerable anyway, so it might not matter.

I don't have an opinion on this.

Jessidhia · 2017-03-10T01:19:54Z

src/parser/index.js

+      }
+
+      superProto[key] = self[key];
+      self[key] = extenderProto[key];


Same thing in these two lines here w.r.t. enumerability.

hzoo · 2017-03-21T21:09:39Z

We moved the previous 7.0 branch to be master now, and master is now 6.x

hzoo · 2017-03-21T21:55:39Z

Hey @andy-ms! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

ghost · 2017-03-21T22:01:22Z

It looks like the test failure is in babel somewhere -- did this PR change it or was the change in babel?

hzoo · 2017-03-21T22:03:35Z

Yeah a different pr we need to fix that I merged, need to mereg babel/babel#5450

ghost · 2017-03-28T17:21:31Z

Hm, still failing.

TypeError: [BABEL] /home/travis/build/babel/babylon/node_modules/istanbul-lib-instrument/dist/index.js: Plugin 0 specified in "/home/travis/build/babel/babylon/.babelrc.env.test" was expected to return a function but returned "undefined" (While processing preset: "/home/travis/build/babel/babylon/node_modules/babel-preset-stage-0/lib/index.js") (While processing preset: "/home/travis/build/babel/babylon/node_modules/babel-preset-es2015/lib/index.js")

existentialism · 2017-03-28T17:27:19Z

Yeah, looks like an unrelated (to this PR) issue with the latest b7 alpha, will take a look after lunch.

ghost · 2017-04-03T21:43:44Z

@existentialism Seems to have been fixed.

ghost · 2017-04-06T17:43:07Z

Again, test failures are probably due to some change in babel?
Update: It's failing on master, made an issue: #453

danez · 2017-04-07T12:41:45Z

Besides it being a massive diff I do not like they idea of restructuring code just to please flow. It might have been easier to start small and just enable flow file by file and PR them separately.
This current approach with the inheriting classes and then the separate file with the type definitions seems rather complicated and unusual to me, and we maybe should think about a better structure of the Parser instance that would also work with flow.

Ohh and we should keep an eye on performance, as a drastic change like this might also have performance impacts.

loganfsmyth · 2017-04-07T16:51:54Z

I'll second performance. The super change could certainly introduce performance changes since it has to look up the prototype chain instead of calling a reference to the function it already has.

That said, I don't have a problem with the structure. I think having the node types in their own file is inevitable since they are used all over the place, and the class extension + mixin approach is a little weird, not doesn't seem too bad to me.

…nd `new` that.

ghost · 2017-04-07T18:57:04Z

Bad news: It looks like with plugins applied, this branch does make parsing a bit slower.
Good news: We can make it a lot faster!

Currently, plugins work by writing functions directly to the instance.
This branch modifies plugins to be functions from a superclass to a new subclass.
But it then takes the methods off of that subclass and directly assigns them...

I have modified this to instead just create a class, and never directly assign to an instance.
These classes can be reused across multiple parse calls, and node seems to have an easier time optimizing them.

Here are my measurements parsing jquery with the flow and jsx plugins applied:

Name	Time
master, no plugins	7.86 8.07 8.02 8.14 7.84
type-check, no plugins	7.93 8.05 7.80 7.97 7.96
master with plugins	22.0 21.2 21.6 21.0
type-check with plugins	25.3 24.1 23.0 22.9
type-check with cached plugins	10.6 10.5 10.3 10.0

It looks like the existence of instance functions was making it run significantly slower, but moving that all into a prototype brought the time back down. There is of course still some increase in time due to the actual work that the plugin does.

Shall I make a pull request doing just this? (UPDATE: #459)

ghost · 2017-05-19T21:03:10Z

Closing since this has been broken into many separate PRs.

#459 Convert each plugin to a function from a class to an overriding class
#460 Type-check tokenizer/index.js
#480 Add type declarations for AST nodes
#481 Convert each file with parser methods to a class in an inheritance chain
#482 Move plugin helpers out of Parser.prototype and into the plugin itself
#484 Type-check CommentsParser and LocationParser
#485 Type-check UtilParser
#486 Type-check node.js
#487 Type-check LValParser
#488 Type-check ExpressionParser
#489 Type-check StatementParser
#490 Type-check options.js and index.js
#491 Type-check utils
#492 Type-check State
#493 Type-check tokenizer/types.js
#494 Type-check estree plugin
#495 Type-check flow plugin
#496 Type-check JSX plugin
#521 Fix type check errors

Enable type-checking in src

29c888b

Andy Hanson added 2 commits March 3, 2017 06:27

Clean up comments

2dcf4d2

Fix lint error

203a898

danez reviewed Mar 3, 2017

View reviewed changes

Specify type of strictMode

f1345d8

danez reviewed Mar 3, 2017

View reviewed changes

Remove broken overload

0d1b8e4

danez reviewed Mar 4, 2017

View reviewed changes

vkurchatkin reviewed Mar 7, 2017

View reviewed changes

Node constructor takes non-null arguments, and replace $FlowCast …

cb5e2b4

…with actual casts

Jessidhia reviewed Mar 10, 2017

View reviewed changes

Andy Hanson added 3 commits March 10, 2017 07:41

Undo unnecessary change

3aede41

Remove TODO comment (spec fixed by #406)

fa4db25

Fix recursive type

ea797ec

hzoo changed the base branch from 7.0 to master March 21, 2017 21:09

Merge branch 'master' into type-check

2ea4c97

existentialism added the Tag: Internal label Mar 22, 2017

danez mentioned this pull request Mar 27, 2017

Reuse plugin "extend" function definitions #435

Closed

Merge remote-tracking branch 'upstream/master' into type-check

5c9a2ac

hzoo added the Priority: High label Mar 30, 2017

Merge branch 'master' into type-check

ba87342

Andy Hanson added 3 commits April 4, 2017 11:04

Merge branch 'master' into type-check

d7cc741

Fix type of parseConditional

9d430de

Merge branch 'master' into type-check

0b8a2fb

Do not apply plugins to the instance, create a plugin-applied class a…

a1ecbec

…nd `new` that.

Handle undefined options

0dd3b8a

ghost mentioned this pull request Apr 9, 2017

Convert each plugin to a function from a class to an overriding class #459

Merged

This was referenced Apr 21, 2017

Add type declarations for AST nodes #480

Merged

Convert each file with parser methods to a class in an inheritance chain #481

Merged

ghost closed this May 19, 2017

ghost deleted the type-check branch May 19, 2017 21:03

This pull request was closed.


		pp.shouldParseAsyncArrow = function () {
		shouldParseAsyncArrow(): boolean {

Enable type-checking in src #393

Enable type-checking in src #393

Conversation

ghost commented Mar 2, 2017

codecov bot commented Mar 2, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Mar 10, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Jessidhia Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Mar 21, 2017

hzoo commented Mar 21, 2017

ghost commented Mar 21, 2017

hzoo commented Mar 21, 2017 • edited Loading

ghost commented Mar 28, 2017

existentialism commented Mar 28, 2017

ghost commented Apr 3, 2017 • edited by ghost Loading

ghost commented Apr 6, 2017 • edited by ghost Loading

danez commented Apr 7, 2017

loganfsmyth commented Apr 7, 2017

ghost commented Apr 7, 2017 • edited by ghost Loading

ghost commented May 19, 2017 • edited by ghost Loading

codecov bot commented Mar 2, 2017 •

edited

Loading

danez Mar 3, 2017 •

edited

Loading

ghost Mar 10, 2017 •

edited by ghost

Loading

Jessidhia Mar 10, 2017 •

edited

Loading

hzoo commented Mar 21, 2017 •

edited

Loading

ghost commented Apr 3, 2017 •

edited by ghost

Loading

ghost commented Apr 6, 2017 •

edited by ghost

Loading

ghost commented Apr 7, 2017 •

edited by ghost

Loading

ghost commented May 19, 2017 •

edited by ghost

Loading