Improve Inline Variable on destructured object patterns

CHANGELOG.md

@@ -7,6 +7,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+#### Inline Variable now handles destructured object patterns
+
+Consider the following code:
+
+```js
+const { userId } = session;
+messages.map(message => ({ userId }));
+```
+
+If you tried to inline `userId`, it wouldn't work because destructured object patterns were not supported.
+
+Now it would work as expected:
+
+```js
+messages.map(message => ({ userId: session.userId }));
+```
+
+Thanks to @noway for [bringing this one up](https://github.com/nicoespeon/abracadabra/issues/25).
+
+Destructured array patterns (e.g. `const [userId] = session`) are still not supported, but we're working on it.
+
 ## [0.7.0] - 2019-09-16
 
 ### Added

src/ast/scope.ts

@@ -1,7 +1,9 @@
 import { NodePath } from "@babel/traverse";
 import * as t from "@babel/types";
 
-export { findScopePath, findParentIfPath, getFunctionScopePath };
+import { areEqual } from "./domain";
+
+export { findScopePath, findParentIfPath, getFunctionScopePath, isShadowIn };
 
 function findScopePath(path: NodePath<t.Node | null>): NodePath | undefined {
   return path.findParent(
@@ -30,3 +32,37 @@ function findParentIfPath(
 function getFunctionScopePath(path: NodePath<t.FunctionDeclaration>): NodePath {
   return path.getFunctionParent() || path.parentPath;
 }
+
+function isShadowIn(
+  id: t.Identifier,
+  ancestors: t.TraversalAncestors
+): boolean {
+  // A variable is "shadow" if one of its ancestor redefines the Identifier.
+  return ancestors.some(
+    ({ node }) => isDeclaredInFunction(node) || isDeclaredInScope(node)
+  );
+
+  function isDeclaredInFunction(node: t.Node): boolean {
+    return (
+      t.isFunctionDeclaration(node) &&
+      node.params.some(node => areEqual(id, node))
+    );
+  }
+
+  function isDeclaredInScope(node: t.Node): boolean {
+    return (
+      t.isBlockStatement(node) &&
+      node.body.some(
+        child =>
+          t.isVariableDeclaration(child) &&
+          child.declarations.some(
+            declaration =>
+              t.isVariableDeclarator(declaration) &&
+              areEqual(id, declaration.id) &&
+              // Of course, if it's the inlined variable it's not a shadow!
+              declaration.id !== id
+          )
+      )
+    );
+  }
+}

src/ast/selection.ts

@@ -12,7 +12,8 @@ export {
   Selectable,
   isSelectableNode,
   isSelectableVariableDeclarator,
-  isSelectableIdentifier
+  isSelectableIdentifier,
+  isSelectableObjectProperty
 };
 
 interface ASTSelection {
@@ -27,11 +28,16 @@ interface ASTPosition {
 
 type SelectablePath = NodePath<SelectableNode>;
 type SelectableNode = Selectable<t.Node>;
-type SelectableObjectProperty = Selectable<t.ObjectProperty>;
 type SelectableIdentifier = Selectable<t.Identifier>;
 type SelectableVariableDeclarator = Selectable<t.VariableDeclarator>;
 type Selectable<T> = T & { loc: t.SourceLocation };
 
+interface SelectableObjectProperty extends t.ObjectProperty {
+  loc: t.SourceLocation;
+  key: Selectable<t.ObjectProperty["key"]>;
+  value: Selectable<t.ObjectProperty["value"]>;
+}
+
 function isSelectableNode(node: t.Node | null): node is SelectableNode {
   return !!node && !!node.loc;
 }
@@ -47,3 +53,14 @@ function isSelectableVariableDeclarator(
 ): declaration is SelectableVariableDeclarator {
   return !!declaration.loc;
 }
+
+function isSelectableObjectProperty(
+  property: t.Node | null
+): property is SelectableObjectProperty {
+  return (
+    t.isObjectProperty(property) &&
+    isSelectableNode(property) &&
+    isSelectableNode(property.value) &&
+    isSelectableNode(property.key)
+  );
+}

src/editor/selection.test.ts

@@ -33,7 +33,7 @@ describe("Selection", () => {
     it("should return selection that ends at given selection start", () => {
       const selection = new Selection([0, 10], [3, 10]);
 
-      const extendedSelection = selection.extendEndTo(
+      const extendedSelection = selection.extendEndToStartOf(
         new Selection([3, 12], [3, 15])
       );
 
@@ -43,7 +43,7 @@ describe("Selection", () => {
     it("should not change selection if the given one starts before our selection ends", () => {
       const selection = new Selection([0, 10], [3, 10]);
 
-      const extendedSelection = selection.extendEndTo(
+      const extendedSelection = selection.extendEndToStartOf(
         new Selection([2, 12], [3, 15])
       );
 
@@ -55,7 +55,7 @@ describe("Selection", () => {
     it("should return selection that starts at given selection end", () => {
       const selection = new Selection([0, 10], [3, 10]);
 
-      const extendedSelection = selection.extendStartTo(
+      const extendedSelection = selection.extendStartToEndOf(
         new Selection([0, 0], [0, 3])
       );
 
@@ -65,7 +65,7 @@ describe("Selection", () => {
     it("should not change selection if the given one ends after our selection starts", () => {
       const selection = new Selection([0, 10], [3, 10]);
 
-      const extendedSelection = selection.extendStartTo(
+      const extendedSelection = selection.extendStartToEndOf(
         new Selection([0, 0], [1, 3])
       );
 

src/editor/selection.ts

@@ -62,18 +62,30 @@ class Selection {
     );
   }
 
-  extendStartTo(selection: Selection): Selection {
+  extendStartToEndOf(selection: Selection): Selection {
     return selection.end.isBefore(this.start)
       ? Selection.fromPositions(selection.end, this.end)
       : this;
   }
 
-  extendEndTo(selection: Selection): Selection {
+  extendStartToStartOf(selection: Selection): Selection {
+    return selection.start.isBefore(this.start)
+      ? Selection.fromPositions(selection.start, this.end)
+      : this;
+  }
+
+  extendEndToStartOf(selection: Selection): Selection {
     return selection.start.isAfter(this.end)
       ? Selection.fromPositions(this.start, selection.start)
       : this;
   }
 
+  extendEndToEndOf(selection: Selection): Selection {
+    return selection.end.isAfter(this.end)
+      ? Selection.fromPositions(this.start, selection.end)
+      : this;
+  }
+
   isInsidePath(path: ast.NodePath): path is ast.SelectablePath {
     return this.isInsideNode(path.node);
   }

src/refactorings/extract-variable/extract-variable.ts

@@ -155,7 +155,7 @@ function getOccurrenceLoc(
   node: ast.SelectableNode,
   selection: Selection
 ): ast.SourceLocation | null {
-  return ast.isObjectProperty(node)
+  return ast.isSelectableObjectProperty(node)
     ? findObjectPropertyLoc(selection, node)
     : ast.isJSXExpressionContainer(node)
     ? node.expression.loc

src/refactorings/inline-variable-or-function/find-inlinable-code.test.ts

@@ -0,0 +1,120 @@
+import * as ast from "../../ast";
+
+import { Selection } from "../../editor/selection";
+import { Update } from "../../editor/editor";
+
+import { InlinableCode, InlinableObjectPattern } from "./find-inlinable-code";
+
+describe("InlinableObjectPattern", () => {
+  it("should return child code updates", () => {
+    const updates = [
+      {
+        code: "userId",
+        selection: ANY_SELECTION
+      },
+      {
+        code: "player.state",
+        selection: ANY_SELECTION
+      }
+    ];
+    const inlinable = createInlinableObjectPattern(
+      new FakeInlinable(updates),
+      ""
+    );
+
+    const result = inlinable.updateIdentifiersWith("");
+
+    expect(result).toEqual(updates);
+  });
+
+  it("should prepend inlined code with init name before forwarding it to child", () => {
+    const child = new FakeInlinable();
+    jest.spyOn(child, "updateIdentifiersWith");
+    const inlinable = createInlinableObjectPattern(child, "user");
+
+    inlinable.updateIdentifiersWith("name");
+
+    expect(child.updateIdentifiersWith).toBeCalledWith("user.name");
+  });
+
+  it("should resolve inlined code path if inlined code is already a member expression", () => {
+    const child = new FakeInlinable();
+    jest.spyOn(child, "updateIdentifiersWith");
+    const inlinable = createInlinableObjectPattern(child, "names");
+
+    inlinable.updateIdentifiersWith("user.first");
+
+    expect(child.updateIdentifiersWith).toBeCalledWith("user.names.first");
+  });
+
+  it("should resolve inlined code path if inlined code is an object property", () => {
+    const child = new FakeInlinable();
+    jest.spyOn(child, "updateIdentifiersWith");
+    const inlinable = createInlinableObjectPattern(child, "user");
+
+    inlinable.updateIdentifiersWith("n: name");
+
+    expect(child.updateIdentifiersWith).toBeCalledWith("user.n");
+  });
+
+  it("should resolve inlined code path if inlined code is a complex member expression", () => {
+    const child = new FakeInlinable();
+    jest.spyOn(child, "updateIdentifiersWith");
+    const inlinable = createInlinableObjectPattern(child, "user");
+
+    inlinable.updateIdentifiersWith("session.data[0].first");
+
+    expect(child.updateIdentifiersWith).toBeCalledWith(
+      "session.data[0].user.first"
+    );
+  });
+});
+
+const ANY_SELECTION = Selection.cursorAt(0, 0);
+
+class FakeInlinable implements InlinableCode {
+  isRedeclared = false;
+  isExported = false;
+  hasIdentifiersToUpdate = false;
+  shouldExtendSelectionToDeclaration = false;
+  valueSelection = ANY_SELECTION;
+  codeToRemoveSelection = ANY_SELECTION;
+
+  private updates: Update[];
+
+  constructor(updates: Update[] = []) {
+    this.updates = updates;
+  }
+
+  updateIdentifiersWith = () => this.updates;
+}
+
+function createInlinableObjectPattern(child: InlinableCode, initName: string) {
+  const ANY_LOC: ast.SourceLocation = {
+    start: { line: 0, column: 0 },
+    end: { line: 0, column: 0 }
+  };
+  const ANY_PROPERTY = ast.objectProperty(
+    ast.identifier(""),
+    ast.identifier("")
+  );
+  const ANY_SELECTABLE_PROPERTY: ast.SelectableObjectProperty = {
+    ...ANY_PROPERTY,
+    loc: ANY_LOC,
+    key: {
+      ...ANY_PROPERTY.key,
+      loc: ANY_LOC
+    },
+    value: {
+      ...ANY_PROPERTY.value,
+      loc: ANY_LOC
+    }
+  };
+
+  return new InlinableObjectPattern(
+    child,
+    initName,
+    ANY_SELECTABLE_PROPERTY,
+    false
+  );
+}