Improve getBindingIdentifiers (#16544)

JLHwung · web-flow · commit 07bd0003cbda · 2024-06-04T05:06:21.000-04:00
* Add more getBindingIdentifiers tests

* fix: do not return class id from getOuterBindingId

* update expression should not introduce new bindings

* fix: register class expression binding from its id node

* defer the fix to Babel 8

* remove unused FlowIgnore comment
diff --git a/packages/babel-traverse/src/scope/index.ts b/packages/babel-traverse/src/scope/index.ts
@@ -369,7 +369,7 @@ const collectorVisitor: Visitor<CollectVisitorState> = {
       // @ts-expect-error Fixme: document symbol ast properties
       !path.get("id").node[NOT_LOCAL_BINDING]
     ) {
-      path.scope.registerBinding("local", path);
+      path.scope.registerBinding("local", path.get("id"), path);
     }
   },
   TSTypeAnnotation(path) {
diff --git a/packages/babel-types/src/retrievers/getBindingIdentifiers.ts b/packages/babel-types/src/retrievers/getBindingIdentifiers.ts
@@ -1,12 +1,14 @@
 import {
   isExportDeclaration,
   isIdentifier,
+  isClassExpression,
   isDeclaration,
   isFunctionDeclaration,
   isFunctionExpression,
   isExportAllDeclaration,
   isAssignmentExpression,
   isUnaryExpression,
+  isUpdateExpression,
 } from "../validators/generated/index.ts";
 import type * as t from "../index.ts";
 
@@ -51,20 +53,18 @@ function getBindingIdentifiers(
 
     if (
       newBindingsOnly &&
-      // These two nodes do not introduce _new_ bindings, but they are included
+      // These nodes do not introduce _new_ bindings, but they are included
       // in getBindingIdentifiers.keys for backwards compatibility.
       // TODO(@nicolo-ribaudo): Check if we can remove them from .keys in a
       // backward-compatible way, and if not what we need to do to remove them
       // in Babel 8.
-      (isAssignmentExpression(id) || isUnaryExpression(id))
+      (isAssignmentExpression(id) ||
+        isUnaryExpression(id) ||
+        isUpdateExpression(id))
     ) {
       continue;
     }
 
-    const keys =
-      // @ts-expect-error getBindingIdentifiers.keys do not cover all AST types
-      getBindingIdentifiers.keys[id.type];
-
     if (isIdentifier(id)) {
       if (duplicates) {
         const _ids = (ids[id.name] = ids[id.name] || []);
@@ -88,11 +88,18 @@ function getBindingIdentifiers(
         continue;
       }
 
-      if (isFunctionExpression(id)) {
+      if (
+        isFunctionExpression(id) ||
+        (process.env.BABEL_8_BREAKING && isClassExpression(id))
+      ) {
         continue;
       }
     }
 
+    const keys =
+      // @ts-expect-error getBindingIdentifiers.keys do not cover all AST types
+      getBindingIdentifiers.keys[id.type];
+
     if (keys) {
       for (let i = 0; i < keys.length; i++) {
         const key = keys[i];
@@ -105,8 +112,6 @@ function getBindingIdentifiers(
       }
     }
   }
-
-  // $FlowIssue Object.create() seems broken
   return ids;
 }
 
diff --git a/packages/babel-types/test/retrievers.js b/packages/babel-types/test/retrievers.js
@@ -1,3 +1,7 @@
+import {
+  describeBabel7,
+  describeBabel8,
+} from "../../../scripts/repo-utils/index.cjs";
 import * as t from "../lib/index.js";
 import { parse } from "@babel/parser";
 
@@ -15,8 +19,28 @@ describe("retrievers", function () {
       ],
       [
         "function declarations",
-        getBody("var foo = 1; function bar() { var baz = 2; }"),
-        ["bar", "foo"],
+        getBody("function bar() { var baz = 2; }"),
+        ["bar"],
+      ],
+      [
+        "function declarations with parameters",
+        getBody(
+          "function f(a, { b }, c = 1, [{ _: [ d ], ...e }]) { var baz = 2; }",
+        ),
+        ["f", "a", "c", "b", "e", "d"],
+      ],
+      [
+        "function expressions with parameters",
+        getBody(
+          "(function f(a, { b }, c = 1, [{ _: [ d ], ...e }]) { var baz = 2; })",
+        )[0].expression,
+        ["f", "a", "c", "b", "e", "d"],
+      ],
+      ["class declarations", getBody("class C { a(b) { let c } }")[0], ["C"]],
+      [
+        "class expressions",
+        getBody("(class C { a(b) { let c } })")[0].expression,
+        ["C"],
       ],
       [
         "object methods",
@@ -33,11 +57,28 @@ describe("retrievers", function () {
         getBody("(class { #a(b) { let c } })")[0].expression.body.body,
         ["b"],
       ],
+      [
+        "for-in statement",
+        getBody("for ([{ _: [ d ], ...e }] in rhs);"),
+        ["e", "d"],
+      ],
+      [
+        "for-of statement",
+        getBody("for ([{ _: [ d ], ...e }] of rhs);"),
+        ["e", "d"],
+      ],
+      ["catch clause", getBody("try { } catch (e) {}")[0].handler, ["e"]],
+      ["labeled statement", getBody("label: x"), ["label"]],
       [
         "export named declarations",
         getBody("export const foo = 'foo';"),
         ["foo"],
       ],
+      [
+        "export function declarations",
+        getBody("export function foo() {}"),
+        ["foo"],
+      ],
       [
         "export default class declarations",
         getBody("export default class foo {}"),
@@ -69,11 +110,165 @@ describe("retrievers", function () {
         getBody("var [ a, ...{ b, ...c } ] = {}"),
         ["a", "b", "c"],
       ],
+      ["unary expression", getBody("void x")[0].expression, ["x"]],
       ["update expression", getBody("++x")[0].expression, ["x"]],
       ["assignment expression", getBody("x ??= 1")[0].expression, ["x"]],
     ])("%s", (_, program, bindingNames) => {
       const ids = t.getBindingIdentifiers(program);
       expect(Object.keys(ids)).toEqual(bindingNames);
     });
   });
+  describe("getBindingIdentifiers(%, /* duplicates */ true)", function () {
+    it.each([
+      ["variable declarations", getBody("var a = 1, a = 2"), { a: 2 }],
+      [
+        "function declarations with parameters",
+        getBody("function f(f) { var f = 1; }"),
+        { f: 2 },
+      ],
+    ])("%s", (_, program, expected) => {
+      const ids = t.getBindingIdentifiers(program, true);
+      for (const name of Object.keys(ids)) {
+        ids[name] = Array.isArray(ids[name]) ? ids[name].length : 1;
+      }
+      expect(ids).toEqual(expected);
+    });
+  });
+  describe("getOuterBindingIdentifiers", function () {
+    it.each([
+      [
+        "variable declarations",
+        getBody("var a = 1; let b = 2; const c = 3;"),
+        ["a", "b", "c"],
+      ],
+      [
+        "function declarations",
+        getBody("function bar() { var baz = 2; }"),
+        ["bar"],
+      ],
+      [
+        "function declarations with parameters",
+        getBody(
+          "function f(a, { b }, c = 1, [{ _: [ d ], ...e }]) { var baz = 2; }",
+        ),
+        ["f"],
+      ],
+      [
+        "function expressions with parameters",
+        getBody(
+          "(function f(a, { b }, c = 1, [{ _: [ d ], ...e }]) { var baz = 2; })",
+        )[0].expression,
+        [],
+      ],
+      ["class declarations", getBody("class C { a(b) { let c } }")[0], ["C"]],
+      [
+        "object methods",
+        getBody("({ a(b) { let c } })")[0].expression.properties[0],
+        ["b"],
+      ],
+      [
+        "class methods",
+        getBody("(class { a(b) { let c } })")[0].expression.body.body,
+        ["b"],
+      ],
+      [
+        "class private methods",
+        getBody("(class { #a(b) { let c } })")[0].expression.body.body,
+        ["b"],
+      ],
+      [
+        "for-in statement",
+        getBody("for ([{ _: [ d ], ...e }] in rhs);"),
+        ["e", "d"],
+      ],
+      [
+        "for-of statement",
+        getBody("for ([{ _: [ d ], ...e }] of rhs);"),
+        ["e", "d"],
+      ],
+      ["catch clause", getBody("try { } catch (e) {}")[0].handler, ["e"]],
+      ["labeled statement", getBody("label: x"), ["label"]],
+      [
+        "export named declarations",
+        getBody("export const foo = 'foo';"),
+        ["foo"],
+      ],
+      [
+        "export function declarations",
+        getBody("export function foo() {}"),
+        ["foo"],
+      ],
+      [
+        "export default class declarations",
+        getBody("export default class foo {}"),
+        ["foo"],
+      ],
+      [
+        "export default referenced identifiers",
+        getBody("export default foo"),
+        [],
+      ],
+      ["export all declarations", getBody("export * from 'x'"), []],
+      [
+        "export all as namespace declarations",
+        getBody("export * as ns from 'x'"),
+        [], // exported bindings are not associated with declarations
+      ],
+      [
+        "export namespace specifiers",
+        getBody("export * as ns from 'x'")[0].specifiers,
+        ["ns"],
+      ],
+      [
+        "object patterns",
+        getBody("const { a, b: { ...c } = { d } } = {}"),
+        ["a", "c"],
+      ],
+      [
+        "array patterns",
+        getBody("var [ a, ...{ b, ...c } ] = {}"),
+        ["a", "b", "c"],
+      ],
+      ["unary expression", getBody("void x")[0].expression, ["x"]],
+      ["update expression", getBody("++x")[0].expression, ["x"]],
+      ["assignment expression", getBody("x ??= 1")[0].expression, ["x"]],
+    ])("%s", (_, program, bindingNames) => {
+      const ids = t.getOuterBindingIdentifiers(program);
+      expect(Object.keys(ids)).toEqual(bindingNames);
+    });
+  });
+  describeBabel7("getOuterBindingIdentifiers - Babel 7", function () {
+    it.each([
+      [
+        "class expressions",
+        getBody("(class C { a(b) { let c } })")[0].expression,
+        ["C"],
+      ],
+    ])("%s", (_, program, bindingNames) => {
+      const ids = t.getOuterBindingIdentifiers(program);
+      expect(Object.keys(ids)).toEqual(bindingNames);
+    });
+  });
+  describeBabel8("getOuterBindingIdentifiers - Babel 8", function () {
+    it.each([
+      [
+        "class expressions",
+        getBody("(class C { a(b) { let c } })")[0].expression,
+        [],
+      ],
+    ])("%s", (_, program, bindingNames) => {
+      const ids = t.getOuterBindingIdentifiers(program);
+      expect(Object.keys(ids)).toEqual(bindingNames);
+    });
+  });
+  describe("getBindingIdentifiers(%, false, false, /* newBindingsOnly */ true)", function () {
+    it.each([
+      ["unary expression", getBody("void x")[0].expression, []],
+      ["update expression", getBody("++x")[0].expression, []],
+      ["assignment expression", getBody("x ??= 1")[0].expression, []],
+    ])("%s", (_, program, bindingNames) => {
+      const ids = t.getBindingIdentifiers(program, false, false, true);
+      expect(Object.keys(ids)).toEqual(bindingNames);
+    });
+  });
 });

Original file line number	Diff line number	Diff line change
`@@ -369,7 +369,7 @@ const collectorVisitor: Visitor<CollectVisitorState> = {`
`369`	`369`	`// @ts-expect-error Fixme: document symbol ast properties`
`370`	`370`	`!path.get("id").node[NOT_LOCAL_BINDING]`
`371`	`371`	`) {`
`372`		`- path.scope.registerBinding("local", path);`
	`372`	`+ path.scope.registerBinding("local", path.get("id"), path);`
`373`	`373`	`}`
`374`	`374`	`},`
`375`	`375`	`TSTypeAnnotation(path) {`