refactor(linter): make disable directives own the rule name #13987

camc314 · 2025-09-21T21:10:20Z

doing this obviously has a perf and memory implication. The alternative is that we have to keep the source text in memory until tsgolint has finished linting (and all source files must be in memory the whole time), which is a worse trade off. Now that disable directives struct owns the rule names we can drop the source text (reducing memory usage) while tsgolint runs

camc314 · 2025-09-21T21:10:37Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

codspeed-hq · 2025-09-21T21:17:39Z

CodSpeed Instrumentation Performance Report

Merging #13987 will not alter performance

_{Comparing c/09-21-refactor_linter_make_disable_directives_own_the_rule_name (2e9ae3f) with main (c778cba)¹}

Summary

✅ 37 untouched

No successful run was found on main (2e9ae3f) during the generation of this report, so c778cba was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

camc314 · 2025-09-22T14:17:26Z

Merge activity

Sep 22, 2:17 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Sep 22, 2:17 PM UTC: camc314 added this pull request to the Graphite merge queue.
Sep 22, 2:21 PM UTC: Merged by the Graphite merge queue.

Copilot

Pull Request Overview

Refactors disable directives to own rule names as String instead of borrowing from source text, enabling memory optimization by allowing source text to be dropped during linting.

Converts lifetime-parameterized structs to owned types by replacing &str with String for rule names
Updates all references and method signatures to work with owned data
Modifies test assertions to handle owned strings properly

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
crates/oxc_linter/src/disable_directives.rs	Core refactoring from borrowed to owned rule names, removing lifetime parameters
crates/oxc_linter/src/context/mod.rs	Updates method signature to return non-lifetime-parameterized DisableDirectives
crates/oxc_linter/src/context/host.rs	Updates method signatures and string handling for owned rule names
crates/oxc_linter/src/rules/unicorn/no_abusive_eslint_disable.rs	Adjusts function call to pass reference to owned string

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-22T14:17:38Z

crates/oxc_linter/src/disable_directives.rs


-#[derive(Debug)]
-pub struct DisableDirectives<'a> {
+#[derive(Debug, Clone)]


Adding Clone to DisableDirectives may have performance implications since it contains collections and boxed data. Consider if this trait bound is necessary or if references could be used instead.

Suggested change

#[derive(Debug, Clone)]

#[derive(Debug)]

Copilot · 2025-09-22T14:17:39Z

crates/oxc_linter/src/disable_directives.rs

+            .map(|interval| interval.val.clone())
+            .collect::<Vec<DisabledRule>>();


Cloning DisabledRule instances in this hot path could impact performance. Consider using references or restructuring to avoid cloning during rule matching.

Copilot · 2025-09-22T14:17:39Z

