Delete tree cursors (#3378)

Author: AndreasArvidsson

packages/lib-engine/src/actions/ShowParseTree.ts

@@ -43,7 +43,12 @@ function parseTree(
   const resultPlayground: string[] = [];
   const resultQuery: string[] = [];
 
-  parseCursor(resultPlayground, resultQuery, contentRange, tree.walk(), 0);
+  const cursor = tree.walk();
+  try {
+    parseCursor(resultPlayground, resultQuery, contentRange, cursor, 0);
+  } finally {
+    cursor.delete();
+  }
 
   return [
     `## ${document.filename} [${contentRange}]\n`,

packages/lib-engine/src/languages/TreeSitterQuery/getChildNodesForFieldName.ts

@@ -6,14 +6,18 @@ export function getChildNodesForFieldName(
 ): Node[] {
   const nodes = [];
   const treeCursor = node.walk();
-  let hasNext = treeCursor.gotoFirstChild();
+  try {
+    let hasNext = treeCursor.gotoFirstChild();
 
-  while (hasNext) {
-    if (treeCursor.currentFieldName === fieldName) {
-      nodes.push(treeCursor.currentNode);
+    while (hasNext) {
+      if (treeCursor.currentFieldName === fieldName) {
+        nodes.push(treeCursor.currentNode);
+      }
+      hasNext = treeCursor.gotoNextSibling();
     }
-    hasNext = treeCursor.gotoNextSibling();
-  }
 
-  return nodes;
+    return nodes;
+  } finally {
+    treeCursor.delete();
+  }
 }

packages/lib-engine/src/languages/TreeSitterQuery/isEven.ts

@@ -13,18 +13,22 @@ export function isEven(node: Node, fieldName: string): boolean {
   }
 
   const treeCursor = node.parent.walk();
-  let hasNext = treeCursor.gotoFirstChild();
-  let even = true;
+  try {
+    let hasNext = treeCursor.gotoFirstChild();
+    let even = true;
 
-  while (hasNext) {
-    if (treeCursor.currentFieldName === fieldName) {
-      if (treeCursor.currentNode.id === node.id) {
-        return even;
+    while (hasNext) {
+      if (treeCursor.currentFieldName === fieldName) {
+        if (treeCursor.currentNode.id === node.id) {
+          return even;
+        }
+        even = !even;
       }
-      even = !even;
+      hasNext = treeCursor.gotoNextSibling();
     }
-    hasNext = treeCursor.gotoNextSibling();
-  }
 
-  throw new Error(`Node not found in parent for field: ${fieldName}`);
+    throw new Error(`Node not found in parent for field: ${fieldName}`);
+  } finally {
+    treeCursor.delete();
+  }
 }

packages/lib-engine/src/util/treeSitterUtils.test.ts

@@ -0,0 +1,85 @@
+import assert from "node:assert/strict";
+import { mock } from "node:test";
+import type { Tree, TreeCursor } from "web-tree-sitter";
+import { Node } from "web-tree-sitter";
+import { FakeIDE, InMemoryTextEditor } from "@cursorless/lib-common";
+import { getChildNodesForFieldName } from "../languages/TreeSitterQuery/getChildNodesForFieldName";
+import { isEven } from "../languages/TreeSitterQuery/isEven";
+import { TestTreeSitter } from "../testUtil/TestTreeSitter";
+import { getChildNodesForFieldName as getChildren } from "./treeSitterUtils";
+
+suite("Tree-sitter cursor lifetime", () => {
+  const treeSitter = new TestTreeSitter();
+  let tree: Tree;
+  const deletions: {
+    cursor: TreeCursor;
+    deletion: ReturnType<typeof mock.method>;
+  }[] = [];
+
+  suiteSetup(async () => {
+    await treeSitter.loadLanguage("clojure");
+  });
+
+  setup(() => {
+    const { document } = new InMemoryTextEditor({
+      ide: new FakeIDE(),
+      content: "{:a 1 :b 2}",
+      languageId: "clojure",
+    });
+    tree = treeSitter.getTree(document);
+    const walk = Node.prototype.walk;
+    mock.method(Node.prototype, "walk", function (this: Node) {
+      const cursor = walk.call(this);
+      deletions.push({ cursor, deletion: mock.method(cursor, "delete") });
+      return cursor;
+    });
+  });
+
+  teardown(() => {
+    try {
+      assert.ok(deletions.length > 0, "Expected traversal to create a cursor");
+      for (const { deletion } of deletions) {
+        assert.equal(deletion.mock.callCount(), 1);
+      }
+    } finally {
+      // Also clean up cursors if a regression leaves them unfreed. Their WASM
+      // finalizers can crash the process, obscuring the assertion failure.
+      for (const { cursor, deletion } of deletions) {
+        if (deletion.mock.callCount() === 0) {
+          cursor.delete();
+        }
+      }
+      deletions.length = 0;
+      mock.restoreAll();
+      tree.delete();
+    }
+  });
+
+  test("releases the cursor on an early return from isEven", () => {
+    const node = tree.rootNode.descendantsOfType("kwd_lit")[0];
+    assert.equal(isEven(node, "value"), true);
+  });
+
+  test("releases the cursor when isEven throws for a missing field", () => {
+    const node = tree.rootNode.descendantsOfType("kwd_lit")[0];
+    assert.throws(() => isEven(node, "missing"), /Node not found in parent/u);
+  });
+
+  for (const [name, getNodes] of [
+    ["query", getChildNodesForFieldName],
+    ["utility", getChildren],
+  ] as const) {
+    test(`${name} traversal releases the cursor after collecting children`, () => {
+      const node = tree.rootNode.descendantsOfType("map_lit")[0];
+      assert.deepEqual(
+        getNodes(node, "value").map((child) => child.text),
+        [":a", "1", ":b", "2"],
+      );
+    });
+
+    test(`${name} traversal releases the cursor for a leaf`, () => {
+      const node = tree.rootNode.descendantsOfType("num_lit")[0];
+      assert.deepEqual(getNodes(node, "value"), []);
+    });
+  }
+});

packages/lib-engine/src/util/treeSitterUtils.ts

@@ -19,21 +19,25 @@ export function getChildNodesForFieldName(
   fieldName: string,
 ): Node[] {
   const treeCursor = node.walk();
-  treeCursor.gotoFirstChild();
+  try {
+    treeCursor.gotoFirstChild();
 
-  const ret = [];
+    const ret = [];
 
-  let hasNext = true;
+    let hasNext = true;
 
-  while (hasNext) {
-    if (treeCursor.currentFieldName === fieldName) {
-      ret.push(treeCursor.currentNode);
+    while (hasNext) {
+      if (treeCursor.currentFieldName === fieldName) {
+        ret.push(treeCursor.currentNode);
+      }
+
+      hasNext = treeCursor.gotoNextSibling();
     }
 
-    hasNext = treeCursor.gotoNextSibling();
+    return ret;
+  } finally {
+    treeCursor.delete();
   }
-
-  return ret;
 }
 
 /**

packages/test-vscode-e2e/src/suite/recorded.vscode.test.ts

@@ -11,7 +11,9 @@ import {
 } from "@cursorless/lib-node-common";
 import {
   getReusableEditor,
+  getParseTreeApi,
   getTestHelpers,
+  resetReusableEditor,
   runCursorlessCommand,
 } from "@cursorless/lib-vscode-common";
 import { endToEndTestSetup, sleepWithBackoff } from "../endToEndTestSetup";
@@ -36,19 +38,56 @@ suite("recorded test cases", function () {
           this.ctx.skip();
         }
 
-        await runRecordedTest({
-          path,
-          spyIde: getSpy(),
-          openNewTestEditor,
-          sleepWithBackoff,
-          testHelpers: await getTestHelpers(),
-          runCursorlessCommand,
-        });
+        try {
+          await runRecordedTest({
+            path,
+            spyIde: getSpy(),
+            openNewTestEditor,
+            sleepWithBackoff,
+            testHelpers: await getTestHelpers(),
+            runCursorlessCommand,
+          });
+        } catch (error) {
+          // A retry must not inherit the failed attempt's selections or
+          // incrementally updated parse tree.
+          resetReusableEditor();
+          await logFailureContext(name);
+          throw error;
+        }
       }),
     );
   }
 });
 
+async function logFailureContext(name: string): Promise<void> {
+  const editor = vscode.window.activeTextEditor;
+  if (editor == null) {
+    console.error(`Recorded test failed: ${name}; no active editor`);
+    return;
+  }
+
+  const { document, selections } = editor;
+  console.error(`Recorded test failed: ${name}`, {
+    uri: document.uri.toString(),
+    languageId: document.languageId,
+    version: document.version,
+    content: document.getText(),
+    selections: selections.map(({ anchor, active }) => ({ anchor, active })),
+  });
+
+  try {
+    const parseTree = await getParseTreeApi();
+    console.error(
+      "Parse tree:",
+      parseTree.getTreeForUri(document.uri).rootNode.toString(),
+    );
+  } catch (error) {
+    // Diagnostics should preserve the original test failure, including for
+    // languages without Tree-sitter support.
+    console.error("Could not read parse tree:", error);
+  }
+}
+
 async function openNewTestEditor(
   content: string,
   languageId: string,