fix(diagnostics): refresh dependent documents after file changes

[hgschmie]

fix(diagnostics): refresh dependent documents after file changes

With pull diagnostics, changing a type declaration in one file can leave stale
warnings in another open file until the language server restarts or the client
requests diagnostics again. The server advertises no inter-file dependencies
and does not request a refresh after these edits.

For example, open these two files with a pull-diagnostic client:

-- types.lua
---@class Provider
---@field set_values fun()

-- settings.lua
---@type Provider
local settings_table = {}
return settings_table

Comment out the field declaration. The missing-field diagnostic in settings.lua
should disappear, and reappear when the declaration is restored.

This change advertises inter-file dependencies and schedules
workspace/diagnostic/refresh after analysis updates from document open/change,
watched Lua file events, tracked-file removal on close, and file renames.
Configuration reloads use the same refresh lifecycle and capability checks.
Refreshes are sent only when the client supports pull diagnostics and refresh
requests; existing push-diagnostic scheduling is retained.

Rapid edits coalesce using the configured diagnostic interval in milliseconds.
Zero requests a refresh without a debounce delay. Pending tasks hold a weak
client reference. Shutdown cancels pending refreshes before its response and
rejects new scheduling by outstanding updates, even while server snapshots remain
alive. Final sends are serialized with shutdown and replacement; dropping the
service provides fallback cancellation.

Validation on macOS arm64:

• cargo test --locked -p emmylua_ls --all-features: 228 tests passed.
• cargo fmt --all -- --check: passed.
• cargo clippy --locked -p emmylua_ls --all-targets --all-features -- -D warnings: passed.
• python3 crates/emmylua_ls/tests/diagnostic_refresh.py <standalone-server>: passed
  with pull/refresh enabled, pull without refresh support, and push diagnostics.

Rust debounce/lifetime tests use a paused Tokio clock. Protocol tests wait for
analysis changes and refresh requests, and handle known server requests explicitly.
They cover required-field removal/restoration, watcher change/delete/create,
untracked deletion, close/delete, rename, watched and client configuration reloads,
and absence of refreshes between shutdown acknowledgement and exit.

[github-actions]

Code Review: Diagnostic Refresh Coalescing

Summary

The change introduces a debounced/coalesced workspace/diagnostic/refresh notification, switches inter_file_dependencies to true, and adds tests. Overall the design is sound, but there are several correctness and robustness concerns.

---

Issues

1. Drop impl is effectively dead code / misleading (High)

impl Drop for DiagnosticService {
    fn drop(&mut self) {
        if let Some(token) = self.refresh_token.get_mut().take() {
            token.cancel();
        }
    }
}

• DiagnosticService is almost certainly held inside an Arc and lives for the whole process lifetime, so Drop will rarely (if ever) run in practice. The test dropping_diagnostics_cancels_pending_refresh passes only because the test constructs a bare value.
• More importantly, Mutex::get_mut() is fine here, but the cancellation is redundant: the spawned task holds a clone of the token, and when the DiagnosticService is dropped the Mutex is dropped too — but the spawned task still holds its own token clone, so it will still fire client.refresh_workspace_diagnostics() unless cancelled. So the Drop impl is needed for correctness, yet it can't be relied on because the service is Arc-held. This is a latent bug: on shutdown the refresh may still be sent.
• Recommendation: Either document that this only matters for tests, or wire cancellation into the actual shutdown path (e.g. cancel via the existing workspace_diagnostic_token / a shutdown signal). Consider using Weak<ClientProxy> in the spawned task so it doesn't keep the client alive.

2. Spawned task captures Arc<ClientProxy> strongly (Medium)

let client = self.client.clone();
tokio::spawn(async move { ... client.refresh_workspace_diagnostics(); });

This keeps the client (and its connection) alive for the duration of the sleep. Combined with issue #1, a pending refresh can outlive the intended lifetime. Prefer Weak + upgrade() inside the task.

3. schedule_diagnostic_refresh holds the mutex across await points (Low/Medium)

let mut pending = self.refresh_token.lock().await;
if let Some(token) = pending.take() { token.cancel(); }
let token = CancellationToken::new();
*pending = Some(token.clone());
let client = self.client.clone();
tokio::spawn(...);

The lock is held while spawning. tokio::spawn itself doesn't await, so this is not a deadlock, but it does serialize callers and means the guard is held longer than necessary. Minor; consider dropping the guard before spawn. Not a blocker.

4. interval semantics: 0 is treated as "fire immediately" (Low)

In the test, schedule_diagnostic_refresh(0) is used to force an immediate refresh. With tokio::time::sleep(Duration::from_millis(0)) this works, but callers passing 0 from config would bypass coalescing entirely. Worth documenting or clamping.

5. inter_file_dependencies: true is a behavioral change with no migration note (Medium)

