From 0341a7ed3cef0d3c165779f6684387cc3408475a Mon Sep 17 00:00:00 2001 From: Joshua Batty Date: Tue, 1 Aug 2023 12:21:58 +1000 Subject: [PATCH] refactor did_* lsp notifications to log errors in server module (#4894) ## Description This PR just bubbles up any errors to be logged in the server module rather that in the `did_change` `did_open` or `did_save` methods. Also made the resync temp folder with workspace logic it's own method on `sync`. --- sway-lsp/src/core/sync.rs | 15 +++++ sway-lsp/src/handlers/notification.rs | 84 +++++++++------------------ sway-lsp/src/server.rs | 14 +++-- sway-lsp/src/server_state.rs | 63 +++++++++----------- 4 files changed, 80 insertions(+), 96 deletions(-) diff --git a/sway-lsp/src/core/sync.rs b/sway-lsp/src/core/sync.rs index 099bc395754..0ca8a032bb8 100644 --- a/sway-lsp/src/core/sync.rs +++ b/sway-lsp/src/core/sync.rs @@ -45,6 +45,21 @@ impl SyncWorkspace { } } + /// Overwrite the contents of the tmp/folder with everything in + /// the current workspace. + pub fn resync(&self) -> Result<(), LanguageServerError> { + self.clone_manifest_dir_to_temp()?; + if let Some(manifest) = self + .manifest_path() + .and_then(|manifest_path| PackageManifestFile::from_dir(&manifest_path).ok()) + { + if let Some(temp_manifest_path) = &self.temp_manifest_path() { + edit_manifest_dependency_paths(&manifest, temp_manifest_path) + } + } + Ok(()) + } + /// Clean up the temp directory that was created once the /// server closes down. pub(crate) fn remove_temp_dir(&self) { diff --git a/sway-lsp/src/handlers/notification.rs b/sway-lsp/src/handlers/notification.rs index bfe2ee2f494..e386d9a61b5 100644 --- a/sway-lsp/src/handlers/notification.rs +++ b/sway-lsp/src/handlers/notification.rs @@ -1,8 +1,7 @@ //! This module is responsible for implementing handlers for Language Server //! Protocol. This module specifically handles notification messages sent by the Client. -use crate::{core::sync, server_state::ServerState}; -use forc_pkg::PackageManifestFile; +use crate::{error::LanguageServerError, server_state::ServerState}; use lsp_types::{ DidChangeTextDocumentParams, DidChangeWatchedFilesParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams, FileChangeType, @@ -11,77 +10,46 @@ use lsp_types::{ pub(crate) async fn handle_did_open_text_document( state: &ServerState, params: DidOpenTextDocumentParams, -) { - match state +) -> Result<(), LanguageServerError> { + let (uri, session) = state .sessions - .uri_and_session_from_workspace(¶ms.text_document.uri) - { - Ok((uri, session)) => { - session.handle_open_file(&uri); - state - .parse_project(uri, params.text_document.uri, session.clone()) - .await; - } - Err(err) => tracing::error!("{}", err.to_string()), - } + .uri_and_session_from_workspace(¶ms.text_document.uri)?; + session.handle_open_file(&uri); + state + .parse_project(uri, params.text_document.uri, session.clone()) + .await; + Ok(()) } pub(crate) async fn handle_did_change_text_document( state: &ServerState, params: DidChangeTextDocumentParams, -) { - match state +) -> Result<(), LanguageServerError> { + let (uri, session) = state .sessions - .uri_and_session_from_workspace(¶ms.text_document.uri) - { - Ok((uri, session)) => { - // update this file with the new changes and write to disk - match session.write_changes_to_file(&uri, params.content_changes) { - Ok(_) => { - state - .parse_project(uri, params.text_document.uri.clone(), session) - .await; - } - Err(err) => tracing::error!("{}", err.to_string()), - } - } - Err(err) => tracing::error!("{}", err.to_string()), - } + .uri_and_session_from_workspace(¶ms.text_document.uri)?; + session.write_changes_to_file(&uri, params.content_changes)?; + state + .parse_project(uri, params.text_document.uri, session.clone()) + .await; + Ok(()) } pub(crate) async fn handle_did_save_text_document( state: &ServerState, params: DidSaveTextDocumentParams, -) { - match state +) -> Result<(), LanguageServerError> { + let (uri, session) = state .sessions - .uri_and_session_from_workspace(¶ms.text_document.uri) - { - Ok((uri, session)) => { - // overwrite the contents of the tmp/folder with everything in - // the current workspace. (resync) - if let Err(err) = session.sync.clone_manifest_dir_to_temp() { - tracing::error!("{}", err.to_string().as_str()); - } - - let _ = session - .sync - .manifest_path() - .and_then(|manifest_path| PackageManifestFile::from_dir(&manifest_path).ok()) - .map(|manifest| { - if let Some(temp_manifest_path) = &session.sync.temp_manifest_path() { - sync::edit_manifest_dependency_paths(&manifest, temp_manifest_path) - } - }); - state - .parse_project(uri, params.text_document.uri, session) - .await; - } - Err(err) => tracing::error!("{}", err.to_string()), - } + .uri_and_session_from_workspace(¶ms.text_document.uri)?; + session.sync.resync()?; + state + .parse_project(uri, params.text_document.uri, session.clone()) + .await; + Ok(()) } -pub(crate) async fn handle_did_change_watched_files( +pub(crate) fn handle_did_change_watched_files( state: &ServerState, params: DidChangeWatchedFilesParams, ) { diff --git a/sway-lsp/src/server.rs b/sway-lsp/src/server.rs index a2e131f761a..15aa5b635dd 100644 --- a/sway-lsp/src/server.rs +++ b/sway-lsp/src/server.rs @@ -33,19 +33,25 @@ impl LanguageServer for ServerState { } async fn did_open(&self, params: DidOpenTextDocumentParams) { - notification::handle_did_open_text_document(self, params).await; + if let Err(err) = notification::handle_did_open_text_document(self, params).await { + tracing::error!("{}", err.to_string()); + } } async fn did_change(&self, params: DidChangeTextDocumentParams) { - notification::handle_did_change_text_document(self, params).await; + if let Err(err) = notification::handle_did_change_text_document(self, params).await { + tracing::error!("{}", err.to_string()); + } } async fn did_save(&self, params: DidSaveTextDocumentParams) { - notification::handle_did_save_text_document(self, params).await; + if let Err(err) = notification::handle_did_save_text_document(self, params).await { + tracing::error!("{}", err.to_string()); + } } async fn did_change_watched_files(&self, params: DidChangeWatchedFilesParams) { - notification::handle_did_change_watched_files(self, params).await; + notification::handle_did_change_watched_files(self, params); } async fn hover(&self, params: HoverParams) -> Result> { diff --git a/sway-lsp/src/server_state.rs b/sway-lsp/src/server_state.rs index c939b03a983..6c77ce48dae 100644 --- a/sway-lsp/src/server_state.rs +++ b/sway-lsp/src/server_state.rs @@ -9,7 +9,7 @@ use crate::{ }; use dashmap::DashMap; use forc_pkg::PackageManifestFile; -use lsp_types::Url; +use lsp_types::{Diagnostic, Url}; use parking_lot::RwLock; use std::{path::PathBuf, sync::Arc}; use tokio::task; @@ -45,47 +45,42 @@ impl ServerState { Ok(()) } - async fn publish_diagnostics(&self, uri: &Url, workspace_uri: &Url, session: Arc) { - let diagnostics_res = { - let mut diagnostics_to_publish = vec![]; - let config = &self.config.read(); - let engines = session.engines.read(); - let tokens = session.token_map().tokens_for_file(engines.se(), uri); - match config.debug.show_collected_tokens_as_warnings { - // If collected_tokens_as_warnings is Parsed or Typed, - // take over the normal error and warning display behavior - // and instead show the either the parsed or typed tokens as warnings. - // This is useful for debugging the lsp parser. - Warnings::Parsed => { - diagnostics_to_publish = debug::generate_warnings_for_parsed_tokens(tokens) - } - Warnings::Typed => { - diagnostics_to_publish = debug::generate_warnings_for_typed_tokens(tokens) + pub(crate) fn diagnostics(&self, uri: &Url, session: Arc) -> Vec { + let mut diagnostics_to_publish = vec![]; + let config = &self.config.read(); + let engines = session.engines.read(); + let tokens = session.token_map().tokens_for_file(engines.se(), uri); + match config.debug.show_collected_tokens_as_warnings { + // If collected_tokens_as_warnings is Parsed or Typed, + // take over the normal error and warning display behavior + // and instead show the either the parsed or typed tokens as warnings. + // This is useful for debugging the lsp parser. + Warnings::Parsed => { + diagnostics_to_publish = debug::generate_warnings_for_parsed_tokens(tokens) + } + Warnings::Typed => { + diagnostics_to_publish = debug::generate_warnings_for_typed_tokens(tokens) + } + Warnings::Default => { + let diagnostics = session.wait_for_parsing(); + if config.diagnostic.show_warnings { + diagnostics_to_publish.extend(diagnostics.warnings); } - Warnings::Default => { - let diagnostics = session.wait_for_parsing(); - if config.diagnostic.show_warnings { - diagnostics_to_publish.extend(diagnostics.warnings); - } - if config.diagnostic.show_errors { - diagnostics_to_publish.extend(diagnostics.errors); - } + if config.diagnostic.show_errors { + diagnostics_to_publish.extend(diagnostics.errors); } } - diagnostics_to_publish - }; - - // Note: Even if the computed diagnostics vec is empty, we still have to push the empty Vec - // in order to clear former diagnostics. Newly pushed diagnostics always replace previously pushed diagnostics. - self.client - .publish_diagnostics(workspace_uri.clone(), diagnostics_res, None) - .await; + } + diagnostics_to_publish } pub(crate) async fn parse_project(&self, uri: Url, workspace_uri: Url, session: Arc) { let should_publish = run_blocking_parse_project(uri.clone(), session.clone()).await; if should_publish { - self.publish_diagnostics(&uri, &workspace_uri, session) + // Note: Even if the computed diagnostics vec is empty, we still have to push the empty Vec + // in order to clear former diagnostics. Newly pushed diagnostics always replace previously pushed diagnostics. + self.client + .publish_diagnostics(workspace_uri.clone(), self.diagnostics(&uri, session), None) .await; } }