## Summary
- extract directive justifications before parsing directive labels, matching ESLint
- return an empty rule value for rule-less directives while preserving their justification
- cover block, enable, line, and next-line directives with both eslint and oxlint prefixes
Closes#25556
## 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
Pure refactor, except for fixing a TS type, and correcting some comments. Runtime behavior is unaltered.
- `LABEL_PATTERN` captures the directive type as well as the label, so the multi-line check is a
single comparison, and `type` no longer comes from `label.slice(7)`.
- Numbered capture groups instead of named ones, so the match check is just `match === null`.
- `match` is no longer reused for a second, unrelated regex.
- `justification` is always set, so it's not optional - correct the type def accordingly.
- Simplify splitting off `justification`.
- Doc comment referenced `match_disable_directive` / `match_enable_directive`. The Rust function is
`match_directive` - correct the comment.
`LINE_DIRECTIVE_PATTERN` had a stray hyphen (`disable-(?:-next)?-line`), so it only matched
`disable--line` / `disable--next-line` - strings `LABEL_PATTERN` can never produce. Therefore it was dead code - remove it.
If the regex _was_ correctly written, the behavior would be wrong. Looks like this is a hangover from the code being taken from ESLint.
Add more debug assertions to debug build-only code which checks the validity of comments and tokens.
Make sure all comments and tokens have spans which are wihin bounds of source text. In particular, this will surface any problems where the source text contains a BOM, which causes us to adjust spans on Rust side when trimming off the BOM.
Tiny perf optimization to linter plugins.
`if (x === false)` is longer than `if (!x)`, but is more performant. The 1st compiles down to a simple equality check between `<value x>` and `kFalse` constant, whereas the 2nd is more like `coerceToBoolean(<value x>) === kFalse` - a more expensive operation.
This follows our convention in all JS plugins code of always using explicit `=== false` / `=== true` comparisons.
Pure refactor. Use debug asserts instead of type assertions in code for deserializing comments and tokns. This gives better error messages if we break the invariants in tests. Makes no difference in release build.
Small optimization to `context.sourceCode.getDisableDirectives`.
`Shebang` comment can only be the first comment in a file, so there's no need to check "is it a `Shebang`?" for every comment in a loop. Instead, just check the first comment before the loop.
Also add type annotation for the function's return type.
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>
Follow-up of #22238, same logic applied to `Token` class
## Problem
`Token` instances are object-pooled and reused across files, similar to
`Comment`.
Their `loc` property is defined as a **prototype getter**, which means
it is not an own
property and is therefore invisible to the spread operator:
```js
const spread = { ...token };
"loc" in spread; // false ❌
```
This silently drops `loc` whenever a JS plugin (or rule) spreads a token
— e.g. when
building a diagnostic from `{ ...token, message: "…" }`.
`JSON.stringify` had the same problem and was patched with a `toJSON()`
workaround.
A separate `Object.defineProperty(Token.prototype, "loc", { enumerable:
true })` call
was used to make `loc` show up in `for…in` iteration, but it still
didn't fix spread.
## Fix
Define `loc` as an own accessor property on every `Token` instance using
`__defineGetter__`. Own properties are visible to spread,
`JSON.stringify,` and
`for…in` iteration, so all three work without any extra workarounds.
The getter function is defined in the class static block and captured in
a `const`
(`getTokenLoc`). Using a `const` rather than a `let` lets V8 skip
reassignment checks
at the `defineGetter(this, "loc", getTokenLoc)` call site — a pattern
already
established in the codebase for `resetLoc`.
As a result:
- `toJSON()` is removed — `JSON.stringify` now works via the own
property directly.
- `Object.defineProperty(Token.prototype, "loc", { enumerable: true })`
is removed —
no longer needed.
## Test
A spread assertion is added to the existing `tokens` fixture:
```js
const spread = { ...firstToken };
assert("loc" in spread); // was false before this fix
assert.deepEqual(spread.loc, firstToken.loc);
```
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
## Summary: Fixing unicorn/expiring-todo-comments via jsPlugin
## Problem
The `unicorn/expiring-todo-comments` rule failed when run through
oxlint's jsPlugin system with error:
```TypeError: Either `node` or `loc` is required```
The rule spreads Comment instances (`{...comment}`) to process each line of multi-line comments, but lost the `loc` property because:
- Comment's `loc` was a prototype getter (not an own enumerable property)
- Spread only copies own enumerable properties
## Solution
Define `loc` as an own accessor property on every `Comment` instance using
`__defineGetter__`. Own properties are visible to spread, `JSON.stringify,` and
`for…in` iteration, so all three work without any extra workarounds.
The getter function is defined in the class static block and captured in a `const`
(`getCommentLoc`). Using a `const` rather than a `let` lets V8 skip reassignment checks
at the `defineGetter(this, "loc", getCommentLoc)` call site — a pattern already
established in the codebase for `resetCommentLoc`.
As a result:
- `toJSON()` is removed — `JSON.stringify` now works via the own property directly.
- `Object.defineProperty(Comment.prototype, "loc", { enumerable: true })` is removed —
no longer needed.
## Test Results
- ✅ unicorn/expiring-todo-comments rule works (detects expired TODOs)
- ✅ updated comments fixture pass
- ✅ All untouched existing tests pass
---------
Co-authored-by: overlookmotel <theoverlookmotel@gmail.com>
Micro-optimization.`Token` and `Comment` classes create functions defined in static blocks to reset the value of `#loc` private property on class instances. Copy these functions into `const` vars, to avoid checks at these functions' call sites. Previously they were stored in `let` bindings, which incurs a check at each call site (because the value might have been re-assigned). The difference in Turbofan-optimized code is just an 8-byte load and a comparison, but why keep that when we can lose it?
Discovered the perf difference while digging into #22238.
Tiny optimization to CFG visitor. Bitwise OR is slightly cheaper than subtraction, and also gives V8 a guaranteed type (`i32` aka SMI) for the value being put into the array.
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.
Use `int32` (`Int32Array`) instead of `uint32`(`Uint32Array`) for getting data from buffer in raw transfer deserializer. The buffer is 2 GiB in size, so all offsets within the buffer can be represented as a `u31` which can be stored in an `i32` without any loss, and with no risk of being interpreted as negative numbers.
Same as in #21129, the advantage of fetching offsets from an `Int32Array` is that V8 statically knows the value can be stored in an SMI, its native integer type, without any range checks.
`>>` is cheaper than `>>>` because `>>` produces a 32-bit _signed_ integer which is V8's native number type (SMI). `>>>` produces a 32-bit _unsigned_ integer, which needs to be boxed and stored on the heap.
Source text in raw transfer is limited to 1 GiB, and therefore source offsets, number of lines, number of tokens, and number of comments after all less than `1 << 30`. Therefore even the sum of 2 of them cannot reach `1 << 31` (the maximum positive integer which can be stored as a positive signed 31-bit int. Therefore it's safe to use `>>` in these binary loops.
Refacfor. In a strange pattern, we were injecting `getNodeLoc` into `deserialize.js` module on every call to `deserializeProgramOnly`. It's a static function, so just import it.
Optimize raw transfer string decoding.
Replace `TextDecoder("utf8")` with `Buffer.prototype.utf8Slice` to decode Rust UTF-8 strings to JS strings.
Benchmarks show this is an average 5% speed-up in `deserializeStr`, though the reason why is unclear. It skips various checks, and avoids creating a temporary `Uint8Array` (which is required with `TextDecoder.decode`) which trims 60 bytes of the size of the assembly created for `deserializeStr`. But that's not enough to explain the speed-up 🤷. Anyway, take the win.
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.
`loc` property of `Token`s and `Comment`s is a getter on these classes' prototypes. Add a `toJSON` method to both classes so `JSON.stringify` includes `loc` in the generated JSON.
Speed up `computeLoc`, which converts a `start` + `end` offset pair to a `Location` of form `{ start: { line: ?, column: ? }, end: { line: ?, column: ? } }`.
`computeLoc` is only called with `start` and `end` pairs from AST nodes, tokens, and comments, which are all produced by Oxc's parser, so it's guaranteed that `start < end`.
Use this invariant for 2 optimizations:
1. Fast path for common case where `start` and `end` are on same line.
2. Reduce range of lines which are searched looking for end to only lines which are after the line `start` is on. `end` can't be on an earlier line, so there's not point search them.
Small optimization. Don't create temporary objects repeatedly just to pass props to `Object.defineProperty`. Re-use a single object and reuse it instead.
Add an object pool for `Location` objects, and recycle them across files.
Usually, only a minority of AST nodes, tokens, and comments will have their `loc` property accessed, so the pool is not pre-populated - `Location` objects are only created on demand.
Like the other object pools, once warmed up over the first batch of files, the fast path for "use an existing object from the pool" will be consistently taken and no further objects will be allocated. All `Location` objects will soon graduate to "old space" where they don't impede or trigger minor GC.
Similar to #20479. `tokensWithLoc` and `commentsWithLoc` contain tokens/comments on which `loc` property has been accessed.
Previously these arrays were grown with `.push(token)`, and shrunk at end of linting file with `tokensWithLoc.length = 0;`. Problem with that is that setting length to 0 frees the array's backing allocation, which means it has to reallocate again on next file when a token's `loc` is accessed.
Instead, never shrink these arrays, and track the active length in separate variables. After warm-up over first batch of files, these arrays will graduate to "old space" and just sit there without further allocations.
Unlike #20479, we don't use `Uint32Array`s as `loc` is calculated lazily, and `Token`'s / `Comment`s don't know what their index is in the `cachedTokens` / `cachedComments` arrays. Adding an `#index` field to `Token` and `Comment` would bloat every instance of these classes by 8 bytes.
Optimize storing regex tokens so they can have their `regex` property set back to `undefined` at the end of linting a file. Previously we pushed the `Token`s into an `Array` and then reset the array with `tokensWithRegex.length = 0;` at the end.
This had a problem: When you set an array's length to 0, V8 releases the whole backing allocation, which means that on the next file, it has to allocate again. Worse, that new allocation is made in "new space", and if there's a lot of object creation in rules while linting the file, it will be copied during minor GC, and may even get graduated to "old space". All of that is expensive.
Instead, use a `Uint32Array` to store indexes, for the same reasons as in #20477.
Possibly the most absurd optimization I've ever encountered.
```diff
- if (regexObjects.length > regexIndex) {
+ if (regexIndex < regexObjects.length) {
regex = regexObjects[regexIndex];
}
```
There is no semantic difference whatsoever between `regexObjects.length > regexIndex` and `regexIndex < regexObjects.length`. But V8's 2nd-tier Maglev compiler treats them differently. In the`>` version, `regexObjects[regexIndex]` incurs a bounds check, whereas in the new `<` version, no bounds check.
This is a micro-optimization, but this function is extremely hot, so it may actually move the needle in code that includes a lot of regexes. The same optimization is also applied in #20480.
Claude found this oddity by reading V8's source code!
Fix a JS-side memory leak from tokens and comments.
## The problem
V8 uses "SlicedString" optimization to avoid copying string data. `sourceText.slice(10, 20)` does not copy bytes 10..20 to a new string, but references the original string data of `sourceText` (much like `&str` in Rust).
#19978 and #20362 introduced caching and recycling of `Token` and `Comment` objects. This was a large performance gain.
However, it contained a flaw. The problem is that `Token` objects now live forever, and they contain little slices of the source text in their `value` property. This means they prevent the source text which they reference from being garbage-collected, and therefore it lives forever too (or at least until another file with an equal or greater number of tokens overwrites the `value` property of all the `Token` objects).
## Solution in this PR
Fix this by setting the `value` property of all `Token`s and `Comment`s to `null` after linting a file in `resetTokens` and `resetComments`.
## Implementation details
This requires some additional book-keeping to track which tokens have been deserialized and which haven't, so that `resetTokens` / `resetComments` do the minimum amount of work necessary - only resetting tokens / comments which actually had their `value` property set in the first place.
The indexes of tokens are stored in 2 x `Uint32Array`s `deserializedTokenIndexes` and `deserializedCommentIndexes`. A simpler solution would have been to store the `Token` / `Comment` objects themselves in a plain `Array`, but `Uint32Array` has 3 advantages:
* 4 bytes to store index (`u32`) per token/comment, instead of 8 bytes to store a pointer to the `Token` / `Comment` itself.
* Faster to write to `Uint32Array` - no write barrier checks.
* On GC runs, a `Uint32Array` is known not to contain references to any other heap objects, so GC tracing doesn't need to walk it, which it would with an `Array`.
`Uint32Array` does have one disadvantage over `Array`:
* 1 level of indirection in `resetTokens` / `resetComments`. You have to read the index, then index in to `cachedTokens` array to get the `Token` object, in order to reset it.
The latter will only have a significant effect when there's a large number of tokens/comments to reset. We avoid this in the worst case where *all* tokens have been deserialized (e.g. by accessing `ast.tokens`). When `allTokensDeserialized === true`, `resetTokens` just loops through `cachedTokens` itself, and ignores the `Uint32Array` entirely.
## Future potential optimizations
It's probably not necessary to reset all tokens quite so aggressively when all tokens have been deserialized. If a rule has done that on 1 file, chances are it'll do it again on the next file, at which point most `Token`s get their `value` property overwritten with slices of *that* source text anyway.
It's acceptable for source texts to hang around in memory for a bit, just not forever. We could choose to do token resetting only e.g. once every 10 files, to reduce the cost.
Possibly clean-up operations could also run in next tick, so they happen in parallel while Rust side is prepping the next file to send over to JS, rather than blocking.
Previously `sourceCode.getCommentsBefore` and `sourceCode.getCommentsAfter` found all comments directly before / directly after a node by finding comments before / after the node and then checking if anything other than whitespace between them by using a regex search on slices of source text.
Replace this with a more performant algorithm. Just find consecutive runs of comments before/after the node in the tokens-and-comments buffer. When a token is found, that's the end of the run of comments. No need for regexes and text searches.
This does require generating the tokens-and-comments buffer, which is not so cheap. But it's likely that if a rule calls one of these methods on one node, it'll also call it on many others, so the savings likely outweigh the cost.
Performance improvement to tokens and comments APIs.
## The problem
Previously, all tokens and comments methods would deserialize *all* tokens/comments into an array of `Token` / `Comment` / `Token | Comment` objects, and then binary search through those arrays to find the token(s) / comment(s) they're looking for.
This has 2 major disadvantages:
1. Files typically contain *a lot* of tokens (even more than the number of AST nodes). Deserializing them all is very costly (up to 30% of total Oxlint runtime when run with only a JS rule which just calls a tokens-related method).
2. The binary searches these methods do are quite expensive. Even in TurboFan-optimized code, accessing `token.start` involves getting pointer to the `Token` object from the `tokens` array, an "is this object a `Token`?" safety check, then reading the `start` field from the `Token` - all just to access a single `u32`, and that happens over and over.
## This PR's solution
Solve both these problems by making tokens and comments methods read `start` / `end` offsets directly from the buffers which contain the tokens/comments data.
This data is tightly packed in memory, and strongly typed (read from `Uint32Array`s), so getting `start` / `end` of a token requires no indirection and no type checks.
More importantly, it removes the need to deserialize all tokens / comments upfront. The desired token(s) are located, touching only the buffer, and then *only* the ones which need to be returned to rule code are deserialized into JS objects.
If a rule accesses `ast.tokens`, `ast.comments`, or `sourceCode.tokensAndComments` then all tokens / comments need to be deserialized, as they're all returned to the rule as an array - but that's unavoidable. This PR doesn't make that any cheaper, but it doesn't make it measurably more costly either.
But where no rule requires the full array of tokens / comments, and they only use token/comment search methods (e.g. `getFirstToken`, `getCommentsBefore`), a great deal of work will be saved. This covers the vast majority of rules.
## Implementation details
The main complication is the `includeComments` option to tokens methods. When `true`, search needs to be over a combined set of both tokens and comments.
When `includeComments: true` option is passed to a tokens method, a buffer is created containing data about all tokens and comments, interleaved in source code order. This buffer can then be used for binary search in tokens methods.
Whether each token / comment has been deserialized already or not is tracked by a "deserialized" flag in the tokens/comments buffers. Each token / comment in the buffer is 16 bytes. This flag lives in byte 15. For tokens, this byte is always already 0 in the buffer when it arrives from Rust side. For comments, we manually set `comment.content = CommentContent::None;` for every comment on Rust side. `comment.content` is positioned at byte 15 in the `Comment` struct, and `CommentContent::None` is stored as 0.
## Possible future improvements
### SoA storage
Binary search operates only on `start` field of tokens / comments, which are 16 bytes apart in the buffer. It would be more efficient if tokens were stored in struct-of-arrays (SoA) style so all `start` values were tightly packed together. This would reduce CPU cache misses in the hot loops of binary searches.
### Pre-compute tokens-and-comments buffer on Rust side
The buffer containing tokens and comments, required to support `includeComments: true`, is currently generated on JS side (but lazily). We could move that to Rust side, which would be faster. However, it might be redundant work in many cases because the buffer is only required if a rule uses `includeComments: true`.
We could alternatively keep the laziness optimization, by calling back into Rust to build the buffer on demand - but JS-Rust calls have a cost too. Maybe communicating via `Atomics` would be faster than an actual function call?
If we had a way to share buffers with WASM, optimal solution might be to generate the buffer lazily (as now) but in WASM, which would be faster for this kind of pure number-crunching, but without the overhead of calling into Rust.
Optimization to comments deserialization. Files which have no comments are probably fairly common. Add a fast path for this case. We have to branch on `length === 0` later on anyway, so might as well branch earlier and exit.
Previously the entire AST had to be deserialized when calling any comments method. This was purely to get the `hashbang` property of `Program`, which has to be added to start of comments array as a `Shebang` comment.
Usually the AST has to be deserialized anyway in order to visit it, but some rules might early exit and return an empty visitor after checking if a certain comment is present. In those cases, it was extremely wasteful to deserialize the whole AST just to check for `hashbang`.
Instead, add a `Line` comment to start of `Vec<Comment>` on Rust side if a hashbang is present. If there are a lot of comments, this involves shifting them all up which could be expensive, but files with hashbangs are rare, and this change enables other optimizations coming in future PRs that rely on all comments being present in the buffer.
Pure refactor. Move `sourceCode` methods related to comments into a separate file `comments_methods.ts`. Does not alter any of the code, just moves it.
Apply the same optimization as #19978 to comments - hold a pool of `Comment` objects, and re-use those objects rather than creating new objects each time.
Same as with `Token`s, `loc` property is a getter which calculates `loc` lazily, and caches it in a private property.
Small optimization.
Initialize `Token` class instances with `null` fields. `null` is one of the cheapest types to create as it's represented in V8 as a static 64-bit value - likely cheaper than creating a string, even an empty one. These placeholder values are immediately overwritten a few lines later, so it doesn't matter what they are initially.
Fixes#19971.
Implement `sourceCode.markVariableAsUsed`.
It has some shortcomings at present - it does mark vars as used, so any other JS plugins can see that, but it doesn't communicate this info back to Rust side, so `no-unused-vars` doesn't act on it. But at least it doesn't cause a crash with "unimplemented" error.
The remaining issue is tracked in #20350.
Perf optimization to merging visit function when compiling AST visitors.
Previously we took an array of `VisitProp` objects, extracted the `fn` property from each into a temporary array, and then called the merger function using spread `merger(...visitFns)`.
Remove the temp array and the mapping entirely by just passing the arrays of `VisitProp` objects directly to the merger function, and destructuring it internally.
This is simpler and also more performant.
My concern with taking this approach originally was that the merged functions might capture the `VisitProp[]` passed in to outer function in its scope, which would bloat them. These functions are extremely hot - called 1,000s or 10,000s of times during AST visitation - so even a small slow-down in these functions would be measurable.
But analysis using V8 built-in functions (`%DebugPrint`) to inspect what these functions capture in their scopes reveals that this concern was unwarranted - they only capture the variables that they actually use, and do not capture to `VisitProp[]` arrays. So they are unchanged after this PR.
Boshen