Repository: DmitrySoshnikov/regexp-tree (js)

Issue:
reordering `a(b)` to `b(a)` does not set parentPath correctly (parent not in sync with parentPath)
I have a test case, where the parents get out of sync.

I think there is something wrong or missing in my latest `setChild` code or perhaps in `getForNode`.

Originally I used `replace()` in the test case and blamed it for the error.
But now I have rewritten it using `setChild`. I generally think `replace()` shouldn't be used for such reorders. But now I have a very similar error.

I already tried to find the bug, but I didn't succeed, yet.
My debugger is non-functional currently, so I only try to use logging and good conclusions.

Perhaps someone else wants to try?

But of course I'm not out of options (I guess I never will), so I'll try more myself.
I just wanted to let everyone know, that there is a problem.

This is the test case:
```js
  it('replaces with already used node with another parent', () => {
    const ast = parser.parse('/a(b)/');

    const bodyPath = new NodePath(ast.body);
    const aCharPath = bodyPath.getChild(0);
    const groupPath = bodyPath.getChild(1);
    const bCharPath = groupPath.getChild();
    const aNode = aCharPath.node;
    const bNode = bCharPath.node;

    bodyPath.node.mark = "body";
    groupPath.node.mark = "group";

    // swap a and b
    groupPath.setChild(aNode);
    bodyPath.setChild(bNode, 0);

    // body and group still at same position
    expect(bodyPath.node.mark).toEqual('body')
    expect(groupPath.node.mark).toEqual('group')
    expect(bodyPath.getChild(1).node.mark).toEqual('group')
    // second is Char 'a' in the Group
    expect(bodyPath.getChild(1).getChild().node.type).toEqual('Char')
    expect(bodyPath.getChild(1).getChild().node.value).toEqual('a')
    // now the parent of 'a' is the Group
    expect(bodyPath.getChild(1).getChild().parent.mark).toEqual('group')
    
    // ...
  });
```
the last `expect` fails like this:
```
    Expected value to equal:
      "group"
    Received:
      "body"
```

If you add this (instead of the last `expect`):
```js
expect(bodyPath.getChild(1).getChild().parentPath.node)
 .toBe(bodyPath.getChild(1).getChild().parent)
```
you get this error:
```
    - Expected
    + Received

    Object {
    -  "expressions": Array [
    -    Object {
    -      "kind": "simple",
    -      "symbol": undefined,
    -      "type": "Char",
    -      "value": "b",
    -    },
    -    Object {
    -      "capturing": true,
    -      "expression": Object {
    -        "kind": "simple",
    -        "symbol": undefined,
    -        "type": "Char",
    -        "value": "a",
    -      },
    -      "mark": "group",
    -      "number": 1,
    -      "type": "Group",
    -    },
    -  ],
    -  "mark": "body",
    -  "type": "Alternative",
    +  "capturing": true,
    +  "expression": Object {
    +    "kind": "simple",
    +    "symbol": undefined,
    +    "type": "Char",
    +    "value": "a",
    +  },
    +  "mark": "group",
    +  "number": 1,
    +  "type": "Group",
     }
```
so the node in the `parentPath` of 'a' is the body (which is wrong) and the `parent` is the group (which is correct). Of course they should be identical (=group).

@DmitrySoshnikov you added the same `NodePath.getForNode` to `replace()` like I did.
I still think, this is correct.

EDIT: I removed some lines about getForNode and index parameter, because that's not ready to be discussed

Pull request:
fix for parent and parentPath not in sync and index != null for single child
* fixes #60
* fixes index != null for single child

