-
-
Notifications
You must be signed in to change notification settings - Fork 724
chore(oxlint/napi): add clearLoadedPlugin function with automatic cleanup in ExternalLinter
#16057
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
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
- Add clearLoadedPlugin() JS function in src-js/plugins/load.ts to clear registeredRules and registeredPluginPaths - Export clearLoadedPlugin from src-js/plugins/index.ts and src-js/index.ts - Add tests for clearLoadedPlugin functionality - Function will be used by language server to reset plugin state Co-authored-by: Sysix <[email protected]>
|
@copilot use the rust function in external_linter.rs create_external_linter |
- Add JsClearLoadedPluginCb type to handle plugin state clearing - Add clearLoadedPlugin callback parameter to create_external_linter - Implement Drop trait for ExternalLinter to call clearLoadedPlugin - Update lint and lint_impl to accept and pass clearLoadedPlugin callback - Update cli.ts to pass clearLoadedPluginWrapper callback - Export ExternalLinterClearLoadedPluginCb from oxc_linter The clearLoadedPlugin function is now automatically called when the ExternalLinter is destroyed, ensuring plugin state is properly cleaned up after linting completes. Co-authored-by: Sysix <[email protected]>
Done! I've implemented Changes in commit
The plugin state is now automatically cleared when linting completes and the |
|
Just to say, please ask for my review on this one. I'd like to stay on top of the Rust-JS interop layer for JS plugins. |
clearLoadedPlugin function with automatic cleanup in ExternalLinter
CodSpeed Performance ReportMerging #16057 will not alter performanceComparing Summary
Footnotes
|
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.
Pull request overview
This PR adds functionality to clear loaded plugin state from both the JavaScript and Rust sides of the oxlint NAPI integration. It introduces a clearLoadedPlugin function that clears internal plugin state (registeredRules and registeredPluginPaths) and integrates it into the ExternalLinter lifecycle via a Drop trait implementation that automatically calls the cleanup function when the linter is destroyed.
Key changes:
- Added
clearLoadedPlugin()function on the JavaScript side to clear plugin state - Added
ExternalLinterClearLoadedPluginCbcallback type on the Rust side - Implemented automatic cleanup via
Droptrait onExternalLinter
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_linter/src/lib.rs |
Exported ExternalLinterClearLoadedPluginCb type |
crates/oxc_linter/src/external_linter.rs |
Added callback type, updated struct, and implemented Drop trait for automatic cleanup |
apps/oxlint/src/run.rs |
Added JsClearLoadedPluginCb type and updated lint function signature |
apps/oxlint/src/js_plugins/external_linter.rs |
Added wrap_clear_loaded_plugin function and updated create_external_linter |
apps/oxlint/src-js/plugins/load.ts |
Implemented clearLoadedPlugin() function |
apps/oxlint/src-js/plugins/index.ts |
Exported clearLoadedPlugin |
apps/oxlint/src-js/index.ts |
Exported async wrapper for public API |
apps/oxlint/src-js/cli.ts |
Added clearLoadedPluginWrapper and passed it to Rust |
apps/oxlint/src-js/bindings.d.ts |
Added type definitions for the new callback |
apps/oxlint/test/clear-loaded-plugin.test.ts |
Added basic tests for the function |
Comments suppressed due to low confidence (1)
apps/oxlint/src-js/index.ts:1
- The documentation states the function 'will not have any effect if no plugins have been loaded yet', but this is misleading. The function is exported and can be called at any time. It will execute and clear the internal state arrays (which will be empty if no plugins were loaded), but it won't fail or be unavailable. Consider revising to: 'This function can be called safely even if no plugins have been loaded yet, in which case it will be a no-op.'
import { debugAssertIsNonNull } from './utils/asserts.js';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Box::new(move || { | ||
| // Call the JavaScript callback to clear loaded plugin state | ||
| let _ = cb.call((), ThreadsafeFunctionCallMode::Blocking); | ||
| }) |
Copilot
AI
Nov 24, 2025
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.
The result of cb.call() is silently discarded. If the JavaScript callback fails or throws an error, it will be ignored. Consider logging the error or adding a comment explaining why errors can be safely ignored during cleanup. This is especially important since this is called from the Drop implementation where panicking would be problematic.
| impl Drop for ExternalLinter { | ||
| fn drop(&mut self) { | ||
| // Clear loaded plugin state when the linter is destroyed | ||
| (self.clear_loaded_plugin)(); | ||
| } |
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.
The Drop implementation makes a blocking call to JavaScript, which is unsafe and can cause deadlocks or panics. If ExternalLinter is dropped during program shutdown or on a thread without an active JS runtime, the ThreadsafeFunctionCallMode::Blocking call will hang indefinitely or panic. Drop implementations must be infallible and non-blocking.
// Instead, use non-blocking mode and ignore errors
impl Drop for ExternalLinter {
fn drop(&mut self) {
// Use NonBlocking mode to avoid deadlocks during shutdown
let _ = (self.clear_loaded_plugin)();
}
}Or remove the Drop implementation entirely and require explicit cleanup by the caller.
| impl Drop for ExternalLinter { | |
| fn drop(&mut self) { | |
| // Clear loaded plugin state when the linter is destroyed | |
| (self.clear_loaded_plugin)(); | |
| } | |
| impl Drop for ExternalLinter { | |
| fn drop(&mut self) { | |
| // Use NonBlocking mode to avoid deadlocks during shutdown | |
| let _ = (self.clear_loaded_plugin)(); | |
| } | |
| } | |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |

Adds
clearLoadedPluginfor clearing loaded plugin state from JS side. When the language server supportsjsPlugins, the internal state needs to be cleared and recreated.The
Dropis an extra call, not sure if this is needed. Sounds good for me, but maybe performance?The goal is that there is one
ExternalLinterto manage multiple workspaces. The plugins load/unload for each workspace will be done in some other PR.Not tested, but all tests are passing 🥳
Copilot Summary
Adds a
clearLoadedPluginfunction to clear loaded plugin state (registeredRulesandregisteredPluginPaths). The function is automatically called when theExternalLinteris destroyed, ensuring plugin state is properly cleaned up after each lint run.Changes
JavaScript Side
src-js/plugins/load.ts: AddedclearLoadedPlugin()function to clear internal plugin statesrc-js/plugins/index.ts: ExportedclearLoadedPluginsrc-js/index.ts: Exported async wrapper for public APIsrc-js/cli.ts: AddedclearLoadedPluginWrapperto pass callback to Rusttest/clear-loaded-plugin.test.ts: Added tests verifying function can be called multiple times safelyRust Side
apps/oxlint/src/run.rs: AddedJsClearLoadedPluginCbcallback type and updatedlintfunction signatureapps/oxlint/src/js_plugins/external_linter.rs: Addedwrap_clear_loaded_pluginand updatedcreate_external_linterto accept the callbackcrates/oxc_linter/src/external_linter.rs: AddedExternalLinterClearLoadedPluginCbtype, updatedExternalLinterstruct, and implementedDroptrait to automatically call clearLoadedPlugincrates/oxc_linter/src/lib.rs: ExportedExternalLinterClearLoadedPluginCbUsage
The function can be called manually:
Additionally, plugin state is automatically cleared when linting completes, as the
ExternalLinterdestructor callsclearLoadedPluginvia theDroptrait implementation.What Gets Cleared
registeredPluginPathsSet (tracks loaded plugin paths)registeredRulesarray (stores loaded rule instances)Execution Flow
clearLoadedPlugincallback is passed from JavaScript to RustExternalLinterstructExternalLinteris dropped, theDropimplementation automatically calls clearLoadedPluginOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.