fix(linter/plugins): get correct plugin name in all cases#17033
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the plugin naming logic for JS plugins to correctly determine the plugin name in all scenarios. The key change is passing the plugin name and whether it's an alias from the Rust side to JavaScript, rather than relying solely on information from plugin.meta.name or package.json after loading.
Key changes:
- Modified the plugin loading callback signature to include a pre-determined plugin name and a boolean flag indicating if it's an alias
- Moved plugin name determination logic to the Rust side (before loading), including normalization of package names
- Added
normalizePluginNamefunction on the JS side that mirrors the Rust implementation to handle plugin names defined inplugin.meta.name
Reviewed changes
Copilot reviewed 21 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_linter/src/external_linter.rs |
Updated ExternalLinterLoadPluginCb type to accept plugin name and alias flag |
crates/oxc_linter/src/config/plugins.rs |
Added documentation cross-referencing JS implementation |
crates/oxc_linter/src/config/config_builder.rs |
Moved plugin name determination logic before plugin loading; normalizes package names for non-relative specifiers |
apps/oxlint/src/run.rs |
Updated JsLoadPluginCb type signature to match new callback interface |
apps/oxlint/src/js_plugins/external_linter.rs |
Updated wrapper function to pass new parameters to JS callback |
apps/oxlint/src-js/plugins/load.ts |
Added normalizePluginName function; updated loadPlugin, registerPlugin, and getPluginName to handle alias priority correctly |
apps/oxlint/src-js/package/rule_tester.ts |
Updated registerPlugin call to include new parameters |
apps/oxlint/src-js/cli.ts |
Updated loadPluginWrapper signature to match new interface |
apps/oxlint/src-js/bindings.d.ts |
Updated TypeScript type definition for JsLoadPluginCb |
apps/oxlint/test/fixtures/plugin_name/* |
Added comprehensive test case covering all plugin naming scenarios |
apps/oxlint/test/fixtures/plugin_name_missing/* |
Added test for relative path plugin without name |
apps/oxlint/test/fixtures/plugin_name_missing2/* |
Added test for npm package plugin without name |
apps/oxlint/test/fixtures/plugin_name_alias/* |
Removed (likely consolidated into main plugin_name test) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CodSpeed Performance ReportMerging #17033 will not alter performanceComparing Summary
Footnotes
|
09aa9a6 to
70afa56
Compare
70afa56 to
50ab8d9
Compare
b99e157 to
038f34a
Compare
50ab8d9 to
c13087b
Compare
Merge activity
|
A couple of changes to how plugin names are dealt with: ### Package names Don't use package name if the specifier used to load the plugin is a relative path. If your project is called `super-server`, and you load a plugin from `../plugins/no-foo.js`, that plugin shouldn't be called `super-server` - that's the name of the project, not the plugin. Now it throws an error, telling you to give the plugin a name. ### Ensure `context.id` matches the plugin name Previously `context.id` might not match the actual plugin name (it could include e.g. `eslint-plugin-` prefix, for example). Now it always matches. This required re-implementing `normalize_plugin_name` on JS side too. ### Tests Add a bunch of tests for every combination I could imagine of `plugin.meta.name`, package name, and alias. Those tests make sure `context.id` is same as the plugin name on Rust side. ### Incomplete There's 2 things remaining to get naming completely correct: 1. Support plugin names with slashes in them e.g. `@scope/eslint-plugin-name` (#14557). 2. It's possible for the same plugin to have different names in different places, because you can alias the same plugin twice with different names in a config, and an override. Plugin names (in `context.id`, and in `// oxlint-disable` comments) should reflect the plugin name *as it is in the config which enabled that rule*. I'm leaving those 2 for now.
038f34a to
415f2f0
Compare
c13087b to
67f8c5d
Compare
camc314
left a comment
There was a problem hiding this comment.
✅This hurts my brain to think about
# Oxlint ### 🚀 Features - 6cc3fdf linter/no-inferrable-types: Implement fixer (#17090) (camc314) - 2067997 linter/no-negation-in-equality-check: Implement suggestion (#17084) (camc314) - 552f9ef vscode: Auto-generate VSCode README configuration from package.json (#16970) (Copilot) - 9190c4b linter/no-unnecessary-array-flat-depth: Implement fixer (#17057) (camc314) - ed789de linter/misrefactored-assign-op: Implement fixer (#17056) (camc314) - a0f74a0 linter/config: Allow aliasing plugin names to allow names the same as builtin plugins (#15569) (Cameron) - a43d251 linter/plugins: `RuleTester` support `languageOptions.globals` (#17009) (overlookmotel) - 35070d9 linter/bad-bitwise-operator: Implement fixer (#17006) (camc314) - 322d995 linter/prefer-enum-initializers: Implement fixer (#17004) (camc314) - ae1e5bc vscode: Add support for tsgolint binary configuration (#16921) (ColemanDunn) - 3bfe31e linter/eslint-plugin-vitest: Add prefer-called-with as vitest compatible jest rule (#16993) (Said Atrahouch) - 0cd075f linter/eslint-plugin-jest: Add fix capabilities to prefer-called-with rule (#16987) (Said Atrahouch) - 357564b linter: Add options for `typescript/require-array-sort-compare` rule. (#16980) (connorshea) - 2b0ffba linter: Add options for `typescript/no-meaningless-void-operator` rule. (#16981) (connorshea) - 8bc4287 linter/plugins: Validate options against schema (#16974) (overlookmotel) - fdc7d08 linter: Implement eslint/capitalized-comments (#16896) (Tu Shaokun) - f8b6561 linter: Add support for `test.for` in vitest (#16925) (camchenry) - 7ee0379 linter/eslint-plugin-vitest: Implement prefer-spy-on (#16426) (Said Atrahouch) - fc96ee0 linter: Implement jest/prefer-to-have-been-called-times (#16938) (秦宇航) - 291b57b ast_tools: Generate TS declaration files for deserializer and walk files (#16912) (camc314) - e31da2a linter: Implement jest/perfer-to-have-been-called (#16899) (秦宇航) - 1a31306 linter/eslint-plugin-vitest: Add require-hook as vitest compatible jest rule (#16880) (Said Atrahouch) - cd3db21 linter: Add ignoredTypeNames option to no-base-to-string rule (#16898) (camc314) - 763b25a linter: Implement eslint/no-inline-comments (#16885) (Tu Shaokun) ### 🐛 Bug Fixes - fb9e193 linter: OOM problems with custom plugins (#17082) (overlookmotel) - 005ec25 linter: Permit `$schema` `.oxlintrc.json` struct (#17060) (Copilot) - fd03131 linter/plugins: Handle plugin names containing slashes (#17073) (overlookmotel) - b2a4fac linter/plugins: Error if plugin name alias is not normalized (#17071) (overlookmotel) - e046c4e linter/no-misused-spread: Add rule options support (#17054) (camc314) - 5c1a9e0 linter/no-deprecated: Add rule options support (#17053) (camc314) - 8c9cafe linter: `import/consistent-type-specifier-style`: add support for declaration files (#16979) (camchenry) - dab4780 linter/no-empty-pattern: Misleading help message for arrays (#17039) (Copilot) - 67f8c5d linter/plugins: Get correct plugin name in all cases (#17033) (overlookmotel) - 674dab9 linter/plugins: Fix indentation in error message (#17018) (overlookmotel) - 6524f72 linter/plugins: Add `@types/node` dev dependency to `oxlint` package (#17016) (overlookmotel) - 7a35513 linter/plugins: Better error for `null` in `globals` in `RuleTester` (#17011) (overlookmotel) - 42603ba linter/plugins: Always define `languageOptions.globals` (#17008) (overlookmotel) - 4cdc2f8 linter: Fix VITEST override rule list and add test for alphabetizing the two lists (#16975) (Connor Shea) - e466562 linter/consistent-type-definitions: Handle parenthesized types in rule (#16998) (camc314) - fce267c linter: Correct vitest plugin source to be `@vitest/eslint-plugin` (#16976) (connorshea) - 477bb57 linter: Fix `vitest/no-restricted-vi-methods` and add tests for it. (#16971) (connorshea) - 7d6974d linter: Ignore oxlint directive comments in capitalized-comments (#16989) (Tu Shaokun) - 23ac6b1 linter/plugins: Apply defaults from `meta.schema` to options (#16930) (overlookmotel) - 2f946cf linter/plugins: Error if `defaultOptions` is not JSON-serializable (#16959) (overlookmotel) - d8b8a57 linter/plugins: Freeze whole of merged options (#16958) (overlookmotel) - d446c43 linter: Prevent extra fields from being present on oxlint config file (#16874) (connorshea) - b845871 linter/plugins: Correctly handle object with `__proto__` keys in options merging (#16928) (overlookmotel) - c897794 linter: Fix eslint/sort-imports allowSeparatedGroups not working with single empty line (#16012) (Duc Nghiem Xuan) - 0c347a1 linter/array-type: Handle satisfies expression (#16903) (camc314) ### ⚡ Performance - 70d853c linter: Avoid cloning source text when cloning AST into fixed-size allocator (#17088) (overlookmotel) - 4d389f7 linter: Less bounds checks in `normalize_plugin_name` (#17030) (overlookmotel) - fd8e9c6 linter/plugins: Speed up cloning JSON objects (#16997) (overlookmotel) - d77e22d linter/plugins: Use `DEFAULT_OPTIONS` for rules with empty array as default options (#16913) (overlookmotel) ### 📚 Documentation - 6d053b4 linter: Fix typo in doc comment (#17091) (overlookmotel) - b5f3c91 linter: Document intentional exclusion of ignoreCase option in jsx-no-duplicate-props (#17046) (Copilot) - a0bf5d8 linter: Fix the config option docs for no-inline-comments rule. (#16983) (connorshea) - ca26a11 linter/plugins: Fix typo in doc comment (#16966) (overlookmotel) - 3183bf8 linter/plugins: Fix typo in JSDoc comment (#16900) (overlookmotel) # Oxfmt ### 🚀 Features - 15dfb55 oxfmt: Respect single nearest `.editorconfig` (#17043) (leaysgur) - 8c33ff4 oxfmt: Expose Node.js API: `format(fileName, sourceText, options?)` (#16939) (leaysgur) ### 🐛 Bug Fixes - d340c87 oxfmt: Update api `FormatOptions` type with `& Record<string, unknown>` (#17036) (leaysgur) - 827a256 oxfmt: Place ignorePatterns at bottom of JSON in --migrate prettier (#16926) (Boshen) Co-authored-by: overlookmotel <557937+overlookmotel@users.noreply.github.com>

A couple of changes to how plugin names are dealt with:
Package names
Don't use package name if the specifier used to load the plugin is a relative path.
If your project is called
super-server, and you load a plugin from../plugins/no-foo.js, that plugin shouldn't be calledsuper-server- that's the name of the project, not the plugin.Now it throws an error, telling you to give the plugin a name.
Ensure
context.idmatches the plugin namePreviously
context.idmight not match the actual plugin name (it could include e.g.eslint-plugin-prefix, for example). Now it always matches.This required re-implementing
normalize_plugin_nameon JS side too.Tests
Add a bunch of tests for every combination I could imagine of
plugin.meta.name, package name, and alias. Those tests make surecontext.idis same as the plugin name on Rust side.Incomplete
There's 2 things remaining to get naming completely correct:
Support plugin names with slashes in them e.g.
@scope/eslint-plugin-name(Linter plugins: Support plugins with/in name #14557).It's possible for the same plugin to have different names in different places, because you can alias the same plugin twice with different names in a config, and an override. Plugin names (in
context.id, and in// oxlint-disablecomments) should reflect the plugin name as it is in the config which enabled that rule.I'm leaving those 2 for now.