Patch:
diff --git a/src/traverse/node-path.js b/src/traverse/node-path.js
index 6610dd4..cd95d3e 100644
--- a/src/traverse/node-path.js
+++ b/src/traverse/node-path.js
@@ -55,21 +55,23 @@ class NodePath {
    */
   setChild(node, index = null, property = null) {
 
+    let childPath;
     if (index != null) {
       if (!property) {
         property = DEFAULT_COLLECTION_PROP;
       }
       this._enforceProp(property);
       this.node[property][index] = node;
-      return NodePath.getForNode(node, this, property, index);
+      childPath = NodePath.getForNode(node, this, property, index);
     } else {
       if (!property) {
         property = DEFAULT_SINGLE_PROP;
       }
       this._enforceProp(property);
       this.node[property] = node;
-      return NodePath.getForNode(node, this, property);
+      childPath = NodePath.getForNode(node, this, property, null);
     }
+    return childPath;
   }
 
   /**
@@ -180,10 +182,11 @@ class NodePath {
    * Replaces a node with the passed one.
    */
   replace(newNode) {
+
     this.node = newNode;
 
     if (!this.parent) {
-      return;
+      return null;
     }
 
     // A node is in a collection.
@@ -305,7 +308,7 @@ class NodePath {
    * The same NodePath can be reused in several places, e.g.
    * a parent node passed for all its children.
    */
-  static getForNode(node, parentPath = null, prop = null, index = null) {
+  static getForNode(node, parentPath = null, prop = null, index = -1) {
     if (!node) {
       return null;
     }
@@ -313,7 +316,7 @@ class NodePath {
     if (!NodePath.registry.has(node)) {
       NodePath.registry.set(
         node,
-        new NodePath(node, parentPath, prop, index)
+        new NodePath(node, parentPath, prop, index == -1 ? null : index)
       );
     }
 
@@ -321,13 +324,14 @@ class NodePath {
 
     if (parentPath !== null) {
       path.parentPath = parentPath;
+      path.parent = path.parentPath.node;
     }
 
     if (prop !== null) {
       path.property = prop;
     }
 
-    if (index !== null) {
+    if (index >= 0) {
       path.index = index;
     }
 


Test patch:
diff --git a/src/traverse/__tests__/node-path-test.js b/src/traverse/__tests__/node-path-test.js
index 2a556af..1213bfc 100644
--- a/src/traverse/__tests__/node-path-test.js
+++ b/src/traverse/__tests__/node-path-test.js
@@ -317,6 +317,67 @@ describe('NodePath', () => {
     });
   });
 
+  it('replaces with already used node with another parent', () => {
+    const ast = parser.parse('/a(b)/');
+
+    const bodyPath = new NodePath(ast.body);
+    const aCharPath = bodyPath.getChild(0);
+    const groupPath = bodyPath.getChild(1);
+    const bCharPath = groupPath.getChild();
+    const aNode = aCharPath.node;
+    const bNode = bCharPath.node;
+
+    bodyPath.node.mark = "body";
+    groupPath.node.mark = "group";
+
+    // swap a and b
+    groupPath.setChild(aNode);
+    bodyPath.setChild(bNode, 0);
+
+    // 'a' and 'b' should be swapped now
+    expect(generator.generate(ast)).toBe('/b(a)/');
+
+    // body and group still ok
+    expect(bodyPath.node.mark).toBe('body')
+    expect(groupPath.node.mark).toBe('group')
+    expect(bodyPath.getChild(1).node.mark).toBe('group')
+
+    // first is now Char 'b' in body
+    expect(bodyPath.getChild(0).node.type).toBe('Char')
+    expect(bodyPath.getChild(0).node.value).toBe('b')
+    // parentPath should point to the same node as parent
+    expect(bodyPath.getChild(0).parentPath.node).toBe(bodyPath.getChild(0).parent)
+    // now the parent of 'b' is the body
+    expect(bodyPath.getChild(0).parent.mark).toBe('body')
+    expect(bodyPath.getChild(0).parent.type).toBe('Alternative')
+    // Group has children property 'expressions'
+    expect(bodyPath.getChild(0).property).toBe('expressions')
+    // and an index of 0
+    expect(bodyPath.getChild(0).index).toBe(0)
+
+    // second is Char 'a' in the Group
+    expect(bodyPath.getChild(1).getChild().node.type).toBe('Char')
+    expect(bodyPath.getChild(1).getChild().node.value).toBe('a')
+    // parentPath should point to the same node as parent
+    expect(bodyPath.getChild(1).getChild().parentPath.node).toBe(bodyPath.getChild(1).getChild().parent)
+    // now the parent of 'a' is the Group
+    expect(bodyPath.getChild(1).getChild().parent.mark).toBe('group')
+    expect(bodyPath.getChild(1).getChild().parent.type).toBe('Group')
+    // Group has child property 'expression'
+    expect(bodyPath.getChild(1).getChild().property).toBe('expression')
+    // and no index!
+    expect(bodyPath.getChild(1).getChild().index).toBe(null)
+
+    // the locally stored NodePaths should have new parents
+    expect(bCharPath.parentPath).toBe(bodyPath);
+    expect(aCharPath.parentPath).toBe(groupPath);
+    // paths from the registry should be the same
+    const bCharPath2 = NodePath.getForNode(bNode);
+    expect(bCharPath2).toBe(bCharPath);
+    const aCharPath2 = NodePath.getForNode(aNode);
+    expect(aCharPath2).toBe(aCharPath);
+  });
+
   it('sets a child node to two new nested nodes', () => {
     const ast = parser.parse('/ab/');
 
@@ -368,7 +429,7 @@ describe('NodePath', () => {
     expect(dPath.parentPath).toBe(alterPath);
     expect(alterPath.parentPath).toBe(groupPath);
 
-    expect(generator.generate(ast)).toBe('/a(cd)/');
+    expect(generator.generate(ast)).toEqual('/a(cd)/');
   });
 
   it('getPreviousSibling', () => {