Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@babel/types builder improvements #14519

Merged
merged 17 commits into from May 9, 2022
Merged

@babel/types builder improvements #14519

merged 17 commits into from May 9, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented May 3, 2022

Q                       A
Supersedes PR Closes #13369
Tests Added + Pass? No yet
Documentation PR Link
Any Dependency Changes?
License MIT

This PR is reviving #13369. Further improvements includes:

  • The default value of each AST node will be type checked
  • Smaller footprints

The test is currently failing. However when I run the failing test, the result diverge from the whole test, implying that it may due to 1) test correlations or 2) or engine optimizer bug. Currently

// Fails
yarn jest ./packages/babel-plugin-proposal-decorators
yarn jest ./packages/babel-plugin-proposal-decorators -t "2021 12"
yarn jest ./packages/babel-plugin-proposal-decorators -t "2021 12 runtime errors  to es2015"
// Pass
yarn jest ./packages/babel-plugin-proposal-decorators -t "2021 12 runtime errors  to es2015 > invalid class decorator return"

Update: the last test is passing because "2021 12 runtime errors to es2015 > invalid class decorator return" does not match any test title (according to jest-runner, which reports "1 skipped, 0 of 1 total"), but jest-light-runner reports "1 passed".

The test is failing because

path.scope.bindingIdentifierEquals(path.node.name, state.innerBinding)
does not guard against undefined state.innerBinding, which is the id of a ClassExpression. Surprisingly the check passes for null state.innerBinding because (undefined as Binding)?.identifier !== null. This is fixed in 055933c.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented May 3, 2022

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

@JLHwung JLHwung marked this pull request as ready for review May 4, 2022
@@ -28,6 +28,10 @@ function sortFieldNames(fields, type) {
});
}

function defaultExpression(field) {
return Array.isArray(field.default) ? "[]" : JSON.stringify(field.default);
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to special-case arrays?

Copy link
Contributor Author

@JLHwung JLHwung May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the behaviour of builder.js:

arg = Array.isArray(field.default) ? [] : field.default;

I will check if we have weird field.default.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had that check to avoid sharing the same [] reference every time, but since it's now a new [] value for every call it's always a different one.

const node: t.ArrayExpression = {
type: "ArrayExpression",
elements,
};
validateNode(node);
return node;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To generate slightly smaller code, we could modify validateNode like this:

function validateNode<N extends t.Node>(node: N): N {
  // todo: because keys not in BUILDER_KEYS are not validated - this actually allows invalid nodes in some cases
  const keys = BUILDER_KEYS[node.type];
  for (const key of keys) {
    validate(node, key, node[key]);
  }
  return node;
}

and then do

export function arrayExpression(
     elements: Array<null | t.Expression | t.SpreadElement> = [],
) {
  return validateNode<t.ArrayExpression>({
    type: "ArrayExpression",
    elements,
  });
}

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 label May 6, 2022
}function ${formatedBuilderNameLocal}(${defArgs.join(
", "
)}): t.${type} { return builder.apply("${type}", arguments); }\n`;
}function ${formatedBuilderNameLocal}(${defArgs.join(", ")}): t.${type} {`;
Copy link
Contributor Author

@JLHwung JLHwung May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type annotation is preserved because we skip validateNode for nodes with empty builder keys.

packages/babel-types/scripts/generators/builders.js Outdated Show resolved Hide resolved
}function ${formatedBuilderNameLocal}(${defArgs.join(", ")}): t.${type} {`;

const nodeObjectExpression = `{\n${objectFields
.map(([k, v]) => (k === v ? ` ${k},` : ` ${k}: ${v},`))
Copy link
Member

@jridgewell jridgewell May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: since these just come from local param names, can't we make it always match?

Copy link
Contributor Author

@JLHwung JLHwung May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the argument name match the AST field name. The exceptions are non-valid binding identifier names, such as CallExpression's arguments and ClassMethod's static.

Copy link
Member

@jridgewell jridgewell May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh

@JLHwung JLHwung merged commit c6e68c7 into babel:main May 9, 2022
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Internal 🏠
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants