From ae3c2e0afc680b46ba7c8c534f4210ce600f54f2 Mon Sep 17 00:00:00 2001 From: Logan Roberts Date: Wed, 26 Aug 2026 18:38:53 -0400 Subject: [PATCH] Require workspace refresh support before enabling pull diagnostics --- docs/CHANGELOG.md | 2 +- src/server.rs | 55 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 1197bc95..d66cc530 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -13,11 +13,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Diagnostic updates no longer go stale in editors that lack workspace refresh support.** Clients like Neovim that negotiate pull diagnostics (`textDocument/diagnostic`) without advertising `workspace.diagnostic.refreshSupport` were left showing stale diagnostics after an edit, because background debounced passes rely on `workspace/diagnostic/refresh` to prompt the editor to re-pull. Pull diagnostics are now only enabled when the client explicitly advertises refresh support; all other clients receive standard push diagnostics (`publishDiagnostics`). - **Argument checks accept the widenings PHP performs and the types the engine admits it does not know.** Four shapes of correct code were reported as type mismatches: a bounded `int<0, max>` passed to a `float` parameter, even though PHP widens an integer to a float on the way in; a `class-string` passed to `non-empty-string`, even though a string that names a class always has content; an `array-key` passed to `int` or to `string`, which is the key type of an array nobody described rather than a value measured to be two things; and a closure body doing `$a & $b` on untyped parameters, which produces a string from two strings just as readily as an int from two numbers. - **`get_class($x) === Foo::class` narrows the same subjects `instanceof` does.** The identity check only pinned a plain variable, so `get_class($this->held) === Sub::class` and `get_class($items[0]) === Sub::class` left the subject at its declared type and every member read past the check was reported as missing. A property fetch, an array element, and a call result are all narrowed now, in the `$x::class === Foo::class` spelling as well. - **A global function written with a leading backslash is the same function.** `\get_class($x) === Foo::class` and `\is_a($x, Foo::class)` narrowed nothing, and `if (!\class_exists('Vendor\Optional\Config')) { return; }` read as an un-negated guard, so it protected the `return;` instead of everything after it and the guarded class was reported as not found. A class named in such a guard with escaped backslashes (`'Vendor\\Optional\\Config'`) is now matched against the reference it guards, too. - **`deprecated_usage` no longer flags an import, or a trait method implementing a deprecated one.** A `use Some\Deprecated\ClassName;` line was reported even though it only says which `ClassName` the file means; whatever the file does with the class is still flagged where it does it. And a trait method that delegates to the deprecated method it implements (`hasProperty()` calling `$this->resolve()->hasProperty()`) was reported, because a trait has no parent chain of its own to inherit the tag from. PHP flattens a trait into the classes that use it, so the question is now asked where those classes answer it. - - **A strict comparison against a written-out value narrows both of its branches.** `=== null` and `=== false` narrowed, but `=== 0`, `=== 0.0`, `=== ''` and `=== []` narrowed neither the branch that held nor the branches after it, so a `foreach ([null, false, 0, 0.0, '', '0', []] as $value)` dispatch chain reported every arm as a type mismatch. The equal branch now holds exactly that value, and the branch that ruled it out drops it: a property documented `bool|'notLoaded'|null` no longer carries its `'notLoaded'` sentinel past the `if` that handled it. A failed strict `in_array($x, [null, ''], true)` drops the same alternatives, so the guard-and-return idiom leaves a definite value behind it. - **A `do`/`while` condition narrows the loop body on every iteration after the first.** `do { $deps[] = $c; $c = $c->getParentClass(); } while ($c !== null);` collected a `list`, because the body was only ever read against the state it was first entered with. The loop re-enters only when the condition held, so the state carried into iteration two onwards is narrowed by it, while the first iteration keeps the state it actually ran on. - **An inline `@var` above an assignment describes the variable after it, not the right-hand side.** `/** @var Base $b */ $b = $b->inner;` resolved the `$b->inner` read against `Base` rather than against whatever `$b` held on the way in, so a loop that unwraps a nested value one level per pass reported the property as missing. The annotation still types the variable for everything after the assignment. diff --git a/src/server.rs b/src/server.rs index 828d2fdb..06dfc161 100644 --- a/src/server.rs +++ b/src/server.rs @@ -117,12 +117,20 @@ impl LanguageServer for Backend { *self.client_name.lock() = info.name.clone(); } + let client_supports_diagnostic_refresh = params + .capabilities + .workspace + .as_ref() + .and_then(|ws| ws.diagnostic.as_ref()) + .and_then(|d| d.refresh_support) + .unwrap_or(false); let client_supports_pull = params .capabilities .text_document .as_ref() .and_then(|td| td.diagnostic.as_ref()) - .is_some(); + .is_some() + && client_supports_diagnostic_refresh; self.supports_pull_diagnostics .store(client_supports_pull, Ordering::Release); @@ -1833,6 +1841,51 @@ mod tests { assert!(options["documentSelector"][0].get("scheme").is_none()); assert!(options["documentSelector"][0].get("pattern").is_none()); } + + #[tokio::test] + async fn pull_diagnostics_requires_refresh_support() { + use tower_lsp::LanguageServer; + + let backend = Backend::new_test(); + let mut params = InitializeParams::default(); + + // 1. Neither text_document.diagnostic nor workspace.diagnostic.refresh_support + let result = backend.initialize(params.clone()).await.unwrap(); + assert!(!backend.supports_pull_diagnostics.load(Ordering::Acquire)); + assert!(result.capabilities.diagnostic_provider.is_none()); + + // 2. Only text_document.diagnostic without workspace.diagnostic.refresh_support (e.g. Neovim <= 0.11) + params.capabilities.text_document = Some(TextDocumentClientCapabilities { + diagnostic: Some(DiagnosticClientCapabilities::default()), + ..Default::default() + }); + let result = backend.initialize(params.clone()).await.unwrap(); + assert!( + !backend.supports_pull_diagnostics.load(Ordering::Acquire), + "Pull diagnostics must be disabled without refresh_support" + ); + assert!( + result.capabilities.diagnostic_provider.is_none(), + "diagnosticProvider must be None when client cannot handle refresh requests" + ); + + // 3. Both text_document.diagnostic AND workspace.diagnostic.refresh_support = true (e.g. VS Code, Neovim >= 0.12) + params.capabilities.workspace = Some(WorkspaceClientCapabilities { + diagnostic: Some(DiagnosticWorkspaceClientCapabilities { + refresh_support: Some(true), + }), + ..Default::default() + }); + let result = backend.initialize(params).await.unwrap(); + assert!( + backend.supports_pull_diagnostics.load(Ordering::Acquire), + "Pull diagnostics must be enabled when client supports refresh" + ); + assert!( + result.capabilities.diagnostic_provider.is_some(), + "diagnosticProvider must be advertised when client supports pull and refresh" + ); + } } // ─── Self-scan helpers ──────────────────────────────────────────────────────