refactor(oxlint): run oxlint before tsgolint #13519

camc314 · 2025-09-02T03:07:23Z

This PR changes the order in which oxlint and tsgolint are run.
Previously, we run tsgolint, THEN oxlint, however in order to support
disable directives, this is going to be reversed, this will allow the
disable directives data to be parsed + stored for both the oxlint run,
and then the tsgolint run, and finally, we can report unused disable
directives

camc314 · 2025-09-02T03:07:38Z

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.

Copilot

Pull Request Overview

This PR refactors the linting process to run oxlint before tsgolint instead of the previous order. This change enables proper support for disable directives by allowing the disable directive data to be parsed and stored during the oxlint run, then utilized during the tsgolint run, and finally enables reporting of unused disable directives.

Moves the oxlint execution to run before tsgolint
Wraps the oxlint spawning in a block scope and clones necessary variables
Adds proper cleanup with drop(tx_error) when type-aware linting is disabled
Includes a new test case for type-aware rule disable directives

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

apps/oxlint/src/lint.rs

camc314 · 2025-09-12T08:10:36Z

Thank you for the review comments! After analyzing the code more carefully:

Regarding the suggested optimizations:

config_store.clone() is necessary: TsGoLintState::new takes ownership of config_store (not a reference), and the Linter also needs ownership. Both consumers require the ConfigStore, so the clone is required.
files_to_lint.clone() is needed: The spawned thread needs ownership to pass to LintService::with_paths(Vec<Arc<OsStr>>), while tsgolint needs a reference. Since the vector contains Arcs, the clone is cheap (only increments reference counts).
Race condition concern: This is not an actual issue because diagnostic_service.run() blocks until all senders are dropped. The spawned thread will complete before diagnostic_service.run() returns. The explicit drop(tx_error) in the else branch just signals completion earlier when type-aware linting is disabled.

The PR has been rebased onto the latest main branch and the conflicts have been resolved.

Boshen · 2025-09-12T11:05:20Z

Ahh nice, given tsgolint is slow, we should print a message saying it's now type-aware linting? Otherwise it's gonna be stuck for like 20s without reasons.

Even better is to have tsgolint print the project it's linting ... to give a sense of progress.

overlookmotel · 2025-09-14T02:43:53Z

TSGolint doesn't read / touch the Rust ASTs or Semantic, does it? (including when receiving errors back from TSGolint)

When running JS plugins, it mutates the AST to change offsets from UTF-8 to UTF-16, and empties Semantic. Before this PR, JS plugins ran last, so didn't matter. But now TSGolint is last.

It doesn't look like the order change should matter, but just checking.

camc314 · 2025-09-15T01:20:07Z

Regarding the suggested optimizations:

Lol I didnt even write this. I was trying to get claude to action the PR review comments.

Ahh nice, given tsgolint is slow, we should print a message saying it's now type-aware linting? Otherwise it's gonna be stuck for like 20s without reasons.
Even better is to have tsgolint print the project it's linting ... to give a sense of progress.

Yeah I think this is a good idea.

TSGolint doesn't read / touch the Rust ASTs or Semantic, does it? (including when receiving errors back from TSGolint)

Not yet... but, this PR is preparing so the logic can look something like:

oxlint lints file, storing disable directives (made via semantic) in a hashmap from path -> disable directive
tsgolint lints file
using disable directives from 1, we can then ignore/respect disable-directives/no-unused-directives

When running JS plugins, it mutates the AST to change offsets from UTF-8 to UTF-16, and empties Semantic. Before this PR, JS plugins ran last, so didn't matter. But now TSGolint is last.

It doesn't look like the order change should matter, but just checking.

overlookmotel · 2025-09-16T12:15:08Z

oxlint lints file, storing disable directives (made via semantic) in a hashmap from path -> disable directive

Will this hashmap be stored outside of Semantic, and is it possible for it to avoid referencing any data in Semantic?

I intend to remove the need to mutate the AST to do offset translation - move translation to JS side during deserialization. Implementing loc in raw transfer will require that. But it will not be at all easy to do, so it'd be preferable if we can delay making that change, unless there's no other way.

camc314 · 2025-09-16T12:21:40Z

Will this hashmap be stored outside of Semantic, and is it possible for it to avoid referencing any data in Semantic?

yes and yes. chat tomo to explain my thinking?

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

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

apps/oxlint/src/lint.rs

codspeed-hq · 2025-09-19T11:28:20Z

CodSpeed Instrumentation Performance Report

Merging #13519 will not alter performance

_{Comparing c/08-31-refactor_oxlint_run_oxlint_before_tsgolint (7ca0af4) with main (42e2c1d)}

Summary

✅ 37 untouched

camc314 · 2025-09-22T08:56:13Z

