fix(linter/plugins): make spreading Comment instances keep loc property

[KuSh]

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

Two-part fix in apps/oxlint/src-js/plugins/comments.ts:

1. WeakMap for location storage (replaced private #loc field)
   • Avoids hidden class transitions (maintains performance)
   • Allows Proxy to work without private field access issues
2. Proxy in constructor (makes loc enumerable for spread)
   • ownKeys: includes "loc" in enumerable properties
   • getOwnPropertyDescriptor: returns getter descriptor for "loc"

Why This Approach

• Private field failed: Can't access #loc through Proxy from external debug/reset code
• Prototype enumerable wasn't enough: Spread only copies own properties, not inherited
• WeakMap + Proxy is clean: Each handles what it does best (storage vs enumeration)

Bonus: Removed toJSON Hack

Before: A toJSON() method was needed to include loc in JSON.stringify because:

• loc was a prototype getter (not an own property)
• Spread and JSON.stringify only copy own properties
• toJSON() manually added loc to serialized output
  After: The Proxy makes loc naturally enumerable, so toJSON() is unnecessary and was removed.

Files Changed

• apps/oxlint/src-js/plugins/comments.ts - Main fix
• apps/oxlint/package.json - Added eslint-plugin-unicorn dev dependency
• New test fixture: apps/oxlint/test/fixtures/unicorn_expiring_todo_comments/

Test Results

• ✅ unicorn/expiring-todo-comments rule works (detects expired TODOs)
• ✅ All 625 existing tests pass

[overlookmotel]

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 Map / WeakMap and proxies as much as possible, because of the relatively poor performance characteristics.

[KuSh]

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 Map / WeakMap and proxies as much as possible, because of the relatively poor performance characteristics.

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.

[KuSh]

@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 Results

Benchmarked 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)

Version	Mean ± σ	vs #loc
#loc (before)	24.989s ± 1.261s	—
Proxy (after)	25.385s ± 1.232s	+0.396s (+1.6%)

eslint-comments (7 real rules — no-use, disable-enable-pair, etc.)

Version	Mean ± σ	vs #loc
#loc (before)	15.204s ± 0.658s	—
Proxy (after)	15.652s ± 0.583s	+0.448s (+2.9%)

Verdict

No measurable regression. Both differences are within noise (standard deviations overlap). The Proxy overhead on .loc access (which has no get trap — delegates to target) is negligible in practice.

The fix is safe to land from a performance standpoint.

Configuration used:

Node v24.14.0
oxc @ 5774052

stress-loc

import 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-comments

Installed as devDependencies at ^4.7.1, which resolved to package version 4.7.1 (latest).
Configured with:

{
  "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).
  }
}

[overlookmotel]

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 const loc = comment.loc; line is hoisted out of the inner for loop by V8 - or indeed that it optimizes out most of the code as no-op.

count here is a meaningless number, but as it's acted on later, none of the code can be removed as no-op.

Also, if I've understood Proxy correctly, it has a perf impact on accessing other fields too (e.g. comment.value) so ideally we want to repeat those accesses 10x too.

[KuSh]

Microbenchmark Results

source: https://gist.github.com/KuSh/7af1829312429dd6e638a47de430a86a

No reset (steady state after warmup)

Variant	Direct	vs C	Spread	vs C
C:proto	0.262	1.0×	2.155	1.0×
B:modvar	0.975	3.7×	2.689	1.25×
E:static#	0.949	3.6×	2.705	1.26×
SR:selfRp	0.993	3.8×	2.682	1.24×
D:delloc	1.200	4.6×	2.945	1.37×
A:proxy	3.057	11.7×	107.145	49.7×

With reset (per file cycle)

Variant	Direct	vs C	Spread	vs C
C:proto	0.963	1.0×	2.325	1.0×
B:modvar	0.878	0.9×	2.892	1.24×
E:static#	0.919	1.0×	2.915	1.25×
D:delloc	1.039	1.1×	2.933	1.26×
SR:selfRp	2.216	2.3×	29.627	12.7×
A:proxy	3.353	3.5×	108.550	46.7×

Conclusions

Without reset, SR ≈ B/E — all three within noise. After warmup, SR's self-replaced data properties are as fast as B/E's getter + #loc path. All ~3.7× vs C.

With reset, SR collapses — Object.defineProperty on every reset + every first access makes it 2.3× on direct access and 12.7× on spread vs C. B/E stay competitive at 0.9-1.0× on direct access (computeLoc dominates, making all variants similar) and 1.24-1.25× on spread.

