fix(linter/plugins): output errors thrown in JS plugins (#14096)

Previously any errors on JS side in JS plugins were silently ignored. Instead create a diagnostic including stack trace.

Author: overlookmotel

apps/oxlint/Cargo.toml

@@ -34,7 +34,7 @@ napi = { workspace = true, features = ["async"], optional = true }
 napi-derive = { workspace = true, optional = true }
 rayon = { workspace = true }
 rustc-hash = { workspace = true }
-serde = { workspace = true }
+serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
 simdutf8 = { workspace = true }
 tempfile = { workspace = true }

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

@@ -6,7 +6,7 @@ import {
 } from '../generated/constants.js';
 import { diagnostics, setupContextForFile } from './context.js';
 import { registeredRules } from './load.js';
-import { assertIs } from './utils.js';
+import { assertIs, getErrorMessage } from './utils.js';
 import { addVisitorToCompiled, compiledVisitor, finalizeCompiledVisitor, initCompiledVisitor } from './visitor.js';
 
 // Lazy implementation
@@ -43,8 +43,40 @@ const textDecoder = new TextDecoder('utf-8', { ignoreBOM: true });
 // Array of `after` hooks to run after traversal. This array reused for every file.
 const afterHooks: AfterHook[] = [];
 
-// Run rules on a file.
+/**
+ * Run rules on a file.
+ *
+ * Main logic is in separate function `lintFileImpl`, because V8 cannot optimize functions containing try/catch.
+ *
+ * @param filePath - Absolute path of file being linted
+ * @param bufferId - ID of buffer containing file data
+ * @param buffer - Buffer containing file data, or `null` if buffer with this ID was previously sent to JS
+ * @param ruleIds - IDs of rules to run on this file
+ * @returns JSON result
+ */
 export function lintFile(filePath: string, bufferId: number, buffer: Uint8Array | null, ruleIds: number[]): string {
+  try {
+    lintFileImpl(filePath, bufferId, buffer, ruleIds);
+    return JSON.stringify({ Success: diagnostics });
+  } catch (err) {
+    return JSON.stringify({ Failure: getErrorMessage(err) });
+  } finally {
+    diagnostics.length = 0;
+  }
+}
+
+/**
+ * Run rules on a file.
+ *
+ * @param filePath - Absolute path of file being linted
+ * @param bufferId - ID of buffer containing file data
+ * @param buffer - Buffer containing file data, or `null` if buffer with this ID was previously sent to JS
+ * @param ruleIds - IDs of rules to run on this file
+ * @returns Diagnostics to send back to Rust
+ * @throws {Error} If any parameters are invalid
+ * @throws {*} If any rule throws
+ */
+function lintFileImpl(filePath: string, bufferId: number, buffer: Uint8Array | null, ruleIds: number[]) {
   // If new buffer, add it to `buffers` array. Otherwise, get existing buffer from array.
   // Do this before checks below, to make sure buffer doesn't get garbage collected when not expected
   // if there's an error.
@@ -142,9 +174,4 @@ export function lintFile(filePath: string, bufferId: number, buffer: Uint8Array
     // Reset array, ready for next file
     afterHooks.length = 0;
   }
-
-  // Send diagnostics back to Rust
-  const ret = JSON.stringify(diagnostics);
-  diagnostics.length = 0;
-  return ret;
 }

apps/oxlint/src/js_plugins/external_linter.rs

@@ -5,6 +5,7 @@ use napi::{
     bindgen_prelude::{FnArgs, Uint8Array},
     threadsafe_function::ThreadsafeFunctionCallMode,
 };
+use serde::Deserialize;
 
 use oxc_allocator::{Allocator, free_fixed_size_allocator};
 use oxc_linter::{
@@ -48,6 +49,13 @@ fn wrap_load_plugin(cb: JsLoadPluginCb) -> ExternalLinterLoadPluginCb {
     })
 }
 
+/// Result returned by `lintFile` JS callback.
+#[derive(Clone, Debug, Deserialize)]
+pub enum LintFileReturnValue {
+    Success(Vec<LintFileResult>),
+    Failure(String),
+}
+
 /// Wrap `lintFile` JS callback as a normal Rust function.
 ///
 /// The returned function creates a `Uint8Array` referencing the memory of the given `Allocator`,
