From 99ce5d3dc8cc54a311ae2ec330a02602806b4d70 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 28 Apr 2024 16:19:55 +0200 Subject: [PATCH] Don't retry position relient requests and version resolve data --- crates/rust-analyzer/src/dispatch.rs | 56 ++-------- crates/rust-analyzer/src/global_state.rs | 4 + crates/rust-analyzer/src/handlers/request.rs | 39 ++++--- crates/rust-analyzer/src/lsp/ext.rs | 3 + crates/rust-analyzer/src/lsp/from_proto.rs | 19 ++-- crates/rust-analyzer/src/lsp/to_proto.rs | 12 +- crates/rust-analyzer/src/main_loop.rs | 110 +++++++++---------- 7 files changed, 115 insertions(+), 128 deletions(-) diff --git a/crates/rust-analyzer/src/dispatch.rs b/crates/rust-analyzer/src/dispatch.rs index 7adaef4ff6ec..cf3b8d331dea 100644 --- a/crates/rust-analyzer/src/dispatch.rs +++ b/crates/rust-analyzer/src/dispatch.rs @@ -95,45 +95,8 @@ impl RequestDispatcher<'_> { self } - /// Dispatches a non-latency-sensitive request onto the thread pool - /// without retrying it if it panics. - pub(crate) fn on_no_retry( - &mut self, - f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result, - ) -> &mut Self - where - R: lsp_types::request::Request + 'static, - R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug, - R::Result: Serialize, - { - let (req, params, panic_context) = match self.parse::() { - Some(it) => it, - None => return self, - }; - - self.global_state.task_pool.handle.spawn(ThreadIntent::Worker, { - let world = self.global_state.snapshot(); - move || { - let result = panic::catch_unwind(move || { - let _pctx = stdx::panic_context::enter(panic_context); - f(world, params) - }); - match thread_result_to_response::(req.id.clone(), result) { - Ok(response) => Task::Response(response), - Err(_) => Task::Response(lsp_server::Response::new_err( - req.id, - lsp_server::ErrorCode::ContentModified as i32, - "content modified".to_owned(), - )), - } - } - }); - - self - } - /// Dispatches a non-latency-sensitive request onto the thread pool. - pub(crate) fn on( + pub(crate) fn on( &mut self, f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result, ) -> &mut Self @@ -142,11 +105,11 @@ impl RequestDispatcher<'_> { R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug, R::Result: Serialize, { - self.on_with_thread_intent::(ThreadIntent::Worker, f) + self.on_with_thread_intent::(ThreadIntent::Worker, f) } /// Dispatches a latency-sensitive request onto the thread pool. - pub(crate) fn on_latency_sensitive( + pub(crate) fn on_latency_sensitive( &mut self, f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result, ) -> &mut Self @@ -155,7 +118,7 @@ impl RequestDispatcher<'_> { R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug, R::Result: Serialize, { - self.on_with_thread_intent::(ThreadIntent::LatencySensitive, f) + self.on_with_thread_intent::(ThreadIntent::LatencySensitive, f) } /// Formatting requests should never block on waiting a for task thread to open up, editors will wait @@ -170,7 +133,7 @@ impl RequestDispatcher<'_> { R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug, R::Result: Serialize, { - self.on_with_thread_intent::(ThreadIntent::LatencySensitive, f) + self.on_with_thread_intent::(ThreadIntent::LatencySensitive, f) } pub(crate) fn finish(&mut self) { @@ -185,7 +148,7 @@ impl RequestDispatcher<'_> { } } - fn on_with_thread_intent( + fn on_with_thread_intent( &mut self, intent: ThreadIntent, f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result, @@ -215,7 +178,12 @@ impl RequestDispatcher<'_> { }); match thread_result_to_response::(req.id.clone(), result) { Ok(response) => Task::Response(response), - Err(_) => Task::Retry(req), + Err(_cancelled) if ALLOW_RETRYING => Task::Retry(req), + Err(_cancelled) => Task::Response(lsp_server::Response::new_err( + req.id, + lsp_server::ErrorCode::ContentModified as i32, + "content modified".to_owned(), + )), } }); diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index 7810979c8ae4..f5b3ef51035d 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/crates/rust-analyzer/src/global_state.rs @@ -488,6 +488,10 @@ impl GlobalStateSnapshot { Ok(res) } + pub(crate) fn file_version(&self, file_id: FileId) -> Option { + Some(self.mem_docs.get(self.vfs_read().file_path(file_id))?.version) + } + pub(crate) fn url_file_version(&self, url: &Url) -> Option { let path = from_proto::vfs_path(url).ok()?; Some(self.mem_docs.get(&path)?.version) diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs index b61a968af14a..0cdb57e74fa7 100644 --- a/crates/rust-analyzer/src/handlers/request.rs +++ b/crates/rust-analyzer/src/handlers/request.rs @@ -954,8 +954,13 @@ pub(crate) fn handle_completion( }; let line_index = snap.file_line_index(position.file_id)?; - let items = - to_proto::completion_items(&snap.config, &line_index, text_document_position, items); + let items = to_proto::completion_items( + &snap.config, + &line_index, + snap.file_version(position.file_id), + text_document_position, + items, + ); let completion_list = lsp_types::CompletionList { is_incomplete: true, items }; Ok(Some(completion_list.into())) @@ -1240,8 +1245,11 @@ pub(crate) fn handle_code_action( frange, )?; for (index, assist) in assists.into_iter().enumerate() { - let resolve_data = - if code_action_resolve_cap { Some((index, params.clone())) } else { None }; + let resolve_data = if code_action_resolve_cap { + Some((index, params.clone(), snap.file_version(file_id))) + } else { + None + }; let code_action = to_proto::code_action(&snap, assist, resolve_data)?; // Check if the client supports the necessary `ResourceOperation`s. @@ -1280,12 +1288,14 @@ pub(crate) fn handle_code_action_resolve( mut code_action: lsp_ext::CodeAction, ) -> anyhow::Result { let _p = tracing::span!(tracing::Level::INFO, "handle_code_action_resolve").entered(); - let params = match code_action.data.take() { - Some(it) => it, - None => return Err(invalid_params_error("code action without data".to_owned()).into()), + let Some(params) = code_action.data.take() else { + return Err(invalid_params_error("code action without data".to_owned()).into()); }; let file_id = from_proto::file_id(&snap, ¶ms.code_action_params.text_document.uri)?; + if snap.file_version(file_id) != params.version { + return Err(invalid_params_error("stale code action".to_owned()).into()); + } let line_index = snap.file_line_index(file_id)?; let range = from_proto::text_range(&line_index, params.code_action_params.range)?; let frange = FileRange { file_id, range }; @@ -1411,12 +1421,11 @@ pub(crate) fn handle_code_lens( pub(crate) fn handle_code_lens_resolve( snap: GlobalStateSnapshot, - code_lens: CodeLens, + mut code_lens: CodeLens, ) -> anyhow::Result { - if code_lens.data.is_none() { - return Ok(code_lens); - } - let Some(annotation) = from_proto::annotation(&snap, code_lens.clone())? else { + let Some(data) = code_lens.data.take() else { return Ok(code_lens) }; + let resolve = serde_json::from_value::(data)?; + let Some(annotation) = from_proto::annotation(&snap, code_lens.range, resolve)? else { return Ok(code_lens); }; let annotation = snap.analysis.resolve_annotation(annotation)?; @@ -1522,8 +1531,12 @@ pub(crate) fn handle_inlay_hints_resolve( let Some(data) = original_hint.data.take() else { return Ok(original_hint) }; let resolve_data: lsp_ext::InlayHintResolveData = serde_json::from_value(data)?; - let Some(hash) = resolve_data.hash.parse().ok() else { return Ok(original_hint) }; let file_id = FileId::from_raw(resolve_data.file_id); + if resolve_data.version != snap.file_version(file_id) { + tracing::warn!("Inlay hint resolve data is outdated"); + return Ok(original_hint); + } + let Some(hash) = resolve_data.hash.parse().ok() else { return Ok(original_hint) }; anyhow::ensure!(snap.file_exists(file_id), "Invalid LSP resolve data"); let line_index = snap.file_line_index(file_id)?; diff --git a/crates/rust-analyzer/src/lsp/ext.rs b/crates/rust-analyzer/src/lsp/ext.rs index e189dbbbd673..2cf9b53f7c8d 100644 --- a/crates/rust-analyzer/src/lsp/ext.rs +++ b/crates/rust-analyzer/src/lsp/ext.rs @@ -561,6 +561,7 @@ pub struct CodeAction { pub struct CodeActionData { pub code_action_params: lsp_types::CodeActionParams, pub id: String, + pub version: Option, } #[derive(Debug, Eq, PartialEq, Clone, Default, Deserialize, Serialize)] @@ -802,6 +803,7 @@ impl Request for OnTypeFormatting { pub struct CompletionResolveData { pub position: lsp_types::TextDocumentPositionParams, pub imports: Vec, + pub version: Option, } #[derive(Debug, Serialize, Deserialize)] @@ -809,6 +811,7 @@ pub struct InlayHintResolveData { pub file_id: u32, // This is a string instead of a u64 as javascript can't represent u64 fully pub hash: String, + pub version: Option, } #[derive(Debug, Serialize, Deserialize)] diff --git a/crates/rust-analyzer/src/lsp/from_proto.rs b/crates/rust-analyzer/src/lsp/from_proto.rs index f42985a91610..b6b20296d80a 100644 --- a/crates/rust-analyzer/src/lsp/from_proto.rs +++ b/crates/rust-analyzer/src/lsp/from_proto.rs @@ -9,10 +9,8 @@ use syntax::{TextRange, TextSize}; use vfs::AbsPathBuf; use crate::{ - from_json, global_state::GlobalStateSnapshot, line_index::{LineIndex, PositionEncoding}, - lsp::utils::invalid_params_error, lsp_ext, }; @@ -105,16 +103,13 @@ pub(crate) fn assist_kind(kind: lsp_types::CodeActionKind) -> Option pub(crate) fn annotation( snap: &GlobalStateSnapshot, - code_lens: lsp_types::CodeLens, + range: lsp_types::Range, + data: lsp_ext::CodeLensResolveData, ) -> anyhow::Result> { - let data = - code_lens.data.ok_or_else(|| invalid_params_error("code lens without data".to_owned()))?; - let resolve = from_json::("CodeLensResolveData", &data)?; - - match resolve.kind { + match data.kind { lsp_ext::CodeLensResolveDataKind::Impls(params) => { if snap.url_file_version(¶ms.text_document_position_params.text_document.uri) - != Some(resolve.version) + != Some(data.version) { return Ok(None); } @@ -123,19 +118,19 @@ pub(crate) fn annotation( let line_index = snap.file_line_index(file_id)?; Ok(Annotation { - range: text_range(&line_index, code_lens.range)?, + range: text_range(&line_index, range)?, kind: AnnotationKind::HasImpls { pos, data: None }, }) } lsp_ext::CodeLensResolveDataKind::References(params) => { - if snap.url_file_version(¶ms.text_document.uri) != Some(resolve.version) { + if snap.url_file_version(¶ms.text_document.uri) != Some(data.version) { return Ok(None); } let pos @ FilePosition { file_id, .. } = file_position(snap, params)?; let line_index = snap.file_line_index(file_id)?; Ok(Annotation { - range: text_range(&line_index, code_lens.range)?, + range: text_range(&line_index, range)?, kind: AnnotationKind::HasReferences { pos, data: None }, }) } diff --git a/crates/rust-analyzer/src/lsp/to_proto.rs b/crates/rust-analyzer/src/lsp/to_proto.rs index d02f4612dc1e..03daccc99c45 100644 --- a/crates/rust-analyzer/src/lsp/to_proto.rs +++ b/crates/rust-analyzer/src/lsp/to_proto.rs @@ -225,13 +225,14 @@ pub(crate) fn snippet_text_edit_vec( pub(crate) fn completion_items( config: &Config, line_index: &LineIndex, + version: Option, tdpp: lsp_types::TextDocumentPositionParams, items: Vec, ) -> Vec { let max_relevance = items.iter().map(|it| it.relevance.score()).max().unwrap_or_default(); let mut res = Vec::with_capacity(items.len()); for item in items { - completion_item(&mut res, config, line_index, &tdpp, max_relevance, item); + completion_item(&mut res, config, line_index, version, &tdpp, max_relevance, item); } if let Some(limit) = config.completion(None).limit { @@ -246,6 +247,7 @@ fn completion_item( acc: &mut Vec, config: &Config, line_index: &LineIndex, + version: Option, tdpp: &lsp_types::TextDocumentPositionParams, max_relevance: u32, item: CompletionItem, @@ -328,7 +330,7 @@ fn completion_item( }) .collect::>(); if !imports.is_empty() { - let data = lsp_ext::CompletionResolveData { position: tdpp.clone(), imports }; + let data = lsp_ext::CompletionResolveData { position: tdpp.clone(), imports, version }; lsp_item.data = Some(to_value(data).unwrap()); } } @@ -483,6 +485,7 @@ pub(crate) fn inlay_hint( to_value(lsp_ext::InlayHintResolveData { file_id: file_id.index(), hash: hash.to_string(), + version: snap.file_version(file_id), }) .unwrap(), ), @@ -1318,7 +1321,7 @@ pub(crate) fn code_action_kind(kind: AssistKind) -> lsp_types::CodeActionKind { pub(crate) fn code_action( snap: &GlobalStateSnapshot, assist: Assist, - resolve_data: Option<(usize, lsp_types::CodeActionParams)>, + resolve_data: Option<(usize, lsp_types::CodeActionParams, Option)>, ) -> Cancellable { let mut res = lsp_ext::CodeAction { title: assist.label.to_string(), @@ -1336,10 +1339,11 @@ pub(crate) fn code_action( match (assist.source_change, resolve_data) { (Some(it), _) => res.edit = Some(snippet_workspace_edit(snap, it)?), - (None, Some((index, code_action_params))) => { + (None, Some((index, code_action_params, version))) => { res.data = Some(lsp_ext::CodeActionData { id: format!("{}:{}:{index}", assist.id.0, assist.id.1.name()), code_action_params, + version, }); } (None, None) => { diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index 95b3d3ecb3c4..9c1d955dd7d3 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -901,6 +901,10 @@ impl GlobalState { use crate::handlers::request as handlers; use lsp_types::request as lsp_request; + const RETRY: bool = true; + const NO_RETRY: bool = false; + + #[rustfmt::skip] dispatcher // Request handlers that must run on the main thread // because they mutate GlobalState: @@ -926,62 +930,58 @@ impl GlobalState { // analysis on the main thread because that would block other // requests. Instead, we run these request handlers on higher priority // threads in the threadpool. - .on_latency_sensitive::(handlers::handle_completion) - .on_latency_sensitive::( - handlers::handle_completion_resolve, - ) - .on_latency_sensitive::( - handlers::handle_semantic_tokens_full, - ) - .on_latency_sensitive::( - handlers::handle_semantic_tokens_full_delta, - ) - .on_latency_sensitive::( - handlers::handle_semantic_tokens_range, - ) + // FIXME: Retrying can make the result of this stale? + .on_latency_sensitive::(handlers::handle_completion) + // FIXME: Retrying can make the result of this stale + .on_latency_sensitive::(handlers::handle_completion_resolve) + .on_latency_sensitive::(handlers::handle_semantic_tokens_full) + .on_latency_sensitive::(handlers::handle_semantic_tokens_full_delta) + .on_latency_sensitive::(handlers::handle_semantic_tokens_range) + // FIXME: Some of these NO_RETRY could be retries if the file they are interested didn't change. // All other request handlers - .on::(handlers::fetch_dependency_list) - .on::(handlers::handle_analyzer_status) - .on::(handlers::handle_syntax_tree) - .on::(handlers::handle_view_hir) - .on::(handlers::handle_view_mir) - .on::(handlers::handle_interpret_function) - .on::(handlers::handle_view_file_text) - .on::(handlers::handle_view_crate_graph) - .on::(handlers::handle_view_item_tree) - .on::(handlers::handle_discover_test) - .on::(handlers::handle_expand_macro) - .on::(handlers::handle_parent_module) - .on::(handlers::handle_runnables) - .on::(handlers::handle_related_tests) - .on::(handlers::handle_code_action) - .on::(handlers::handle_code_action_resolve) - .on::(handlers::handle_hover) - .on::(handlers::handle_open_docs) - .on::(handlers::handle_open_cargo_toml) - .on::(handlers::handle_move_item) - .on::(handlers::handle_workspace_symbol) - .on::(handlers::handle_document_symbol) - .on::(handlers::handle_goto_definition) - .on::(handlers::handle_goto_declaration) - .on::(handlers::handle_goto_implementation) - .on::(handlers::handle_goto_type_definition) - .on_no_retry::(handlers::handle_inlay_hints) - .on::(handlers::handle_inlay_hints_resolve) - .on::(handlers::handle_code_lens) - .on::(handlers::handle_code_lens_resolve) - .on::(handlers::handle_folding_range) - .on::(handlers::handle_signature_help) - .on::(handlers::handle_prepare_rename) - .on::(handlers::handle_rename) - .on::(handlers::handle_references) - .on::(handlers::handle_document_highlight) - .on::(handlers::handle_call_hierarchy_prepare) - .on::(handlers::handle_call_hierarchy_incoming) - .on::(handlers::handle_call_hierarchy_outgoing) - .on::(handlers::handle_will_rename_files) - .on::(handlers::handle_ssr) - .on::(handlers::handle_view_recursive_memory_layout) + .on::(handlers::handle_document_symbol) + .on::(handlers::handle_folding_range) + .on::(handlers::handle_signature_help) + .on::(handlers::handle_will_rename_files) + .on::(handlers::handle_goto_definition) + .on::(handlers::handle_goto_declaration) + .on::(handlers::handle_goto_implementation) + .on::(handlers::handle_goto_type_definition) + .on::(handlers::handle_inlay_hints) + .on::(handlers::handle_inlay_hints_resolve) + .on::(handlers::handle_code_lens) + .on::(handlers::handle_code_lens_resolve) + .on::(handlers::handle_prepare_rename) + .on::(handlers::handle_rename) + .on::(handlers::handle_references) + .on::(handlers::handle_document_highlight) + .on::(handlers::handle_call_hierarchy_prepare) + .on::(handlers::handle_call_hierarchy_incoming) + .on::(handlers::handle_call_hierarchy_outgoing) + // All other request handlers (lsp extension) + .on::(handlers::fetch_dependency_list) + .on::(handlers::handle_analyzer_status) + .on::(handlers::handle_view_file_text) + .on::(handlers::handle_view_crate_graph) + .on::(handlers::handle_view_item_tree) + .on::(handlers::handle_discover_test) + .on::(handlers::handle_workspace_symbol) + .on::(handlers::handle_ssr) + .on::(handlers::handle_view_recursive_memory_layout) + .on::(handlers::handle_syntax_tree) + .on::(handlers::handle_view_hir) + .on::(handlers::handle_view_mir) + .on::(handlers::handle_interpret_function) + .on::(handlers::handle_expand_macro) + .on::(handlers::handle_parent_module) + .on::(handlers::handle_runnables) + .on::(handlers::handle_related_tests) + .on::(handlers::handle_code_action) + .on::(handlers::handle_code_action_resolve) + .on::(handlers::handle_hover) + .on::(handlers::handle_open_docs) + .on::(handlers::handle_open_cargo_toml) + .on::(handlers::handle_move_item) .finish(); }