Repository: nicoespeon/abracadabra (ts)

Issue:
Make "Convert for to forEach" handle for…of loops
The for loop to foreach refactoring apparently does not work on `for (const entry of entries)` style loops. At least the plugin says it did not detect a valid for-loop.
Would be great to make it work for this style of loops.

Abracadabra version: 3.2.3


Pull request:
Add For...of -> forEach refactoring for arrays
Resolves #92 

I'm new to the code base and AST/babel so let me know if there are some things you'd like me to change.

Patch:
diff --git a/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.ts b/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.ts
index 84b5f818..76e27836 100644
--- a/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.ts
+++ b/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.ts
@@ -21,23 +21,13 @@ async function convertForToForeach(editor: Editor) {
 function updateCode(ast: t.AST, selection: Selection): t.Transformed {
   return t.transformAST(
     ast,
-    createVisitor(selection, (path, accessor, list) => {
+    createVisitor(selection, (path, getParams, list) => {
       const { body } = path.node;
-
-      const item = t.identifier(singular(getListName(list)));
       const forEachBody = t.isBlockStatement(body)
         ? body
         : t.blockStatement([body]);
 
-      replaceListWithItemIn(forEachBody, list, accessor, item, path.scope);
-
-      // After we replaced, we check if there are remaining accessors.
-      const params = isAccessorReferencedIn(forEachBody, accessor)
-        ? [item, accessor]
-        : [item];
-
-      path.replaceWith(t.forEach(list, params, forEachBody));
-
+      path.replaceWith(t.forEach(list, getParams(forEachBody), forEachBody));
       path.stop();
     })
   );
@@ -46,9 +36,11 @@ function updateCode(ast: t.AST, selection: Selection): t.Transformed {
 function createVisitor(
   selection: Selection,
   onMatch: (
-    path: t.NodePath<t.ForStatement>,
-    accessor: t.Identifier,
-    list: List
+    path: t.NodePath<t.ForStatement | t.ForOfStatement>,
+    getParams: (
+      body: t.BlockStatement
+    ) => (t.Identifier | t.ObjectPattern | t.ArrayPattern)[],
+    list: List | t.Expression
   ) => void
 ): t.Visitor {
   return {
@@ -70,11 +62,54 @@ function createVisitor(
       const list = getList(test, init);
       if (!list) return;
 
-      onMatch(path, left, list);
+      onMatch(
+        path,
+        (body) => {
+          const item = t.identifier(singular(getListName(list)));
+          replaceListWithItemIn(body, list, left, item, path.scope);
+
+          // After we replaced, we check if there are remaining accessors.
+          return isAccessorReferencedIn(body, left) ? [item, left] : [item];
+        },
+        list
+      );
+    },
+
+    ForOfStatement(path) {
+      if (!selection.isInsidePath(path)) return;
+
+      const { left, right } = path.node;
+      if (!t.isVariableDeclaration(left)) return;
+      if (!isList(right, path)) return;
+
+      const identifier = getIdentifier(left);
+      if (!identifier) return;
+
+      onMatch(path, () => [identifier], right);
     }
   };
 }
 
+function isList(expression: t.Expression, path: t.NodePath<t.ForOfStatement>) {
+  if (t.isArrayExpression(expression)) return true;
+  if (!t.isIdentifier(expression)) return false;
+  const identifier = expression as t.Identifier;
+  return identifierPointsToArray(identifier.name, path);
+}
+
+function identifierPointsToArray(
+  name: string,
+  path: t.NodePath<t.ForOfStatement>
+) {
+  const binding = path.scope.getBinding(name);
+  if (!binding) return false;
+  const parent = binding.path.parent as t.VariableDeclaration;
+  if (!t.isVariableDeclaration(parent)) return false;
+  if (parent.declarations.length !== 1) return false;
+  const value = parent.declarations[0].init;
+  return t.isArrayExpression(value);
+}
+
 function hasChildWhichMatchesSelection(
   path: t.NodePath,
   selection: Selection
@@ -158,6 +193,17 @@ function getListFromMemberExpression(node: t.Node): List | undefined {
   return object;
 }
 
+function getIdentifier(
+  declaration: t.VariableDeclaration
+): t.Identifier | t.ObjectPattern | t.ArrayPattern | undefined {
+  // for...of doesn't support multiple declarations anyway.
+  if (declaration.declarations.length !== 1) return;
+  const id = declaration.declarations[0].id;
+  if (!t.isIdentifier(id) && !t.isObjectPattern(id) && !t.isArrayPattern(id))
+    return;
+  return id;
+}
+
 function getListName(list: List): string {
   return t.isIdentifier(list) ? list.name : getListName(list.property);
 }


Test patch:
diff --git a/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.test.ts b/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.test.ts
index a65110a9..b268839a 100644
--- a/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.test.ts
+++ b/src/refactorings/convert-for-to-foreach/convert-for-to-foreach.test.ts
@@ -1,8 +1,13 @@
 import { ErrorReason, Code } from "../../editor/editor";
 import { InMemoryEditor } from "../../editor/adapters/in-memory-editor";
 import { testEach } from "../../tests-helpers";
+import * as t from "../../ast";
 
-import { convertForToForeach } from "./convert-for-to-foreach";
+import {
+  canConvertForLoop,
+  convertForToForeach
+} from "./convert-for-to-foreach";
+import { Selection } from "../../editor/selection";
 
 describe("Convert For To Foreach", () => {
   testEach<{ code: Code; expected: Code }>(
@@ -149,6 +154,86 @@ items.forEach(item => {
 }`,
         expected: `this.data[0].items.forEach(item => {
   console.log(item);
+});`
+      },
+      {
+        description: "for-of",
+        code: `const items = ['foo', 'bar', 'baz'];
+
+[cursor]for (const val of items) {
+  console.log(val);
+}`,
+        expected: `const items = ['foo', 'bar', 'baz'];
+
+items.forEach(val => {
+  console.log(val);
+});`
+      },
+      {
+        description: "selected for-of only",
+        code: `const items = ['foo', 'bar', 'baz'];
+
+for (const val of items) {
+  console.log(val);
+}
+
+[cursor]for (const val of items) {
+  console.log(val);
+}`,
+        expected: `const items = ['foo', 'bar', 'baz'];
+
+for (const val of items) {
+  console.log(val);
+}
+
+items.forEach(val => {
+  console.log(val);
+});`
+      },
+      {
+        description: "for-of, with object destructuring",
+        code: `const items = [{name: 'joe', age: 40}, {name: 'danielle', age: 25}, {name: 'jane', age: 50}];
+
+[cursor]for (const {name, age} of items) {
+  console.log(name, age);
+}`,
+        expected: `const items = [{name: 'joe', age: 40}, {name: 'danielle', age: 25}, {name: 'jane', age: 50}];
+
+items.forEach(({name, age}) => {
+  console.log(name, age);
+});`
+      },
+      {
+        description: "for-of, with array destructuring",
+        code: `const items = [[0, 1], [1, 2], [2, 3]];
+
+[cursor]for (const [one, two] of items) {
+  console.log(one, two);
+}`,
+        expected: `const items = [[0, 1], [1, 2], [2, 3]];
+
+items.forEach(([one, two]) => {
+  console.log(one, two);
+});`
+      },
+      {
+        description: "for-of, without block statement",
+        code: `const items = ['foo', 'bar', 'baz'];
+
+[cursor]for (const item of items)
+  console.log(item);`,
+        expected: `const items = ['foo', 'bar', 'baz'];
+
+items.forEach(item => {
+  console.log(item);
+});`
+      },
+      {
+        description: "for-of, inline",
+        code: `for (const item of ['foo', 'bar', 'baz'])
+  console.log(item);`,
+        expected: `['foo', 'bar', 'baz'].forEach(item => {
+  console.log(item);
 });`
       }
     ],
@@ -180,6 +265,44 @@ items.forEach(item => {
         description: "for-loop with init not starting at 0",
         code: `for (let i = 1; i < items.length; i++) {
   console.log(items[i]);
+}`
+      },
+      {
+        description: "for-of but a string",
+        code: `const str = 'abcde';
+  for (let character of str) {
+  console.log(character);
+}`
+      },
+      {
+        description: "for-of but an inline string",
+        code: `for (let character of 'abcde') {
+  console.log(character);
+}`
+      },
+      // 👇 These patterns could be converted (they support forEach) but are not implemented yet.
+      {
+        description: "for-of but a map",
+        code: `const map = new Map();
+map.set('Me', {text: 'hello'});
+for (let entry of map) {
+  console.log(entry);
+}`
+      },
+      {
+        description: "for-of but a set",
+        code: `const set = new Set();
+set.add('Me');
+for (let entry of set) {
+  console.log(entry);
+}`
+      },
+      {
+        description: "for-of but a set",
+        code: `const typedArray = new Int8Array(8);
+typedArray[0] = 32;
+for (let entry of typedArray) {
+  console.log(entry);
 }`
       }
     ],
@@ -193,6 +316,26 @@ items.forEach(item => {
     }
   );
 
+  it("should call onMatch for a matching for-loop", async () => {
+    const code = `for (let i = 0; i < items.length; i++) {
+  console.log(items[i]);
+}`;
+    const onMatch = jest.fn();
+
+    t.traverseAST(t.parse(code), {
+      enter(path) {
+        const visitor = canConvertForLoop(Selection.cursorAt(0, 0), onMatch);
+        const visitorNode = visitor[path.node.type];
+        if (typeof visitorNode === "function") {
+          // @ts-expect-error visitor can expect `NodePath<File>` but `path` is typed as `NodePath<Node>`. It should be OK at runtime.
+          visitorNode.bind(visitor)(path, path.state);
+        }
+      }
+    });
+
+    expect(onMatch).toHaveBeenCalled();
+  });
+
   it("should show an error message if refactoring can't be made", async () => {
     const code = `// This is a comment, can't be refactored`;
     const editor = new InMemoryEditor(code);