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.

Sign up for GitHub

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

Jump to bottom

Update scope info after destructuring transform #14494

Merged
merged 8 commits into from May 4, 2022
Merged

Update scope info after destructuring transform #14494

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e53065d
Introduce test suite for #14438
peey Apr 26, 2022
d40dce6
Attempt to fix #14438
peey Apr 26, 2022
d84141c
Improve tests
peey Apr 26, 2022
84d6c4a
Fix #14438
peey Apr 26, 2022
d503eb7
Take care of comments in JLHwung's review
peey Apr 26, 2022
ce4a6bf
Clear traverse scope cache for through testing
peey Apr 26, 2022
8d1bb65
Move test to runnable plugin
peey Apr 26, 2022
999fd51
LVal.get('id') -> LVal.getBindingIdentifiers()
peey Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/babel-plugin-transform-destructuring/src/index.ts
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@ export default declare((api, options) => {
// top-level statement.
path.replaceWith(declaration.node);
path.insertAfter(t.exportNamedDeclaration(null, specifiers));
path.scope.crawl();
},

ForXStatement(path) {
Expand Down Expand Up @@ -82,6 +83,7 @@ export default declare((api, options) => {
t.expressionStatement(t.assignmentExpression("=", left, temp)),
);

path.scope.crawl();
peey marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand Down Expand Up @@ -115,6 +117,7 @@ export default declare((api, options) => {
const block = node.body;
// @ts-expect-error: ensureBlock ensures that node.body is a BlockStatement
block.body = nodes.concat(block.body);
path.scope.crawl();
peey marked this conversation as resolved.
Show resolved Hide resolved
},

CatchClause({ node, scope }) {
Expand Down
1 change: 1 addition & 0 deletions packages/babel-plugin-transform-destructuring/src/util.ts
View file
Open in desktop
Expand Up @@ -637,6 +637,7 @@ export function convertVariableDeclaration(
} else {
path.replaceWithMultiple(nodesOut);
}
path.scope.crawl();
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Is Line 625 - 633 still required now that we crawl the scope?

Copy link
Contributor Author

@peey peey Apr 26, 2022 •
edited

Choose a reason for hiding this comment

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

I don't understand the plugin source very well but removing them doesn't cause any tests to fail. I'll remove them, let me know if you think we should keep them.

peey marked this conversation as resolved.
Show resolved Hide resolved
}

export function convertAssignmentExpression(
Expand Down
70 changes: 70 additions & 0 deletions packages/babel-plugin-transform-destructuring/test/api.js
View file
Open in desktop
@@ -0,0 +1,70 @@
import { parse } from "@babel/parser";
import { transformFromAstSync } from "@babel/core";
import _traverse from "@babel/traverse";
const traverse = _traverse.default;

import path from "path";
import { fileURLToPath } from "url";
const __dirname = path.dirname(fileURLToPath(import.meta.url));

import glob from "glob";
import { promises as fs } from "fs";

const files = await new Promise((resolve, reject) => {
glob(
path.resolve(__dirname, "fixtures/**/@(exec|input).js"),
function (err, files) {
if (err) reject(err);
else resolve(files);
},
);
});

describe("regression-14438", function () {
describe.each(files)("fixture: %s", file => {
let src, originalAst, transformed;

beforeEach(async () => {
src = await fs.readFile(file, "utf-8");

originalAst = parse(src, {
allowImportExportEverywhere: true,
allowAwaitOutsideFunction: true,
allowReturnOutsideFunction: true,
allowSuperOutsideMethod: true,
allowUndeclaredExports: true,
errorRecovery: true,
attachComment: false,
createParenthesizedExpressions: true,
});

transformed = transformFromAstSync(originalAst, src, {
plugins: [
[
"@babel/plugin-transform-destructuring",
{ useBuiltIns: true, loose: true },
],
],
configFile: false,
ast: true,
code: true,
});
});
peey marked this conversation as resolved.
Show resolved Hide resolved

it("has correct scope information after transform", function () {
if (transformed.ast) {
// exclude transformation failures: they're tested by the other cases
traverse(transformed.ast, {
VariableDeclarator(path) {
const b = path.scope.getBinding(path.get("id").node.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Further improvement idea: To make it a universal check, we should loop through Object.keys(path.getBindingIdentifiers()). The current check is ok for transform-destructuring since path.get("id") is always an identifier after transformed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but if the bindings don't exist, would that be returned in path.getBindingIdentifiers()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the AST level, instead of LHS of the variable declarator we can also go for all identifiers that are present. But I couldn't think of a way to exclude globals or member expressions (e.g. slice in arr.slice or Array global) which definitely won't have scope bindings.

Copy link
Contributor

Choose a reason for hiding this comment

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

path.getBindingIdentifiers() extracts ids from the AST, it does not access the scope info.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the AST level, instead of LHS of the variable declarator we can also go for all identifiers that are present.

hasBinding will respect globals. path.isReferencedIdentifier() can exclude referenced identifiers. I think we can start from VariableDeclarator / Function params only.

The most strict check will be to compare the scope info after transform against a new scope created from scratch (without traverse cache), but I guess it will fail many, if not most current tests.

if (!b) {
peey marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(
`No original binding for ${path.get("id").node.name}`,
);
}
},
});
}
});
});
});