diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 85a351e15..62c9acb1e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -1423,6 +1423,18 @@ support sees it. Push delivery still caches each published set with a `resultId`, so a pull from a push client answers with what was last published instead of an empty set. +A `textDocument/diagnostic` pull is held until the diagnostics reflect the +document's current text, since editors pull right after every `didChange` +and some (Neovim) do not pull the document again when the server sends +`workspace/diagnostic/refresh`. The pull waits for startup to finish, for +the edit's background parse, for the pass that parse queues, and for the +pass in flight, without computing anything on the request path. Each stage +hands the URI on before letting go of it (`edits_awaiting_parse`, then +`pending_uris`, then `in_flight_uris` on `DiagnosticState`), so the wait +never sees a gap. A newer pull for the same document releases the held +one, and shutdown or the worker stopping releases every held pull. The +`workspace/diagnostic` pull still answers at once from the cache. + Each worker is created during `initialized` via `clone_for_blocking`, which hands it a `Backend` sharing the main one's `BackendState`, so every worker sees all mutations the main `Backend` makes. The PHPStan and PHPCS workers each have their own notify handle, pending-URI slot, and diagnostic cache so they run independently of each other and of the native diagnostic worker. Non-`Arc` fields are snapshotted at spawn time: `php_version`, `vendor_uri_prefixes`, and `vendor_dir_paths`. These fields are only written during `initialized` (before the workers are spawned) and never change afterwards. If a future feature adds runtime PHP version changes, the workers would need to be notified or re-cloned. This invariant ("init-time fields are write-once") should be verified before adding any post-init mutation to these fields. diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index a688d3f58..faf363567 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -598,6 +598,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Same answers in the browser build.** Go-to-definition, rename, hover, signature help, and highlights in the WebAssembly build now resolve types as thoroughly as the language server does. - **`@see` links in hover.** A `@see` reference now links to the class it names, through the declaring file's `use` imports, rather than to another class with the same short name. Go-to-definition and type hierarchy no longer land in the wrong file in the same situation. - **Stale diagnostics after a quick edit.** An editor that pulls diagnostics no longer occasionally keeps showing the previous set when it asks for them just as a new set finishes. +- **Diagnostics catch up after an edit in Neovim.** Fixing an error, such as closing a class's brace, now clears its diagnostic once the edit has been checked, where Neovim used to keep the old diagnostics until the file was reloaded with `:e`. A file opened while the project is still being indexed now shows its diagnostics once indexing finishes, without needing an edit first. Contributed by @stubbedev. - **Completion after an emoji.** Member, variable, array-key, class-name and PHPDoc tag completion now work on a line that has an emoji or other character outside the Basic Multilingual Plane before the cursor. - **A call still missing its `)`.** While you type a call or array whose closing bracket is not there yet, the rest of the class or function it is in keeps working: its imports are no longer reported as unused, and its members still complete, hover and resolve. The same goes for a statement being typed at the end of a file, such as the newest route in a routes file, from the moment its `(` is typed. - **Constants in `switch` cases.** Hover, go-to-definition, find references and rename now work on a constant or class constant written in a `case` label, and an import used only there is no longer reported as unused. diff --git a/src/backend/documents.rs b/src/backend/documents.rs index 043b3096c..a3f69483d 100644 --- a/src/backend/documents.rs +++ b/src/backend/documents.rs @@ -175,9 +175,11 @@ impl Backend { } return; } + self.edit_awaiting_parse(&uri, &text); let backend = self.clone_for_blocking(); tokio::spawn(async move { let refresh_backend = backend.clone_for_blocking(); + let (parsed_uri, parsed_text) = (uri.clone(), Arc::clone(&text)); let committed = run_blocking_cancel_safe("did_change resource scan", move || { let is_latest_text = backend .open_files @@ -194,6 +196,7 @@ impl Backend { true }) .await; + refresh_backend.edit_parsed(&parsed_uri, &parsed_text); if committed == Some(true) { refresh_backend.request_code_lens_refresh().await; @@ -217,10 +220,12 @@ impl Backend { } self.schedule_diagnostics(uri.clone()); } else { + self.edit_awaiting_parse(&uri, &text); let backend = self.clone_for_blocking(); tokio::spawn(async move { let refresh_backend = backend.clone_for_blocking(); let uri_for_diagnostics = uri.clone(); + let (parsed_uri, parsed_text) = (uri.clone(), Arc::clone(&text)); let committed = run_blocking_cancel_safe("did_change parse", move || { let parse_lock = { let mut locks = backend.did_change_parse_locks.lock(); @@ -260,6 +265,7 @@ impl Backend { true }) .await; + refresh_backend.edit_parsed(&parsed_uri, &parsed_text); // A new symbol map was committed. Tokens the editor already // holds were computed from the pre-edit map (the diff --git a/src/backend/startup.rs b/src/backend/startup.rs index 13c5d3de9..1d430ffa0 100644 --- a/src/backend/startup.rs +++ b/src/backend/startup.rs @@ -408,10 +408,7 @@ impl Backend { // (php_version, vendor_uri_prefixes, vendor_dir_paths) are // snapshotted — they are only written during init (above) and // never change afterwards. - let worker_backend = self.clone_for_blocking(); - tokio::spawn(async move { - worker_backend.diagnostic_worker().await; - }); + self.spawn_diagnostic_worker(); // Spawn the PHPStan worker as a separate background task. // PHPStan is extremely slow and resource-intensive, so it runs @@ -562,6 +559,7 @@ impl Backend { // and pull handlers know the project is fully indexed. self.init_complete .store(true, std::sync::atomic::Ordering::Release); + self.diag.pass_done.notify_waiters(); // Files opened during startup (before indexing finished) were // not diagnosed because `schedule_diagnostics` skips work when diff --git a/src/diagnostics/delivery.rs b/src/diagnostics/delivery.rs index 3ea93945d..a7fdded17 100644 --- a/src/diagnostics/delivery.rs +++ b/src/diagnostics/delivery.rs @@ -8,11 +8,31 @@ use tower_lsp_server::gen_lsp_types::*; use crate::Backend; +use super::state::DiagnosticState; use super::{ignore_rules, incremental, stale, suppression}; /// How long to wait after the last keystroke before publishing diagnostics. const DIAGNOSTIC_DEBOUNCE_MS: u64 = 500; +/// Marks the diagnostic worker as running from the moment it is spawned +/// for as long as its task lives, and wakes held document pulls when it +/// stops however it stops. +struct WorkerAlive(DiagnosticState); + +impl WorkerAlive { + fn start(state: &DiagnosticState) -> Self { + state.worker_alive.store(true, Ordering::Release); + Self(state.clone()) + } +} + +impl Drop for WorkerAlive { + fn drop(&mut self) { + self.0.worker_alive.store(false, Ordering::Release); + self.0.pass_done.notify_waiters(); + } +} + impl Backend { /// Deliver diagnostics for a single file. /// @@ -531,14 +551,12 @@ impl Backend { } // In pull mode we deliberately KEEP the previously cached full set. - // The pull handler no longer computes on the request path (that - // wedged the transport — see `trigger_diagnostics_for_pull`), so the - // editor would otherwise flicker to empty between an edit and the - // background recompute. Keeping the stale set means a pull returns - // the old diagnostics until the worker recomputes, bumps the - // resultId, and requests a refresh, which makes the editor re-pull - // the fresh set. The resultId is bumped only by `assemble_and_push`, - // so the "unchanged" fast-path stays correct. + // A document pull waits for the pass queued here (see + // `await_fresh_diagnostics`), but the workspace pull answers at + // once, and an empty set there would flicker the editor between + // an edit and the background recompute. The resultId is bumped + // only by `assemble_and_push`, so the "unchanged" fast-path stays + // correct. // Both modes: queue for the debounced background worker. // In push mode the worker pushes the full assembled set. @@ -557,6 +575,32 @@ impl Backend { // at a time). } + /// Record that `text`, the latest edit of `uri`, is being parsed on a + /// background task that queues the file's diagnostic pass when done, + /// so a document pull sent meanwhile waits for that pass. + pub(crate) fn edit_awaiting_parse(&self, uri: &str, text: &Arc) { + self.diag + .edits_awaiting_parse + .lock() + .insert(uri.to_string(), Arc::clone(text)); + } + + /// Called once the background parse of `text` is over, whether it + /// queued a pass, found a newer edit, or failed. A newer edit's entry + /// is left to that edit's own parse. + pub(crate) fn edit_parsed(&self, uri: &str, text: &Arc) { + { + let mut awaiting = self.diag.edits_awaiting_parse.lock(); + if awaiting + .get(uri) + .is_some_and(|latest| Arc::ptr_eq(latest, text)) + { + awaiting.remove(uri); + } + } + self.diag.pass_done.notify_waiters(); + } + /// Schedule all external tool runs (PHPStan, PHPCS, PHPMD, Mago) for a /// single file. /// @@ -652,13 +696,11 @@ impl Backend { /// cached full diagnostics are stale or missing. Does **not** /// compute synchronously; it only queues the file for the debounced /// background worker and returns immediately (see the comment inside - /// for why). The pull handler reads whatever is currently cached in - /// `diag_last_full`, which may be stale until the worker finishes and - /// requests a `workspace/diagnostic/refresh`. + /// for why). The pull handler then waits for that pass before it + /// answers. pub(crate) fn trigger_diagnostics_for_pull(&self, uri_str: &str) { // Don't compute diagnostics before initialization is complete. - // The pull handler will return empty results; once `initialized` - // finishes it schedules all open files which populates the cache. + // The pull handler waits for it and calls this again. if !self.init_complete.load(Ordering::Acquire) { return; } @@ -676,9 +718,9 @@ impl Backend { // // Instead, ensure the debounced background worker will (re)compute // this file off the async runtime; it caches the full set and - // requests a workspace diagnostic refresh when done. This pull - // returns whatever is currently cached (stale results are kept until - // the refresh, so the editor never flickers to empty). + // requests a workspace diagnostic refresh when done. The pull + // waits for that pass without occupying anything but its own + // request (see `await_fresh_diagnostics`). { let mut pending = self.diag.pending_uris.lock(); pending.insert(uri_str.to_string()); @@ -712,7 +754,20 @@ impl Backend { /// the version counter will have moved, and step 1 picks up /// immediately after step 4 finishes — giving the two-slot /// (one running + one pending) behaviour. - pub(crate) async fn diagnostic_worker(&self) { + /// + /// The worker counts as running from this call on, so a document pull + /// that lands before the task is first polled already waits for it. + pub(crate) fn spawn_diagnostic_worker(&self) -> tokio::task::JoinHandle<()> { + let alive = WorkerAlive::start(&self.diag); + let worker = self.clone_for_blocking(); + tokio::spawn(async move { + let _alive = alive; + worker.diagnostic_worker().await; + }) + } + + /// The loop [`Self::spawn_diagnostic_worker`] runs. + async fn diagnostic_worker(&self) { loop { if self.shutdown_flag.load(Ordering::Acquire) { return; @@ -738,9 +793,14 @@ impl Backend { } // ── Step 3: snapshot all pending URIs ──────────────────── + // They move to `in_flight_uris` under the `pending_uris` + // guard, so a pull held on one of them never sees it as done + // before its pass has delivered. let uris: std::collections::HashSet = { let mut pending = self.diag.pending_uris.lock(); - std::mem::take(&mut *pending) + let uris = std::mem::take(&mut *pending); + self.diag.in_flight_uris.lock().extend(uris.iter().cloned()); + uris }; if uris.is_empty() { continue; @@ -751,14 +811,12 @@ impl Backend { // read lock before each async publish call so that // `did_change` is never blocked. for uri in &uris { - let content = { - let files = self.open_files.read(); - match files.get(uri) { - Some(c) => c.clone(), - None => continue, - } - }; - self.publish_diagnostics_for_file(uri, &content).await; + let content = self.open_files.read().get(uri).cloned(); + if let Some(content) = content { + self.publish_diagnostics_for_file(uri, &content).await; + } + self.diag.in_flight_uris.lock().remove(uri); + self.diag.pass_done.notify_waiters(); } } } diff --git a/src/diagnostics/mod.rs b/src/diagnostics/mod.rs index df734a379..4bbd62dd4 100644 --- a/src/diagnostics/mod.rs +++ b/src/diagnostics/mod.rs @@ -425,8 +425,9 @@ pub(crate) fn checked_byte_range_to_lsp_range( // ── Tests ─────────────────────────────────────────────────────────────────── #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::sync::Arc; + use std::sync::atomic::Ordering; use super::*; @@ -487,8 +488,6 @@ mod tests { /// fill every slot and wedge the transport (the regression this guards). #[test] fn pull_diagnostics_defer_to_worker_and_never_compute_inline() { - use std::sync::atomic::Ordering; - let backend = crate::Backend::new_test(); backend.init_complete.store(true, Ordering::Release); backend.client_caps.write().pull_diagnostics = true; @@ -695,6 +694,7 @@ mod tests { #[tokio::test] async fn push_client_pull_returns_last_published_set() { let backend = Backend::new_test(); + backend.init_complete.store(true, Ordering::Release); assert!(!backend.caps().pull_diagnostics); let uri = "file:///push.php"; backend.diag.last_fast.lock().insert( @@ -710,33 +710,293 @@ mod tests { // Push mode never asks the client to re-pull. assert!(!backend.assemble_and_push(uri).await); - let pull = |previous_result_id: Option| { - backend - .document_pull_diagnostic(DocumentDiagnosticParams { - text_document: TextDocumentIdentifier { - uri: uri.parse().unwrap(), - }, - identifier: None, - previous_result_id, - work_done_progress_params: Default::default(), - partial_result_params: Default::default(), - }) - .unwrap() - }; - - let DocumentDiagnosticReport::RelatedFullDocumentDiagnosticReport(full) = pull(None) else { - panic!("first pull must return the full set"); - }; - let report = full.full_document_diagnostic_report; + let full = full_report(pull_document(&backend, uri, None).await) + .expect("first pull must return the full set"); + let report = full; assert_eq!(report.items.len(), 1); assert_eq!(report.items[0].message, "boom".to_string().into()); // Re-publishing the same set leaves the client's result valid. backend.assemble_and_push(uri).await; - assert!(matches!( - pull(report.result_id), - DocumentDiagnosticReport::RelatedUnchangedDocumentDiagnosticReport(_) - )); + assert!(full_report(pull_document(&backend, uri, report.result_id).await).is_none()); + } + + /// The full set a pull answered with, or `None` for `Unchanged`. + pub(crate) fn full_report( + report: DocumentDiagnosticReport, + ) -> Option { + match report { + DocumentDiagnosticReport::RelatedFullDocumentDiagnosticReport(full) => { + Some(full.full_document_diagnostic_report) + } + DocumentDiagnosticReport::RelatedUnchangedDocumentDiagnosticReport(_) => None, + } + } + + async fn pull_document( + backend: &Backend, + uri: &str, + previous_result_id: Option, + ) -> DocumentDiagnosticReport { + backend + .document_pull_diagnostic(DocumentDiagnosticParams { + text_document: TextDocumentIdentifier { + uri: uri.parse().unwrap(), + }, + identifier: None, + previous_result_id, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }) + .await + .unwrap() + } + + fn edit_open_file(backend: &Backend, uri: &str, text: &str) { + backend + .open_files + .write() + .insert(uri.to_string(), Arc::new(text.to_string())); + backend.update_ast(uri, text); + backend.schedule_diagnostics(uri.to_string()); + } + + /// A pull sent right after an edit must answer with the diagnostics + /// of the edited text. Editors pull immediately after `didChange`; + /// answering from the cache then reported the set from before the + /// edit as `Unchanged`, and a client that only re-pulls the workspace + /// on refresh (Neovim) never saw the fresh set. + #[tokio::test] + async fn pull_after_edit_answers_with_the_edited_text() { + let mut backend = Backend::new_test(); + // Parse edits on a background task, as the server does, so the + // pull lands before the edit's pass is queued. + backend.state_mut().sync_ast_updates = false; + backend.init_complete.store(true, Ordering::Release); + backend.client_caps.write().pull_diagnostics = true; + let worker = backend.spawn_diagnostic_worker(); + + let uri = "file:///stale_pull.php"; + edit_open_file(&backend, uri, " tokio::task::JoinHandle { + let backend = backend.clone_for_blocking(); + tokio::spawn(async move { pull_document(&backend, uri, None).await }) } /// `publish_diagnostics_for_file` must not resurrect a closed file's diff --git a/src/diagnostics/pull.rs b/src/diagnostics/pull.rs index 6c12ce2e4..989d8ce86 100644 --- a/src/diagnostics/pull.rs +++ b/src/diagnostics/pull.rs @@ -14,15 +14,57 @@ use tower_lsp_server::jsonrpc::Result; use crate::Backend; +use super::state::DiagnosticState; + +/// A held document pull's place in `pull_generations`. Taking one +/// supersedes the URI's previous pull; dropping it, when the pull answers +/// or the client cancels it, removes the entry unless a newer pull owns it. +struct PullTicket<'a> { + state: &'a DiagnosticState, + uri: &'a str, + generation: u64, +} + +impl<'a> PullTicket<'a> { + fn take(state: &'a DiagnosticState, uri: &'a str) -> Self { + let generation = state.pull_seq.fetch_add(1, Ordering::Relaxed); + state + .pull_generations + .lock() + .insert(uri.to_string(), generation); + state.pass_done.notify_waiters(); + Self { + state, + uri, + generation, + } + } + + fn superseded(&self) -> bool { + self.state.pull_generations.lock().get(self.uri) != Some(&self.generation) + } +} + +impl Drop for PullTicket<'_> { + fn drop(&mut self) { + let mut generations = self.state.pull_generations.lock(); + if generations.get(self.uri) == Some(&self.generation) { + generations.remove(self.uri); + } + } +} + impl Backend { /// Body of `textDocument/diagnostic`. See [`server`](crate::server)'s /// module docs for how pull and push delivery are selected. - pub(crate) fn document_pull_diagnostic( + pub(crate) async fn document_pull_diagnostic( &self, params: DocumentDiagnosticParams, ) -> Result { let uri_str = params.text_document.uri.to_string(); + self.await_fresh_diagnostics(&uri_str).await; + // Check resultId — if the client sends back the same resultId we // last returned AND the full cache is still present (not // invalidated by schedule_diagnostics), the diagnostics have not @@ -50,25 +92,6 @@ impl Backend { } } - // In pull mode the pull request *triggers* native diagnostic - // computation, but never computes it inline: a synchronous - // compute on the request path would hold a tower-lsp concurrency - // slot for seconds and wedge the transport under a typing burst - // (see `trigger_diagnostics_for_pull`). If the full cache is - // missing for this URI, this call queues the debounced - // background worker and returns whatever is currently cached - // (possibly empty); the worker recomputes, caches the full set, - // and requests a `workspace/diagnostic/refresh` so the editor - // re-pulls the real results a moment later. - let needs_compute = { - let cache = self.diag.last_full.lock(); - !cache.contains_key(&uri_str) - }; - - if needs_compute { - self.trigger_diagnostics_for_pull(&uri_str); - } - let (diagnostics, result_id) = { let cache = self.diag.last_full.lock(); let ids = self.diag.result_ids.lock(); @@ -90,6 +113,70 @@ impl Backend { ) } + /// Hold a document pull until the diagnostics it answers with reflect + /// the document's current text. + /// + /// Editors pull right after every `didChange`. Answered at once, that + /// pull reports the set from before the edit, as `Unchanged` when the + /// client echoes the last `resultId`, and the fresh set only reaches a + /// client that pulls the document again on `workspace/diagnostic/refresh`. + /// Neovim answers the refresh with a `workspace/diagnostic` pull only, + /// so it kept showing the stale set until the buffer was reloaded. The + /// same went for a pull that landed while the project was still being + /// indexed at startup, which was answered with an empty set. + /// + /// So the pull waits for startup to finish, queues a pass when nothing + /// is cached for the document, and waits while the worker still owes + /// the document a pass. Nothing is computed on the request path: a + /// synchronous compute would hold a tower-lsp concurrency slot for + /// seconds and wedge the transport under a typing burst (see + /// `trigger_diagnostics_for_pull`). A newer pull for the same document + /// releases the held one, so a typing burst holds at most one request + /// per document. A cancelled request drops this future, which is + /// equally safe. + async fn await_fresh_diagnostics(&self, uri: &str) { + let ticket = PullTicket::take(&self.diag, uri); + let mut queued = false; + + loop { + let woken = self.diag.pass_done.notified(); + tokio::pin!(woken); + // Register before checking, so a wake-up between the check + // and the await is not lost. + woken.as_mut().enable(); + if ticket.superseded() || self.shutdown_flag.load(Ordering::Acquire) { + return; + } + if self.init_complete.load(Ordering::Acquire) { + if !queued && !self.diag.last_full.lock().contains_key(uri) { + self.trigger_diagnostics_for_pull(uri); + } + queued = true; + if !self.diagnostic_pass_outstanding(uri) { + return; + } + } + woken.await; + } + } + + /// Whether the running diagnostic worker still owes `uri` a pass: its + /// latest edit is still being parsed, or it is queued for the worker, + /// or being diagnosed by it. Each stage hands the URI on before it + /// lets go of it, so they are read in that order: the parse queues the + /// URI before it drops its `edits_awaiting_parse` entry, and the worker + /// moves it into `in_flight_uris` before it releases `pending_uris`. + fn diagnostic_pass_outstanding(&self, uri: &str) -> bool { + if !self.diag.worker_alive.load(Ordering::Acquire) { + return false; + } + if self.diag.edits_awaiting_parse.lock().contains_key(uri) { + return true; + } + let pending = self.diag.pending_uris.lock(); + pending.contains(uri) || self.diag.in_flight_uris.lock().contains(uri) + } + /// Body of `workspace/diagnostic`. Reports open files from the live /// per-file pipeline and unopened files from the background workspace /// pass. diff --git a/src/diagnostics/state.rs b/src/diagnostics/state.rs index c5eadf41e..f2cc2b78e 100644 --- a/src/diagnostics/state.rs +++ b/src/diagnostics/state.rs @@ -24,6 +24,32 @@ pub(crate) struct DiagnosticState { pub(crate) notify: Arc, /// File URIs that need a diagnostic pass, drained by the worker. pub(crate) pending_uris: Arc>>, + /// Open files whose latest edit is still being parsed, with that + /// edit's text. The parse queues the file's diagnostic pass, so until + /// it has, the file is owed a pass that `pending_uris` does not show + /// yet. The entry is removed only after the pass is queued, and only + /// by the parse of the text it holds. + pub(crate) edits_awaiting_parse: Arc>>>, + /// File URIs the worker has drained from `pending_uris` and is still + /// diagnosing. A URI moves here under the `pending_uris` guard, so + /// it is never missing from both while its pass is outstanding. + pub(crate) in_flight_uris: Arc>>, + /// Wakes held document pulls (with `notify_waiters`) whenever startup + /// completes, the worker finishes a URI's pass, a newer document pull + /// supersedes a held one, the worker stops, or the server shuts down. + pub(crate) pass_done: Arc, + /// Whether the diagnostic worker is running. Cleared when it stops, + /// on shutdown or a panic, so pulls stop waiting on a queue nothing + /// drains any more. + pub(crate) worker_alive: Arc, + /// The latest `textDocument/diagnostic` request held per URI, drawn + /// from `pull_seq`, so a held pull can tell that a newer one for the + /// same document replaced it. The newest pull removes its entry when + /// it answers. + pub(crate) pull_generations: Arc>>, + /// Session-global sequence feeding `pull_generations`, so an entry + /// removed and re-created never reuses a value a straggler holds. + pub(crate) pull_seq: Arc, /// Last-published slow diagnostics per file URI. pub(crate) last_slow: Arc>>>, /// What each open file's next slow pass can start from instead of @@ -103,6 +129,12 @@ impl DiagnosticState { version: Arc::new(AtomicU64::new(0)), notify: Arc::new(Notify::new()), pending_uris: Arc::new(Mutex::new(HashSet::new())), + edits_awaiting_parse: Arc::new(Mutex::new(HashMap::new())), + in_flight_uris: Arc::new(Mutex::new(HashSet::new())), + pass_done: Arc::new(Notify::new()), + worker_alive: Arc::new(AtomicBool::new(false)), + pull_generations: Arc::new(Mutex::new(HashMap::new())), + pull_seq: Arc::new(AtomicU64::new(0)), last_slow: Arc::new(Mutex::new(HashMap::new())), slow_baselines: Arc::new(Mutex::new(HashMap::new())), full_slow_pass_requested: Arc::new(Mutex::new(HashSet::new())), diff --git a/src/mem_audit.rs b/src/mem_audit.rs index 44f28cd6b..8a0c017e2 100644 --- a/src/mem_audit.rs +++ b/src/mem_audit.rs @@ -1970,6 +1970,9 @@ pub(crate) fn report(backend: &Backend, runner_content_bytes: usize) { }); probe("diagnostic state", &mut || { *backend.diag.pending_uris.lock() = Default::default(); + *backend.diag.edits_awaiting_parse.lock() = Default::default(); + *backend.diag.in_flight_uris.lock() = Default::default(); + *backend.diag.pull_generations.lock() = Default::default(); *backend.diag.last_slow.lock() = Default::default(); *backend.diag.last_fast.lock() = Default::default(); *backend.diag.last_full.lock() = Default::default(); diff --git a/src/server.rs b/src/server.rs index a9767653e..ab05cc346 100644 --- a/src/server.rs +++ b/src/server.rs @@ -287,6 +287,7 @@ impl LanguageServer for Backend { // of sleeping until the next edit arrives. self.diag.notify.notify_one(); self.diag.workspace_pull_notify.notify_one(); + self.diag.pass_done.notify_waiters(); self.phpstan_tool.notify.notify_one(); self.phpcs_tool.notify.notify_one(); self.phpmd_tool.notify.notify_one(); @@ -1263,7 +1264,7 @@ impl LanguageServer for Backend { &self, params: DocumentDiagnosticParams, ) -> Result { - self.document_pull_diagnostic(params) + self.document_pull_diagnostic(params).await } async fn workspace_diagnostic( diff --git a/src/twig/tests.rs b/src/twig/tests.rs index c2db6101d..7fa23a69f 100644 --- a/src/twig/tests.rs +++ b/src/twig/tests.rs @@ -187,3 +187,69 @@ fn deleting_a_template_on_disk_rediagnoses_the_files_that_name_it() { assert!(backend.diag.pending_uris.lock().contains(&index)); } + +/// A pull sent right after an edit to a template waits for the +/// background re-scan of the edit and the pass it queues, and answers +/// with the edited template's diagnostics. +#[tokio::test] +async fn a_pull_right_after_editing_a_template_answers_for_the_edit() { + use crate::diagnostics::tests::full_report; + use std::sync::atomic::Ordering; + use tower_lsp_server::gen_lsp_types::*; + + let page = "{% include 'menu.html' %}\n{% include 'footer.html' %}\n"; + let (mut backend, dir) = workspace(&[ + ("theme/default/menu.html", "\n"), + ("theme/default/index.html", page), + ]); + backend.state_mut().sync_ast_updates = false; + backend.ensure_workspace_indexed(); + backend.init_complete.store(true, Ordering::Release); + backend.client_caps.write().pull_diagnostics = true; + let worker = backend.spawn_diagnostic_worker(); + + let index = open(&backend, &dir, "theme/default/index.html", page).await; + let pull = |previous_result_id: Option| { + backend.document_pull_diagnostic(DocumentDiagnosticParams { + text_document: TextDocumentIdentifier { + uri: index.parse().unwrap(), + }, + identifier: None, + previous_result_id, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }) + }; + + let missing = + full_report(pull(None).await.unwrap()).expect("a first pull returns the full set"); + assert_eq!(missing.items.len(), 1, "{:?}", missing.items); + + backend + .on_did_change(DidChangeTextDocumentParams { + text_document: VersionedTextDocumentIdentifier { + text_document_identifier: TextDocumentIdentifier { + uri: index.parse().unwrap(), + }, + version: 2, + }, + content_changes: vec![ + TextDocumentContentChangeEvent::TextDocumentContentChangeWholeDocument( + TextDocumentContentChangeWholeDocument { + text: "{% include 'menu.html' %}\n".to_string(), + }, + ), + ], + }) + .await; + let fixed = full_report(pull(missing.result_id.clone()).await.unwrap()) + .expect("a pull right after the edit must not answer Unchanged"); + assert!( + fixed.items.is_empty(), + "stale diagnostics: {:?}", + fixed.items + ); + assert!(backend.diag.edits_awaiting_parse.lock().is_empty()); + + worker.abort(); +} diff --git a/tests/integration/diagnostics_pull.rs b/tests/integration/diagnostics_pull.rs new file mode 100644 index 000000000..9004249b0 --- /dev/null +++ b/tests/integration/diagnostics_pull.rs @@ -0,0 +1,74 @@ +//! `textDocument/diagnostic` answers with the diagnostics of the text the +//! editor holds, including a pull sent right after an edit, as Neovim +//! sends one, before the server has re-parsed the edit. + +#[cfg(test)] +mod tests { + use crate::common::{APP_PSR4_COMPOSER, create_psr4_workspace, open_document}; + use tower_lsp_server::LanguageServer; + use tower_lsp_server::gen_lsp_types::*; + + async fn pull( + backend: &phpantom_lsp::Backend, + uri: &Uri, + previous_result_id: Option, + ) -> DocumentDiagnosticReport { + backend + .diagnostic(DocumentDiagnosticParams { + text_document: TextDocumentIdentifier { uri: uri.clone() }, + identifier: None, + previous_result_id, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }) + .await + .unwrap() + } + + fn full(report: DocumentDiagnosticReport) -> Option { + match report { + DocumentDiagnosticReport::RelatedFullDocumentDiagnosticReport(full) => { + Some(full.full_document_diagnostic_report) + } + DocumentDiagnosticReport::RelatedUnchangedDocumentDiagnosticReport(_) => None, + } + } + + #[tokio::test] + async fn a_pull_right_after_fixing_an_error_no_longer_reports_it() { + let broken = "