Skip to content

fix(data-structures): deep-copy nodes in BinarySearchTree/RedBlackTree.from()#7230

Open
LeSingh1 wants to merge 1 commit into
denoland:mainfrom
LeSingh1:fix-tree-from-deep-copy
Open

fix(data-structures): deep-copy nodes in BinarySearchTree/RedBlackTree.from()#7230
LeSingh1 wants to merge 1 commit into
denoland:mainfrom
LeSingh1:fix-tree-from-deep-copy

Conversation

@LeSingh1

Copy link
Copy Markdown
Contributor

Problem

BinarySearchTree.from() and RedBlackTree.from() are documented to create an
independent copy of an existing tree, but they only performed a partial copy.
The traversal that walks the source tree created a copy of each child node and
fixed up the child's parent pointer, but it never re-linked the parent copy's
left/right pointers to point at those new child copies. Because
BinarySearchNode.from() / RedBlackNode.from() seed a copy's left/right
with the original node's children, the returned tree ended up still
referencing the source tree's descendant nodes.

RedBlackTree.from() was additionally worse: it seeded the result with the
source tree's root node itself (via setRoot(result, root)) rather than a
copy of it.

The net effect is that the "copy" shares nodes with the original, so mutating
the copy corrupts the original (and vice versa):

import { BinarySearchTree } from "@std/data-structures";

const original = BinarySearchTree.from([3, 10, 13, 4, 6, 7, 1, 14]);
const copy = BinarySearchTree.from(original);

copy.remove(7);
original.find(7); // null  ❌ — 7 was removed from the original too

Fixes #6314
Fixes #6315

Fix

  • Re-link node.left / node.right to the freshly created child copies during
    the copy traversal in both BinarySearchTree.from() and
    RedBlackTree.from().
  • In RedBlackTree.from(), copy the root node before seeding the result with it
    (so the traversal no longer mutates the source root).

The mapped / custom-comparator paths are unchanged (they already rebuild the
tree via insert()).

Verification

  • Added regression tests for both trees asserting that a removal in either the
    copy or the original leaves the other intact. They fail on main
    (original.find(7) returns null) and pass with this change.
  • deno test data_structures/ → 350 passed, 0 failed.
  • deno fmt --check and deno lint on the changed files are clean.
  • deno check on the changed files passes.

…e.from()

When copying an existing tree, from() created a copy of only the root
node and then, while walking the tree, built copies of each child but
never re-linked the parent copy's left/right pointers to those copies.
As a result the returned tree still referenced the original tree's
descendant nodes, so mutating the copy (e.g. remove()) corrupted the
original. RedBlackTree.from() was additionally seeding the result with
the original root node itself rather than a copy.

Re-link node.left/node.right to the freshly created child copies during
the traversal, and copy the root node in RedBlackTree.from() before
using it. Add regression tests asserting that removals in either the
copy or the original do not affect the other.

Fixes denoland#6314
Fixes denoland#6315
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant