From 05aa5b854b230c2f6ba182c0f2c94f3de9a47204 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Thu, 23 Jan 2020 09:26:08 +0100 Subject: Remove RWLock from check watcher. @matklad mentioned this might be a good idea. So the general idea is that we don't really need the lock, as we can just clone the check watcher state when creating a snapshot. We can then use `Arc::get_mut` to get mutable access to the state from `WorldState` when needed. Running with this it seems to improve responsiveness a bit while cargo is running, but I have no hard numbers to prove it. In any case, a serialization point less is always better when we're trying to be responsive. --- crates/ra_lsp_server/src/main_loop.rs | 14 +++++++------- crates/ra_lsp_server/src/main_loop/handlers.rs | 5 ++--- crates/ra_lsp_server/src/world.rs | 4 ++-- 3 files changed, 11 insertions(+), 12 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 7822be2e2..746a8fbe9 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -586,12 +586,14 @@ fn on_notification( fn on_check_task( task: CheckTask, - world_state: &WorldState, + world_state: &mut WorldState, task_sender: &Sender, ) -> Result<()> { match task { CheckTask::ClearDiagnostics => { - let cleared_files = world_state.check_watcher.state.write().clear(); + let state = Arc::get_mut(&mut world_state.check_watcher.state) + .expect("couldn't get check watcher state as mutable"); + let cleared_files = state.clear(); // Send updated diagnostics for each cleared file for url in cleared_files { @@ -600,11 +602,9 @@ fn on_check_task( } CheckTask::AddDiagnostic(url, diagnostic) => { - world_state - .check_watcher - .state - .write() - .add_diagnostic_with_fixes(url.clone(), diagnostic); + let state = Arc::get_mut(&mut world_state.check_watcher.state) + .expect("couldn't get check watcher state as mutable"); + state.add_diagnostic_with_fixes(url.clone(), diagnostic); // We manually send a diagnostic update when the watcher asks // us to, to avoid the issue of having to change the file to diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs index 8e43f0575..666f2ee29 100644 --- a/crates/ra_lsp_server/src/main_loop/handlers.rs +++ b/crates/ra_lsp_server/src/main_loop/handlers.rs @@ -674,8 +674,7 @@ pub fn handle_code_action( res.push(action.into()); } - for fix in world.check_watcher.read().fixes_for(¶ms.text_document.uri).into_iter().flatten() - { + for fix in world.check_watcher.fixes_for(¶ms.text_document.uri).into_iter().flatten() { let fix_range = fix.location.range.conv_with(&line_index); if fix_range.intersection(&range).is_none() { continue; @@ -895,7 +894,7 @@ pub fn publish_diagnostics( tags: None, }) .collect(); - if let Some(check_diags) = world.check_watcher.read().diagnostics_for(&uri) { + if let Some(check_diags) = world.check_watcher.diagnostics_for(&uri) { diagnostics.extend(check_diags.iter().cloned()); } Ok(req::PublishDiagnosticsParams { uri, diagnostics, version: None }) diff --git a/crates/ra_lsp_server/src/world.rs b/crates/ra_lsp_server/src/world.rs index e7a0acfc7..3059ef9ec 100644 --- a/crates/ra_lsp_server/src/world.rs +++ b/crates/ra_lsp_server/src/world.rs @@ -63,7 +63,7 @@ pub struct WorldSnapshot { pub workspaces: Arc>, pub analysis: Analysis, pub latest_requests: Arc>, - pub check_watcher: Arc>, + pub check_watcher: CheckState, vfs: Arc>, } @@ -220,7 +220,7 @@ impl WorldState { analysis: self.analysis_host.analysis(), vfs: Arc::clone(&self.vfs), latest_requests: Arc::clone(&self.latest_requests), - check_watcher: self.check_watcher.state.clone(), + check_watcher: (*self.check_watcher.state).clone(), } } -- cgit v1.2.3 From b90ea640e6484788f8728be6e48cc55d90e488ba Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 24 Jan 2020 16:35:37 +0100 Subject: Cancel requests during shutdown --- crates/ra_lsp_server/src/main_loop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 746a8fbe9..83adf9711 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -210,7 +210,7 @@ pub fn main_loop( )?; } } - + world_state.analysis_host.request_cancellation(); log::info!("waiting for tasks to finish..."); task_receiver.into_iter().for_each(|task| { on_task(task, &connection.sender, &mut loop_state.pending_requests, &mut world_state) -- cgit v1.2.3 From f44aee27d31ca331fee8993152c42875b3364711 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 25 Jan 2020 11:53:40 +0100 Subject: Disable env_logger humantime feature We rarely care about timings of events, and, when we care, we need millisecond precision --- crates/ra_lsp_server/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/Cargo.toml b/crates/ra_lsp_server/Cargo.toml index 4ee3fb49f..5df0496dd 100644 --- a/crates/ra_lsp_server/Cargo.toml +++ b/crates/ra_lsp_server/Cargo.toml @@ -26,7 +26,7 @@ lsp-server = "0.3.0" ra_project_model = { path = "../ra_project_model" } ra_prof = { path = "../ra_prof" } ra_vfs_glob = { path = "../ra_vfs_glob" } -env_logger = { version = "0.7.1", default-features = false, features = ["humantime"] } +env_logger = { version = "0.7.1", default-features = false } ra_cargo_watch = { path = "../ra_cargo_watch" } either = "1.5" -- cgit v1.2.3 From 40109941db20180eb71b70c23c578fed5244bd74 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 25 Jan 2020 13:27:36 +0100 Subject: Use default threadpool size --- crates/ra_lsp_server/src/main_loop.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 83adf9711..315f4a4d6 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -29,9 +29,6 @@ use crate::{ Result, ServerConfig, }; -const THREADPOOL_SIZE: usize = 8; -const MAX_IN_FLIGHT_LIBS: usize = THREADPOOL_SIZE - 3; - #[derive(Debug)] pub struct LspError { pub code: i32, @@ -168,7 +165,7 @@ pub fn main_loop( ) }; - let pool = ThreadPool::new(THREADPOOL_SIZE); + let pool = ThreadPool::default(); let (task_sender, task_receiver) = unbounded::(); let (libdata_sender, libdata_receiver) = unbounded::(); @@ -371,7 +368,8 @@ fn loop_turn( loop_state.pending_libraries.extend(changes); } - while loop_state.in_flight_libraries < MAX_IN_FLIGHT_LIBS + let max_in_flight_libs = pool.max_count().saturating_sub(2).max(1); + while loop_state.in_flight_libraries < max_in_flight_libs && !loop_state.pending_libraries.is_empty() { let (root, files) = loop_state.pending_libraries.pop().unwrap(); -- cgit v1.2.3 From 493a903f226d148fec4b539f65b78a408e4dcb2c Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 26 Jan 2020 12:02:56 +0100 Subject: Bump main thread priority on windows --- crates/ra_lsp_server/Cargo.toml | 3 +++ crates/ra_lsp_server/src/main_loop.rs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/Cargo.toml b/crates/ra_lsp_server/Cargo.toml index 5df0496dd..fdf81ed87 100644 --- a/crates/ra_lsp_server/Cargo.toml +++ b/crates/ra_lsp_server/Cargo.toml @@ -30,6 +30,9 @@ env_logger = { version = "0.7.1", default-features = false } ra_cargo_watch = { path = "../ra_cargo_watch" } either = "1.5" +[target.'cfg(windows)'.dependencies] +winapi = "0.3" + [dev-dependencies] tempfile = "3" test_utils = { path = "../test_utils" } diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 315f4a4d6..15bf519c9 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -57,6 +57,25 @@ pub fn main_loop( ) -> Result<()> { log::info!("server_config: {:#?}", config); + // Windows scheduler implements priority boosts: if thread waits for an + // event (like a condvar), and event fires, priority of the thread is + // temporary bumped. This optimization backfires in our case: each time the + // `main_loop` schedules a task to run on a threadpool, the worker threads + // gets a higher priority, and (on a machine with fewer cores) displaces the + // main loop! We work-around this by marking the main loop as a + // higher-priority thread. + // + // https://docs.microsoft.com/en-us/windows/win32/procthread/scheduling-priorities + // https://docs.microsoft.com/en-us/windows/win32/procthread/priority-boosts + // https://github.com/rust-analyzer/rust-analyzer/issues/2835 + #[cfg(windows)] + unsafe { + use winapi::um::processthreadsapi::*; + let thread = GetCurrentThread(); + let thread_priority_above_normal = 1; + SetThreadPriority(thread, thread_priority_above_normal); + } + let mut loop_state = LoopState::default(); let mut world_state = { let feature_flags = { -- cgit v1.2.3 From 2d2585e03feeaa4a83d59a7ea2cdfbe948526840 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 29 Jan 2020 10:46:56 +0100 Subject: Don't compute diagnostics on the main thread closes #2909 --- crates/ra_lsp_server/src/main_loop.rs | 56 +++++++++++++++++------------------ 1 file changed, 28 insertions(+), 28 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 15bf519c9..52c7e2bdb 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -9,7 +9,7 @@ use std::{error::Error, fmt, panic, path::PathBuf, sync::Arc, time::Instant}; use crossbeam_channel::{select, unbounded, RecvError, Sender}; use lsp_server::{Connection, ErrorCode, Message, Notification, Request, RequestId, Response}; -use lsp_types::{ClientCapabilities, NumberOrString, Url}; +use lsp_types::{ClientCapabilities, NumberOrString}; use ra_cargo_watch::{CheckOptions, CheckTask}; use ra_ide::{Canceled, FeatureFlags, FileId, LibraryData, SourceRootId}; use ra_prof::profile; @@ -352,7 +352,7 @@ fn loop_turn( world_state.maybe_collect_garbage(); loop_state.in_flight_libraries -= 1; } - Event::CheckWatcher(task) => on_check_task(task, world_state, task_sender)?, + Event::CheckWatcher(task) => on_check_task(pool, task, world_state, task_sender)?, Event::Msg(msg) => match msg { Message::Request(req) => on_request( world_state, @@ -602,31 +602,23 @@ fn on_notification( } fn on_check_task( + pool: &ThreadPool, task: CheckTask, world_state: &mut WorldState, task_sender: &Sender, ) -> Result<()> { - match task { + let urls = match task { CheckTask::ClearDiagnostics => { let state = Arc::get_mut(&mut world_state.check_watcher.state) .expect("couldn't get check watcher state as mutable"); - let cleared_files = state.clear(); - - // Send updated diagnostics for each cleared file - for url in cleared_files { - publish_diagnostics_for_url(&url, world_state, task_sender)?; - } + state.clear() } CheckTask::AddDiagnostic(url, diagnostic) => { let state = Arc::get_mut(&mut world_state.check_watcher.state) .expect("couldn't get check watcher state as mutable"); state.add_diagnostic_with_fixes(url.clone(), diagnostic); - - // We manually send a diagnostic update when the watcher asks - // us to, to avoid the issue of having to change the file to - // receive updated diagnostics. - publish_diagnostics_for_url(&url, world_state, task_sender)?; + vec![url] } CheckTask::Status(progress) => { @@ -636,22 +628,30 @@ fn on_check_task( }; let not = notification_new::(params); task_sender.send(Task::Notify(not)).unwrap(); + Vec::new() } - } - Ok(()) -} + }; + + let subscriptions = urls + .into_iter() + .map(|url| { + let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?; + Ok(world_state.vfs.read().path2file(&path).map(|it| FileId(it.0))) + }) + .filter_map(|res| res.transpose()) + .collect::>>()?; + + // We manually send a diagnostic update when the watcher asks + // us to, to avoid the issue of having to change the file to + // receive updated diagnostics. + update_file_notifications_on_threadpool( + pool, + world_state.snapshot(), + false, + task_sender.clone(), + subscriptions, + ); -fn publish_diagnostics_for_url( - url: &Url, - world_state: &WorldState, - task_sender: &Sender, -) -> Result<()> { - let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?; - if let Some(file_id) = world_state.vfs.read().path2file(&path) { - let params = handlers::publish_diagnostics(&world_state.snapshot(), FileId(file_id.0))?; - let not = notification_new::(params); - task_sender.send(Task::Notify(not)).unwrap(); - } Ok(()) } -- cgit v1.2.3 From aaa4861a0b2f6cb2f9f271961d9836976e94b139 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 29 Jan 2020 11:15:08 +0100 Subject: More uniform naming --- crates/ra_lsp_server/src/main_loop.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 52c7e2bdb..2daaf1b4a 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -452,7 +452,7 @@ fn on_request( world: &mut WorldState, pending_requests: &mut PendingRequests, pool: &ThreadPool, - sender: &Sender, + task_sender: &Sender, msg_sender: &Sender, request_received: Instant, req: Request, @@ -461,7 +461,7 @@ fn on_request( req: Some(req), pool, world, - sender, + task_sender, msg_sender, pending_requests, request_received, @@ -661,7 +661,7 @@ struct PoolDispatcher<'a> { world: &'a mut WorldState, pending_requests: &'a mut PendingRequests, msg_sender: &'a Sender, - sender: &'a Sender, + task_sender: &'a Sender, request_received: Instant, } @@ -708,7 +708,7 @@ impl<'a> PoolDispatcher<'a> { self.pool.execute({ let world = self.world.snapshot(); - let sender = self.sender.clone(); + let sender = self.task_sender.clone(); move || { let result = f(world, params); let task = result_to_task::(id, result); @@ -786,7 +786,7 @@ fn update_file_notifications_on_threadpool( pool: &ThreadPool, world: WorldSnapshot, publish_decorations: bool, - sender: Sender, + task_sender: Sender, subscriptions: Vec, ) { log::trace!("updating notifications for {:?}", subscriptions); @@ -802,7 +802,7 @@ fn update_file_notifications_on_threadpool( } Ok(params) => { let not = notification_new::(params); - sender.send(Task::Notify(not)).unwrap(); + task_sender.send(Task::Notify(not)).unwrap(); } } } @@ -815,7 +815,7 @@ fn update_file_notifications_on_threadpool( } Ok(params) => { let not = notification_new::(params); - sender.send(Task::Notify(not)).unwrap(); + task_sender.send(Task::Notify(not)).unwrap(); } } } -- cgit v1.2.3 From 9753eb98ccbe4c5abefacde2fc60e919cf3c5645 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 29 Jan 2020 11:21:49 +0100 Subject: Complain loudly if the main loop is blocked --- crates/ra_lsp_server/src/main_loop.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 2daaf1b4a..9901fe931 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -5,7 +5,14 @@ mod handlers; mod subscriptions; pub(crate) mod pending_requests; -use std::{error::Error, fmt, panic, path::PathBuf, sync::Arc, time::Instant}; +use std::{ + env, + error::Error, + fmt, panic, + path::PathBuf, + sync::Arc, + time::{Duration, Instant}, +}; use crossbeam_channel::{select, unbounded, RecvError, Sender}; use lsp_server::{Connection, ErrorCode, Message, Notification, Request, RequestId, Response}; @@ -425,6 +432,19 @@ fn loop_turn( loop_state.subscriptions.subscriptions(), ) } + + let loop_duration = loop_start.elapsed(); + if loop_duration > Duration::from_millis(10) { + log::error!("overly long loop turn: {:?}", loop_duration); + if env::var("RA_PROFILE").is_ok() { + show_message( + req::MessageType::Error, + format!("overly long loop turn: {:?}", loop_duration), + &connection.sender, + ); + } + } + Ok(()) } -- cgit v1.2.3 From 7cc0a8652870402db3b072cab030ba28d6b96b39 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 29 Jan 2020 14:04:10 +0100 Subject: Fix long loop timeout --- crates/ra_lsp_server/src/main_loop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 9901fe931..d850ded37 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -434,7 +434,7 @@ fn loop_turn( } let loop_duration = loop_start.elapsed(); - if loop_duration > Duration::from_millis(10) { + if loop_duration > Duration::from_millis(100) { log::error!("overly long loop turn: {:?}", loop_duration); if env::var("RA_PROFILE").is_ok() { show_message( -- cgit v1.2.3 From 9d5a5211a4ab83fb891c6eb32f6ed6d63b6b027f Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jan 2020 13:34:44 +0100 Subject: Small cleanup --- crates/ra_lsp_server/src/main_loop/handlers.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs index 666f2ee29..9aa1e7eea 100644 --- a/crates/ra_lsp_server/src/main_loop/handlers.rs +++ b/crates/ra_lsp_server/src/main_loop/handlers.rs @@ -1,7 +1,11 @@ //! This module is responsible for implementing handlers for Lanuage Server Protocol. //! The majority of requests are fulfilled by calling into the `ra_ide` crate. -use std::{fmt::Write as _, io::Write as _}; +use std::{ + fmt::Write as _, + io::Write as _, + process::{self, Stdio}, +}; use either::Either; use lsp_server::ErrorCode; @@ -582,21 +586,19 @@ pub fn handle_formatting( let file_line_index = world.analysis().file_line_index(file_id)?; let end_position = TextUnit::of_str(&file).conv_with(&file_line_index); - use std::process; let mut rustfmt = process::Command::new("rustfmt"); if let Some(&crate_id) = crate_ids.first() { // Assume all crates are in the same edition let edition = world.analysis().crate_edition(crate_id)?; rustfmt.args(&["--edition", &edition.to_string()]); } - rustfmt.stdin(process::Stdio::piped()).stdout(process::Stdio::piped()); if let Ok(path) = params.text_document.uri.to_file_path() { if let Some(parent) = path.parent() { rustfmt.current_dir(parent); } } - let mut rustfmt = rustfmt.spawn()?; + let mut rustfmt = rustfmt.stdin(Stdio::piped()).stdout(Stdio::piped()).spawn()?; rustfmt.stdin.as_mut().unwrap().write_all(file.as_bytes())?; -- cgit v1.2.3 From 24ad1cce2c3cf2c0ce8288fc02c4c55529598990 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 2 Feb 2020 18:54:26 +0100 Subject: Avoid premature pessimization The extra allocation for message should not matter here at all, but using a static string is just as ergonomic, if not more, and there's no reason to write deliberately slow code --- crates/ra_lsp_server/src/main_loop.rs | 1 - 1 file changed, 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index d850ded37..508fe08c0 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -403,7 +403,6 @@ fn loop_turn( let sender = libdata_sender.clone(); pool.execute(move || { log::info!("indexing {:?} ... ", root); - let _p = profile(&format!("indexed {:?}", root)); let data = LibraryData::prepare(root, files); sender.send(data).unwrap(); }); -- cgit v1.2.3 From 790788d5f4013d8d92f110bc12a581d18cf4b6ae Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 31 Jan 2020 19:23:25 +0100 Subject: Rework how we send diagnostics to client. The previous way of sending from the thread pool suffered from stale diagnostics due to being canceled before we could clear the old ones. The key change is moving to sending diagnostics from the main loop thread, but doing all the hard work in the thread pool. This should provide the best of both worlds, with little to no of the downsides. This should hopefully fix a lot of issues, but we'll need testing in each individual issue to be sure. --- crates/ra_lsp_server/src/diagnostics.rs | 85 ++++++++++++++++++++++++++ crates/ra_lsp_server/src/lib.rs | 1 + crates/ra_lsp_server/src/main_loop.rs | 76 ++++++++++++----------- crates/ra_lsp_server/src/main_loop/handlers.rs | 37 +++-------- crates/ra_lsp_server/src/world.rs | 11 ++-- 5 files changed, 140 insertions(+), 70 deletions(-) create mode 100644 crates/ra_lsp_server/src/diagnostics.rs (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/diagnostics.rs b/crates/ra_lsp_server/src/diagnostics.rs new file mode 100644 index 000000000..ea08bce24 --- /dev/null +++ b/crates/ra_lsp_server/src/diagnostics.rs @@ -0,0 +1,85 @@ +//! Book keeping for keeping diagnostics easily in sync with the client. +use lsp_types::{CodeActionOrCommand, Diagnostic, Range}; +use ra_ide::FileId; +use std::{collections::HashMap, sync::Arc}; + +pub type CheckFixes = Arc>>; + +#[derive(Debug, Default, Clone)] +pub struct DiagnosticCollection { + pub native: HashMap>, + pub check: HashMap>, + pub check_fixes: CheckFixes, +} + +#[derive(Debug, Clone)] +pub struct Fix { + pub range: Range, + pub action: CodeActionOrCommand, +} + +#[derive(Debug)] +pub enum DiagnosticTask { + ClearCheck, + AddCheck(FileId, Diagnostic, Vec), + SetNative(FileId, Vec), +} + +impl DiagnosticCollection { + pub fn clear_check(&mut self) -> Vec { + Arc::make_mut(&mut self.check_fixes).clear(); + self.check.drain().map(|(key, _value)| key).collect() + } + + pub fn add_check_diagnostic( + &mut self, + file_id: FileId, + diagnostic: Diagnostic, + fixes: Vec, + ) { + let diagnostics = self.check.entry(file_id).or_default(); + for existing_diagnostic in diagnostics.iter() { + if are_diagnostics_equal(&existing_diagnostic, &diagnostic) { + return; + } + } + + let check_fixes = Arc::make_mut(&mut self.check_fixes); + check_fixes + .entry(file_id) + .or_default() + .extend(fixes.into_iter().map(|action| Fix { range: diagnostic.range, action })); + diagnostics.push(diagnostic); + } + + pub fn set_native_diagnostics(&mut self, file_id: FileId, diagnostics: Vec) { + self.native.insert(file_id, diagnostics); + } + + pub fn diagnostics_for(&self, file_id: FileId) -> impl Iterator { + let native = self.native.get(&file_id).into_iter().flatten(); + let check = self.check.get(&file_id).into_iter().flatten(); + native.chain(check) + } + + pub fn handle_task(&mut self, task: DiagnosticTask) -> Vec { + match task { + DiagnosticTask::ClearCheck => self.clear_check(), + DiagnosticTask::AddCheck(file_id, diagnostic, fixes) => { + self.add_check_diagnostic(file_id, diagnostic, fixes); + vec![file_id] + } + DiagnosticTask::SetNative(file_id, diagnostics) => { + self.set_native_diagnostics(file_id, diagnostics); + vec![file_id] + } + } + } +} + +fn are_diagnostics_equal(left: &Diagnostic, right: &Diagnostic) -> bool { + left.source == right.source + && left.severity == right.severity + && left.range == right.range + && left.message == right.message +} diff --git a/crates/ra_lsp_server/src/lib.rs b/crates/ra_lsp_server/src/lib.rs index 2ca149fd5..1208c1343 100644 --- a/crates/ra_lsp_server/src/lib.rs +++ b/crates/ra_lsp_server/src/lib.rs @@ -29,6 +29,7 @@ mod markdown; pub mod req; mod config; mod world; +mod diagnostics; pub type Result = std::result::Result>; pub use crate::{ diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 508fe08c0..12961ba37 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -17,16 +17,17 @@ use std::{ use crossbeam_channel::{select, unbounded, RecvError, Sender}; use lsp_server::{Connection, ErrorCode, Message, Notification, Request, RequestId, Response}; use lsp_types::{ClientCapabilities, NumberOrString}; -use ra_cargo_watch::{CheckOptions, CheckTask}; +use ra_cargo_watch::{url_from_path_with_drive_lowercasing, CheckOptions, CheckTask}; use ra_ide::{Canceled, FeatureFlags, FileId, LibraryData, SourceRootId}; use ra_prof::profile; -use ra_vfs::{VfsTask, Watch}; +use ra_vfs::{VfsFile, VfsTask, Watch}; use relative_path::RelativePathBuf; use rustc_hash::FxHashSet; use serde::{de::DeserializeOwned, Serialize}; use threadpool::ThreadPool; use crate::{ + diagnostics::DiagnosticTask, main_loop::{ pending_requests::{PendingRequest, PendingRequests}, subscriptions::Subscriptions, @@ -254,6 +255,7 @@ pub fn main_loop( enum Task { Respond(Response), Notify(Notification), + Diagnostic(DiagnosticTask), } enum Event { @@ -359,7 +361,7 @@ fn loop_turn( world_state.maybe_collect_garbage(); loop_state.in_flight_libraries -= 1; } - Event::CheckWatcher(task) => on_check_task(pool, task, world_state, task_sender)?, + Event::CheckWatcher(task) => on_check_task(task, world_state, task_sender)?, Event::Msg(msg) => match msg { Message::Request(req) => on_request( world_state, @@ -464,6 +466,7 @@ fn on_task( Task::Notify(n) => { msg_sender.send(n.into()).unwrap(); } + Task::Diagnostic(task) => on_diagnostic_task(task, msg_sender, state), } } @@ -621,23 +624,26 @@ fn on_notification( } fn on_check_task( - pool: &ThreadPool, task: CheckTask, world_state: &mut WorldState, task_sender: &Sender, ) -> Result<()> { - let urls = match task { + match task { CheckTask::ClearDiagnostics => { - let state = Arc::get_mut(&mut world_state.check_watcher.state) - .expect("couldn't get check watcher state as mutable"); - state.clear() + task_sender.send(Task::Diagnostic(DiagnosticTask::ClearCheck))?; } - CheckTask::AddDiagnostic(url, diagnostic) => { - let state = Arc::get_mut(&mut world_state.check_watcher.state) - .expect("couldn't get check watcher state as mutable"); - state.add_diagnostic_with_fixes(url.clone(), diagnostic); - vec![url] + CheckTask::AddDiagnostic { url, diagnostic, fixes } => { + let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?; + let file_id = world_state + .vfs + .read() + .path2file(&path) + .map(|it| FileId(it.0)) + .ok_or_else(|| format!("unknown file: {}", path.to_string_lossy()))?; + + task_sender + .send(Task::Diagnostic(DiagnosticTask::AddCheck(file_id, diagnostic, fixes)))?; } CheckTask::Status(progress) => { @@ -647,31 +653,30 @@ fn on_check_task( }; let not = notification_new::(params); task_sender.send(Task::Notify(not)).unwrap(); - Vec::new() } }; - let subscriptions = urls - .into_iter() - .map(|url| { - let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?; - Ok(world_state.vfs.read().path2file(&path).map(|it| FileId(it.0))) - }) - .filter_map(|res| res.transpose()) - .collect::>>()?; + Ok(()) +} - // We manually send a diagnostic update when the watcher asks - // us to, to avoid the issue of having to change the file to - // receive updated diagnostics. - update_file_notifications_on_threadpool( - pool, - world_state.snapshot(), - false, - task_sender.clone(), - subscriptions, - ); +fn on_diagnostic_task(task: DiagnosticTask, msg_sender: &Sender, state: &mut WorldState) { + let subscriptions = state.diagnostics.handle_task(task); - Ok(()) + for file_id in subscriptions { + let path = state.vfs.read().file2path(VfsFile(file_id.0)); + let uri = match url_from_path_with_drive_lowercasing(&path) { + Ok(uri) => uri, + Err(err) => { + log::error!("Couldn't convert path to url ({}): {:?}", err, path.to_string_lossy()); + continue; + } + }; + + let diagnostics = state.diagnostics.diagnostics_for(file_id).cloned().collect(); + let params = req::PublishDiagnosticsParams { uri, diagnostics, version: None }; + let not = notification_new::(params); + msg_sender.send(not.into()).unwrap(); + } } struct PoolDispatcher<'a> { @@ -819,9 +824,8 @@ fn update_file_notifications_on_threadpool( log::error!("failed to compute diagnostics: {:?}", e); } } - Ok(params) => { - let not = notification_new::(params); - task_sender.send(Task::Notify(not)).unwrap(); + Ok(task) => { + task_sender.send(Task::Diagnostic(task)).unwrap(); } } } diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs index 9aa1e7eea..839bca342 100644 --- a/crates/ra_lsp_server/src/main_loop/handlers.rs +++ b/crates/ra_lsp_server/src/main_loop/handlers.rs @@ -33,6 +33,7 @@ use crate::{ to_call_hierarchy_item, to_location, Conv, ConvWith, FoldConvCtx, MapConvWith, TryConvWith, TryConvWithToVec, }, + diagnostics::DiagnosticTask, req::{self, Decoration, InlayHint, InlayHintsParams, InlayKind}, world::WorldSnapshot, LspError, Result, @@ -656,6 +657,7 @@ pub fn handle_code_action( .filter(|(diag_range, _fix)| diag_range.intersection(&range).is_some()) .map(|(_range, fix)| fix); + // TODO: When done, we won't need this, only the one pulling from world.diagnostics for source_edit in fixes_from_diagnostics { let title = source_edit.label.clone(); let edit = source_edit.try_conv_with(&world)?; @@ -676,28 +678,12 @@ pub fn handle_code_action( res.push(action.into()); } - for fix in world.check_watcher.fixes_for(¶ms.text_document.uri).into_iter().flatten() { - let fix_range = fix.location.range.conv_with(&line_index); + for fix in world.check_fixes.get(&file_id).into_iter().flatten() { + let fix_range = fix.range.conv_with(&line_index); if fix_range.intersection(&range).is_none() { continue; } - - let edit = { - let edits = vec![TextEdit::new(fix.location.range, fix.replacement.clone())]; - let mut edit_map = std::collections::HashMap::new(); - edit_map.insert(fix.location.uri.clone(), edits); - WorkspaceEdit::new(edit_map) - }; - - let action = CodeAction { - title: fix.title.clone(), - kind: Some("quickfix".to_string()), - diagnostics: Some(fix.diagnostics.clone()), - edit: Some(edit), - command: None, - is_preferred: None, - }; - res.push(action.into()); + res.push(fix.action.clone()); } for assist in world.analysis().assists(FileRange { file_id, range })?.into_iter() { @@ -875,14 +861,10 @@ pub fn handle_document_highlight( )) } -pub fn publish_diagnostics( - world: &WorldSnapshot, - file_id: FileId, -) -> Result { +pub fn publish_diagnostics(world: &WorldSnapshot, file_id: FileId) -> Result { let _p = profile("publish_diagnostics"); - let uri = world.file_id_to_uri(file_id)?; let line_index = world.analysis().file_line_index(file_id)?; - let mut diagnostics: Vec = world + let diagnostics: Vec = world .analysis() .diagnostics(file_id)? .into_iter() @@ -896,10 +878,7 @@ pub fn publish_diagnostics( tags: None, }) .collect(); - if let Some(check_diags) = world.check_watcher.diagnostics_for(&uri) { - diagnostics.extend(check_diags.iter().cloned()); - } - Ok(req::PublishDiagnosticsParams { uri, diagnostics, version: None }) + Ok(DiagnosticTask::SetNative(file_id, diagnostics)) } pub fn publish_decorations( diff --git a/crates/ra_lsp_server/src/world.rs b/crates/ra_lsp_server/src/world.rs index 3059ef9ec..1ee02b47c 100644 --- a/crates/ra_lsp_server/src/world.rs +++ b/crates/ra_lsp_server/src/world.rs @@ -12,9 +12,7 @@ use crossbeam_channel::{unbounded, Receiver}; use lsp_server::ErrorCode; use lsp_types::Url; use parking_lot::RwLock; -use ra_cargo_watch::{ - url_from_path_with_drive_lowercasing, CheckOptions, CheckState, CheckWatcher, -}; +use ra_cargo_watch::{url_from_path_with_drive_lowercasing, CheckOptions, CheckWatcher}; use ra_ide::{ Analysis, AnalysisChange, AnalysisHost, CrateGraph, FeatureFlags, FileId, LibraryData, SourceRootId, @@ -25,6 +23,7 @@ use ra_vfs_glob::{Glob, RustPackageFilterBuilder}; use relative_path::RelativePathBuf; use crate::{ + diagnostics::{CheckFixes, DiagnosticCollection}, main_loop::pending_requests::{CompletedRequest, LatestRequests}, LspError, Result, }; @@ -55,6 +54,7 @@ pub struct WorldState { pub task_receiver: Receiver, pub latest_requests: Arc>, pub check_watcher: CheckWatcher, + pub diagnostics: DiagnosticCollection, } /// An immutable snapshot of the world's state at a point in time. @@ -63,7 +63,7 @@ pub struct WorldSnapshot { pub workspaces: Arc>, pub analysis: Analysis, pub latest_requests: Arc>, - pub check_watcher: CheckState, + pub check_fixes: CheckFixes, vfs: Arc>, } @@ -159,6 +159,7 @@ impl WorldState { task_receiver, latest_requests: Default::default(), check_watcher, + diagnostics: Default::default(), } } @@ -220,7 +221,7 @@ impl WorldState { analysis: self.analysis_host.analysis(), vfs: Arc::clone(&self.vfs), latest_requests: Arc::clone(&self.latest_requests), - check_watcher: (*self.check_watcher.state).clone(), + check_fixes: Arc::clone(&self.diagnostics.check_fixes), } } -- cgit v1.2.3 From cde20bf8f0a354dd1471bbb8c6e5d09cc79675d5 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Mon, 3 Feb 2020 12:18:06 +0100 Subject: Remove stray todo --- crates/ra_lsp_server/src/main_loop/handlers.rs | 1 - 1 file changed, 1 deletion(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs index 839bca342..282f6e8fc 100644 --- a/crates/ra_lsp_server/src/main_loop/handlers.rs +++ b/crates/ra_lsp_server/src/main_loop/handlers.rs @@ -657,7 +657,6 @@ pub fn handle_code_action( .filter(|(diag_range, _fix)| diag_range.intersection(&range).is_some()) .map(|(_range, fix)| fix); - // TODO: When done, we won't need this, only the one pulling from world.diagnostics for source_edit in fixes_from_diagnostics { let title = source_edit.label.clone(); let edit = source_edit.try_conv_with(&world)?; -- cgit v1.2.3 From 5db7c8642beb1cd4c09359c3f3266d67557a30f9 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 7 Feb 2020 12:30:29 +0100 Subject: Don't crash when recieving unkown file for cargo diagnostic. --- crates/ra_lsp_server/src/main_loop.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index 12961ba37..fcae37bb7 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -635,12 +635,16 @@ fn on_check_task( CheckTask::AddDiagnostic { url, diagnostic, fixes } => { let path = url.to_file_path().map_err(|()| format!("invalid uri: {}", url))?; - let file_id = world_state - .vfs - .read() - .path2file(&path) - .map(|it| FileId(it.0)) - .ok_or_else(|| format!("unknown file: {}", path.to_string_lossy()))?; + let file_id = match world_state.vfs.read().path2file(&path) { + Some(file) => FileId(file.0), + None => { + log::error!( + "File with cargo diagnostic not found in VFS: {}", + path.to_string_lossy() + ); + return Ok(()); + } + }; task_sender .send(Task::Diagnostic(DiagnosticTask::AddCheck(file_id, diagnostic, fixes)))?; -- cgit v1.2.3 From 137a878461662b7bf7f1acf3855b166c1c19fc2f Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 7 Feb 2020 12:35:36 +0100 Subject: to_string_lossy() -> display() --- crates/ra_lsp_server/src/main_loop.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop.rs b/crates/ra_lsp_server/src/main_loop.rs index fcae37bb7..ceff82fda 100644 --- a/crates/ra_lsp_server/src/main_loop.rs +++ b/crates/ra_lsp_server/src/main_loop.rs @@ -638,10 +638,7 @@ fn on_check_task( let file_id = match world_state.vfs.read().path2file(&path) { Some(file) => FileId(file.0), None => { - log::error!( - "File with cargo diagnostic not found in VFS: {}", - path.to_string_lossy() - ); + log::error!("File with cargo diagnostic not found in VFS: {}", path.display()); return Ok(()); } }; -- cgit v1.2.3 From 9769c5140c9c406a4cc880e698593a6c4bcc6826 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 9 Feb 2020 15:32:53 +0100 Subject: Simplify Assists interface Instead of building a physical tree structure, just "tag" related assists with the same group --- crates/ra_lsp_server/src/main_loop/handlers.rs | 76 ++++++++++++++++---------- 1 file changed, 48 insertions(+), 28 deletions(-) (limited to 'crates/ra_lsp_server') diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs index 282f6e8fc..65e8bc856 100644 --- a/crates/ra_lsp_server/src/main_loop/handlers.rs +++ b/crates/ra_lsp_server/src/main_loop/handlers.rs @@ -2,20 +2,21 @@ //! The majority of requests are fulfilled by calling into the `ra_ide` crate. use std::{ + collections::hash_map::Entry, fmt::Write as _, io::Write as _, process::{self, Stdio}, }; -use either::Either; use lsp_server::ErrorCode; use lsp_types::{ CallHierarchyIncomingCall, CallHierarchyIncomingCallsParams, CallHierarchyItem, CallHierarchyOutgoingCall, CallHierarchyOutgoingCallsParams, CallHierarchyPrepareParams, - CodeAction, CodeActionResponse, CodeLens, Command, CompletionItem, Diagnostic, - DocumentFormattingParams, DocumentHighlight, DocumentSymbol, FoldingRange, FoldingRangeParams, - Hover, HoverContents, Location, MarkupContent, MarkupKind, Position, PrepareRenameResponse, - Range, RenameParams, SymbolInformation, TextDocumentIdentifier, TextEdit, WorkspaceEdit, + CodeAction, CodeActionOrCommand, CodeActionResponse, CodeLens, Command, CompletionItem, + Diagnostic, DocumentFormattingParams, DocumentHighlight, DocumentSymbol, FoldingRange, + FoldingRangeParams, Hover, HoverContents, Location, MarkupContent, MarkupKind, Position, + PrepareRenameResponse, Range, RenameParams, SymbolInformation, TextDocumentIdentifier, + TextEdit, WorkspaceEdit, }; use ra_ide::{ AssistId, FileId, FilePosition, FileRange, Query, RangeInfo, Runnable, RunnableKind, @@ -685,34 +686,53 @@ pub fn handle_code_action( res.push(fix.action.clone()); } + let mut groups = FxHashMap::default(); for assist in world.analysis().assists(FileRange { file_id, range })?.into_iter() { - let title = assist.label.clone(); + let arg = to_value(assist.source_change.try_conv_with(&world)?)?; + + let (command, title, arg) = match assist.group_label { + None => ("rust-analyzer.applySourceChange", assist.label.clone(), arg), + + // Group all assists with the same `group_label` into a single CodeAction. + Some(group_label) => { + match groups.entry(group_label.clone()) { + Entry::Occupied(entry) => { + let idx: usize = *entry.get(); + match &mut res[idx] { + CodeActionOrCommand::CodeAction(CodeAction { + command: Some(Command { arguments: Some(arguments), .. }), + .. + }) => match arguments.as_mut_slice() { + [serde_json::Value::Array(arguments)] => arguments.push(arg), + _ => panic!("invalid group"), + }, + _ => panic!("invalid group"), + } + continue; + } + Entry::Vacant(entry) => { + entry.insert(res.len()); + } + } + ("rust-analyzer.selectAndApplySourceChange", group_label, to_value(vec![arg])?) + } + }; - let command = match assist.change_data { - Either::Left(change) => Command { - title, - command: "rust-analyzer.applySourceChange".to_string(), - arguments: Some(vec![to_value(change.try_conv_with(&world)?)?]), - }, - Either::Right(changes) => Command { - title, - command: "rust-analyzer.selectAndApplySourceChange".to_string(), - arguments: Some(vec![to_value( - changes - .into_iter() - .map(|change| change.try_conv_with(&world)) - .collect::>>()?, - )?]), - }, + let command = Command { + title: assist.label.clone(), + command: command.to_string(), + arguments: Some(vec![arg]), + }; + + let kind = match assist.id { + AssistId("introduce_variable") => Some("refactor.extract.variable".to_string()), + AssistId("add_custom_impl") => Some("refactor.rewrite.add_custom_impl".to_string()), + _ => None, }; let action = CodeAction { - title: command.title.clone(), - kind: match assist.id { - AssistId("introduce_variable") => Some("refactor.extract.variable".to_string()), - AssistId("add_custom_impl") => Some("refactor.rewrite.add_custom_impl".to_string()), - _ => None, - }, + title, + kind, diagnostics: None, edit: None, command: Some(command), -- cgit v1.2.3