-
-
Notifications
You must be signed in to change notification settings - Fork 715
fix(language_server): don't resend diagnostic on save, when typeAware is disabled and run is onType
#13604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(language_server): don't resend diagnostic on save, when typeAware is disabled and run is onType
#13604
Conversation
When lint.run="onType" and type_aware=false, saving would clear all diagnostics. Now returns None instead of empty Vec to preserve existing diagnostics.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fixes an edge case where diagnostics would be cleared when saving a file with oxc.lint.run: "onType" and oxc.typeAware: false. The issue occurred because the logic assumed TypeScript linting was always available in onType mode, but when type awareness is disabled, no linter exists to provide diagnostics.
- Updates logic to check if TypeScript linter exists before attempting to run it on save
- Adds test coverage for the edge case scenario
- Preserves existing diagnostics when no linters should run
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server_linter.rs | Updates the linter run logic to conditionally run TypeScript linting based on availability |
| on-save-no-type-aware.ts | Adds test fixture for the edge case scenario |
| on-save-no-type-aware.ts.snap | Snapshot test output for the new test case |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Thank you for looking after the bug.
My guess is, that you are still on the latest release, and not using the The lines: oxc/crates/oxc_language_server/src/linter/server_linter.rs Lines 356 to 361 in 78a1146
It will only insert it to If you want to try it yourself, you can follow the instructions here: |
Ah, I see it was already fixed. Thanks! I'll check more carefully next time. Thanks for the review. I'll go ahead and close this PR. |
|
Hey @Sysix, could you review this once more? I tested and my fix also seems valid. |
Sysix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is optimization for onSave configuration.
If typeAware is not enabled, your MR will not send the latest (redundant) diagnostics to the client.
typeAware is disabled and run is onType
CodSpeed Instrumentation Performance ReportMerging #13604 will not alter performanceComparing Summary
Footnotes |
Merge activity
You must have a Graphite account in order to use the merge queue. Create an account and try again using this link |
## [1.15.0] - 2025-09-11 ### 💥 BREAKING CHANGES - edc70ea allocator/pool: [**BREAKING**] Remove `disable_fixed_size` Cargo feature (#13625) (overlookmotel) ### 🚀 Features - b20b56d linter: Add `vue/no-multiple-slot-args` rule (#13579) (Sysix) - aafe08c linter: Add `vue/define-emits-declaration` rule (#13567) (Sysix) - 2ed5059 linter: Add `vue/define-props-declaration` rule (#13566) (Sysix) - a718c23 linter: Add `vue/valid-define-props` rule (#13565) (Sysix) - 75a673e editor: Support relative path for `oxc.path.server` (#13542) (Sysix) - 4af886b linter: Add `unicorn/no-array-reverse` rule (#13530) (yefan) - 2db32eb data_structures: Add `boxed_slice!` and `boxed_array!` macros (#13596) (overlookmotel) ### 🐛 Bug Fixes - fb9d0f4 language_server: Don't resend diagnostic on save, when `typeAware` is disabled and run is onType (#13604) (YongSeok Jang (장용석)) - 2f36350 editor: Add notice for a possible restart when fixing `filename-case` (#13557) (Sysix) - e17fccc linter: Update `RuleRunner` impl after merge (#13642) (camc314) - 3d27c5b linter/no-unused-private-class-members: False positive with spread expr (#13634) (yefan) - 8314ed5 linter/tsgolint: Correct comment (#13589) (camc314) - 198243b semantic: Dont parse `@` as jsdoc tags inside quotes (#13571) (Gwenn Le Bihan) - 89084d7 linter/custom-plugins: Enforce exact matching for disable directives (#13538) (Copilot) - 277c5e1 linter: Output `eslint-plugin-vue` for vue diagnostics (#13564) (Sysix) - 34d3cde rust: Fix clippy issues (#13540) (Boshen) - 5fccafc linter: `unicorn/prefer-array-flat-map` ignore `React.Children` (#13534) (Sysix) - 7e78e39 linter: Don't panic when parsing regex with multiple parentheses (#13524) (Sysix) - 0d867b1 linter: Skip running tsgolint when no files need type aware linting (#13502) (Copilot) - b677376 language_server: Include the diagnostic of the other linter (#13490) (Sysix) - e87d7bd linter: Parse regex inside `new RegExp()` with parentheses (#13448) (Sysix) - 5990f17 linter: Change `typescript/no-confusing-void-expression` to pedantic (#13473) (Boshen) ### 🚜 Refactor - 7775c21 linter/plugins: Remove `oxlint2` Cargo feature (#13648) (overlookmotel) - 8f37e88 linter: Update tsgolint payload (#13547) (camchenry) - 2d53203 linter/plugins: Move `tokio` usage from `oxc_linter` to `napi/oxlint2` (#13647) (overlookmotel) - 6cd6be2 linter: Add `--experimental-js-plugins` CLI arg (#13658) (overlookmotel) - 476729b linter: Simplify `RuleRunner` trait definition (#13637) (camchenry) - 2f02ac6 linter/plugins: Remove `disable_oxlint2` Cargo feature (#13626) (overlookmotel) - ff9e4fb linter/plugins: Use fixed-size allocators when `ExternalLinter` exists (#13623) (overlookmotel) - f9bff64 linter_codegen: Improve code style for collecting nodes (#13636) (camchenry) - babbaca all: Remove `pub` from modules with no exports (#13618) (overlookmotel) - 91759c6 linter/plugins: Only use `RawTransferFileSystem` if JS plugins registered (#13599) (overlookmotel) - 118020c linter/plugins: Discard `ExternalLinter` if no JS plugins registered (#13598) (overlookmotel) - 8d30bce linter/tsgolint: Report an error if the tsgolint exe could not be found (#13590) (camc314) - bccc276 eslint/for-direction: Clean up implementation and improve documentation (#13532) (Antoine Zanardi) - 1425da2 eslint/default-case-last: Simplify default case last check in switch statement (#13529) (Antoine Zanardi) - d245376 oxlint: Remove unused `runner` module (#13561) (camc314) - 53f2fc1 eslint/default-case: Simplify implementation and enhance readability (#13430) (Antoine Zanardi) - 6f15060 eslint/block-scoped-var: Clean up implementation and improve documentation (#13417) (Antoine Zanardi) - 671e0fd language_server: Only store one instance of a diagnostic (#13514) (Sysix) - 1b425d6 eslint/default-case-last: Simplify implementation and enhance readability (#13515) (Antoine Zanardi) - e4bbbce eslint/default-param-last: Simplify implementation and enhance readability (#13516) (Antoine Zanardi) - e0396fd linter: Remove `static` lifetime from disable directives function argument (#13492) (camc314) ### 📚 Documentation - eb1f167 linter: Note which rules require type info to run on rule page (#13675) (camc314) - e66f93b linter: Fix backtick formatting in no-return-wrap (#13633) (camc314) ### ⚡ Performance - e6a25e7 linter: Remove unnecessary `should_run` check (#13639) (camchenry) - f6a9687 linter: Store rules by AST type in a boxed array (#13578) (overlookmotel) - b81f081 linter: Reduce indirection (#13574) (overlookmotel) - a744aff linter: Skip rules that do not have any relevant node types (#13138) (camchenry) ### 🎨 Styling - e110476 linter: Reformat code (#13573) (overlookmotel) ### 🧪 Testing - 58e6c94 oxlint: Add test for ignorePatterns whitelist (#13372) (Sysix) Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
Fixes an edge case introduced in #13332 where diagnostics disappear when saving a file with
oxc.lint.run: "onType"andoxc.typeAware: false.Problem
The logic from #13332 assumes
tsgolintalways exists whenRun::OnType:But when
type_aware: false, there is notsgo_linter, causing the server to return empty diagnostics instead of None, which clears all error highlights in VSCode.Solution
Check if
tsgo_linteractually exists:Now returns None when no linters should run, preserving existing diagnostics.
Test: Added
test_lint_on_run_on_type_on_save_without_type_awarewith fixture