diff --git a/source/language_service/src/lib.rs b/source/language_service/src/lib.rs index ad0426ddbef..615838db0ec 100644 --- a/source/language_service/src/lib.rs +++ b/source/language_service/src/lib.rs @@ -23,6 +23,7 @@ mod tests; 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::{ @@ -35,7 +36,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. @@ -48,6 +53,62 @@ 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`]. + 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 VersionWaitResult { + /// 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, + /// 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 +/// 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(); + 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) + } + + 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 { @@ -57,6 +118,7 @@ impl LanguageService { position_encoding, state: Rc::default(), state_updater: Option::default(), + version_waiters: Rc::default(), } } @@ -64,7 +126,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 +146,7 @@ impl LanguageService { self.position_encoding, ), recv, + version_waiters: self.version_waiters.clone(), }; self.state_updater = Some(send); handler @@ -183,6 +246,49 @@ impl LanguageService { }); } + /// Waits until the compilation state reflects exactly `version` of `uri`, reporting + /// [`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 + /// 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 + 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 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 VersionWaitResult::Never, + }, + } + }; + + if receiver.await.is_err() { + // The update handler shut down while we were parked. + return VersionWaitResult::Never; + } + } + } + } + #[must_use] pub fn get_code_actions(&self, uri: &str, range: Range) -> Vec { self.document_op( @@ -336,6 +442,7 @@ impl LanguageService { pub struct UpdateHandler<'a> { updater: CompilationStateUpdater<'a>, recv: UnboundedReceiver, + version_waiters: Rc, } impl UpdateHandler<'_> { @@ -346,10 +453,38 @@ 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]; + + // 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); + + // 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"); + } + + 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(); } /// Convenience method to apply *only* the pending updates @@ -361,19 +496,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 +536,17 @@ 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. + self.version_waiters.wake_all(); + } +} + +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(); } } @@ -487,3 +645,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/state.rs b/source/language_service/src/state.rs index df573131707..41ed2bb609e 100644 --- a/source/language_service/src/state.rs +++ b/source/language_service/src/state.rs @@ -677,6 +677,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 diff --git a/source/language_service/src/tests.rs b/source/language_service/src/tests.rs index 27d0cef67a1..63e04ff12c4 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, VersionWaitResult, + 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; @@ -18,11 +22,11 @@ async fn single_document() { 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); + let mut update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); ls.update_document("foo.qs", 1, "namespace Foo { }", "qsharp"); - worker.apply_pending().await; + update_handler.apply_pending().await; check_errors_and_compilation( &ls, @@ -53,11 +57,11 @@ async fn single_document_update() { 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); + let mut update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); ls.update_document("foo.qs", 1, "namespace Foo { }", "qsharp"); - worker.apply_pending().await; + update_handler.apply_pending().await; check_errors_and_compilation( &ls, @@ -89,7 +93,7 @@ async fn single_document_update() { "qsharp", ); - worker.apply_pending().await; + update_handler.apply_pending().await; check_errors_and_compilation( &ls, @@ -120,7 +124,7 @@ async fn document_in_project() { 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); + let mut update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); ls.update_document("project/src/this_file.qs", 1, "namespace Foo { }", "qsharp"); @@ -134,7 +138,7 @@ async fn document_in_project() { ); // now process background work - worker.apply_pending().await; + update_handler.apply_pending().await; check_errors_and_compilation( &ls, @@ -174,7 +178,7 @@ async fn completions_requested_before_document_load() { let errors = RefCell::new(Vec::new()); let test_cases = RefCell::new(Vec::new()); let mut ls = LanguageService::new(Encoding::Utf8); - let _worker = create_update_handler(&mut ls, &errors, &test_cases); + let _update_handler = create_update_handler(&mut ls, &errors, &test_cases); ls.update_document( "foo.qs", @@ -205,7 +209,7 @@ async fn completions_requested_after_document_load() { let 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, &errors, &test_cases); + let mut update_handler = create_update_handler(&mut ls, &errors, &test_cases); // this test is a contrast to `completions_requested_before_document_load` // we want to ensure that completions load when the update_document call has been awaited @@ -216,7 +220,7 @@ async fn completions_requested_after_document_load() { "qsharp" ); - worker.apply_pending().await; + update_handler.apply_pending().await; assert!( &ls.get_completions( @@ -293,7 +297,7 @@ async fn package_aware_foreign_fir_transform_diagnostic() { ))); let diagnostics = RefCell::new(Vec::<(String, compile::Error)>::new()); let mut ls = LanguageService::new(Encoding::Utf8); - let mut worker = ls.create_update_handler( + let mut update_handler = ls.create_update_handler( |update: DiagnosticUpdate| { diagnostics .borrow_mut() @@ -309,7 +313,7 @@ async fn package_aware_foreign_fir_transform_diagnostic() { ); ls.update_document("project/src/main.qs", 1, user_source, "qsharp"); - worker.apply_pending().await; + update_handler.apply_pending().await; let diagnostics = diagnostics.borrow(); let [(uri, error)] = diagnostics.as_slice() else { @@ -367,6 +371,274 @@ 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 update_handler = 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); + // Mock the function that would be created by createHostYield + let yield_to_host = || { + // The first time the update handler yields, simulate two more updates coming in before it resumes + 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(()) + }; + + update_handler.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)]); + + // 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. +#[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 update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); + + // Deliberately erroneous code so we'll see an error from this file if this code + // is included in the compilation + ls.update_document("foo.qs", 1, "namespace Foo { ", "qsharp"); + + let ls = RefCell::new(ls); + let yields = Cell::new(0); + let yield_to_host = || { + if yields.replace(yields.get() + 1) == 0 { + // Simulate the arrival of another update while the handler is yielding + let mut ls = ls.borrow_mut(); + ls.update_document("bar.qs", 1, "namespace Bar { ", "qsharp"); + ls.stop_updates(); + } + std::future::ready(()) + }; + + update_handler.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"]); +} + +#[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 update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 1, "namespace Foo { }", "qsharp"); + update_handler.apply_pending().await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWaitResult::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 update_handler = 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" + ); + + update_handler.apply_pending().await; + + assert_eq!(wait.await, VersionWaitResult::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 update_handler = 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"); + update_handler.apply_pending().await; + + assert_eq!(wait.await, VersionWaitResult::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 update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.update_document("foo.qs", 2, "namespace Foo { }", "qsharp"); + update_handler.apply_pending().await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWaitResult::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] +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 update_handler = 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, update_handler.run(|| std::future::ready(()))).await; + + assert_eq!(result, VersionWaitResult::Never); +} + +/// 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 update_handler = create_update_handler(&mut ls, &received_errors, &test_cases); + + ls.stop_updates(); + update_handler.run(|| std::future::ready(())).await; + + assert_eq!( + ls.wait_for_document_version("foo.qs", 1).await, + VersionWaitResult::Never + ); +} + +#[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/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..4e7e3eb7de0 100644 --- a/source/npm/qsharp/src/language-service/language-service.ts +++ b/source/npm/qsharp/src/language-service/language-service.ts @@ -47,6 +47,25 @@ 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. 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; + // 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 +91,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 +141,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 +197,7 @@ export class QSharpLanguageService implements ILanguageService { this.onDiagnostics.bind(this), this.onTestCallables.bind(this), host, + createHostYield(), ); } @@ -192,16 +245,32 @@ 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)); - return this.languageService.get_completions(documentUri, position); + ): Promise { + // 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, + completionWaitTimeoutMs, + ); + + const completions: CompletionListResult = + this.languageService.get_completions(documentUri, position); + + if (status !== "ready") { + log.info( + `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). + completions.isIncomplete = true; + } + + return completions; } async getFormatChanges(documentUri: string): Promise { 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..9e9c11f0f47 100644 --- a/source/playground/src/main.tsx +++ b/source/playground/src/main.tsx @@ -308,9 +308,11 @@ function registerMonacoLanguageServiceProviders( ) => { const completions = await languageService.getCompletions( model.uri.toString(), + model.getVersionId(), monacoPositionToLsPosition(position), ); return { + incomplete: completions.isIncomplete, suggestions: completions.items.map((i) => { let kind; switch (i.kind) { diff --git a/source/vscode/src/language-service/activate.ts b/source/vscode/src/language-service/activate.ts index a31e25e784b..cca7c5122a7 100644 --- a/source/vscode/src/language-service/activate.ts +++ b/source/vscode/src/language-service/activate.ts @@ -173,6 +173,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 +181,7 @@ async function loadLanguageService( resolvePath: async (a, b) => resolvePath(a, b), fetchGithub: fetchGithubRaw, }); + await updateLanguageServiceConfiguration(languageService); const end = performance.now(); sendTelemetryEvent( 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/wasm/src/language_service.rs b/source/wasm/src/language_service.rs index da05212cad1..824d97e8698 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::VersionWaitResult; use qsls::protocol::{DiagnosticUpdate, TestCallable, TestCallables}; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; @@ -20,6 +21,20 @@ 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] pub struct LanguageService(qsls::LanguageService); @@ -31,11 +46,15 @@ impl LanguageService { 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 +113,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()) }) } @@ -185,6 +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. + /// + /// 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, + ) -> PromiseVersionWaitStatus { + 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((VersionWaitResult::Ready, _)) => "ready", + 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}" + ); + "timeout" + } + }; + Ok(JsValue::from_str(result)) + }) + .unchecked_into() + } + 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());