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

Disallow duplicated AST nodes #11807

Merged
merged 4 commits into from Jul 14, 2020
Merged

Disallow duplicated AST nodes #11807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5da54d9
chore: enable checkDuplicatedNodes
JLHwung Jul 7, 2020
a9a4bc8
Fix reused nodes - part 3
JLHwung Jul 8, 2020
b7f9cf5
chore: ignore vscode configs
JLHwung Jul 8, 2020
1e796b3
address review comments
JLHwung Jul 11, 2020
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Expand Up @@ -72,3 +72,4 @@ packages/babel-standalone/babel.min.js
/eslint/*/node_modules
/eslint/*/LICENSE
!/packages/babel-eslint-plugin/LICENSE
/.vscode
2 changes: 1 addition & 1 deletion packages/babel-helper-builder-react-jsx-experimental/src/index.js
View file
Open in desktop
Expand Up @@ -457,7 +457,7 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
}

return makeTrace(
state.fileNameIdentifier,
t.cloneNode(state.fileNameIdentifier),
location.start.line,
location.start.column,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/babel-helper-create-class-features-plugin/src/decorators.js
View file
Open in desktop
Expand Up @@ -137,7 +137,7 @@ export function buildDecoratedClass(ref, path, elements, file) {
let replacement = template.expression.ast`
${addDecorateHelper(file)}(
${classDecorators || t.nullLiteral()},
function (${initializeId}, ${superClass ? superId : null}) {
function (${initializeId}, ${superClass ? t.cloneNode(superId) : null}) {
${node}
return { F: ${t.cloneNode(node.id)}, d: ${definitions} };
},
Expand All @@ -158,7 +158,7 @@ export function buildDecoratedClass(ref, path, elements, file) {
}

return {
instanceNodes: [template.statement.ast`${initializeId}(this)`],
instanceNodes: [template.statement.ast`${t.cloneNode(initializeId)}(this)`],
wrapClass(path) {
path.replaceWith(replacement);
return path.get(classPathDesc);
Expand Down
23 changes: 12 additions & 11 deletions packages/babel-helper-create-class-features-plugin/src/fields.js
View file
Open in desktop
Expand Up @@ -45,8 +45,9 @@ export function buildPrivateNamesNodes(privateNamesMap, loose, state) {
// In spec mode, only instance fields need a "private name" initializer
// because static fields are directly assigned to a variable in the
// buildPrivateStaticFieldInitSpec function.
const { id, static: isStatic, method: isMethod, getId, setId } = value;
const { static: isStatic, method: isMethod, getId, setId } = value;
const isAccessor = getId || setId;
const id = t.cloneNode(value.id);
if (loose) {
initNodes.push(
template.statement.ast`
Expand Down Expand Up @@ -157,7 +158,7 @@ const privateInVisitor = privateNameVisitorFactory({
if (loose) {
const { id } = privateNamesMap.get(name);
path.replaceWith(template.expression.ast`
Object.prototype.hasOwnProperty.call(${right}, ${id})
Object.prototype.hasOwnProperty.call(${right}, ${t.cloneNode(id)})
`);
return;
}
Expand All @@ -169,7 +170,7 @@ const privateInVisitor = privateNameVisitorFactory({
return;
}

path.replaceWith(template.expression.ast`${id}.has(${right})`);
path.replaceWith(template.expression.ast`${t.cloneNode(id)}.has(${right})`);
},
});

Expand Down Expand Up @@ -327,8 +328,8 @@ const privateNameHandlerLoose = {

return template.expression`BASE(REF, PROP)[PROP]`({
BASE: file.addHelper("classPrivateFieldLooseBase"),
REF: object,
PROP: privateNamesMap.get(name).id,
REF: t.cloneNode(object),
PROP: t.cloneNode(privateNamesMap.get(name).id),
});
},

Expand Down Expand Up @@ -387,7 +388,7 @@ function buildPrivateFieldInitLoose(ref, prop, privateNamesMap) {
const value = prop.node.value || prop.scope.buildUndefinedNode();

return template.statement.ast`
Object.defineProperty(${ref}, ${id}, {
Object.defineProperty(${ref}, ${t.cloneNode(id)}, {
// configurable is false by default
// enumerable is false by default
writable: true,
Expand All @@ -400,7 +401,7 @@ function buildPrivateInstanceFieldInitSpec(ref, prop, privateNamesMap) {
const { id } = privateNamesMap.get(prop.node.key.id.name);
const value = prop.node.value || prop.scope.buildUndefinedNode();

return template.statement.ast`${id}.set(${ref}, {
return template.statement.ast`${t.cloneNode(id)}.set(${ref}, {
// configurable is always false for private elements
// enumerable is always false for private elements
writable: true,
Expand All @@ -422,7 +423,7 @@ function buildPrivateStaticFieldInitSpec(prop, privateNamesMap) {
});

return template.statement.ast`
var ${id.name} = {
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
// writable is false by default
Expand All @@ -434,7 +435,7 @@ function buildPrivateStaticFieldInitSpec(prop, privateNamesMap) {

const value = prop.node.value || prop.scope.buildUndefinedNode();
return template.statement.ast`
var ${id} = {
var ${t.cloneNode(id)} = {
// configurable is false by default
// enumerable is false by default
writable: true,
Expand Down Expand Up @@ -603,14 +604,14 @@ function buildPrivateMethodDeclaration(prop, privateNamesMap, loose = false) {
if (isStatic && !loose) {
return t.variableDeclaration("var", [
t.variableDeclarator(
id,
t.cloneNode(id),
t.functionExpression(id, params, body, generator, async),
),
]);
}

return t.variableDeclaration("var", [
t.variableDeclarator(methodId, methodValue),
t.variableDeclarator(t.cloneNode(methodId), methodValue),
]);
}

Expand Down
3 changes: 2 additions & 1 deletion packages/babel-helper-create-class-features-plugin/src/index.js
View file
Open in desktop
@@ -1,3 +1,4 @@
import { types as t } from "@babel/core";
import nameFunction from "@babel/helper-function-name";
import splitExportDeclaration from "@babel/helper-split-export-declaration";
import {
Expand Down Expand Up @@ -129,7 +130,7 @@ export function createClassFeaturePlugin({
nameFunction(path);
ref = path.scope.generateUidIdentifier("class");
} else {
ref = path.node.id;
ref = t.cloneNode(path.node.id);
}

// NODE: These three functions don't support decorators yet,
Expand Down
8 changes: 7 additions & 1 deletion packages/babel-helper-create-class-features-plugin/src/misc.js
View file
Open in desktop
Expand Up @@ -72,8 +72,14 @@ export function injectInitialization(path, constructor, nodes, renamer) {
if (isDerived) {
const bareSupers = [];
constructor.traverse(findBareSupers, bareSupers);
let isFirst = true;
for (const bareSuper of bareSupers) {
bareSuper.insertAfter(nodes);
if (isFirst) {
Copy link
Contributor Author

@JLHwung JLHwung Jul 8, 2020 •
edited

Choose a reason for hiding this comment

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

The test legacy-class-prototype-properties/child-classes-properties is failing if we change these to bareSuper.insertAfter(nodes.map(n => t.cloneNode(n))) unconditionally. The current approach happens to make our test cases happy and also slightly faster because we get rid of redundant clone.

However, I think it reveals an issue which is not caught in our current test cases.

I suggest we address that in a separate PR or even let it stay in the backlog since it is related to legacy decorators only.

I am not familiar with decorators part, suggestions are definitely welcome.

Copy link
Member

Choose a reason for hiding this comment

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

It is probably failing because in the decorators plugin we use a WARNING_CALLS WeakSet to track warnings: if we clone the node we cannot find it anymore in the set.

bareSuper.insertAfter(nodes);
isFirst = false;
} else {
bareSuper.insertAfter(nodes.map(n => t.cloneNode(n)));
}
}
} else {
constructor.get("body").unshiftContainer("body", nodes);
Expand Down
10 changes: 7 additions & 3 deletions packages/babel-helper-member-expression-to-functions/src/index.js
View file
Open in desktop
Expand Up @@ -235,8 +235,12 @@ const handle = {
t.binaryExpression(
"===",
baseNeedsMemoised
? t.assignmentExpression("=", baseRef, startingNode)
: baseRef,
? t.assignmentExpression(
"=",
t.cloneNode(baseRef),
t.cloneNode(startingNode),
)
: t.cloneNode(baseRef),
t.nullLiteral(),
),
t.binaryExpression(
Expand All @@ -263,7 +267,7 @@ const handle = {
false,
true,
),
[context, ...endParent.arguments],
[t.cloneNode(context), ...endParent.arguments],
false,
),
);
Expand Down
4 changes: 3 additions & 1 deletion packages/babel-helper-module-transforms/src/rewrite-live-references.js
View file
Open in desktop
Expand Up @@ -337,7 +337,9 @@ const rewriteReferencesVisitor = {
path
.get("left")
.replaceWith(
t.variableDeclaration("let", [t.variableDeclarator(newLoopId)]),
t.variableDeclaration("let", [
t.variableDeclarator(t.cloneNode(newLoopId)),
]),
);
scope.registerDeclaration(path.get("left"));
}
Expand Down
1 change: 1 addition & 0 deletions packages/babel-helper-transform-fixture-test-runner/src/index.js
View file
Open in desktop
Expand Up @@ -146,6 +146,7 @@ function run(task) {
function getOpts(self) {
const newOpts = merge(
{
ast: true,
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
cwd: path.dirname(self.loc),
filename: self.loc,
filenameRelative: self.filename,
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-async-generator-functions/src/for-await.js
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ export default function (path, { getAsyncIterator }) {
ITERATOR_KEY: scope.generateUidIdentifier("iterator"),
GET_ITERATOR: getAsyncIterator,
OBJECT: node.right,
STEP_VALUE: stepValue,
STEP_VALUE: t.cloneNode(stepValue),
STEP_KEY: stepKey,
});

Expand Down
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-decorators/src/transformer-legacy.js
View file
Open in desktop
Expand Up @@ -160,7 +160,7 @@ function applyTargetDecorators(path, state, decoratedProps) {
acc = acc.concat([
t.assignmentExpression(
"=",
descriptor,
t.cloneNode(descriptor),
t.callExpression(state.addHelper("applyDecoratedDescriptor"), [
t.cloneNode(target),
t.cloneNode(property),
Expand Down
4 changes: 2 additions & 2 deletions packages/babel-plugin-proposal-function-bind/src/index.js
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@ export default declare(api => {

function getTempId(scope) {
let id = scope.path.getData("functionBind");
if (id) return id;
if (id) return t.cloneNode(id);

id = scope.generateDeclaredUidIdentifier("context");
return scope.path.setData("functionBind", id);
Expand Down Expand Up @@ -35,7 +35,7 @@ export default declare(api => {
bind.callee.object,
);
}
return tempId;
return t.cloneNode(tempId);
}

return {
Expand Down
16 changes: 10 additions & 6 deletions packages/babel-plugin-proposal-partial-application/src/index.js
View file
Open in desktop
Expand Up @@ -97,27 +97,27 @@ export default declare(api => {
"=",
t.cloneNode(functionLVal),
t.memberExpression(
receiverLVal,
t.cloneNode(receiverLVal),
node.callee.property,
false,
false,
),
),
...argsInitializers,
t.functionExpression(
node.callee.property,
t.cloneNode(node.callee.property),
placeholdersParams,
t.blockStatement(
[
t.returnStatement(
t.callExpression(
t.memberExpression(
functionLVal,
t.cloneNode(functionLVal),
t.identifier("call"),
false,
false,
),
[receiverLVal, ...args],
[t.cloneNode(receiverLVal), ...args],
),
),
],
Expand All @@ -132,10 +132,14 @@ export default declare(api => {
t.assignmentExpression("=", t.cloneNode(functionLVal), node.callee),
...argsInitializers,
t.functionExpression(
node.callee,
t.cloneNode(node.callee),
placeholdersParams,
t.blockStatement(
[t.returnStatement(t.callExpression(functionLVal, args))],
[
t.returnStatement(
t.callExpression(t.cloneNode(functionLVal), args),
),
],
[],
),
false,
Expand Down
4 changes: 2 additions & 2 deletions packages/babel-plugin-proposal-pipeline-operator/src/buildOptimizedSequenceExpression.js
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@ const buildOptimizedSequenceExpression = ({ assign, call, path }) => {

call.callee = evalSequence;

path.scope.push({ id: placeholderNode });
path.scope.push({ id: t.cloneNode(placeholderNode) });

return t.sequenceExpression([assign, call]);
}
Expand All @@ -40,7 +40,7 @@ const buildOptimizedSequenceExpression = ({ assign, call, path }) => {
return t.sequenceExpression([pipelineLeft, calledExpression.body]);
}

path.scope.push({ id: placeholderNode });
path.scope.push({ id: t.cloneNode(placeholderNode) });

if (param) {
path.get("right").scope.rename(param.name, placeholderNode.name);
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-plugin-proposal-pipeline-operator/src/smartVisitor.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import { types as t } from "@babel/core";

const updateTopicReferenceVisitor = {
PipelinePrimaryTopicReference(path) {
path.replaceWith(this.topicId);
path.replaceWith(t.cloneNode(this.topicId));
},
PipelineTopicExpression(path) {
path.skip();
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-plugin-transform-block-scoping/src/index.js
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ export default declare((api, opts) => {
const decl = node.declarations[i];
const assign = t.assignmentExpression(
"=",
decl.id,
t.cloneNode(decl.id),
decl.init || scope.buildUndefinedNode(),
);
assign._ignoreBlockScopingTDZ = true;
Expand Down
8 changes: 4 additions & 4 deletions packages/babel-plugin-transform-modules-amd/src/index.js
View file
Open in desktop
Expand Up @@ -71,8 +71,8 @@ export default declare((api, options) => {
new Promise((${resolveId}, ${rejectId}) =>
${requireId}(
[${getImportSource(t, path.node)}],
imported => ${resolveId}(${result}),
${rejectId}
imported => ${t.cloneNode(resolveId)}(${result}),
${t.cloneNode(rejectId)}
)
)`,
);
Expand All @@ -84,7 +84,7 @@ export default declare((api, options) => {
if (requireId) {
injectWrapper(
path,
buildAnonymousWrapper({ REQUIRE: requireId }),
buildAnonymousWrapper({ REQUIRE: t.cloneNode(requireId) }),
);
}
return;
Expand All @@ -94,7 +94,7 @@ export default declare((api, options) => {
const importNames = [];
if (requireId) {
amdArgs.push(t.stringLiteral("require"));
importNames.push(requireId);
importNames.push(t.cloneNode(requireId));
}

let moduleName = getModuleName(this.file.opts, options);
Expand Down