crates/oxc_linter/src/disable_directives.rs

        for disable in &matched_intervals {
-            self.mark_disable_directive_used(*disable);
+            self.mark_disable_directive_used(disable.clone());


Another clone operation that could be avoided. Since disable is already a clone from line 170, consider passing a reference or restructuring to eliminate this additional clone.

doing this obviously has a perf and memory implication. The alternative is that we have to keep the source text in memory until tsgolint has finished linting (and all source files must be in memory the whole time), which is a worse trade off. Now that disable directives struct owns the rule names we can drop the source text (reducing memory usage) while tsgolint runs

## [1.17.0] - 2025-09-23 ### 🚀 Features - f47f978 linter/plugins: Add `definePlugin` API (#14008) (overlookmotel) - 3e117c6 linter/plugins: Add `defineRule` API (#13945) (overlookmotel) - 2dc8adb linter/plugins: Add `createOnce` API (#13944) (overlookmotel) - bef8753 linter/plugins: ESTree-compatible AST for JS plugins (#13942) (overlookmotel) - 55c2e11 editor: Add `oxc.fmt.experimental` flag (#13923) (Sysix) - a21ff54 language_server: Introduce `ServerFormatter` (#13700) (Sysix) - a14aa79 npm/oxlint: Convert to ES modules (#13876) (Boshen) - 15eb326 linter: Add `vue/no-required-prop-with-default` rule (#13613) (yefan) - 5d3ba00 linter: Add `vue/require-typed-ref` rule (#13857) (yefan) - b52389a node: Bump `engines` field to require Node.js 20.19.0+ for ESM support (#13879) (Copilot) - 8221a01 linter: Add `eslint/no-misleading-character-class` (#13365) (Sysix) - c75ae8c linter/plugins: Add options to `Context` (#13810) (overlookmotel) - 53d04dd linter: Convert `oxlint` to NAPI app (#13723) (overlookmotel) ### 🐛 Bug Fixes - 1f4be38 napi/parser: Generate `.d.mts` extension for types (#14038) (Daniel Roe) - a018756 linter/plugins: Pin `tsdown` dependency to 0.15.1 (#14005) (overlookmotel) - b8790c2 language_server: Output correct position for parser & semantic errors (#14010) (Sysix) - 4d3c6e1 linter: Support let-chains in codegen node type detection (#14006) (camc314) - a34918a linter/plugins: Avoid lint warnings compiling WASM or big-endian (#13968) (overlookmotel) - 27022ab linter/switch-case-braces: Add support for string including colon on case expression (#13687) (Lukinoh) - dd3843f linter: Set package version in `package.json` (#13890) (overlookmotel) - fa866b3 linter: Sort optimized/unoptimized diagnostics before comparison (#13850) (camc314) - fac7624 linter/plugins: Improve error for no JS plugins (#13858) (overlookmotel) ### 🚜 Refactor - bb040bc parser, linter: Replace `.mjs` files with `.js` (#14045) (overlookmotel) - b353750 linter/plugins: Remove unnecessary `Serialize` derives (#14031) (overlookmotel) - 0d48511 linter/plugins: Improve handling `Context` method calls in `createOnce` (#14032) (overlookmotel) - 36ac0fb language_server: Don't create `mpsc` channel (#14011) (Sysix) - 2e9ae3f linter: Make disable directives own the rule name (#13987) (camc314) - 6bc7664 oxlint: Run oxlint before tsgolint (#13519) (camc314) - 0c93f33 language_server: Use minimal text edit for `ServerFormatter` (#13960) (Sysix) - 823cb57 langauge_server: Move `Backend` into own file (#13955) (Sysix) - dc700f5 language_server: Introduce `LSPFileSystem` (#13731) (Sysix) - 42e2c1d linter: Remove `Rc` from `DisableDirectives` (#13924) (camc314) - ac3e9e9 napi/parser: Move JS code into `src-js` directory (#13899) (overlookmotel) - e90bfe3 linter: Remove double reference in unicorn/error-message (#13916) (camc314) - 83791a0 linter: Remove double reference in vue/jsx-no-undef (#13915) (camc314) - ebd22de linter: Remove double reference in vue/require-typed-ref (#13914) (camc314) - 873e231 language_server: Move `needs_linter_restart` to `ServerLinter` (#13834) (Sysix) - 7e0d736 linter/plugins: Rename `--experimental-js-plugins` to `--js-plugins` (#13860) (overlookmotel) - 6245c8c linter/plugins: Make `Context` properties getters (#13809) (overlookmotel) - a2342a6 linter/plugins: Import named in tests (#13807) (overlookmotel) ### 📚 Documentation - 601c876 linter: Add comment explaining why Mimalloc is feature-gated (#14037) (overlookmotel) ### ⚡ Performance - 4d04c6e linter/plugins: Flatten `LintFileResult` fields (#14033) (overlookmotel) - 06363e0 linter: Add node type codegen support for `match node.kind()` (#13736) (camchenry) - a79af0a linter: Replace `for (... of ...)` loops (#13913) (overlookmotel) - b53292d linter: Use unstable sort where possible (#13818) (sapphi-red) ### 🎨 Styling - 8083740 linter: Import `Serialize` at top level (#14030) (overlookmotel) ### 🧪 Testing - b807b6f language_server: Skip formatter test on windows (#13986) (Sysix) - 2600858 language_server: Add Tester for ServerFormatter (#13969) (Sysix) - 37aadf0 language_server: Add `test_and_snapshot_multiple_file` (#13966) (Sysix) - ad14a41 editor: Fix test setup (#13929) (Sysix) - f51240e linter/plugins: Tests for different import styles (#13859) (overlookmotel) - 407c95e linter/plugins: Check `this` is undefined in visit functions (#13811) (overlookmotel) - f023a22 linter/plugins: Include stack trace in plugin loading errors (#13808) (overlookmotel) ### 💼 Other - 0630d68 linter: Build `oxlint` locally with Mimalloc in release mode (#14034) (overlookmotel) Co-authored-by: camc314 <[email protected]>

github-actions bot added A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Sep 21, 2025

camc314 mentioned this pull request Sep 21, 2025

refactor(oxlint): run oxlint before tsgolint #13519

Merged

camc314 mentioned this pull request Sep 21, 2025

feat(linter): make disable directives work for type aware rules #13988

Closed

camc314 force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from d60917f to 41a3918 Compare September 22, 2025 08:49

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from 1c2eedd to 447c37d Compare September 22, 2025 08:49

camc314 force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from 41a3918 to 6e70f5c Compare September 22, 2025 08:55

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch 2 times, most recently from 4bf74b6 to f435641 Compare September 22, 2025 08:58

camc314 force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from 6e70f5c to 67b287d Compare September 22, 2025 09:00

graphite-app bot changed the base branch from c/08-31-refactor_oxlint_run_oxlint_before_tsgolint to graphite-base/13987 September 22, 2025 09:04

graphite-app bot force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from 67b287d to 83fb5cb Compare September 22, 2025 09:13

graphite-app bot force-pushed the graphite-base/13987 branch from f435641 to 6bc7664 Compare September 22, 2025 09:13

graphite-app bot changed the base branch from graphite-base/13987 to main September 22, 2025 09:14

graphite-app bot force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from 83fb5cb to 30ae587 Compare September 22, 2025 09:14

camc314 force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from 30ae587 to c73c828 Compare September 22, 2025 14:10

camc314 marked this pull request as ready for review September 22, 2025 14:16

Copilot AI review requested due to automatic review settings September 22, 2025 14:16

camc314 added the 0-merge Merge with Graphite Merge Queue label Sep 22, 2025

Copilot AI reviewed Sep 22, 2025

View reviewed changes

graphite-app bot force-pushed the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch from c73c828 to 2e9ae3f Compare September 22, 2025 14:17

graphite-app bot merged commit 2e9ae3f into main Sep 22, 2025
25 checks passed

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 22, 2025

graphite-app bot deleted the c/09-21-refactor_linter_make_disable_directives_own_the_rule_name branch September 22, 2025 14:21

Boshen mentioned this pull request Sep 23, 2025

release(oxlint): v1.17.0 #14054

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(linter): make disable directives own the rule name #13987

refactor(linter): make disable directives own the rule name #13987

Uh oh!

camc314 commented Sep 21, 2025 •

edited

Loading

Uh oh!

camc314 commented Sep 21, 2025 •

edited

Loading

Uh oh!

codspeed-hq bot commented Sep 21, 2025 •

edited

Loading

Uh oh!

camc314 commented Sep 22, 2025 •

edited by graphite-app bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 22, 2025

Uh oh!

Copilot AI Sep 22, 2025

Uh oh!

Copilot AI Sep 22, 2025

Uh oh!

Uh oh!

Uh oh!

		.map(\|interval\| interval.val.clone())
		.collect::<Vec<DisabledRule>>();

Uh oh!

refactor(linter): make disable directives own the rule name #13987

refactor(linter): make disable directives own the rule name #13987

Uh oh!

Conversation

camc314 commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camc314 commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Instrumentation Performance Report

Merging #13987 will not alter performance

Summary

Footnotes

Uh oh!

camc314 commented Sep 22, 2025 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camc314 commented Sep 21, 2025 •

edited

Loading

camc314 commented Sep 21, 2025 •

edited

Loading

codspeed-hq bot commented Sep 21, 2025 •

edited

Loading

camc314 commented Sep 22, 2025 •

edited by graphite-app bot

Loading