## Summary
- reject JS plugins whose normalized names collide
- keep registered plugin names isolated per LSP workspace
- add a CLI regression fixture covering two plugins that expose the same rule name
Fixes#25164
Closes#22131
## Summary
- normalize `oxlint-plugin-*` names the same way as existing
`eslint-plugin-*` names
- keep Rust `normalize_plugin_name` and JS `normalizePluginName`
behavior in sync
- reject `oxlint-plugin-*` aliases as non-normalized plugin names
- add JS plugin fixtures for package-name, `meta.name`, and scoped
package normalization
## AI Assistance
This PR was prepared with AI assistance. I reviewed the generated
changes and test results, and I am responsible for the contribution.
Co-authored-by: xiaokhkh <10765730+xiaokhkh@users.noreply.github.com>
Co-authored-by: Cameron <cameron.clark@hey.com>
Fix an incompatibility with `eslint-plugin-jsdoc`. It relies on properties of `context` being values rather than getters:
https://github.com/gajus/eslint-plugin-jsdoc/blob/26276d428631a2625575607cba7875dab6797091/src/buildForbidRuleDefinition.js#L86-L103
When `options` property is a getter, this code throws an error because it attempts to define a property with both `value` and `get` in its descriptor.
This PR replaces the `id` and `options` getters with value properties.
The getters were there to prevent the `Context` object from being mutated by rule code, and allow the object to be frozen, as it is in ESLint. But each rule gets its own `Context` object, so if it chooses to mutate it, it only harms itself, and can't impact other rules/plugins. In ESLint, trying to alter the frozen `Context` object would be a hard error, so it's doubtful that any ESLint plugins do this anyway.
This is no real downside to this change for us, so we might as well make the change, and fix `eslint-plugin-jsdoc` and any other plugins which may depend on ESLint's behavior here.
Closes#18611.
Previously `definePlugin` and `defineRule` were doing double duty:
1. Providing type safety (standard `defineConfig` pattern).
2. Converting a plugin/rule to be ESLint compatible
Combining the two duties is probably confusing to users. Users are familiar with the `defineConfig` pattern and these functions generally don't do anything at runtime - just require a specific type as param and return the input untouched.
This PR adds a separate function `eslintCompatPlugin` which does the conversion to an ESLint-compatible plugin.
`definePlugin` and `defineRule` are now no-ops at runtime - they just pass through the plugin/rule unchanged.
Workspaces support (#17809) added overhead on JS side of hashmap lookups. Every call to `lintFile` has to look up details (e.g. registered rules, rule options) from hashmaps keyed by workspace URI.
### Perf
Take a different approach which is optimized for common case of there being only 1 workspace:
* At any time there's 1 "current workspace".
* Store rules, options, and CWD in top-level vars (back to how it was before #17809).
* Switch workspaces only when required. Switching reassigns the values in these top level vars.
Before:
* Get rules, options, and CWD from hashmaps keyed by workspace URI.
After:
* Quick check "is the current workspace the one we need?".
* If so, do nothing - the top-level vars `registeredRules`, `allOptions`, and `cwd` elready contain the right settings for the desired workspace.
* If current workspace is not the right one, look up the details for desired workspace in `workspaces` hash map and write them into the top-level vars.
In CLI, `workspaceUri` passed to `lintFile` etc is `null`, so the quick "in the right workspace?" check is particularly cheap - just `null === null`. So this reduces the overhead that workspace support adds to CLI to just that 1 instruction.
Add of these functions are entirely synchronous, so there's no possibility of a race where 1 call switches out the values in the top-level vars, while another call is busy using them.
### CLI: No call to `createWorkspace`
CLI now doesn't need to call `createWorkspace` at all. The "are we in right workspace?" check always succeeds, so it's irrelevant whether a workspace has been created or not - `workspaces` hashmap is never accessed.
So workspaces is now an LSP-only thing.
### Bundle
This PR also removes all imports from other files from `src-js/workspace/index.ts`. It now only imports `debugAssert`, which is optimized out in release build., leaving no imports at all.
Previously, the imports from other files were causing most of the JS plugins code to get loaded on first call to `createWorkspace`, which broke the lazy loading optimization - Oxlint CLI and LSP were both loading all the JS plugins code eagerly even if the user doesn't use JS plugins.
With this change, the lazy loading optimization is restored. This is visible in the conformance snapshot change in this PR.
Pure refactor. Remove these 2 types which were both just aliases for `string`.
In Rust type wrappers can be useful (e.g. `ScopeId`) but in TS they don't add type safety, and IMO make the code less readable.
## The problem
### 1. Bug with plugins outside of workspace directory
There was a bug in workspaces implementation. `loadPlugin` was assuming that the plugin file is within the workspace, but that's not necessarily the case. e.g.:
* Workspace 1: `/path/to/project/packages/foo`
* Workspace 2: `/path/to/project/packages/bar`
* JS plugin: `/path/to/project/node_modules/eslint-plugin-whatever`
This problem arises because `loadPlugin` is determining which workspace to load a plugin for by search for the workspace whose root directory contains the plugin file.
### 2. Performance
Every call to `loadPlugin`, `setupRuleConfigs`, or `lintFile` was first searching through all workspaces to find the right one (`getResponsibleWorkspace`).
This is really expensive when there are lots of workspaces, and quite expensive even when there's only 1 because iterating hashsets is relatively slow.
## This PR
This PR separates out CWD and workspace identifiers into 2 different concerns.
* Each workspace has an ID which is unique.
* Each workspace has a CWD (path to root directory as a file path).
The workspace ID is currently the workspace's root directory as a `file://...` URL. Using that as it's guaranteed unique, even for paths which are not valid UTF-8 (`Path::to_string_lossy` can produce the same `&str` for different paths). But ID it doesn't have to be the URI - we could change it later to an integer ID if we want.
Rust-side code passes the workspace ID to every call to `loadPlugin`, `setupRuleConfigs`, or `lintFile`.
In CLI, it just passes `null` as the ID. There's only ever 1 workspace in CLI, so no point in the expense of passing a `String` on every call.
These functions look up the CWD from the workspace ID in the `cwds` map.
Separating these 2 concerns solves both problems listed above:
1. Plugins can be outside of the workspace's CWD (see added test).
2. `getResponsibleWorkspace` is removed, solving the perf issue.
Note: The multiple hashmap lookups on each call to `lintFile` remain a perf drain. This is addressed in a later PR in this stack.
This PR introduces workspace isolation for the oxlint NAPI plugin system, adding `createWorkspace` and `destroyWorkspace` functions to manage multi workspace support for the language server.
This does not fully solve the integration, but should be a good start for all future work. One example is the buffer for multiple calls at the same time (?).
Added a new workspace isolation layer that uses the current working directory (cwd) as the workspace identifier:
- `createWorkspace(cwd: string)`: Initializes a new isolated workspace for plugins and configurations
- `destroyWorkspace(cwd: string)`: Cleans up workspace resources, unloading all associated plugins and options
- `getResponsibleWorkspace(filePath: string)`: Determines which workspace is responsible for a given file path, reflecting the changes in https://github.com/oxc-project/oxc/pull/17853
Rules' default options are serialized to JSON and then `JSON.parse`-ed again, to ensure they don't contain cyclic references.
In the process, `Infinity` was converted to `null` (because JSON can't encode `Infinity`). This broke one of `eslint-plugin-stylistic`'s rules, which has an option which defaults to `Infinity`.
Fix this by replacing `Infinity` with a placeholder string in `JSON.stringify` and then replacing it before `JSON.parse`.
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.
Fixes#15631.
#16930 implemented options validation against `rule.meta.schema`, for the purpose of supporting default options added by schema. But it didn't actually do anything if validation failed.
Implement validation of options against schema, and pass any errors back to Rust to print.
ESLint has 2 different ways to define default options for a rule - `rule.meta.defaultOptions`, or `rule.meta.schema` containing a schema which defines default values.
We previously supported the former, but not the latter. This PR implements support for defining defaults via `rule.meta.schema`.
We compile the schema and apply it to options on JS side, using [`ajv`](https://www.npmjs.com/package/ajv), which is what ESLint uses. But this process only happens once before the start of linting, not once for every file, so the perf hit should be small.
This also prepares the way for validating options against the schema (#15631).
Previously we had a pattern of destructuring properties of global objects into top-level vars. e.g.:
```js
const { keys: ObjectKeys, hasOwn } = Object,
{ isArray } = Array;
```
Doing this is more performant, but it makes the code harder to read.
Instead, write code normally, and use a TSDown plugin to convert:
```js
const keys = Object.keys(obj);
```
to:
```js
import { ObjectKeys } from "../utils/globals.ts";
const keys = ObjectKeys(obj);
```
Defining all the destructured globals in a single file (`utils/globals.ts`) is advantageous for bundle size. If the TSDown plugin created top level `const ObjectKeys = Object.keys;` statements in each file, then you can end up with this in bundle as minifier doesn't de-duplicate:
```js
const ObjectKeys = Object.keys;
const ObjectKeys$1 = Object.keys;
const ObjectKeys$2 = Object.keys;
```
The globals we use have to be manually added to `utils/globals.ts`, but TSDown plugin warns when this needs to be done, and doesn't generate a broken build in the meantime.
This plugin is run in debug builds too, so that the tests will catch any bugs.
Throw an error if `rule.meta.defaultOptions` isn't JSON-serializable. This makes using `deepFreezeJsonArray` on it safe - can't lead to infinite loop if `defaultOptions` is self-referential.
Small optimization to options merging. Treat rules which define `defaultOptions` as an empty array the same as those which provide no `defaultOptions` at all. This makes options merging take a faster path for these rules.
Fix for an edge case.
Code for merging options was assuming that `defaultOptions` is a plain object with standard prototype, but that assumption doesn't actually hold. Alter the merging logic to ignore properties on prototype of objects in `defaultOptions`.
Use `const` where variables are not reassigned instead of `let`. Preparation for enabling ESLint's `prefer-const` rule (running as an Oxlint JS plugin) in our linting setup.
Part of #16018.
Implement `RuleTester` class for testing rules. Has same API as ESLint's `RuleTester`.
A few features not implemented yet:
* Fixes are not executed, or checked against provided `output`.
* Suggestions are not checked (we don't yet have support for suggestions in JS plugins).
* No use of config provided through `RuleTester` class constructor or via `RuleTester.setDefaultConfig`.
Pure refactor. Move a few lines of code into `loadPlugin`, to leave a sync function `registerPlugin` which contains the logic for registering a plugin.
Required for #16206, which will call this method directly, skipping `import`-ing the plugin from a module.
A step towards #15630. Where a rule provides default options, pass them to `create` function as `context.options`.
We don't yet have full options support (#14825), so this is incomplete. Once user can provide options for rules in config, those options need to be merged with default options.
Initially, we should probably do that merging on JS side (borrowing ESLint's code), and then later on port it to Rust.
Fix an edge case bug.
Gracefully handle plugins or files being linted that have file paths which are not valid UTF-8. Previously these would cause panics.
* Load plugin: Convert plugin path to a `file://...` URL on Rust side, instead of on JS side. File URLs can encode any path via URL escaping.
* Lint file: Convert path to `String` with `to_string_lossy`.
Avoiding calling `pathToFileURL` on JS side should also be more performant, but that's a very marginal benefit, as loading plugins is a one-time cost in each lint run.
This adds a dependency on `url` crate. It's a very popular and battle-tested crate. 430m downloads. https://crates.io/crates/url
I could not work out how to add tests for this, as it seems it's not possible to create a file on Mac whose path is not valid UTF-8. It *is* possible on Linux and Windows, but no way to check such a file into Git without it malfunctioning on Mac. We could create test files when tests run, dependent on platform, but I didn't want to create complication in the test infra just for this edge case. It *should* work!
Preparatory work for supporting rule options.
Store options for all rules in an `allOptions` array. Currently this array only contains a single value for empty options, but in future we'll populate it with options extracted from configs, sent over from Rust side.
Am adding this now, before full implementation (#14825), as it's required for rule tester.
Pure refactor. Split up `plugins/utils.ts` into 3 files in `utils` directory.
Most importantly, assertion functions are by themselves in an `asserts.ts` file. This paves the way for the next PR (#15946).
These checks are "sanity checks" only. The inputs to these functions comes from Rust code which we control, and would be `debug_assert!` if this was Rust. So only run these assertions in debug build.
Adapted from #15868. Enable `strictNullChecks` in TSConfig for `apps/oxlint`.
Rather than using `!` all over the place, I've added a function `assertIsNonNull`, which performs the same function. Idea is that we can later on produce a debug build where `assertIsNonNull` does an actual runtime assertion (rather than just a TS `asserts`) that the value is not `null` or `undefined`. We can use the debug build in tests, and catch any logic errors.
Unfortunately, TypeScript doesn't seem able to track side effects, so there seems to be no way to tell it "after calling `initAst()`, `ast` is always non-null". Don't want to switch to a having a `getAst` function which returns a non-null value, as that'd introduce unnecessary temp vars, and function calls, on hot paths. So this seems to be the best we can do without incurring runtime cost.
Fill out the TS type def for `RuleMeta`, based on [ESLint's docs](https://eslint.org/docs/latest/extend/custom-rules).
Most of the properties can be ignored at runtime in Oxlint (they're primarily useful for generating docs), but this has also surfaced a few features we need to implement. These are marked with "TODO" comments, and I'll also open issues for them shortly.
Fix#15325.
Some ESLint rules create a wrapped copy of `context`, in order to patch `report` and pass that wrapped context object to another rule.
Some rules also rely on unspecified details of ESLint's implementation - that `context` is formed of 2 layers - top layer being `RuleContext` (`id`, `report` etc), and bottom layer being `FileContext` (`filename`, `sourceText` etc).
This usage was broken in our implementation because:
1. We collapsed the 2 layers into 1.
2. We used a `Context` class which stored its internal data in a private property. When `context.report` was called with a different `this`, it failed because the private property couldn't be accessed from an object which isn't an instance of the class.
Fix this problem by:
1. Splitting `Context` into 2 layers like ESLint does.
2. Not using classes or private properties.
3. No methods/getters on context objects use `this`.
We now have:
* Separate `Context` object per rule, containing properties related to the rule - a plain object which uses data from closure scope.
* All rule `Context`s inherit from the same singleton `FILE_CONTEXT` object, which provides getters for properties related to the file (same for all rules).
Tests adapted from #15326.
If rule is defined using `createOnce`, and it returns an empty visitor (`before` / `after` hooks only), then don't run the hooks - the rule is a no-op.
Reason this is important is forward-compatibility with an optimization we could make where we calculate on Rust side whether AST contains any nodes that rules act on, and if it doesn't, skip calling into JS entirely. In that case, the `before` hook wouldn't run on those files.
We can't emulate that behavior entirely, but we can at least avoid users creating rules which *only* contain a `before` hook, and then finding that when we apply that optimization later on, it stops behaving as expected.
i.e. we make the `before` hook never be called for a rule defined like this:
```js
export default {
createOnce(context) {
return {
before() {
if (content.filename.endsWith('.tsx') {
context.report({ message: 'No TSX!', node: context.sourceCode.ast });
}
},
};
},
};
```
Small optimization. Store if a rule is fixable as a single `isFixable` boolean, instead of putting the whole `RuleMeta` object in `Context`.
This is more efficient on memory, and a boolean is faster to check as it's stored inline in the object, as opposed to strings which are stored as a reference to another object on the heap.
Boshen