From 77fa1961f491703489372c880f834f294c7b5e24 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 2 Jul 2026 09:53:55 -0700 Subject: [PATCH 01/13] Drop redundant document updates in the LS If the user types five characters, we get five document update events. If no language service requests are made in the interim, only the last one actually matters. At ~120ms each, it's worth dropping the ones we don't need. We already had code for detecting that an update superseded a previous update (same file, higher version), but it never actually kicked in because of node's event loop: processing the update blocks the event loop so keystrokes that happen during that processing get queued up in VS Code without turning into events that would queue up (and get merged) in our language service. Mine and I figured out where to put a setTimeout to yield so that more events could join the queue, but it was very fussy trying to tune the timeout duration. It also fought with a prior fix (#1682) with its own timeout. A better fix (guided by me but implemented by Copilot) makes completion wait until the version it wants is available (instead of for a fixed amount of time) and uses a flexible amount of yielding in the update handler. Benefits: - No longer sleep for >= 50ms before returning completions - Drop document updates that would serve no purpose Sadly, there's no good way to measure the improvement since we don't have the ability to detect when the keystroke comes in - we can only measure from the start of our processing to its end, skipping over the time spent waiting for a turn in the node event loop. --- source/language_service/src/lib.rs | 146 +++++++++++++- source/language_service/src/protocol.rs | 3 + source/language_service/src/state.rs | 17 ++ source/language_service/src/tests.rs | 162 +++++++++++++++- .../language_service/src/typing_simulation.rs | 39 ++++ source/npm/qsharp/src/compiler/compiler.ts | 5 +- .../src/language-service/language-service.ts | 83 +++++++- source/npm/qsharp/test/languageService.js | 17 +- source/playground/src/main.tsx | 1 + source/vscode/package.json | 10 + source/vscode/src/config.ts | 6 + .../vscode/src/language-service/activate.ts | 14 +- .../vscode/src/language-service/completion.ts | 8 +- .../completion-retrigger.test.ts | 182 ++++++++++++++++++ .../suites/language-service/index.browser.ts | 2 + .../suites/language-service/index.node.ts | 2 + .../update-coalescing.test.ts | 119 ++++++++++++ source/wasm/src/language_service.rs | 78 +++++++- 18 files changed, 863 insertions(+), 31 deletions(-) create mode 100644 source/language_service/src/typing_simulation.rs create mode 100644 source/vscode/test/suites/language-service/completion-retrigger.test.ts create mode 100644 source/vscode/test/suites/language-service/update-coalescing.test.ts diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index ad0426ddbef..41fdf508059 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -20,9 +20,11 @@ mod state; mod test_utils; #[cfg(test)] mod tests; +pub mod typing_simulation; use compilation::Compilation; use futures::channel::mpsc::{TryRecvError, UnboundedReceiver, UnboundedSender, unbounded}; +use futures::channel::oneshot; use futures_util::StreamExt; use log::{trace, warn}; use protocol::{ @@ -48,6 +50,19 @@ pub struct LanguageService { state: Rc>, /// Channel for compilation state update messages coming from the client. state_updater: Option>, + /// Callers parked in [`LanguageService::wait_for_document_version`]. Woken after + /// every applied batch of updates, at which point they re-inspect the state. + version_waiters: Rc>>>, +} + +/// The outcome of waiting for a specific version of a document to be compiled. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum VersionWait { + /// The compilation state reflects exactly the requested version. + Ready, + /// The document has already moved past the requested version. That version was + /// coalesced away and will never be compiled, so it can no longer be answered for. + Superseded, } impl LanguageService { @@ -57,6 +72,7 @@ impl LanguageService { position_encoding, state: Rc::default(), state_updater: Option::default(), + version_waiters: Rc::default(), } } @@ -64,7 +80,7 @@ impl LanguageService { /// to the update channel and apply them, sequentially, to the compilation state. /// /// This method *must* be called for the language service to do any work. - /// The caller needs to start the handler by calling `.run()` . + /// The caller needs to start the handler by calling `.run()` with a yield function. pub fn create_update_handler<'a>( &mut self, diagnostics_receiver: impl Fn(DiagnosticUpdate) + 'a, @@ -84,6 +100,7 @@ impl LanguageService { self.position_encoding, ), recv, + version_waiters: self.version_waiters.clone(), }; self.state_updater = Some(send); handler @@ -183,6 +200,49 @@ impl LanguageService { }); } + /// Waits until the compilation state reflects exactly `version` of `uri`. + /// + /// The match has to be exact. A later version is not an acceptable substitute: the + /// caller's `position` was computed against `version`, so against newer text it may + /// point somewhere else entirely, or not exist at all. + /// + /// The returned future is independent of `&self` so that callers can hold it across + /// await points without keeping the language service borrowed. + pub fn wait_for_document_version( + &self, + uri: &str, + version: u32, + ) -> impl std::future::Future + 'static + use<> { + let state = self.state.clone(); + let waiters = self.version_waiters.clone(); + let uri = uri.to_string(); + + async move { + loop { + // Scoped so the borrow is released before awaiting. Holding it across an + // await would break the updater, which needs mutable access. + let receiver = { + let state = state.borrow(); + match state.get_open_document_version(&uri) { + Some(current) if current == version => return VersionWait::Ready, + Some(current) if current > version => return VersionWait::Superseded, + // Either behind, or the document hasn't been processed at all yet. + _ => { + let (send, recv) = oneshot::channel(); + waiters.borrow_mut().push(send); + recv + } + } + }; + + if receiver.await.is_err() { + // The update handler is gone, so the version will never arrive. + return VersionWait::Superseded; + } + } + } + } + #[must_use] pub fn get_code_actions(&self, uri: &str, range: Range) -> Vec { self.document_op( @@ -336,8 +396,14 @@ impl LanguageService { pub struct UpdateHandler<'a> { updater: CompilationStateUpdater<'a>, recv: UnboundedReceiver, + version_waiters: Rc>>>, } +/// Caps how many times [`UpdateHandler::run`] will give the host a turn to deliver more +/// input before processing what it has. Only bounds the worst case: the loop normally +/// exits earlier, as soon as a yield produces nothing new. +const MAX_YIELDS_PER_BATCH: usize = 4; + impl UpdateHandler<'_> { /// Runs the update handler. This method is expected to run /// for the entire lifetime of the language service. @@ -346,9 +412,37 @@ impl UpdateHandler<'_> { /// language service has explicitly closed the message /// channel, in `stop_update_handler()`. /// - pub async fn run(&mut self) { + /// `yield_to_host` gives the host event loop a turn. Applying an update blocks that + /// loop, so input events that arrive while one is in flight aren't delivered until + /// we yield. Without yielding, the channel looks empty and each event ends up being + /// processed on its own instead of being coalesced with the others. + pub async fn run(&mut self, yield_to_host: F) + where + F: Fn() -> Fut, + Fut: std::future::Future, + { while let Some(update) = self.recv.next().await { - self.apply_this_and_pending(vec![update]).await; + let mut updates = vec![update]; + + // Keep giving the host a turn for as long as it has more to deliver. When the + // user pauses, the first yield comes back with nothing and this exits after a + // single tick. While they're typing, the backlog arrives over a few passes. + for _ in 0..MAX_YIELDS_PER_BATCH { + yield_to_host().await; + + let batch_before = updates.len(); + let drained = self.drain_pending(&mut updates); + if drained == 0 { + break; + } + + // Every drained update either claims a new slot in the batch or merges + // into an existing one, and each merge discards one update. + let dropped = drained - (updates.len() - batch_before); + trace!("drained {drained} update(s), merging dropped {dropped} as redundant"); + } + + self.apply(updates).await; } } @@ -361,19 +455,31 @@ impl UpdateHandler<'_> { /// if `run()` has been called. #[cfg(test)] async fn apply_pending(&mut self) { - self.apply_this_and_pending(vec![]).await; + let mut updates = Vec::new(); + self.drain_pending(&mut updates); + self.apply(updates).await; } - async fn apply_this_and_pending(&mut self, mut updates: Vec) { - // Consume any backed up messages in the channel as well. + /// Drains everything currently queued into `updates`, returning the number of + /// messages received. A closed channel reports zero, since `Closed` means empty + /// *and* closed: messages already buffered are still handed over first. + /// + /// The count is of messages received rather than the length of `updates`, because + /// [`push_update`] merges redundant updates in place and so may not grow it. + fn drain_pending(&mut self, updates: &mut Vec) -> usize { + let mut received = 0; loop { match self.recv.try_recv() { - Ok(update) => push_update(&mut updates, update), - Err(TryRecvError::Closed) => return, // channel has been closed, don't bother with updates. - Err(TryRecvError::Empty) => break, + Ok(update) => { + push_update(updates, update); + received += 1; + } + Err(TryRecvError::Empty | TryRecvError::Closed) => return received, } } + } + async fn apply(&mut self, mut updates: Vec) { trace!("applying {} updates", updates.len()); if updates.len() > 100 { // This indicates that we're not keeping up with incoming updates. @@ -389,6 +495,11 @@ impl UpdateHandler<'_> { apply_update(&mut self.updater, update).await; } trace!("end applying updates"); + + // Let anyone waiting on a particular version re-check where the state landed. + for waiter in self.version_waiters.borrow_mut().drain(..) { + let _ = waiter.send(()); + } } } @@ -487,3 +598,20 @@ enum Update { notebook_uri: String, }, } + +impl Update { + #[cfg(test)] + fn summary(&self) -> String { + match self { + Update::Configuration { .. } => "Configuration".to_string(), + Update::Document { uri, version, .. } => format!("Document({uri}, {version})"), + Update::CloseDocument { uri, .. } => format!("CloseDocument({uri})"), + Update::NotebookDocument { notebook_uri, .. } => { + format!("NotebookDocument({notebook_uri})") + } + Update::CloseNotebookDocument { notebook_uri } => { + format!("CloseNotebookDocument({notebook_uri})") + } + } + } +} diff --git a/source/language_service/src/protocol.rs b/source/language_service/src/protocol.rs index 3b524425c9a..e727bddc562 100644 --- a/source/language_service/src/protocol.rs +++ b/source/language_service/src/protocol.rs @@ -17,6 +17,9 @@ pub struct WorkspaceConfigurationUpdate { pub language_features: Option, pub lints_config: Option>, pub dev_diagnostics: Option, + /// Test-only. Artificially slows down each document update to make the update + /// loop's coalescing behavior observable in a release build. + pub simulated_compile_delay_ms: Option, } #[derive(Clone, Debug, Diagnostic, Error)] diff --git a/source/language_service/src/state.rs b/source/language_service/src/state.rs index df573131707..bec0a513b07 100644 --- a/source/language_service/src/state.rs +++ b/source/language_service/src/state.rs @@ -70,6 +70,8 @@ struct Configuration { pub lints_config: Vec, /// Enables non-user-facing developer diagnostics. pub dev_diagnostics: bool, + /// Test-only. See [`crate::typing_simulation`]. + pub simulated_compile_delay_ms: u32, } impl Default for Configuration { @@ -80,6 +82,7 @@ impl Default for Configuration { language_features: LanguageFeatures::default(), lints_config: Vec::default(), dev_diagnostics: false, + simulated_compile_delay_ms: 0, } } } @@ -191,6 +194,8 @@ impl<'a> CompilationStateUpdater<'a> { self.insert_buffer_aware_compilation(project); + crate::typing_simulation::busy_wait(self.configuration.simulated_compile_delay_ms); + self.publish_diagnostics_and_test_callables(); } @@ -585,6 +590,11 @@ impl<'a> CompilationStateUpdater<'a> { self.configuration.dev_diagnostics = dev_diagnostics; } + // Doesn't affect compilation output, so never triggers a recompile. + if let Some(delay_ms) = configuration.simulated_compile_delay_ms { + self.configuration.simulated_compile_delay_ms = delay_ms; + } + // Possible optimization: some projects will have overrides for these configurations, // so workspace updates won't impact them. We could exclude those projects // from recompilation, but we don't right now. @@ -677,6 +687,12 @@ fn is_openqasm_file(language_id: &str) -> bool { } impl CompilationState { + /// The version of `uri` that the client last told us about and that has since been + /// applied. `None` if we haven't processed any update for the document yet. + pub(crate) fn get_open_document_version(&self, uri: &str) -> Option { + self.open_documents.get(uri).map(|doc| doc.version) + } + pub(crate) fn get_compilation(&self, uri: &str) -> Option<&Compilation> { let compilation_uri = &self .open_documents @@ -794,5 +810,6 @@ fn merge_configurations( .unwrap_or(workspace_scope.language_features), lints_config: merged_lints, dev_diagnostics: workspace_scope.dev_diagnostics, + simulated_compile_delay_ms: workspace_scope.simulated_compile_delay_ms, } } diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 27d0cef67a1..26e6ade530a 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -2,13 +2,17 @@ // Licensed under the MIT License. use crate::{ - Encoding, LanguageService, UpdateHandler, - protocol::{DiagnosticUpdate, ErrorKind, TestCallables}, + Encoding, LanguageService, Update, UpdateHandler, + protocol::{DiagnosticUpdate, ErrorKind, TestCallables, WorkspaceConfigurationUpdate}, + push_update, }; use expect_test::{Expect, expect}; use miette::Diagnostic; use qsc::{compile, line_column::Position, project}; -use std::{cell::RefCell, rc::Rc}; +use std::{ + cell::{Cell, RefCell}, + rc::Rc, +}; use test_fs::{FsNode, TestProjectHost, dir, file}; pub(crate) mod test_fs; @@ -367,6 +371,158 @@ fn assert_compilation(ls: &LanguageService, uri: &str, expected: &Expect) { expected.assert_debug_eq(&compilation.user_unit().sources); } +/// Drives the real `run()` loop to verify that updates delivered by the host while an +/// update is in flight get coalesced into a single compilation. +/// +/// The host event loop is simulated by the yield closure: it pushes updates on the +/// iteration where a real host would have delivered queued keystrokes. No timers are +/// involved, so this is deterministic. +#[tokio::test] +async fn run_coalesces_updates_delivered_while_yielding() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + // Unterminated namespace, so every version reports a diagnostic and is therefore + // observable in `received_errors`. + ls.update_document("foo.qs", 1, "namespace Foo { ", "qsharp"); + + let ls = RefCell::new(ls); + let yields = Cell::new(0); + let yield_to_host = || { + match yields.replace(yields.get() + 1) { + 0 => { + // Two more keystrokes land while the first update is being handled. + ls.borrow_mut() + .update_document("foo.qs", 2, "namespace Foo { a", "qsharp"); + ls.borrow_mut() + .update_document("foo.qs", 3, "namespace Foo { ab", "qsharp"); + } + // Nothing further arrives, so the loop should stop yielding. Closing the + // channel is what lets `run()` return instead of waiting forever. + _ => ls.borrow_mut().stop_updates(), + } + std::future::ready(()) + }; + + worker.run(yield_to_host).await; + + let applied: Vec> = received_errors + .borrow() + .iter() + .map(|(_, version, _, _)| *version) + .collect(); + + // All three collapse into one compilation. Version 1 is included even though it was + // dequeued first, because yielding happens before it is applied, so there is nothing + // in flight that has to be finished. The diagnostics that do get published describe + // the document as it actually stands. + assert_eq!(applied, vec![Some(3)]); + + // One yield to pick up the backlog, one to discover there is nothing left. + assert_eq!(yields.get(), 2); +} + +/// Coalescing must not drop updates that aren't redundant with each other. +#[tokio::test] +async fn run_applies_updates_to_distinct_documents() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 1, "namespace Foo { ", "qsharp"); + + let ls = RefCell::new(ls); + let yields = Cell::new(0); + let yield_to_host = || { + match yields.replace(yields.get() + 1) { + 0 => ls + .borrow_mut() + .update_document("bar.qs", 1, "namespace Bar { ", "qsharp"), + _ => ls.borrow_mut().stop_updates(), + } + std::future::ready(()) + }; + + worker.run(yield_to_host).await; + + // Diagnostics get republished for every compilation on each update, so compare the + // set of documents that were compiled rather than the exact publish sequence. + let mut applied: Vec = received_errors + .borrow() + .iter() + .map(|(uri, _, _, _)| uri.clone()) + .collect(); + applied.sort(); + applied.dedup(); + + assert_eq!(applied, ["bar.qs", "foo.qs"]); +} + +#[test] +fn push_update_merges_consecutive_updates_to_same_document() { + let mut updates = Vec::new(); + push_update(&mut updates, document_update("foo.qs", 1)); + push_update(&mut updates, document_update("foo.qs", 2)); + push_update(&mut updates, document_update("foo.qs", 3)); + + assert_eq!(update_summaries(&updates), ["Document(foo.qs, 3)"]); +} + +#[test] +fn push_update_keeps_updates_to_different_documents() { + let mut updates = Vec::new(); + push_update(&mut updates, document_update("foo.qs", 1)); + push_update(&mut updates, document_update("bar.qs", 1)); + push_update(&mut updates, document_update("foo.qs", 2)); + + assert_eq!( + update_summaries(&updates), + [ + "Document(foo.qs, 1)", + "Document(bar.qs, 1)", + "Document(foo.qs, 2)" + ] + ); +} + +#[test] +fn push_update_does_not_merge_across_a_configuration_update() { + let mut updates = Vec::new(); + push_update(&mut updates, document_update("foo.qs", 1)); + push_update( + &mut updates, + Update::Configuration { + changed: WorkspaceConfigurationUpdate::default(), + }, + ); + push_update(&mut updates, document_update("foo.qs", 2)); + + assert_eq!( + update_summaries(&updates), + [ + "Document(foo.qs, 1)", + "Configuration", + "Document(foo.qs, 2)" + ] + ); +} + +fn document_update(uri: &str, version: u32) -> Update { + Update::Document { + uri: uri.into(), + version, + text: "namespace Foo { }".into(), + language_id: "qsharp".into(), + } +} + +fn update_summaries(updates: &[Update]) -> Vec { + updates.iter().map(Update::summary).collect() +} + type ErrorInfo = ( String, Option, diff --git a/source/language_service/src/typing_simulation.rs b/source/language_service/src/typing_simulation.rs new file mode 100644 index 00000000000..58110fb8095 --- /dev/null +++ b/source/language_service/src/typing_simulation.rs @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Test-only hook for simulating a slow compilation. +//! +//! The update loop's coalescing behavior only manifests when compilation blocks the +//! host event loop long enough for input events to queue up behind it. Release builds +//! compile too fast to reproduce that reliably, so tests inject an artificial delay here. +//! +//! The delay must be a synchronous busy-wait, not a timer: yielding to the host event +//! loop would let queued events drain and defeat the entire purpose. Only the host knows +//! how to read a clock (`std::time` is unavailable on `wasm32-unknown-unknown`), so the +//! waiting itself is delegated to a callback registered by the WASM layer. + +use std::cell::RefCell; + +type BusyWaitCallback = Box; + +thread_local! { + static BUSY_WAIT_CB: RefCell> = const { RefCell::new(None) }; +} + +/// Registers the busy-wait callback. Should be called once during initialization. +pub fn set_busy_wait_callback(busy_wait: BusyWaitCallback) { + BUSY_WAIT_CB.with(|f| *f.borrow_mut() = Some(busy_wait)); +} + +/// Blocks the current thread for `ms` milliseconds. No-op if no callback is registered +/// or if `ms` is zero. +pub(crate) fn busy_wait(ms: u32) { + if ms == 0 { + return; + } + BUSY_WAIT_CB.with(|f| { + if let Some(cb) = f.borrow().as_ref() { + cb(ms); + } + }); +} diff --git a/source/npm/qsharp/src/compiler/compiler.ts b/source/npm/qsharp/src/compiler/compiler.ts index 1b8e9a80f66..1f34f2c47bf 100644 --- a/source/npm/qsharp/src/compiler/compiler.ts +++ b/source/npm/qsharp/src/compiler/compiler.ts @@ -16,6 +16,7 @@ import { ProjectType, } from "../../lib/web/qsc_wasm.js"; import { log } from "../log.js"; +import { createHostYield } from "../language-service/language-service.js"; import type { IServiceProxy, ServiceProtocol, @@ -138,9 +139,11 @@ export class Compiler implements ICompiler { fetchGithub: async () => "", findManifestDirectory: async () => null, }, + createHostYield(), ); languageService.update_document("code", 1, code, "qsharp"); - // Yield to let the language service update loop handle the update + // Yield to let the language service update loop pick up the update. Closing the + // loop below is what makes it stop absorbing and apply what it has. await Promise.resolve(); languageService.stop_update_loop(); await update_loop; diff --git a/source/npm/qsharp/src/language-service/language-service.ts b/source/npm/qsharp/src/language-service/language-service.ts index 802d2190273..12c907e5fb6 100644 --- a/source/npm/qsharp/src/language-service/language-service.ts +++ b/source/npm/qsharp/src/language-service/language-service.ts @@ -47,6 +47,26 @@ export type LanguageServiceEvent = | LanguageServiceDiagnosticEvent | LanguageServiceTestCallablesEvent; +/** + * A completion list, plus whether the caller should ask again as the user keeps typing. + * + * VS Code filters a complete list client-side and won't re-request it, so this has to be + * set whenever the list isn't the real answer for the requested document version. + */ +export type CompletionListResult = ICompletionList & { + isIncomplete?: boolean; +}; + +/** + * How long to wait for a completion request's document version to be compiled. + * + * This is a liveness backstop, not a tuning knob. A version that gets coalesced away is + * reported as superseded immediately, so hitting this timeout means the update is + * genuinely stuck, such as behind a slow project load. Expiring returns an incomplete + * list, so VS Code asks again rather than showing a stale one. + */ +const completionWaitTimeoutMs = 2000; + // These need to be async/promise results for when communicating across a WebWorker, however // for running the compiler in the same thread the result will be synchronous (a resolved promise). export interface ILanguageService { @@ -72,8 +92,9 @@ export interface ILanguageService { getCodeActions(documentUri: string, range: IRange): Promise; getCompletions( documentUri: string, + version: number, position: IPosition, - ): Promise; + ): Promise; getFormatChanges(documentUri: string): Promise; getHover( documentUri: string, @@ -121,6 +142,38 @@ export const qsharpGithubUriScheme = "qsharp-github-source"; export type ILanguageServiceWorker = ILanguageService & IServiceProxy; +/** + * Builds a function that yields to the host's macrotask queue. + * + * The update loop calls this to let the host deliver input events that piled up while + * a compilation was blocking the event loop, so they can be coalesced instead of + * processed one at a time. + * + * The primitive matters. `setImmediate` runs in Node's check phase, after the poll + * phase where the extension host reads queued IPC messages. `MessageChannel` posts an + * unclamped task that queues behind already-posted message events. A plain + * `setTimeout(0)` is neither: it runs in the timers phase, which can precede poll, and + * browsers clamp it to 4ms once nested. It is only a last resort. + */ +export function createHostYield(): () => Promise { + if (typeof setImmediate === "function") { + return () => new Promise((resolve) => setImmediate(resolve)); + } + + if (typeof MessageChannel === "function") { + const channel = new MessageChannel(); + const pending: (() => void)[] = []; + channel.port1.onmessage = () => pending.shift()?.(); + return () => + new Promise((resolve) => { + pending.push(resolve); + channel.port2.postMessage(null); + }); + } + + return () => new Promise((resolve) => setTimeout(resolve, 0)); +} + export class QSharpLanguageService implements ILanguageService { private languageService: LanguageService; private eventHandler = @@ -145,6 +198,7 @@ export class QSharpLanguageService implements ILanguageService { this.onDiagnostics.bind(this), this.onTestCallables.bind(this), host, + createHostYield(), ); } @@ -192,15 +246,26 @@ export class QSharpLanguageService implements ILanguageService { async getCompletions( documentUri: string, + version: number, position: IPosition, - ): Promise { - // Tiny delay to let the compilation catch up before we invoke - // the completion provider. - // This becomes important when the completion list is triggered - // during typing. If the last character typed is significant to - // the completion (e.g. in `Foo.` completions) - // it's critical that the completion provider "sees" this character. - await new Promise((resolve) => setTimeout(resolve, 50)); + ): Promise { + // The position was computed against this exact version of the document, so a later + // one is not an acceptable substitute: it may put the position somewhere else + // entirely. This matters most when the last character typed is significant to the + // completion, as in `Foo.`. + const status = await this.languageService.wait_for_document_version( + documentUri, + version, + completionWaitTimeoutMs, + ); + + if (status !== "ready") { + // Can't answer for this version, and answering for another would be wrong. + // Reporting the list as incomplete makes VS Code ask again on the next keystroke, + // and that request will be for a version that does get compiled. + return { items: [], isIncomplete: true }; + } + return this.languageService.get_completions(documentUri, position); } diff --git a/source/npm/qsharp/test/languageService.js b/source/npm/qsharp/test/languageService.js index a5dfbecf26f..47873d27244 100644 --- a/source/npm/qsharp/test/languageService.js +++ b/source/npm/qsharp/test/languageService.js @@ -30,6 +30,7 @@ test("devDiagnostics configuration works", async () => { try { // Collect diagnostics events as they are raised const diagnosticEvents = []; + let notify = () => {}; languageService.addEventListener("diagnostics", (event) => { diagnosticEvents.push({ uri: event.detail.uri, @@ -37,13 +38,23 @@ test("devDiagnostics configuration works", async () => { code: diag.code, })), }); + notify(); }); + // The update loop yields to the host event loop before applying updates, so how + // many turns this takes isn't something the test can predict. + const nextDiagnostics = () => + new Promise((resolve) => { + notify = resolve; + }); + // Enable dev diagnostics await languageService.updateConfiguration({ devDiagnostics: true, }); + const gotDiagnostics = nextDiagnostics(); + // Update a document await languageService.updateDocument( "test.qs", @@ -52,7 +63,7 @@ test("devDiagnostics configuration works", async () => { "qsharp", ); - await new Promise((resolve) => setTimeout(resolve, 0)); + await gotDiagnostics; // Should have received diagnostic events assert.deepEqual(diagnosticEvents, [ @@ -69,11 +80,13 @@ test("devDiagnostics configuration works", async () => { // Test disabling dev diagnostics diagnosticEvents.length = 0; + const gotClearedDiagnostics = nextDiagnostics(); + await languageService.updateConfiguration({ devDiagnostics: false, }); - await new Promise((resolve) => setTimeout(resolve, 0)); + await gotClearedDiagnostics; // Diagnostics should be cleared assert.deepEqual(diagnosticEvents, [ diff --git a/source/playground/src/main.tsx b/source/playground/src/main.tsx index e5ddc9b9ab9..659183b2e61 100644 --- a/source/playground/src/main.tsx +++ b/source/playground/src/main.tsx @@ -308,6 +308,7 @@ function registerMonacoLanguageServiceProviders( ) => { const completions = await languageService.getCompletions( model.uri.toString(), + model.getVersionId(), monacoPositionToLsPosition(position), ); return { diff --git a/source/vscode/package.json b/source/vscode/package.json index a0d32a20924..414852f0aa5 100644 --- a/source/vscode/package.json +++ b/source/vscode/package.json @@ -185,6 +185,16 @@ "hidden" ] }, + "Q#.dev.simulatedCompileDelayMs": { + "type": "number", + "default": 0, + "minimum": 0, + "maximum": 5000, + "description": "Block the extension host for this many milliseconds on every document compilation. This is for internal development and testing purposes, and will make the editor unresponsive while typing.", + "tags": [ + "hidden" + ] + }, "Q#.notifications.suppressUpdateNotifications": { "type": "boolean", "default": false, diff --git a/source/vscode/src/config.ts b/source/vscode/src/config.ts index 8fd4746dcc2..db106c8238c 100644 --- a/source/vscode/src/config.ts +++ b/source/vscode/src/config.ts @@ -47,6 +47,12 @@ export function getShowDevDiagnostics(): boolean { .get("dev.showDevDiagnostics", false); } +export function getSimulatedCompileDelayMs(): number { + return vscode.workspace + .getConfiguration("Q#") + .get("dev.simulatedCompileDelayMs", 0); +} + export function getUploadSupplementalData(): boolean { return vscode.workspace .getConfiguration("Q#") diff --git a/source/vscode/src/language-service/activate.ts b/source/vscode/src/language-service/activate.ts index a31e25e784b..860c8fcb4cd 100644 --- a/source/vscode/src/language-service/activate.ts +++ b/source/vscode/src/language-service/activate.ts @@ -14,7 +14,10 @@ import { openqasmLanguageId, qsharpLanguageId, } from "../common.js"; -import { getShowDevDiagnostics } from "../config.js"; +import { + getShowDevDiagnostics, + getSimulatedCompileDelayMs, +} from "../config.js"; import { fetchGithubRaw, findManifestDirectory, @@ -173,6 +176,7 @@ async function loadLanguageService( const wasmUri = vscode.Uri.joinPath(baseUri, "./wasm/qsc_wasm_bg.wasm"); const wasmBytes = await vscode.workspace.fs.readFile(wasmUri); await loadWasmModule(wasmBytes); + const languageService = await getLanguageService({ findManifestDirectory, readFile, @@ -180,6 +184,7 @@ async function loadLanguageService( resolvePath: async (a, b) => resolvePath(a, b), fetchGithub: fetchGithubRaw, }); + await updateLanguageServiceConfiguration(languageService); const end = performance.now(); sendTelemetryEvent( @@ -307,7 +312,10 @@ function registerConfigurationChangeHandlers( languageService: ILanguageService, ) { return vscode.workspace.onDidChangeConfiguration((event) => { - if (event.affectsConfiguration("Q#.dev.showDevDiagnostics")) { + if ( + event.affectsConfiguration("Q#.dev.showDevDiagnostics") || + event.affectsConfiguration("Q#.dev.simulatedCompileDelayMs") + ) { updateLanguageServiceConfiguration(languageService); } }); @@ -317,12 +325,14 @@ async function updateLanguageServiceConfiguration( languageService: ILanguageService, ) { const showDevDiagnostics = getShowDevDiagnostics(); + const simulatedCompileDelayMs = getSimulatedCompileDelayMs(); log.debug("Show dev diagnostics set to: " + showDevDiagnostics); // Update all configuration settings languageService.updateConfiguration({ devDiagnostics: showDevDiagnostics, + simulatedCompileDelayMs, lints: [{ lint: "needlessOperation", level: "warn" }], }); } diff --git a/source/vscode/src/language-service/completion.ts b/source/vscode/src/language-service/completion.ts index 7a986b9be88..5e6bd9aaa13 100644 --- a/source/vscode/src/language-service/completion.ts +++ b/source/vscode/src/language-service/completion.ts @@ -51,6 +51,7 @@ class QSharpCompletionItemProvider implements vscode.CompletionItemProvider { const start = performance.now(); const completions = await this.languageService.getCompletions( document.uri.toString(), + document.version, position, ); const end = performance.now(); @@ -132,6 +133,11 @@ class QSharpCompletionItemProvider implements vscode.CompletionItemProvider { results = results.concat(this.openqasm_samples); } - return results; + // Preserves `isIncomplete`, which is what makes VS Code ask again on the next + // keystroke instead of filtering this list client-side. + return new vscode.CompletionList( + results, + completions.isIncomplete === true, + ); } } diff --git a/source/vscode/test/suites/language-service/completion-retrigger.test.ts b/source/vscode/test/suites/language-service/completion-retrigger.test.ts new file mode 100644 index 00000000000..3b147592e10 --- /dev/null +++ b/source/vscode/test/suites/language-service/completion-retrigger.test.ts @@ -0,0 +1,182 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { assert } from "chai"; +import * as vscode from "vscode"; +import { + activateExtension, + openDocumentAndWaitForProcessing, +} from "../extensionUtils"; + +/** + * Measures whether VS Code re-invokes a completion provider as the user keeps typing + * after an earlier suggest request. + * + * This decides how the language service should handle a completion request whose + * document version gets coalesced away by the update loop. Such a request cannot be + * answered correctly, since its position refers to text the user has moved past, so it + * has to return nothing. That is only safe if VS Code comes back and asks again. + * + * VS Code re-queries a provider on subsequent keystrokes only when that provider + * returned `isIncomplete: true`; a complete list is filtered client-side instead. + * Both cases are measured below, because that difference is exactly what determines + * whether returning an empty incomplete list is a sufficient mitigation. + * + * The recorder is registered as a *second* completion provider. VS Code queries every + * registered provider within a suggest session and tracks incompleteness per provider, + * so this observes the real behavior without any production instrumentation. + * + * This must drive the editor with the `type` command rather than `editor.edit()` or + * `vscode.executeCompletionItemProvider`. Only real typing runs the suggest widget's + * trigger/re-trigger logic, which is the thing being measured. + */ +suite("Completion re-trigger behavior", function suite() { + const workspaceFolder = + vscode.workspace.workspaceFolders && vscode.workspace.workspaceFolders[0]; + assert(workspaceFolder, "Expecting an open folder"); + + const noErrorsQs = vscode.Uri.joinPath(workspaceFolder.uri, "no-errors.qs"); + + // Long enough that each keystroke lands while a compile is blocking the extension + // host, which is the condition that causes updates to coalesce in the first place. + const simulatedCompileDelayMs = 100; + + // Roughly a fast typist, and deliberately shorter than the simulated compile. + const keystrokeIntervalMs = 40; + + type Invocation = { + version: number; + triggerKind: vscode.CompletionTriggerKind; + triggerCharacter: string | undefined; + }; + + let invocations: Invocation[] = []; + let recorder: vscode.Disposable | undefined; + + this.beforeAll(async () => { + await activateExtension(); + + await vscode.workspace + .getConfiguration("Q#") + .update( + "dev.simulatedCompileDelayMs", + simulatedCompileDelayMs, + vscode.ConfigurationTarget.Global, + ); + }); + + this.afterAll(async () => { + await vscode.workspace + .getConfiguration("Q#") + .update( + "dev.simulatedCompileDelayMs", + undefined, + vscode.ConfigurationTarget.Global, + ); + }); + + this.afterEach(async () => { + recorder?.dispose(); + recorder = undefined; + await vscode.commands.executeCommand( + "workbench.action.revertAndCloseActiveEditor", + ); + }); + + /** + * Registers the recording provider, types `Std.Di` one character at a time, and + * returns the invocations that happened strictly after `.` triggered suggest. + */ + async function typeAndRecord(isIncomplete: boolean) { + invocations = []; + recorder = vscode.languages.registerCompletionItemProvider( + "qsharp", + { + provideCompletionItems(document, _position, _token, context) { + invocations.push({ + version: document.version, + triggerKind: context.triggerKind, + triggerCharacter: context.triggerCharacter, + }); + // Returns an item rather than an empty list, because VS Code discards an + // empty result and closes the session, which would suppress re-triggering + // for reasons unrelated to what is being measured here. + return new vscode.CompletionList( + [new vscode.CompletionItem("ZzProbeItem")], + isIncomplete, + ); + }, + }, + ".", + ); + + const doc = await openDocumentAndWaitForProcessing(noErrorsQs); + const editor = await vscode.window.showTextDocument(doc); + + // Land the cursor at the end of the `let foo = "hello!";` line so typed text + // forms a fresh expression rather than editing existing code. + const insertAt = new vscode.Position(3, 26); + editor.selection = new vscode.Selection(insertAt, insertAt); + + let versionAtDot = 0; + for (const ch of ["S", "t", "d", ".", "D", "i"]) { + await vscode.commands.executeCommand("type", { text: ch }); + if (ch === ".") { + versionAtDot = doc.version; + } + await new Promise((resolve) => setTimeout(resolve, keystrokeIntervalMs)); + } + + // Give any trailing re-triggers a chance to land. + await new Promise((resolve) => setTimeout(resolve, 1500)); + + // Keyed on the document version rather than the trigger kind: when a suggest + // session is already open, VS Code may re-query an incomplete provider instead of + // starting a fresh trigger-character session, so the kind isn't dependable. + const afterDot = invocations.filter((i) => i.version > versionAtDot); + + console.log( + `qsharp-tests: isIncomplete=${isIncomplete} versionAtDot=${versionAtDot} finalDocVersion=${doc.version}\n` + + `qsharp-tests: all invocations: ${invocations + .map( + (i) => + `v${i.version}/${vscode.CompletionTriggerKind[i.triggerKind]}${i.triggerCharacter ? `('${i.triggerCharacter}')` : ""}`, + ) + .join(", ")}\n` + + `qsharp-tests: invocations after dot: ${afterDot.length}`, + ); + + assert.isNotEmpty( + invocations, + "expected the completion provider to be invoked while typing", + ); + assert.isAbove( + doc.version, + versionAtDot, + "expected more edits after the `.` keystroke", + ); + + return { versionAtDot, afterDot, doc }; + } + + test("a complete list is NOT re-requested on later keystrokes", async () => { + const { afterDot } = await typeAndRecord(false); + + assert.isEmpty( + afterDot, + "expected VS Code to filter a complete list client-side rather than re-requesting", + ); + }); + + test("an incomplete list IS re-requested on later keystrokes", async () => { + const { afterDot } = await typeAndRecord(true); + + assert.isNotEmpty( + afterDot, + "VS Code did not re-invoke the provider after an incomplete list. Returning an " + + "empty incomplete list is therefore NOT a sufficient mitigation for a " + + "coalesced-away completion request, and the update loop must avoid coalescing " + + "past a version a completion request is waiting on (plan Phase 3b).", + ); + }); +}); diff --git a/source/vscode/test/suites/language-service/index.browser.ts b/source/vscode/test/suites/language-service/index.browser.ts index a526e9e751a..41fddd7cb63 100644 --- a/source/vscode/test/suites/language-service/index.browser.ts +++ b/source/vscode/test/suites/language-service/index.browser.ts @@ -12,6 +12,8 @@ export function run(): Promise { // paths here since ESBuild needs these modules to be // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports + require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports + require("./update-coalescing.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); diff --git a/source/vscode/test/suites/language-service/index.node.ts b/source/vscode/test/suites/language-service/index.node.ts index a3e85a4154f..82d74ad27d5 100644 --- a/source/vscode/test/suites/language-service/index.node.ts +++ b/source/vscode/test/suites/language-service/index.node.ts @@ -17,6 +17,8 @@ export async function run(): Promise { // paths here since ESBuild needs these modules to be // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports + require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports + require("./update-coalescing.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); diff --git a/source/vscode/test/suites/language-service/update-coalescing.test.ts b/source/vscode/test/suites/language-service/update-coalescing.test.ts new file mode 100644 index 00000000000..2b05a159b10 --- /dev/null +++ b/source/vscode/test/suites/language-service/update-coalescing.test.ts @@ -0,0 +1,119 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { assert } from "chai"; +import * as vscode from "vscode"; +import { + activateExtension, + openDocumentAndWaitForProcessing, + waitForCondition, + TEST_TIMEOUT_MS, +} from "../extensionUtils"; + +/** + * Verifies that edits arriving while a compilation is in flight are coalesced into far + * fewer compilations than there were edits. + * + * Compilation blocks the extension host, so the edits have to originate on the other + * side of that boundary to pile up the way real keystrokes do. They are therefore issued + * without awaiting, letting the editor apply them and deliver the change notifications + * while the host is busy. + * + * Which version was compiled is read from the dev status diagnostic, whose message + * carries `version=N`. That diagnostic is the only place a document version is + * observable from outside the language service. + */ +suite("Update coalescing", function suite() { + const workspaceFolder = + vscode.workspace.workspaceFolders && vscode.workspace.workspaceFolders[0]; + assert(workspaceFolder, "Expecting an open folder"); + + const noErrorsQs = vscode.Uri.joinPath(workspaceFolder.uri, "no-errors.qs"); + + // Long enough that every edit lands while a compilation is blocking the host. + const simulatedCompileDelayMs = 100; + const editCount = 20; + + this.beforeAll(async () => { + await activateExtension(); + await vscode.workspace + .getConfiguration("Q#") + .update( + "dev.simulatedCompileDelayMs", + simulatedCompileDelayMs, + vscode.ConfigurationTarget.Global, + ); + }); + + this.afterAll(async () => { + await vscode.workspace + .getConfiguration("Q#") + .update( + "dev.simulatedCompileDelayMs", + undefined, + vscode.ConfigurationTarget.Global, + ); + await vscode.commands.executeCommand( + "workbench.action.revertAndCloseActiveEditor", + ); + }); + + test("many rapid edits produce far fewer compilations", async () => { + const doc = await openDocumentAndWaitForProcessing(noErrorsQs); + const editor = await vscode.window.showTextDocument(doc); + + const compiledVersions: number[] = []; + const recorder = vscode.languages.onDidChangeDiagnostics((event) => { + if (!event.uris.some((u) => u.toString() === doc.uri.toString())) { + return; + } + for (const diagnostic of vscode.languages.getDiagnostics(doc.uri)) { + const match = /version=(\d+)/.exec(diagnostic.message); + if (match) { + const version = Number(match[1]); + if (compiledVersions.at(-1) !== version) { + compiledVersions.push(version); + } + } + } + }); + + try { + const insertAt = new vscode.Position(3, 26); + editor.selection = new vscode.Selection(insertAt, insertAt); + + // Deliberately not awaited individually, so they queue up in the editor rather + // than being serialized behind each compilation. + const typed: Thenable[] = []; + for (let i = 0; i < editCount; i++) { + typed.push(vscode.commands.executeCommand("type", { text: "a" })); + } + await Promise.all(typed); + + const finalVersion = doc.version; + + await waitForCondition( + () => compiledVersions.includes(finalVersion), + vscode.languages.onDidChangeDiagnostics, + TEST_TIMEOUT_MS, + `Final document version ${finalVersion} was never compiled. ` + + `Compiled versions: ${compiledVersions.join(", ")}`, + ); + + console.log( + `qsharp-tests: ${editCount} edits produced document version ${finalVersion}; ` + + `compiled versions: ${compiledVersions.join(", ")}`, + ); + + // The exact count depends on machine speed, so this only asserts that coalescing + // happened at all. Without it there would be one compilation per edit. + assert.isBelow( + compiledVersions.length, + editCount, + "expected fewer compilations than edits", + ); + } finally { + recorder.dispose(); + } + }); +}); diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index da05212cad1..b0be0c44441 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -13,6 +13,7 @@ use qsc::{ target::Profile, }; use qsc_project::Manifest; +use qsls::VersionWait; use qsls::protocol::{DiagnosticUpdate, TestCallable, TestCallables}; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; @@ -20,6 +21,12 @@ use std::str::FromStr; use wasm_bindgen::prelude::*; use wasm_bindgen_futures::future_to_promise; +#[wasm_bindgen] +extern "C" { + #[wasm_bindgen(js_name = setTimeout)] + fn set_timeout(closure: &js_sys::Function, ms: i32); +} + #[wasm_bindgen] pub struct LanguageService(qsls::LanguageService); @@ -28,14 +35,25 @@ impl LanguageService { #[wasm_bindgen(constructor)] #[allow(clippy::new_without_default)] // wasm-bindgen requires constructor to be explicitly defined pub fn new() -> Self { + // Only ever does anything when a test opts in via `simulatedCompileDelayMs`. + qsls::typing_simulation::set_busy_wait_callback(Box::new(|ms: u32| { + let end = js_sys::Date::now() + f64::from(ms); + while js_sys::Date::now() < end { + std::hint::spin_loop(); + } + })); LanguageService(qsls::LanguageService::new(Encoding::Utf16)) } + /// `yield_to_host` must return a promise that resolves on a later iteration of the + /// host's event loop, giving it a chance to deliver queued input events. The choice + /// of primitive is left to JavaScript because it differs by host. pub fn start_update_loop( &mut self, diagnostics_callback: &DiagnosticsCallback, test_callables_callback: &TestCallableCallback, host: ProjectHost, + yield_to_host: &js_sys::Function, ) -> js_sys::Promise { let diagnostics_callback = diagnostics_callback .dyn_ref::() @@ -94,12 +112,23 @@ impl LanguageService { ) .expect("callback should succeed"); }; - let mut worker = + let mut handler = self.0 .create_update_handler(diagnostics_callback, test_callables_callback, host); + let yield_to_host = yield_to_host.clone(); + let yield_to_host = move || { + let promise = yield_to_host + .call0(&JsValue::NULL) + .expect("yield_to_host should not throw"); + let future = wasm_bindgen_futures::JsFuture::from(js_sys::Promise::from(promise)); + async move { + let _ = future.await; + } + }; + future_to_promise(async move { - worker.run().await; + handler.run(yield_to_host).await; Ok(JsValue::undefined()) }) } @@ -125,6 +154,7 @@ impl LanguageService { .map(|features| features.iter().collect::()), lints_config: config.lints, dev_diagnostics: config.devDiagnostics, + simulated_compile_delay_ms: config.simulatedCompileDelayMs, }); } @@ -185,10 +215,47 @@ impl LanguageService { .collect() } + /// Resolves once the compilation state reflects exactly `version` of `uri`, or the + /// document moves past it, or `timeout_ms` elapses. Resolves to `"ready"`, + /// `"superseded"` or `"timeout"`. + /// + /// The timeout is a liveness backstop rather than a tuning knob. A version that gets + /// coalesced away reports `"superseded"` immediately, so reaching the timeout means + /// the update is genuinely stuck, for instance behind a slow project load. + pub fn wait_for_document_version( + &self, + uri: &str, + version: u32, + timeout_ms: i32, + ) -> js_sys::Promise { + let wait = self.0.wait_for_document_version(uri, version); + let uri = uri.to_string(); + + future_to_promise(async move { + let timeout = + wasm_bindgen_futures::JsFuture::from(js_sys::Promise::new(&mut |resolve, _| { + set_timeout(&resolve, timeout_ms); + })); + futures_util::pin_mut!(wait, timeout); + + let result = match futures_util::future::select(wait, timeout).await { + futures_util::future::Either::Left((VersionWait::Ready, _)) => "ready", + futures_util::future::Either::Left((VersionWait::Superseded, _)) => "superseded", + futures_util::future::Either::Right(_) => { + log::debug!( + "timed out after {timeout_ms}ms waiting for {uri} version {version}" + ); + "timeout" + } + }; + Ok(JsValue::from_str(result)) + }) + } + pub fn get_completions(&self, uri: &str, position: IPosition) -> ICompletionList { let position: Position = position.into(); let completion_list = self.0.get_completions(uri, position.into()); - CompletionList { + let result: ICompletionList = CompletionList { items: completion_list .items .into_iter() @@ -220,7 +287,8 @@ impl LanguageService { }) .collect(), } - .into() + .into(); + result } pub fn get_definition(&self, uri: &str, position: IPosition) -> Option { @@ -382,6 +450,7 @@ serializable_type! { pub languageFeatures: Option>, pub lints: Option>, pub devDiagnostics: Option, + pub simulatedCompileDelayMs: Option, }, r#"export interface IWorkspaceConfiguration { targetProfile?: TargetProfile; @@ -389,6 +458,7 @@ serializable_type! { languageFeatures?: LanguageFeatures[]; lints?: ({ lint: string; level: string } | { group: string; level: string })[]; devDiagnostics?: boolean; + simulatedCompileDelayMs?: number; }"#, IWorkspaceConfiguration } From 3cd36192118cd33bbfb6f25f55f51fa53dafd6a5 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 30 Jul 2026 16:45:01 -0700 Subject: [PATCH 02/13] Apply suggestions from Copilot code review One looks like a typo. The other is a more interesting shutdown cleanup fix. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- source/language_service/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index 41fdf508059..05340a06b54 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -212,7 +212,7 @@ impl LanguageService { &self, uri: &str, version: u32, - ) -> impl std::future::Future + 'static + use<> { + ) -> impl std::future::Future + 'static { let state = self.state.clone(); let waiters = self.version_waiters.clone(); let uri = uri.to_string(); @@ -444,6 +444,9 @@ impl UpdateHandler<'_> { self.apply(updates).await; } + + // Drop any waiters so `wait_for_document_version` callers can observe shutdown. + self.version_waiters.borrow_mut().clear(); } /// Convenience method to apply *only* the pending updates From 8d4c4c57f9ca3bf518347533728e7e2ca6564971 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 30 Jul 2026 18:17:36 -0700 Subject: [PATCH 03/13] Make waiter-shutdown interaction more explicit --- source/language_service/src/lib.rs | 75 +++++++++++++++++++++------- source/language_service/src/tests.rs | 39 ++++++++++++++- 2 files changed, 95 insertions(+), 19 deletions(-) diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index 05340a06b54..ecf0529e546 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -37,7 +37,11 @@ use qsc::{ }; use qsc_project::JSProjectHost; use state::{CompilationState, CompilationStateUpdater}; -use std::{cell::RefCell, fmt::Debug, rc::Rc}; +use std::{ + cell::{Cell, RefCell}, + fmt::Debug, + rc::Rc, +}; pub struct LanguageService { /// All [`Position`]s and [`Range`]s will be mapped using this encoding. @@ -50,9 +54,8 @@ pub struct LanguageService { state: Rc>, /// Channel for compilation state update messages coming from the client. state_updater: Option>, - /// Callers parked in [`LanguageService::wait_for_document_version`]. Woken after - /// every applied batch of updates, at which point they re-inspect the state. - version_waiters: Rc>>>, + /// Callers parked in [`LanguageService::wait_for_document_version`]. + version_waiters: Rc, } /// The outcome of waiting for a specific version of a document to be compiled. @@ -65,6 +68,44 @@ pub enum VersionWait { Superseded, } +/// Callers parked in [`LanguageService::wait_for_document_version`], waiting for the +/// next batch of updates to be applied so they can re-inspect the compilation state. +#[derive(Default)] +struct VersionWaiters { + parked: RefCell>>, + is_shut_down: Cell, +} + +impl VersionWaiters { + /// Returns `None` once the update handler has stopped, since no further version + /// will ever be compiled and nothing would arrive to wake the caller. + fn park(&self) -> Option> { + if self.is_shut_down.get() { + return None; + } + let (send, recv) = oneshot::channel(); + self.parked.borrow_mut().push(send); + Some(recv) + } + + fn wake_all(&self) { + // Taken rather than drained in place: a woken caller may immediately re-park, + // and that would re-enter the borrow if it were still held here. + let parked = std::mem::take(&mut *self.parked.borrow_mut()); + for waiter in parked { + let _ = waiter.send(()); + } + } + + /// Releases everyone currently parked and rejects any that arrive later. Dropping + /// the senders is what lets parked callers observe the shutdown. + fn shut_down(&self) { + self.is_shut_down.set(true); + let parked = std::mem::take(&mut *self.parked.borrow_mut()); + drop(parked); + } +} + impl LanguageService { #[must_use] pub fn new(position_encoding: Encoding) -> Self { @@ -207,12 +248,13 @@ impl LanguageService { /// point somewhere else entirely, or not exist at all. /// /// The returned future is independent of `&self` so that callers can hold it across - /// await points without keeping the language service borrowed. + /// await points without keeping the language service borrowed. The `use<>` bound is + /// precise capturing, which is what keeps the input lifetimes out of the future. pub fn wait_for_document_version( &self, uri: &str, version: u32, - ) -> impl std::future::Future + 'static { + ) -> impl std::future::Future + 'static + use<> { let state = self.state.clone(); let waiters = self.version_waiters.clone(); let uri = uri.to_string(); @@ -227,16 +269,16 @@ impl LanguageService { Some(current) if current == version => return VersionWait::Ready, Some(current) if current > version => return VersionWait::Superseded, // Either behind, or the document hasn't been processed at all yet. - _ => { - let (send, recv) = oneshot::channel(); - waiters.borrow_mut().push(send); - recv - } + _ => match waiters.park() { + Some(receiver) => receiver, + // The update handler is gone, so the version will never arrive. + None => return VersionWait::Superseded, + }, } }; if receiver.await.is_err() { - // The update handler is gone, so the version will never arrive. + // The update handler shut down while we were parked. return VersionWait::Superseded; } } @@ -396,7 +438,7 @@ impl LanguageService { pub struct UpdateHandler<'a> { updater: CompilationStateUpdater<'a>, recv: UnboundedReceiver, - version_waiters: Rc>>>, + version_waiters: Rc, } /// Caps how many times [`UpdateHandler::run`] will give the host a turn to deliver more @@ -445,8 +487,7 @@ impl UpdateHandler<'_> { self.apply(updates).await; } - // Drop any waiters so `wait_for_document_version` callers can observe shutdown. - self.version_waiters.borrow_mut().clear(); + self.version_waiters.shut_down(); } /// Convenience method to apply *only* the pending updates @@ -500,9 +541,7 @@ impl UpdateHandler<'_> { trace!("end applying updates"); // Let anyone waiting on a particular version re-check where the state landed. - for waiter in self.version_waiters.borrow_mut().drain(..) { - let _ = waiter.send(()); - } + self.version_waiters.wake_all(); } } diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 26e6ade530a..8b572265b6a 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use crate::{ - Encoding, LanguageService, Update, UpdateHandler, + Encoding, LanguageService, Update, UpdateHandler, VersionWait, protocol::{DiagnosticUpdate, ErrorKind, TestCallables, WorkspaceConfigurationUpdate}, push_update, }; @@ -461,6 +461,43 @@ async fn run_applies_updates_to_distinct_documents() { assert_eq!(applied, ["bar.qs", "foo.qs"]); } +/// A caller parked on a version that can no longer arrive has to be released when the +/// update handler stops, rather than waiting forever. +#[tokio::test] +async fn wait_for_document_version_released_when_handler_stops() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + // The wait doesn't borrow `ls`, so updates can still be stopped while it's alive. + let wait = ls.wait_for_document_version("foo.qs", 1); + ls.stop_updates(); + + // `join` polls the wait first, so it is parked by the time the handler shuts down. + let (result, ()) = futures::future::join(wait, worker.run(|| std::future::ready(()))).await; + + assert_eq!(result, VersionWait::Superseded); +} + +/// Once the handler has stopped there is nothing left to wake a new caller, so parking +/// one would hang it until its timeout. +#[tokio::test] +async fn wait_for_document_version_returns_immediately_after_handler_stops() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.stop_updates(); + worker.run(|| std::future::ready(())).await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWait::Superseded + ); +} + #[test] fn push_update_merges_consecutive_updates_to_same_document() { let mut updates = Vec::new(); From e41c8b1ccedafe7faebaa85aaba9ed078b9d6d00 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 30 Jul 2026 18:19:45 -0700 Subject: [PATCH 04/13] Drop stale comment --- .../test/suites/language-service/completion-retrigger.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/vscode/test/suites/language-service/completion-retrigger.test.ts b/source/vscode/test/suites/language-service/completion-retrigger.test.ts index 3b147592e10..de89c9ad1d5 100644 --- a/source/vscode/test/suites/language-service/completion-retrigger.test.ts +++ b/source/vscode/test/suites/language-service/completion-retrigger.test.ts @@ -176,7 +176,7 @@ suite("Completion re-trigger behavior", function suite() { "VS Code did not re-invoke the provider after an incomplete list. Returning an " + "empty incomplete list is therefore NOT a sufficient mitigation for a " + "coalesced-away completion request, and the update loop must avoid coalescing " + - "past a version a completion request is waiting on (plan Phase 3b).", + "past a version a completion request is waiting on.", ); }); }); From b9c148da62fe2f11b2112c72cad1d46f29627435 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 31 Jul 2026 09:47:50 -0700 Subject: [PATCH 05/13] Drop test hook supporting simulated compilation delays --- source/language_service/src/lib.rs | 1 - source/language_service/src/protocol.rs | 3 - source/language_service/src/state.rs | 11 -- .../language_service/src/typing_simulation.rs | 39 ------ source/vscode/package.json | 10 -- source/vscode/src/config.ts | 6 - .../vscode/src/language-service/activate.ts | 12 +- .../completion-retrigger.test.ts | 24 +--- .../suites/language-service/index.browser.ts | 1 - .../suites/language-service/index.node.ts | 1 - .../update-coalescing.test.ts | 119 ------------------ source/wasm/src/language_service.rs | 10 -- 12 files changed, 3 insertions(+), 234 deletions(-) delete mode 100644 source/language_service/src/typing_simulation.rs delete mode 100644 source/vscode/test/suites/language-service/update-coalescing.test.ts diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index ecf0529e546..3f68ce56abd 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -20,7 +20,6 @@ mod state; mod test_utils; #[cfg(test)] mod tests; -pub mod typing_simulation; use compilation::Compilation; use futures::channel::mpsc::{TryRecvError, UnboundedReceiver, UnboundedSender, unbounded}; diff --git a/source/language_service/src/protocol.rs b/source/language_service/src/protocol.rs index e727bddc562..3b524425c9a 100644 --- a/source/language_service/src/protocol.rs +++ b/source/language_service/src/protocol.rs @@ -17,9 +17,6 @@ pub struct WorkspaceConfigurationUpdate { pub language_features: Option, pub lints_config: Option>, pub dev_diagnostics: Option, - /// Test-only. Artificially slows down each document update to make the update - /// loop's coalescing behavior observable in a release build. - pub simulated_compile_delay_ms: Option, } #[derive(Clone, Debug, Diagnostic, Error)] diff --git a/source/language_service/src/state.rs b/source/language_service/src/state.rs index bec0a513b07..41ed2bb609e 100644 --- a/source/language_service/src/state.rs +++ b/source/language_service/src/state.rs @@ -70,8 +70,6 @@ struct Configuration { pub lints_config: Vec, /// Enables non-user-facing developer diagnostics. pub dev_diagnostics: bool, - /// Test-only. See [`crate::typing_simulation`]. - pub simulated_compile_delay_ms: u32, } impl Default for Configuration { @@ -82,7 +80,6 @@ impl Default for Configuration { language_features: LanguageFeatures::default(), lints_config: Vec::default(), dev_diagnostics: false, - simulated_compile_delay_ms: 0, } } } @@ -194,8 +191,6 @@ impl<'a> CompilationStateUpdater<'a> { self.insert_buffer_aware_compilation(project); - crate::typing_simulation::busy_wait(self.configuration.simulated_compile_delay_ms); - self.publish_diagnostics_and_test_callables(); } @@ -590,11 +585,6 @@ impl<'a> CompilationStateUpdater<'a> { self.configuration.dev_diagnostics = dev_diagnostics; } - // Doesn't affect compilation output, so never triggers a recompile. - if let Some(delay_ms) = configuration.simulated_compile_delay_ms { - self.configuration.simulated_compile_delay_ms = delay_ms; - } - // Possible optimization: some projects will have overrides for these configurations, // so workspace updates won't impact them. We could exclude those projects // from recompilation, but we don't right now. @@ -810,6 +800,5 @@ fn merge_configurations( .unwrap_or(workspace_scope.language_features), lints_config: merged_lints, dev_diagnostics: workspace_scope.dev_diagnostics, - simulated_compile_delay_ms: workspace_scope.simulated_compile_delay_ms, } } diff --git a/source/language_service/src/typing_simulation.rs b/source/language_service/src/typing_simulation.rs deleted file mode 100644 index 58110fb8095..00000000000 --- a/source/language_service/src/typing_simulation.rs +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -//! Test-only hook for simulating a slow compilation. -//! -//! The update loop's coalescing behavior only manifests when compilation blocks the -//! host event loop long enough for input events to queue up behind it. Release builds -//! compile too fast to reproduce that reliably, so tests inject an artificial delay here. -//! -//! The delay must be a synchronous busy-wait, not a timer: yielding to the host event -//! loop would let queued events drain and defeat the entire purpose. Only the host knows -//! how to read a clock (`std::time` is unavailable on `wasm32-unknown-unknown`), so the -//! waiting itself is delegated to a callback registered by the WASM layer. - -use std::cell::RefCell; - -type BusyWaitCallback = Box; - -thread_local! { - static BUSY_WAIT_CB: RefCell> = const { RefCell::new(None) }; -} - -/// Registers the busy-wait callback. Should be called once during initialization. -pub fn set_busy_wait_callback(busy_wait: BusyWaitCallback) { - BUSY_WAIT_CB.with(|f| *f.borrow_mut() = Some(busy_wait)); -} - -/// Blocks the current thread for `ms` milliseconds. No-op if no callback is registered -/// or if `ms` is zero. -pub(crate) fn busy_wait(ms: u32) { - if ms == 0 { - return; - } - BUSY_WAIT_CB.with(|f| { - if let Some(cb) = f.borrow().as_ref() { - cb(ms); - } - }); -} diff --git a/source/vscode/package.json b/source/vscode/package.json index 414852f0aa5..a0d32a20924 100644 --- a/source/vscode/package.json +++ b/source/vscode/package.json @@ -185,16 +185,6 @@ "hidden" ] }, - "Q#.dev.simulatedCompileDelayMs": { - "type": "number", - "default": 0, - "minimum": 0, - "maximum": 5000, - "description": "Block the extension host for this many milliseconds on every document compilation. This is for internal development and testing purposes, and will make the editor unresponsive while typing.", - "tags": [ - "hidden" - ] - }, "Q#.notifications.suppressUpdateNotifications": { "type": "boolean", "default": false, diff --git a/source/vscode/src/config.ts b/source/vscode/src/config.ts index db106c8238c..8fd4746dcc2 100644 --- a/source/vscode/src/config.ts +++ b/source/vscode/src/config.ts @@ -47,12 +47,6 @@ export function getShowDevDiagnostics(): boolean { .get("dev.showDevDiagnostics", false); } -export function getSimulatedCompileDelayMs(): number { - return vscode.workspace - .getConfiguration("Q#") - .get("dev.simulatedCompileDelayMs", 0); -} - export function getUploadSupplementalData(): boolean { return vscode.workspace .getConfiguration("Q#") diff --git a/source/vscode/src/language-service/activate.ts b/source/vscode/src/language-service/activate.ts index 860c8fcb4cd..cca7c5122a7 100644 --- a/source/vscode/src/language-service/activate.ts +++ b/source/vscode/src/language-service/activate.ts @@ -14,10 +14,7 @@ import { openqasmLanguageId, qsharpLanguageId, } from "../common.js"; -import { - getShowDevDiagnostics, - getSimulatedCompileDelayMs, -} from "../config.js"; +import { getShowDevDiagnostics } from "../config.js"; import { fetchGithubRaw, findManifestDirectory, @@ -312,10 +309,7 @@ function registerConfigurationChangeHandlers( languageService: ILanguageService, ) { return vscode.workspace.onDidChangeConfiguration((event) => { - if ( - event.affectsConfiguration("Q#.dev.showDevDiagnostics") || - event.affectsConfiguration("Q#.dev.simulatedCompileDelayMs") - ) { + if (event.affectsConfiguration("Q#.dev.showDevDiagnostics")) { updateLanguageServiceConfiguration(languageService); } }); @@ -325,14 +319,12 @@ async function updateLanguageServiceConfiguration( languageService: ILanguageService, ) { const showDevDiagnostics = getShowDevDiagnostics(); - const simulatedCompileDelayMs = getSimulatedCompileDelayMs(); log.debug("Show dev diagnostics set to: " + showDevDiagnostics); // Update all configuration settings languageService.updateConfiguration({ devDiagnostics: showDevDiagnostics, - simulatedCompileDelayMs, lints: [{ lint: "needlessOperation", level: "warn" }], }); } diff --git a/source/vscode/test/suites/language-service/completion-retrigger.test.ts b/source/vscode/test/suites/language-service/completion-retrigger.test.ts index de89c9ad1d5..41fb091caf4 100644 --- a/source/vscode/test/suites/language-service/completion-retrigger.test.ts +++ b/source/vscode/test/suites/language-service/completion-retrigger.test.ts @@ -37,11 +37,7 @@ suite("Completion re-trigger behavior", function suite() { const noErrorsQs = vscode.Uri.joinPath(workspaceFolder.uri, "no-errors.qs"); - // Long enough that each keystroke lands while a compile is blocking the extension - // host, which is the condition that causes updates to coalesce in the first place. - const simulatedCompileDelayMs = 100; - - // Roughly a fast typist, and deliberately shorter than the simulated compile. + // Roughly a fast typist. const keystrokeIntervalMs = 40; type Invocation = { @@ -55,24 +51,6 @@ suite("Completion re-trigger behavior", function suite() { this.beforeAll(async () => { await activateExtension(); - - await vscode.workspace - .getConfiguration("Q#") - .update( - "dev.simulatedCompileDelayMs", - simulatedCompileDelayMs, - vscode.ConfigurationTarget.Global, - ); - }); - - this.afterAll(async () => { - await vscode.workspace - .getConfiguration("Q#") - .update( - "dev.simulatedCompileDelayMs", - undefined, - vscode.ConfigurationTarget.Global, - ); }); this.afterEach(async () => { diff --git a/source/vscode/test/suites/language-service/index.browser.ts b/source/vscode/test/suites/language-service/index.browser.ts index 41fddd7cb63..e494c8e9460 100644 --- a/source/vscode/test/suites/language-service/index.browser.ts +++ b/source/vscode/test/suites/language-service/index.browser.ts @@ -13,7 +13,6 @@ export function run(): Promise { // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports - require("./update-coalescing.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); diff --git a/source/vscode/test/suites/language-service/index.node.ts b/source/vscode/test/suites/language-service/index.node.ts index 82d74ad27d5..6dd20140fa2 100644 --- a/source/vscode/test/suites/language-service/index.node.ts +++ b/source/vscode/test/suites/language-service/index.node.ts @@ -18,7 +18,6 @@ export async function run(): Promise { // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports - require("./update-coalescing.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); diff --git a/source/vscode/test/suites/language-service/update-coalescing.test.ts b/source/vscode/test/suites/language-service/update-coalescing.test.ts deleted file mode 100644 index 2b05a159b10..00000000000 --- a/source/vscode/test/suites/language-service/update-coalescing.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -import { assert } from "chai"; -import * as vscode from "vscode"; -import { - activateExtension, - openDocumentAndWaitForProcessing, - waitForCondition, - TEST_TIMEOUT_MS, -} from "../extensionUtils"; - -/** - * Verifies that edits arriving while a compilation is in flight are coalesced into far - * fewer compilations than there were edits. - * - * Compilation blocks the extension host, so the edits have to originate on the other - * side of that boundary to pile up the way real keystrokes do. They are therefore issued - * without awaiting, letting the editor apply them and deliver the change notifications - * while the host is busy. - * - * Which version was compiled is read from the dev status diagnostic, whose message - * carries `version=N`. That diagnostic is the only place a document version is - * observable from outside the language service. - */ -suite("Update coalescing", function suite() { - const workspaceFolder = - vscode.workspace.workspaceFolders && vscode.workspace.workspaceFolders[0]; - assert(workspaceFolder, "Expecting an open folder"); - - const noErrorsQs = vscode.Uri.joinPath(workspaceFolder.uri, "no-errors.qs"); - - // Long enough that every edit lands while a compilation is blocking the host. - const simulatedCompileDelayMs = 100; - const editCount = 20; - - this.beforeAll(async () => { - await activateExtension(); - await vscode.workspace - .getConfiguration("Q#") - .update( - "dev.simulatedCompileDelayMs", - simulatedCompileDelayMs, - vscode.ConfigurationTarget.Global, - ); - }); - - this.afterAll(async () => { - await vscode.workspace - .getConfiguration("Q#") - .update( - "dev.simulatedCompileDelayMs", - undefined, - vscode.ConfigurationTarget.Global, - ); - await vscode.commands.executeCommand( - "workbench.action.revertAndCloseActiveEditor", - ); - }); - - test("many rapid edits produce far fewer compilations", async () => { - const doc = await openDocumentAndWaitForProcessing(noErrorsQs); - const editor = await vscode.window.showTextDocument(doc); - - const compiledVersions: number[] = []; - const recorder = vscode.languages.onDidChangeDiagnostics((event) => { - if (!event.uris.some((u) => u.toString() === doc.uri.toString())) { - return; - } - for (const diagnostic of vscode.languages.getDiagnostics(doc.uri)) { - const match = /version=(\d+)/.exec(diagnostic.message); - if (match) { - const version = Number(match[1]); - if (compiledVersions.at(-1) !== version) { - compiledVersions.push(version); - } - } - } - }); - - try { - const insertAt = new vscode.Position(3, 26); - editor.selection = new vscode.Selection(insertAt, insertAt); - - // Deliberately not awaited individually, so they queue up in the editor rather - // than being serialized behind each compilation. - const typed: Thenable[] = []; - for (let i = 0; i < editCount; i++) { - typed.push(vscode.commands.executeCommand("type", { text: "a" })); - } - await Promise.all(typed); - - const finalVersion = doc.version; - - await waitForCondition( - () => compiledVersions.includes(finalVersion), - vscode.languages.onDidChangeDiagnostics, - TEST_TIMEOUT_MS, - `Final document version ${finalVersion} was never compiled. ` + - `Compiled versions: ${compiledVersions.join(", ")}`, - ); - - console.log( - `qsharp-tests: ${editCount} edits produced document version ${finalVersion}; ` + - `compiled versions: ${compiledVersions.join(", ")}`, - ); - - // The exact count depends on machine speed, so this only asserts that coalescing - // happened at all. Without it there would be one compilation per edit. - assert.isBelow( - compiledVersions.length, - editCount, - "expected fewer compilations than edits", - ); - } finally { - recorder.dispose(); - } - }); -}); diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index b0be0c44441..427b04a54f9 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -35,13 +35,6 @@ impl LanguageService { #[wasm_bindgen(constructor)] #[allow(clippy::new_without_default)] // wasm-bindgen requires constructor to be explicitly defined pub fn new() -> Self { - // Only ever does anything when a test opts in via `simulatedCompileDelayMs`. - qsls::typing_simulation::set_busy_wait_callback(Box::new(|ms: u32| { - let end = js_sys::Date::now() + f64::from(ms); - while js_sys::Date::now() < end { - std::hint::spin_loop(); - } - })); LanguageService(qsls::LanguageService::new(Encoding::Utf16)) } @@ -154,7 +147,6 @@ impl LanguageService { .map(|features| features.iter().collect::()), lints_config: config.lints, dev_diagnostics: config.devDiagnostics, - simulated_compile_delay_ms: config.simulatedCompileDelayMs, }); } @@ -450,7 +442,6 @@ serializable_type! { pub languageFeatures: Option>, pub lints: Option>, pub devDiagnostics: Option, - pub simulatedCompileDelayMs: Option, }, r#"export interface IWorkspaceConfiguration { targetProfile?: TargetProfile; @@ -458,7 +449,6 @@ serializable_type! { languageFeatures?: LanguageFeatures[]; lints?: ({ lint: string; level: string } | { group: string; level: string })[]; devDiagnostics?: boolean; - simulatedCompileDelayMs?: number; }"#, IWorkspaceConfiguration } From 20e51592ec35396dc94c42121a45f622e205df59 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 31 Jul 2026 10:34:42 -0700 Subject: [PATCH 06/13] Drop flaky test --- source/language_service/src/tests.rs | 77 +++++++++++++++++++ .../completion-retrigger.test.ts | 69 ++++++++++++----- .../suites/language-service/index.browser.ts | 1 - .../suites/language-service/index.node.ts | 1 - 4 files changed, 126 insertions(+), 22 deletions(-) diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 8b572265b6a..54dbcb8f30c 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -461,6 +461,83 @@ async fn run_applies_updates_to_distinct_documents() { assert_eq!(applied, ["bar.qs", "foo.qs"]); } +#[tokio::test] +async fn wait_for_document_version_ready_when_already_current() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 1, "namespace Foo { }", "qsharp"); + worker.apply_pending().await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWait::Ready + ); +} + +/// A caller that parks while its version is still queued is woken once it lands. +#[tokio::test] +async fn wait_for_document_version_resolves_once_applied() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 1, "namespace Foo { }", "qsharp"); + + let wait = ls.wait_for_document_version("foo.qs", 1); + futures_util::pin_mut!(wait); + assert!( + futures::poll!(wait.as_mut()).is_pending(), + "expected the caller to park until the update is applied" + ); + + worker.apply_pending().await; + + assert_eq!(wait.await, VersionWait::Ready); +} + +/// The case the completion path is built around: the requested version gets merged away +/// before it is ever compiled, so it can never be answered for. +#[tokio::test] +async fn wait_for_document_version_superseded_when_coalesced_away() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 1, "namespace Foo { ", "qsharp"); + + let wait = ls.wait_for_document_version("foo.qs", 1); + futures_util::pin_mut!(wait); + assert!(futures::poll!(wait.as_mut()).is_pending()); + + // Version 1 is still queued, so this merges over it and only version 2 is compiled. + ls.update_document("foo.qs", 2, "namespace Foo { a", "qsharp"); + worker.apply_pending().await; + + assert_eq!(wait.await, VersionWait::Superseded); +} + +/// A version the state has already moved past is reported without parking at all. +#[tokio::test] +async fn wait_for_document_version_superseded_without_parking() { + let received_errors = RefCell::new(Vec::new()); + let test_cases = RefCell::new(Vec::new()); + let mut ls = LanguageService::new(Encoding::Utf8); + let mut worker = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 2, "namespace Foo { }", "qsharp"); + worker.apply_pending().await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWait::Superseded + ); +} + /// A caller parked on a version that can no longer arrive has to be released when the /// update handler stops, rather than waiting forever. #[tokio::test] diff --git a/source/vscode/test/suites/language-service/completion-retrigger.test.ts b/source/vscode/test/suites/language-service/completion-retrigger.test.ts index 41fb091caf4..e315cd5ec31 100644 --- a/source/vscode/test/suites/language-service/completion-retrigger.test.ts +++ b/source/vscode/test/suites/language-service/completion-retrigger.test.ts @@ -6,6 +6,7 @@ import * as vscode from "vscode"; import { activateExtension, openDocumentAndWaitForProcessing, + waitForCondition, } from "../extensionUtils"; /** @@ -40,6 +41,10 @@ suite("Completion re-trigger behavior", function suite() { // Roughly a fast typist. const keystrokeIntervalMs = 40; + // Generous, but short enough to fail with a useful message rather than hitting the + // suite-wide timeout. + const activeEditorTimeoutMs = 10_000; + type Invocation = { version: number; triggerKind: vscode.CompletionTriggerKind; @@ -91,70 +96,94 @@ suite("Completion re-trigger behavior", function suite() { const doc = await openDocumentAndWaitForProcessing(noErrorsQs); const editor = await vscode.window.showTextDocument(doc); + // `type` goes to whichever editor is active, so typing before this settles sends + // the keystrokes nowhere and no suggest session is ever opened. + await waitForCondition( + () => + vscode.window.activeTextEditor?.document.uri.toString() === + doc.uri.toString(), + vscode.window.onDidChangeActiveTextEditor, + activeEditorTimeoutMs, + "the document never became the active editor", + ); + // Land the cursor at the end of the `let foo = "hello!";` line so typed text // forms a fresh expression rather than editing existing code. const insertAt = new vscode.Position(3, 26); editor.selection = new vscode.Selection(insertAt, insertAt); - let versionAtDot = 0; + const startVersion = doc.version; + for (const ch of ["S", "t", "d", ".", "D", "i"]) { await vscode.commands.executeCommand("type", { text: ch }); - if (ch === ".") { - versionAtDot = doc.version; - } await new Promise((resolve) => setTimeout(resolve, keystrokeIntervalMs)); } // Give any trailing re-triggers a chance to land. await new Promise((resolve) => setTimeout(resolve, 1500)); - // Keyed on the document version rather than the trigger kind: when a suggest - // session is already open, VS Code may re-query an incomplete provider instead of - // starting a fresh trigger-character session, so the kind isn't dependable. - const afterDot = invocations.filter((i) => i.version > versionAtDot); + // Keyed on the trigger kind, which is the only signal specific to the behavior + // under test. The editor also opens unrelated `Invoke` sessions, sometimes many of + // them against an unchanging document, and counting those is just machine-speed + // noise. `TriggerForIncompleteCompletions` means VS Code came back *because* the + // provider reported the list incomplete. + const retriggers = invocations.filter( + (i) => + i.triggerKind === + vscode.CompletionTriggerKind.TriggerForIncompleteCompletions, + ); console.log( - `qsharp-tests: isIncomplete=${isIncomplete} versionAtDot=${versionAtDot} finalDocVersion=${doc.version}\n` + + `qsharp-tests: isIncomplete=${isIncomplete} finalDocVersion=${doc.version}\n` + `qsharp-tests: all invocations: ${invocations .map( (i) => `v${i.version}/${vscode.CompletionTriggerKind[i.triggerKind]}${i.triggerCharacter ? `('${i.triggerCharacter}')` : ""}`, ) .join(", ")}\n` + - `qsharp-tests: invocations after dot: ${afterDot.length}`, + `qsharp-tests: incomplete re-triggers: ${retriggers.length}`, ); + // Checked before the invocation count so that a failure distinguishes "the + // keystrokes never arrived" from "they arrived but suggest didn't run". + assert.isAbove( + doc.version, + startVersion, + "the typed characters never reached the document", + ); assert.isNotEmpty( invocations, "expected the completion provider to be invoked while typing", ); - assert.isAbove( - doc.version, - versionAtDot, - "expected more edits after the `.` keystroke", - ); - return { versionAtDot, afterDot, doc }; + return { retriggers, doc }; } test("a complete list is NOT re-requested on later keystrokes", async () => { - const { afterDot } = await typeAndRecord(false); + const { retriggers } = await typeAndRecord(false); assert.isEmpty( - afterDot, + retriggers, "expected VS Code to filter a complete list client-side rather than re-requesting", ); }); test("an incomplete list IS re-requested on later keystrokes", async () => { - const { afterDot } = await typeAndRecord(true); + const { retriggers, doc } = await typeAndRecord(true); assert.isNotEmpty( - afterDot, + retriggers, "VS Code did not re-invoke the provider after an incomplete list. Returning an " + "empty incomplete list is therefore NOT a sufficient mitigation for a " + "coalesced-away completion request, and the update loop must avoid coalescing " + "past a version a completion request is waiting on.", ); + + // The point of the mitigation: the request that eventually gets answered is for + // the version the update loop settles on, not the one that was coalesced away. + assert.isTrue( + retriggers.some((i) => i.version === doc.version), + `expected a re-trigger for the final document version ${doc.version}`, + ); }); }); diff --git a/source/vscode/test/suites/language-service/index.browser.ts b/source/vscode/test/suites/language-service/index.browser.ts index e494c8e9460..a526e9e751a 100644 --- a/source/vscode/test/suites/language-service/index.browser.ts +++ b/source/vscode/test/suites/language-service/index.browser.ts @@ -12,7 +12,6 @@ export function run(): Promise { // paths here since ESBuild needs these modules to be // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports - require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); diff --git a/source/vscode/test/suites/language-service/index.node.ts b/source/vscode/test/suites/language-service/index.node.ts index 6dd20140fa2..a3e85a4154f 100644 --- a/source/vscode/test/suites/language-service/index.node.ts +++ b/source/vscode/test/suites/language-service/index.node.ts @@ -17,7 +17,6 @@ export async function run(): Promise { // paths here since ESBuild needs these modules to be // real paths on disk at bundling time. require("./language-service.test"); // eslint-disable-line @typescript-eslint/no-require-imports - require("./completion-retrigger.test"); // eslint-disable-line @typescript-eslint/no-require-imports }, { timeout: TEST_TIMEOUT_MS }, ); From c269c3205dfea709961f0337656c9504fbf13570 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 31 Jul 2026 14:40:44 -0700 Subject: [PATCH 07/13] Return speculative results on version mismatch We were requiring an exact match and returning an empty list, which would result in an empty dialog and possibly even flickering (if it had previously been non-empty). Since, prior to this change, we weren't even checking the version and since there's no clear value to return in an error state, just return what we used to - the value computed from the "future" version. Given that I can't seem to get this to happen in practice, I think it's preferable to introducing a ton of code to make it wake up at the exact revision. On top of the at, the editor will likely discard the results if a non-trivial edit has happened in the interim. --- .../src/language-service/language-service.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/source/npm/qsharp/src/language-service/language-service.ts b/source/npm/qsharp/src/language-service/language-service.ts index 12c907e5fb6..329cc416f26 100644 --- a/source/npm/qsharp/src/language-service/language-service.ts +++ b/source/npm/qsharp/src/language-service/language-service.ts @@ -259,14 +259,19 @@ export class QSharpLanguageService implements ILanguageService { completionWaitTimeoutMs, ); - if (status !== "ready") { - // Can't answer for this version, and answering for another would be wrong. - // Reporting the list as incomplete makes VS Code ask again on the next keystroke, - // and that request will be for a version that does get compiled. - return { items: [], isIncomplete: true }; + const completions: CompletionListResult = + this.languageService.get_completions(documentUri, position); + + if (status != "ready") { + log.info( + `Providing completions for ${documentUri} from a different version than requested`, + ); + // Attempt to signal to the editor that the list is provisional and a fresh request should + // be made on the next keystroke (vs just filtering). + completions.isIncomplete = true; } - return this.languageService.get_completions(documentUri, position); + return completions; } async getFormatChanges(documentUri: string): Promise { From cf77d2326a2c6c65d317b5af7f5abccef73fa511 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 31 Jul 2026 14:48:48 -0700 Subject: [PATCH 08/13] Thread isIncomplete through monaco --- source/playground/src/main.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/source/playground/src/main.tsx b/source/playground/src/main.tsx index 659183b2e61..9e9c11f0f47 100644 --- a/source/playground/src/main.tsx +++ b/source/playground/src/main.tsx @@ -312,6 +312,7 @@ function registerMonacoLanguageServiceProviders( monacoPositionToLsPosition(position), ); return { + incomplete: completions.isIncomplete, suggestions: completions.items.map((i) => { let kind; switch (i.kind) { From 29748c61514e38bfd49656ef3b0c0d8b231f9aea Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 31 Jul 2026 16:57:40 -0700 Subject: [PATCH 09/13] Tidy up some loose ends --- source/language_service/src/lib.rs | 23 ++- .../src/language-service/language-service.ts | 19 +- .../completion-retrigger.test.ts | 189 ------------------ source/wasm/src/language_service.rs | 20 +- 4 files changed, 40 insertions(+), 211 deletions(-) delete mode 100644 source/vscode/test/suites/language-service/completion-retrigger.test.ts diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index 3f68ce56abd..0c0a64726cd 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -83,7 +83,10 @@ impl VersionWaiters { return None; } let (send, recv) = oneshot::channel(); - self.parked.borrow_mut().push(send); + let mut parked = self.parked.borrow_mut(); + // Backstop for callers that gave up; `wake_all` collects the rest on the next update. + parked.retain(|sender| !sender.is_canceled()); + parked.push(send); Some(recv) } @@ -240,11 +243,9 @@ impl LanguageService { }); } - /// Waits until the compilation state reflects exactly `version` of `uri`. - /// - /// The match has to be exact. A later version is not an acceptable substitute: the - /// caller's `position` was computed against `version`, so against newer text it may - /// point somewhere else entirely, or not exist at all. + /// Waits until the compilation state reflects exactly `version` of `uri`, reporting + /// [`VersionWait::Superseded`] if the document moves past it first. Whether a + /// superseded version is still usable is left to the caller. /// /// The returned future is independent of `&self` so that callers can hold it across /// await points without keeping the language service borrowed. The `use<>` bound is @@ -486,6 +487,8 @@ impl UpdateHandler<'_> { self.apply(updates).await; } + // Not just left to `drop`, so waiters are released as soon as the loop ends even + // if the handler itself is kept alive. self.version_waiters.shut_down(); } @@ -544,6 +547,14 @@ impl UpdateHandler<'_> { } } +impl Drop for UpdateHandler<'_> { + /// Covers every way the handler can go away, including `run` never being called or + /// its future being cancelled, not just the loop exiting normally. + fn drop(&mut self) { + self.version_waiters.shut_down(); + } +} + fn push_update(pending_updates: &mut Vec, update: Update) { // Dedup consecutive updates to the same document. match &update { diff --git a/source/npm/qsharp/src/language-service/language-service.ts b/source/npm/qsharp/src/language-service/language-service.ts index 329cc416f26..4e7e3eb7de0 100644 --- a/source/npm/qsharp/src/language-service/language-service.ts +++ b/source/npm/qsharp/src/language-service/language-service.ts @@ -60,10 +60,9 @@ export type CompletionListResult = ICompletionList & { /** * How long to wait for a completion request's document version to be compiled. * - * This is a liveness backstop, not a tuning knob. A version that gets coalesced away is - * reported as superseded immediately, so hitting this timeout means the update is - * genuinely stuck, such as behind a slow project load. Expiring returns an incomplete - * list, so VS Code asks again rather than showing a stale one. + * This is a liveness backstop, not a tuning knob. Expiring doesn't fail the request: the + * list is computed against whatever version is current and flagged incomplete, so the + * cost of waiting too little is a provisional answer rather than no answer. */ const completionWaitTimeoutMs = 2000; @@ -249,10 +248,10 @@ export class QSharpLanguageService implements ILanguageService { version: number, position: IPosition, ): Promise { - // The position was computed against this exact version of the document, so a later - // one is not an acceptable substitute: it may put the position somewhere else - // entirely. This matters most when the last character typed is significant to the - // completion, as in `Foo.`. + // The position was computed against this version, so answering against an older one + // gets the wrong list: the last character typed is often what determines the answer, + // as in `Foo.`. Waiting avoids that. A newer version is a lesser problem, since the + // position only drifts if the intervening edits moved it, so it's tolerated below. const status = await this.languageService.wait_for_document_version( documentUri, version, @@ -262,9 +261,9 @@ export class QSharpLanguageService implements ILanguageService { const completions: CompletionListResult = this.languageService.get_completions(documentUri, position); - if (status != "ready") { + if (status !== "ready") { log.info( - `Providing completions for ${documentUri} from a different version than requested`, + `Providing completions for ${documentUri} from a ${status === "timeout" ? "older" : "newer"} version than requested`, ); // Attempt to signal to the editor that the list is provisional and a fresh request should // be made on the next keystroke (vs just filtering). diff --git a/source/vscode/test/suites/language-service/completion-retrigger.test.ts b/source/vscode/test/suites/language-service/completion-retrigger.test.ts deleted file mode 100644 index e315cd5ec31..00000000000 --- a/source/vscode/test/suites/language-service/completion-retrigger.test.ts +++ /dev/null @@ -1,189 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -import { assert } from "chai"; -import * as vscode from "vscode"; -import { - activateExtension, - openDocumentAndWaitForProcessing, - waitForCondition, -} from "../extensionUtils"; - -/** - * Measures whether VS Code re-invokes a completion provider as the user keeps typing - * after an earlier suggest request. - * - * This decides how the language service should handle a completion request whose - * document version gets coalesced away by the update loop. Such a request cannot be - * answered correctly, since its position refers to text the user has moved past, so it - * has to return nothing. That is only safe if VS Code comes back and asks again. - * - * VS Code re-queries a provider on subsequent keystrokes only when that provider - * returned `isIncomplete: true`; a complete list is filtered client-side instead. - * Both cases are measured below, because that difference is exactly what determines - * whether returning an empty incomplete list is a sufficient mitigation. - * - * The recorder is registered as a *second* completion provider. VS Code queries every - * registered provider within a suggest session and tracks incompleteness per provider, - * so this observes the real behavior without any production instrumentation. - * - * This must drive the editor with the `type` command rather than `editor.edit()` or - * `vscode.executeCompletionItemProvider`. Only real typing runs the suggest widget's - * trigger/re-trigger logic, which is the thing being measured. - */ -suite("Completion re-trigger behavior", function suite() { - const workspaceFolder = - vscode.workspace.workspaceFolders && vscode.workspace.workspaceFolders[0]; - assert(workspaceFolder, "Expecting an open folder"); - - const noErrorsQs = vscode.Uri.joinPath(workspaceFolder.uri, "no-errors.qs"); - - // Roughly a fast typist. - const keystrokeIntervalMs = 40; - - // Generous, but short enough to fail with a useful message rather than hitting the - // suite-wide timeout. - const activeEditorTimeoutMs = 10_000; - - type Invocation = { - version: number; - triggerKind: vscode.CompletionTriggerKind; - triggerCharacter: string | undefined; - }; - - let invocations: Invocation[] = []; - let recorder: vscode.Disposable | undefined; - - this.beforeAll(async () => { - await activateExtension(); - }); - - this.afterEach(async () => { - recorder?.dispose(); - recorder = undefined; - await vscode.commands.executeCommand( - "workbench.action.revertAndCloseActiveEditor", - ); - }); - - /** - * Registers the recording provider, types `Std.Di` one character at a time, and - * returns the invocations that happened strictly after `.` triggered suggest. - */ - async function typeAndRecord(isIncomplete: boolean) { - invocations = []; - recorder = vscode.languages.registerCompletionItemProvider( - "qsharp", - { - provideCompletionItems(document, _position, _token, context) { - invocations.push({ - version: document.version, - triggerKind: context.triggerKind, - triggerCharacter: context.triggerCharacter, - }); - // Returns an item rather than an empty list, because VS Code discards an - // empty result and closes the session, which would suppress re-triggering - // for reasons unrelated to what is being measured here. - return new vscode.CompletionList( - [new vscode.CompletionItem("ZzProbeItem")], - isIncomplete, - ); - }, - }, - ".", - ); - - const doc = await openDocumentAndWaitForProcessing(noErrorsQs); - const editor = await vscode.window.showTextDocument(doc); - - // `type` goes to whichever editor is active, so typing before this settles sends - // the keystrokes nowhere and no suggest session is ever opened. - await waitForCondition( - () => - vscode.window.activeTextEditor?.document.uri.toString() === - doc.uri.toString(), - vscode.window.onDidChangeActiveTextEditor, - activeEditorTimeoutMs, - "the document never became the active editor", - ); - - // Land the cursor at the end of the `let foo = "hello!";` line so typed text - // forms a fresh expression rather than editing existing code. - const insertAt = new vscode.Position(3, 26); - editor.selection = new vscode.Selection(insertAt, insertAt); - - const startVersion = doc.version; - - for (const ch of ["S", "t", "d", ".", "D", "i"]) { - await vscode.commands.executeCommand("type", { text: ch }); - await new Promise((resolve) => setTimeout(resolve, keystrokeIntervalMs)); - } - - // Give any trailing re-triggers a chance to land. - await new Promise((resolve) => setTimeout(resolve, 1500)); - - // Keyed on the trigger kind, which is the only signal specific to the behavior - // under test. The editor also opens unrelated `Invoke` sessions, sometimes many of - // them against an unchanging document, and counting those is just machine-speed - // noise. `TriggerForIncompleteCompletions` means VS Code came back *because* the - // provider reported the list incomplete. - const retriggers = invocations.filter( - (i) => - i.triggerKind === - vscode.CompletionTriggerKind.TriggerForIncompleteCompletions, - ); - - console.log( - `qsharp-tests: isIncomplete=${isIncomplete} finalDocVersion=${doc.version}\n` + - `qsharp-tests: all invocations: ${invocations - .map( - (i) => - `v${i.version}/${vscode.CompletionTriggerKind[i.triggerKind]}${i.triggerCharacter ? `('${i.triggerCharacter}')` : ""}`, - ) - .join(", ")}\n` + - `qsharp-tests: incomplete re-triggers: ${retriggers.length}`, - ); - - // Checked before the invocation count so that a failure distinguishes "the - // keystrokes never arrived" from "they arrived but suggest didn't run". - assert.isAbove( - doc.version, - startVersion, - "the typed characters never reached the document", - ); - assert.isNotEmpty( - invocations, - "expected the completion provider to be invoked while typing", - ); - - return { retriggers, doc }; - } - - test("a complete list is NOT re-requested on later keystrokes", async () => { - const { retriggers } = await typeAndRecord(false); - - assert.isEmpty( - retriggers, - "expected VS Code to filter a complete list client-side rather than re-requesting", - ); - }); - - test("an incomplete list IS re-requested on later keystrokes", async () => { - const { retriggers, doc } = await typeAndRecord(true); - - assert.isNotEmpty( - retriggers, - "VS Code did not re-invoke the provider after an incomplete list. Returning an " + - "empty incomplete list is therefore NOT a sufficient mitigation for a " + - "coalesced-away completion request, and the update loop must avoid coalescing " + - "past a version a completion request is waiting on.", - ); - - // The point of the mitigation: the request that eventually gets answered is for - // the version the update loop settles on, not the one that was coalesced away. - assert.isTrue( - retriggers.some((i) => i.version === doc.version), - `expected a re-trigger for the final document version ${doc.version}`, - ); - }); -}); diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index 427b04a54f9..ab567ec490d 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -21,10 +21,18 @@ use std::str::FromStr; use wasm_bindgen::prelude::*; use wasm_bindgen_futures::future_to_promise; +#[wasm_bindgen(typescript_custom_section)] +const VERSION_WAIT_STATUS: &'static str = r#" +export type VersionWaitStatus = "ready" | "superseded" | "timeout"; +"#; + #[wasm_bindgen] extern "C" { #[wasm_bindgen(js_name = setTimeout)] fn set_timeout(closure: &js_sys::Function, ms: i32); + + #[wasm_bindgen(typescript_type = "Promise")] + pub type PromiseVersionWaitStatus; } #[wasm_bindgen] @@ -208,18 +216,17 @@ impl LanguageService { } /// Resolves once the compilation state reflects exactly `version` of `uri`, or the - /// document moves past it, or `timeout_ms` elapses. Resolves to `"ready"`, - /// `"superseded"` or `"timeout"`. + /// document moves past it, or `timeout_ms` elapses. /// - /// The timeout is a liveness backstop rather than a tuning knob. A version that gets - /// coalesced away reports `"superseded"` immediately, so reaching the timeout means - /// the update is genuinely stuck, for instance behind a slow project load. + /// The timeout is a liveness backstop rather than a tuning knob. Supersession is + /// reported as soon as the next batch of updates lands, so reaching the timeout + /// means the update is genuinely stuck, for instance behind a slow project load. pub fn wait_for_document_version( &self, uri: &str, version: u32, timeout_ms: i32, - ) -> js_sys::Promise { + ) -> PromiseVersionWaitStatus { let wait = self.0.wait_for_document_version(uri, version); let uri = uri.to_string(); @@ -242,6 +249,7 @@ impl LanguageService { }; Ok(JsValue::from_str(result)) }) + .unchecked_into() } pub fn get_completions(&self, uri: &str, position: IPosition) -> ICompletionList { From 249ee465b1a70f0eab6fe742ab2bd760be181bb0 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 4 Aug 2026 10:04:31 -0700 Subject: [PATCH 10/13] Rename VersionWait --- source/language_service/src/lib.rs | 14 +++++++------- source/language_service/src/tests.rs | 14 +++++++------- source/wasm/src/language_service.rs | 8 +++++--- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index 0c0a64726cd..fd58b48ddb2 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -59,7 +59,7 @@ pub struct LanguageService { /// The outcome of waiting for a specific version of a document to be compiled. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum VersionWait { +pub enum VersionWaitResult { /// The compilation state reflects exactly the requested version. Ready, /// The document has already moved past the requested version. That version was @@ -244,7 +244,7 @@ impl LanguageService { } /// Waits until the compilation state reflects exactly `version` of `uri`, reporting - /// [`VersionWait::Superseded`] if the document moves past it first. Whether a + /// [`VersionWaitResult::Superseded`] if the document moves past it first. Whether a /// superseded version is still usable is left to the caller. /// /// The returned future is independent of `&self` so that callers can hold it across @@ -254,7 +254,7 @@ impl LanguageService { &self, uri: &str, version: u32, - ) -> impl std::future::Future + 'static + use<> { + ) -> impl std::future::Future + 'static + use<> { let state = self.state.clone(); let waiters = self.version_waiters.clone(); let uri = uri.to_string(); @@ -266,20 +266,20 @@ impl LanguageService { let receiver = { let state = state.borrow(); match state.get_open_document_version(&uri) { - Some(current) if current == version => return VersionWait::Ready, - Some(current) if current > version => return VersionWait::Superseded, + Some(current) if current == version => return VersionWaitResult::Ready, + Some(current) if current > version => return VersionWaitResult::Superseded, // Either behind, or the document hasn't been processed at all yet. _ => match waiters.park() { Some(receiver) => receiver, // The update handler is gone, so the version will never arrive. - None => return VersionWait::Superseded, + None => return VersionWaitResult::Superseded, }, } }; if receiver.await.is_err() { // The update handler shut down while we were parked. - return VersionWait::Superseded; + return VersionWaitResult::Superseded; } } } diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 54dbcb8f30c..8be71e2ff8d 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -2,7 +2,7 @@ // Licensed under the MIT License. use crate::{ - Encoding, LanguageService, Update, UpdateHandler, VersionWait, + Encoding, LanguageService, Update, UpdateHandler, VersionWaitResult, protocol::{DiagnosticUpdate, ErrorKind, TestCallables, WorkspaceConfigurationUpdate}, push_update, }; @@ -473,7 +473,7 @@ async fn wait_for_document_version_ready_when_already_current() { assert_eq!( ls.wait_for_document_version("foo.qs", 1).await, - VersionWait::Ready + VersionWaitResult::Ready ); } @@ -496,7 +496,7 @@ async fn wait_for_document_version_resolves_once_applied() { worker.apply_pending().await; - assert_eq!(wait.await, VersionWait::Ready); + assert_eq!(wait.await, VersionWaitResult::Ready); } /// The case the completion path is built around: the requested version gets merged away @@ -518,7 +518,7 @@ async fn wait_for_document_version_superseded_when_coalesced_away() { ls.update_document("foo.qs", 2, "namespace Foo { a", "qsharp"); worker.apply_pending().await; - assert_eq!(wait.await, VersionWait::Superseded); + assert_eq!(wait.await, VersionWaitResult::Superseded); } /// A version the state has already moved past is reported without parking at all. @@ -534,7 +534,7 @@ async fn wait_for_document_version_superseded_without_parking() { assert_eq!( ls.wait_for_document_version("foo.qs", 1).await, - VersionWait::Superseded + VersionWaitResult::Superseded ); } @@ -554,7 +554,7 @@ async fn wait_for_document_version_released_when_handler_stops() { // `join` polls the wait first, so it is parked by the time the handler shuts down. let (result, ()) = futures::future::join(wait, worker.run(|| std::future::ready(()))).await; - assert_eq!(result, VersionWait::Superseded); + assert_eq!(result, VersionWaitResult::Superseded); } /// Once the handler has stopped there is nothing left to wake a new caller, so parking @@ -571,7 +571,7 @@ async fn wait_for_document_version_returns_immediately_after_handler_stops() { assert_eq!( ls.wait_for_document_version("foo.qs", 1).await, - VersionWait::Superseded + VersionWaitResult::Superseded ); } diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index ab567ec490d..14a4e552268 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -13,7 +13,7 @@ use qsc::{ target::Profile, }; use qsc_project::Manifest; -use qsls::VersionWait; +use qsls::VersionWaitResult; use qsls::protocol::{DiagnosticUpdate, TestCallable, TestCallables}; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; @@ -238,8 +238,10 @@ impl LanguageService { futures_util::pin_mut!(wait, timeout); let result = match futures_util::future::select(wait, timeout).await { - futures_util::future::Either::Left((VersionWait::Ready, _)) => "ready", - futures_util::future::Either::Left((VersionWait::Superseded, _)) => "superseded", + futures_util::future::Either::Left((VersionWaitResult::Ready, _)) => "ready", + futures_util::future::Either::Left((VersionWaitResult::Superseded, _)) => { + "superseded" + } futures_util::future::Either::Right(_) => { log::debug!( "timed out after {timeout_ms}ms waiting for {uri} version {version}" From 73be54a0bfbb66caeaff1c93cac299d0eb43c122 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 4 Aug 2026 10:13:36 -0700 Subject: [PATCH 11/13] Introduce VersionWaitResult::Never --- source/language_service/src/lib.rs | 12 ++++++++---- source/language_service/src/tests.rs | 4 ++-- source/wasm/src/language_service.rs | 2 ++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index fd58b48ddb2..a8becd1b099 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -65,6 +65,9 @@ pub enum VersionWaitResult { /// The document has already moved past the requested version. That version was /// coalesced away and will never be compiled, so it can no longer be answered for. Superseded, + /// Indicates that the language service has been shutdown and the awaited version + /// will never be compiled. + Never, } /// Callers parked in [`LanguageService::wait_for_document_version`], waiting for the @@ -244,8 +247,9 @@ impl LanguageService { } /// Waits until the compilation state reflects exactly `version` of `uri`, reporting - /// [`VersionWaitResult::Superseded`] if the document moves past it first. Whether a - /// superseded version is still usable is left to the caller. + /// [`VersionWaitResult::Superseded`] if the document moves past it first, or + /// [`VersionWaitResult::Never`] if the language service stops updating before the + /// version can arrive. Whether a superseded version is still usable is left to the caller. /// /// The returned future is independent of `&self` so that callers can hold it across /// await points without keeping the language service borrowed. The `use<>` bound is @@ -272,14 +276,14 @@ impl LanguageService { _ => match waiters.park() { Some(receiver) => receiver, // The update handler is gone, so the version will never arrive. - None => return VersionWaitResult::Superseded, + None => return VersionWaitResult::Never, }, } }; if receiver.await.is_err() { // The update handler shut down while we were parked. - return VersionWaitResult::Superseded; + return VersionWaitResult::Never; } } } diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 8be71e2ff8d..1f93203027f 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -554,7 +554,7 @@ async fn wait_for_document_version_released_when_handler_stops() { // `join` polls the wait first, so it is parked by the time the handler shuts down. let (result, ()) = futures::future::join(wait, worker.run(|| std::future::ready(()))).await; - assert_eq!(result, VersionWaitResult::Superseded); + assert_eq!(result, VersionWaitResult::Never); } /// Once the handler has stopped there is nothing left to wake a new caller, so parking @@ -571,7 +571,7 @@ async fn wait_for_document_version_returns_immediately_after_handler_stops() { assert_eq!( ls.wait_for_document_version("foo.qs", 1).await, - VersionWaitResult::Superseded + VersionWaitResult::Never ); } diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index 14a4e552268..1c42010bbed 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -242,6 +242,8 @@ impl LanguageService { futures_util::future::Either::Left((VersionWaitResult::Superseded, _)) => { "superseded" } + // Treat LS shutdown as a timeout - the distinction isn't important + futures_util::future::Either::Left((VersionWaitResult::Never, _)) => "timeout", futures_util::future::Either::Right(_) => { log::debug!( "timed out after {timeout_ms}ms waiting for {uri} version {version}" From 2a772da71586bad3394dfeff795af974b14eb3d3 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 4 Aug 2026 10:27:08 -0700 Subject: [PATCH 12/13] Only yield once per update --- source/language_service/src/lib.rs | 27 ++++++++--------------- source/language_service/src/tests.rs | 32 ++++++++++++---------------- 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index a8becd1b099..615838db0ec 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -445,11 +445,6 @@ pub struct UpdateHandler<'a> { version_waiters: Rc, } -/// Caps how many times [`UpdateHandler::run`] will give the host a turn to deliver more -/// input before processing what it has. Only bounds the worst case: the loop normally -/// exits earlier, as soon as a yield produces nothing new. -const MAX_YIELDS_PER_BATCH: usize = 4; - impl UpdateHandler<'_> { /// Runs the update handler. This method is expected to run /// for the entire lifetime of the language service. @@ -470,21 +465,17 @@ impl UpdateHandler<'_> { while let Some(update) = self.recv.next().await { let mut updates = vec![update]; - // Keep giving the host a turn for as long as it has more to deliver. When the - // user pauses, the first yield comes back with nothing and this exits after a - // single tick. While they're typing, the backlog arrives over a few passes. - for _ in 0..MAX_YIELDS_PER_BATCH { - yield_to_host().await; + // We could consider yielding more than once, but empirically, that doesn't + // obviously result in more batching. + yield_to_host().await; - let batch_before = updates.len(); - let drained = self.drain_pending(&mut updates); - if drained == 0 { - break; - } + let batch_before = updates.len(); + let drained = self.drain_pending(&mut updates); - // Every drained update either claims a new slot in the batch or merges - // into an existing one, and each merge discards one update. - let dropped = drained - (updates.len() - batch_before); + // Every drained update either claims a new slot in the batch or merges + // into an existing one, and each merge discards one update. + let dropped = drained - (updates.len() - batch_before); + if dropped > 0 { trace!("drained {drained} update(s), merging dropped {dropped} as redundant"); } diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 1f93203027f..99e94ae2058 100644 --- a/source/language_service/src/tests.rs +++ b/source/language_service/src/tests.rs @@ -391,17 +391,14 @@ async fn run_coalesces_updates_delivered_while_yielding() { let ls = RefCell::new(ls); let yields = Cell::new(0); let yield_to_host = || { - match yields.replace(yields.get() + 1) { - 0 => { - // Two more keystrokes land while the first update is being handled. - ls.borrow_mut() - .update_document("foo.qs", 2, "namespace Foo { a", "qsharp"); - ls.borrow_mut() - .update_document("foo.qs", 3, "namespace Foo { ab", "qsharp"); - } - // Nothing further arrives, so the loop should stop yielding. Closing the - // channel is what lets `run()` return instead of waiting forever. - _ => ls.borrow_mut().stop_updates(), + if yields.replace(yields.get() + 1) == 0 { + let mut ls = ls.borrow_mut(); + // Two more keystrokes land while the first update is being handled. + ls.update_document("foo.qs", 2, "namespace Foo { a", "qsharp"); + ls.update_document("foo.qs", 3, "namespace Foo { ab", "qsharp"); + // Nothing further arrives. Closing the channel is what lets `run()` return + // instead of waiting forever once this batch has been applied. + ls.stop_updates(); } std::future::ready(()) }; @@ -420,8 +417,8 @@ async fn run_coalesces_updates_delivered_while_yielding() { // the document as it actually stands. assert_eq!(applied, vec![Some(3)]); - // One yield to pick up the backlog, one to discover there is nothing left. - assert_eq!(yields.get(), 2); + // The handler yields exactly once per batch, and everything landed in one batch. + assert_eq!(yields.get(), 1); } /// Coalescing must not drop updates that aren't redundant with each other. @@ -437,11 +434,10 @@ async fn run_applies_updates_to_distinct_documents() { let ls = RefCell::new(ls); let yields = Cell::new(0); let yield_to_host = || { - match yields.replace(yields.get() + 1) { - 0 => ls - .borrow_mut() - .update_document("bar.qs", 1, "namespace Bar { ", "qsharp"), - _ => ls.borrow_mut().stop_updates(), + if yields.replace(yields.get() + 1) == 0 { + let mut ls = ls.borrow_mut(); + ls.update_document("bar.qs", 1, "namespace Bar { ", "qsharp"); + ls.stop_updates(); } std::future::ready(()) }; From 37544854ce37eb81d204e33a6da5a867ccf7221d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 4 Aug 2026 10:33:13 -0700 Subject: [PATCH 13/13] Revert debugging change --- source/wasm/src/language_service.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index 1c42010bbed..824d97e8698 100644 --- a/source/wasm/src/language_service.rs +++ b/source/wasm/src/language_service.rs @@ -259,7 +259,7 @@ impl LanguageService { pub fn get_completions(&self, uri: &str, position: IPosition) -> ICompletionList { let position: Position = position.into(); let completion_list = self.0.get_completions(uri, position.into()); - let result: ICompletionList = CompletionList { + CompletionList { items: completion_list .items .into_iter() @@ -291,8 +291,7 @@ impl LanguageService { }) .collect(), } - .into(); - result + .into() } pub fn get_definition(&self, uri: &str, position: IPosition) -> Option {