Switching this to true tells clients that cross-file invalidation is supported. This is only correct if the server actually emits workspace/diagnostic/refresh in all cases where cross-file results change. The watcher handler now does this, but check other mutation paths (e.g. config changes, didChangeConfiguration, workspace folder changes) — if any of them mutate analysis without scheduling a refresh, clients will show stale diagnostics. Audit all analysis().update(...) call sites.

6. Watcher handler: removed_file computed inside the closure but file_ids may be empty on removal (Low)

let mut removed_file = false;
for uri in &removed_uris {
    removed_file |= analysis.get_file_id(uri).is_some();
    analysis.remove_file_by_uri(uri);
}

This is correct, but note removed_uris may contain URIs that were never tracked — removed_file stays false and no refresh is scheduled. That's probably intended, but if a removed file was a dependency of another file, the dependent's diagnostics could be stale. Consider whether removal of an untracked file that is still referenced by tracked files should trigger a refresh.

7. Test flakiness / timing assumptions (Medium)

• refresh_coalesces_and_is_scoped_to_its_client sleeps 100ms for a 30ms interval — reasonable, but on a loaded CI machine the spawned task may not have run. Prefer tokio::time::pause()/advance() or a channel-based synchronization instead of wall-clock sleeps.
• Client.__init__ does time.sleep(1) after initialized — brittle. Prefer waiting for a specific notification/response.
• refreshes(duration=0.6) uses wall-clock polling; on slow CI the refresh may arrive after the window and the assertion == int(refresh) will fail intermittently. Consider a longer/adaptive deadline or explicit synchronization.
• The Python test asserts client.refreshes() == int(refresh) immediately after 10 rapid didChange notifications. This depends on the server having processed all 10 before the 0.6s window closes. Under load this can flake.

8. Python test: receive auto-responds to any request with id (Low)

if 'method' in message and 'id' in message:
    result = [] if message['method'] == 'workspace/configuration' else None
    self.write({'id': message['id'], 'result': result})

This blindly replies null to server→client requests other than workspace/configuration. If the server sends client/registerCapability or similar, the null result may be invalid and could cause the server to error. Safer to handle known methods explicitly and log unexpected ones.

9. Drop + Mutex interaction with async (Low)

Drop uses get_mut() which is correct (no async), but if DiagnosticService is ever dropped while another task holds the lock, get_mut() will panic? No — get_mut on tokio::sync::Mutex returns &mut T only if &mut self is available, which it is in Drop. Fine. Just noting for reviewers.

10. Missing refresh on didClose (Low)

process_did_open_text_document and process_did_change_text_document schedule a refresh, but didClose (if it mutates analysis) does not appear to. If closing a file changes cross-file diagnostics, clients won't be refreshed. Verify.

---

Recommendations (priority order)

1. Fix lifetime of spawned refresh task — use Weak<ClientProxy> and/or tie cancellation to real shutdown, not Drop of an Arc-held struct.
2. Audit all analysis mutation paths for missing schedule_diagnostic_refresh now that inter_file_dependencies = true.
3. Reduce test flakiness — replace wall-clock sleeps with tokio::time::pause/advance in Rust tests and explicit synchronization in the Python test.
4. Harden the Python test client to not blindly reply null to unknown server requests.
5. Minor: drop the mutex guard before tokio::spawn; document interval == 0 behavior.

Nits

• refresh_token: Mutex<Option<CancellationToken>> is not Arc-wrapped unlike the other token fields — inconsistent, though fine given &self usage. Confirm no Clone of DiagnosticService is expected.
• The doc comment "Coalesce cross-file invalidations after analysis has incorporated an edit" is good; consider noting the interval unit (ms) explicitly.

[hgschmie]

I had GPT address the issues. I'd like to re-trigger the code review but I don't know how.

[hgschmie]

rebased to current main

[CppCXY]

Doing dependency diagnostics for Lua is meaningless. In practice, because globals can be defined anywhere, a change to any line of code in Lua can cause semantic changes across all files. We split diagnostics into two phases: a fast current-file diagnostic via textDocument/diagnostic, and a slow full global diagnostic via workspace/diagnostic. The current workspace/diagnostic implementation may not be able to respond promptly due to issues with the migration.

[hgschmie]

Thanks for explaining. I have seen this with VSCode, where the IDE, after a few seconds of inactivity does a full scan of all files in the workspace. How much of that support is supposed to come from the IDE proper and how much can be driven by the analyzer?

Lastly, I am using this change with a local fork of emmylua and I had great success (especially with files updating instantly). I have a modest sized workspace (~ 2,000 files) and I don't really see any performance issues. I would be curious to learn more what problem you see with potential semantic changes across all files if it is fast enough.

The current lsp4ij for IntelliJ does not implement workspace/diagnostic. I might start by addressing that issue first.

GPT also pointed out to me that "There is also an analyzer-side gap to investigate: its workspace diagnostic loop is invalidated on saves and workspace reloads, but ordinary unsaved edits and watched Lua-file changes do not mark it for another pass."

I will work in that direction, would you be open to reviewing a patch once I wrap that work up?