B/E are the only viable approach — correct spread behavior, cheap reset (#loc = null), competitive performance.

[KuSh]

Complete Benchmark Results

Benchmark	Proxy A	Variant B	Speedup
5000-comment stress file (single file, 1 rule)	229.7 ± 11.5 ms	197.7 ± 8.7 ms	1.16× (16%)
VS Code src/ (10 rules × 10 iterations, whole tree)	21.073 ± 0.149 s	20.159 ± 0.204 s	1.045× (4.5%)

The stress file isolates just the loc access overhead (no file I/O or parsing noise) — Proxy is 16% slower there. On the full VS Code tree, the Proxy overhead is diluted but still measurable at ~0.9s saved per run (4.5%) — real-world improvement with no downsides.

[overlookmotel]

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

[overlookmotel]

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.

[KuSh]

No problem at all! Thanks for that

[overlookmotel]

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:

Why the original benchmark was misleading: inline-cache pollution

All variants were measured through a single shared access function (testDirectAccess) in one
process. But each variant is a different V8 hidden class (map), and the property loads in that
function (comment.loc, .type, .range, …) are backed by inline caches keyed on the map. Feeding
one function four different maps drives those caches monomorphic → polymorphic → megamorphic — and
the Proxy variant poisons them completely. Worse, the "no-reset" pass ran all variants through
the function before the "with-reset" pass measured anything, so the later numbers were dominated by
megamorphic dispatch rather than the loc strategy under test.

That produced a first-mover advantage: whichever variant ran first got a clean monomorphic cache
during its own measurement (until the next variant's warmup deoptimized the function), so it looked
fastest. Reordering the variants changed the ranking — the tell-tale sign the harness, not the code,
was being measured.

Fix: run each variant in its own child process. Each process only ever sees one Comment class,
so the access function stays monomorphic — which also matches production, where there's only ever one
Comment class. Results are then order-independent.

Adapted version which fixes this: https://gist.github.com/overlookmotel/9687d97e8decbb08787e03cc2e888bdb

Results

variant	construct min	construct median	reset min	reset median	no-reset min	no-reset median	with-reset min	with-reset median
C:proto	0.0209	0.0218	0.0035	0.0142	0.2140	0.2177	0.2324	0.2371
B:modvar	0.2715	0.2963	0.0034	0.0144	0.2136	0.2181	0.2313	0.2378
D:defGet	0.1970	0.2143	0.0034	0.0143	0.2138	0.2190	0.2314	0.2355
SR:selfRp	0.2625	0.2911	0.3195	0.3242	0.2678	0.2738	1.6973	1.7249
A:proxy	0.0544	0.0549	0.0001	0.0001	2.6068	2.6385	2.7924	2.9478

construct is ms to build a pool of 5000; reset / no-reset / with-reset are ms/file.

Run on Mac Mini M4 Pro.

Upshot

• Fastest version which has the desired { ...comment } behavior is much slower to construct new Comment instances than what we have now.
• But the field access and reset times are unchanged.
• In fact, once the code is tiered up to TurboFan optimizing compiler (which it will be if comments are touched enough to make it matter), the generated assembly for loc field access is identical to current version.
• Proxy + Map is faster on construction and reset, but loses badly on field access, which is the hotter path (I switched to Map as WeakMap doesn't have a clear method, and all the keys are long-lived pooled objects anyway so the "weakness" doesn't buy anything).

"B:modvar" is 13.5x slower to construct a Comment. I managed to trim that down a bit in variant "D:defGet" by using Object.prototype.__defineGetter__ instead of Object.defineProperty, but it's still 10x slower than what we have at present.

However, that doesn't matter too much! As Comment instances are pooled, and re-used over and over, Comments only get created while linting the first few files, and thereafter we only hit the fast path.

So, in short, I think we can do this!

Next steps

Code

I 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 consts is intentional. It allows V8 to optimize better - the fact that LOC_GETTER_D is a const allows it to skip checks in defineGetter(this, "loc", LOC_GETTER_D) because it knows it can't be reassigned.

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.

Tests

Could you please reduce the test case to not require eslint-plugin-unicorn as a dependency? It can just reproduce the pattern that plugin requires, without the plugin itself.

Or testing the spread behavior could probably be folded into one of the existing fixtures that tests comments, rather than introducing a new fixture.

Tokens

We should probably apply the same change to Token. That has the same object pool pattern, and the same problem with spread.

@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 Locations in a Map?

On assumption that loc field is rarely accessed, maybe it'd be a good idea to store loc objects in a Map<Comment, Location> (borrowing from your original approach). Advantages:

1. Remove #loc field -> each Comment is 8 bytes smaller.
2. Much faster reset. Just map.clear().

However, this makes accessing loc more expensive. Whether it's overall a positive/negative depends on how many comments have their loc accessed. I don't know how we know what's the common behavior. Maybe my assumption that it's relatively rare is completely wrong!

Anyway, that'd be best left to a separate PR.

---

Just to say again, thanks very much for bringing this up and chasing down the perf impact. Really appreciate your dogged determination!