fix(linter/plugins): reset ancestors if error during AST walk

apps/oxlint/src-js/plugins/lint.ts

@@ -21,7 +21,7 @@ import { TOKEN } from '../../dist/src-js/raw-transfer/lazy-common.js';
 import { walkProgram } from '../generated/walk.js';
 */
 
-import { walkProgram } from "../generated/walk.js";
+import { walkProgram, ancestors } from "../generated/walk.js";
 
 import type { AfterHook, BufferWithArrays } from "./types.ts";
 
@@ -209,8 +209,13 @@ export function lintFileImpl(
   if (needsVisit) {
     if (ast === null) initAst();
     debugAssertIsNonNull(ast);
+
+    debugAssert(ancestors.length === 0, "`ancestors` should be empty before walking AST");
+
     walkProgram(ast, compiledVisitor);
 
+    debugAssert(ancestors.length === 0, "`ancestors` should be empty after walking AST");
+
     // Lazy implementation
     /*
     const sourceIsAscii = sourceText.length === sourceByteLen;
@@ -255,5 +260,6 @@ export function resetFile() {
  */
 export function resetStateAfterError() {
   diagnostics.length = 0;
+  ancestors.length = 0;
   resetFile();
 }

apps/oxlint/test/fixtures/lint_visit_error/files/2.js

@@ -0,0 +1 @@
+let y;

apps/oxlint/test/fixtures/lint_visit_error/options.json

@@ -0,0 +1,3 @@
+{
+  "singleThread": true
+}

apps/oxlint/test/fixtures/lint_visit_error/output.snap.md

@@ -4,12 +4,18 @@
 # stdout
 ```
   x Error running JS plugin.
-  | File path: <fixture>/files/index.js
-  | Error: Whoops!
-  |     at Identifier (<fixture>/plugin.ts:12:19)
+  | File path: <fixture>/files/1.js
+  | Error: Identifier in 1st file: x
+  |     at Identifier (<fixture>/plugin.ts:41:32)
 
-Found 0 warnings and 1 error.
-Finished in Xms on 1 file using X threads.
+  x error-plugin(error): Identifier in 2nd file: y
+   ,-[files/2.js:1:5]
+ 1 | let y;
+   :     ^
+   `----
+
+Found 0 warnings and 2 errors.
+Finished in Xms on 2 files using X threads.
 ```
 
 # stderr

apps/oxlint/test/fixtures/lint_visit_error/plugin.ts

@@ -1,19 +1,69 @@
-import type { Plugin } from "#oxlint";
+import assert from "node:assert";
+
+import type { Plugin, Rule } from "#oxlint";
+
+// Aim of this test is:
+//
+// 1. Check that errors thrown during AST visitation are handled correctly and shown to user as diagnostics.
+// 2. Check that global state is reset after an error during visiting a file, so it's in correct initial state
+//    when Oxlint starts linting the next file.
+//
+// The 2nd is tricky to test because usually the order Oxlint lints files in is non-deterministic.
+// To make this test deterministic, we run it with `oxlint --threads 1`
+// (`options.json` file for this fixture contains `"singleThread": true`).
+// This guarantees that `1.js` is linted before `2.js`.
+//
+// This rule throws an error during AST visitation when linting 1st file. If global state is not reset properly
+// before linting 2nd file, debug assertions will throw an error on 2nd file.
+//
+// In particular, we want to check that `ancestors` is cleared after an error while visiting a file.
+// If `ancestors` is cleared after that error, then `Program Program` visitor will not be called.
+// If it *is* called, then we have 2 `Program` nodes in `ancestors`, which is wrong
+// - some nodes from 1st file's AST remain in `ancestors` when we start walking AST of 2nd file.
+//
+// Actually `debugAssert(ancestors.length === 0)` before calling `walkProgram` in `src-js/plugins/lint.ts` should
+// throw in this case before the AST even gets walked, so the `"Program Program"` visitor should not be called anyway.
+// But including it just to make sure.
+
+let fileIndex = 0;
+
+const rule: Rule = {
+  create(context) {
+    const isFirstFileLinted = fileIndex === 0;
+    fileIndex++;
+
+    // Check the order files get linted in is what we expect
+    const isFirstFile = context.filename.endsWith("1.js");
+    assert(isFirstFile === isFirstFileLinted);
+
+    return {
+      Identifier(node) {
+        if (isFirstFile) throw new Error(`Identifier in 1st file: ${node.name}`);
+
+        context.report({
+          message: `Identifier in 2nd file: ${node.name}`,
+          node,
+        });
+      },
+
+      // This visitor should not be called.
+      // If it is, we have 2 `Program` nodes in `ancestors`.
+      "Program Program"(node) {
+        context.report({
+          message: "Ancestors has not been cleared after error",
+          node,
+        });
+      },
+    };
+  },
+};
 
 const plugin: Plugin = {
   meta: {
     name: "error-plugin",
   },
   rules: {
-    error: {
-      create(_context) {
-        return {
-          Identifier(_node) {
-            throw new Error("Whoops!");
-          },
-        };
-      },
-    },
+    error: rule,
   },
 };
 

tasks/ast_tools/src/generators/estree_visit.rs

@@ -203,6 +203,21 @@ fn generate(codegen: &Codegen) -> Codes {
 
         /* IF ANCESTORS */
         export const ancestors = [];
+
+        /**
+         * Check that `ancestors` array is kept in sync with traversal.
+         * This function is only included in debug build. Minifier will remove it in release build.
+         */
+        function debugCheckAncestorsOnExit(lenBefore, node) {
+            if (!DEBUG) return;
+            if (ancestors.length !== lenBefore) {
+                throw new Error(
+                    '`ancestors` is out of sync with traversal. '
+                    + `Its length has changed from ${lenBefore} to ${ancestors.length} `
+                    + `while visiting children of \\`${node.type}\\`.`
+                );
+            }
+        }
         /* END_IF */
 
         const { isArray } = Array;
@@ -265,14 +280,18 @@ fn generate(codegen: &Codegen) -> Codes {
                     if (enter !== null) enter(node);
                 }}
                 if (ANCESTORS) ancestors.unshift(node);
+                const ancestorsLen = ANCESTORS && DEBUG ? ancestors.length : 0;
             ");
 
             for key in &node.keys {
                 write_it!(walk_fn_body, "walkNode(node.{key}, visitors);\n");
             }
 
             walk_fn_body.push_str("
-                if (ANCESTORS) ancestors.shift();
+                if (ANCESTORS) {
+                    debugCheckAncestorsOnExit(ancestorsLen, node);
+                    ancestors.shift();
+                }
                 if (exit !== null) exit(node);
             ");