Add workaround for Safari for loop lexical scope bug #567
Conversation
Safari raises a syntax error for a `let` or `const` declaration in a
`for` loop initalization that shadows a parent function's parameter:
```js
function a(b) {
for (let b;;);
}
```
This is valid code, but Safari throws a syntax error. The bug has been
[reported](https://bugs.webkit.org/show_bug.cgi?id=171041) and since
[fixed](https://trac.webkit.org/changeset/217200/webkit/trunk/Source) in
WebKit, so future versions of Safari will not have this problem.
This modifies the scope tracker's `canUseInReferencedScopes` method to
detect cases where a rename would trigger this bug and use a different
name.
Fixes babel#559
Codecov Report
@@ Coverage Diff @@
## master #567 +/- ##
==========================================
+ Coverage 82.93% 82.99% +0.05%
==========================================
Files 41 41
Lines 2743 2758 +15
Branches 958 967 +9
==========================================
+ Hits 2275 2289 +14
Misses 282 282
- Partials 186 187 +1
Continue to review full report at Codecov.
|
| (binding.path.parentPath.parent.type === "ForOfStatement" && | ||
| binding.path.parentPath.key === "left"); | ||
| if (isBlockScoped && isForLoopDeclaration) { | ||
| const functionParentScope = binding.scope.getFunctionParent(); |
There was a problem hiding this comment.
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;;);There was a problem hiding this comment.
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.
| (binding.path.parentPath.parent.type === "ForInStatement" && | ||
| binding.path.parentPath.key === "left") || | ||
| (binding.path.parentPath.parent.type === "ForOfStatement" && | ||
| binding.path.parentPath.key === "left"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Thanks for the PR. It looks great. I have added some review inline. I'll try this out tomorrow. |
|
Any eta on this PR going in and being released? Old babili releases have some breaking bugs and this is blocking us from upgrading. |
|
I just encountered this one as well :) |
Safari raises a syntax error for a
letorconstdeclaration in aforloop initalization that shadows a parent function's parameter:This is valid code, but Safari throws a syntax error. The bug has been reported and since fixed in WebKit, so future versions of Safari will not have this problem.
This modifies the scope tracker's
canUseInReferencedScopesmethod to detect cases where a rename would trigger this bug and use a different name.Fixes #559
This is my first time dealing with Babel internals, so please let me know if there's a better way to do this!