From e12c144c1b928250df3425d896687784744307fe Mon Sep 17 00:00:00 2001 From: Grant Lemons Date: Thu, 2 Jan 2025 16:06:57 -0600 Subject: [PATCH 1/3] refactor: update error handling in harper_ls --- harper-ls/src/backend.rs | 173 ++++++++++++++++++++------------- harper-ls/src/config.rs | 25 ++--- harper-ls/src/dictionary_io.rs | 31 ++++-- 3 files changed, 134 insertions(+), 95 deletions(-) diff --git a/harper-ls/src/backend.rs b/harper-ls/src/backend.rs index 73ceedeb..444bd827 100644 --- a/harper-ls/src/backend.rs +++ b/harper-ls/src/backend.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::path::{Component, PathBuf}; use std::sync::Arc; -use anyhow::anyhow; +use anyhow::{anyhow, Context}; use harper_comments::CommentParser; use harper_core::linting::{LintGroup, Linter}; use harper_core::parsers::{CollapseIdentifiers, IsolateEnglish, Markdown, Parser, PlainEnglish}; @@ -13,7 +13,6 @@ use harper_core::{ use harper_html::HtmlParser; use serde_json::Value; use tokio::sync::{Mutex, RwLock}; -use tower_lsp::jsonrpc::Result; use tower_lsp::lsp_types::notification::PublishDiagnostics; use tower_lsp::lsp_types::{ CodeActionOrCommand, CodeActionParams, CodeActionProviderCapability, CodeActionResponse, @@ -51,58 +50,68 @@ impl Backend { /// Rewrites a path to a filename using the same conventions as /// [Neovim's undo-files](https://neovim.io/doc/user/options.html#'undodir'). - fn file_dict_name(url: &Url) -> Option { + fn file_dict_name(url: &Url) -> anyhow::Result { let mut rewritten = String::new(); // We assume all URLs are local files and have a base - for seg in url.to_file_path().ok()?.components() { + for seg in url + .to_file_path() + .map_err(|_| anyhow!("Unable to convert URL to file path."))? + .components() + { if !matches!(seg, Component::RootDir) { rewritten.push_str(&seg.as_os_str().to_string_lossy()); rewritten.push('%'); } } - Some(rewritten.into()) + Ok(rewritten.into()) } /// Get the location of the file's specific dictionary - async fn get_file_dict_path(&self, url: &Url) -> Option { + async fn get_file_dict_path(&self, url: &Url) -> anyhow::Result { let config = self.config.read().await; - Some(config.file_dict_path.join(Self::file_dict_name(url)?)) + Ok(config.file_dict_path.join(Self::file_dict_name(url)?)) } - /// Load a speCific file's dictionary - async fn load_file_dictionary(&self, url: &Url) -> Option { - match load_dict(self.get_file_dict_path(url).await?).await { - Ok(dict) => Some(dict), - Err(_err) => Some(FullDictionary::new()), - } + /// Load a specific file's dictionary + async fn load_file_dictionary(&self, url: &Url) -> anyhow::Result { + let path = self + .get_file_dict_path(url) + .await + .context("Unable to get file path.")?; + + load_dict(path) + .await + .map_err(|err| info!("{err}")) + .or(Ok(FullDictionary::new())) } async fn save_file_dictionary(&self, url: &Url, dict: impl Dictionary) -> anyhow::Result<()> { - Ok(save_dict( + save_dict( self.get_file_dict_path(url) .await - .ok_or(anyhow!("Could not compute dictionary path."))?, + .context("Unable to get file path.")?, dict, ) - .await?) + .await + .context("Unable to save dictionary to path.") } async fn load_user_dictionary(&self) -> FullDictionary { let config = self.config.read().await; - match load_dict(&config.user_dict_path).await { - Ok(dict) => dict, - Err(_err) => FullDictionary::new(), - } + load_dict(&config.user_dict_path) + .await + .map_err(|err| info!("{err}")) + .unwrap_or(FullDictionary::new()) } async fn save_user_dictionary(&self, dict: impl Dictionary) -> anyhow::Result<()> { let config = self.config.read().await; - Ok(save_dict(&config.user_dict_path, dict).await?) + save_dict(&config.user_dict_path, dict).await } async fn generate_global_dictionary(&self) -> anyhow::Result { @@ -119,12 +128,11 @@ impl Backend { self.load_file_dictionary(url) ); - let Some(file_dictionary) = file_dictionary else { - return Err(anyhow!("Unable to compute dictionary path.")); - }; - - let mut global_dictionary = global_dictionary?; - global_dictionary.add_dictionary(Arc::new(file_dictionary)); + let mut global_dictionary = + global_dictionary.context("Unable to load global dictionary.")?; + global_dictionary.add_dictionary(Arc::new( + file_dictionary.context("Unable to load file dictionary.")?, + )); Ok(global_dictionary) } @@ -134,18 +142,12 @@ impl Backend { url: &Url, language_id: Option<&str>, ) -> anyhow::Result<()> { - let content = match tokio::fs::read_to_string( + let content = tokio::fs::read_to_string( url.to_file_path() - .map_err(|_| anyhow::format_err!("Could not extract file path."))?, + .map_err(|_| anyhow!("Unable to convert URL to file path."))?, ) .await - { - Ok(content) => content, - Err(err) => { - error!("Error updating document from file: {}", err); - return Ok(()); - } - }; + .with_context(|| format!("Unable to read from file {:?}", url))?; self.update_document(url, &content, language_id).await } @@ -161,7 +163,11 @@ impl Backend { let mut doc_lock = self.doc_state.lock().await; let config_lock = self.config.read().await; - let dict = Arc::new(self.generate_file_dictionary(url).await?); + let dict = Arc::new( + self.generate_file_dictionary(url) + .await + .context("Unable to generate file dictionary.")?, + ); let doc_state = doc_lock.entry(url.clone()).or_insert(DocumentState { linter: LintGroup::new(config_lock.lint_config, dict.clone()), @@ -190,7 +196,10 @@ impl Backend { if doc_state.ident_dict != new_dict { doc_state.ident_dict = new_dict.clone(); - let mut merged = self.generate_file_dictionary(url).await?; + let mut merged = self + .generate_file_dictionary(url) + .await + .context("Unable to generate file dictionary.")?; merged.add_dictionary(new_dict); let merged = Arc::new(merged); @@ -236,7 +245,7 @@ impl Backend { &self, url: &Url, range: Range, - ) -> Result> { + ) -> tower_lsp::jsonrpc::Result> { let (config, mut doc_states) = tokio::join!(self.config.read(), self.doc_state.lock()); let Some(doc_state) = doc_states.get_mut(url) else { return Ok(Vec::new()); @@ -307,15 +316,7 @@ impl Backend { /// Update the configuration of the server and publish document updates that /// match it. async fn update_config_from_obj(&self, json_obj: Value) { - let new_config = match Config::from_lsp_config(json_obj) { - Ok(new_config) => new_config, - Err(err) => { - error!("Unable to change config: {}", err); - return; - } - }; - - { + if let Ok(new_config) = Config::from_lsp_config(json_obj).map_err(|err| error!("{err}")) { let mut config = self.config.write().await; *config = new_config; } @@ -339,7 +340,10 @@ impl Backend { #[tower_lsp::async_trait] impl LanguageServer for Backend { - async fn initialize(&self, _: InitializeParams) -> Result { + async fn initialize( + &self, + _: InitializeParams, + ) -> tower_lsp::jsonrpc::Result { Ok(InitializeResult { server_info: None, capabilities: ServerCapabilities { @@ -375,21 +379,23 @@ impl LanguageServer for Backend { } async fn did_save(&self, params: DidSaveTextDocumentParams) { - let _ = self - .update_document_from_file(¶ms.text_document.uri, None) - .await; + self.update_document_from_file(¶ms.text_document.uri, None) + .await + .map_err(|err| error!("{err}")) + .err(); self.publish_diagnostics(¶ms.text_document.uri).await; } async fn did_open(&self, params: DidOpenTextDocumentParams) { - let _ = self - .update_document( - ¶ms.text_document.uri, - ¶ms.text_document.text, - Some(¶ms.text_document.language_id), - ) - .await; + self.update_document( + ¶ms.text_document.uri, + ¶ms.text_document.text, + Some(¶ms.text_document.language_id), + ) + .await + .map_err(|err| error!("{err}")) + .err(); self.publish_diagnostics(¶ms.text_document.uri).await; } @@ -407,7 +413,10 @@ impl LanguageServer for Backend { async fn did_close(&self, _params: DidCloseTextDocumentParams) {} - async fn execute_command(&self, params: ExecuteCommandParams) -> Result> { + async fn execute_command( + &self, + params: ExecuteCommandParams, + ) -> tower_lsp::jsonrpc::Result> { let mut string_args = params .arguments .into_iter() @@ -431,8 +440,14 @@ impl LanguageServer for Backend { let mut dict = self.load_user_dictionary().await; dict.append_word(word, WordMetadata::default()); - let _ = self.save_user_dictionary(dict).await; - let _ = self.update_document_from_file(&file_url, None).await; + self.save_user_dictionary(dict) + .await + .map_err(|err| error!("{err}")) + .err(); + self.update_document_from_file(&file_url, None) + .await + .map_err(|err| error!("{err}")) + .err(); self.publish_diagnostics(&file_url).await; } "HarperAddToFileDict" => { @@ -444,14 +459,26 @@ impl LanguageServer for Backend { let file_url = second.parse().unwrap(); - let Some(mut dict) = self.load_file_dictionary(&file_url).await else { - error!("Unable resolve dictionary path: {file_url}"); - return Ok(None); + let mut dict = match self + .load_file_dictionary(&file_url) + .await + .map_err(|err| error!("{err}")) + { + Ok(dict) => dict, + Err(_) => { + return Ok(None); + } }; dict.append_word(word, WordMetadata::default()); - let _ = self.save_file_dictionary(&file_url, dict).await; - let _ = self.update_document_from_file(&file_url, None).await; + self.save_file_dictionary(&file_url, dict) + .await + .map_err(|err| error!("{err}")) + .err(); + self.update_document_from_file(&file_url, None) + .await + .map_err(|err| error!("{err}")) + .err(); self.publish_diagnostics(&file_url).await; } "HarperOpen" => match open::that(&first) { @@ -490,12 +517,18 @@ impl LanguageServer for Backend { }; for url in urls { - let _ = self.update_document_from_file(&url, None).await; + self.update_document_from_file(&url, None) + .await + .map_err(|err| error!("{err}")) + .err(); self.publish_diagnostics(&url).await; } } - async fn code_action(&self, params: CodeActionParams) -> Result> { + async fn code_action( + &self, + params: CodeActionParams, + ) -> tower_lsp::jsonrpc::Result> { let actions = self .generate_code_actions(¶ms.text_document.uri, params.range) .await?; @@ -503,7 +536,7 @@ impl LanguageServer for Backend { Ok(Some(actions)) } - async fn shutdown(&self) -> Result<()> { + async fn shutdown(&self) -> tower_lsp::jsonrpc::Result<()> { let doc_states = self.doc_state.lock().await; // Clears the diagnostics for open buffers. diff --git a/harper-ls/src/config.rs b/harper-ls/src/config.rs index e28a7d2d..e07c3e0f 100644 --- a/harper-ls/src/config.rs +++ b/harper-ls/src/config.rs @@ -1,5 +1,6 @@ use std::path::PathBuf; +use anyhow::{bail, Result}; use dirs::{config_dir, data_local_dir}; use harper_core::linting::LintGroupConfig; use resolve_path::PathResolveExt; @@ -42,18 +43,16 @@ pub struct CodeActionConfig { } impl CodeActionConfig { - pub fn from_lsp_config(value: Value) -> anyhow::Result { + pub fn from_lsp_config(value: Value) -> Result { let mut base = CodeActionConfig::default(); let Value::Object(value) = value else { - return Err(anyhow::format_err!( - "The code action configuration must be an object." - )); + bail!("The code action configuration must be an object."); }; if let Some(force_stable_val) = value.get("forceStable") { let Value::Bool(force_stable) = force_stable_val else { - return Err(anyhow::format_err!("forceStable must be a boolean value.")); + bail!("forceStable must be a boolean value."); }; base.force_stable = *force_stable; }; @@ -73,24 +72,22 @@ pub struct Config { } impl Config { - pub fn from_lsp_config(value: Value) -> anyhow::Result { + pub fn from_lsp_config(value: Value) -> Result { let mut base = Config::default(); let Value::Object(value) = value else { - return Err(anyhow::format_err!("Settings must be an object.")); + bail!("Settings must be an object."); }; let Some(Value::Object(value)) = value.get("harper-ls") else { - return Err(anyhow::format_err!( - "Settings must contain a \"harper-ls\" key." - )); + bail!("Settings must contain a \"harper-ls\" key."); }; if let Some(v) = value.get("userDictPath") { if let Value::String(path) = v { base.user_dict_path = path.try_resolve()?.to_path_buf(); } else { - return Err(anyhow::format_err!("userDict path must be a string.")); + bail!("userDict path must be a string."); } } @@ -98,7 +95,7 @@ impl Config { if let Value::String(path) = v { base.file_dict_path = path.try_resolve()?.to_path_buf(); } else { - return Err(anyhow::format_err!("fileDict path must be a string.")); + bail!("fileDict path must be a string."); } } @@ -118,9 +115,7 @@ impl Config { if let Value::Bool(v) = v { base.isolate_english = *v; } else { - return Err(anyhow::format_err!( - "isolateEnglish path must be a boolean." - )); + bail!("isolateEnglish path must be a boolean."); } } diff --git a/harper-ls/src/dictionary_io.rs b/harper-ls/src/dictionary_io.rs index 0884a911..4e5b548a 100644 --- a/harper-ls/src/dictionary_io.rs +++ b/harper-ls/src/dictionary_io.rs @@ -1,27 +1,34 @@ use std::path::Path; +use anyhow::{Context, Result}; use harper_core::{Dictionary, FullDictionary, WordMetadata}; use tokio::fs::{self, File}; -use tokio::io::{self, AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, BufReader, BufWriter}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, BufReader, BufWriter}; /// Save the contents of a dictionary to a file. /// Ensures that the path to the destination exists. -pub async fn save_dict(path: impl AsRef, dict: impl Dictionary) -> io::Result<()> { +pub async fn save_dict(path: impl AsRef, dict: impl Dictionary) -> Result<()> { if let Some(parent) = path.as_ref().parent() { - fs::create_dir_all(parent).await?; + fs::create_dir_all(parent) + .await + .context("Unable to create parent directories")?; } - let file = File::create(path.as_ref()).await?; + let file = File::create(path.as_ref()) + .await + .with_context(|| format!("Unable to create file {:?}", path.as_ref()))?; let mut write = BufWriter::new(file); - write_word_list(dict, &mut write).await?; + write_word_list(dict, &mut write) + .await + .with_context(|| format!("Unable to write to file {:?}", path.as_ref()))?; write.flush().await?; Ok(()) } -async fn write_word_list(dict: impl Dictionary, mut w: impl AsyncWrite + Unpin) -> io::Result<()> { +async fn write_word_list(dict: impl Dictionary, mut w: impl AsyncWrite + Unpin) -> Result<()> { let mut cur_str = String::new(); for word in dict.words_iter() { @@ -35,16 +42,20 @@ async fn write_word_list(dict: impl Dictionary, mut w: impl AsyncWrite + Unpin) Ok(()) } -pub async fn load_dict(path: impl AsRef) -> io::Result { - let file = File::open(path.as_ref()).await?; +pub async fn load_dict(path: impl AsRef) -> Result { + let file = File::open(path.as_ref()) + .await + .with_context(|| format!("Unable to open file {:?}", path.as_ref()))?; let read = BufReader::new(file); - dict_from_word_list(read).await + dict_from_word_list(read) + .await + .with_context(|| format!("Unable to read from file {:?}", path.as_ref())) } /// This function could definitely be optimized to use less memory. /// Right now it isn't an issue. -async fn dict_from_word_list(mut r: impl AsyncRead + Unpin) -> io::Result { +async fn dict_from_word_list(mut r: impl AsyncRead + Unpin) -> Result { let mut str = String::new(); r.read_to_string(&mut str).await?; From 0b58c4709020d7d5319915c0a111b1d9c0fc6b1d Mon Sep 17 00:00:00 2001 From: Grant Lemons Date: Tue, 14 Jan 2025 12:19:11 -0700 Subject: [PATCH 2/3] refactor: remove anyhow from dictionary_io --- harper-ls/src/backend.rs | 4 +++- harper-ls/src/dictionary_io.rs | 24 ++++++------------------ 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/harper-ls/src/backend.rs b/harper-ls/src/backend.rs index 8b09fd9c..d8b6ece8 100644 --- a/harper-ls/src/backend.rs +++ b/harper-ls/src/backend.rs @@ -111,7 +111,9 @@ impl Backend { async fn save_user_dictionary(&self, dict: impl Dictionary) -> anyhow::Result<()> { let config = self.config.read().await; - save_dict(&config.user_dict_path, dict).await + save_dict(&config.user_dict_path, dict) + .await + .map_err(|err| anyhow!("Unable to save dictionary to file: {err}")) } async fn generate_global_dictionary(&self) -> anyhow::Result { diff --git a/harper-ls/src/dictionary_io.rs b/harper-ls/src/dictionary_io.rs index 4e5b548a..164e484b 100644 --- a/harper-ls/src/dictionary_io.rs +++ b/harper-ls/src/dictionary_io.rs @@ -1,28 +1,20 @@ use std::path::Path; -use anyhow::{Context, Result}; use harper_core::{Dictionary, FullDictionary, WordMetadata}; use tokio::fs::{self, File}; -use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, BufReader, BufWriter}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, BufReader, BufWriter, Result}; /// Save the contents of a dictionary to a file. /// Ensures that the path to the destination exists. pub async fn save_dict(path: impl AsRef, dict: impl Dictionary) -> Result<()> { if let Some(parent) = path.as_ref().parent() { - fs::create_dir_all(parent) - .await - .context("Unable to create parent directories")?; + fs::create_dir_all(parent).await?; } - let file = File::create(path.as_ref()) - .await - .with_context(|| format!("Unable to create file {:?}", path.as_ref()))?; + let file = File::create(path.as_ref()).await?; let mut write = BufWriter::new(file); - write_word_list(dict, &mut write) - .await - .with_context(|| format!("Unable to write to file {:?}", path.as_ref()))?; - + write_word_list(dict, &mut write).await?; write.flush().await?; Ok(()) @@ -43,14 +35,10 @@ async fn write_word_list(dict: impl Dictionary, mut w: impl AsyncWrite + Unpin) } pub async fn load_dict(path: impl AsRef) -> Result { - let file = File::open(path.as_ref()) - .await - .with_context(|| format!("Unable to open file {:?}", path.as_ref()))?; + let file = File::open(path.as_ref()).await?; let read = BufReader::new(file); - dict_from_word_list(read) - .await - .with_context(|| format!("Unable to read from file {:?}", path.as_ref())) + dict_from_word_list(read).await } /// This function could definitely be optimized to use less memory. From 317bb40333665f4260f3adddda8e759e7f17732e Mon Sep 17 00:00:00 2001 From: Grant Lemons Date: Tue, 14 Jan 2025 12:35:05 -0700 Subject: [PATCH 3/3] style: make error messages more gramatically correct --- harper-ls/src/backend.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/harper-ls/src/backend.rs b/harper-ls/src/backend.rs index 7a4b83e9..bc6a779f 100644 --- a/harper-ls/src/backend.rs +++ b/harper-ls/src/backend.rs @@ -85,7 +85,7 @@ impl Backend { let path = self .get_file_dict_path(url) .await - .context("Unable to get file path.")?; + .context("Unable to get the file path.")?; load_dict(path) .await @@ -97,11 +97,11 @@ impl Backend { save_dict( self.get_file_dict_path(url) .await - .context("Unable to get file path.")?, + .context("Unable to get the file path.")?, dict, ) .await - .context("Unable to save dictionary to path.") + .context("Unable to save the dictionary to path.") } async fn load_user_dictionary(&self) -> FullDictionary { @@ -118,7 +118,7 @@ impl Backend { save_dict(&config.user_dict_path, dict) .await - .map_err(|err| anyhow!("Unable to save dictionary to file: {err}")) + .map_err(|err| anyhow!("Unable to save the dictionary to file: {err}")) } async fn generate_global_dictionary(&self) -> Result { @@ -136,9 +136,9 @@ impl Backend { ); let mut global_dictionary = - global_dictionary.context("Unable to load global dictionary.")?; + global_dictionary.context("Unable to load the global dictionary.")?; global_dictionary.add_dictionary(Arc::new( - file_dictionary.context("Unable to load file dictionary.")?, + file_dictionary.context("Unable to load the file dictionary.")?, )); Ok(global_dictionary) @@ -169,7 +169,7 @@ impl Backend { let dict = Arc::new( self.generate_file_dictionary(url) .await - .context("Unable to generate file dictionary.")?, + .context("Unable to generate the file dictionary.")?, ); let doc_state = doc_lock.entry(url.clone()).or_insert(DocumentState {