fix(linter/plugins): make spreading Comment instances keep loc property#22238
Conversation
bf5ebe9 to
14e310b
Compare
|
Thanks for this. Have you by any chance measured perf before/after this change? Unfortunately we don't have any JS benchmarks at present, so we measure on a rather ad hoc basis, e.g. running Oxlint with some JS plugins on VS Code repo. I'm asking because generally we've tried to avoid |
No, but if the procedure is written somewhere I could do that. I've tried to find another way but couldn't come up with another one, but will be willing to try if you have some in mind. Do you know some jsPlugins that exercise comments? unicorn doesn't work without that change so I can't compare with before. |
14e310b to
d6eb543
Compare
|
@overlookmotel did bench the branch with eslint-comments (thanks HansAuger on discord for the suggestion) and a custom plugin to specifically stress loc access: Bench ResultsBenchmarked on VS Code repo (~10,500 files, 24 threads), 5 runs with 2 warmup each using hyperfine. stress-loc (10× .loc access per comment — amplifies overhead)
eslint-comments (7 real rules — no-use, disable-enable-pair, etc.)
VerdictNo measurable regression. Both differences are within noise (standard deviations overlap). The Proxy overhead on The fix is safe to land from a performance standpoint. Configuration used:Node v24.14.0 stress-locimport type { Comment, Plugin, Rule } from "#oxlint/plugins";
const stressLocRule: Rule = {
create(context) {
const { sourceCode } = context;
let programNode: any;
let totalComments = 0;
return {
Program(node) {
programNode = node;
const comments = sourceCode.getAllComments();
for (const comment of comments) {
totalComments++;
// 10 iterations per comment to amplify any overhead signal.
for (let i = 0; i < 10; i++) {
const loc = comment.loc;
const _startLine = loc.start.line;
const _startColumn = loc.start.column;
const _endLine = loc.end.line;
const _endColumn = loc.end.column;
}
const _type = comment.type;
const _value = comment.value;
const _range = comment.range;
}
},
"Program:exit"() {
if (totalComments > 0 && programNode) {
context.report({ node: programNode, message: `processed ${totalComments} comments` });
}
},
};
},
};
const plugin: Plugin = {
meta: { name: "comment-perf" },
rules: { "stress-loc": stressLocRule },
};
export default plugin;Configured with: {
"jsPlugins": ["./test/fixtures/comment_perf/plugin.ts"],
"rules": {
"comment-perf/stress-loc": "error"
}
}eslint-commentsInstalled as {
"jsPlugins": [{ "name": "eslint-comments", "specifier": "@eslint-community/eslint-plugin-eslint-comments" }],
"rules": {
"eslint-comments/disable-enable-pair": "error",
"eslint-comments/no-duplicate-disable": "error",
"eslint-comments/no-unlimited-disable": "error",
"eslint-comments/no-restricted-disable": "error",
"eslint-comments/no-use": "error",
"eslint-comments/require-description": "error",
"eslint-comments/no-aggregating-enable": "error"
// Skipped no-unused-disable (heavy patching, likely incompatible).
}
} |
d6eb543 to
a261679
Compare
|
If you have time, could you try altering the stress rule to this: const stressLocRule: Rule = {
create(context) {
const { sourceCode } = context;
return {
Program(node) {
const comments = sourceCode.getAllComments();
let count = 0;
// 10 iterations per comment to amplify any overhead signal
for (let i = 0; i < 10; i++) {
for (const comment of comments) {
const loc = comment.loc;
count += loc.start.line;
count += loc.start.column;
count += loc.end.line;
count += loc.end.column;
count += comment.type.length;
count += comment.value.length;
count += comment.range[0];
count += comment.range[1];
}
}
if (count > 0) {
context.report({ node, message: `got ${count} count` });
}
},
};
},
};In your original version, possible that that
Also, if I've understood |
Microbenchmark Resultssource: https://gist.github.com/KuSh/7af1829312429dd6e638a47de430a86a No reset (steady state after warmup)
With reset (per file cycle)
ConclusionsWithout reset, SR ≈ B/E — all three within noise. After warmup, SR's self-replaced data properties are as fast as B/E's getter + With reset, SR collapses — B/E are the only viable approach — correct spread behavior, cheap reset ( |
Complete Benchmark Results
The stress file isolates just the |
|
Excellent work! Thanks loads for digging deep into this. I have to say I'm mystified why "B:modvar" is performing better than "C:proto" with reset. As I mentioned on Discord, I'm mostly away from keyboard until next week. I'll take a closer look as soon as I'm back in the saddle. Again, thanks loads for getting into this in depth. For the record, more context here: https://discord.com/channels/1079625926024900739/1080712072012238858/threads/1508788046110396476 |
e527b4d to
ef640fb
Compare
ef640fb to
e89fee8
Compare
|
I'm going through this now. Have moved the docs corrections to #22891, since they're orthogonal to this PR (so deleted last commit from this PR), and rebased on main. Hope you don't mind me fiddling with your work. |
|
No problem at all! Thanks for that |
|
OK, I've dug into this. Firstly, there was a problem with the benchmark. If you changed the order of the variants, the results changed. Claude's explanation:
Adapted version which fixes this: https://gist.github.com/overlookmotel/9687d97e8decbb08787e03cc2e888bdb Results
Run on Mac Mini M4 Pro. Upshot
"B:modvar" is 13.5x slower to construct a However, that doesn't matter too much! As So, in short, I think we can do this! Next stepsCodeI think variant D is the winner. That's IMO what we should go with in this PR. Note: The weird pattern with copying the functions defined in the static block into If I may ask, please try to avoid any extraneous changes from the previous versions surviving. Small diff is the name of the game, and AI doesn't seem to always get that. TestsCould you please reduce the test case to not require Or testing the spread behavior could probably be folded into one of the existing fixtures that tests comments, rather than introducing a new fixture. TokensWe should probably apply the same change to @KuSh If you can be bothered, that'd be great (in a separate PR). Otherwise no worries, I'll do it. Note: Unfortunately we should not apply this change to AST nodes too. AST node objects are not pooled, so the increased construction cost would be a killer. We'll have to find another way to tackle that. Store
|
#22919) 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.
…#22928) Oxlint's JS-side code is processed by a TSDown plugin that transforms global property accesses to `const`s imported from `globals.ts`. e.g.: ```js function foo(obj) { return Object.keys(obj); } ``` is transformed to: ```js import { ObjectKeys } from "./utils/globals.ts"; function foo(obj) { return ObjectKeys(obj); } ``` The purpose of this transform is: 1\. Better perf - avoid a property lookup/check on each call. 2\. More robust in the face of user code (plugins) altering globals e.g. `Object.keys = function myWeirdFunction() {}`. While delving into the assembly that V8 produces while reviewing #22238, I discovered that in fact the transform was having a _negative_ effect on perf, because TSDown puts the global `const`s in a separate chunk `global.js`. Because the consts are in a different module, this actually makes them _more_ costly to access, rather than less. In fact, it turns out that TurboFan doesn't insert a check at all in the original code - it instead has machinery to trigger de-opt if the global is reassigned. So the original perf rationale for this transform was completely misguided - it doesn't in fact produce a perf gain at all! We still want to keep it for the robustness advantage, but we don't want it to be perf _loss_, as it was before this PR. This PR fixes the perf by instead defining consts inline in each file. The example above is now transformed to: ```js const ObjectKeys = Object.keys; function foo(obj) { return ObjectKeys(obj); } ``` This also removes the need to manually maintain the `globals.ts` file. It's now automatic. Additionally, improve the transform in a few minor ways: 1. Support deeper nested global props e.g. `Object.prototype.toString`. 2. Skip converting e.g. `Array.prototype.slice.call(arguments)` (because `.call` requires `this`, so `ArrayPrototypeSliceCall(arguments)` is incorrect). 3. Do replace methods which take `this` (e.g. `Promise.resolve`) when they're not used in a method call. Note: The robustness advantage of this transform may appear to be marginal, and not worth the bother of having this transform at all. Personally, I think it's worthwhile. Oxlint JS-side code has access to a slice of Rust's memory as a `Uint8Array`. Writing to that buffer could cause UB, reading/writing out of bounds, or other serious problems on Rust side, so IMO it's critical to protect against that in every way we can.
d27809d to
89b0ef2
Compare
|
@overlookmotel done, I'll create another PR for Token |
…lugin
Adds a test fixture to exercise the jsPlugin infrastructure with the
real eslint-plugin-unicorn package. This test exposes a bug where
context.report({ loc: ..., messageId: ... }) fails with 'Either node
or loc is required'.
Changes:
- Add eslint-plugin-unicorn as dev dependency to apps/oxlint
- Add test fixture at test/fixtures/unicorn_expiring_todo_comments/
- Use alias 'unicorn-js' since 'unicorn' is reserved for native rules
- Replace private #loc field with WeakMap storage - Add Proxy in constructor to make loc enumerable for spread - Fix unicorn/expiring-todo-comments rule failing with spread comments
Replace Proxy (ownKeys/getOwnPropertyDescriptor traps) with Object.defineProperty + #loc private field for Comment.loc. Variant B: static #LOC_DESC shared getter via Object.defineProperty in constructor, Benchmarks (VS Code src/, 10 rules x 10 iterations): Proxy (A): 21.07s Variant B: 20.16s (4.5% faster)
…-plugin-unicorn dep and integrate spread test in the comment fixture
aa7fe1e to
12fada5
Compare
12fada5 to
ef3b353
Compare
overlookmotel
left a comment
There was a problem hiding this comment.
Thank you!
I pushed a few trivial commits - set field order back to what it was before, added comments, and simplified the tests.
The feedback I gave about the tests before was completely wrong! Usually we report not assert, but in this case it seems we diverge from that rule. Sorry I wasted your time with that.
Hope you don't mind me tweaking. Keen to get this merged!
There was a problem hiding this comment.
Pull request overview
Fixes oxlint’s jsPlugin interoperability for rules (notably unicorn/expiring-todo-comments) that clone comment objects via object spread, by ensuring Comment.loc is preserved as an own enumerable property.
Changes:
- Defines
locas an own accessor property on eachCommentinstance via__defineGetter__, while still caching the computed value in#loc. - Removes the
toJSON()workaround and prototype-levellocenumerability tweak, since serialization/spread now work through the instance property. - Extends the comments plugin fixture to assert
{ ...comment }includeslocand preserves object identity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/oxlint/src-js/plugins/comments.ts | Changes Comment.loc to be an own enumerable accessor so spreading/serialization keep loc, while retaining lazy caching/reset behavior. |
| apps/oxlint/test/fixtures/comments/plugin.ts | Updates fixture assertions to validate loc survives object spread cloning. |
Comment instances keep loc property
…erty (#22947) 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>
|
Thanks a lot for the support @overlookmotel, glad it is finally merged. |
# Oxlint ### 🚀 Features - e805174 linter: Add schema for `jest/vitest/max-expects` (#23105) (Sysix) - 7850577 linter: Add schema for `jest/vitest/expect-expect` (#23104) (Sysix) - 75f641a linter: Add schema for `jest/vitest/consistent-test-it` (#23103) (Sysix) - 5125f89 linter/unicorn: Support no-null `checkArguments` option (#23098) (camc314) - b8b9797 linter: Add schema for `import-max-dependencies` (#23096) (Sysix) - 65cb47a linter/eslint: Support no-unused-expressions `ignoreDirectives` option (#23097) (camc314) - f6c36d5 linter: Add schema for `import/prefer-default-export` (#23091) (Sysix) - 0d4a5d1 linter: Add schema for `eslint/sort-vars` (#23090) (Sysix) - fdb5bf5 linter: Add schema for `eslint/radix` (#23082) (Sysix) - 05b4dcf linter: Add schema for `eslint/prefer-const` (#23081) (Sysix) - 5a06c4d linter/vue: Implement next-tick-style rule (#23041) (Alex Peshkov) - e38a36a linter: Add schema for `eslint/operator-assignment` (#23080) (Sysix) - 907cee7 linter: Add schema for `eslint/no-warning-comments` (#23075) (Sysix) - 9470bb2 linter: Add schema for `eslint/no-unused-vars` (#23073) (Sysix) - 234b5cf linter: Add schema for `eslint/no-shadow` (#23072) (Sysix) - de0dd8b linter: Add schema for `eslint/no-restricted-exports` (#23020) (Sysix) - faa3e0d linter: Add schema for `eslint/no-param-reassign` (#23018) (Sysix) - dbc9c27 linter: Add schema for `eslint/no-magic-numbers` (#23017) (Sysix) - 38d3569 linter: Add schema for `eslint/no-inner-declarations` (#23016) (Sysix) - 008fa41 linter: Add schema for `eslint/no-constant-condition` (#22991) (Sysix) - ca44623 linter: Add schema for `eslint/no-empty-function` (#22988) (Sysix) - 43eb04d linter: Add schema for `eslint/id-match` (#22987) (Sysix) - a800f27 linter: Add schema for `eslint/capitalized-comments` (#22984) (Sysix) - 96e2d32 linter: Add schema for `eslint/id-length` (#22963) (Sysix) - 545493f linter: Add schema for `eslint/complexity` (#22960) (Sysix) - 5f0b558 linter: Add schema for `eslint/class-methods-use-this` (#22959) (Sysix) - 719b720 linter: Add schema for simple rule configurations (#22948) (Sysix) - fd00966 linter: Add right schema for `eslint/max-*` rules (#22923) (Sysix) - 1226d78 linter: Fill schema with rule configurations (#22907) (Sysix) - 8f423c1 linter/vue: Implement `require-direct-export` rule (#17623) (yefan) - 78e915b linter/vue: Implement no-reserved-props rule (#22914) (bab) - 0f200a9 linter/vue: Implement require-prop-types rule (#22083) (Alex Peshkov) - 5da9da9 linter/vue: Implement no-reserved-keys rule (#21780) (bab) - 75e14a8 linter/vue: Implement prop-name-casing rule (#22892) (bab) - 85efabf semantic: Make building the class table optional, off by default (#22862) (Boshen) ### 🐛 Bug Fixes - a49b0cf linter/no-map-spread: Remove ineffective autofix (#22956) (camc314) - cf53285 parser: Report reserved type-declaration names in the parser (#23035) (Boshen) - 0383e61 linter: Fix schema for rules without a config (#22946) (Sysix) - 4d722e0 parser: Report duplicate switch `default` clause in the parser (#23012) (Boshen) - 6cb34b8 linter/plugins: Make spreading `Token` instances keep `loc` property (#22947) (Nicolas Le Cam) - 27de044 linter/plugins: Make spreading `Comment` instances keep `loc` property (#22238) (Nicolas Le Cam) - 742fd0b linter/double-comparisons: Make fixer a suggestion (#22968) (camc314) - 93f4494 linter: Respect default child config plugin when extending parent config (#22903) (Sysix) - 594ed86 linter: Deny unknown options for some rules (#22924) (Sysix) - 3253038 linter/expect-expect: Align default rule options (#22890) (camc314) - bbe44ea linter: Respect default plugins from extended config (#22896) (Sysix) ### ⚡ Performance - 0b7ce7e linter/plugins: Create global prop vars at top level of modules (#22928) (overlookmotel) - 0f7c319 linter/plugins: Define class `#loc` setter functions as `const`s (#22919) (overlookmotel) ### 📚 Documentation - 7b0380d linter: Remove preserve-caught-error note (#22994) (camc314) - dadafe3 oxlint, oxfmt: Mention migrate skills in npm READMEs (#22965) (Boshen) # Oxfmt ### 🚀 Features - 3da77e0 oxfmt: Format `parser:json5` files by `oxc_formatter_json` (#22990) (leaysgur) - c786f0d oxfmt: Format `parser:jsonc` files by `oxc_formatter_json` (#22913) (leaysgur) - 27a6db8 formatter_json: Implement jsonc variant (#22912) (leaysgur) ### 🐛 Bug Fixes - 2aedd52 oxfmt: Avoid JS promise rejects for all TSFN call sites (#23107) (leaysgur) - 01e0871 formatter,formatter_json: Handle PS/LS as line terminator (#22978) (leaysgur) - 23902d9 formatter_json: Handle CR only line breaks (#22977) (leaysgur) - 136b72b formatter_json: Use line_suffix for line comment outside array (#22931) (leaysgur) - 44e40fa formatter_json: Expand line comment inside array (#22911) (leaysgur) - 2c86896 formatter_json: Avoid example binary name collision (#22904) (camc314) ### 📚 Documentation - cc69d8d formatter_json: Update AGENTS.md (#22981) (leaysgur) - 0490721 formatter_json: Update AGENTS.md (#22976) (leaysgur) - dadafe3 oxlint, oxfmt: Mention migrate skills in npm READMEs (#22965) (Boshen) - f88961a oxfmt: Annotate each config option with supported languages (#22953) (leaysgur) - 7e514bf formatter_json: Update AGENTS.md (#22930) (leaysgur) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Summary: Fixing unicorn/expiring-todo-comments via jsPlugin
Problem
The
unicorn/expiring-todo-commentsrule failed when run through oxlint's jsPlugin system with error:TypeError: Either `node` or `loc` is requiredThe rule spreads Comment instances (
{...comment}) to process each line of multi-line comments, but lost thelocproperty because:locwas a prototype getter (not an own enumerable property)Solution
Define
locas an own accessor property on everyCommentinstance using__defineGetter__. Own properties are visible to spread,JSON.stringify,andfor…initeration, 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 aconstrather than aletlets V8 skip reassignment checksat the
defineGetter(this, "loc", getCommentLoc)call site — a pattern alreadyestablished in the codebase for
resetCommentLoc.As a result:
toJSON()is removed —JSON.stringifynow works via the own property directly.Object.defineProperty(Comment.prototype, "loc", { enumerable: true })is removed —no longer needed.
Test Results