Skip to content

Conversation

@lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Nov 27, 2025

Big changes from the original approach

  • Options IDs are stored in their own arrays. This was laid out in refactor(linter/plugins): prepare for rule options #16158. The original approach was to store ruleId-optionId pairs as tuples in an array - several heap allocated objects and excessive pointer indirection.
  • The rust code populates the options by calling a new JS callback named setupConfigs(). I expect we would use this callback for more plugin related data, including settings which are currently implemented in a hacky way.
  • Investigate a refactor to avoid requiring a mutable reference to ExternalPluginStore everywhere.

Smaller Changes

  • Removed the extra fixture ("working version"). There were no meaningful changes compared to the first fixture. Probably slop.

Copilot AI review requested due to automatic review settings November 27, 2025 20:58
@lilnasy lilnasy marked this pull request as draft November 27, 2025 20:58
@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI A-editor Area - Editor and Language Server A-linter-plugins Area - Linter JS plugins C-enhancement Category - New feature or request labels Nov 27, 2025
Copilot finished reviewing on behalf of lilnasy November 27, 2025 21:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements rule options support for external JavaScript plugins in the linter. It introduces an ExternalOptionsId type to track and pass rule-specific configuration options from Rust to JavaScript plugins, allowing JS plugin rules to receive configuration parameters similar to ESLint rule options.

