Merge pull request #30 from CraigMacomber/isSharedBug

Fix IsShared bug when editRange removes multiple layers

Author: qwertie

b+tree.js

@@ -3,10 +3,12 @@ var __extends = (this && this.__extends) || (function () {
     var extendStatics = function (d, b) {
         extendStatics = Object.setPrototypeOf ||
             ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
-            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
+            function (d, b) { for (var p in b) if (Object.prototype.hasOwnProperty.call(b, p)) d[p] = b[p]; };
         return extendStatics(d, b);
     };
     return function (d, b) {
+        if (typeof b !== "function" && b !== null)
+            throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
         extendStatics(d, b);
         function __() { this.constructor = d; }
         d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
@@ -979,9 +981,16 @@ var BTree = /** @class */ (function () {
             return typeof r === "number" ? r : r.break;
         }
         finally {
-            while (root.keys.length <= 1 && !root.isLeaf)
+            var isShared = undefined;
+            while (root.keys.length <= 1 && !root.isLeaf) {
+                isShared || (isShared = root.isShared);
                 this._root = root = root.keys.length === 0 ? EmptyLeaf :
                     root.children[0];
+            }
+            // If any ancestor of the new root was shared, the new root must also be shared
+            if (isShared) {
+                root.isShared = true;
+            }
         }
     };
     /** Same as `editRange` except that the callback is called for all pairs. */

b+tree.test.ts

@@ -371,6 +371,49 @@ describe('Simple tests on leaf nodes', () =>
   });
 });
 
+// Tests relating to `isShared` and cloning.
+// Tests on this subject that do not care about the specific interior structure of the tree
+// (and are thus maxNodeSize agnostic) can be added to testBTree to be testing on different branching factors instead.
+describe("cloning and sharing tests", () => {
+  test("Regression test for failing to propagate shared when removing top two layers", () => {
+    // This tests make a full 3 layer tree (height = 2), so use a small branching factor.
+    const maxNodeSize = 4;
+    const tree = new BTree<number, number>(
+    undefined,
+    simpleComparator,
+    maxNodeSize
+    );
+    // Build a 3 layer complete tree, all values 0.
+    for (
+    let index = 0;
+    index < maxNodeSize * maxNodeSize * maxNodeSize;
+    index++
+    ) {
+    tree.set(index, 0);
+    }
+    // Leaf nodes don't count, so this is depth 2
+    expect(tree.height).toBe(2);
+  
+    // Edit the tree so it has a node in the second layer with exactly one child (key 0).
+    tree.deleteRange(1, maxNodeSize * maxNodeSize, false);
+    expect(tree.height).toBe(2);
+  
+    // Make a clone that should never be mutated.
+    const clone = tree.clone();
+  
+    // Mutate the original tree in such a way that clone gets mutated due to incorrect is shared tracking.
+    // Delete everything outside of the internal node with only one child, so its child becomes the new root.
+    tree.deleteRange(maxNodeSize, Number.POSITIVE_INFINITY, false);
+    expect(tree.height).toBe(0);
+  
+    // Modify original
+    tree.set(0, 1);
+  
+    // Check that clone is not modified as well:
+    expect(clone.get(0)).toBe(0);
+  });
+});
+
 describe('B+ tree with fanout 32', testBTree.bind(null, 32));
 describe('B+ tree with fanout 10', testBTree.bind(null, 10));
 describe('B+ tree with fanout 4',  testBTree.bind(null, 4));

b+tree.ts

@@ -1094,9 +1094,16 @@ export default class BTree<K=any, V=any> implements ISortedMapF<K,V>, ISortedMap
       var r = root.forRange(low, high, includeHigh, true, this, initialCounter || 0, onFound);
       return typeof r === "number" ? r : r.break!;
     } finally {
-      while (root.keys.length <= 1 && !root.isLeaf)
+      let isShared: undefined | true = undefined;
+      while (root.keys.length <= 1 && !root.isLeaf) {
+        isShared ||= root.isShared;
         this._root = root = root.keys.length === 0 ? EmptyLeaf :
                     (root as any as BNodeInternal<K,V>).children[0];
+      }
+      // If any ancestor of the new root was shared, the new root must also be shared
+      if (isShared) {
+        root.isShared = true;
+      }
     }
   }
 

package-lock.json

@@ -58,7 +58,7 @@
     "testpack-cli": "^1.1.4",
     "ts-jest": "^26.4.3",
     "ts-node": "^7.0.1",
-    "typescript": "^3.8.3",
+    "typescript": "^4.0.8",
     "uglify-js": "^3.11.4"
   },
   "dependencies": {},