Add validators for Flow AST node fields #7107

bcherny · 2017-12-26T04:02:34Z

Q	A
Fixed Issues?	no
Patch: Bug Fix?	YES
Major: Breaking Change?	no
Minor: New Feature?	no
Tests Added + Pass?	YES
Documentation PR	no
Any Dependency Changes?	no
License	MIT

In this PR, we add types to the existing Flow AST nodes. This brings the quality of Flow AST node definitions up to par with existing definitions for TypeScript/Core/Experimental/etc. nodes, and enables:

Run time type checking for Flow node fields
Compile time type checking for Flow node fields (once we regenerate Flow & TS types)

I tried my best to make sure field type definitions are thorough and complete. I used existing Babel unit tests as my source of truth.

This PR complements, and is independent of, #7101.

bcherny · 2017-12-26T04:03:04Z

cc @loganfsmyth

babel-bot · 2017-12-26T04:11:55Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6381/

existentialism

Awesome work!

Just a couple comments and:

We should probably make sure any type's visitor list is up to date with their fields so they can be set in their builder (similar to the DeclareModule comment I left above)
Can you run node scripts/generate-babel-types-docs to regenerate @babel/types' README?

existentialism · 2017-12-26T15:18:47Z

packages/babel-types/src/definitions/flow.js

  },
 });

 defineType("DeclareExportDeclaration", {
  visitor: ["declaration", "specifiers", "source"],
  aliases: ["Flow", "FlowDeclaration", "Statement", "Declaration"],
  fields: {
-    // todo
+    declaration: validateOptionalType("TSType"),


TSType -> Flow

existentialism · 2017-12-26T15:31:32Z

packages/babel-types/src/definitions/flow.js

-    // todo
+    key: validateType("Identifier"),
+    value: validateType(["Flow", "Identifier"]),
+    kind: validate(assertOneOf("init")),


"init" | "get" | "set"

existentialism · 2017-12-26T15:36:29Z

packages/babel-types/src/definitions/flow.js

+    id: validateOptionalType("Identifier"),
+    key: validateType("Flow"),
+    value: validateType(["Flow", "Identifier"]),
+    static: validate(assertValueType("boolean")),


IIRC, this can have variance:
http://astexplorer.net/#/gist/46bffcf1acdaccb35dda30ee3713da48/1f7ef4e223f06eefbc87a893a9a875313b5c9145

existentialism · 2017-12-26T16:01:02Z

packages/babel-types/src/definitions/flow.js

-    // todo
+    id: validateType(["Identifier", "StringLiteral"]),
+    body: validateType("BlockStatement"),
+    kind: {


This needs to either be:

validateOptional(assertOneOf("CommonJS", "ES"))

or

{ optional: true, validate: assertOneOf("CommonJS", "ES"), }

existentialism · 2017-12-26T16:05:54Z

packages/babel-types/src/definitions/flow.js

+    typeParameters: validateOptionalType("TypeParameterDeclaration"),
+    extends: validateOptionalType("InterfaceExtends"),
+    mixins: validateOptional(arrayOfType("Flow")),
+    body: validateType("ObjectTypeAnnotation"),
  },
 });

 defineType("DeclareModule", {
  visitor: ["id", "body"],


Let's add kind to visitor list so we can do something like:

t.declareModule(t.identifier('d'), t.blockStatement([]), "ES")

bcherny · 2017-12-26T20:18:08Z

@existentialism Thanks for all the comments - very helpful! One question (otherwise comments are all addressed)-

We should probably make sure any type's visitor list is up to date with their fields so they can be set in their builder (similar to the DeclareModule comment I left above)

Currently, it looks like visitor arrays are usually subsets of fields arrays. It sounds like this isn't intentional?

nicolo-ribaudo · 2017-12-26T20:54:41Z

It is intentionals, because visitors is an array containing the fields which should be visited by the traverser.

bcherny · 2017-12-26T20:56:27Z

@nicolo-ribaudo Can you clarify - should visitors equal Object.keys(fields)? Or not always?

nicolo-ribaudo · 2017-12-26T21:02:09Z

packages/babel-types/src/definitions/flow.js

 });

 defineType("ArrayTypeAnnotation", {
  visitor: ["elementType"],
  aliases: ["Flow"],
  fields: {
-    // todo
+    elementType: validateType("Flow"),


This whould also allow thing like TypeAlias and ClassImplements, but it should only allow types.

What would you use instead? FlowBaseAnnotation?

existentialism · 2017-12-26T21:13:16Z

Yep, that's what I get for reviewing before coffee... I meant to say builder key instead of visitor :)

For ref: https://github.com/babel/babel/blob/master/packages/babel-types/src/definitions/core.js#L48

nicolo-ribaudo

There are many places (I just comented few of them) where you used Flow but it includes too many types. I think that we could introduce a new FlowType alias.

nicolo-ribaudo · 2017-12-26T21:12:38Z

packages/babel-types/src/definitions/flow.js

-    // todo
+    id: validateType("Identifier"),
+    typeParameters: validateOptionalType("TypeParameterDeclaration"),
+    right: validateType("Flow"),


Also here, it should only allow types.

nicolo-ribaudo · 2017-12-26T21:15:30Z

packages/babel-types/src/definitions/flow.js

+    id: validateType("Identifier"),
+    typeParameters: validateOptionalType("TypeParameterDeclaration"),
+    extends: validate(arrayOfType("InterfaceExtends")),
+    mixins: validate(arrayOfType("InterfaceExtends")),


Just for curiosity, what is this?

See facebook/flow@b72d1c6

nicolo-ribaudo · 2017-12-26T21:24:46Z

packages/babel-types/src/definitions/flow.js

  },
 });

 defineType("DeclareModule", {
-  visitor: ["id", "body"],
+  visitor: ["id", "body", "kind"],


Let's add kind to visitor list so we can do something like:

t.declareModule(t.identifier('d'), t.blockStatement([]), "ES")

IIRC, the parameters are defined in the builder array. visitor should only contain the nodes which should be traversed.

nicolo-ribaudo · 2017-12-26T21:25:41Z

@nicolo-ribaudo Can you clarify - should visitors equal Object.keys(fields)? Or not always?

Not always, it should be something like Object.keys(fields).filter(isNode)

bcherny · 2017-12-26T21:54:47Z

@existentialism @nicolo-ribaudo All comments are addressed (pending FlowType).

@nicolo-ribaudo Let me know what should go in FlowType, and I can add it.

nicolo-ribaudo · 2017-12-26T22:10:26Z

I think it should contain any node whose name ends with TypeAnnotation (but not the TypeAnnotation node itself)

bcherny · 2017-12-26T22:57:25Z

@nicolo-ribaudo @existentialism Done.

bcherny · 2017-12-28T04:52:02Z

Can we go ahead and merge this?

nicolo-ribaudo

I left a few comments where I think that FlowType should be used. I also noticed a problem with the name of a field and a place where the type should be Expression instead of Flow.

nicolo-ribaudo · 2017-12-28T17:23:58Z

packages/babel-types/src/definitions/flow.js

-    // todo
+    id: validateType("Identifier"),
+    typeParameters: validateOptionalType("TypeParameterDeclaration"),
+    supertype: validateOptionalType("Flow"),


nicolo-ribaudo · 2017-12-28T17:24:40Z

packages/babel-types/src/definitions/flow.js

+    typeParameters: validateOptionalType("TypeParameterDeclaration"),
+    params: validate(arrayOfType("FunctionTypeParam")),
+    rest: validateOptionalType("FunctionTypeParam"),
+    returnType: validateType("Flow"),


nicolo-ribaudo · 2017-12-28T17:26:00Z

packages/babel-types/src/definitions/flow.js

  },
 });

 defineType("FunctionTypeParam", {
  visitor: ["name", "typeAnnotation"],
  aliases: ["Flow"],
  fields: {
-    // todo
+    name: validateOptionalType("Identifier"),
+    typeAnnotation: validateType("Flow"),


nicolo-ribaudo · 2017-12-28T17:26:37Z

packages/babel-types/src/definitions/flow.js

  fields: {
-    // todo
+    types: validate(arrayOfType("Flow")),


nicolo-ribaudo · 2017-12-28T17:26:51Z

packages/babel-types/src/definitions/flow.js

  fields: {
-    // todo
+    typeAnnotation: validateType("Flow"),


nicolo-ribaudo · 2017-12-28T17:39:50Z

packages/babel-types/src/definitions/flow.js

-    typeAnnotation: {
-      validate: assertNodeType("Flow"),
-    },
+    typeAnnotation: validateType("Flow"),


nicolo-ribaudo · 2017-12-28T17:39:56Z

packages/babel-types/src/definitions/flow.js

  },
 });

 defineType("TypeCastExpression", {
  visitor: ["expression", "typeAnnotation"],
  aliases: ["Flow", "ExpressionWrapper", "Expression"],
  fields: {
-    // todo
+    expression: validateType("Flow"),


Expression, I think.

nicolo-ribaudo · 2017-12-28T17:41:52Z

packages/babel-types/src/definitions/flow.js

-    },
+    name: validate(assertValueType("string")),
+    bound: validateOptionalType("TypeAnnotation"),
+    default: validateOptionalType("Flow"),


nicolo-ribaudo · 2017-12-28T17:44:06Z

packages/babel-types/src/definitions/flow.js

-        assertEach(assertNodeType("Flow")),
-      ),
-    },
+    params: validate(arrayOfType("Flow")),


nicolo-ribaudo · 2017-12-28T17:44:14Z

packages/babel-types/src/definitions/flow.js

  fields: {
-    // todo
+    types: validate(arrayOfType("Flow")),


bcherny · 2017-12-29T01:25:58Z

@nicolo-ribaudo Done.

bcherny · 2017-12-29T23:16:51Z

^ Looks like this PR caught a bug in an existing test!

add validators for Flow fields

c9c2c3e

existentialism added pkg: types area: flow labels Dec 26, 2017

existentialism requested changes Dec 26, 2017

View reviewed changes

address review comments

d92eca0

nicolo-ribaudo reviewed Dec 26, 2017

View reviewed changes

review comments, round 2

86eab81

create FlowType alias, and use it where appropriate

9bc52a3

existentialism approved these changes Dec 28, 2017

View reviewed changes

nicolo-ribaudo reviewed Dec 28, 2017

View reviewed changes

existentialism added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 28, 2017

review comments, round 3

197df7d

fix test

2dd60f9

existentialism approved these changes Dec 29, 2017

View reviewed changes

nicolo-ribaudo approved these changes Dec 30, 2017

View reviewed changes

existentialism merged commit 4208099 into babel:master Dec 30, 2017

bcherny deleted the flow-types2 branch December 31, 2017 18:25

existentialism mentioned this pull request Jan 9, 2018

Regen lib/types #7180

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Add validators for Flow AST node fields #7107

Add validators for Flow AST node fields #7107

Conversation

bcherny commented Dec 26, 2017 • edited Loading

bcherny commented Dec 26, 2017

babel-bot commented Dec 26, 2017 • edited Loading

existentialism left a 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

bcherny commented Dec 26, 2017 • edited Loading

nicolo-ribaudo commented Dec 26, 2017

bcherny commented Dec 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

existentialism commented Dec 26, 2017 • edited Loading

nicolo-ribaudo left a 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

nicolo-ribaudo commented Dec 26, 2017

bcherny commented Dec 26, 2017 • edited Loading

nicolo-ribaudo commented Dec 26, 2017

bcherny commented Dec 26, 2017

bcherny commented Dec 28, 2017

nicolo-ribaudo left a comment • 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

bcherny commented Dec 29, 2017

bcherny commented Dec 29, 2017

bcherny commented Dec 26, 2017 •

edited

Loading

babel-bot commented Dec 26, 2017 •

edited

Loading

bcherny commented Dec 26, 2017 •

edited

Loading

existentialism commented Dec 26, 2017 •

edited

Loading

bcherny commented Dec 26, 2017 •

edited

Loading

nicolo-ribaudo left a comment •

edited

Loading