From cb21eac55618f2d8b4a9affd85e1723e339e558e Mon Sep 17 00:00:00 2001 From: Peter Wagenet Date: Mon, 17 Nov 2025 20:27:20 -0800 Subject: [PATCH 01/23] feat(linter/plugins): rule options --- apps/oxlint/src-js/bindings.d.ts | 11 +- apps/oxlint/src-js/cli.ts | 23 +++- apps/oxlint/src-js/plugins/config.ts | 16 +++ apps/oxlint/src-js/plugins/index.ts | 1 + apps/oxlint/src-js/plugins/lint.ts | 8 +- apps/oxlint/src-js/plugins/options.ts | 29 ++++- apps/oxlint/src/js_plugins/external_linter.rs | 42 +++++++- apps/oxlint/src/lint.rs | 14 ++- apps/oxlint/src/run.rs | 36 +++++-- .../custom_plugin_with_options/.oxlintrc.json | 17 +++ .../custom_plugin_with_options/files/index.js | 1 + .../custom_plugin_with_options/output.snap.md | 25 +++++ .../custom_plugin_with_options/plugin.ts | 29 +++++ .../src/linter/server_linter.rs | 13 +-- .../oxc_linter/src/config/config_builder.rs | 34 +++--- crates/oxc_linter/src/config/config_store.rs | 79 +++++++++++--- crates/oxc_linter/src/config/mod.rs | 4 +- crates/oxc_linter/src/config/rules.rs | 100 ++++++++++++++++-- crates/oxc_linter/src/external_linter.rs | 8 +- .../oxc_linter/src/external_plugin_store.rs | 39 ++++++- crates/oxc_linter/src/lib.rs | 10 +- crates/oxc_linter/src/tester.rs | 2 +- napi/playground/src/lib.rs | 8 +- tasks/benchmark/benches/linter.rs | 5 +- 24 files changed, 479 insertions(+), 75 deletions(-) create mode 100644 apps/oxlint/src-js/plugins/config.ts create mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json create mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js create mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md create mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts diff --git a/apps/oxlint/src-js/bindings.d.ts b/apps/oxlint/src-js/bindings.d.ts index f0a161bcdd837..0259264e048a1 100644 --- a/apps/oxlint/src-js/bindings.d.ts +++ b/apps/oxlint/src-js/bindings.d.ts @@ -10,23 +10,28 @@ export declare function getBufferOffset(buffer: Uint8Array): number /** JS callback to lint a file. */ export type JsLintFileCb = - ((arg0: string, arg1: number, arg2: Uint8Array | undefined | null, arg3: Array, arg4: string) => string) + ((arg0: string, arg1: number, arg2: Uint8Array | undefined | null, arg3: Array, arg4: Array, arg5: string) => string) /** JS callback to load a JS plugin. */ export type JsLoadPluginCb = ((arg0: string, arg1?: string | undefined | null) => Promise) +/** JS callback to setup configs. */ +export type JsSetupConfigsCb = + ((arg: string) => void) + /** * NAPI entry point. * * JS side passes in: * 1. `args`: Command line arguments (process.argv.slice(2)) * 2. `load_plugin`: Load a JS plugin from a file path. - * 3. `lint_file`: Lint a file. + * 3. `setup_configs`: Setup configuration options. + * 4. `lint_file`: Lint a file. * * Returns `true` if linting succeeded without errors, `false` otherwise. */ -export declare function lint(args: Array, loadPlugin: JsLoadPluginCb, lintFile: JsLintFileCb): Promise +export declare function lint(args: Array, loadPlugin: JsLoadPluginCb, setupConfigs: JsSetupConfigsCb, lintFile: JsLintFileCb): Promise /** * Parse AST into provided `Uint8Array` buffer, synchronously. diff --git a/apps/oxlint/src-js/cli.ts b/apps/oxlint/src-js/cli.ts index 34450e0f6c0c9..29a49ab330f85 100644 --- a/apps/oxlint/src-js/cli.ts +++ b/apps/oxlint/src-js/cli.ts @@ -5,6 +5,7 @@ import { debugAssertIsNonNull } from "./utils/asserts.js"; // Using `typeof wrapper` here makes TS check that the function signatures of `loadPlugin` and `loadPluginWrapper` // are identical. Ditto `lintFile` and `lintFileWrapper`. let loadPlugin: typeof loadPluginWrapper | null = null; +let setupConfigs: typeof setupConfigsWrapper | null = null; let lintFile: typeof lintFileWrapper | null = null; /** @@ -21,7 +22,7 @@ function loadPluginWrapper(path: string, packageName: string | null): Promise { - ({ loadPlugin, lintFile } = mod); + ({ loadPlugin, lintFile, setupConfigs } = mod); return loadPlugin(path, packageName); }); } @@ -29,6 +30,18 @@ function loadPluginWrapper(path: string, packageName: string | null): Promise = Object.freeze({}); * @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 + * @param optionsIds - IDs of options to use for rules on this file * @param settingsJSON - Settings for file, as JSON * @returns Diagnostics or error serialized to JSON string */ @@ -56,11 +57,9 @@ export function lintFile( bufferId: number, buffer: Uint8Array | null, ruleIds: number[], + optionsIds: number[], settingsJSON: string, ): string { - // TODO: Get `optionsIds` from Rust side - const optionsIds = ruleIds.map((_) => DEFAULT_OPTIONS_ID); - try { lintFileImpl(filePath, bufferId, buffer, ruleIds, optionsIds, settingsJSON); @@ -164,6 +163,9 @@ export function lintFileImpl( // Set `options` for rule const optionsId = optionsIds[i]; debugAssert(optionsId < allOptions.length, "Options ID out of bounds"); + + // If the rule has no user-provided options, use the plugin-provided default + // options (which falls back to `DEFAULT_OPTIONS`) ruleDetails.options = optionsId === DEFAULT_OPTIONS_ID ? ruleDetails.defaultOptions : allOptions[optionsId]; diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index fdb70c038a14b..a5e5058f2f3a2 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -22,8 +22,10 @@ export type Options = JsonValue[]; // Default rule options export const DEFAULT_OPTIONS: Readonly = Object.freeze([]); -// All rule options -export const allOptions: Readonly[] = [DEFAULT_OPTIONS]; +// All rule options. +// Indexed by options ID sent alongside ruleId for each file. +// Element 0 is always the default options (empty array). +export let allOptions: Readonly[] = [DEFAULT_OPTIONS]; // Index into `allOptions` for default options export const DEFAULT_OPTIONS_ID = 0; @@ -128,3 +130,26 @@ function mergeValues(configValue: JsonValue, defaultValue: JsonValue): JsonValue return freeze(merged); } + +/** + * Set all external rule options. + * Called once from Rust after config building, before any linting occurs. + * @param optionsJson - JSON string of outer array of per-options arrays. + */ +export function setOptions(optionsJson: string): void { + allOptions = JSON.parse(optionsJson); + if (DEBUG) { + if (!isArray(allOptions)) + throw new TypeError(`Expected optionsJson to decode to an array, got ${typeof allOptions}`); + // Basic shape validation: each element must be an array (options tuple array) + for (let i = 0; i < allOptions.length; i++) { + const el = allOptions[i]; + if (!isArray(el)) + throw new TypeError(`Each options entry must be an array, got ${typeof el}`); + } + } + // `allOptions`' type is `readonly`, which is less specific than the expected + // mutable `JsonValue[]` type, and therefore unassignable to it. + // The type assertion is safe because `allOptions` is mutable until this call. + deepFreezeArray(allOptions as JsonValue[]); +} diff --git a/apps/oxlint/src/js_plugins/external_linter.rs b/apps/oxlint/src/js_plugins/external_linter.rs index 59e6bd5c9f24d..56e886da15231 100644 --- a/apps/oxlint/src/js_plugins/external_linter.rs +++ b/apps/oxlint/src/js_plugins/external_linter.rs @@ -15,18 +15,20 @@ use oxc_linter::{ use crate::{ generated::raw_transfer_constants::{BLOCK_ALIGN, BUFFER_SIZE}, - run::{JsLintFileCb, JsLoadPluginCb}, + run::{JsLintFileCb, JsLoadPluginCb, JsSetupConfigsCb}, }; /// Wrap JS callbacks as normal Rust functions, and create [`ExternalLinter`]. pub fn create_external_linter( load_plugin: JsLoadPluginCb, + setup_configs: JsSetupConfigsCb, lint_file: JsLintFileCb, ) -> ExternalLinter { let rust_load_plugin = wrap_load_plugin(load_plugin); + let rust_setup_configs = wrap_setup_configs(setup_configs); let rust_lint_file = wrap_lint_file(lint_file); - ExternalLinter::new(rust_load_plugin, rust_lint_file) + ExternalLinter::new(rust_load_plugin, rust_setup_configs, rust_lint_file) } /// Wrap `loadPlugin` JS callback as a normal Rust function. @@ -59,6 +61,39 @@ pub enum LintFileReturnValue { Failure(String), } +/// Wrap `setupConfigs` JS callback as a normal Rust function. +/// +/// Use an `mpsc::channel` to wait for the result from JS side, and block current thread until `setupConfigs` +/// completes execution. +fn wrap_setup_configs( + cb: JsSetupConfigsCb, +) -> Box Result<(), String> + Send + Sync> { + Box::new(move |options_json: String| { + let (tx, rx) = channel(); + + // Send data to JS + let status = cb.call_with_return_value( + options_json, + ThreadsafeFunctionCallMode::NonBlocking, + move |result, _env| { + let _ = match &result { + Ok(()) => tx.send(Ok(())), + Err(e) => tx.send(Err(e.to_string())), + }; + result + }, + ); + + assert!(status == Status::Ok, "Failed to schedule setupConfigs callback: {status:?}"); + + match rx.recv() { + Ok(Ok(())) => Ok(()), + Ok(Err(err)) => Err(err), + Err(err) => panic!("setupConfigs callback did not respond: {err}"), + } + }) +} + /// Wrap `lintFile` JS callback as a normal Rust function. /// /// The returned function creates a `Uint8Array` referencing the memory of the given `Allocator`, @@ -72,6 +107,7 @@ fn wrap_lint_file(cb: JsLintFileCb) -> ExternalLinterLintFileCb { Box::new( move |file_path: String, rule_ids: Vec, + options_ids: Vec, settings_json: String, allocator: &Allocator| { let (tx, rx) = channel(); @@ -87,7 +123,7 @@ fn wrap_lint_file(cb: JsLintFileCb) -> ExternalLinterLintFileCb { // Send data to JS let status = cb.call_with_return_value( - FnArgs::from((file_path, buffer_id, buffer, rule_ids, settings_json)), + FnArgs::from((file_path, buffer_id, buffer, rule_ids, options_ids, settings_json)), ThreadsafeFunctionCallMode::NonBlocking, move |result, _env| { let _ = match &result { diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 07e13aa61e1ee..a10aae4a24158 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -277,7 +277,7 @@ impl CliRunner { || nested_configs.values().any(|config| config.plugins().has_import()); let mut options = LintServiceOptions::new(self.cwd).with_cross_module(use_cross_module); - let lint_config = match config_builder.build(&external_plugin_store) { + let lint_config = match config_builder.build(&mut external_plugin_store) { Ok(config) => config, Err(e) => { print_and_flush_stdout( @@ -291,6 +291,18 @@ impl CliRunner { } }; + // TODO: refactor this elsewhere. + // This code is in the oxlint app, not in oxc_linter crate + if let Some(external_linter) = &external_linter + && let Err(err) = external_plugin_store.setup_configs(external_linter) + { + print_and_flush_stdout( + stdout, + &format!("Failed to setup external plugin options: {err}\n"), + ); + return CliRunResult::InvalidOptionConfig; + } + let report_unused_directives = match inline_config_options.report_unused_directives { ReportUnusedDirectives::WithoutSeverity(true) => Some(AllowWarnDeny::Warn), ReportUnusedDirectives::WithSeverity(Some(severity)) => Some(severity), diff --git a/apps/oxlint/src/run.rs b/apps/oxlint/src/run.rs index ff572e0f478d9..6b818741c74cd 100644 --- a/apps/oxlint/src/run.rs +++ b/apps/oxlint/src/run.rs @@ -40,12 +40,28 @@ pub type JsLintFileCb = ThreadsafeFunction< u32, // Buffer ID Option, // Buffer (optional) Vec, // Array of rule IDs + Vec, // Array of options IDs String, // Stringified settings effective for the file )>, // Return value String, // `Vec`, serialized to JSON // Arguments (repeated) - FnArgs<(String, u32, Option, Vec, String)>, + FnArgs<(String, u32, Option, Vec, Vec, String)>, + // Error status + Status, + // CalleeHandled + false, +>; + +/// JS callback to setup configs. +#[napi] +pub type JsSetupConfigsCb = ThreadsafeFunction< + // Arguments + String, // Stringified options array + // Return value + (), // `void` + // Arguments (repeated) + String, // Error status Status, // CalleeHandled @@ -57,20 +73,27 @@ pub type JsLintFileCb = ThreadsafeFunction< /// JS side passes in: /// 1. `args`: Command line arguments (process.argv.slice(2)) /// 2. `load_plugin`: Load a JS plugin from a file path. -/// 3. `lint_file`: Lint a file. +/// 3. `setup_configs`: Setup configuration options. +/// 4. `lint_file`: Lint a file. /// /// Returns `true` if linting succeeded without errors, `false` otherwise. #[expect(clippy::allow_attributes)] #[allow(clippy::trailing_empty_array, clippy::unused_async)] // https://github.com/napi-rs/napi-rs/issues/2758 #[napi] -pub async fn lint(args: Vec, load_plugin: JsLoadPluginCb, lint_file: JsLintFileCb) -> bool { - lint_impl(args, load_plugin, lint_file).await.report() == ExitCode::SUCCESS +pub async fn lint( + args: Vec, + load_plugin: JsLoadPluginCb, + setup_configs: JsSetupConfigsCb, + lint_file: JsLintFileCb, +) -> bool { + lint_impl(args, load_plugin, setup_configs, lint_file).await.report() == ExitCode::SUCCESS } /// Run the linter. async fn lint_impl( args: Vec, load_plugin: JsLoadPluginCb, + setup_configs: JsSetupConfigsCb, lint_file: JsLintFileCb, ) -> CliRunResult { // Convert String args to OsString for compatibility with bpaf @@ -104,10 +127,11 @@ async fn lint_impl( // JS plugins are only supported on 64-bit little-endian platforms at present #[cfg(all(target_pointer_width = "64", target_endian = "little"))] - let external_linter = Some(crate::js_plugins::create_external_linter(load_plugin, lint_file)); + let external_linter = + Some(crate::js_plugins::create_external_linter(load_plugin, setup_configs, lint_file)); #[cfg(not(all(target_pointer_width = "64", target_endian = "little")))] let external_linter = { - let (_, _) = (load_plugin, lint_file); + let (_, _, _) = (load_plugin, setup_configs, lint_file); None }; diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json b/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json new file mode 100644 index 0000000000000..e4351dac06e8d --- /dev/null +++ b/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json @@ -0,0 +1,17 @@ +{ + "categories": { + "correctness": "off" + }, + "jsPlugins": [ + "./plugin.ts" + ], + "rules": { + "test-plugin-options/check-options": [ + "error", + true, + { + "expected": "options value" + } + ] + } +} diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js b/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js new file mode 100644 index 0000000000000..eab74692130a6 --- /dev/null +++ b/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js @@ -0,0 +1 @@ +debugger; diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md b/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md new file mode 100644 index 0000000000000..15102f183129c --- /dev/null +++ b/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md @@ -0,0 +1,25 @@ +# Exit code +1 + +# stdout +``` + x test-plugin-options(check-options): [ + | true, + | { + | "expected": "options value" + | } + | ] + ,-[files/index.js:1:1] + 1 | debugger; + : ^ + `---- + +Found 0 warnings and 1 error. +Finished in Xms on 1 file using X threads. +``` + +# stderr +``` +WARNING: JS plugins are experimental and not subject to semver. +Breaking changes are possible while JS plugins support is under development. +``` diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts b/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts new file mode 100644 index 0000000000000..dd9171db4e5fe --- /dev/null +++ b/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts @@ -0,0 +1,29 @@ +import { definePlugin } from "oxlint"; +import type { Node } from "#oxlint"; + +const SPAN: Node = { + start: 0, + end: 0, + range: [0, 0], + loc: { + start: { line: 0, column: 0 }, + end: { line: 0, column: 0 }, + }, +}; + +export default definePlugin({ + meta: { + name: "test-plugin-options", + }, + rules: { + "check-options": { + create(context) { + context.report({ + message: JSON.stringify(context.options, null, 2), + node: SPAN, + }); + return {}; + }, + }, + }, +}); diff --git a/crates/oxc_language_server/src/linter/server_linter.rs b/crates/oxc_language_server/src/linter/server_linter.rs index 6f0628bc3a02f..822a455b9b3da 100644 --- a/crates/oxc_language_server/src/linter/server_linter.rs +++ b/crates/oxc_language_server/src/linter/server_linter.rs @@ -90,9 +90,9 @@ impl ServerLinterBuilder { && nested_configs.pin().values().any(|config| config.plugins().has_import())); extended_paths.extend(config_builder.extended_paths.clone()); - let base_config = config_builder.build(&external_plugin_store).unwrap_or_else(|err| { + let base_config = config_builder.build(&mut external_plugin_store).unwrap_or_else(|err| { warn!("Failed to build config: {err}"); - ConfigStoreBuilder::empty().build(&external_plugin_store).unwrap() + ConfigStoreBuilder::empty().build(&mut external_plugin_store).unwrap() }); let lint_options = LintOptions { @@ -249,10 +249,11 @@ impl ServerLinterBuilder { continue; }; extended_paths.extend(config_store_builder.extended_paths.clone()); - let config = config_store_builder.build(&external_plugin_store).unwrap_or_else(|err| { - warn!("Failed to build nested config for {}: {:?}", dir_path.display(), err); - ConfigStoreBuilder::empty().build(&external_plugin_store).unwrap() - }); + let config = + config_store_builder.build(&mut external_plugin_store).unwrap_or_else(|err| { + warn!("Failed to build nested config for {}: {:?}", dir_path.display(), err); + ConfigStoreBuilder::empty().build(&mut external_plugin_store).unwrap() + }); nested_configs.pin().insert(dir_path.to_path_buf(), config); } diff --git a/crates/oxc_linter/src/config/config_builder.rs b/crates/oxc_linter/src/config/config_builder.rs index 88200ebacbcfc..b7765d2babdb4 100644 --- a/crates/oxc_linter/src/config/config_builder.rs +++ b/crates/oxc_linter/src/config/config_builder.rs @@ -17,7 +17,7 @@ use crate::{ ESLintRule, OxlintOverrides, OxlintRules, overrides::OxlintOverride, plugins::LintPlugins, }, external_linter::ExternalLinter, - external_plugin_store::{ExternalRuleId, ExternalRuleLookupError}, + external_plugin_store::{ExternalOptionsId, ExternalRuleId, ExternalRuleLookupError}, rules::RULES, }; @@ -30,7 +30,7 @@ use super::{ #[must_use = "You dropped your builder without building a Linter! Did you mean to call .build()?"] pub struct ConfigStoreBuilder { pub(super) rules: FxHashMap, - pub(super) external_rules: FxHashMap, + pub(super) external_rules: FxHashMap, config: LintConfig, categories: OxlintCategories, overrides: OxlintOverrides, @@ -386,7 +386,7 @@ impl ConfigStoreBuilder { /// Returns [`ConfigBuilderError::UnknownRules`] if there are rules that could not be matched. pub fn build( mut self, - external_plugin_store: &ExternalPluginStore, + external_plugin_store: &mut ExternalPluginStore, ) -> Result { // When a plugin gets disabled before build(), rules for that plugin aren't removed until // with_filters() gets called. If the user never calls it, those now-undesired rules need @@ -413,8 +413,14 @@ impl ConfigStoreBuilder { .collect(); rules.sort_unstable_by_key(|(r, _)| r.id()); - let mut external_rules: Vec<_> = self.external_rules.into_iter().collect(); - external_rules.sort_unstable_by_key(|(r, _)| *r); + // Convert HashMap entries (ExternalRuleId -> (ExternalOptionsId, AllowWarnDeny)) + // into Vec<(ExternalRuleId, ExternalOptionsId, AllowWarnDeny)> and sort by rule id. + let mut external_rules: Vec<_> = self + .external_rules + .into_iter() + .map(|(rule_id, (options_id, severity))| (rule_id, options_id, severity)) + .collect(); + external_rules.sort_unstable_by_key(|(r, _, _)| *r); Ok(Config::new(rules, external_rules, self.categories, self.config, resolved_overrides)) } @@ -422,7 +428,7 @@ impl ConfigStoreBuilder { fn resolve_overrides( &self, overrides: OxlintOverrides, - external_plugin_store: &ExternalPluginStore, + external_plugin_store: &mut ExternalPluginStore, ) -> Result { let resolved = overrides .into_iter() @@ -444,7 +450,11 @@ impl ConfigStoreBuilder { // Convert to vectors builtin_rules.extend(rules_map.into_iter()); - external_rules.extend(external_rules_map.into_iter()); + external_rules.extend( + external_rules_map + .into_iter() + .map(|(rule_id, (options_id, severity))| (rule_id, options_id, severity)), + ); Ok(ResolvedOxlintOverride { files: override_config.files, @@ -814,10 +824,10 @@ mod test { let mut desired_plugins = LintPlugins::default(); desired_plugins.set(LintPlugins::TYPESCRIPT, false); - let external_plugin_store = ExternalPluginStore::default(); + let mut external_plugin_store = ExternalPluginStore::default(); let linter = ConfigStoreBuilder::default() .with_builtin_plugins(desired_plugins) - .build(&external_plugin_store) + .build(&mut external_plugin_store) .unwrap(); for (rule, _) in linter.base.rules.iter() { let name = rule.name(); @@ -1262,7 +1272,7 @@ mod test { ) .unwrap(); - let config = builder.build(&external_plugin_store).unwrap(); + let config = builder.build(&mut external_plugin_store).unwrap(); // Apply overrides for a foo.test.ts file (matches both overrides) let resolved = config.apply_overrides(Path::new("foo.test.ts")); @@ -1288,7 +1298,7 @@ mod test { &mut external_plugin_store, ) .unwrap() - .build(&external_plugin_store) + .build(&mut external_plugin_store) .unwrap() } @@ -1301,7 +1311,7 @@ mod test { &mut external_plugin_store, ) .unwrap() - .build(&external_plugin_store) + .build(&mut external_plugin_store) .unwrap() } } diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index 0b94ad4944221..b34faa5f15df0 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -7,7 +7,7 @@ use rustc_hash::FxHashMap; use crate::{ AllowWarnDeny, - external_plugin_store::{ExternalPluginStore, ExternalRuleId}, + external_plugin_store::{ExternalOptionsId, ExternalPluginStore, ExternalRuleId}, rules::{RULES, RuleEnum}, }; @@ -23,7 +23,7 @@ pub struct ResolvedLinterState { pub rules: Arc<[(RuleEnum, AllowWarnDeny)]>, pub config: Arc, - pub external_rules: Arc<[(ExternalRuleId, AllowWarnDeny)]>, + pub external_rules: Arc<[(ExternalRuleId, ExternalOptionsId, AllowWarnDeny)]>, } #[derive(Debug, Default, Clone)] @@ -55,7 +55,7 @@ pub struct ResolvedOxlintOverride { #[derive(Debug, Clone)] pub struct ResolvedOxlintOverrideRules { pub(crate) builtin_rules: Vec<(RuleEnum, AllowWarnDeny)>, - pub(crate) external_rules: Vec<(ExternalRuleId, AllowWarnDeny)>, + pub(crate) external_rules: Vec<(ExternalRuleId, ExternalOptionsId, AllowWarnDeny)>, } #[derive(Debug, Clone)] @@ -80,7 +80,7 @@ pub struct Config { impl Config { pub fn new( rules: Vec<(RuleEnum, AllowWarnDeny)>, - mut external_rules: Vec<(ExternalRuleId, AllowWarnDeny)>, + mut external_rules: Vec<(ExternalRuleId, ExternalOptionsId, AllowWarnDeny)>, categories: OxlintCategories, config: LintConfig, overrides: ResolvedOxlintOverrides, @@ -97,7 +97,7 @@ impl Config { ), config: Arc::new(config), external_rules: Arc::from({ - external_rules.retain(|(_, sev)| sev.is_warn_deny()); + external_rules.retain(|(_, _, sev)| sev.is_warn_deny()); external_rules.into_boxed_slice() }), }, @@ -174,8 +174,13 @@ impl Config { .cloned() .collect::>(); - let mut external_rules = - self.base.external_rules.iter().copied().collect::>(); + // Build a hashmap of existing external rules keyed by rule id with value (options_id, severity) + let mut external_rules: FxHashMap = + self.base + .external_rules + .iter() + .map(|(rule_id, options_id, severity)| (*rule_id, (*options_id, *severity))) + .collect(); // Track which plugins have already had their category rules applied. // Start with the root plugins since they already have categories applied in base_rules. @@ -219,8 +224,8 @@ impl Config { } } - for (external_rule_id, severity) in &override_config.rules.external_rules { - external_rules.insert(*external_rule_id, *severity); + for (external_rule_id, options_id, severity) in &override_config.rules.external_rules { + external_rules.insert(*external_rule_id, (*options_id, *severity)); } if let Some(override_env) = &override_config.env { @@ -253,7 +258,8 @@ impl Config { let external_rules = external_rules .into_iter() - .filter(|(_, severity)| severity.is_warn_deny()) + .filter(|(_, (_, severity))| severity.is_warn_deny()) + .map(|(rule_id, (options_id, severity))| (rule_id, options_id, severity)) .collect::>(); ResolvedLinterState { @@ -356,7 +362,7 @@ mod test { use super::{ConfigStore, ResolvedOxlintOverrides}; use crate::{ - AllowWarnDeny, ExternalPluginStore, LintPlugins, RuleCategory, RuleEnum, + AllowWarnDeny, ExternalOptionsId, ExternalPluginStore, LintPlugins, RuleCategory, RuleEnum, config::{ LintConfig, OxlintEnv, OxlintGlobals, OxlintSettings, categories::OxlintCategories, @@ -707,7 +713,7 @@ mod test { let store = ConfigStore::new( Config::new( vec![], - vec![(rule_id, AllowWarnDeny::Deny)], + vec![(rule_id, ExternalOptionsId::NONE, AllowWarnDeny::Deny)], OxlintCategories::default(), LintConfig::default(), overrides, @@ -1052,4 +1058,53 @@ mod test { assert_eq!(store_with_nested_configs.number_of_rules(false), None); assert_eq!(store_with_nested_configs.number_of_rules(true), None); } + + #[test] + fn test_external_rule_options_override_precedence() { + // Prepare external plugin store with a custom plugin and rule + let mut store = ExternalPluginStore::new(true); + store.register_plugin( + "path/to/custom".to_string().into(), + "custom".to_string(), + 0, + vec!["my-rule".to_string()], + ); + + // Base config has external rule with options A, severity warn + let base_external_rule_id = store.lookup_rule_id("custom", "my-rule").unwrap(); + let base_options_id = store.add_options(serde_json::json!([{ "opt": "A" }])); + + let base = Config::new( + vec![], + vec![(base_external_rule_id, base_options_id, AllowWarnDeny::Warn)], + OxlintCategories::default(), + LintConfig::default(), + ResolvedOxlintOverrides::new(vec![ResolvedOxlintOverride { + files: GlobSet::new(vec!["*.js"]), + env: None, + globals: None, + plugins: None, + // Override redefines the same rule with options B and severity error + rules: ResolvedOxlintOverrideRules { + builtin_rules: vec![], + external_rules: vec![( + base_external_rule_id, + store.add_options(serde_json::json!([{ "opt": "B" }])), + AllowWarnDeny::Deny, + )], + }, + }]), + ); + + let config_store = ConfigStore::new(base, FxHashMap::default(), store); + let resolved = config_store.resolve(&PathBuf::from_str("/root/a.js").unwrap()); + + // Should prefer override (options B, severity error) + assert_eq!(resolved.external_rules.len(), 1); + let (rid, opts_id, sev) = resolved.external_rules[0]; + assert_eq!(rid, base_external_rule_id); + assert!(sev.is_warn_deny()); + // opts_id should not equal the base options id + assert_ne!(opts_id, base_options_id); + } } diff --git a/crates/oxc_linter/src/config/mod.rs b/crates/oxc_linter/src/config/mod.rs index 84c5ce2902bbc..4dda1aa486788 100644 --- a/crates/oxc_linter/src/config/mod.rs +++ b/crates/oxc_linter/src/config/mod.rs @@ -114,14 +114,14 @@ mod test { let config = Oxlintrc::from_file(&fixture_path).unwrap(); let mut set = FxHashMap::default(); let mut external_rules_for_override = FxHashMap::default(); - let external_linter_store = ExternalPluginStore::default(); + let mut external_linter_store = ExternalPluginStore::default(); config .rules .override_rules( &mut set, &mut external_rules_for_override, &RULES, - &external_linter_store, + &mut external_linter_store, ) .unwrap(); diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index d42bb08e3c7bc..7c70ad8b16308 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -13,7 +13,7 @@ use oxc_diagnostics::{Error, OxcDiagnostic}; use crate::{ AllowWarnDeny, ExternalPluginStore, LintPlugins, - external_plugin_store::{ExternalRuleId, ExternalRuleLookupError}, + external_plugin_store::{ExternalOptionsId, ExternalRuleId, ExternalRuleLookupError}, rules::{RULES, RuleEnum}, utils::{is_eslint_rule_adapted_to_typescript, is_jest_rule_adapted_to_vitest}, }; @@ -63,9 +63,12 @@ impl OxlintRules { pub(crate) fn override_rules( &self, rules_for_override: &mut RuleSet, - external_rules_for_override: &mut FxHashMap, + external_rules_for_override: &mut FxHashMap< + ExternalRuleId, + (ExternalOptionsId, AllowWarnDeny), + >, all_rules: &[RuleEnum], - external_plugin_store: &ExternalPluginStore, + external_plugin_store: &mut ExternalPluginStore, ) -> Result<(), ExternalRuleLookupError> { let mut rules_to_replace = vec![]; @@ -106,10 +109,22 @@ impl OxlintRules { if external_plugin_store.is_enabled() { let external_rule_id = external_plugin_store.lookup_rule_id(plugin_name, rule_name)?; + + // Add options to store and get options ID + let options_id = if let Some(config) = &rule_config.config { + external_plugin_store.add_options(config.clone()) + } else { + // No options - use reserved index 0 + ExternalOptionsId::NONE + }; + external_rules_for_override .entry(external_rule_id) - .and_modify(|sev| *sev = severity) - .or_insert(severity); + .and_modify(|(opts_id, sev)| { + *opts_id = options_id; + *sev = severity; + }) + .or_insert((options_id, severity)); } } } @@ -329,6 +344,7 @@ mod test { use serde::Deserialize; use serde_json::{Value, json}; + use crate::external_plugin_store::ExternalOptionsId; use crate::{ AllowWarnDeny, ExternalPluginStore, rules::{RULES, RuleEnum}, @@ -381,9 +397,14 @@ mod test { fn r#override(rules: &mut RuleSet, rules_rc: &Value) { let rules_config = OxlintRules::deserialize(rules_rc).unwrap(); let mut external_rules_for_override = FxHashMap::default(); - let external_linter_store = ExternalPluginStore::default(); + let mut external_linter_store = ExternalPluginStore::default(); rules_config - .override_rules(rules, &mut external_rules_for_override, &RULES, &external_linter_store) + .override_rules( + rules, + &mut external_rules_for_override, + &RULES, + &mut external_linter_store, + ) .unwrap(); } @@ -517,4 +538,69 @@ mod test { assert_eq!(r2.plugin_name, "unicorn"); assert!(r2.severity.is_warn_deny()); } + + #[test] + fn test_external_rule_options_are_recorded() { + // Register a fake external plugin and rule + let mut store = ExternalPluginStore::new(true); + store.register_plugin( + "path/to/custom-plugin".to_string().into(), + "custom".to_string(), + 0, + vec!["my-rule".to_string()], + ); + + // Configure rule with options array (non-empty) and ensure options id != 0 + let rules = OxlintRules::deserialize(&json!({ + "custom/my-rule": ["warn", {"foo": 1}] + })) + .unwrap(); + + let mut builtin_rules = RuleSet::default(); + let mut external_rules = FxHashMap::default(); + rules.override_rules(&mut builtin_rules, &mut external_rules, &[], &mut store).unwrap(); + + assert_eq!(builtin_rules.len(), 0); + assert_eq!(external_rules.len(), 1); + let (_rule_id, (options_id, severity)) = + external_rules.iter().next().map(|(k, v)| (*k, *v)).unwrap(); + assert_ne!( + options_id, + ExternalOptionsId::NONE, + "non-empty options should allocate a new id" + ); + assert!(severity.is_warn_deny()); + + // Now configure with no options which should map to reserved index 0 + let rules_no_opts = OxlintRules::deserialize(&json!({ + "custom/my-rule": "error" + })) + .unwrap(); + let mut builtin_rules2 = RuleSet::default(); + let mut external_rules2 = FxHashMap::default(); + rules_no_opts + .override_rules(&mut builtin_rules2, &mut external_rules2, &[], &mut store) + .unwrap(); + let (_rid2, (options_id2, severity2)) = + external_rules2.iter().next().map(|(k, v)| (*k, *v)).unwrap(); + assert_eq!(options_id2, ExternalOptionsId::NONE, "no options should use reserved id 0"); + assert!(severity2.is_warn_deny()); + + // Test that null config values also map to reserved index 0 + // This tests the case where config might be explicitly null (though unlikely in practice) + let null_options_id = store.add_options(serde_json::Value::Null); + assert_eq!( + null_options_id, + ExternalOptionsId::NONE, + "null options should use reserved id 0" + ); + + // Test that empty array also maps to reserved index 0 + let empty_array_id = store.add_options(serde_json::json!([])); + assert_eq!( + empty_array_id, + ExternalOptionsId::NONE, + "empty array options should use reserved id 0" + ); + } } diff --git a/crates/oxc_linter/src/external_linter.rs b/crates/oxc_linter/src/external_linter.rs index d9f3a213444fe..5d2c4b31ef9b2 100644 --- a/crates/oxc_linter/src/external_linter.rs +++ b/crates/oxc_linter/src/external_linter.rs @@ -10,8 +10,10 @@ pub type ExternalLinterLoadPluginCb = Box< + Sync, >; +pub type ExternalLinterSetupConfigsCb = Box Result<(), String> + Send + Sync>; + pub type ExternalLinterLintFileCb = Box< - dyn Fn(String, Vec, String, &Allocator) -> Result, String> + dyn Fn(String, Vec, Vec, String, &Allocator) -> Result, String> + Sync + Send, >; @@ -46,15 +48,17 @@ pub struct JsFix { pub struct ExternalLinter { pub(crate) load_plugin: ExternalLinterLoadPluginCb, + pub(crate) setup_configs: ExternalLinterSetupConfigsCb, pub(crate) lint_file: ExternalLinterLintFileCb, } impl ExternalLinter { pub fn new( load_plugin: ExternalLinterLoadPluginCb, + setup_configs: ExternalLinterSetupConfigsCb, lint_file: ExternalLinterLintFileCb, ) -> Self { - Self { load_plugin, lint_file } + Self { load_plugin, setup_configs, lint_file } } } diff --git a/crates/oxc_linter/src/external_plugin_store.rs b/crates/oxc_linter/src/external_plugin_store.rs index 6b57bb0792136..9fa934903a257 100644 --- a/crates/oxc_linter/src/external_plugin_store.rs +++ b/crates/oxc_linter/src/external_plugin_store.rs @@ -5,7 +5,9 @@ use std::{ use rustc_hash::{FxHashMap, FxHashSet}; -use oxc_index::{IndexVec, define_index_type}; +use oxc_index::{IndexVec, define_index_type, index_vec}; + +use crate::ExternalLinter; define_index_type! { pub struct ExternalPluginId = u32; @@ -15,6 +17,16 @@ define_index_type! { pub struct ExternalRuleId = u32; } +define_index_type! { + pub struct ExternalOptionsId = u32; +} + +impl ExternalOptionsId { + /// The value `0`. + /// Used as the ID when a rule does not have options. + pub const NONE: Self = Self::from_usize(0); +} + #[derive(Debug)] pub struct ExternalPluginStore { registered_plugin_paths: FxHashSet, @@ -22,6 +34,7 @@ pub struct ExternalPluginStore { plugins: IndexVec, plugin_names: FxHashMap, rules: IndexVec, + options: IndexVec, // `true` for `oxlint`, `false` for language server is_enabled: bool, @@ -35,11 +48,14 @@ impl Default for ExternalPluginStore { impl ExternalPluginStore { pub fn new(is_enabled: bool) -> Self { + let options = index_vec![serde_json::json!([])]; + Self { registered_plugin_paths: FxHashSet::default(), plugins: IndexVec::default(), plugin_names: FxHashMap::default(), rules: IndexVec::default(), + options, is_enabled, } } @@ -119,6 +135,27 @@ impl ExternalPluginStore { let plugin = &self.plugins[external_rule.plugin_id]; (&plugin.name, &external_rule.name) } + + /// Add options to the store and return its ID. + /// Returns index 0 for empty arrays or null values (no options). + pub fn add_options(&mut self, options: serde_json::Value) -> ExternalOptionsId { + // If it's null or an empty array, return reserved index 0 + if options.is_null() || options.as_array().is_some_and(Vec::is_empty) { + return ExternalOptionsId::from_usize(0); + } + + self.options.push(options) + } + + /// # Errors + /// Returns an error if serialization of rule options fails. + pub fn setup_configs(&self, external_linter: &ExternalLinter) -> Result<(), String> { + let json = serde_json::to_string(&self.options); + match json { + Ok(options_json) => (external_linter.setup_configs)(options_json), + Err(err) => Err(format!("Failed to serialize external plugin options: {err}")), + } + } } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index de5dc48e458ff..27c3effd94486 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -63,7 +63,7 @@ pub use crate::{ ExternalLinter, ExternalLinterLintFileCb, ExternalLinterLoadPluginCb, JsFix, LintFileResult, PluginLoadResult, }, - external_plugin_store::{ExternalPluginStore, ExternalRuleId}, + external_plugin_store::{ExternalOptionsId, ExternalPluginStore, ExternalRuleId}, fixer::{Fix, FixKind, Message, PossibleFixes}, frameworks::FrameworkFlags, lint_runner::{DirectivesStore, LintRunner, LintRunnerBuilder}, @@ -384,7 +384,7 @@ impl Linter { fn run_external_rules<'a>( &self, - external_rules: &[(ExternalRuleId, AllowWarnDeny)], + external_rules: &[(ExternalRuleId, ExternalOptionsId, AllowWarnDeny)], path: &Path, ctx_host: &mut Rc>, allocator: &'a Allocator, @@ -461,9 +461,11 @@ impl Linter { None => "{}".to_string(), }; + // Pass AST and rule IDs + options IDs to JS let result = (external_linter.lint_file)( path.to_string_lossy().into_owned(), - external_rules.iter().map(|(rule_id, _)| rule_id.raw()).collect(), + external_rules.iter().map(|(rule_id, _, _)| rule_id.raw()).collect(), + external_rules.iter().map(|(_, options_id, _)| options_id.raw()).collect(), settings_json, allocator, ); @@ -477,7 +479,7 @@ impl Linter { let mut span = Span::new(diagnostic.start, diagnostic.end); span_converter.convert_span_back(&mut span); - let (external_rule_id, severity) = + let (external_rule_id, _options_id, severity) = external_rules[diagnostic.rule_index as usize]; let (plugin_name, rule_name) = self.config.resolve_plugin_rule_names(external_rule_id); diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 759fc521c01ed..4a65830a32baf 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -505,7 +505,7 @@ impl Tester { }), ) .with_rule(rule, AllowWarnDeny::Warn) - .build(&external_plugin_store) + .build(&mut external_plugin_store) .unwrap(), FxHashMap::default(), external_plugin_store, diff --git a/napi/playground/src/lib.rs b/napi/playground/src/lib.rs index d5e1e56b98467..56b028355f085 100644 --- a/napi/playground/src/lib.rs +++ b/napi/playground/src/lib.rs @@ -375,7 +375,7 @@ impl Oxc { ) { // Only lint if there are no syntax errors if run_options.lint && self.diagnostics.is_empty() { - let external_plugin_store = ExternalPluginStore::default(); + let mut external_plugin_store = ExternalPluginStore::default(); let semantic_ret = SemanticBuilder::new().with_cfg(true).build(program); let semantic = semantic_ret.semantic; let lint_config = if linter_options.config.is_some() { @@ -386,12 +386,12 @@ impl Oxc { false, oxlintrc, None, - &mut ExternalPluginStore::default(), + &mut external_plugin_store, ) .unwrap_or_default(); - config_builder.build(&external_plugin_store) + config_builder.build(&mut external_plugin_store) } else { - ConfigStoreBuilder::default().build(&external_plugin_store) + ConfigStoreBuilder::default().build(&mut external_plugin_store) }; let lint_config = lint_config.unwrap(); let linter_ret = Linter::new( diff --git a/tasks/benchmark/benches/linter.rs b/tasks/benchmark/benches/linter.rs index bcea7bdde3af1..e4bcdb5298836 100644 --- a/tasks/benchmark/benches/linter.rs +++ b/tasks/benchmark/benches/linter.rs @@ -38,8 +38,9 @@ fn bench_linter(criterion: &mut Criterion) { let semantic = semantic_ret.semantic; let module_record = Arc::new(ModuleRecord::new(path, &parser_ret.module_record, &semantic)); - let external_plugin_store = ExternalPluginStore::default(); - let lint_config = ConfigStoreBuilder::all().build(&external_plugin_store).unwrap(); + let mut external_plugin_store = ExternalPluginStore::default(); + let lint_config = + ConfigStoreBuilder::all().build(&mut external_plugin_store).unwrap(); let linter = Linter::new( LintOptions::default(), ConfigStore::new(lint_config, FxHashMap::default(), external_plugin_store), From 449f47c20e642ea2f4aaad92b3b707e07602527e Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 09:51:36 +0000 Subject: [PATCH 02/23] docs: update comments --- apps/oxlint/src-js/cli.ts | 4 ++-- apps/oxlint/src-js/plugins/config.ts | 2 +- apps/oxlint/src-js/plugins/lint.ts | 4 ++-- apps/oxlint/src-js/plugins/options.ts | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/oxlint/src-js/cli.ts b/apps/oxlint/src-js/cli.ts index 29a49ab330f85..299bb910f69f9 100644 --- a/apps/oxlint/src-js/cli.ts +++ b/apps/oxlint/src-js/cli.ts @@ -35,7 +35,7 @@ function loadPluginWrapper(path: string, packageName: string | null): Promise = Object.freeze({}); * @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 - * @param optionsIds - IDs of options to use for rules on this file + * @param optionsIds - IDs of options to use for rules on this file, in same order as `ruleIds` * @param settingsJSON - Settings for file, as JSON * @returns Diagnostics or error serialized to JSON string */ @@ -84,7 +84,7 @@ export function lintFile( * @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 - * @param optionsIds - IDs of options to use for rules on this file + * @param optionsIds - IDs of options to use for rules on this file, in same order as `ruleIds` * @param settingsJSON - Stringified settings for this file * @throws {Error} If any parameters are invalid * @throws {*} If any rule throws diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index a5e5058f2f3a2..32ff2317c9372 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -23,7 +23,7 @@ export type Options = JsonValue[]; export const DEFAULT_OPTIONS: Readonly = Object.freeze([]); // All rule options. -// Indexed by options ID sent alongside ruleId for each file. +// Indexed by options ID sent alongside rule ID for each file. // Element 0 is always the default options (empty array). export let allOptions: Readonly[] = [DEFAULT_OPTIONS]; @@ -134,7 +134,7 @@ function mergeValues(configValue: JsonValue, defaultValue: JsonValue): JsonValue /** * Set all external rule options. * Called once from Rust after config building, before any linting occurs. - * @param optionsJson - JSON string of outer array of per-options arrays. + * @param optionsJSON - Array of all rule options across all configurations, serialized as JSON */ export function setOptions(optionsJson: string): void { allOptions = JSON.parse(optionsJson); From 793bc49d05646c1a33b6f23a80a28ab2376bfb03 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 09:58:54 +0000 Subject: [PATCH 03/23] style: formatting --- apps/oxlint/src-js/plugins/options.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index 32ff2317c9372..1a9aba60f645a 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -138,16 +138,20 @@ function mergeValues(configValue: JsonValue, defaultValue: JsonValue): JsonValue */ export function setOptions(optionsJson: string): void { allOptions = JSON.parse(optionsJson); + + // Validate that `allOptions` is an array of arrays if (DEBUG) { - if (!isArray(allOptions)) + if (!isArray(allOptions)) { throw new TypeError(`Expected optionsJson to decode to an array, got ${typeof allOptions}`); - // Basic shape validation: each element must be an array (options tuple array) + } for (let i = 0; i < allOptions.length; i++) { const el = allOptions[i]; - if (!isArray(el)) + if (!isArray(el)) { throw new TypeError(`Each options entry must be an array, got ${typeof el}`); + } } } + // `allOptions`' type is `readonly`, which is less specific than the expected // mutable `JsonValue[]` type, and therefore unassignable to it. // The type assertion is safe because `allOptions` is mutable until this call. From e4c82ce084b72d4124c53c2a3b2794d38053c68e Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:00:04 +0000 Subject: [PATCH 04/23] style: use `for ... of` loop This code is debug-only, so perf doesn't matter. --- apps/oxlint/src-js/plugins/options.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index 1a9aba60f645a..4a5e380536424 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -144,10 +144,9 @@ export function setOptions(optionsJson: string): void { if (!isArray(allOptions)) { throw new TypeError(`Expected optionsJson to decode to an array, got ${typeof allOptions}`); } - for (let i = 0; i < allOptions.length; i++) { - const el = allOptions[i]; - if (!isArray(el)) { - throw new TypeError(`Each options entry must be an array, got ${typeof el}`); + for (const options of allOptions) { + if (!isArray(options)) { + throw new TypeError(`Each options entry must be an array, got ${typeof options}`); } } } From 5769d4ea23dab9bae223581f1d2ec8aef68f05bb Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:02:42 +0000 Subject: [PATCH 05/23] docs: shorten comment --- apps/oxlint/src-js/plugins/options.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/oxlint/src-js/plugins/options.ts b/apps/oxlint/src-js/plugins/options.ts index 4a5e380536424..98827cdcd339f 100644 --- a/apps/oxlint/src-js/plugins/options.ts +++ b/apps/oxlint/src-js/plugins/options.ts @@ -151,8 +151,6 @@ export function setOptions(optionsJson: string): void { } } - // `allOptions`' type is `readonly`, which is less specific than the expected - // mutable `JsonValue[]` type, and therefore unassignable to it. - // The type assertion is safe because `allOptions` is mutable until this call. + // `allOptions`' type is `Readonly`, but the array is mutable until after this call deepFreezeArray(allOptions as JsonValue[]); } From a62ff03608c889f4b03909b953d5df2bfc104f84 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:06:32 +0000 Subject: [PATCH 06/23] refactor: export `ExternalLinterSetupConfigsCb` from `oxc_linter` crate --- apps/oxlint/src/js_plugins/external_linter.rs | 8 +++----- crates/oxc_linter/src/lib.rs | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/apps/oxlint/src/js_plugins/external_linter.rs b/apps/oxlint/src/js_plugins/external_linter.rs index 56e886da15231..9169acca0524a 100644 --- a/apps/oxlint/src/js_plugins/external_linter.rs +++ b/apps/oxlint/src/js_plugins/external_linter.rs @@ -9,8 +9,8 @@ use serde::Deserialize; use oxc_allocator::{Allocator, free_fixed_size_allocator}; use oxc_linter::{ - ExternalLinter, ExternalLinterLintFileCb, ExternalLinterLoadPluginCb, LintFileResult, - PluginLoadResult, + ExternalLinter, ExternalLinterLintFileCb, ExternalLinterLoadPluginCb, + ExternalLinterSetupConfigsCb, LintFileResult, PluginLoadResult, }; use crate::{ @@ -65,9 +65,7 @@ pub enum LintFileReturnValue { /// /// Use an `mpsc::channel` to wait for the result from JS side, and block current thread until `setupConfigs` /// completes execution. -fn wrap_setup_configs( - cb: JsSetupConfigsCb, -) -> Box Result<(), String> + Send + Sync> { +fn wrap_setup_configs(cb: JsSetupConfigsCb) -> ExternalLinterSetupConfigsCb { Box::new(move |options_json: String| { let (tx, rx) = channel(); diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 27c3effd94486..613d29a2f2d76 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -60,8 +60,8 @@ pub use crate::{ }, context::{ContextSubHost, LintContext}, external_linter::{ - ExternalLinter, ExternalLinterLintFileCb, ExternalLinterLoadPluginCb, JsFix, - LintFileResult, PluginLoadResult, + ExternalLinter, ExternalLinterLintFileCb, ExternalLinterLoadPluginCb, + ExternalLinterSetupConfigsCb, JsFix, LintFileResult, PluginLoadResult, }, external_plugin_store::{ExternalOptionsId, ExternalPluginStore, ExternalRuleId}, fixer::{Fix, FixKind, Message, PossibleFixes}, From a91c4f9315f1ef950a14929c783f3be805a5b39d Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:11:10 +0000 Subject: [PATCH 07/23] docs: revise comment --- apps/oxlint/src/js_plugins/external_linter.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/oxlint/src/js_plugins/external_linter.rs b/apps/oxlint/src/js_plugins/external_linter.rs index 9169acca0524a..cca811da4bf22 100644 --- a/apps/oxlint/src/js_plugins/external_linter.rs +++ b/apps/oxlint/src/js_plugins/external_linter.rs @@ -63,8 +63,9 @@ pub enum LintFileReturnValue { /// Wrap `setupConfigs` JS callback as a normal Rust function. /// -/// Use an `mpsc::channel` to wait for the result from JS side, and block current thread until `setupConfigs` -/// completes execution. +/// The JS-side `setupConfigs` function is synchronous, but it's wrapped in a `ThreadsafeFunction`, +/// so cannot be called synchronously. Use an `mpsc::channel` to wait for the result from JS side, +/// and block current thread until `setupConfigs` completes execution. fn wrap_setup_configs(cb: JsSetupConfigsCb) -> ExternalLinterSetupConfigsCb { Box::new(move |options_json: String| { let (tx, rx) = channel(); From d516f76c896635f2335f9b600a3ce1e40aae90fc Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:13:09 +0000 Subject: [PATCH 08/23] docs: revise comment --- apps/oxlint/src/run.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/oxlint/src/run.rs b/apps/oxlint/src/run.rs index 6b818741c74cd..1fe8a2a858017 100644 --- a/apps/oxlint/src/run.rs +++ b/apps/oxlint/src/run.rs @@ -57,7 +57,7 @@ pub type JsLintFileCb = ThreadsafeFunction< #[napi] pub type JsSetupConfigsCb = ThreadsafeFunction< // Arguments - String, // Stringified options array + String, // Options array, as JSON string // Return value (), // `void` // Arguments (repeated) From 330604e980ea3135875dbeb26ae12fa20839fd9d Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:48:51 +0000 Subject: [PATCH 09/23] move calling `setupConfigs` to after check for `list_rules` --- apps/oxlint/src/lint.rs | 24 ++++++++++---------- crates/oxc_linter/src/config/config_store.rs | 4 ++++ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index a10aae4a24158..09a183d5eca1a 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -291,18 +291,6 @@ impl CliRunner { } }; - // TODO: refactor this elsewhere. - // This code is in the oxlint app, not in oxc_linter crate - if let Some(external_linter) = &external_linter - && let Err(err) = external_plugin_store.setup_configs(external_linter) - { - print_and_flush_stdout( - stdout, - &format!("Failed to setup external plugin options: {err}\n"), - ); - return CliRunResult::InvalidOptionConfig; - } - let report_unused_directives = match inline_config_options.report_unused_directives { ReportUnusedDirectives::WithoutSeverity(true) => Some(AllowWarnDeny::Warn), ReportUnusedDirectives::WithSeverity(Some(severity)) => Some(severity), @@ -341,6 +329,18 @@ impl CliRunner { return CliRunResult::None; } + // Send JS plugins config to JS side + if let Some(external_linter) = &external_linter { + let res = config_store.external_plugin_store().setup_configs(external_linter); + if let Err(err) = res { + print_and_flush_stdout( + stdout, + &format!("Failed to setup external plugin options: {err}\n"), + ); + return CliRunResult::InvalidOptionConfig; + } + } + let files_to_lint = paths .into_iter() .filter(|path| !ignore_matcher.should_ignore(Path::new(path))) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index b34faa5f15df0..e96f28f236e5a 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -351,6 +351,10 @@ impl ConfigStore { ) -> (/* plugin name */ &str, /* rule name */ &str) { self.external_plugin_store.resolve_plugin_rule_names(external_rule_id) } + + pub fn external_plugin_store(&self) -> &ExternalPluginStore { + &self.external_plugin_store + } } #[cfg(test)] From 07c023d996cc3ffc995dc347e67565ff14ecfa0e Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:51:46 +0000 Subject: [PATCH 10/23] docs: add comment --- crates/oxc_linter/src/external_plugin_store.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/oxc_linter/src/external_plugin_store.rs b/crates/oxc_linter/src/external_plugin_store.rs index 9fa934903a257..51569e51421bc 100644 --- a/crates/oxc_linter/src/external_plugin_store.rs +++ b/crates/oxc_linter/src/external_plugin_store.rs @@ -147,6 +147,8 @@ impl ExternalPluginStore { self.options.push(options) } + /// Send options to JS side. + /// /// # Errors /// Returns an error if serialization of rule options fails. pub fn setup_configs(&self, external_linter: &ExternalLinter) -> Result<(), String> { From 63881d71c052a3a3ed96382166f1d1376c5287f7 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 10:58:41 +0000 Subject: [PATCH 11/23] test: combine test into existing test fixture --- .../custom_plugin_with_options/.oxlintrc.json | 17 ----------- .../custom_plugin_with_options/files/index.js | 1 - .../custom_plugin_with_options/output.snap.md | 25 ---------------- .../custom_plugin_with_options/plugin.ts | 29 ------------------- .../test/fixtures/options/.oxlintrc.json | 9 +++++- .../test/fixtures/options/output.snap.md | 17 +++++++++-- apps/oxlint/test/fixtures/options/plugin.ts | 11 +++++++ 7 files changed, 34 insertions(+), 75 deletions(-) delete mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json delete mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js delete mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md delete mode 100644 apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json b/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json deleted file mode 100644 index e4351dac06e8d..0000000000000 --- a/apps/oxlint/test/fixtures/custom_plugin_with_options/.oxlintrc.json +++ /dev/null @@ -1,17 +0,0 @@ -{ - "categories": { - "correctness": "off" - }, - "jsPlugins": [ - "./plugin.ts" - ], - "rules": { - "test-plugin-options/check-options": [ - "error", - true, - { - "expected": "options value" - } - ] - } -} diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js b/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js deleted file mode 100644 index eab74692130a6..0000000000000 --- a/apps/oxlint/test/fixtures/custom_plugin_with_options/files/index.js +++ /dev/null @@ -1 +0,0 @@ -debugger; diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md b/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md deleted file mode 100644 index 15102f183129c..0000000000000 --- a/apps/oxlint/test/fixtures/custom_plugin_with_options/output.snap.md +++ /dev/null @@ -1,25 +0,0 @@ -# Exit code -1 - -# stdout -``` - x test-plugin-options(check-options): [ - | true, - | { - | "expected": "options value" - | } - | ] - ,-[files/index.js:1:1] - 1 | debugger; - : ^ - `---- - -Found 0 warnings and 1 error. -Finished in Xms on 1 file using X threads. -``` - -# stderr -``` -WARNING: JS plugins are experimental and not subject to semver. -Breaking changes are possible while JS plugins support is under development. -``` diff --git a/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts b/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts deleted file mode 100644 index dd9171db4e5fe..0000000000000 --- a/apps/oxlint/test/fixtures/custom_plugin_with_options/plugin.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { definePlugin } from "oxlint"; -import type { Node } from "#oxlint"; - -const SPAN: Node = { - start: 0, - end: 0, - range: [0, 0], - loc: { - start: { line: 0, column: 0 }, - end: { line: 0, column: 0 }, - }, -}; - -export default definePlugin({ - meta: { - name: "test-plugin-options", - }, - rules: { - "check-options": { - create(context) { - context.report({ - message: JSON.stringify(context.options, null, 2), - node: SPAN, - }); - return {}; - }, - }, - }, -}); diff --git a/apps/oxlint/test/fixtures/options/.oxlintrc.json b/apps/oxlint/test/fixtures/options/.oxlintrc.json index dfbce6c0cf2b1..6c243ae2d4a19 100644 --- a/apps/oxlint/test/fixtures/options/.oxlintrc.json +++ b/apps/oxlint/test/fixtures/options/.oxlintrc.json @@ -4,7 +4,14 @@ "correctness": "off" }, "rules": { - "options-plugin/options": "error", + "options-plugin/no-options": "error", + "options-plugin/options": [ + "error", + true, + { + "expected": "options value" + } + ], "options-plugin/default-options": "error" } } diff --git a/apps/oxlint/test/fixtures/options/output.snap.md b/apps/oxlint/test/fixtures/options/output.snap.md index 1986f2f332bac..eaec2acaccb53 100644 --- a/apps/oxlint/test/fixtures/options/output.snap.md +++ b/apps/oxlint/test/fixtures/options/output.snap.md @@ -34,7 +34,7 @@ : ^ `---- - x options-plugin(options): + x options-plugin(no-options): | options: [] | isDeepFrozen: true ,-[files/index.js:1:1] @@ -42,7 +42,20 @@ : ^ `---- -Found 0 warnings and 2 errors. + x options-plugin(options): + | options: [ + | true, + | { + | "expected": "options value" + | } + | ] + | isDeepFrozen: true + ,-[files/index.js:1:1] + 1 | debugger; + : ^ + `---- + +Found 0 warnings and 3 errors. Finished in Xms on 1 file using X threads. ``` diff --git a/apps/oxlint/test/fixtures/options/plugin.ts b/apps/oxlint/test/fixtures/options/plugin.ts index f7688f0ac88f6..7f83cae63fb39 100644 --- a/apps/oxlint/test/fixtures/options/plugin.ts +++ b/apps/oxlint/test/fixtures/options/plugin.ts @@ -15,6 +15,17 @@ const plugin: Plugin = { name: "options-plugin", }, rules: { + "no-options": { + create(context) { + context.report({ + message: + `\noptions: ${JSON.stringify(context.options, null, 2)}\n` + + `isDeepFrozen: ${isDeepFrozen(context.options)}`, + node: SPAN, + }); + return {}; + }, + }, options: { create(context) { context.report({ From 8ecc0714244792d50ae00f782f967c7579e89bd3 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:03:09 +0000 Subject: [PATCH 12/23] test: test all JSON value types --- apps/oxlint/test/fixtures/options/.oxlintrc.json | 8 ++++++-- apps/oxlint/test/fixtures/options/output.snap.md | 11 +++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/apps/oxlint/test/fixtures/options/.oxlintrc.json b/apps/oxlint/test/fixtures/options/.oxlintrc.json index 6c243ae2d4a19..6fedc5250e103 100644 --- a/apps/oxlint/test/fixtures/options/.oxlintrc.json +++ b/apps/oxlint/test/fixtures/options/.oxlintrc.json @@ -7,9 +7,13 @@ "options-plugin/no-options": "error", "options-plugin/options": [ "error", - true, + false, { - "expected": "options value" + "array": [ + {"deep": true, "str": "hello"}, + 456 + ], + "not": null } ], "options-plugin/default-options": "error" diff --git a/apps/oxlint/test/fixtures/options/output.snap.md b/apps/oxlint/test/fixtures/options/output.snap.md index eaec2acaccb53..c47ffc3bd89ec 100644 --- a/apps/oxlint/test/fixtures/options/output.snap.md +++ b/apps/oxlint/test/fixtures/options/output.snap.md @@ -44,9 +44,16 @@ x options-plugin(options): | options: [ - | true, + | false, | { - | "expected": "options value" + | "array": [ + | { + | "deep": true, + | "str": "hello" + | }, + | 456 + | ], + | "not": null | } | ] | isDeepFrozen: true From 71a16fa796820a1b201b6ce43af31087b36a0a22 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:06:41 +0000 Subject: [PATCH 13/23] style: shorten code (dereference early) --- crates/oxc_linter/src/config/config_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index e96f28f236e5a..fec441a620ce8 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -179,7 +179,7 @@ impl Config { self.base .external_rules .iter() - .map(|(rule_id, options_id, severity)| (*rule_id, (*options_id, *severity))) + .map(|&(rule_id, options_id, severity)| (rule_id, (options_id, severity))) .collect(); // Track which plugins have already had their category rules applied. From fde302122b3e0c60d1c41293248916841f1897a4 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:15:38 +0000 Subject: [PATCH 14/23] refactor: convert to `PathBuf` explicitly Personally I try to avoid `.into()` as it's not clear from the code "into what?". `T::from` is equivalent, but more explicit. --- crates/oxc_linter/src/config/config_store.rs | 2 +- crates/oxc_linter/src/config/rules.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index fec441a620ce8..1c9afcc1fec2e 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -1068,7 +1068,7 @@ mod test { // Prepare external plugin store with a custom plugin and rule let mut store = ExternalPluginStore::new(true); store.register_plugin( - "path/to/custom".to_string().into(), + PathBuf::from("path/to/custom"), "custom".to_string(), 0, vec!["my-rule".to_string()], diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index 7c70ad8b16308..1ba93513e3eb2 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -340,6 +340,8 @@ impl ESLintRule { #[cfg(test)] #[expect(clippy::default_trait_access)] mod test { + use std::path::PathBuf; + use rustc_hash::FxHashMap; use serde::Deserialize; use serde_json::{Value, json}; @@ -544,7 +546,7 @@ mod test { // Register a fake external plugin and rule let mut store = ExternalPluginStore::new(true); store.register_plugin( - "path/to/custom-plugin".to_string().into(), + PathBuf::from("path/to/custom-plugin"), "custom".to_string(), 0, vec!["my-rule".to_string()], From 0ec8111e65425247e04cd393f2f18900a8b6ba74 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:15:57 +0000 Subject: [PATCH 15/23] style: longer var names --- crates/oxc_linter/src/config/config_store.rs | 10 +++++----- crates/oxc_linter/src/config/rules.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index 1c9afcc1fec2e..1c47c99882bdf 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -1105,10 +1105,10 @@ mod test { // Should prefer override (options B, severity error) assert_eq!(resolved.external_rules.len(), 1); - let (rid, opts_id, sev) = resolved.external_rules[0]; - assert_eq!(rid, base_external_rule_id); - assert!(sev.is_warn_deny()); - // opts_id should not equal the base options id - assert_ne!(opts_id, base_options_id); + let (rule_id, options_id, severity) = resolved.external_rules[0]; + assert_eq!(rule_id, base_external_rule_id); + assert!(severity.is_warn_deny()); + // `options_id` should not equal the base options ID + assert_ne!(options_id, base_options_id); } } diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index 1ba93513e3eb2..f610d4b3d7b57 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -583,7 +583,7 @@ mod test { rules_no_opts .override_rules(&mut builtin_rules2, &mut external_rules2, &[], &mut store) .unwrap(); - let (_rid2, (options_id2, severity2)) = + let (_rule_id2, (options_id2, severity2)) = external_rules2.iter().next().map(|(k, v)| (*k, *v)).unwrap(); assert_eq!(options_id2, ExternalOptionsId::NONE, "no options should use reserved id 0"); assert!(severity2.is_warn_deny()); From 8f2a6e282b2cb2b29170493ae44f972840b616e5 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:18:27 +0000 Subject: [PATCH 16/23] test: fix tests Make sure severity is as expected. `is_warn_deny()` returns true for either `Warn` or `Deny`. We want to be more specific. --- crates/oxc_linter/src/config/config_store.rs | 2 +- crates/oxc_linter/src/config/rules.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index 1c47c99882bdf..980ad0a9ed881 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -1107,7 +1107,7 @@ mod test { assert_eq!(resolved.external_rules.len(), 1); let (rule_id, options_id, severity) = resolved.external_rules[0]; assert_eq!(rule_id, base_external_rule_id); - assert!(severity.is_warn_deny()); + assert_eq!(severity, AllowWarnDeny::Deny); // `options_id` should not equal the base options ID assert_ne!(options_id, base_options_id); } diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index f610d4b3d7b57..5efef7a774188 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -571,7 +571,7 @@ mod test { ExternalOptionsId::NONE, "non-empty options should allocate a new id" ); - assert!(severity.is_warn_deny()); + assert_eq!(severity, AllowWarnDeny::Warn); // Now configure with no options which should map to reserved index 0 let rules_no_opts = OxlintRules::deserialize(&json!({ @@ -586,7 +586,7 @@ mod test { let (_rule_id2, (options_id2, severity2)) = external_rules2.iter().next().map(|(k, v)| (*k, *v)).unwrap(); assert_eq!(options_id2, ExternalOptionsId::NONE, "no options should use reserved id 0"); - assert!(severity2.is_warn_deny()); + assert_eq!(severity2, AllowWarnDeny::Deny); // Test that null config values also map to reserved index 0 // This tests the case where config might be explicitly null (though unlikely in practice) From a0cf14669c132225c1740b072b7839b95aa7d4fc Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:20:28 +0000 Subject: [PATCH 17/23] style: combine imports --- crates/oxc_linter/src/config/rules.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index 5efef7a774188..b014d05a7eda6 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -346,9 +346,9 @@ mod test { use serde::Deserialize; use serde_json::{Value, json}; - use crate::external_plugin_store::ExternalOptionsId; use crate::{ AllowWarnDeny, ExternalPluginStore, + external_plugin_store::ExternalOptionsId, rules::{RULES, RuleEnum}, }; From 9b8c8485354659134ab7914bc142e18f989836a9 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:25:46 +0000 Subject: [PATCH 18/23] test: make test more realistic --- crates/oxc_linter/src/config/rules.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index b014d05a7eda6..3d3d0a603a4ef 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -560,7 +560,7 @@ mod test { let mut builtin_rules = RuleSet::default(); let mut external_rules = FxHashMap::default(); - rules.override_rules(&mut builtin_rules, &mut external_rules, &[], &mut store).unwrap(); + rules.override_rules(&mut builtin_rules, &mut external_rules, &RULES, &mut store).unwrap(); assert_eq!(builtin_rules.len(), 0); assert_eq!(external_rules.len(), 1); @@ -581,7 +581,7 @@ mod test { let mut builtin_rules2 = RuleSet::default(); let mut external_rules2 = FxHashMap::default(); rules_no_opts - .override_rules(&mut builtin_rules2, &mut external_rules2, &[], &mut store) + .override_rules(&mut builtin_rules2, &mut external_rules2, &RULES, &mut store) .unwrap(); let (_rule_id2, (options_id2, severity2)) = external_rules2.iter().next().map(|(k, v)| (*k, *v)).unwrap(); From 67edfd9554d293b6319e6f1a640c1feb40f2d66d Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:26:07 +0000 Subject: [PATCH 19/23] refactor: shorten code Dereference during destructuring. --- crates/oxc_linter/src/config/rules.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index 3d3d0a603a4ef..63e22ad81afb5 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -564,8 +564,7 @@ mod test { assert_eq!(builtin_rules.len(), 0); assert_eq!(external_rules.len(), 1); - let (_rule_id, (options_id, severity)) = - external_rules.iter().next().map(|(k, v)| (*k, *v)).unwrap(); + let (_rule_id, &(options_id, severity)) = external_rules.iter().next().unwrap(); assert_ne!( options_id, ExternalOptionsId::NONE, @@ -583,8 +582,7 @@ mod test { rules_no_opts .override_rules(&mut builtin_rules2, &mut external_rules2, &RULES, &mut store) .unwrap(); - let (_rule_id2, (options_id2, severity2)) = - external_rules2.iter().next().map(|(k, v)| (*k, *v)).unwrap(); + let (_rule_id2, &(options_id2, severity2)) = external_rules2.iter().next().unwrap(); assert_eq!(options_id2, ExternalOptionsId::NONE, "no options should use reserved id 0"); assert_eq!(severity2, AllowWarnDeny::Deny); From 9ab19a31e22fed8c89d2124b78510a07f17ba414 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:38:41 +0000 Subject: [PATCH 20/23] style: rename vars Unlike in JS, in Rust you can have multiple bindings in same scope with same name. The old binding is destroyed when the new one is created. This is quite idiomatic in Rust. --- crates/oxc_linter/src/config/rules.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index 63e22ad81afb5..abe1d89634b66 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -577,14 +577,14 @@ mod test { "custom/my-rule": "error" })) .unwrap(); - let mut builtin_rules2 = RuleSet::default(); - let mut external_rules2 = FxHashMap::default(); + let mut builtin_rules = RuleSet::default(); + let mut external_rules = FxHashMap::default(); rules_no_opts - .override_rules(&mut builtin_rules2, &mut external_rules2, &RULES, &mut store) + .override_rules(&mut builtin_rules, &mut external_rules, &RULES, &mut store) .unwrap(); - let (_rule_id2, &(options_id2, severity2)) = external_rules2.iter().next().unwrap(); - assert_eq!(options_id2, ExternalOptionsId::NONE, "no options should use reserved id 0"); - assert_eq!(severity2, AllowWarnDeny::Deny); + let (_rule_id, &(options_id, severity)) = external_rules.iter().next().unwrap(); + assert_eq!(options_id, ExternalOptionsId::NONE, "no options should use reserved id 0"); + assert_eq!(severity, AllowWarnDeny::Deny); // Test that null config values also map to reserved index 0 // This tests the case where config might be explicitly null (though unlikely in practice) From 6667b91e664e80d7b76ac524bdd5f70bbd58fe41 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:39:01 +0000 Subject: [PATCH 21/23] style: full stops on comments --- crates/oxc_linter/src/config/rules.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index abe1d89634b66..e2c6b18f72a3d 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -586,8 +586,8 @@ mod test { assert_eq!(options_id, ExternalOptionsId::NONE, "no options should use reserved id 0"); assert_eq!(severity, AllowWarnDeny::Deny); - // Test that null config values also map to reserved index 0 - // This tests the case where config might be explicitly null (though unlikely in practice) + // Test that null config values also map to reserved index 0. + // This tests the case where config might be explicitly null (though unlikely in practice). let null_options_id = store.add_options(serde_json::Value::Null); assert_eq!( null_options_id, From 624eff88c5ac60fa4da184d3b0cfd4598a137214 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 11:49:40 +0000 Subject: [PATCH 22/23] refactor: remove unnecessary type annotation --- crates/oxc_linter/src/config/config_store.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/config/config_store.rs b/crates/oxc_linter/src/config/config_store.rs index 980ad0a9ed881..912c623af0689 100644 --- a/crates/oxc_linter/src/config/config_store.rs +++ b/crates/oxc_linter/src/config/config_store.rs @@ -175,12 +175,12 @@ impl Config { .collect::>(); // Build a hashmap of existing external rules keyed by rule id with value (options_id, severity) - let mut external_rules: FxHashMap = - self.base - .external_rules - .iter() - .map(|&(rule_id, options_id, severity)| (rule_id, (options_id, severity))) - .collect(); + let mut external_rules = self + .base + .external_rules + .iter() + .map(|&(rule_id, options_id, severity)| (rule_id, (options_id, severity))) + .collect::>(); // Track which plugins have already had their category rules applied. // Start with the root plugins since they already have categories applied in base_rules. From 736841bf77f28ce4bd6da2b146083cd9c7ad0410 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Mon, 1 Dec 2025 12:00:07 +0000 Subject: [PATCH 23/23] refactor: assume `options` passed to `add_options` is an array --- crates/oxc_linter/src/config/rules.rs | 17 ----------------- crates/oxc_linter/src/external_plugin_store.rs | 18 +++++++++++------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/crates/oxc_linter/src/config/rules.rs b/crates/oxc_linter/src/config/rules.rs index e2c6b18f72a3d..35eceb10ba678 100644 --- a/crates/oxc_linter/src/config/rules.rs +++ b/crates/oxc_linter/src/config/rules.rs @@ -585,22 +585,5 @@ mod test { let (_rule_id, &(options_id, severity)) = external_rules.iter().next().unwrap(); assert_eq!(options_id, ExternalOptionsId::NONE, "no options should use reserved id 0"); assert_eq!(severity, AllowWarnDeny::Deny); - - // Test that null config values also map to reserved index 0. - // This tests the case where config might be explicitly null (though unlikely in practice). - let null_options_id = store.add_options(serde_json::Value::Null); - assert_eq!( - null_options_id, - ExternalOptionsId::NONE, - "null options should use reserved id 0" - ); - - // Test that empty array also maps to reserved index 0 - let empty_array_id = store.add_options(serde_json::json!([])); - assert_eq!( - empty_array_id, - ExternalOptionsId::NONE, - "empty array options should use reserved id 0" - ); } } diff --git a/crates/oxc_linter/src/external_plugin_store.rs b/crates/oxc_linter/src/external_plugin_store.rs index 51569e51421bc..921626cf701e3 100644 --- a/crates/oxc_linter/src/external_plugin_store.rs +++ b/crates/oxc_linter/src/external_plugin_store.rs @@ -136,14 +136,18 @@ impl ExternalPluginStore { (&plugin.name, &external_rule.name) } - /// Add options to the store and return its ID. - /// Returns index 0 for empty arrays or null values (no options). + /// Add options to the store and return its [`ExternalOptionsId`]. + /// + /// `options` must be a `serde_json::Value::Array`. + /// + /// # Panics + /// Panics in debug build if `options` is not an array or is an empty array. pub fn add_options(&mut self, options: serde_json::Value) -> ExternalOptionsId { - // If it's null or an empty array, return reserved index 0 - if options.is_null() || options.as_array().is_some_and(Vec::is_empty) { - return ExternalOptionsId::from_usize(0); - } - + debug_assert!(options.is_array(), "`options` should be an array"); + debug_assert!( + !options.as_array().unwrap().is_empty(), + "`options` should never be an empty `Vec`" + ); self.options.push(options) }