@@ -79,7 +87,7 @@ fn wrap_lint_file(cb: JsLintFileCb) -> ExternalLinterLintFileCb {
             ThreadsafeFunctionCallMode::NonBlocking,
             move |result, _env| {
                 let _ = match &result {
-                    Ok(r) => match serde_json::from_str::<Vec<LintFileResult>>(r) {
+                    Ok(r) => match serde_json::from_str::<LintFileReturnValue>(r) {
                         Ok(v) => tx.send(Ok(v)),
                         Err(_e) => tx.send(Err("Failed to deserialize lint result".to_string())),
                     },
@@ -90,14 +98,15 @@ fn wrap_lint_file(cb: JsLintFileCb) -> ExternalLinterLintFileCb {
             },
         );
 
-        if status != Status::Ok {
-            return Err(format!("Failed to schedule callback: {status:?}"));
-        }
+        assert!(status == Status::Ok, "Failed to schedule callback: {status:?}");
 
         match rx.recv() {
-            Ok(Ok(x)) => Ok(x),
-            Ok(Err(e)) => Err(format!("Callback reported error: {e}")),
-            Err(e) => Err(format!("Callback did not respond: {e}")),
+            Ok(Ok(x)) => match x {
+                LintFileReturnValue::Success(diagnostics) => Ok(diagnostics),
+                LintFileReturnValue::Failure(err) => Err(err),
+            },
+            Ok(Err(err)) => panic!("Callback reported error: {err}"),
+            Err(err) => panic!("Callback did not respond: {err}"),
         }
     })
 }

apps/oxlint/test/__snapshots__/e2e.test.ts.snap

@@ -690,6 +690,71 @@ exports[`oxlint CLI > should report an error if a custom plugin throws an error
 "
 `;
 
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in \`after\` hook 1`] = `
+"
+  x Error running JS plugin.
+  | File path: <root>/oxc/apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/index.js
+  | Error: Whoops!
+  |     at after (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/test_plugin/index.js:10:19)
+
+Found 0 warnings and 1 error.
+Finished in Xms on 1 file using X threads."
+`;
+
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in \`before\` hook 1`] = `
+"
+  x Error running JS plugin.
+  | File path: <root>/oxc/apps/oxlint/test/fixtures/custom_plugin_lint_before_hook_error/index.js
+  | Error: Whoops!
+  |     at before (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_before_hook_error/test_plugin/index.js:10:19)
+
+Found 0 warnings and 1 error.
+Finished in Xms on 1 file using X threads."
+`;
+
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in \`create\` method 1`] = `
+"
+  x Error running JS plugin.
+  | File path: <root>/oxc/apps/oxlint/test/fixtures/custom_plugin_lint_create_error/index.js
+  | Error: Whoops!
+  |     at Object.create (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_create_error/test_plugin/index.js:8:15)
+
+Found 0 warnings and 1 error.
+Finished in Xms on 1 file using X threads."
+`;
+
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in \`createOnce\` method 1`] = `
+"Failed to parse configuration file.
+
+  x Failed to load JS plugin: ./test_plugin
+  |   Error: Whoops!
+  |     at Object.createOnce (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_createOnce_error/test_plugin/index.js:8:15)
+"
+`;
+
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in \`fix\` function 1`] = `
+"
+  x Error running JS plugin.
+  | File path: <root>/oxc/apps/oxlint/test/fixtures/custom_plugin_lint_fix_error/index.js
+  | Error: Whoops!
+  |     at Object.fix (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_fix_error/test_plugin/index.js:14:23)
+  |     at Identifier (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_fix_error/test_plugin/index.js:10:21)
+
+Found 0 warnings and 1 error.
+Finished in Xms on 1 file using X threads."
+`;
+
+exports[`oxlint CLI > should report an error if a custom plugin throws an error during linting > in visit function 1`] = `
+"
+  x Error running JS plugin.
+  | File path: <root>/oxc/apps/oxlint/test/fixtures/custom_plugin_lint_visit_error/index.js
+  | Error: Whoops!
+  |     at Identifier (<root>/apps/oxlint/test/fixtures/custom_plugin_lint_visit_error/test_plugin/index.js:10:19)
+
+Found 0 warnings and 1 error.
+Finished in Xms on 1 file using X threads."
+`;
+
 exports[`oxlint CLI > should report an error if a rule is not found within a custom plugin 1`] = `
 "Failed to parse configuration file.
 

apps/oxlint/test/e2e.test.ts

@@ -6,6 +6,7 @@ import { describe, expect, it } from 'vitest';
 import { execa } from 'execa';
 
 const PACKAGE_ROOT_PATH = dirname(import.meta.dirname);
+const ROOT_PATH = pathJoin(PACKAGE_ROOT_PATH, '../../../');
 const CLI_PATH = pathJoin(PACKAGE_ROOT_PATH, 'dist/cli.js');
 const ROOT_URL = new URL('../../../', import.meta.url).href;
 const FIXTURES_URL = new URL('./fixtures/', import.meta.url).href;
@@ -37,12 +38,19 @@ function normalizeOutput(output: string): string {
   // Remove lines from stack traces which are outside `fixtures` directory.
   // Replace path to repo root in stack traces with `<root>`.
   lines = lines.flatMap((line) => {
-    // e.g. ` | at file:///path/to/oxc/apps/oxlint/test/fixtures/foor/bar.js:1:1`
-    // e.g. ` | at whatever (file:///path/to/oxc/apps/oxlint/test/fixtures/foor/bar.js:1:1)`
-    const match = line.match(/^(\s*\|\s+at (?:.+?\()?)(.+)$/);
+    // e.g. ` | at file:///path/to/oxc/apps/oxlint/test/fixtures/foo/bar.js:1:1`
+    // e.g. ` | at whatever (file:///path/to/oxc/apps/oxlint/test/fixtures/foo/bar.js:1:1)`
+    let match = line.match(/^(\s*\|\s+at (?:.+?\()?)(.+)$/);
     if (match) {
       const [, premable, at] = match;
       return at.startsWith(FIXTURES_URL) ? [`${premable}<root>/${at.slice(ROOT_URL.length)}`] : [];
+    } else {
+      // e.g. ` | File path: /path/to/oxc/apps/oxlint/test/fixtures/foo/bar.js`
+      match = line.match(/^(\s*\|\s+File path: )(.+)$/);
+      if (match) {
+        const [, premable, path] = match;
+        if (path.startsWith(ROOT_PATH)) return [`${premable}<root>/${path.slice(ROOT_PATH.length)}`];
+      }
     }
     return [line];
   });
