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

Simplify (transpiled) babel-types builder wrappers #13843

Merged
merged 7 commits into from Oct 21, 2021

Conversation

@lightmare
Copy link
Contributor

@lightmare lightmare commented Oct 13, 2021

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

It's a bit hacky, but bear with me. The builder function is super internal, only used from auto-generated code, so I think it can afford having a weird calling convention, such as receiving a string in this.

The end result of this PR is that transpiled babel-standalone ends up containing this:

        function arrayExpression$2(elements) {
          return builder.apply("ArrayExpression", arguments);
        }

instead of this:

        function arrayExpression$2(elements) {
          return builder.apply(void 0, ["ArrayExpression"].concat(Array.prototype.slice.call(arguments)));
        }

@codesandbox
Copy link

@codesandbox codesandbox bot commented Oct 13, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 546fe9a:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Oct 13, 2021

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

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 13, 2021

Related: #13369
I think we can revive #13369 and get rid of arguments spread here. The non-spread version is 40% faster.

@jridgewell
Copy link
Member

@jridgewell jridgewell commented Oct 13, 2021

Why not just pass the (already known, because the call signature is correct) parameters into the builder?

// before
function arrayExpression$2(elements) {
  return builder.apply("ArrayExpression", arguments);
}

// after
function arrayExpression$2(elements) {
  return builder("ArrayExpression", elements);
}

@lightmare
Copy link
Contributor Author

@lightmare lightmare commented Oct 13, 2021

I think we can revive #13369 and get rid of arguments spread here. The non-spread version is 40% faster.

I've just tried transplanting the benchmark from that PR onto this one, and got this:

$ yarn node --predictable babel-types/builders/stringLiteral.mjs 
baseline stringLiteral builder: 5_672_661 ops/sec ±0.65% (0ms)
current stringLiteral builder: 8_521_407 ops/sec ±0.52% (0ms)

Why not just pass the (already known, because the call signature is correct) parameters into the builder?

I don't know the history or importance of that check, but one notable difference is that the current builder function throws if it gets too many arguments. If you only forwarded the ones named in the wrapper, that call-site error would become silent.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Oct 15, 2021

I don't know the history or importance of that check

That check is important because builders don't accept params for every possible property, so it's easy to accidentally pass extra params.

Object.keys(VISITOR_KEYS),
Object.keys(FLIPPED_ALIAS_KEYS),
Object.keys(DEPRECATED_KEYS),
);
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 15, 2021

Choose a reason for hiding this comment

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

Q: what is this change needed for?

Copy link
Contributor Author

@lightmare lightmare Oct 15, 2021

Choose a reason for hiding this comment

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

Not needed, it's insignificant. I just randomly noticed the extra concat because this line was right above the builder function when I was reviewing it in the resulting babel.js.

@jridgewell
Copy link
Member

@jridgewell jridgewell commented Oct 15, 2021

That check is important because builders don't accept params for every possible property, so it's easy to accidentally pass extra params.

We could easily pass arguments.length to the builder implementation, if we only want one error check location. Or, we could move the check into the arrayExpression wrapper itself.

Really, I'm not sure why we need a builder function at all. Why not let arrayExpression generate it's own object? It would probably be faster to do return { type: 'ArrayExpression', elements } instead of return builder("ArrayExpression", elements }, since we wouldn't need to dynamically build the object from the BUILDER_KEYS.

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 15, 2021

Why not let arrayExpression generate it's own object

🙌 for plain object AST nodes, and we can replace startNode in babel parser, too.

we wouldn't need to dynamically build the object from the BUILDER_KEYS

It's a shotgun that t.BUILDER_KEYS can be changed by users. Just like babel/eslint-parser changes VISITOR_KEYS on the fly.

@@ -121,7 +121,7 @@ import type * as t from "../..";
output += `/** @deprecated */
function ${type}(...args: Array<any>): any {
console.trace("The node type ${type} has been renamed to ${newType}");
return builder("${type}", ...args);
return builder.apply("${type}", arguments);
Copy link
Member

@fedeci fedeci Oct 16, 2021

Choose a reason for hiding this comment

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

Should it evaluate the args passed to the function? Otherwise if we want to keep arguments we can remove ...args: Array<any> from the signature.

Suggested change
return builder.apply("${type}", arguments);
return builder.apply("${type}", args);

Copy link
Contributor Author

@lightmare lightmare Oct 16, 2021

Choose a reason for hiding this comment

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

Good catch. The point of this PR is to avoid the unnecessary conversion from arguments to an actual Array (whether by slicing or shovelling), so the answer is: I should remove ...args from the signature as well.

Copy link
Contributor Author

@lightmare lightmare Oct 16, 2021

Choose a reason for hiding this comment

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

I wasn't sure whether I could change the deprecated wrapper function signatures (as these are public). If yes, then perhaps they could be as verbose as the non-deprecated ones, i.e.

`
function ${type}(${generateBuilderArgs(newType).join(", ")}): t.${type} {
`

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 16, 2021

Choose a reason for hiding this comment

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

Yeah it is better: even if they are deprecated they still work so we can provide a good type signature.

...args: Array<any>
): T {
export default function builder<T extends t.Node>(this: T["type"]): T {
const type = this;
const keys = BUILDER_KEYS[type];
Copy link
Member

@jridgewell jridgewell Oct 19, 2021

Choose a reason for hiding this comment

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

Before merging, can we discuss whether mutating BUILD_KEYS is actually an intentional API? For the Babel 8 breaking changes, we could stop doing this and perform much faster node building by specializing each builder function. Eg, t.identifer(name) can be (a little more than) function identifier(name) { return { type: 'Identifier', name } }.

Re: #13843 (comment), #13843 (comment)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Oct 19, 2021

Choose a reason for hiding this comment

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

We support swapping the parser and the generator, but not the other packages.
You can handle custom AST modifications by changing VISITOR_KEYS.

On the other hand, I don't see why someone would want to change BUILDER_KEYS (you can just directly create the node without using the builder function).

Copy link
Contributor

@JLHwung JLHwung Oct 19, 2021

Choose a reason for hiding this comment

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

On the other hand, I don't see why someone would want to change BUILDER_KEYS (you can just directly create the node without using the builder function).

In our codebase we don't mutate BUILDER_KEYS. I think we can expand builder keys in babel-types from Babel 8 and leave a note that the BUILDER_KEYS export is for reference only, it does not change the builder behavior.

On VISITOR_KEYS, I think we can offer an overrideVisit(originalVisit) API for those mutating VISITOR_KEYS: So we can expand VISITOR_KEYS in babel-traverse, too. It should remove the slow object access due to dynamic property access on different AST nodes. TS expands visitor keys, too.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants