Cleanup babel types by jridgewell · Pull Request #5971 · babel/babel

jridgewell · 2017-07-19T05:55:08Z

Q	A
Patch: Bug Fix?	No
Major: Breaking Change?	Yes
Minor: New Feature?	No
Deprecations?	Bad node types
Spec Compliancy?	Yes?
Tests Added/Pass?	No
Fixed Tickets	Fixes #4059
License	MIT
Doc PR
Dependency Changes

This provides a much stricter node system. It also does a deep validation of the AST before printing (should we only do this in testing?).

mention-bot · 2017-07-19T05:55:11Z

@jridgewell, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ForbesLindesay, @existentialism and @xtuc to be potential reviewers.

hzoo · 2017-08-02T21:35:24Z

packages/babel-core/src/transformation/file/index.js

      );
      traverse(this.ast, visitor, this.scope);

+      t.validateDeep(this.ast);


curious how much slower it gets when doing this (I guess it's 1 one time check)?

We could only do this when NODE_ENV === 'test'

mikesherov · 2017-08-04T11:26:17Z

packages/babel-preset-es2015/test/fixtures/traceur/ArrowFunctions/SloppyArguments.js

-var f1 = implements => implements;
-var f2 = implements => { return implements; };
-var f3 = (implements) => { return implements; };
+var f1 = i => i;


These changes seem incorrect. Isn't the whole point of this test file to check sloppy arguments in arrow functions?

Looks like it, but it's impossible to tell where these identifiers were defined. The only other alternative it to forbid Identifiers inside VariableDeclarators from having these.

danez · 2017-08-04T20:39:02Z

packages/babel-traverse/src/path/replacement.js


+  this.debug(() => `Replace with ${node && node.type}`);
+
+  this.node = this.container[this.key] = node;


Shouldn't we first validate and then replace? Or what is the idea of this change?

The node should already be valid, so we just need to check parent-child validations after the replacement.

danez · 2017-08-04T20:43:40Z

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

    name: {
      validate: assertNodeType("JSXIdentifier", "JSXMemberExpression"),
    },
+    // WTF is this on the OpeningElement and not the Element?


Seems so, as it is like that in every parser.

nicolo-ribaudo · 2017-08-07T07:09:25Z

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

  fields: {
    argument: {
-      validate: assertNodeType("LVal"),
+      validate: assertNodeType("Identifier"),


It can also be a member expression, if the destructuring is not part of a parameter/declaration:

var a = {}; [...a.b] = [1, 2, 3]; a.b; // 1, 2, 3

And also a pattern, if the rest element is not inside an object destructuring.

nicolo-ribaudo · 2017-08-07T07:43:15Z

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


 defineType("ForOfStatement", {
-  visitor: ["left", "right", "body"],
+  visitor: ["left", "right", "body", "await"],


Should await be in visitor or in builder?

Builder. 😬

tmp Lock down type system Update tests Add implicit fields to nodes Add missing fields RestElements must be last element Do not mutate an inherited fields map Remove ObjectPatternProperty Fix a few types Deeply validate before printing Update docs

jridgewell

First pass after the typescript changes. I'll finish tomorrow.

jridgewell · 2017-08-16T01:58:29Z

packages/babel-preset-es2015/test/fixtures/traceur/ArrowFunctions/SloppyArguments.js

-var f1 = implements => implements;
-var f2 = implements => { return implements; };
-var f3 = (implements) => { return implements; };
+var f1 = i => i;


Looks like it, but it's impossible to tell where these identifiers were defined. The only other alternative it to forbid Identifiers inside VariableDeclarators from having these.

jridgewell · 2017-08-16T01:59:26Z

packages/babel-traverse/src/path/replacement.js


+  this.debug(() => `Replace with ${node && node.type}`);
+
+  this.node = this.container[this.key] = node;


The node should already be valid, so we just need to check parent-child validations after the replacement.

jridgewell · 2017-08-16T04:19:58Z

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


 defineType("ForOfStatement", {
-  visitor: ["left", "right", "body"],
+  visitor: ["left", "right", "body", "await"],


Builder. 😬

jridgewell

Alright, this should be good for a final review.

jridgewell · 2017-08-17T02:59:30Z

packages/babel-core/src/transformation/file/index.js

      );
      traverse(this.ast, visitor, this.scope);

+      t.validateDeep(this.ast);


We could only do this when NODE_ENV === 'test'

nicolo-ribaudo

I also have few comments/questions not related to your changes:

LabeledStatement#body, IfStatement#consequent, IfStatement#alternate and others are defined as Statement, but they should disallow declarations (except for plain functions). What is the reason for declarations to have a Statement alias?
Can AssignmentPatterns and ArrayPatterns have decorators?

nicolo-ribaudo · 2017-08-28T16:49:08Z

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

    flags: {
-      validate: assertValueType("string"),
      default: "",
    },


Maybe we should check that there are only valid flags?

nicolo-ribaudo · 2017-08-28T17:14:20Z

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

    ...patternLikeCommon,
    argument: {
-      validate: assertNodeType("LVal"),
+      validate: assertNodeType("Identifier", "MemberExpression"),


This can also be a Pattern:

function a(...[x]) {}

nicolo-ribaudo · 2017-08-28T17:20:35Z

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

+      validate: chain(assertNodeType("BlockStatement"), function(node) {
+        if (!node.handler && !node.finalizer) {
+          throw new TypeError(
+            "TryStatement expects either a handler or finalizer, or both",


Now you can remove

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

Line 618 in cee4cde

// todo: at least handler or finalizer should be set to be valid

nicolo-ribaudo · 2017-08-28T17:54:24Z

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

    validate: chain(
      assertValueType("array"),
-      assertEach(assertNodeType("LVal")),
+      assertEach(assertNodeType("Identifier", "Pattern", "RestElement")),


You can use assertNodeType("PatternLike")

nicolo-ribaudo · 2017-08-28T18:50:52Z

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

+    throw new TypeError(
+      `Property ${key} of ${node.type} expected node to be of a type ${JSON.stringify(
+        types,
+      )} ` + `but instead got ${JSON.stringify(val && val.type)}`,


Nit: here it isn't needed to use two concatenated templates. (Also line 81)

nicolo-ribaudo · 2017-08-28T19:00:37Z

packages/babel-types/src/validators.js

-    return false;
-  } else {
-    return esutils.keyword.isIdentifierNameES6(name);
+    if (reserved && name === "await") return false;


reserved && is not needed, since this is inside if (reserved) {

nicolo-ribaudo · 2017-08-28T19:04:26Z

scripts/generate-babel-types-docs.js

+          " (default: `" + util.inspect(defaultValue) + "`"
        );
+        if (types.BUILDER_KEYS[key].indexOf(field) < 0) {
+          fieldDescription.push(", non-buildable");


Maybe something like excluded from the factory function would be more clear?

existentialism added pkg: types PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release labels Jul 19, 2017

existentialism added this to the Babel 7 milestone Jul 25, 2017

hzoo mentioned this pull request Aug 1, 2017

Create aug-02.md babel/notes#29

Merged

hzoo reviewed Aug 2, 2017

View reviewed changes

mikesherov reviewed Aug 4, 2017

View reviewed changes

danez reviewed Aug 4, 2017

View reviewed changes

hzoo mentioned this pull request Aug 5, 2017

Don't insert the same node into the AST multiple times (fixes babel/babili#556) #6054

Merged

nicolo-ribaudo reviewed Aug 7, 2017

View reviewed changes

This was referenced Aug 9, 2017

babel-types: Add missing field, fix incorrect definitions #6083

Merged

AST seems to allow MemberExpression in function parameter list (T7068) #4059

Closed

jridgewell mentioned this pull request Aug 14, 2017

linting: disallow t.identifier("undefined") in plugins #6096

Merged

jridgewell added 2 commits August 15, 2017 22:14

Cleanup type definitions

bdf5ef4

tmp Lock down type system Update tests Add implicit fields to nodes Add missing fields RestElements must be last element Do not mutate an inherited fields map Remove ObjectPatternProperty Fix a few types Deeply validate before printing Update docs

Merge latest 7.0

361a64d

jridgewell force-pushed the cleanup-babel-types branch from 08d5aed to 361a64d Compare August 16, 2017 04:23

jridgewell commented Aug 16, 2017

View reviewed changes

jridgewell added 2 commits August 16, 2017 22:56

Fix tests

39622ab

Final pass

7a34e91

jridgewell commented Aug 17, 2017

View reviewed changes

jridgewell mentioned this pull request Aug 26, 2017

Invalid arrow function destructuring allowed #6161

Closed

hzoo requested review from Jessidhia, existentialism and loganfsmyth August 28, 2017 15:13

nicolo-ribaudo reviewed Aug 28, 2017

View reviewed changes

danez closed this Aug 31, 2017

danez reopened this Aug 31, 2017

danez changed the base branch from 7.0 to master August 31, 2017 17:02

hzoo added this to the Babel 7.next milestone Sep 13, 2017

hzoo removed this from the Babel 7 Beta milestone Sep 13, 2017

This was referenced Sep 27, 2017

Create sept-26.md babel/notes#36

Merged

Issue with reusing AST nodes #6375

Closed

hzoo added the Priority: High label Nov 29, 2017

nicolo-ribaudo removed Priority: High labels Feb 14, 2019

nicolo-ribaudo mentioned this pull request Dec 2, 2019

Dec 04 meeting babel/notes#104

Closed

11 tasks

nicolo-ribaudo mentioned this pull request Dec 23, 2019

(opt-in) Improve @babel/types with env.BABEL_TYPES_8_BREAKING #10917

Merged

nicolo-ribaudo closed this in #10917 Jan 11, 2020

nicolo-ribaudo mentioned this pull request Feb 26, 2020

Enable the BABEL_TYPES_8_BREAKING flag in @babel/types #11172

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 12, 2020

github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2020


		this.debug(() => `Replace with ${node && node.type}`);

		this.node = this.container[this.key] = node;

Uh oh!

Conversation

jridgewell commented Jul 19, 2017 • edited by hzoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danez Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jridgewell commented Jul 19, 2017 •

edited by hzoo

Loading

danez Aug 4, 2017 •

edited

Loading