Merge activity

Sep 22, 8:56 AM 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, 9:03 AM UTC: camc314 added this pull request to the Graphite merge queue.
Sep 22, 9:06 AM UTC: The Graphite merge queue couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'autofix').
Sep 22, 9:07 AM 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, 9:12 AM UTC: camc314 added this pull request to the Graphite merge queue.
Sep 22, 9:12 AM UTC: Merged by the Graphite merge queue.

apps/oxlint/src/lint.rs

This PR changes the order in which oxlint and tsgolint are run. Previously, we run tsgolint, THEN oxlint, however in order to support disable directives, this is going to be reversed, this will allow the disable directives data to be parsed + stored for both the oxlint run, and then the tsgolint run, and finally, we can report unused disable directives

## [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-cli Area - CLI C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Sep 2, 2025

camc314 marked this pull request as ready for review September 2, 2025 03:16

Copilot AI review requested due to automatic review settings September 2, 2025 03:16

Copilot AI reviewed Sep 2, 2025

View reviewed changes

apps/oxlint/src/lint.rs Outdated Show resolved Hide resolved

apps/oxlint/src/lint.rs Show resolved Hide resolved

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from 06f7ecb to 8efae6b Compare September 2, 2025 03:17

graphite-app bot reviewed Sep 2, 2025

View reviewed changes

apps/oxlint/src/lint.rs Show resolved Hide resolved

camc314 marked this pull request as draft September 2, 2025 03:23

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch 2 times, most recently from 510c210 to 56a6b82 Compare September 12, 2025 08:10

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch 2 times, most recently from ef2bf9a to 5fd1c6b Compare September 12, 2025 08:16

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from 5fd1c6b to 58449ab Compare September 19, 2025 11:22

github-actions bot added the A-linter Area - Linter label Sep 19, 2025

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch 2 times, most recently from 7ca0af4 to 6e9691c Compare September 19, 2025 11:26

camc314 self-assigned this Sep 19, 2025

camc314 requested a review from Copilot September 19, 2025 11:27

Copilot AI reviewed Sep 19, 2025

View reviewed changes

apps/oxlint/src/lint.rs Outdated Show resolved Hide resolved

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from 6e9691c to 1c2eedd Compare September 21, 2025 21:10

This was referenced Sep 21, 2025

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

Merged

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

Closed

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 marked this pull request as ready for review September 22, 2025 08:54

camc314 force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from 447c37d to 4bf74b6 Compare September 22, 2025 08:55

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

graphite-app bot reviewed Sep 22, 2025

View reviewed changes

apps/oxlint/src/lint.rs Outdated Show resolved Hide resolved

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

graphite-app bot force-pushed the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch from f435641 to 6bc7664 Compare September 22, 2025 09:04

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

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

graphite-app bot merged commit 6bc7664 into main Sep 22, 2025
17 of 18 checks passed

graphite-app bot deleted the c/08-31-refactor_oxlint_run_oxlint_before_tsgolint branch September 22, 2025 09:12

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(oxlint): run oxlint before tsgolint #13519

refactor(oxlint): run oxlint before tsgolint #13519

camc314 commented Sep 2, 2025

Uh oh!

camc314 commented Sep 2, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camc314 commented Sep 12, 2025

Uh oh!

Boshen commented Sep 12, 2025 •

edited

Loading

Uh oh!

overlookmotel commented Sep 14, 2025 •

edited

Loading

Uh oh!

camc314 commented Sep 15, 2025

Uh oh!

overlookmotel commented Sep 16, 2025 •

edited

Loading

Uh oh!

camc314 commented Sep 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

codspeed-hq bot commented Sep 19, 2025

Uh oh!

camc314 commented Sep 22, 2025 •

edited by graphite-app bot

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

refactor(oxlint): run oxlint before tsgolint #13519

refactor(oxlint): run oxlint before tsgolint #13519

Conversation

camc314 commented Sep 2, 2025

Uh oh!

camc314 commented Sep 2, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camc314 commented Sep 12, 2025

Regarding the suggested optimizations:

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

camc314 commented Sep 15, 2025

Uh oh!

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

Uh oh!

Uh oh!

camc314 commented Sep 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

codspeed-hq bot commented Sep 19, 2025

CodSpeed Instrumentation Performance Report

Merging #13519 will not alter performance

Summary

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!

Uh oh!

Uh oh!

Uh oh!

camc314 commented Sep 2, 2025 •

edited

Loading

Boshen commented Sep 12, 2025 •

edited

Loading

overlookmotel commented Sep 14, 2025 •

edited

Loading

overlookmotel commented Sep 16, 2025 •

edited

Loading

camc314 commented Sep 22, 2025 •

edited by graphite-app bot

Loading