fix(data-structures): implement deep cloning for BinarySearchNode and…

data_structures/_binary_search_node.ts

@@ -17,13 +17,27 @@ export class BinarySearchNode<T> {
   }
 
   static from<T>(node: BinarySearchNode<T>): BinarySearchNode<T> {
-    const copy: BinarySearchNode<T> = new BinarySearchNode(
-      node.parent,
-      node.value,
-    );
-    copy.left = node.left;
-    copy.right = node.right;
-    return copy;
+    // New deep copy
+    // 1. Create a brand new node (with no parent at first)
+    const clone = new BinarySearchNode<T>(null, node.value);
+
+    // 2. Recursively clone the left subtree
+    if (node.left) {
+      clone.left = BinarySearchNode.from(node.left);
+      clone.left.parent = clone;
+    }
+
+    // 3. Recursively clone the right subtree
+    if (node.right) {
+      clone.right = BinarySearchNode.from(node.right);
+      clone.right.parent = clone;
+    }
+
+    // 4. We do NOT copy over 'parent' from the old node!  Instead, when the
+    //    parent's 'from()' call links the child, it sets child.parent = parent.
+    //    This ensures the copied tree has all new references.
+
+    return clone;
   }
 
   directionFromParent(): Direction | null {

data_structures/_binary_search_node_test.ts

@@ -1,5 +1,5 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
-import { assertEquals, assertStrictEquals } from "@std/assert";
+import { assert, assertEquals, assertStrictEquals } from "@std/assert";
 import { BinarySearchNode } from "./_binary_search_node.ts";
 
 let parent: BinarySearchNode<number>;
@@ -23,20 +23,55 @@ Deno.test("BinarySearchNode", () => {
   assertStrictEquals(child.value, 7);
 });
 
-Deno.test("BinarySearchNode.from()", () => {
-  beforeEach();
-  const parentClone: BinarySearchNode<number> = BinarySearchNode.from(parent);
-  const childClone: BinarySearchNode<number> = BinarySearchNode.from(child);
+Deno.test("BinarySearchNode.from() - deep copy", () => {
+  // Construct a small test tree:
+  //         (root:5)
+  //        /        \
+  // (left:3)        (right:7)
+  //       \
+  //       (4)
+  //
+  const root = new BinarySearchNode<number>(null, 5);
+  const leftChild = new BinarySearchNode<number>(root, 3);
+  const rightChild = new BinarySearchNode<number>(root, 7);
+  root.left = leftChild;
+  root.right = rightChild;
+
+  // Add a right child to the left child
+  const leftRightChild = new BinarySearchNode<number>(leftChild, 4);
+  leftChild.right = leftRightChild;
+
+  // Invoke the new from() logic to perform a deep copy
+  const rootClone = BinarySearchNode.from(root);
+
+  // 1. rootClone is a different reference from root but has the same value
+  assert(rootClone !== root);
+  assertStrictEquals(rootClone.value, 5);
+  // The root node's parent must be null
+  assertStrictEquals(rootClone.parent, null);
+
+  // 2. Verify the cloned leftChild
+  assert(rootClone.left !== null);
+  assert(rootClone.left !== leftChild);
+  assertStrictEquals(rootClone.left!.value, 3);
+  // Verify the cloned leftChild's parent points to the new rootClone
+  assertStrictEquals(rootClone.left!.parent, rootClone);
 
-  assertStrictEquals(parentClone.parent, null);
-  assertStrictEquals(parentClone.left, null);
-  assertStrictEquals(parentClone.right, child);
-  assertStrictEquals(parentClone.value, 5);
+  // 3. Verify the cloned rightChild
+  assert(rootClone.right !== null);
+  assert(rootClone.right !== rightChild);
+  assertStrictEquals(rootClone.right!.value, 7);
+  assertStrictEquals(rootClone.right!.parent, rootClone);
 
-  assertStrictEquals(childClone.parent, parent);
-  assertStrictEquals(childClone.left, null);
-  assertStrictEquals(childClone.right, null);
-  assertStrictEquals(childClone.value, 7);
+  // 4. Verify the cloned leftRightChild
+  const clonedLeftRightChild = rootClone.left!.right;
+  assert(clonedLeftRightChild !== null);
+  // It should not be the same as the original leftRightChild
+  assert(clonedLeftRightChild !== leftRightChild);
+  // But the value should be 4
+  assertStrictEquals(clonedLeftRightChild!.value, 4);
+  // And its parent should point to the cloned leftChild
+  assertStrictEquals(clonedLeftRightChild!.parent, rootClone.left);
 });
 
 Deno.test("BinarySearchNode.directionFromParent()", () => {

data_structures/binary_search_tree.ts

@@ -258,31 +258,13 @@ export class BinarySearchTree<T> implements Iterable<T> {
       if (options?.compare || options?.map) {
         unmappedValues = collection;
       } else {
-        const nodes: BinarySearchNode<U>[] = [];
+        // Just do a single deep clone from the root
         if (collection.#root) {
           result.#root = BinarySearchNode.from(
             collection.#root as unknown as BinarySearchNode<U>,
           );
-          nodes.push(result.#root);
-        }
-        while (nodes.length) {
-          const node: BinarySearchNode<U> = nodes.pop()!;
-          const left: BinarySearchNode<U> | null = node.left
-            ? BinarySearchNode.from(node.left)
-            : null;
-          const right: BinarySearchNode<U> | null = node.right
-            ? BinarySearchNode.from(node.right)
-            : null;
-
-          if (left) {
-            left.parent = node;
-            nodes.push(left);
-          }
-          if (right) {
-            right.parent = node;
-            nodes.push(right);
-          }
         }
+        // copy the size from the original
         result.#size = collection.#size;
       }
     } else {

data_structures/binary_search_tree_test.ts

@@ -556,3 +556,36 @@ Deno.test("BinarySearchTree.clear()", () => {
   tree.clear();
   assert(tree.isEmpty());
 });
+
+Deno.test("BinarySearchTree.from() bug demo - removing in copy corrupts original", () => {
+  // 1. Build the original tree
+  const values = [3, 10, 13, 4, 6, 7, 1, 14];
+  const original = BinarySearchTree.from(values);
+  // original should have 8 elements and include 7
+
+  // 2. Create a copy without passing compare/map
+  const copy = BinarySearchTree.from(original);
+  // copy and original should be independent trees
+
+  // 3. Remove value 7 from the copy
+  const removedInCopy = 7;
+  const removeResult = copy.remove(removedInCopy);
+  // If all is well, removing 7 from copy should not affect original
+
+  // 4. Original tree should still find 7
+  const stillInOriginal = original.find(removedInCopy);
+
+  // Assertion: original should still contain 7
+  assertStrictEquals(
+    stillInOriginal !== null,
+    true,
+    `Expected original.find(${removedInCopy}) to not be null, but got null.`,
+  );
+
+  // Additionally, check if removeResult is true
+  assertStrictEquals(
+    removeResult,
+    true,
+    `Expected copy.remove(${removedInCopy}) to return true, but got false.`,
+  );
+});