From 26b4e0909ef8ec3a45b08f2a475124ed83defdac Mon Sep 17 00:00:00 2001
From: Ben Newman <ben@benjamn.com>
Date: Thu, 8 Dec 2016 10:16:48 -0500
Subject: [PATCH] Add (and fix) failing test of function parameter bindings in
 a catch block (#4880)

* Add failing test of function parameter bindings in a catch block.

This test can be run in isolation via the following command:

  TEST_GREP='block-scoping.*function in catch' make test-only

This test fails because BlockScoping#getLetReferences accidentally
considers the parameters of the function declaration as let bindings in
the catch scope. When the name of the catch parameter is the same as one
of the function's parameter names, the function declaration will be
unnecessarily wrapped to isolate its parameters from the outer scope.

While the extra wrapping may not seem harmful in this case, this behavior
is a symptom of a deeper problem that causes very subtle bugs in transform
code involving catch parameters and function declarations. This test case
was just the simplest example I could find to demonstrate the problem.

I have a proposed fix for this problem that I will push as soon as the
tests fail for this commit.

* Make BlockScoping#getLetReferences ignore function parameters.
---
 .../src/index.js                                           | 6 +++++-
 .../test/fixtures/general/function-in-catch/actual.js      | 7 +++++++
 .../test/fixtures/general/function-in-catch/expected.js    | 7 +++++++
 3 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/actual.js
 create mode 100644 packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/expected.js

diff --git a/packages/babel-plugin-transform-es2015-block-scoping/src/index.js b/packages/babel-plugin-transform-es2015-block-scoping/src/index.js
index 6f21235196..eb27618ae3 100644
--- a/packages/babel-plugin-transform-es2015-block-scoping/src/index.js
+++ b/packages/babel-plugin-transform-es2015-block-scoping/src/index.js
@@ -524,7 +524,11 @@ class BlockScoping {
     //
     for (let i = 0; i < declarators.length; i++) {
       let declar = declarators[i];
-      let keys = t.getBindingIdentifiers(declar);
+      // Passing true as the third argument causes t.getBindingIdentifiers
+      // to return only the *outer* binding identifiers of this
+      // declaration, rather than (for example) mistakenly including the
+      // parameters of a function declaration. Fixes #4880.
+      let keys = t.getBindingIdentifiers(declar, false, true);
       extend(this.letReferences, keys);
       this.hasLetReferences = true;
     }
diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/actual.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/actual.js
new file mode 100644
index 0000000000..c38a4caa3a
--- /dev/null
+++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/actual.js
@@ -0,0 +1,7 @@
+try {
+  foo();
+} catch (x) {
+  function harmless(x) {
+    return x;
+  }
+}
diff --git a/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/expected.js b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/expected.js
new file mode 100644
index 0000000000..9403930476
--- /dev/null
+++ b/packages/babel-plugin-transform-es2015-block-scoping/test/fixtures/general/function-in-catch/expected.js
@@ -0,0 +1,7 @@
+try {
+  foo();
+} catch (x) {
+  var harmless = function (x) {
+    return x;
+  };
+}