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

Fixed generateUid creating clashing ids after scope re-crawling #11375

Merged
merged 1 commit into from Apr 7, 2020
Merged

Fixed generateUid creating clashing ids after scope re-crawling #11375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 41 additions & 40 deletions packages/babel-traverse/src/scope/index.js
View file
Open in desktop
Expand Up @@ -155,6 +155,7 @@ const collectorVisitor = {
For(path) {
for (const key of (t.FOR_INIT_KEYS: Array)) {
const declar = path.get(key);
// delegate block scope handling to the `BlockScoped` method
if (declar.isVar()) {
const parentScope =
path.scope.getFunctionParent() || path.scope.getProgramParent();
Expand Down Expand Up @@ -253,6 +254,31 @@ const collectorVisitor = {
}
}
},

CatchClause(path) {
path.scope.registerBinding("let", path);
},

Function(path) {
if (
path.isFunctionExpression() &&
path.has("id") &&
!path.get("id").node[t.NOT_LOCAL_BINDING]
) {
path.scope.registerBinding("local", path.get("id"), path);
}

const params: Array<NodePath> = path.get("params");
for (const param of params) {
path.scope.registerBinding("param", param);
}
},

ClassExpression(path) {
if (path.has("id") && !path.get("id").node[t.NOT_LOCAL_BINDING]) {
path.scope.registerBinding("local", path);
}
},
};

let uid = 0;
Expand All @@ -279,6 +305,7 @@ export default class Scope {
this.path = path;

this.labels = new Map();
this.inited = false;
}

/**
Expand Down Expand Up @@ -761,7 +788,10 @@ export default class Scope {
}

init() {
if (!this.references) this.crawl();
Copy link
Member Author

Choose a reason for hiding this comment

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

this check was super implicit and confusing (nested scopes don't even gather references so this object stays always empty for them, but is initialized in .crawl)

if (!this.inited) {
this.inited = true;
this.crawl();
}
}

crawl() {
Expand All @@ -773,50 +803,24 @@ export default class Scope {
this.uids = Object.create(null);
this.data = Object.create(null);

// ForStatement - left, init

if (path.isLoop()) {
for (const key of (t.FOR_INIT_KEYS: Array<string>)) {
const node = path.get(key);
if (node.isBlockScoped()) this.registerBinding(node.node.kind, node);
}
}

// FunctionExpression - id

if (path.isFunctionExpression() && path.has("id")) {
if (!path.get("id").node[t.NOT_LOCAL_BINDING]) {
// TODO: explore removing this as it should be covered by collectorVisitor
Copy link
Member Author

Choose a reason for hiding this comment

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

attempt to remove this has resulted in 14~ failing tests, most of the updated snapshots seemed OK, but some didn't. It requires more investigation and I'm afraid that I won't have time to do this

Copy link
Member

Choose a reason for hiding this comment

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

👍

if (path.isFunction()) {
if (
path.isFunctionExpression() &&
path.has("id") &&
!path.get("id").node[t.NOT_LOCAL_BINDING]
) {
this.registerBinding("local", path.get("id"), path);
}
}

// Class

if (path.isClassExpression() && path.has("id")) {
if (!path.get("id").node[t.NOT_LOCAL_BINDING]) {
this.registerBinding("local", path);
}
}

// Function - params, rest

if (path.isFunction()) {
const params: Array<NodePath> = path.get("params");
for (const param of params) {
this.registerBinding("param", param);
}
}

// CatchClause - param

if (path.isCatchClause()) {
this.registerBinding("let", path);
}

// Program

const parent = this.getProgramParent();
if (parent.crawling) return;
const programParent = this.getProgramParent();
if (programParent.crawling) return;

const state = {
references: [],
Expand All @@ -832,11 +836,8 @@ export default class Scope {
for (const path of state.assignments) {
// register undeclared bindings as globals
const ids = path.getBindingIdentifiers();
let programParent;
for (const name of Object.keys(ids)) {
if (path.scope.getBinding(name)) continue;

programParent = programParent || path.scope.getProgramParent();
programParent.addGlobal(ids[name]);
}

Expand All @@ -850,7 +851,7 @@ export default class Scope {
if (binding) {
binding.reference(ref);
} else {
ref.scope.getProgramParent().addGlobal(ref.node);
programParent.addGlobal(ref.node);
}
}

Expand Down
36 changes: 36 additions & 0 deletions packages/babel-traverse/test/scope.js
View file
Open in desktop
Expand Up @@ -321,6 +321,42 @@ describe("scope", () => {

expect(path.scope.generateUid("jsx")).toBe("_jsx2");
});

test("generateUid collision check after re-crawling (function expression local id)", function() {
const path = getPath("var fn = function _name(){}");

path.scope.crawl();
path.scope.crawl();

expect(path.scope.generateUid("name")).toBe("_name2");
});

test("generateUid collision check after re-crawling (function params)", function() {
const path = getPath("[].map(_unicorn => [_unicorn])");

path.scope.crawl();
path.scope.crawl();

expect(path.scope.generateUid("unicorn")).toBe("_unicorn2");
});

test("generateUid collision check after re-crawling (catch clause)", function() {
const path = getPath("try {} catch (_err) {}");

path.scope.crawl();
path.scope.crawl();

expect(path.scope.generateUid("err")).toBe("_err2");
});

test("generateUid collision check after re-crawling (class expression local id)", function() {
const path = getPath("var C = class _Cls{}");

path.scope.crawl();
path.scope.crawl();

expect(path.scope.generateUid("Cls")).toBe("_Cls2");
});
});

describe("duplicate bindings", () => {
Expand Down