Skip to content

refactor(lsp): tools can report diagnostics for other uris too#16783

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too
Dec 13, 2025
Merged

refactor(lsp): tools can report diagnostics for other uris too#16783
graphite-app[bot] merged 1 commit intomainfrom
12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too

Conversation

@Sysix
Copy link
Member

@Sysix Sysix commented Dec 12, 2025

This PR refactors the LSP tool diagnostic API to allow tools to report diagnostics for multiple URIs, not just the file being analyzed. This enables cross-file analysis features like ts config deprecation detection.

@github-actions github-actions bot added A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 12, 2025
Copy link
Member Author

Sysix commented Dec 12, 2025


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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the LSP tool diagnostic API to allow tools to report diagnostics for multiple URIs, not just the file being analyzed. This enables cross-file analysis features like import cycle detection where a tool analyzing one file may need to report issues in related files.

Key changes:

  • Introduces DiagnosticResult type alias as Vec<(Uri, Vec<Diagnostic>)> to return diagnostics for multiple URIs
  • Updates all diagnostic trait methods to return DiagnosticResult instead of Option<Vec<Diagnostic>>
  • Adds collect_diagnostics_with aggregation helper in WorkspaceWorker to merge diagnostics from multiple tools
  • Updates backend to use publish_all_diagnostics helper for publishing multi-URI diagnostic results

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
crates/oxc_language_server/src/tool.rs Defines new DiagnosticResult type and updates trait method signatures to return multi-URI diagnostics
crates/oxc_language_server/src/worker.rs Adds collect_diagnostics_with aggregator and refactors diagnostic methods to handle multi-URI results
crates/oxc_language_server/src/tests.rs Updates fake tool implementation to return diagnostics using new tuple structure
crates/oxc_language_server/src/linter/tester.rs Adds snapshot helpers for multi-URI diagnostic results
crates/oxc_language_server/src/linter/server_linter.rs Implements new trait interface by wrapping single-file diagnostics in Vec with URI
crates/oxc_language_server/src/backend.rs Updates diagnostic publishing to use publish_all_diagnostics helper for multi-URI results
crates/oxc_language_server/src/linter/snapshots/*.snap Updates snapshot test outputs to include "File URI" prefix for each file's diagnostics

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Sysix Sysix force-pushed the 12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too branch from 3c1dfc8 to ff29f34 Compare December 12, 2025 18:46
@Sysix Sysix marked this pull request as ready for review December 12, 2025 18:47
@Sysix Sysix requested a review from camc314 as a code owner December 12, 2025 18:47
Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new multi-URI diagnostic plumbing is solid, but a few behavioral edges need tightening: aggregation order is nondeterministic due to HashMap iteration, snapshot URI scrubbing can now panic!() for non-file:///non-repo URIs, and the tester output conflates “ignored” with “no diagnostics.” Additionally, the backend appears to have dropped LSP diagnostic version publication for did_open/did_change, which can cause stale diagnostics if publish_all_diagnostics doesn’t preserve versions.

Additional notes (1)
  • Compatibility | crates/oxc_language_server/src/backend.rs:510-510
    In did_open / did_change, the old flow published diagnostics with a version (Some(params.text_document.version)), but the new flow calls publish_all_diagnostics(diagnostics) without passing the version.

If publish_all_diagnostics doesn’t incorporate the document version, clients may receive diagnostics that apply to a different buffer version (especially with on-type linting), which can lead to stale/incorrect squiggles.

This is a behavior change; it should be intentional and verified.

Summary of changes

Summary of changes

Multi-URI diagnostics support

  • Introduced DiagnosticResult = Vec<(Uri, Vec<Diagnostic>)> in crates/oxc_language_server/src/tool.rs.
  • Updated Tool::{run_diagnostic, run_diagnostic_on_save, run_diagnostic_on_change} to return DiagnosticResult (defaulting to an empty vector) instead of Option<Vec<Diagnostic>>.

Worker aggregation changes

  • Refactored WorkspaceWorker to aggregate diagnostics across tools and URIs via collect_diagnostics_with, merging results into a per-Uri collection (HashMap<Uri, Vec<Diagnostic>>).
  • Updated all worker diagnostic entry points (run_diagnostic*) to return multi-URI results.

Backend publishing flow

  • Updated backend call sites (did_open, did_change, did_save, and initial workspace scan) to consume multi-URI results and publish via publish_all_diagnostics.

Linter and tests/snapshots

  • Updated ServerLinter to wrap single-file lint results into vec![(uri.clone(), diagnostics)].
  • Updated snapshot formatting to include Linted file: and per-entry File URI: headers; added URI “scrubbing” helper for deterministic snapshots.
  • Updated fake tool and worker tests to assert against the new (Uri, Vec<Diagnostic>) structure.

@charliecreates charliecreates bot removed the request for review from CharlieHelps December 12, 2025 18:52
@camc314 camc314 self-assigned this Dec 13, 2025
@Sysix Sysix force-pushed the 12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too branch from ff29f34 to 8d04ae2 Compare December 13, 2025 14:13
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Dec 13, 2025
Copy link
Contributor

camc314 commented Dec 13, 2025

Merge activity

> This PR refactors the LSP tool diagnostic API to allow tools to report diagnostics for multiple URIs, not just the file being analyzed. This enables cross-file analysis features like ts config deprecation detection.
@graphite-app graphite-app bot force-pushed the 12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too branch from 8d04ae2 to a3b9eff Compare December 13, 2025 15:00
@graphite-app graphite-app bot merged commit a3b9eff into main Dec 13, 2025
21 checks passed
@graphite-app graphite-app bot deleted the 12-12-refactor_lsp_tools_can_report_diagnostics_for_other_uris_too branch December 13, 2025 15:05
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants