Repository: nicoespeon/abracadabra (ts)

Issue:
"Change Signature" doesn't work on a nested function
## Describe the bug

When I try to run "Change Signature" on a function nested inside another function, it doesn't really work.

At first I thought it wasn't doing anything, but after I wrote a test for it, I realized it might actually be changing the call sites but not the function itself?

(I'm uncertain because the unit tests aren't run using the actual vscode editor, which I understand is partly because Jest didn't seem to provide the APIs you'd need to write a runner analogous to the simple Mocha runner that the VSCode extension docs and examples suggest you use; it looks like Jest has a lot more layers than Mocha? I wonder if <https://github.com/macabeus/jest-environment-vscode-extension> would work for you?)

Actually, when I run it live it modifies the head of the outer function ...

## How to reproduce
I've actually written a test for this:

https://github.com/nicoespeon/abracadabra/blob/0def578c5dd37db9c9bc113507606d49138e5111/src/refactorings/change-signature/change-signature.test.ts#L176-L190

I've also submitted that as <https://github.com/nicoespeon/abracadabra/pull/838> (currently marked draft because I had no issue to reference and I'm not sure you want to merge a failing test), and you can see the failure there.

## Expected behavior

I expected changing the signature of a nested function to adjust both the parameters in the function itself, and the arguments passed in calls to the function; at worst, I expected the refactoring to be unavailable.

## Screenshots

_If applicable, add screenshots to help explain your problem._

## Additional information

- Version of the extension impacted: v7.0.0

_🧙‍ Add any other context about the problem here._


Pull request:
Fix "Change Signature" for nested functions
Fixes #841 

Patch:
diff --git a/src/editor/adapters/in-memory-editor.ts b/src/editor/adapters/in-memory-editor.ts
index 9315189d..d5099371 100644
--- a/src/editor/adapters/in-memory-editor.ts
+++ b/src/editor/adapters/in-memory-editor.ts
@@ -380,8 +380,8 @@ export class InMemoryEditor implements Editor {
           new CodeReference(
             obj.filename,
             new Selection(
-              [pos.row, pos.col],
-              [pos.row, pos.col + functionReferences.length]
+              [pos.row - 1, pos.col],
+              [pos.row - 1, pos.col + functionReferences.length]
             )
           )
         );
diff --git a/src/editor/adapters/vscode-editor.ts b/src/editor/adapters/vscode-editor.ts
index 48f01fc7..d0bbb50a 100644
--- a/src/editor/adapters/vscode-editor.ts
+++ b/src/editor/adapters/vscode-editor.ts
@@ -314,15 +314,8 @@ export class VSCodeEditor implements Editor {
     )) as vscode.Location[];
 
     return locations.map((loc) => {
-      const start = loc.range.start;
-      const end = loc.range.end;
-
       const path = new AbsolutePath(loc.uri.path);
-
-      const codeReferenceSelection = new Selection(
-        [start.line + 1, start.character],
-        [end.line + 1, end.character]
-      );
+      const codeReferenceSelection = createSelectionFromVSCode(loc.range);
 
       return new CodeReference(path, codeReferenceSelection);
     });
diff --git a/src/refactorings/change-signature/change-signature.ts b/src/refactorings/change-signature/change-signature.ts
index 87aa855f..7c305d46 100644
--- a/src/refactorings/change-signature/change-signature.ts
+++ b/src/refactorings/change-signature/change-signature.ts
@@ -1,8 +1,9 @@
 import * as t from "../../ast";
-import { parse } from "../../ast";
+import { isSelectablePath, parse } from "../../ast";
 import { isFunctionDeclarationOrArrowFunction } from "../../ast/identity";
 import { Editor, ErrorReason, SelectedPosition } from "../../editor/editor";
 import { Path } from "../../editor/path";
