-
-
Notifications
You must be signed in to change notification settings - Fork 833
feat(linter): Add support for options for jsPlugins #15822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| import { setupFileContext, resetFileContext } from './context.js'; | ||
| import { registeredRules } from './load.js'; | ||
| import { diagnostics } from './report.js'; | ||
| import { registeredRules, allOptions, areOptionsInitialized, setOptions, type Options } from './load.js'; | ||
| import { setSettingsForFile, resetSettings } from './settings.js'; | ||
| import { ast, initAst, resetSourceAndAst, setupSourceForFile } from './source_code.js'; | ||
| import { assertIs, getErrorMessage } from './utils.js'; | ||
| import { addVisitorToCompiled, compiledVisitor, finalizeCompiledVisitor, initCompiledVisitor } from './visitor.js'; | ||
| import { getExternalRuleOptions } from '../bindings.js'; | ||
|
|
||
| // Lazy implementation | ||
| /* | ||
|
|
@@ -38,19 +39,19 @@ const PARSER_SERVICES_DEFAULT: Record<string, unknown> = Object.freeze({}); | |
| * @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 | ||
| * @param ruleData - Array of [ruleId, optionsId] pairs for rules to run on this file | ||
| * @param settingsJSON - Settings for file, as JSON | ||
| * @returns Diagnostics or error serialized to JSON string | ||
| * @returns JSON result | ||
| */ | ||
| export function lintFile( | ||
| filePath: string, | ||
| bufferId: number, | ||
| buffer: Uint8Array | null, | ||
| ruleIds: number[], | ||
| ruleData: Array<[number, number]>, | ||
| settingsJSON: string, | ||
| ): string { | ||
| try { | ||
| lintFileImpl(filePath, bufferId, buffer, ruleIds, settingsJSON); | ||
| lintFileImpl(filePath, bufferId, buffer, ruleData, settingsJSON); | ||
| return JSON.stringify({ Success: diagnostics }); | ||
| } catch (err) { | ||
| return JSON.stringify({ Failure: getErrorMessage(err) }); | ||
|
|
@@ -65,8 +66,8 @@ export function lintFile( | |
| * @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 | ||
| * @param settingsJSON - Stringified settings for this file | ||
| * @param ruleData - Array of [ruleId, optionsId] pairs for rules to run on this file | ||
| * @param settingsJSON - Settings for file, as JSON | ||
| * @returns Diagnostics to send back to Rust | ||
| * @throws {Error} If any parameters are invalid | ||
| * @throws {*} If any rule throws | ||
|
|
@@ -75,7 +76,7 @@ function lintFileImpl( | |
| filePath: string, | ||
| bufferId: number, | ||
| buffer: Uint8Array | null, | ||
| ruleIds: number[], | ||
| ruleData: Array<[number, number]>, | ||
| settingsJSON: string, | ||
| ) { | ||
| // If new buffer, add it to `buffers` array. Otherwise, get existing buffer from array. | ||
|
|
@@ -101,8 +102,8 @@ function lintFileImpl( | |
| if (typeof filePath !== 'string' || filePath.length === 0) { | ||
| throw new Error('expected filePath to be a non-zero length string'); | ||
| } | ||
| if (!Array.isArray(ruleIds) || ruleIds.length === 0) { | ||
| throw new Error('Expected `ruleIds` to be a non-zero len array'); | ||
| if (!Array.isArray(ruleData) || ruleData.length === 0) { | ||
| throw new Error('Expected `ruleData` to be a non-zero len array'); | ||
| } | ||
|
|
||
| // Pass file path to context module, so `Context`s know what file is being linted | ||
|
|
@@ -126,19 +127,27 @@ function lintFileImpl( | |
| // Get visitors for this file from all rules | ||
| initCompiledVisitor(); | ||
|
|
||
| for (let i = 0, len = ruleIds.length; i < len; i++) { | ||
| const ruleId = ruleIds[i], | ||
| ruleDetails = registeredRules[ruleId]; | ||
| // Initialize external rule options if not already initialized | ||
| if (!areOptionsInitialized()) { | ||
| const optionsJson = getExternalRuleOptions(); | ||
| if (optionsJson !== null && optionsJson.length > 0) { | ||
| setOptions(optionsJson); | ||
| } | ||
| } | ||
|
|
||
| // Set `ruleIndex` for rule. It's used when sending diagnostics back to Rust. | ||
| for (let i = 0; i < ruleData.length; i++) { | ||
| const [ruleId, optionsId] = ruleData[i]; | ||
| const ruleDetails = registeredRules[ruleId]; | ||
| const options = allOptions[optionsId]; | ||
| ruleDetails.ruleIndex = i; | ||
| ruleDetails.options = options === undefined || !Array.isArray(options) ? [] : (options as Options); | ||
overlookmotel marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+141
to
+143
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be able to compress this to: ruleDetails.ruleIndex = i;
ruleDetails.options = allOptions[optionsId];We need to make sure
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we can get rid of |
||
|
|
||
| const { rule, context } = ruleDetails; | ||
| const { rule } = ruleDetails; | ||
|
|
||
| let { visitor } = ruleDetails; | ||
| if (visitor === null) { | ||
| // Rule defined with `create` method | ||
| visitor = rule.create(context); | ||
| visitor = rule.create(ruleDetails.context); | ||
|
Comment on lines
-136
to
+150
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes are good, but unrelated to this PR. I've broken out this change to #15842 (merged). Please rebase on latest main. |
||
| } else { | ||
| // Rule defined with `createOnce` method | ||
| const { beforeHook, afterHook } = ruleDetails; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,43 @@ import type { JsonValue } from './json.ts'; | |||||
| import type { RuleMeta } from './rule_meta.ts'; | ||||||
| import type { AfterHook, BeforeHook, Visitor, VisitorWithHooks } from './types.ts'; | ||||||
|
|
||||||
| // All external rule options serialized from Rust. | ||||||
| // Indexed by ExternalOptionsId (numeric) sent alongside ruleId for each file. | ||||||
| // Element 0 is always an empty array (reserved for rules with no options). | ||||||
| export let allOptions: unknown[][] = []; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Then can get rid of an |
||||||
|
|
||||||
overlookmotel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // Track if options have been initialized to avoid re-initialization | ||||||
| let optionsInitialized = false; | ||||||
|
|
||||||
| /** | ||||||
| * 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 { | ||||||
| try { | ||||||
| const parsed = JSON.parse(optionsJson); | ||||||
| if (!Array.isArray(parsed)) throw new TypeError('Expected optionsJson to decode to an array'); | ||||||
| // Basic shape validation: each element must be an array (options tuple array) | ||||||
| for (let i = 0; i < parsed.length; i++) { | ||||||
| const el = parsed[i]; | ||||||
| if (!Array.isArray(el)) throw new TypeError('Each options entry must be an array'); | ||||||
| } | ||||||
| // Always update options - they may be set multiple times during the process | ||||||
| allOptions = parsed; | ||||||
| optionsInitialized = true; | ||||||
| } catch (err) { | ||||||
| // Re-throw with clearer message for Rust side logging. | ||||||
| throw new Error( | ||||||
| 'Failed to parse external rule options JSON: ' + (err instanceof Error ? err.message : String(err)), | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function areOptionsInitialized(): boolean { | ||||||
| return optionsInitialized; | ||||||
| } | ||||||
|
|
||||||
| const ObjectKeys = Object.keys; | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
|
|
@@ -329,6 +329,13 @@ impl CliRunner { | |
| return CliRunResult::None; | ||
| } | ||
|
|
||
| // After building config, serialize external rule options for JS side. | ||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
| { | ||
| let store = config_store.external_plugin_store(); | ||
| crate::set_external_options_json(store); | ||
| } | ||
|
Comment on lines
+332
to
+337
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of storing the options in a |
||
|
|
||
| let files_to_lint = paths | ||
| .into_iter() | ||
| .filter(|path| !ignore_matcher.should_ignore(Path::new(path))) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,30 @@ use crate::{ | |
| result::CliRunResult, | ||
| }; | ||
|
|
||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
| use oxc_linter::ExternalPluginStore; | ||
|
|
||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
| use std::sync::OnceLock; | ||
|
|
||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
overlookmotel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| static EXTERNAL_OPTIONS_JSON: OnceLock<String> = OnceLock::new(); | ||
|
|
||
| /// Set serialized external rule options JSON after building configs. | ||
| /// Called from Rust side (internal) before any linting, then consumed on first call to `lint`. | ||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
| pub fn set_external_options_json(plugin_store: &ExternalPluginStore) { | ||
| let _ = EXTERNAL_OPTIONS_JSON.set(plugin_store.serialize_all_options()); | ||
| } | ||
|
|
||
| /// JS callable function to retrieve the serialized external rule options. | ||
| /// Returns a JSON string of options arrays. Called once from JS after creating the external linter. | ||
| #[cfg(all(feature = "napi", target_pointer_width = "64", target_endian = "little"))] | ||
| #[napi] | ||
| pub fn get_external_rule_options() -> Option<String> { | ||
| EXTERNAL_OPTIONS_JSON.get().cloned() | ||
| } | ||
|
|
||
|
Comment on lines
+19
to
+42
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need any of this (see above). |
||
| /// JS callback to load a JS plugin. | ||
| #[napi] | ||
| pub type JsLoadPluginCb = ThreadsafeFunction< | ||
|
|
@@ -39,13 +63,13 @@ pub type JsLintFileCb = ThreadsafeFunction< | |
| String, // Absolute path of file to lint | ||
| u32, // Buffer ID | ||
| Option<Uint8Array>, // Buffer (optional) | ||
| Vec<u32>, // Array of rule IDs | ||
| String, // Stringified settings effective for the file | ||
| Vec<(u32, u32)>, // Array of [ruleId, optionsId] pairs | ||
| String, // Settings JSON | ||
| )>, | ||
| // Return value | ||
| String, // `Vec<LintFileResult>`, serialized to JSON | ||
| // Arguments (repeated) | ||
| FnArgs<(String, u32, Option<Uint8Array>, Vec<u32>, String)>, | ||
| FnArgs<(String, u32, Option<Uint8Array>, Vec<(u32, u32)>, String)>, | ||
| // Error status | ||
| Status, | ||
| // CalleeHandled | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,14 @@ describe('oxlint CLI', () => { | |
| await testFixture('custom_plugin_via_overrides'); | ||
| }); | ||
|
|
||
| it('should pass options to custom plugin rules', async () => { | ||
| await testFixture('custom_plugin_with_options'); | ||
| }); | ||
|
|
||
| it('should pass options to custom plugin rules (working version)', async () => { | ||
| await testFixture('custom_plugin_options_working'); | ||
| }); | ||
|
Comment on lines
+99
to
+105
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need 2 tests here? |
||
|
|
||
| it('should report an error if a custom plugin is missing', async () => { | ||
| await testFixture('missing_custom_plugin'); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "jsPlugins": ["./plugin.js"], | ||
| "rules": { | ||
| "test-plugin-options/check-options": ["error", true, { "expected": "production" }] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| // Test file with debugger statement | ||
| debugger; | ||
|
|
||
| console.log('Hello, world!'); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Exit code | ||
| 1 | ||
|
|
||
| # stdout | ||
| ``` | ||
| ! eslint(no-debugger): `debugger` statement is not allowed | ||
| ,-[files/index.js:2:1] | ||
| 1 | // Test file with debugger statement | ||
| 2 | debugger; | ||
| : ^^^^^^^^^ | ||
| 3 | | ||
| `---- | ||
| help: Remove the debugger statement | ||
|
|
||
| x test-plugin-options(check-options): Expected value to be production, got disabled | ||
| ,-[files/index.js:2:1] | ||
| 1 | // Test file with debugger statement | ||
| 2 | debugger; | ||
| : ^^^^^^^^^ | ||
| 3 | | ||
| `---- | ||
|
|
||
| Found 1 warning 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. | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| export default { | ||
| meta: { | ||
| name: 'test-plugin-options', | ||
| }, | ||
| rules: { | ||
| 'check-options': { | ||
| meta: { | ||
| messages: { | ||
| wrongValue: 'Expected value to be {{expected}}, got {{actual}}', | ||
| noOptions: 'No options provided', | ||
| }, | ||
| }, | ||
| createOnce(context) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use This will also simplify the implementation - can do the check for options in body of |
||
| // Don't access context.options here - it's not available yet! | ||
| // Options are only available in visitor methods after setupContextForFile is called. | ||
|
|
||
| return { | ||
| before() { | ||
| // Options are now available since setupContextForFile was called | ||
| const options = context.options; | ||
|
|
||
| // Check if options were passed correctly | ||
| if (!options || options.length === 0) { | ||
| context.report({ | ||
| messageId: 'noOptions', | ||
| loc: { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } }, | ||
| }); | ||
| return false; | ||
| } | ||
| return true; | ||
| }, | ||
|
|
||
| DebuggerStatement(node) { | ||
| // Options are available in visitor methods | ||
| const options = context.options; | ||
|
|
||
| // First option is a boolean | ||
| const shouldReport = options[0]; | ||
| // Second option is an object | ||
| const config = options[1] || {}; | ||
|
|
||
| if (shouldReport) { | ||
| context.report({ | ||
| messageId: 'wrongValue', | ||
| data: { | ||
| expected: String(config.expected || 'enabled'), | ||
| actual: 'disabled', | ||
| }, | ||
| node, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
|
Comment on lines
+17
to
+53
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can greatly simplify all this. Just unconditionally report |
||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with:
I know the original is more idiomatic, but in
apps/oxlintwe prioritize perf over readability! Array destructuring is much more expensive for the JS engine.