Continuation of #21132. All uses of the `Uint32Array` view of the buffer in raw transfer code have been removed in preceding PRs. So we can now remove it entirely.
Fixes#20700.
Patch global `WeakMap` before loading plugins to make a `WeakMap` keyed by `context.sourceCode` act as if every file has a unique `context.sourceCode`.
The runtime cost of this patch is very low - for the use case of `WeakMap` keyed by `SourceCode`, it should be *faster* than an actual `WeakMap`, as no methods do actual hash map lookups. The implementation takes care not to leak memory even if plugins use `WeakMap`s extensively or in pathological patterns.
I rebased #20611 on top of this, and after this PR, 100% of `eslint-plugin-mocha`'s conformance tests pass.
It's pretty hacky to be patching globals, but I believe the implementation to be robust. My rationale for this solution is explained in detail in #20700.
Small optimization. Don't create temporary objects repeatedly just to pass props to `Object.defineProperty`. Re-use a single object and reuse it instead.
Perf optimization to visitor compilation.
Previously the flow for compiling and using a visitor was:
1. Call `initCompiledVisitor`.
2. Call `addVisitorToCompiled` with each visitor object.
3. Call `finalizeVisitor`.
4. Walk AST with compiled visitor.
`initCompiledVisitor` resets state after previous compilation.
Change this to:
1. Call `addVisitorToCompiled` with each visitor object.
2. Call `finalizeVisitor`.
3. Walk AST with compiled visitor.
4. Call `resetCompiledVisitor`.
The difference is that resetting state now happens at *end* of linting a file, rather than at the start of linting the *next* file.
The previous lifecycle design was motivated by needing to ensure state is reset even if an error is thrown during linting, but this case is now handled correctly by another means (#20166).
The reset-at-the-end lifecycle makes more sense.
It also has a perf benefit: Visit functions are freed earlier, so they can be garbage collected. This reduces the chance of ephemeral visit functions being pointlessly migrated to "old space" before they're deleted, which makes garbage collection cheaper. Also, they can now be garbage collected *between* linting files, while Rust side code is busy doing other tasks - rather than GC having to happen during linting on JS side, blocking main JS thread.
The `createOnce` API allows defining `before` and `after` hooks which run before / after each file. These can be used to track state.
It is therefore important that if the `before` hook is called, then the `after` hook is too.
It is possible for this invariant to be broken if an error is thrown during AST visitation. Because all rules run together in a single AST pass, an error in one rule can affect another. Previously in such cases, the `after` hook could fail to be called.
Fix this by calling `after` hooks in the `try` / `catch` which wraps the whole linting process.
The PR adds a comprehensive test fixture which tests behavior is correct in all circumstances, with errors being thrown at various stages, in the rule with the `after` hook, or in another rule.
Remove the old `lint_before_hook_error` and `lint_after_hook_error` test fixtures as the new fixture covers the same ground.
Fix a bug where internal state of visitor compilation could fail to be reset if an error is thrown during compilation, for example due to an invalid visitor being passed.
If this happens, previously some of the visitor functions defined for one file could stick around, and be erroneously included in the compiled visitor for next file, leading to them firing on that 2nd file when they shouldn't.
Fix that by resetting visitor compilation state in the `try` / `catch` which wraps the whole linting process.
In tests, run rules in a predictable order (the order they're defined in config).
This makes it possible to control in tests with multiple rules in what order they run, which makes the tests in next 2 PRs (#20166, #20167) feasible.
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.
We had pieces of commented-out code related to an alternative implementation of linter plugins using lazy deserialization. This code is now very out of date, and so largely pointless. Remove it.
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.
## 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
Pass `cwd` into `setupFileContext`, so it can be set per file, rather than being a global setting.
This is preamble to #17809. This PR is some of the changes from that one split off, so the diff on #17809 is focused just on that one change.
This PR does nothing in itself, as currently `process.cwd()` is always passed to `setupFileContext`.
Pure refactor. No point passing an object around through functions, when `parserServices` is always an empty object, as Oxlint provides no parser services. Just define it as an empty object inline.
Closes#12526.
Handle BOM on start of files in the same way that ESLint does - do not include it in the source text on JS side, but `context.sourceCode.hasBOM` evaluates to `true`.
Method:
* Alter `program.source_text` to trim off the BOM before passing AST to JS side.
* Add a `has_bom` flag to `RawTransferMetadata`.
* Add ability to add an offset in the conversion from UTF-8 to UTF-16 spans.
The result is that the file as it's seen on JS side is as if the BOM didn't exist (except for the `hasBOM` flag). Spans are converted accordingly in JS-side AST, and converted back when passing diagnostics back to Rust.
Implement CFG (or "code path analysis" as ESLint calls it).
This is the was the last remaining gap in our API.
The implementation is pretty slow, I imagine. I've pulled in ESLint's code for the actual control flow graph construction, and copied ESLint's code for the rest.
Fitting with how we combine enter and exit nodes for leaf nodes (`Identifier`s etc) required skipping creating "steps" for exiting leaf nodes, which should also be a perf gain, as they're so common. But that apart, I've made no effort to optimize it. There's plenty of scope for improving it. I've added comments with some starting points for doing that, and will open an issue in the hope someone would like to pick up on that.
I also cannot for the life of me figure out how to add the CFG event handlers (`onCodePathStart` etc) to `VisitorObject` type, without breaking everything. Me and Claude went at it for about an hour, and got nowhere. So I'm just leaving that for now.
While walking the AST, `ancestors` array is pushed to when entering a node, and popped again when exiting. `ancestors` is empty at start of AST traversal and, because each push is followed by a pop, `ancestors` is empty again at the end of AST traversal.
`ancestors` is a singleton, reused for every AST traversal.
`ancestors` is used by `esquery` when determining if a node matches a selector. Note that it's not the same as what's returned from `sourceCode.getAncestors()`. `ancestors` is used only internally by `esquery`, so only way to see what it contains is by observing the behavior of selectors.
This PR fixes a bug where `ancestors` was not reset (emptied) if an error is thrown during AST traversal. In that case `ancestors` was left in the state it was in at time that the error was thrown, and this affects behavior of selectors when visiting the *next* AST.
Fix it by resetting `ancestors` of there's an error. Also add a bunch of debug assertions to ensure `ancestors` remains in sync with AST traversal at all times.
Pure refactor. Alter how state is reset after linting a file.
Move logic for resetting state into a new function `resetStateAfterError` which `RuleTester` can also use. This ensures that `RuleTester` follows the same code path as the actual linter.
Add error messages to all `debugAssert()` calls, for easier debugging.
When running Oxlint normally, we can easily figure out where errors are coming from stack traces, but in conformance test snapshot, line and column are removed from errors in the snapshot, so it's less easy.
Only alters comments.
* Make sure all `// @ts-expect-error` comments include "TODO" where it's an issue we need to fix.
* Format all the "TODO" comments consistently.
Pure refactor. Just shorten the code for some debug assertions. They don't need error messages - if any fail, we can find out what's failing from stack trace.
- This PR implements a naive, inefficient solution to #16359.
- To establish current and expected behavior, this PR serializes
rust-resolved and -overridden globals once for every file.
## Future work
- [ ] Add test for globals from the `env` field. Implement support if
not currently supported.
- [ ] Optimization: we want to serialize `globals` for all files only
once at the start of the lint run.
## AI Usage
- This PR was drafted by Claude Sonnet 4.5 in Plan + Agent mode, then
manually reviewed and iterated on.
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
We expect the common case to be that linting does not report any errors - because if there are errors, user will generally fix them!
Make a fast path for this common case - just return `null` from `lintFile`, skipping JSON serialization on JS side, and deserialization on Rust side.
As @camc314 pointed out in https://github.com/oxc-project/oxc/pull/16296#pullrequestreview-3524629951:
It's probably cheaper to leave the `messageId` field present in the JSON sent over to Rust and let `serde` handle skipping it, than to loop through all the diagnostics on JS side to remove it.
`RuleTester` should be moved into a separate NPM package (#16018), at which point we can do a different build for that package, and not have `messageId` stored in `DiagnosticReport` in the first place in the main build. So then this issue will become moot.
Closes#15630.
* #16170 added support for default options defined in the rule.
* #16215 added support for options defined in configs.
* #16217 implemented the logic for merging the 2 but only applied it in `RuleTester`.
Bring these elements together, and support merging default options from rule into options provided in config.
- Implements #14825
- Rebased #15822. Refactored. Pruned the slop. Tests passing.
### Big changes from the original approach
- [x] Options IDs are stored in their own arrays. This was laid out in
#16158. The original approach was to store `ruleId`-`optionId` pairs as
tuples in an array - several heap allocated objects and excessive
pointer indirection.
- [x] 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.
<details>
<summary>
#### Smaller Changes
</summary>
- Removed the extra fixture ("working version"). There were no
meaningful changes compared to the first fixture. Probably slop.
</details>
---------
Co-authored-by: Peter Wagenet <peter@wagenet.us>
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
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`.
Store `messageId` in `DiagnosticReport` objects. It's not required for sending to Rust, but it is needed for rule tester (#16206).
I think it's fine to add a little code that's redundant in the main runtime, because it only has an overhead when a rule produces diagnostics, which should be an uncommon path.
If there's an error during AST visitation, ensure state is reset to free memory.
This is only observable if user were to schedule some code in a later tick to access e.g. `sourceCode.text`, and that runs before the next file is linted. I've not added tests, as it'd be hard to write one which isn't flaky.
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.
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.
Rename assertion functions to clarify that they're not "real" assertions which we can rely on for validating input, and are only for compile-time testing or debug assertions.
`typeAssertIs` is not a great name, but I couldn't come up with anything better. `debugAssertIs` would be not be accurate, as it'd imply that in debug builds in performs an actual assertion, which it doesn't.
In any case, we should replace `typeAssertIs` with specialized assertion functions which do actually perform runtime assertions in debug builds (e.g. `typeAssertIs<Fix>()` -> `debugAssertIsFix()`).
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.
Small optimization to linter plugins JS code. Use the cheapest `for` loops, rather than `for of`, and avoid accessing `length` on each turn of the loop.
overlookmotel