+import { Position } from "../../editor/position";
 import { Selection } from "../../editor/selection";
 
 export async function changeSignature(editor: Editor) {
@@ -181,25 +182,20 @@ export function createVisitor(
   return {
     FunctionDeclaration(path) {
       if (!selection.isInsidePath(path)) return;
-
       if (!hasParameters(path.node)) return;
 
       onMatch(path, selection);
     },
     ArrowFunctionExpression(path) {
       if (!selection.isInsidePath(path)) return;
-
       if (!hasParameters(path.node)) return;
-
       if (!t.isVariableDeclarator(path.parent)) return;
-
       if (!path.parent.loc) return;
 
       onMatch(path, Selection.fromAST(path.parent.loc));
     },
     ClassMethod(path) {
       if (!selection.isInsidePath(path)) return;
-
       if (!hasParameters(path.node)) return;
 
       onMatch(path, selection);
@@ -258,10 +254,15 @@ function createVisitorForReferences(
 ): t.Visitor {
   return {
     CallExpression(path) {
-      const nodeSelection = new Selection(
-        [path.node.loc?.start.line || 0, 0],
-        [path.node.loc?.end.line || 0, 0]
-      );
+      if (!isSelectablePath(path)) return;
+
+      // Since we visit nodes from parent to children, first check
+      // if a child would match the selection closer.
+      if (hasChildWhichMatchesSelection(path, selection)) return;
+
+      const start = Position.fromAST(path.node.loc.start).putAtStartOfLine();
+      const end = Position.fromAST(path.node.loc.end).putAtStartOfLine();
+      const nodeSelection = Selection.fromPositions(start, end);
 
       if (!selection.isSameLineThan(nodeSelection)) return;
 
@@ -269,20 +270,78 @@ function createVisitorForReferences(
     },
     FunctionDeclaration(path) {
       if (!selection.isInsidePath(path)) return;
+
+      // Since we visit nodes from parent to children, first check
+      // if a child would match the selection closer.
+      if (hasChildWhichMatchesSelection(path, selection)) return;
+
       onMatch(path);
     },
     ArrowFunctionExpression(path) {
-      if (!selection.isInsidePath(path)) return;
+      if (!selection.isInsidePath(path.parentPath)) return;
+
+      // Since we visit nodes from parent to children, first check
+      // if a child would match the selection closer.
+      if (hasChildWhichMatchesSelection(path, selection)) return;
 
       onMatch(path);
     },
     ClassMethod(path) {
       if (!selection.isInsidePath(path)) return;
+
+      // Since we visit nodes from parent to children, first check
+      // if a child would match the selection closer.
+      if (hasChildWhichMatchesSelection(path, selection)) return;
+
       onMatch(path);
     }
   };
 }
 
+function hasChildWhichMatchesSelection(
+  path: t.NodePath,
+  selection: Selection
+): boolean {
+  let result = false;
+
+  path.traverse({
+    CallExpression(childPath) {
+      if (!isSelectablePath(childPath)) return;
+
+      const start = Position.fromAST(
+        childPath.node.loc.start
+      ).putAtStartOfLine();
+      const end = Position.fromAST(childPath.node.loc.end).putAtStartOfLine();
+      const nodeSelection = Selection.fromPositions(start, end);
+
+      if (!selection.isSameLineThan(nodeSelection)) return;
+
+      result = true;
+      childPath.stop();
+    },
+    FunctionDeclaration(childPath) {
+      if (!selection.isInsidePath(childPath)) return;
+
+      result = true;
+      childPath.stop();
+    },
+    ArrowFunctionExpression(childPath) {
+      if (!selection.isInsidePath(childPath.parentPath)) return;
+
+      result = true;
+      childPath.stop();
+    },
+    ClassMethod(childPath) {
+      if (!selection.isInsidePath(childPath)) return;
+
+      result = true;
+      childPath.stop();
+    }
+  });
+
+  return result;
+}
+
 function hasParameters(
   node: t.FunctionDeclaration | t.ArrowFunctionExpression | t.ClassMethod
 ) {


Test patch:
diff --git a/src/editor/editor-contract-test.ts b/src/editor/editor-contract-test.ts
index f71a3844..9278b6aa 100644
--- a/src/editor/editor-contract-test.ts
+++ b/src/editor/editor-contract-test.ts
@@ -427,8 +427,8 @@ add(1, 2);
 
     assert.strictEqual(codeReferences.length, 2);
     assert.deepStrictEqual(codeReferences, [
-      new CodeReference(filePath, new Selection([1, 9], [1, 12])),
-      new CodeReference(filePath, new Selection([4, 0], [4, 3]))
+      new CodeReference(filePath, new Selection([0, 9], [0, 12])),
+      new CodeReference(filePath, new Selection([3, 0], [3, 3]))
     ]);
   });
 }
diff --git a/src/refactorings/change-signature/change-signature.test.ts b/src/refactorings/change-signature/change-signature.test.ts
index 7362e4ec..70d3d7a8 100644
--- a/src/refactorings/change-signature/change-signature.test.ts
+++ b/src/refactorings/change-signature/change-signature.test.ts
@@ -171,6 +171,23 @@ describe("Change Signature", () => {
           add(" years", 7);
           add();
           add(undefined, 1);`
+      },
+      {
+        description: "of a nested function",
+        code: `function outer() {
+          function [cursor]add(a, b) {
+            return a + b;
+          }
+
+          return add(1, 2);
+        }`,
+        expected: `function outer() {
+          function add(b, a) {
+            return a + b;
+          }
+
+          return add(2, 1);
+        }`
       }
     ],
     async ({ code, expected }) => {