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

Add workaround for Safari for loop lexical scope bug #567

Merged
merged 4 commits into from Jun 13, 2017
Merged

Add workaround for Safari for loop lexical scope bug #567

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b61b04
Add workaround for Safari for loop lexical scope bug
btmills Jun 6, 2017
40f0b4c
More idiomatic implementation
btmills Jun 6, 2017
75488d9
More extensive tests
btmills Jun 6, 2017
4665255
Better use path API
btmills Jun 6, 2017
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
63 changes: 63 additions & 0 deletions packages/babel-plugin-minify-mangle-names/__tests__/mangle-names-test.js
View file
Open in desktop
Expand Up @@ -1281,4 +1281,67 @@ describe("mangle-names", () => {
`);
expect(transform(source, { topLevel: true }, "module")).toBe(expected);
});

// Safari raises a syntax error for a `let` or `const` declaration in a `for`
// loop initialization that shadows a parent function's parameter.
// https://github.com/babel/babili/issues/559
// https://bugs.webkit.org/show_bug.cgi?id=171041
// https://trac.webkit.org/changeset/217200/webkit/trunk/Source
describe("Safari for loop lexical scope workaround", () => {
it("should not shadow params in ForStatement.init", () => {
const source = unpad(`
function a(b) {
for (let c;;);
}
`);
const expected = unpad(`
function a(a) {
for (let b;;);
}
`);
expect(transform(source)).toBe(expected);
});

it("should not shadow params in ForInStatement.left", () => {
const source = unpad(`
function a(b) {
for (let c in d);
}
`);
const expected = unpad(`
function a(a) {
for (let b in d);
}
`);
expect(transform(source)).toBe(expected);
});

it("should not shadow params in ForOfStatement.left", () => {
const source = unpad(`
function a(b) {
for (const c of d);
}
`);
const expected = unpad(`
function a(a) {
for (const b of d);
}
`);
expect(transform(source)).toBe(expected);
});

it("should not shadow params in complex ForOfStatement.left", () => {
const source = unpad(`
function a(b) {
for (const [c, d] of e);
}
`);
const expected = unpad(`
function a(a) {
for (const [b, c] of e);
}
`);
expect(transform(source)).toBe(expected);
});
});
});
31 changes: 31 additions & 0 deletions packages/babel-plugin-minify-mangle-names/src/scope-tracker.js
View file
Open in desktop
Expand Up @@ -121,6 +121,37 @@ module.exports = class ScopeTracker {
return false;
}

// Safari raises a syntax error for a `let` or `const` declaration in a
// `for` loop initialization that shadows a parent function's parameter.
// https://github.com/babel/babili/issues/559
// https://bugs.webkit.org/show_bug.cgi?id=171041
// https://trac.webkit.org/changeset/217200/webkit/trunk/Source
const isBlockScoped = binding.kind === "let" || binding.kind === "const";
const isForLoopDeclaration =
(binding.path.parentPath.parent.type === "ForStatement" &&
binding.path.parentPath.key === "init") ||
(binding.path.parentPath.parent.type === "ForInStatement" &&
binding.path.parentPath.key === "left") ||
(binding.path.parentPath.parent.type === "ForOfStatement" &&
binding.path.parentPath.key === "left");
Copy link
Member

Choose a reason for hiding this comment

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

ForIn and ForOf can be combined as ForXStatement and it's better to use path's API to verify by passing the node -

path.isForStatement({ init: varDeclaration.node });
path.isForXStatement({ left: varDeclaration.node });

Wrote something up with a few more corner cases here - http://astexplorer.net/#/gist/1336be60e031cffc194b693ec358074f/c8d813db7e9ff40eb57be5ac807ce16751554fd8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tips! I've updated this PR with what I hope is more idiomatic traversal code. The only intentional change to the logic is ensuring that the parent scope belongs to a function and not Program.

I also added a whole bunch of other tests including the other corner cases you identified.

if (isBlockScoped && isForLoopDeclaration) {
const functionParentScope = binding.scope.getFunctionParent();
Copy link
Member

@boopathi boopathi Jun 6, 2017 •
edited

Choose a reason for hiding this comment

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

thing to note here: getFunctionParent in babel returns either a Function or Program. Is this okay? or de we specifically look for function?

eg:

for (let c;;);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good to know. Safari appears fine with let a; for (let a; false;); - no syntax error there. I found .isFunction() and used that to make sure that we're inside a function scope.

const isIncorrectlyTopLevelInSafari =
binding.scope.parent === functionParentScope;
if (isIncorrectlyTopLevelInSafari) {
const parentFunctionBinding = this.bindings
.get(functionParentScope)
.get(next);
if (parentFunctionBinding) {
const parentFunctionHasParamBinding =
parentFunctionBinding.kind === "param";
if (parentFunctionHasParamBinding) {
return false;
}
}
}
}

for (let i = 0; i < binding.constantViolations.length; i++) {
const violation = binding.constantViolations[i];
if (tracker.hasBindingOrReference(violation.scope, binding, next)) {
Expand Down