@@ -129,6 +137,44 @@ describe('oxlint CLI', () => {
     expect(normalizeOutput(stdout)).toMatchSnapshot();
   });
 
+  describe('should report an error if a custom plugin throws an error during linting', () => {
+    it('in `create` method', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_create_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+
+    it('in `createOnce` method', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_createOnce_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+
+    it('in visit function', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_visit_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+
+    it('in `before` hook', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_before_hook_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+
+    it('in `after` hook', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_after_hook_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+
+    it('in `fix` function', async () => {
+      const { stdout, exitCode } = await runOxlint('test/fixtures/custom_plugin_lint_fix_error');
+      expect(exitCode).toBe(1);
+      expect(normalizeOutput(stdout)).toMatchSnapshot();
+    });
+  });
+
   it('should report the correct severity when using a custom plugin', async () => {
     const { stdout, exitCode } = await runOxlint('test/fixtures/basic_custom_plugin_warn_severity');
     expect(exitCode).toBe(0);

apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/.oxlintrc.json

@@ -0,0 +1,8 @@
+{
+  "plugins": ["./test_plugin"],
+  "categories": { "correctness": "off" },
+  "rules": {
+    "error-plugin/error": "error"
+  },
+  "ignorePatterns": ["test_plugin/**"]
+}

apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/index.js

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

apps/oxlint/test/fixtures/custom_plugin_lint_after_hook_error/test_plugin/index.js

@@ -0,0 +1,16 @@
+export default {
+  meta: {
+    name: "error-plugin",
+  },
+  rules: {
+    error: {
+      createOnce(_context) {
+        return {
+          after() {
+            throw new Error("Whoops!");
+          },
+        };
+      },
+    },
+  },
+};

apps/oxlint/test/fixtures/custom_plugin_lint_before_hook_error/.oxlintrc.json

@@ -0,0 +1,8 @@
+{
+  "plugins": ["./test_plugin"],
+  "categories": { "correctness": "off" },
+  "rules": {
+    "error-plugin/error": "error"
+  },
+  "ignorePatterns": ["test_plugin/**"]
+}

apps/oxlint/test/fixtures/custom_plugin_lint_before_hook_error/index.js

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