Key changes:

  • Added ExternalOptionsId type and options storage to ExternalPluginStore with index 0 reserved for "no options"
  • Updated external rule signatures from (ExternalRuleId, AllowWarnDeny) to (ExternalRuleId, ExternalOptionsId, AllowWarnDeny) throughout the codebase
  • Implemented options serialization/deserialization flow between Rust and JavaScript using OnceLock for thread-safe caching

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/oxc_linter/src/external_plugin_store.rs Added ExternalOptionsId type and options storage with index 0 reserved for empty/null options
crates/oxc_linter/src/external_linter.rs Updated callback signature to include options_ids parameter
crates/oxc_linter/src/lib.rs Updated external rule handling to pass options IDs to JS side
crates/oxc_linter/src/config/rules.rs Modified override_rules to track options alongside severity; added comprehensive tests
crates/oxc_linter/src/config/config_store.rs Updated rule storage and override logic to handle options IDs; added override precedence test
crates/oxc_linter/src/config/config_builder.rs Updated builder to accept mutable reference to external plugin store for options registration
crates/oxc_linter/src/config/mod.rs Updated test to use mutable external plugin store
crates/oxc_linter/src/tester.rs Updated to pass mutable reference to external plugin store
crates/oxc_language_server/src/linter/server_linter.rs Updated config building calls to use mutable external plugin store
tasks/benchmark/benches/linter.rs Updated benchmark to use mutable external plugin store
napi/playground/src/lib.rs Updated playground to properly share external plugin store between config building and linter creation
apps/oxlint/src/run.rs Added OnceLock-based caching for serialized options JSON and export function for JS
apps/oxlint/src/lint.rs Added serialization of external options after config building
apps/oxlint/src/js_plugins/external_linter.rs Updated wrapper to forward options IDs to JS callback
apps/oxlint/src-js/plugins/options.ts Added options initialization logic and storage
apps/oxlint/src-js/plugins/load.ts Duplicated options initialization logic (should be removed)
apps/oxlint/src-js/plugins/lint.ts Added lazy options initialization and options ID handling; duplicate ruleIndex assignment
apps/oxlint/src-js/cli.ts Updated wrapper to forward options IDs parameter
apps/oxlint/src-js/bindings.js Exported getExternalRuleOptions function
apps/oxlint/src-js/bindings.d.ts Added TypeScript declaration for getExternalRuleOptions
apps/oxlint/test/fixtures/custom_plugin_with_options/* Added integration test fixture for external plugin with options

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 35 to 40
/// 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()
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation comment states "Returns a JSON string of options arrays" but the function returns Option<String>, which can be None. The comment should clarify when None is returned (e.g., "Returns a JSON string of options arrays, or None if options haven't been set yet").

Copilot uses AI. Check for mistakes.
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 27, 2025

CodSpeed Performance Report

Merging #16215 will not alter performance

Comparing lilnasy:js-plugin-options (42eee5d) with main (505ceb1)

Summary

✅ 42 untouched
⏩ 3 skipped1

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@lilnasy lilnasy force-pushed the js-plugin-options branch 2 times, most recently from a7a3ca1 to 9c2d8a4 Compare November 27, 2025 21:11
@overlookmotel
Copy link
Member

Just a note: As I mentioned on Discord, I don't think we should worry about merging options with default options in this first PR - can leave that to a follow-on. But FYI I've implemented the merging logic in #16217 because I needed it for the rule tester.

@lilnasy lilnasy force-pushed the js-plugin-options branch 2 times, most recently from d62762c to 3c46139 Compare November 28, 2025 13:01
@overlookmotel
Copy link
Member

Refactor to avoid requiring a mutable reference to ExternalPluginStore everywhere.

I know it was met that said this should be possible, but I could very well be wrong. If it's not possible, don't worry about it.

Comment on lines 294 to 304
// TODO: refactor this elsewhere.
// This code is in the oxlint app, not in oxc_linter crate
if let Some(ref external_linter) = external_linter {
if 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;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could use help here. I exposed setup_configs() for the oxlint crate by putting it on ExternalPluginStore only because it's the first place I found. I could do with a few pointers on separation of concerns regarding these structs.

@lilnasy lilnasy marked this pull request as ready for review November 28, 2025 20:25
@lilnasy lilnasy requested a review from Copilot November 28, 2025 20:25
Copilot finished reviewing on behalf of lilnasy November 28, 2025 20:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +4 to +14
* Populates Rust-resolved configuration options on the JS side.
* Called from Rust side after all configuration options have been resolved.
*
* Note: the name `setupConfigs` is currently incorrect, as we only populate rule options.
* The intention is for this function to transfer all configurations in a multi-config workspace.
* The configuration relevant to each file would then be resolved on the JS side.
*
* @param optionsJSON - JSON serialization of an array containing all rule options across all configurations.
* @returns "ok" on success, or error message on failure
*/
export function setupConfigs(optionsJSON: string): string {
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The comment states "Note: the name setupConfigs is currently incorrect, as we only populate rule options." While this is acknowledged as a TODO, consider renaming to setupOptions or setupRuleOptions to accurately reflect the current functionality. This would make the code clearer until multi-config workspace support is implemented.

Suggested change
* Populates Rust-resolved configuration options on the JS side.
* Called from Rust side after all configuration options have been resolved.
*
* Note: the name `setupConfigs` is currently incorrect, as we only populate rule options.
* The intention is for this function to transfer all configurations in a multi-config workspace.
* The configuration relevant to each file would then be resolved on the JS side.
*
* @param optionsJSON - JSON serialization of an array containing all rule options across all configurations.
* @returns "ok" on success, or error message on failure
*/
export function setupConfigs(optionsJSON: string): string {
* Populates Rust-resolved rule options on the JS side.
* Called from Rust side after all rule options have been resolved.
*
* The intention is for this function to eventually transfer all configurations in a multi-config workspace.
* The configuration relevant to each file would then be resolved on the JS side.
*
* @param optionsJSON - JSON serialization of an array containing all rule options across all configurations.
* @returns "ok" on success, or error message on failure
*/
export function setupOptions(optionsJSON: string): string {

Copilot uses AI. Check for mistakes.
Comment on lines +158 to 162

// 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];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@overlookmotel You mentioned this could be simplified to allOptions[optionsId]. How would plugin-provided default options get used in that case?

const el = parsed[i];
if (!isArray(el)) throw new TypeError("Each options entry must be an array", { cause: el });
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a debug check because I this validation is done probably done on the rust side as well. I need to confirm this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-editor Area - Editor and Language Server A-linter Area - Linter A-linter-plugins Area - Linter JS plugins C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants